Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1512243005:
Finish support for <image> type parsing using CSSParserTokens (Closed)
|
Created:
5 years ago by rwlbuis Modified:
5 years ago CC:
chromium-reviews, blink-reviews-css, dglazkov+blink, apavlov+blink_chromium.org, darktears, blink-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFinish support for <image> type parsing using CSSParserTokens
Finish support for <image> type by adding support for
-webkit-radial-gradient, -webkit-repeated-radial-gradient
and -webkit-gradient.
BUG=499780
Committed: https://crrev.com/ac986b051f2d6fbe6b1af09fb4b6cdeed633a770
Cr-Commit-Position: refs/heads/master@{#365225}
Patch Set 1 #Patch Set 2 : Patch for review #
Total comments: 20
Patch Set 3 : Address review comments #Patch Set 4 : Fix final issue #
Total comments: 1
Patch Set 5 : Patch for landing #Messages
Total messages: 24 (12 generated)
Description was changed from
==========
consumeImage end
BUG=
==========
to
==========
Finish support for <image> type parsing using CSSParserTokens
Finish support for <image> type by adding support for
-webkit-radial-gradient, -webkit-repeated-radial-gradient,
and -webkit-gradient.
==========
Description was changed from
==========
Finish support for <image> type parsing using CSSParserTokens
Finish support for <image> type by adding support for
-webkit-radial-gradient, -webkit-repeated-radial-gradient,
and -webkit-gradient.
==========
to
==========
Finish support for <image> type parsing using CSSParserTokens
Finish support for <image> type by adding support for
-webkit-radial-gradient, -webkit-repeated-radial-gradient
and -webkit-gradient.
==========
rob.buis@samsung.com changed reviewers: + alancutter@chromium.org, timloh@chromium.org
PTAL.
Description was changed from
==========
Finish support for <image> type parsing using CSSParserTokens
Finish support for <image> type by adding support for
-webkit-radial-gradient, -webkit-repeated-radial-gradient
and -webkit-gradient.
==========
to
==========
Finish support for <image> type parsing using CSSParserTokens
Finish support for <image> type by adding support for
-webkit-radial-gradient, -webkit-repeated-radial-gradient
and -webkit-gradient.
BUG=499780
==========
https://codereview.chromium.org/1512243005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/1512243005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:835: static bool consumePosition2Values(CSSParserTokenRange& range, CSSParserMode cssParserMode, UnitlessQuirk unitless, RefPtrWillBeRawPtr<CSSValue>& resultX, RefPtrWillBeRawPtr<CSSValue>& resultY) consumeOneOrTwoValuedPosition maybe? :S https://codereview.chromium.org/1512243005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2430: // Disallow currentcolor. This comment doesn't add value https://codereview.chromium.org/1512243005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2446: } else if (id == CSSValueColorStop) { IMO clearer as } else { ASSERT(id == CSSValueColorStop); since otherwise it suggests there's a case which isn't caught here. https://codereview.chromium.org/1512243005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2458: stop.m_color = consumeDeprecatedGradientStopColor(args, cssParserMode); Could put this outside the if/else. Also might be better to have "float position" before the if/else and assign stop.m_position after the if/else https://codereview.chromium.org/1512243005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2516: while (!args.atEnd()) { maybe clearer as while (consumeComma..(args)) ? https://codereview.chromium.org/1512243005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2517: // The function name needs to be one of "from", "to", or "color-stop." not sure this comment helps https://codereview.chromium.org/1512243005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2556: // Optional background-position comment is weird, this isn't background-position, and <position> has a broader syntax. https://codereview.chromium.org/1512243005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2559: consumePosition2Values(args, cssParserMode, UnitlessQuirk::Forbid, centerX, centerY); OK This function (and the equivalent in the legacy parser) can fail but still consume, e.g. "background-image: -webkit-radial-gradient(left white, black)" will be parsed as valid but ignore the "left". I guess it's fine :/ https://codereview.chromium.org/1512243005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2565: // CSS3 radial gradients always share the same start and end point. drop the comment, -webkit-* probably doesn't count as css3 ;) https://codereview.chromium.org/1512243005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2577: RefPtrWillBeRawPtr<CSSPrimitiveValue> horizontalSize = nullptr; I'd move these into the first if branch and the the setEnd*Size calls inside the nested if branch.
Fixed most, will check on more thing. https://codereview.chromium.org/1512243005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/1512243005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:835: static bool consumePosition2Values(CSSParserTokenRange& range, CSSParserMode cssParserMode, UnitlessQuirk unitless, RefPtrWillBeRawPtr<CSSValue>& resultX, RefPtrWillBeRawPtr<CSSValue>& resultY) On 2015/12/14 06:59:54, Timothy Loh wrote: > consumeOneOrTwoValuedPosition maybe? :S Done. https://codereview.chromium.org/1512243005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2430: // Disallow currentcolor. On 2015/12/14 06:59:55, Timothy Loh wrote: > This comment doesn't add value Yeah, I thought for a first review I'd keep them but most of them are pretty obvious indeed. Removed. https://codereview.chromium.org/1512243005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2446: } else if (id == CSSValueColorStop) { On 2015/12/14 06:59:55, Timothy Loh wrote: > IMO clearer as > > } else { > ASSERT(id == CSSValueColorStop); > > since otherwise it suggests there's a case which isn't caught here. Done. I basically forgot about the if at the top of the method :) Does show that this was not high quality code/refactored a lot... https://codereview.chromium.org/1512243005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2458: stop.m_color = consumeDeprecatedGradientStopColor(args, cssParserMode); On 2015/12/14 06:59:55, Timothy Loh wrote: > Could put this outside the if/else. Also might be better to have "float > position" before the if/else and assign stop.m_position after the if/else Done. https://codereview.chromium.org/1512243005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2516: while (!args.atEnd()) { On 2015/12/14 06:59:55, Timothy Loh wrote: > maybe clearer as while (consumeComma..(args)) ? Oops, forgot, will have a look... https://codereview.chromium.org/1512243005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2517: // The function name needs to be one of "from", "to", or "color-stop." On 2015/12/14 06:59:55, Timothy Loh wrote: > not sure this comment helps Done. https://codereview.chromium.org/1512243005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2556: // Optional background-position On 2015/12/14 06:59:55, Timothy Loh wrote: > comment is weird, this isn't background-position, and <position> has a broader > syntax. Done. https://codereview.chromium.org/1512243005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2559: consumePosition2Values(args, cssParserMode, UnitlessQuirk::Forbid, centerX, centerY); On 2015/12/14 06:59:54, Timothy Loh wrote: > OK > > This function (and the equivalent in the legacy parser) can fail but still > consume, e.g. "background-image: -webkit-radial-gradient(left white, black)" > will be parsed as valid but ignore the "left". I guess it's fine :/ Main thing is that everything remains green and we do not accept anything we should not accept IMHO. https://codereview.chromium.org/1512243005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2565: // CSS3 radial gradients always share the same start and end point. On 2015/12/14 06:59:55, Timothy Loh wrote: > drop the comment, -webkit-* probably doesn't count as css3 ;) Done. https://codereview.chromium.org/1512243005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2577: RefPtrWillBeRawPtr<CSSPrimitiveValue> horizontalSize = nullptr; On 2015/12/14 06:59:54, Timothy Loh wrote: > I'd move these into the first if branch and the the setEnd*Size calls inside the > nested if branch. Done.
On 2015/12/14 19:38:39, rwlbuis wrote: > https://codereview.chromium.org/1512243005/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2516: while > (!args.atEnd()) { > On 2015/12/14 06:59:55, Timothy Loh wrote: > > maybe clearer as while (consumeComma..(args)) ? > > Oops, forgot, will have a look... Done. PTAL!
lgtm https://codereview.chromium.org/1512243005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/1512243005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2442: const CSSParserToken& arg = args.consumeIncludingWhitespace(); Probably should be in the else branch, since we don't want to consume for from/to.
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 Link to the patchset: https://codereview.chromium.org/1512243005/#ps80001 (title: "Patch for landing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1512243005/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1512243005/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by rob.buis@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1512243005/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1512243005/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_android_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by rob.buis@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1512243005/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1512243005/80001
Message was sent while issue was closed.
Description was changed from
==========
Finish support for <image> type parsing using CSSParserTokens
Finish support for <image> type by adding support for
-webkit-radial-gradient, -webkit-repeated-radial-gradient
and -webkit-gradient.
BUG=499780
==========
to
==========
Finish support for <image> type parsing using CSSParserTokens
Finish support for <image> type by adding support for
-webkit-radial-gradient, -webkit-repeated-radial-gradient
and -webkit-gradient.
BUG=499780
==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from
==========
Finish support for <image> type parsing using CSSParserTokens
Finish support for <image> type by adding support for
-webkit-radial-gradient, -webkit-repeated-radial-gradient
and -webkit-gradient.
BUG=499780
==========
to
==========
Finish support for <image> type parsing using CSSParserTokens
Finish support for <image> type by adding support for
-webkit-radial-gradient, -webkit-repeated-radial-gradient
and -webkit-gradient.
BUG=499780
Committed: https://crrev.com/ac986b051f2d6fbe6b1af09fb4b6cdeed633a770
Cr-Commit-Position: refs/heads/master@{#365225}
==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/ac986b051f2d6fbe6b1af09fb4b6cdeed633a770 Cr-Commit-Position: refs/heads/master@{#365225} |
