|
|
Created:
3 years, 9 months ago by jfernandez Modified:
3 years, 8 months ago CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, chromium-reviews, dglazkov+blink, rwlbuis Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[css-align] Adapt the place-content shorthand to the new baseline syntax
Now that the align-content and justify-content CSS properties are
adapted to the new baseline-position CSS values syntax we can adapt the
shorthand that controls such properties to the new syntax as well.
In addition, this patch makes public some of the utility functions
defined in the CSSPropertiesAlignmentUtils class, so that they can be
shared between the different parsing and computed style logic of the
different alignment related CSS properties.
BUG=703584
Review-Url: https://codereview.chromium.org/2762623002
Cr-Commit-Position: refs/heads/master@{#460630}
Committed: https://chromium.googlesource.com/chromium/src/+/600d3038e72aa2a0b9bb94a4bd9230226bd662a6
Patch Set 1 #Patch Set 2 : Additional refactoring to deal with baseline value ID #
Total comments: 11
Patch Set 3 : Keep utility functions private and make consumeSimplifiedXXX public #
Total comments: 2
Messages
Total messages: 25 (9 generated)
Description was changed from ========== [css-align] Adapt the place-content shorthand to the new baseline syntax Now that the align-content and justify-content CSS properties are adapted to the new baseline-position CSS values syntax we can adapt the shorthand that controls such properties to the new syntax as well. In addition, this patch makes public some of the utility functions defined in the CSSPropertiesAlignmentUtils class, so that they can be shared between the different parsing and computed style logic of the different alignment related CSS properties. BUG=565883 ========== to ========== [css-align] Adapt the place-content shorthand to the new baseline syntax Now that the align-content and justify-content CSS properties are adapted to the new baseline-position CSS values syntax we can adapt the shorthand that controls such properties to the new syntax as well. In addition, this patch makes public some of the utility functions defined in the CSSPropertiesAlignmentUtils class, so that they can be shared between the different parsing and computed style logic of the different alignment related CSS properties. BUG=565883 ==========
jfernandez@igalia.com changed reviewers: + meade@chromium.org, sashab@chromium.org
PTAL
Description was changed from ========== [css-align] Adapt the place-content shorthand to the new baseline syntax Now that the align-content and justify-content CSS properties are adapted to the new baseline-position CSS values syntax we can adapt the shorthand that controls such properties to the new syntax as well. In addition, this patch makes public some of the utility functions defined in the CSSPropertiesAlignmentUtils class, so that they can be shared between the different parsing and computed style logic of the different alignment related CSS properties. BUG=565883 ========== to ========== [css-align] Adapt the place-content shorthand to the new baseline syntax Now that the align-content and justify-content CSS properties are adapted to the new baseline-position CSS values syntax we can adapt the shorthand that controls such properties to the new syntax as well. In addition, this patch makes public some of the utility functions defined in the CSSPropertiesAlignmentUtils class, so that they can be shared between the different parsing and computed style logic of the different alignment related CSS properties. BUG=703584 ==========
meade@chromium.org changed reviewers: + bugsnash@chromium.org - sashab@chromium.org
+bugsnash - you've been in this code recently, could you also PTAL?
On 2017/03/28 at 00:20:30, meade wrote: > +bugsnash - you've been in this code recently, could you also PTAL? Rather than making all those methods public I think it makes more sense to move the consumeSimplifiedContentPosition method into the utils file and make that method public. The naming and structure of those utils files might need a rethink...
https://codereview.chromium.org/2762623002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (left): https://codereview.chromium.org/2762623002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3304: if (identMatches<CSSValueNormal, CSSValueBaseline, CSSValueLastBaseline>( should CSSValueLastBaseline be removed? it's not covered by CSSPropertyAlignmentUtils::isBaselineKeyword https://codereview.chromium.org/2762623002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/2762623002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3312: CSSPropertyAlignmentUtils::consumeBaselineKeyword(range); consumeBaselineKeyword doesn't consume whitespace, will that be an issue here? https://codereview.chromium.org/2762623002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3326: if (CSSPropertyAlignmentUtils::isContentPositionKeyword(id)) { this should be combined with the if block above that checks for CSSValueNormal https://codereview.chromium.org/2762623002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/properties/CSSPropertyAlignmentUtils.h (right): https://codereview.chromium.org/2762623002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyAlignmentUtils.h:22: static bool isOverflowKeyword(CSSValueID); this one doesn't need to be made public either way https://codereview.chromium.org/2762623002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyAlignmentUtils.h:23: static CSSIdentifierValue* consumeSelfPositionKeyword(CSSParserTokenRange&); as above
On 2017/03/29 02:00:48, Bugs Nash wrote: > On 2017/03/28 at 00:20:30, meade wrote: > > +bugsnash - you've been in this code recently, could you also PTAL? > > Rather than making all those methods public I think it makes more sense to move > the consumeSimplifiedContentPosition method into the utils file and make that > method public. The naming and structure of those utils files might need a > rethink... yes, I agree it's a better solution. I'll provide a new patch following that approach.
Submitted a new patch addressing the review comments and suggestions. Please, take a another look. https://codereview.chromium.org/2762623002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (left): https://codereview.chromium.org/2762623002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3304: if (identMatches<CSSValueNormal, CSSValueBaseline, CSSValueLastBaseline>( On 2017/03/29 02:01:01, Bugs Nash wrote: > should CSSValueLastBaseline be removed? it's not covered by > CSSPropertyAlignmentUtils::isBaselineKeyword Actually not. It's debatable whether we need a different approach for the general parsing, though. The issue is that the spec changed the baseline values from "last-baseline" to "last baseline", so instead of having single CSSValue to describe the baseline behavior we have two now. I thought that it could be a good idea to keep the CSSPrimitiveValue <==> ComputedStyleConstant mapping, specially considering this change in the syntax just affects to "last-baseline", since "first baseline" (new) is equivalent to "baseline" So, the answer is that we do need to keep CSSValueLastBaseline but it won 't be allowed as an input, from the CSS parsing point of view. We will use is just for generating the ComputedStyle representation. https://codereview.chromium.org/2762623002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/2762623002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3312: CSSPropertyAlignmentUtils::consumeBaselineKeyword(range); On 2017/03/29 02:01:01, Bugs Nash wrote: > consumeBaselineKeyword doesn't consume whitespace, will that be an issue here? I'm not sure I understand this comment. consumeBaselineKeyword uses the function CSSPropertyParserHelpers::consumeIdent(range), which as far as I understand, it consumes whitespace. https://codereview.chromium.org/2762623002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3326: if (CSSPropertyAlignmentUtils::isContentPositionKeyword(id)) { On 2017/03/29 02:01:01, Bugs Nash wrote: > this should be combined with the if block above that checks for CSSValueNormal Done. https://codereview.chromium.org/2762623002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/properties/CSSPropertyAlignmentUtils.h (right): https://codereview.chromium.org/2762623002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyAlignmentUtils.h:22: static bool isOverflowKeyword(CSSValueID); On 2017/03/29 02:01:01, Bugs Nash wrote: > this one doesn't need to be made public either way Ok. https://codereview.chromium.org/2762623002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyAlignmentUtils.h:23: static CSSIdentifierValue* consumeSelfPositionKeyword(CSSParserTokenRange&); On 2017/03/29 02:01:01, Bugs Nash wrote: > as above Ok, but I think we will need it for implementing the place-items and place-self shorthand. Anyway, I agree on keeping it as private for now.
lgtm with clarification question The removal of code duplication and addition of DCHECKs in this patch is great, keep up the good code health work https://codereview.chromium.org/2762623002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/2762623002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3312: CSSPropertyAlignmentUtils::consumeBaselineKeyword(range); On 2017/03/29 at 10:43:10, jfernandez wrote: > On 2017/03/29 02:01:01, Bugs Nash wrote: > > consumeBaselineKeyword doesn't consume whitespace, will that be an issue here? > > I'm not sure I understand this comment. consumeBaselineKeyword uses the > function CSSPropertyParserHelpers::consumeIdent(range), which as far as I understand, it consumes whitespace. cool np https://codereview.chromium.org/2762623002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/properties/CSSPropertyAlignmentUtils.cpp (right): https://codereview.chromium.org/2762623002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyAlignmentUtils.cpp:167: if (CSSPropertyParserHelpers::identMatches<CSSValueNormal>(id) || Didn't you say you were going to keep CSSValueLastBaseline here? Or did I misunderstand your last comment?
Thanks for the review. https://codereview.chromium.org/2762623002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/properties/CSSPropertyAlignmentUtils.cpp (right): https://codereview.chromium.org/2762623002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyAlignmentUtils.cpp:167: if (CSSPropertyParserHelpers::identMatches<CSSValueNormal>(id) || On 2017/03/29 21:52:11, Bugs Nash wrote: > Didn't you say you were going to keep CSSValueLastBaseline here? Or did I > misunderstand your last comment? Probably I didn't explain it properly. I thought you were suggesting that since CSSValueLastBaseline was handled by isBaselineKeyword we could remove the keyword itself. My answer was that I do want to keep the keyword, as it will be used for generating the computed style value. The spec doesn't allow "last-baseline" now, and it specifies "last baseline" instead; however, we still need the old CSSValueLastBaseline for other purposes. We don't need to use it in the identMatches clause above, though. The allowed baseline syntax for CSS parsing is handled below by isBaselineKeyword.
On 2017/03/29 at 22:03:51, jfernandez wrote: > Thanks for the review. > > https://codereview.chromium.org/2762623002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/css/properties/CSSPropertyAlignmentUtils.cpp (right): > > https://codereview.chromium.org/2762623002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/css/properties/CSSPropertyAlignmentUtils.cpp:167: if (CSSPropertyParserHelpers::identMatches<CSSValueNormal>(id) || > On 2017/03/29 21:52:11, Bugs Nash wrote: > > Didn't you say you were going to keep CSSValueLastBaseline here? Or did I > > misunderstand your last comment? > > Probably I didn't explain it properly. I thought you were suggesting that since CSSValueLastBaseline was handled by isBaselineKeyword we > could remove the keyword itself. My answer was that I do want to keep the keyword, as it will be used for generating the computed style value. No, I was saying that isBaselineKeyword *doesn't* handle CSSValueLastBaseline. It handles CSSValueBaseline, but not CSSValueLastBaseline > The spec doesn't allow "last-baseline" now, and it specifies "last baseline" instead; however, we still need the old > CSSValueLastBaseline for other purposes. > > We don't need to use it in the identMatches clause above, though. The allowed baseline syntax for CSS parsing is handled below by isBaselineKeyword.
On 2017/03/29 23:03:43, Bugs Nash wrote: > On 2017/03/29 at 22:03:51, jfernandez wrote: > > Thanks for the review. > > > > > https://codereview.chromium.org/2762623002/diff/40001/third_party/WebKit/Sour... > > File > third_party/WebKit/Source/core/css/properties/CSSPropertyAlignmentUtils.cpp > (right): > > > > > https://codereview.chromium.org/2762623002/diff/40001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/css/properties/CSSPropertyAlignmentUtils.cpp:167: > if (CSSPropertyParserHelpers::identMatches<CSSValueNormal>(id) || > > On 2017/03/29 21:52:11, Bugs Nash wrote: > > > Didn't you say you were going to keep CSSValueLastBaseline here? Or did I > > > misunderstand your last comment? > > > > Probably I didn't explain it properly. I thought you were suggesting that > since CSSValueLastBaseline was handled by isBaselineKeyword we > > could remove the keyword itself. My answer was that I do want to keep the > keyword, as it will be used for generating the computed style value. > > No, I was saying that isBaselineKeyword *doesn't* handle CSSValueLastBaseline. > It handles CSSValueBaseline, but not CSSValueLastBaseline > Ok, got it. But that's intentionally and correct. We don't want to allow CSSValueLastBaseline as a valid input for parsing.
On 2017/03/30 at 00:01:32, jfernandez wrote: > On 2017/03/29 23:03:43, Bugs Nash wrote: > > On 2017/03/29 at 22:03:51, jfernandez wrote: > > > Thanks for the review. > > > > > > > > https://codereview.chromium.org/2762623002/diff/40001/third_party/WebKit/Sour... > > > File > > third_party/WebKit/Source/core/css/properties/CSSPropertyAlignmentUtils.cpp > > (right): > > > > > > > > https://codereview.chromium.org/2762623002/diff/40001/third_party/WebKit/Sour... > > > > > third_party/WebKit/Source/core/css/properties/CSSPropertyAlignmentUtils.cpp:167: > > if (CSSPropertyParserHelpers::identMatches<CSSValueNormal>(id) || > > > On 2017/03/29 21:52:11, Bugs Nash wrote: > > > > Didn't you say you were going to keep CSSValueLastBaseline here? Or did I > > > > misunderstand your last comment? > > > > > > Probably I didn't explain it properly. I thought you were suggesting that > > since CSSValueLastBaseline was handled by isBaselineKeyword we > > > could remove the keyword itself. My answer was that I do want to keep the > > keyword, as it will be used for generating the computed style value. > > > > No, I was saying that isBaselineKeyword *doesn't* handle CSSValueLastBaseline. > > It handles CSSValueBaseline, but not CSSValueLastBaseline > > > > Ok, got it. But that's intentionally and correct. We don't want to allow > CSSValueLastBaseline as a valid input for parsing. sgtm
The CQ bit was checked by jfernandez@igalia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
lgtm
The CQ bit was checked by jfernandez@igalia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490834437971820, "parent_rev": "73bcfa377fd851623f577ef447c281f2c81866b6", "commit_rev": "600d3038e72aa2a0b9bb94a4bd9230226bd662a6"}
Message was sent while issue was closed.
Description was changed from ========== [css-align] Adapt the place-content shorthand to the new baseline syntax Now that the align-content and justify-content CSS properties are adapted to the new baseline-position CSS values syntax we can adapt the shorthand that controls such properties to the new syntax as well. In addition, this patch makes public some of the utility functions defined in the CSSPropertiesAlignmentUtils class, so that they can be shared between the different parsing and computed style logic of the different alignment related CSS properties. BUG=703584 ========== to ========== [css-align] Adapt the place-content shorthand to the new baseline syntax Now that the align-content and justify-content CSS properties are adapted to the new baseline-position CSS values syntax we can adapt the shorthand that controls such properties to the new syntax as well. In addition, this patch makes public some of the utility functions defined in the CSSPropertiesAlignmentUtils class, so that they can be shared between the different parsing and computed style logic of the different alignment related CSS properties. BUG=703584 Review-Url: https://codereview.chromium.org/2762623002 Cr-Commit-Position: refs/heads/master@{#460630} Committed: https://chromium.googlesource.com/chromium/src/+/600d3038e72aa2a0b9bb94a4bd92... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/600d3038e72aa2a0b9bb94a4bd92... |