Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1963843002:
Implement stricter hashless hex color parsing (Closed)
|
Created:
4 years, 7 months ago by rwlbuis Modified:
4 years, 7 months ago CC:
chromium-reviews, blink-reviews-css, dglazkov+blink, apavlov+blink_chromium.org, darktears, blink-reviews, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement stricter hashless hex color parsing
According to the spec[1] for both <number> and <dimension> we need to check
that the component values are non-negative and integers. Implement that and
add some subtests for it.
With this change we are passing the official test[2] linked from the spec.
BUG=611442
[1] https://quirks.spec.whatwg.org/#the-hashless-hex-color-quirk
[2] http://w3c-test.org/quirks-mode/hashless-hex-color.html
Committed: https://crrev.com/08ced96eadd5a34cdd251a02e68d686d556f8a12
Cr-Commit-Position: refs/heads/master@{#393502}
Patch Set 1 #Patch Set 2 : V2 #Patch Set 3 : Add more subtests #
Total comments: 4
Patch Set 4 : Address review comments #
Total comments: 2
Patch Set 5 : Address timloh review comments #Patch Set 6 : Patch for landing #
Messages
Total messages: 19 (9 generated)
Description was changed from ========== colorquirk WIP BUG= ========== to ========== Implement stricter hashless hex color parsing According to the spec[1] for both <number> and <dimension> we need to check that the component values are non-negative and integers. Implement that and add some subtests for it. [1] https://quirks.spec.whatwg.org/#the-hashless-hex-color-quirk ==========
Patchset #3 (id:40001) has been deleted
rob.buis@samsung.com changed reviewers: + rune@opera.com, timloh@chromium.org
PTAL. I don't think this conflicts with Noel's patch. @timloh note that something like -0000FF would still be accepted, because for dimensions we are not supposed to ask about numeric sign, not sure how to solve that one....
https://codereview.chromium.org/1963843002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/css/parsing-color-quirk.html (left): https://codereview.chromium.org/1963843002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/css/parsing-color-quirk.html:193: </script> What's the pass rate of http://w3c-test.org/quirks-mode/hashless-hex-color.html now? Would be good if we could import that instead. https://codereview.chromium.org/1963843002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp (right): https://codereview.chromium.org/1963843002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:416: color = String::number(static_cast<int>(token.numericValue())) + String(token.value()); What happens with the static_cast when numericValue() is larger than an int (could happen since we store the int as a double)? Could the result be something with less than 6 digits? Perhaps have an upper check as well?
https://codereview.chromium.org/1963843002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/css/parsing-color-quirk.html (left): https://codereview.chromium.org/1963843002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/css/parsing-color-quirk.html:193: </script> On 2016/05/10 21:27:32, rune wrote: > What's the pass rate of http://w3c-test.org/quirks-mode/hashless-hex-color.html > now? Would be good if we could import that instead. Without patch 26 fail, with patch 0 fail :) Thanks, I forgot about this test, will see if I can import it. https://codereview.chromium.org/1963843002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp (right): https://codereview.chromium.org/1963843002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:416: color = String::number(static_cast<int>(token.numericValue())) + String(token.value()); On 2016/05/10 21:27:32, rune wrote: > What happens with the static_cast when numericValue() is larger than an int > (could happen since we store the int as a double)? Could the result be something > with less than 6 digits? Perhaps have an upper check as well? Good point! Will have a look.
I guess the -0000FF case can just stay as-is since this way we match spec. BTW I don't think we should be importing the test like this, if anything it should go through the regular test importing system (i.e. into LayoutTests/imported with via update-w3c-deps). For now we should just add our own test and mention in the patch description that the w3c test is now passing. https://codereview.chromium.org/1963843002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp (right): https://codereview.chromium.org/1963843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:415: || token.numericValue() < 0. || token.numericValue() > std::numeric_limits<int>::max()) Why not compare to 10^6, similar to number types?
Ok I'll bring back the old test change and won't import the w3c test in this CL. https://codereview.chromium.org/1963843002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp (right): https://codereview.chromium.org/1963843002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:415: || token.numericValue() < 0. || token.numericValue() > std::numeric_limits<int>::max()) On 2016/05/11 02:12:55, Timothy Loh wrote: > Why not compare to 10^6, similar to number types? Ah, I got confused by https://codereview.chromium.org/1936913002/ , guess 10^6 is fine then. Seems also like we can lump the NumberToken and DimensionToken blocks together. Will fix tomorrow.
lgtm
lgtm, but I'd wait for Noel to land his patch first and rebase over it (since his was sent out first). His patch is just about done so it shouldn't be much of a wait.
Description was changed from ========== Implement stricter hashless hex color parsing According to the spec[1] for both <number> and <dimension> we need to check that the component values are non-negative and integers. Implement that and add some subtests for it. [1] https://quirks.spec.whatwg.org/#the-hashless-hex-color-quirk ========== to ========== Implement stricter hashless hex color parsing According to the spec[1] for both <number> and <dimension> we need to check that the component values are non-negative and integers. Implement that and add some subtests for it. With this change we are passing the official test[2] linked from the spec. [1] https://quirks.spec.whatwg.org/#the-hashless-hex-color-quirk [2] http://w3c-test.org/quirks-mode/hashless-hex-color.html ==========
Description was changed from ========== Implement stricter hashless hex color parsing According to the spec[1] for both <number> and <dimension> we need to check that the component values are non-negative and integers. Implement that and add some subtests for it. With this change we are passing the official test[2] linked from the spec. [1] https://quirks.spec.whatwg.org/#the-hashless-hex-color-quirk [2] http://w3c-test.org/quirks-mode/hashless-hex-color.html ========== to ========== Implement stricter hashless hex color parsing According to the spec[1] for both <number> and <dimension> we need to check that the component values are non-negative and integers. Implement that and add some subtests for it. With this change we are passing the official test[2] linked from the spec. BUG=611442 [1] https://quirks.spec.whatwg.org/#the-hashless-hex-color-quirk [2] http://w3c-test.org/quirks-mode/hashless-hex-color.html ==========
The CQ bit was checked by rob.buis@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org, rune@opera.com Link to the patchset: https://codereview.chromium.org/1963843002/#ps120001 (title: "Patch for landing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1963843002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1963843002/120001
Message was sent while issue was closed.
Description was changed from ========== Implement stricter hashless hex color parsing According to the spec[1] for both <number> and <dimension> we need to check that the component values are non-negative and integers. Implement that and add some subtests for it. With this change we are passing the official test[2] linked from the spec. BUG=611442 [1] https://quirks.spec.whatwg.org/#the-hashless-hex-color-quirk [2] http://w3c-test.org/quirks-mode/hashless-hex-color.html ========== to ========== Implement stricter hashless hex color parsing According to the spec[1] for both <number> and <dimension> we need to check that the component values are non-negative and integers. Implement that and add some subtests for it. With this change we are passing the official test[2] linked from the spec. BUG=611442 [1] https://quirks.spec.whatwg.org/#the-hashless-hex-color-quirk [2] http://w3c-test.org/quirks-mode/hashless-hex-color.html ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Implement stricter hashless hex color parsing According to the spec[1] for both <number> and <dimension> we need to check that the component values are non-negative and integers. Implement that and add some subtests for it. With this change we are passing the official test[2] linked from the spec. BUG=611442 [1] https://quirks.spec.whatwg.org/#the-hashless-hex-color-quirk [2] http://w3c-test.org/quirks-mode/hashless-hex-color.html ========== to ========== Implement stricter hashless hex color parsing According to the spec[1] for both <number> and <dimension> we need to check that the component values are non-negative and integers. Implement that and add some subtests for it. With this change we are passing the official test[2] linked from the spec. BUG=611442 [1] https://quirks.spec.whatwg.org/#the-hashless-hex-color-quirk [2] http://w3c-test.org/quirks-mode/hashless-hex-color.html Committed: https://crrev.com/08ced96eadd5a34cdd251a02e68d686d556f8a12 Cr-Commit-Position: refs/heads/master@{#393502} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/08ced96eadd5a34cdd251a02e68d686d556f8a12 Cr-Commit-Position: refs/heads/master@{#393502} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
