Closed
Bug 1385836
Opened 4 years ago
Closed 4 years ago
Remove special cases obsoleted by "replacement" becoming a label of the replacement encoding
Categories
(Core :: Internationalization, enhancement, P3)
Core
Internationalization
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.
Updated•4 years ago
|
Priority: -- → P3
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•4 years ago
|
Attachment #8901739 -
Flags: review?(VYV03354)
Comment 3•4 years ago
|
||
mozreview-review |
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.
Assignee | ||
Comment 4•4 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 5•4 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 6•4 years ago
|
||
mozreview-review-reply |
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•4 years ago
|
||
mozreview-review |
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
![]() |
||
Comment 9•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5804f73c5bd5
Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•3 years ago
|
Assignee: nobody → hsivonen
You need to log in
before you can comment on or make changes to this bug.
Description
•