Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1816253002:
Move grid-template-columns/grid-template-rows into CSSPropertyParser (Closed)
|
Created:
4 years, 9 months ago by rwlbuis Modified:
4 years, 8 months 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. |
DescriptionMove grid-template-columns/grid-template-rows into CSSPropertyParser
Move the grid-template-columns/grid-template-rows longhands from
LegacyCSSPropertyParser into CSSPropertyParser.
BUG=499780
Committed: https://crrev.com/92e2a550b54002eaf9307642f534db1625f0646d
Cr-Commit-Position: refs/heads/master@{#383935}
Patch Set 1 #Patch Set 2 : Fix tests #Patch Set 3 : Rebase #Patch Set 4 : More cleanups #
Total comments: 17
Patch Set 5 : Address some of the review comments #Patch Set 6 : Address one more thing #Patch Set 7 : Address final issue #
Total comments: 10
Patch Set 8 : Address more comments #Patch Set 9 : Add TODO for track-list #Patch Set 10 : Patch for landing #
Messages
Total messages: 24 (10 generated)
Patchset #2 (id:20001) has been deleted
Description was changed from ========== grid-template-* WIP auto-rows WIP patch from issue 1813053004 at patchset 20001 (http://crrev.com/1813053004#ps20001) BUG= ========== to ========== Move grid-template-columns/grid-template-rows into CSSPropertyParser Move the grid-template-columns/grid-template-rows longhands from LegacyCSSPropertyParser into CSSPropertyParser. BUG=499780 ==========
rob.buis@samsung.com changed reviewers: + jfernandez@igalia.com, timloh@chromium.org
PTAL. This adds another helper method we can build on (consumeGridTrackList).
https://codereview.chromium.org/1816253002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/1816253002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3107: static bool consumeGridLineNames(CSSParserTokenRange& range, CSSValueList& valueList) I think this would be clearer it returned a CSSGridLineNamesValue (we'll need to make sure we don't update the range if we fail and callsites will need to append the value to their lists). The function signature right now suggests that it it will return false if it doesn't consume anything. https://codereview.chromium.org/1816253002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3122: static bool consumeGridTrackRepeatFunction(CSSParserTokenRange& range, CSSParserMode cssParserMode, CSSValueList& list, bool& isAutoRepeat) Maybe better / more consistent with everything else if the caller has to check functionId() instead. Otherwise we'll end up consuming repeat(random junk) and it would behave the same as if the function wasn't there. https://codereview.chromium.org/1816253002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3133: if (!isAutoRepeat) { clearer to not negate here and swap the blocks around https://codereview.chromium.org/1816253002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3134: if (args.peek().type() != NumberToken || args.peek().numericValueType() != IntegerValueType) consumePositiveInteger?? https://codereview.chromium.org/1816253002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3149: while (!args.atEnd()) { I think you can do: do { .. } while (!args.atEnd() && !isAutoRepeat); and then remove the numberOfTracks variable and just use repeatedValues->length() when doing the division below. https://codereview.chromium.org/1816253002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3179: if (range.peek().id() == CSSValueNone) I think consumeGridTrackList (or just consumeTrackList?) shouldn't include this bit and just be responsible for parsing <track-size>|<auto-track-size>. We could have some function like consumeGridTemplatesRowsOrColumns which handles none. The grid shorthand is going to need to re-use this code for <track-size>. https://codereview.chromium.org/1816253002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3187: while (!range.atEnd()) { Maybe like this, and then you don't need seenTrackSizeOrRepeatFunction. I generally prefer a do-while loop to indicate that you must have at least one iteration. do { .. } while (!range.atEnd() && range.peek().type() != DelimiterToken);
PTAL. https://codereview.chromium.org/1816253002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/1816253002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3107: static bool consumeGridLineNames(CSSParserTokenRange& range, CSSValueList& valueList) On 2016/03/24 04:29:31, Timothy Loh wrote: > I think this would be clearer it returned a CSSGridLineNamesValue (we'll need to > make sure we don't update the range if we fail and callsites will need to append > the value to their lists). The function signature right now suggests that it it > will return false if it doesn't consume anything. Done. https://codereview.chromium.org/1816253002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3122: static bool consumeGridTrackRepeatFunction(CSSParserTokenRange& range, CSSParserMode cssParserMode, CSSValueList& list, bool& isAutoRepeat) On 2016/03/24 04:29:31, Timothy Loh wrote: > Maybe better / more consistent with everything else if the caller has to check > functionId() instead. Otherwise we'll end up consuming repeat(random junk) and > it would behave the same as if the function wasn't there. Done. https://codereview.chromium.org/1816253002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3133: if (!isAutoRepeat) { On 2016/03/24 04:29:31, Timothy Loh wrote: > clearer to not negate here and swap the blocks around Done. https://codereview.chromium.org/1816253002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3134: if (args.peek().type() != NumberToken || args.peek().numericValueType() != IntegerValueType) On 2016/03/24 04:29:31, Timothy Loh wrote: > consumePositiveInteger?? That would work, but it will create a CSSValue for nothing IMHO. https://codereview.chromium.org/1816253002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3149: while (!args.atEnd()) { On 2016/03/24 04:29:31, Timothy Loh wrote: > I think you can do: > > do { > .. > } while (!args.atEnd() && !isAutoRepeat); > > and then remove the numberOfTracks variable and just use > repeatedValues->length() when doing the division below. I don't think this will work since then it would include grid lines as well. https://codereview.chromium.org/1816253002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3179: if (range.peek().id() == CSSValueNone) On 2016/03/24 04:29:31, Timothy Loh wrote: > I think consumeGridTrackList (or just consumeTrackList?) shouldn't include this > bit and just be responsible for parsing <track-size>|<auto-track-size>. We could > have some function like consumeGridTemplatesRowsOrColumns which handles none. > The grid shorthand is going to need to re-use this code for <track-size>. Agreed, good point, done. https://codereview.chromium.org/1816253002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3187: while (!range.atEnd()) { On 2016/03/24 04:29:31, Timothy Loh wrote: > Maybe like this, and then you don't need seenTrackSizeOrRepeatFunction. I > generally prefer a do-while loop to indicate that you must have at least one > iteration. > > do { > .. > } while (!range.atEnd() && range.peek().type() != DelimiterToken); Done.
https://codereview.chromium.org/1816253002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/1816253002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3134: if (args.peek().type() != NumberToken || args.peek().numericValueType() != IntegerValueType) On 2016/03/24 20:40:32, rwlbuis wrote: > On 2016/03/24 04:29:31, Timothy Loh wrote: > > consumePositiveInteger?? > > That would work, but it will create a CSSValue for nothing IMHO. We need to use it to handle calc() correctly. Maybe add a TODO to make a consumeIntegerRaw or something (similar to how we have consumeNumberRaw) https://codereview.chromium.org/1816253002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3149: while (!args.atEnd()) { On 2016/03/24 20:40:32, rwlbuis wrote: > On 2016/03/24 04:29:31, Timothy Loh wrote: > > I think you can do: > > > > do { > > .. > > } while (!args.atEnd() && !isAutoRepeat); > > > > and then remove the numberOfTracks variable and just use > > repeatedValues->length() when doing the division below. > > I don't think this will work since then it would include grid lines as well. Not sure what you mean exactly :/ https://codereview.chromium.org/1816253002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/1816253002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3112: rangeCopy.consumeIncludingWhitespace(); // Skip '['. Not a fan of the two comments here, at least because they're using different words. Maybe just declare rangeCopy first, and then change both checks to if (range.consumeIncludingWhitespace().type() != XBracketToken) return nullptr; https://codereview.chromium.org/1816253002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3181: if (range.peek().id() == CSSValueNone) I think you forgot to remove this from here since you added it to consumeGridTemplatesRowsOrColumns https://codereview.chromium.org/1816253002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3189: do { This allows <line-names> directly before <auto-repeat>, and allows a leading [ <fixed-size> | <fixed-repeat> ] to be omitted. Looks like the existing code also gets these cases wrong so we could just add a TODO for them.
jfernandez@igalia.com changed reviewers: + rego@igalia.com, svillar@igalia.com
Almost there. BTW aren't you removing the code in the legacy parser? Or will it be done later? https://codereview.chromium.org/1816253002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/1816253002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3189: do { On 2016/03/29 02:47:39, Timothy Loh wrote: > This allows <line-names> directly before <auto-repeat>, and allows a leading [ > <fixed-size> | <fixed-repeat> ] to be omitted. Looks like the existing code also > gets these cases wrong so we could just add a TODO for them. That's right. If we specify <line-names> then there should be a track size declaration as well. I agree it should be fixed after the move. https://codereview.chromium.org/1816253002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3192: if (!consumeGridTrackRepeatFunction(range, cssParserMode, *values, isAutoRepeat) || (isAutoRepeat && seenAutoRepeat)) I would prefer this to be in two different conditions because consumeGridTrackRepeatFunction() modifies the value of isAutoRepeat (someone "optimizing" the code could wrongly decide to put the second condition in the first place with the hope of having cheaper checks)
On 2016/03/29 08:27:37, svillar wrote: > Almost there. Good :) > BTW aren't you removing the code in the legacy parser? Or will it be done later? There are still some shorthands relying on this code, but once they are ported (soon!), then yes I can remove this code. In fact by that time we should be able to remove the complete LegacyCSSPropertyParser.cpp.
On 2016/03/29 08:27:37, svillar wrote: > https://codereview.chromium.org/1816253002/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3192: if > (!consumeGridTrackRepeatFunction(range, cssParserMode, *values, isAutoRepeat) || > (isAutoRepeat && seenAutoRepeat)) > I would prefer this to be in two different conditions because > consumeGridTrackRepeatFunction() modifies the value of isAutoRepeat (someone > "optimizing" the code could wrongly decide to put the second condition in the > first place with the hope of having cheaper checks) Done.
PTAL. https://codereview.chromium.org/1816253002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/1816253002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3149: while (!args.atEnd()) { On 2016/03/29 02:47:39, Timothy Loh wrote: > Not sure what you mean exactly :/ Sorry, I meant I think we still need numberOfTracks, since it does not include gridline names, whereas repeatedValues->length() could include them. https://codereview.chromium.org/1816253002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/1816253002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3112: rangeCopy.consumeIncludingWhitespace(); // Skip '['. On 2016/03/29 02:47:39, Timothy Loh wrote: > Not a fan of the two comments here, at least because they're using different > words. Maybe just declare rangeCopy first, and then change both checks to > > if (range.consumeIncludingWhitespace().type() != XBracketToken) > return nullptr; Done. https://codereview.chromium.org/1816253002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3181: if (range.peek().id() == CSSValueNone) On 2016/03/29 02:47:39, Timothy Loh wrote: > I think you forgot to remove this from here since you added it to > consumeGridTemplatesRowsOrColumns Done. https://codereview.chromium.org/1816253002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3189: do { On 2016/03/29 02:47:39, Timothy Loh wrote: > This allows <line-names> directly before <auto-repeat>, and allows a leading [ > <fixed-size> | <fixed-repeat> ] to be omitted. Looks like the existing code also > gets these cases wrong so we could just add a TODO for them. Done. https://codereview.chromium.org/1816253002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3192: if (!consumeGridTrackRepeatFunction(range, cssParserMode, *values, isAutoRepeat) || (isAutoRepeat && seenAutoRepeat)) On 2016/03/29 08:27:36, svillar wrote: > I would prefer this to be in two different conditions because > consumeGridTrackRepeatFunction() modifies the value of isAutoRepeat (someone > "optimizing" the code could wrongly decide to put the second condition in the > first place with the hope of having cheaper checks) Done.
lgtm https://codereview.chromium.org/1816253002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/1816253002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3189: do { On 2016/03/29 21:15:51, rwlbuis wrote: > On 2016/03/29 02:47:39, Timothy Loh wrote: > > This allows <line-names> directly before <auto-repeat>, and allows a leading [ > > <fixed-size> | <fixed-repeat> ] to be omitted. Looks like the existing code > also > > gets these cases wrong so we could just add a TODO for them. > > Done. Not sure the TODO is accurate, or I'm misunderstanding it. I would've written something like "<line-names> should not be able to directly precede <auto-repeat>"
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/1816253002/#ps200001 (title: "Patch for landing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1816253002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1816253002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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/1816253002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1816253002/200001
Message was sent while issue was closed.
Description was changed from ========== Move grid-template-columns/grid-template-rows into CSSPropertyParser Move the grid-template-columns/grid-template-rows longhands from LegacyCSSPropertyParser into CSSPropertyParser. BUG=499780 ========== to ========== Move grid-template-columns/grid-template-rows into CSSPropertyParser Move the grid-template-columns/grid-template-rows longhands from LegacyCSSPropertyParser into CSSPropertyParser. BUG=499780 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Move grid-template-columns/grid-template-rows into CSSPropertyParser Move the grid-template-columns/grid-template-rows longhands from LegacyCSSPropertyParser into CSSPropertyParser. BUG=499780 ========== to ========== Move grid-template-columns/grid-template-rows into CSSPropertyParser Move the grid-template-columns/grid-template-rows longhands from LegacyCSSPropertyParser into CSSPropertyParser. BUG=499780 Committed: https://crrev.com/92e2a550b54002eaf9307642f534db1625f0646d Cr-Commit-Position: refs/heads/master@{#383935} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/92e2a550b54002eaf9307642f534db1625f0646d Cr-Commit-Position: refs/heads/master@{#383935} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
