Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2497873002:
Tidy up ScriptLoader (MIME) type matching. (Closed)
DescriptionTidy up ScriptLoader (MIME) type matching.
MIME is case-insensitively handled within Blink, so remove some
unnecessary normalization of MIME type (and "language=") strings
in ScriptLoader.
R=
BUG=
Committed: https://crrev.com/688599703941eb149f30815d0968ae0c163c0194
Cr-Commit-Position: refs/heads/master@{#432162}
Patch Set 1 #
Total comments: 11
Patch Set 2 : perform case-insensitive equality checks over ASCII only #
Messages
Total messages: 21 (12 generated)
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
sigbjornf@opera.com changed reviewers: + kinuko@chromium.org, tkent@chromium.org
please take a look. not a lot in this one, but the explicit "lowering" encountered while reading this code struck me as better avoided.
lgtm. I added some comments, but they are not your fault. you may land this as is. https://codereview.chromium.org/2497873002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/ScriptLoader.cpp (right): https://codereview.chromium.org/2497873002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ScriptLoader.cpp:143: return equalIgnoringCase(language, "javascript") || Maybe equalIgnoringASCIICase() should be used. AFAIK, equalIgnoringCase(input, literal) and equalIgnoringASCIICase(input, literal) has a different behavior only if the literal contains 'k'. https://codereview.chromium.org/2497873002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ScriptLoader.cpp:412: bool isText = mimeType.startsWith("text/", TextCaseInsensitive); Maybe TextCaseASCIIInsensitive instead of TextCaseInsensitive. https://codereview.chromium.org/2497873002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ScriptLoader.cpp:418: !isText && mimeType.startsWith("application/", TextCaseInsensitive); Ditto. https://codereview.chromium.org/2497873002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ScriptLoader.cpp:493: String mimeType = resource->httpContentType(); We should remove .lower() in Resource::httpContentType() for consistency.
lgtm https://codereview.chromium.org/2497873002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/MIMETypeRegistry.h (right): https://codereview.chromium.org/2497873002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/MIMETypeRegistry.h:39: // The MIMETypeRegistry predicates are all case-insensitive. Thanks for adding this comment!
https://codereview.chromium.org/2497873002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/ScriptLoader.cpp (right): https://codereview.chromium.org/2497873002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ScriptLoader.cpp:143: return equalIgnoringCase(language, "javascript") || On 2016/11/14 23:44:32, tkent wrote: > Maybe equalIgnoringASCIICase() should be used. > AFAIK, equalIgnoringCase(input, literal) and equalIgnoringASCIICase(input, > literal) has a different behavior only if the literal contains 'k'. Now that sounds too interesting :-) Which equality predicate is wrongly folding some codepoints onto 'k'?
https://codereview.chromium.org/2497873002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/ScriptLoader.cpp (right): https://codereview.chromium.org/2497873002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ScriptLoader.cpp:143: return equalIgnoringCase(language, "javascript") || On 2016/11/15 at 08:03:43, sof wrote: > On 2016/11/14 23:44:32, tkent wrote: > > Maybe equalIgnoringASCIICase() should be used. > > AFAIK, equalIgnoringCase(input, literal) and equalIgnoringASCIICase(input, > > literal) has a different behavior only if the literal contains 'k'. > > Now that sounds too interesting :-) Which equality predicate is wrongly folding some codepoints onto 'k'? equalIgnoringCase() assumes U+212A KELVIN SIGN and U+006B 'k' are equal :) http://unicode.org/Public/UCD/latest/ucd/CaseFolding.txt I think there are no other cases which map a non-ASCII character to a lower ASCII character.
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2497873002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/ScriptLoader.cpp (right): https://codereview.chromium.org/2497873002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ScriptLoader.cpp:143: return equalIgnoringCase(language, "javascript") || On 2016/11/15 08:16:49, tkent wrote: > On 2016/11/15 at 08:03:43, sof wrote: > > On 2016/11/14 23:44:32, tkent wrote: > > > Maybe equalIgnoringASCIICase() should be used. > > > AFAIK, equalIgnoringCase(input, literal) and equalIgnoringASCIICase(input, > > > literal) has a different behavior only if the literal contains 'k'. > > > > Now that sounds too interesting :-) Which equality predicate is wrongly > folding some codepoints onto 'k'? > > equalIgnoringCase() assumes U+212A KELVIN SIGN and U+006B 'k' are equal :) > http://unicode.org/Public/UCD/latest/ucd/CaseFolding.txt > > I think there are no other cases which map a non-ASCII character to a lower > ASCII character. > Excellent, thanks for the pointer - so not behaving wrongly at all (if you know your case folding tables :) ) I've switched to equalIgnoringASCIICase(). https://codereview.chromium.org/2497873002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ScriptLoader.cpp:412: bool isText = mimeType.startsWith("text/", TextCaseInsensitive); On 2016/11/14 23:44:32, tkent wrote: > Maybe TextCaseASCIIInsensitive instead of TextCaseInsensitive. Yes, a newer addition - switched to it. https://codereview.chromium.org/2497873002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ScriptLoader.cpp:418: !isText && mimeType.startsWith("application/", TextCaseInsensitive); On 2016/11/14 23:44:32, tkent wrote: > Ditto. Same, switched. https://codereview.chromium.org/2497873002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ScriptLoader.cpp:493: String mimeType = resource->httpContentType(); On 2016/11/14 23:44:32, tkent wrote: > We should remove .lower() in Resource::httpContentType() for consistency. Maybe. I'm not convinced it represents an improvement unless you also change the return type to be something that enforces correct matching/equality checking over MIME types.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sigbjornf@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org, kinuko@chromium.org Link to the patchset: https://codereview.chromium.org/2497873002/#ps20001 (title: "perform case-insensitive equality checks over ASCII only")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Tidy up ScriptLoader (MIME) type matching. MIME is case-insensitively handled within Blink, so remove some unnecessary normalization of MIME type (and "language=") strings in ScriptLoader. R= BUG= ========== to ========== Tidy up ScriptLoader (MIME) type matching. MIME is case-insensitively handled within Blink, so remove some unnecessary normalization of MIME type (and "language=") strings in ScriptLoader. R= BUG= Committed: https://crrev.com/688599703941eb149f30815d0968ae0c163c0194 Cr-Commit-Position: refs/heads/master@{#432162} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/688599703941eb149f30815d0968ae0c163c0194 Cr-Commit-Position: refs/heads/master@{#432162} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
