Runtime: Use Strings resx file for the exception messages within System.Text.Encodings.Web

[ahsonkhan]

See https://github.com/dotnet/corefx/pull/39900#discussion_r309495299

We currently inline the exception message strings when throwing in System.Text.Encodings.Web.
We should be adding these to a Strings.resx file and use SR.X to access them.

Even though we're not currently localizing exception strings on .NET Core, being in the Strings.resx file does mean that a) there aren't two different copies that get out of sync if the message gets tweaked and b) we could localize if desired.

Look at other projects for examples. E.g:
https://github.com/dotnet/corefx/blob/c6f5ceedea28edf01ae7d3e13c02935669dad434/src/System.Text.Json/src/Resources/Strings.resx#L138-L140
https://github.com/dotnet/corefx/blob/47c35fe385c18d5f0a4ceacea381f790db472ba2/src/System.Text.Json/src/System/Text/Json/ThrowHelper.cs#L443-L446
https://github.com/dotnet/corefx/blob/82408cd90f4d4573d502e8df2ca437b35e6a37f7/src/System.Text.Json/src/System/Text/Json/Reader/JsonReaderHelper.Unescaping.cs#L209

The use of the "ThrowHelper" pattern isn't necessary here (applied on case-by-case basis where performance concerns were higher than code readability).

Another example:
https://github.com/dotnet/corefx/blob/82408cd90f4d4573d502e8df2ca437b35e6a37f7/src/System.Private.Xml/src/Resources/Strings.resx#L134-L136
https://github.com/dotnet/corefx/blob/e5d38a54cf21f6427ae4fe99c3841c8067139e75/src/System.Private.Xml/src/System/Xml/Xslt/XslCompiledTransform.cs#L475-L485

cc @GrabYourPitchforks

[stephentoub]

a) there aren't two different copies that get out of sync if the message gets tweaked and b) we could localize if desired.

And maybe most importantly, (c) for size we could choose to have the linker (e.g. when linking a whole app) strip out all of the resx contents; that's harder to do when the string is directly embedded in the code because it's harder to determine with 100% fidelity that the string isn't being used for something functional.

[danmosemsft]

Anyone notice any more resx debt? My assumption had been that there isn't any.

[ahsonkhan]

A naiive regex search for Exception\(".+?"\) within cs files shows quite a few results but most are false positives.
1144 results in 472 files (almost all are in tests, or under #if DEBUG, or single word arg names, and can be ignored).

Some are in Common:
https://github.com/dotnet/corefx/blob/cc5653fcceefce5eac38dc7847b12b371027f78f/src/Common/src/System/IO/RowConfigReader.cs#L53
https://github.com/dotnet/corefx/blob/cc5653fcceefce5eac38dc7847b12b371027f78f/src/Common/src/CoreLib/System/PasteArguments.Windows.cs#L38
https://github.com/dotnet/corefx/blob/cc5653fcceefce5eac38dc7847b12b371027f78f/src/Common/src/CoreLib/System/Diagnostics/Tracing/DiagnosticCounter.cs#L86

https://github.com/dotnet/corefx/blob/cc5653fcceefce5eac38dc7847b12b371027f78f/src/System.Composition.Hosting/src/System/Composition/Hosting/Core/ExportDescriptorRegistryUpdate.cs#L35
https://github.com/dotnet/corefx/blob/cc5653fcceefce5eac38dc7847b12b371027f78f/src/System.Data.SqlClient/src/Interop/SNINativeMethodWrapper.Unix.cs#L13

System.Drawing.Common has some.
https://github.com/dotnet/corefx/blob/cc5653fcceefce5eac38dc7847b12b371027f78f/src/System.Drawing.Common/src/System/Drawing/Font.Unix.cs#L218
https://github.com/dotnet/corefx/blob/cc5653fcceefce5eac38dc7847b12b371027f78f/src/System.Drawing.Common/src/System/Drawing/Printing/PrinterSettings.Unix.cs#L105

And then a few sprinkled in various assemblies. For example:
https://github.com/dotnet/corefx/blob/cc5653fcceefce5eac38dc7847b12b371027f78f/src/System.IO.Packaging/src/System/IO/Packaging/ZipPackage.cs#L356
https://github.com/dotnet/corefx/blob/cc5653fcceefce5eac38dc7847b12b371027f78f/src/System.Private.DataContractSerialization/src/System/Runtime/Serialization/DataContractSerializer.cs#L46
https://github.com/dotnet/corefx/blob/cc5653fcceefce5eac38dc7847b12b371027f78f/src/System.Private.Xml/src/System/Xml/Xsl/Xslt/XsltLoader.cs#L1318

Note: This is not an exhaustive list but it looks like ~25 or so real instances.

Question: Should single word messages like Argument...Exception("foo") be changed too?

[danmosemsft]

Ok, so a little debt for if we decide to support localization (let's not discuss that in this issue as it's already discussed elsewhere and there is no plan at this point..)

[danmosemsft]

Question: Should single word messages like Argument...Exception("foo") be changed too

In this case the exception provides the message. Ideally would use nameof though.

[Marusyk]

Can I go ahead with other projects, or it should be separate issue?

[ahsonkhan]

Can I go ahead with other projects, or it should be separate issue?

Feel free to fix other instances (you don't have to wait to file a separate issue). However, I would fix that in a separate PR.