Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1508913002:
Add 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. |
DescriptionAdd support for <image> type parsing using CSSParserTokens
Add support for <image> type [1] parsing using CSSParserTokens
by introducing consumeImage.
To keep the size reasonable, this patch only supports linear-gradient,
-webkit-linear-gradient, -webkit-repeated-linear-gradient, radial-gradient
and -webkit-crossfade.
BUG=499780
[1] https://drafts.csswg.org/css-images-3/#typedef-image
Committed: https://crrev.com/a6478c04b55bf419bd5ca29500a388a2d4e9718a
Cr-Commit-Position: refs/heads/master@{#364361}
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Add deprecated linear gradient support #
Total comments: 18
Patch Set 4 : Address review comments #Patch Set 5 : Fix endX code #
Total comments: 6
Patch Set 6 : Patch for landing #Messages
Total messages: 26 (16 generated)
Description was changed from ========== Start of consumeImage Cleanup BUG= ========== to ========== Add support for <image> type parsing using CSSParserTokens Add support for <image> type [1] parsing using CSSParserTokens through comsumeImage. Since the needed code to support all gradients is a lot, this patch only covers linear-gradient and radial-gradient. BUG=499780 [1] https://drafts.csswg.org/css-images-3/#typedef-image ==========
Description was changed from ========== Add support for <image> type parsing using CSSParserTokens Add support for <image> type [1] parsing using CSSParserTokens through comsumeImage. Since the needed code to support all gradients is a lot, this patch only covers linear-gradient and radial-gradient. BUG=499780 [1] https://drafts.csswg.org/css-images-3/#typedef-image ========== to ========== Add support for <image> type parsing using CSSParserTokens Add support for <image> type [1] parsing using CSSParserTokens through comsumeImage. To keep the size reasonable, this patch only covers crossfade, linear and radial gradients. BUG=499780 [1] https://drafts.csswg.org/css-images-3/#typedef-image ==========
Description was changed from ========== Add support for <image> type parsing using CSSParserTokens Add support for <image> type [1] parsing using CSSParserTokens through comsumeImage. To keep the size reasonable, this patch only covers crossfade, linear and radial gradients. BUG=499780 [1] https://drafts.csswg.org/css-images-3/#typedef-image ========== to ========== Add support for <image> type parsing using CSSParserTokens Add support for <image> type [1] parsing using CSSParserTokens through comsumeImage. To keep the size reasonable, this patch only covers crossfade, linear and radial gradients. BUG=499780 [1] https://drafts.csswg.org/css-images-3/#typedef-image ==========
Description was changed from ========== Add support for <image> type parsing using CSSParserTokens Add support for <image> type [1] parsing using CSSParserTokens through comsumeImage. To keep the size reasonable, this patch only covers crossfade, linear and radial gradients. BUG=499780 [1] https://drafts.csswg.org/css-images-3/#typedef-image ========== to ========== Add support for <image> type parsing using CSSParserTokens Add support for <image> type [1] parsing using CSSParserTokens by introducing consumeImage. To keep the size reasonable, this patch only covers crossfade, linear and radial gradients. BUG=499780 [1] https://drafts.csswg.org/css-images-3/#typedef-image ==========
rob.buis@samsung.com changed reviewers: + alancutter@chromium.org, timloh@chromium.org
PTAL.
Description was changed from ========== Add support for <image> type parsing using CSSParserTokens Add support for <image> type [1] parsing using CSSParserTokens by introducing consumeImage. To keep the size reasonable, this patch only covers crossfade, linear and radial gradients. BUG=499780 [1] https://drafts.csswg.org/css-images-3/#typedef-image ========== to ========== Add support for <image> type parsing using CSSParserTokens Add support for <image> type [1] parsing using CSSParserTokens by introducing consumeImage. To keep the size reasonable, this patch does not support yet webkit-gradient, webkit-radial-gradient and webkit-repeating-radial-gradient. BUG=499780 [1] https://drafts.csswg.org/css-images-3/#typedef-image ==========
Description was changed from ========== Add support for <image> type parsing using CSSParserTokens Add support for <image> type [1] parsing using CSSParserTokens by introducing consumeImage. To keep the size reasonable, this patch does not support yet webkit-gradient, webkit-radial-gradient and webkit-repeating-radial-gradient. BUG=499780 [1] https://drafts.csswg.org/css-images-3/#typedef-image ========== to ========== Add support for <image> type parsing using CSSParserTokens Add support for <image> type [1] parsing using CSSParserTokens by introducing consumeImage. To keep the size reasonable, this patch does not yet support webkit-gradient, webkit-radial-gradient and webkit-repeating-radial-gradient. BUG=499780 [1] https://drafts.csswg.org/css-images-3/#typedef-image ==========
On 2015/12/08 03:53:16, rwlbuis wrote: > PTAL. Patch set #3 now also has support for -webkit-linear-gradient and -webkit-repeating-linear-gradient.
Description was changed from ========== Add support for <image> type parsing using CSSParserTokens Add support for <image> type [1] parsing using CSSParserTokens by introducing consumeImage. To keep the size reasonable, this patch does not yet support webkit-gradient, webkit-radial-gradient and webkit-repeating-radial-gradient. BUG=499780 [1] https://drafts.csswg.org/css-images-3/#typedef-image ========== to ========== Add support for <image> type parsing using CSSParserTokens Add support for <image> type [1] parsing using CSSParserTokens by introducing consumeImage. To keep the size reasonable, this patch does not yet support -webkit-gradient, -webkit-radial-gradient and -webkit-repeating-radial-gradient. BUG=499780 [1] https://drafts.csswg.org/css-images-3/#typedef-image ==========
Probably should mention which image types *are* implemented in the description, not just those that aren't https://codereview.chromium.org/1508913002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/1508913002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2415: while (!range.atEnd()) { Can we just use a regular do-while loop here and move the initial comma consumption to the callers? As an interface, it isn't immediately clear what the expectComma boolean should do, so I think it should be removed. https://codereview.chromium.org/1508913002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2446: RefPtrWillBeRawPtr<CSSPrimitiveValue> shapeValue = nullptr; imo shapeValue -> shape, sizeValue -> sizeKeyword https://codereview.chromium.org/1508913002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2463: if (sizeValue || horizontalSize) Probably nicer to leave out the horizontalSize check here. This way it's clearer that this loop is only about filling in the cssvalues, and all the checks that the correct things were set happens below. https://codereview.chromium.org/1508913002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2473: if (sizeValue || horizontalSize) and leave out the sizeValue check here https://codereview.chromium.org/1508913002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2494: if (!verticalSize && horizontalSize && horizontalSize->isPercentage()) // TODO(timloh): Calcs with both lengths and percentages should be rejected. https://codereview.chromium.org/1508913002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2538: RefPtrWillBeRawPtr<CSSPrimitiveValue> location = consumeIdent<CSSValueLeft, CSSValueRight>(args); Should be enough to just write if (!endX) endX = consume... I don't see how the branch below assigning to endY is ever going to be hit. https://codereview.chromium.org/1508913002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2550: endY = cssValuePool().createIdentifierValue(CSSValueTop); Unreachable? https://codereview.chromium.org/1508913002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2588: static PassRefPtrWillBeRawPtr<CSSValue> consumeGeneratedImage(CSSParserTokenRange& range, CSSParserContext context) As usual, we need to make sure this doesn't accidentally consume garbage functions. Also it might be easier to do the end-of-function checks here? Maybe we should do something like CSSParserTokenRange rangeCopy = range; CSSParserTokenRange args = consumeFunction(rangeCopy); RefPtrWillBeRawPtr<CSSValue> result = nullptr; if (id == ..) result == consume..(); else if (id == ..) result == consume..(); else return nullptr; if (!result || !args.atEnd()) return nullptr; range = rangeCopy; return result;
Patchset #4 (id:60001) has been deleted
Description was changed from ========== Add support for <image> type parsing using CSSParserTokens Add support for <image> type [1] parsing using CSSParserTokens by introducing consumeImage. To keep the size reasonable, this patch does not yet support -webkit-gradient, -webkit-radial-gradient and -webkit-repeating-radial-gradient. BUG=499780 [1] https://drafts.csswg.org/css-images-3/#typedef-image ========== to ========== Add support for <image> type parsing using CSSParserTokens Add support for <image> type [1] parsing using CSSParserTokens by introducing consumeImage. To keep the size reasonable, this patch only supports linear-gradient, adial-gradient and -webkit-crossfade. BUG=499780 [1] https://drafts.csswg.org/css-images-3/#typedef-image ==========
Description was changed from ========== Add support for <image> type parsing using CSSParserTokens Add support for <image> type [1] parsing using CSSParserTokens by introducing consumeImage. To keep the size reasonable, this patch only supports linear-gradient, adial-gradient and -webkit-crossfade. BUG=499780 [1] https://drafts.csswg.org/css-images-3/#typedef-image ========== to ========== Add support for <image> type parsing using CSSParserTokens Add support for <image> type [1] parsing using CSSParserTokens by introducing consumeImage. To keep the size reasonable, this patch only supports linear-gradient, -webkit-linear-gradient, radial-gradient and -webkit-crossfade. BUG=499780 [1] https://drafts.csswg.org/css-images-3/#typedef-image ==========
Description was changed from ========== Add support for <image> type parsing using CSSParserTokens Add support for <image> type [1] parsing using CSSParserTokens by introducing consumeImage. To keep the size reasonable, this patch only supports linear-gradient, -webkit-linear-gradient, radial-gradient and -webkit-crossfade. BUG=499780 [1] https://drafts.csswg.org/css-images-3/#typedef-image ========== to ========== Add support for <image> type parsing using CSSParserTokens Add support for <image> type [1] parsing using CSSParserTokens by introducing consumeImage. To keep the size reasonable, this patch only supports linear-gradient, -webkit-linear-gradient, -webkit-repeated-linear-gradient, radial-gradient and -webkit-crossfade. BUG=499780 [1] https://drafts.csswg.org/css-images-3/#typedef-image ==========
Fixed the Description too. One more issue to look at... https://codereview.chromium.org/1508913002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/1508913002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2415: while (!range.atEnd()) { On 2015/12/09 06:59:57, Timothy Loh wrote: > Can we just use a regular do-while loop here and move the initial comma > consumption to the callers? As an interface, it isn't immediately clear what the > expectComma boolean should do, so I think it should be removed. Done. https://codereview.chromium.org/1508913002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2446: RefPtrWillBeRawPtr<CSSPrimitiveValue> shapeValue = nullptr; On 2015/12/09 06:59:57, Timothy Loh wrote: > imo shapeValue -> shape, sizeValue -> sizeKeyword Done. https://codereview.chromium.org/1508913002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2463: if (sizeValue || horizontalSize) On 2015/12/09 06:59:57, Timothy Loh wrote: > Probably nicer to leave out the horizontalSize check here. This way it's clearer > that this loop is only about filling in the cssvalues, and all the checks that > the correct things were set happens below. Done. https://codereview.chromium.org/1508913002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2473: if (sizeValue || horizontalSize) On 2015/12/09 06:59:57, Timothy Loh wrote: > and leave out the sizeValue check here Done. https://codereview.chromium.org/1508913002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2494: if (!verticalSize && horizontalSize && horizontalSize->isPercentage()) On 2015/12/09 06:59:57, Timothy Loh wrote: > // TODO(timloh): Calcs with both lengths and percentages should be rejected. Done. https://codereview.chromium.org/1508913002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2538: RefPtrWillBeRawPtr<CSSPrimitiveValue> location = consumeIdent<CSSValueLeft, CSSValueRight>(args); On 2015/12/09 06:59:57, Timothy Loh wrote: > Should be enough to just write > > if (!endX) > endX = consume... I think endX and location have to be separate. Otherwise we can't return null if endX was already set and location is not null. > I don't see how the branch below assigning to endY is ever going to be hit. Ouch, I now see what you mean, will have a look. https://codereview.chromium.org/1508913002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2550: endY = cssValuePool().createIdentifierValue(CSSValueTop); On 2015/12/09 06:59:57, Timothy Loh wrote: > Unreachable? Sorry, I merged a patch wrong, this is still needed for the deprecated linear-gradient cases. https://codereview.chromium.org/1508913002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2588: static PassRefPtrWillBeRawPtr<CSSValue> consumeGeneratedImage(CSSParserTokenRange& range, CSSParserContext context) On 2015/12/09 06:59:57, Timothy Loh wrote: > As usual, we need to make sure this doesn't accidentally consume garbage > functions. Also it might be easier to do the end-of-function checks here? Maybe > we should do something like > > CSSParserTokenRange rangeCopy = range; > CSSParserTokenRange args = consumeFunction(rangeCopy); > RefPtrWillBeRawPtr<CSSValue> result = nullptr; > if (id == ..) > result == consume..(); > else if (id == ..) > result == consume..(); > else > return nullptr; > > if (!result || !args.atEnd()) > return nullptr; > range = rangeCopy; > return result; Done.
I think my latest upload covers all review comments, PTAL. https://codereview.chromium.org/1508913002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/1508913002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2538: RefPtrWillBeRawPtr<CSSPrimitiveValue> location = consumeIdent<CSSValueLeft, CSSValueRight>(args); On 2015/12/09 21:31:13, rwlbuis wrote: > On 2015/12/09 06:59:57, Timothy Loh wrote: > > Should be enough to just write > > > > if (!endX) > > endX = consume... > > I think endX and location have to be separate. Otherwise we can't return null if > endX was already set and location is not null. Oh, I see why you mean here too now, should be fixed.
lgtm https://codereview.chromium.org/1508913002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/1508913002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2588: static PassRefPtrWillBeRawPtr<CSSValue> consumeGeneratedImage(CSSParserTokenRange& range, CSSParserContext context) On 2015/12/09 21:31:14, rwlbuis wrote: > On 2015/12/09 06:59:57, Timothy Loh wrote: > > As usual, we need to make sure this doesn't accidentally consume garbage > > functions. Also it might be easier to do the end-of-function checks here? > Maybe > > we should do something like > > > > CSSParserTokenRange rangeCopy = range; > > CSSParserTokenRange args = consumeFunction(rangeCopy); > > RefPtrWillBeRawPtr<CSSValue> result = nullptr; > > if (id == ..) > > result == consume..(); > > else if (id == ..) > > result == consume..(); > > else > > return nullptr; > > > > if (!result || !args.atEnd()) > > return nullptr; > > range = rangeCopy; > > return result; > > Done. Any reason to not use else if here? (which imo makes it super obvious that we only take one case) https://codereview.chromium.org/1508913002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/1508913002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2409: static bool consumeGradientColorStops(CSSParserTokenRange& range, CSSParserContext context, CSSGradientValue* gradient) Can you update all the CSSParserContext arguments similar to https://codereview.chromium.org/1514583004/? That one changes consumeColor so hopefully you don't mind rebasing over it. https://codereview.chromium.org/1508913002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2528: if (gradientType == CSSLinearGradient && !endX && !endY) Clearer like this IMO, moving the two !endX && !endY checks together and removing the unneeded endY case. if (!endX && !endY) { if (gradientType == CSSLinearGradient) return nullptr; endY = cssValuePool().createIdentifierValue(CSSValueTop); } else if (!endX) { endX = consumeIdent<CSSValueLeft, CSSValueRight>(args); } https://codereview.chromium.org/1508913002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2542: } Since you made expectComma start at true, we need else { expectComma = false; } Otherwise we'll reject linear-gradient(blue, white) and accept linear-gradient(, blue, white)
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/1508913002/#ps120001 (title: "Patch for landing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1508913002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1508913002/120001
https://codereview.chromium.org/1508913002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/1508913002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2409: static bool consumeGradientColorStops(CSSParserTokenRange& range, CSSParserContext context, CSSGradientValue* gradient) On 2015/12/10 04:20:07, Timothy Loh wrote: > Can you update all the CSSParserContext arguments similar to > https://codereview.chromium.org/1514583004/ That one changes consumeColor so > hopefully you don't mind rebasing over it. Done. https://codereview.chromium.org/1508913002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2528: if (gradientType == CSSLinearGradient && !endX && !endY) On 2015/12/10 04:20:07, Timothy Loh wrote: > Clearer like this IMO, moving the two !endX && !endY checks together and > removing the unneeded endY case. > > if (!endX && !endY) { > if (gradientType == CSSLinearGradient) > return nullptr; > endY = cssValuePool().createIdentifierValue(CSSValueTop); > } else if (!endX) { > endX = consumeIdent<CSSValueLeft, CSSValueRight>(args); > } Much better, done. https://codereview.chromium.org/1508913002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2542: } On 2015/12/10 04:20:07, Timothy Loh wrote: > Since you made expectComma start at true, we need > > else { > expectComma = false; > } > > Otherwise we'll reject linear-gradient(blue, white) and accept linear-gradient(, > blue, white) Done.
Message was sent while issue was closed.
Description was changed from ========== Add support for <image> type parsing using CSSParserTokens Add support for <image> type [1] parsing using CSSParserTokens by introducing consumeImage. To keep the size reasonable, this patch only supports linear-gradient, -webkit-linear-gradient, -webkit-repeated-linear-gradient, radial-gradient and -webkit-crossfade. BUG=499780 [1] https://drafts.csswg.org/css-images-3/#typedef-image ========== to ========== Add support for <image> type parsing using CSSParserTokens Add support for <image> type [1] parsing using CSSParserTokens by introducing consumeImage. To keep the size reasonable, this patch only supports linear-gradient, -webkit-linear-gradient, -webkit-repeated-linear-gradient, radial-gradient and -webkit-crossfade. BUG=499780 [1] https://drafts.csswg.org/css-images-3/#typedef-image ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Add support for <image> type parsing using CSSParserTokens Add support for <image> type [1] parsing using CSSParserTokens by introducing consumeImage. To keep the size reasonable, this patch only supports linear-gradient, -webkit-linear-gradient, -webkit-repeated-linear-gradient, radial-gradient and -webkit-crossfade. BUG=499780 [1] https://drafts.csswg.org/css-images-3/#typedef-image ========== to ========== Add support for <image> type parsing using CSSParserTokens Add support for <image> type [1] parsing using CSSParserTokens by introducing consumeImage. To keep the size reasonable, this patch only supports linear-gradient, -webkit-linear-gradient, -webkit-repeated-linear-gradient, radial-gradient and -webkit-crossfade. BUG=499780 [1] https://drafts.csswg.org/css-images-3/#typedef-image Committed: https://crrev.com/a6478c04b55bf419bd5ca29500a388a2d4e9718a Cr-Commit-Position: refs/heads/master@{#364361} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/a6478c04b55bf419bd5ca29500a388a2d4e9718a Cr-Commit-Position: refs/heads/master@{#364361} |
