|
|
Created:
4 years, 9 months ago by rwlbuis Modified:
4 years, 9 months ago Reviewers:
Timothy Loh, jfernandez 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. |
DescriptionMove grid-auto-column/grid-auto-row into CSSPropertyParser
Move the grid-auto-column/grid-auto-row longhands from
LegacyCSSPropertyParser into CSSPropertyParser.
Introduce the consumeGridTrackSize helper function that
we can use in upcoming patches.
BUG=499780
Committed: https://crrev.com/d255da18fe5a85dd249ed8f99e84a2a1c66806bf
Cr-Commit-Position: refs/heads/master@{#382749}
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Clean up a bit more #
Total comments: 12
Patch Set 4 : Address review comments #
Created: 4 years, 9 months ago
Messages
Total messages: 14 (7 generated)
Description was changed from ========== foo 1813763003 WIP BUG= ========== to ========== auto-rows WIP ==========
Description was changed from ========== auto-rows WIP ========== to ========== Move grid-auto-column/grid-auto-row into CSSPropertyParser Move the grid-auto-column/grid-auto-row longhands from LegacyCSSPropertyParser into CSSPropertyParser. Introduce the consumeGridTrackSize helper function that we can use in upcoming patches. BUG=499780 ==========
Description was changed from ========== Move grid-auto-column/grid-auto-row into CSSPropertyParser Move the grid-auto-column/grid-auto-row longhands from LegacyCSSPropertyParser into CSSPropertyParser. Introduce the consumeGridTrackSize helper function that we can use in upcoming patches. BUG=499780 ========== to ========== Move grid-auto-column/grid-auto-row into CSSPropertyParser Move the grid-auto-column/grid-auto-row longhands from LegacyCSSPropertyParser into CSSPropertyParser. Introduce the consumeGridTrackSize helper function that we can use in upcoming patches. BUG=499780 ==========
rob.buis@samsung.com changed reviewers: + jfernandez@igalia.com, timloh@chromium.org
PTAL. Note that we'll have to keep an eye on any code changes in these helper functions in the legacy parser now that they are duplicated...
https://codereview.chromium.org/1813053004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/1813053004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3072: if (identMatches<CSSValueMinContent, CSSValueMaxContent, CSSValueAuto>(token.id())) Maybe nicer if we check the restriction first? if (restriction == AllowAll) { ... } return consumeLengthOrPercent(); https://codereview.chromium.org/1813053004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3078: double flexValue = range.consumeIncludingWhitespace().numericValue(); Probably should finish validating before consuming so we don't update the range and then return nullptr https://codereview.chromium.org/1813053004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3090: if (identMatches<CSSValueAuto>(token.id())) IMO clearer to not use a ternary and just write if (restriction == AllowAll && identMatches..) https://codereview.chromium.org/1813053004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3094: // The spec defines the following grammar: minmax( <track-breadth> , <track-breadth> ). Comment isn't very useful imo https://codereview.chromium.org/1813053004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3095: CSSParserTokenRange args = consumeFunction(range); Should we make a copy of the range? If the arguments are invalid we'd end up still consuming the minmax() function and the "[ <line-names>? <string> <track-size>? <line-names>? ]+ [ / <track-list> ]?" case of grid will be neater if this function doesn't do that. https://codereview.chromium.org/1813053004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.h (right): https://codereview.chromium.org/1813053004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.h:46: enum TrackSizeRestriction { FixedSizeOnly, AllowAll }; TODO to move to cpp file once legacy parser is removed?
All issues should be fixed, PTAL. https://codereview.chromium.org/1813053004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/1813053004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3072: if (identMatches<CSSValueMinContent, CSSValueMaxContent, CSSValueAuto>(token.id())) On 2016/03/22 06:33:59, Timothy Loh wrote: > Maybe nicer if we check the restriction first? > > if (restriction == AllowAll) { > ... > } > return consumeLengthOrPercent(); Done. https://codereview.chromium.org/1813053004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3078: double flexValue = range.consumeIncludingWhitespace().numericValue(); On 2016/03/22 06:33:59, Timothy Loh wrote: > Probably should finish validating before consuming so we don't update the range > and then return nullptr Done. https://codereview.chromium.org/1813053004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3090: if (identMatches<CSSValueAuto>(token.id())) On 2016/03/22 06:33:59, Timothy Loh wrote: > IMO clearer to not use a ternary and just write > > if (restriction == AllowAll && identMatches..) Done. https://codereview.chromium.org/1813053004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3094: // The spec defines the following grammar: minmax( <track-breadth> , <track-breadth> ). On 2016/03/22 06:33:59, Timothy Loh wrote: > Comment isn't very useful imo Done. https://codereview.chromium.org/1813053004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3095: CSSParserTokenRange args = consumeFunction(range); On 2016/03/22 06:33:59, Timothy Loh wrote: > Should we make a copy of the range? If the arguments are invalid we'd end up > still consuming the minmax() function and the "[ <line-names>? <string> > <track-size>? <line-names>? ]+ [ / <track-list> ]?" case of grid will be neater > if this function doesn't do that. Done. https://codereview.chromium.org/1813053004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.h (right): https://codereview.chromium.org/1813053004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.h:46: enum TrackSizeRestriction { FixedSizeOnly, AllowAll }; On 2016/03/22 06:33:59, Timothy Loh wrote: > TODO to move to cpp file once legacy parser is removed? Done.
lgtm
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/1813053004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1813053004/60001
Message was sent while issue was closed.
Description was changed from ========== Move grid-auto-column/grid-auto-row into CSSPropertyParser Move the grid-auto-column/grid-auto-row longhands from LegacyCSSPropertyParser into CSSPropertyParser. Introduce the consumeGridTrackSize helper function that we can use in upcoming patches. BUG=499780 ========== to ========== Move grid-auto-column/grid-auto-row into CSSPropertyParser Move the grid-auto-column/grid-auto-row longhands from LegacyCSSPropertyParser into CSSPropertyParser. Introduce the consumeGridTrackSize helper function that we can use in upcoming patches. BUG=499780 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Move grid-auto-column/grid-auto-row into CSSPropertyParser Move the grid-auto-column/grid-auto-row longhands from LegacyCSSPropertyParser into CSSPropertyParser. Introduce the consumeGridTrackSize helper function that we can use in upcoming patches. BUG=499780 ========== to ========== Move grid-auto-column/grid-auto-row into CSSPropertyParser Move the grid-auto-column/grid-auto-row longhands from LegacyCSSPropertyParser into CSSPropertyParser. Introduce the consumeGridTrackSize helper function that we can use in upcoming patches. BUG=499780 Committed: https://crrev.com/d255da18fe5a85dd249ed8f99e84a2a1c66806bf Cr-Commit-Position: refs/heads/master@{#382749} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/d255da18fe5a85dd249ed8f99e84a2a1c66806bf Cr-Commit-Position: refs/heads/master@{#382749} |