1385836 - Remove special cases obsoleted by "replacement" becoming a label of the replacement encoding

Status: RESOLVED FIXED

(Core :: Internationalization, enhancement, P3)

Description

[Henri Sivonen (:hsivonen)]

https://github.com/whatwg/encoding/issues/70 removed the special case from the spec. After bug 1372230 lands, we can remove the special case from the code.

Comment 1

[Henri Sivonen (:hsivonen)]

Attachment: Bug 1385836 - Remove special cases obsoleted by "replacement" becoming a label of the replacement encoding.

Review commit: https://reviewboard.mozilla.org/r/173184/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/173184/

Comment 2

[Henri Sivonen (:hsivonen)]

Comment on attachment 8901739 [details]
Bug 1385836 - Remove special cases obsoleted by "replacement" becoming a label of the replacement encoding.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/173184/diff/1-2/

Comment 3

[Masatoshi Kimura [:emk]]

Comment on attachment 8901739 [details]
Bug 1385836 - Remove special cases obsoleted by "replacement" becoming a label of the replacement encoding.

https://reviewboard.mozilla.org/r/173184/#review178560

::: layout/base/nsDocumentViewer.cpp:3395
(Diff revision 2)
> -  // than a canonical name. However, in case where the input is a canonical
> -  // name, "replacement" doesn't survive label resolution. Additionally, the
> -  // empty string means no hint.
>    const Encoding* encoding = nullptr;
>    if (!aForceCharacterSet.IsEmpty()) {
> -    if (aForceCharacterSet.EqualsLiteral("replacement")) {
> +    if (!(encoding = Encoding::ForLabel(aForceCharacterSet))) {

Do we still need the label resolution in a post-57 world?

::: layout/base/nsDocumentViewer.cpp:3473
(Diff revision 2)
> -  // than a canonical name. However, in case where the input is a canonical
> -  // name, "replacement" doesn't survive label resolution. Additionally, the
> -  // empty string means no hint.
>    const Encoding* encoding = nullptr;
>    if (!aHintCharacterSet.IsEmpty()) {
> -    if (aHintCharacterSet.EqualsLiteral("replacement")) {
> +    if (!(encoding = Encoding::ForLabel(aHintCharacterSet))) {

Ditto.

::: netwerk/base/nsUnicharStreamLoader.cpp:181
(Diff revision 2)
>    if (NS_FAILED(rv) || mCharset.IsEmpty()) {
>      // The observer told us nothing useful
>      mCharset.AssignLiteral("UTF-8");
>    }
>  
> -  // Sadly, nsIUnicharStreamLoader is exposed to extensions, so we can't
> +  const Encoding* encoding = Encoding::ForLabel(mCharset);

Ditto.

Comment 4

[Henri Sivonen (:hsivonen)]

Comment on attachment 8901739 [details]
Bug 1385836 - Remove special cases obsoleted by "replacement" becoming a label of the replacement encoding.

https://reviewboard.mozilla.org/r/173184/#review178560

> Do we still need the label resolution in a post-57 world?

This code isn't performance-sensitive, so doing lookup by label instead of name seems acceptable and is more robust against Thunderbird bugs.

> Ditto.

I guess this could be changed to use `const mozilla::Encoding*` instead of a string.

> Ditto.

It appears that calendar code in comm-central passes a raw label through here.

Comment 5

[Henri Sivonen (:hsivonen)]

Comment on attachment 8901739 [details]
Bug 1385836 - Remove special cases obsoleted by "replacement" becoming a label of the replacement encoding.

https://reviewboard.mozilla.org/r/173184/#review178560

> I guess this could be changed to use `const mozilla::Encoding*` instead of a string.

This methods seems to be comm-central-only. There's one in-tree (c-c) C++ caller. Bug 829543 suggests that there are Thunderbird extension callers, too.

Getting rid of this method seems to be out of scope for this bug (in-scope for bug 829543), so I'd prefer to land the patch as-is.

Comment 6

[Henri Sivonen (:hsivonen)]

Comment on attachment 8901739 [details]
Bug 1385836 - Remove special cases obsoleted by "replacement" becoming a label of the replacement encoding.

https://reviewboard.mozilla.org/r/173184/#review178560

> It appears that calendar code in comm-central passes a raw label through here.

As for m-c callers, I'd like to remove the entire class once we remove the old style system, so I think it's not worthwhile to polish this too much.

Comment 7

[Masatoshi Kimura [:emk]]

Comment on attachment 8901739 [details]
Bug 1385836 - Remove special cases obsoleted by "replacement" becoming a label of the replacement encoding.

https://reviewboard.mozilla.org/r/173184/#review178998

OK, go for it.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5804f73c5bd5
Remove special cases obsoleted by "replacement" becoming a label of the replacement encoding. r=emk

Comment 9

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/5804f73c5bd5