Closed Bug 1385836 Opened 3 years ago Closed 2 years ago

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

Categories

(Core :: Internationalization, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: hsivonen, Assigned: hsivonen)

References

Details

Attachments

(1 file)

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.
Priority: -- → P3
Attachment #8901739 - Flags: review?(VYV03354)
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 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 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 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 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.
Attachment #8901739 - Flags: review?(VYV03354) → review+
Pushed by hsivonen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5804f73c5bd5
Remove special cases obsoleted by "replacement" becoming a label of the replacement encoding. r=emk
https://hg.mozilla.org/mozilla-central/rev/5804f73c5bd5
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Assignee: nobody → hsivonen
You need to log in before you can comment on or make changes to this bug.