Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1085)

Issue 2762623002: [css-align] Adapt the place-content shorthand to the new baseline syntax (Closed)

Created:
3 years, 9 months ago by jfernandez
Modified:
3 years, 8 months ago
Reviewers:
meade_UTC10, Bugs Nash
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
Unified diffs Side-by-side diffs Delta from patch set Stats (+52 lines, -38 lines) Patch
M third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp View 1 2 1 chunk +6 lines, -27 lines 0 comments Download
M third_party/WebKit/Source/core/css/properties/CSSPropertyAlignmentUtils.h View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/core/css/properties/CSSPropertyAlignmentUtils.cpp View 1 2 3 chunks +45 lines, -11 lines 2 comments Download

Messages

Total messages: 25 (9 generated)
jfernandez
PTAL
3 years, 9 months ago (2017-03-20 10:20:14 UTC) #3
meade_UTC10
+bugsnash - you've been in this code recently, could you also PTAL?
3 years, 9 months ago (2017-03-28 00:20:30 UTC) #6
Bugs Nash
On 2017/03/28 at 00:20:30, meade wrote: > +bugsnash - you've been in this code recently, ...
3 years, 8 months ago (2017-03-29 02:00:48 UTC) #7
Bugs Nash
https://codereview.chromium.org/2762623002/diff/20001/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (left): https://codereview.chromium.org/2762623002/diff/20001/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp#oldcode3304 third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3304: if (identMatches<CSSValueNormal, CSSValueBaseline, CSSValueLastBaseline>( should CSSValueLastBaseline be removed? it's ...
3 years, 8 months ago (2017-03-29 02:01:01 UTC) #8
jfernandez
On 2017/03/29 02:00:48, Bugs Nash wrote: > On 2017/03/28 at 00:20:30, meade wrote: > > ...
3 years, 8 months ago (2017-03-29 08:23:49 UTC) #9
jfernandez
Submitted a new patch addressing the review comments and suggestions. Please, take a another look. ...
3 years, 8 months ago (2017-03-29 10:43:10 UTC) #10
Bugs Nash
lgtm with clarification question The removal of code duplication and addition of DCHECKs in this ...
3 years, 8 months ago (2017-03-29 21:52:11 UTC) #11
jfernandez
Thanks for the review. https://codereview.chromium.org/2762623002/diff/40001/third_party/WebKit/Source/core/css/properties/CSSPropertyAlignmentUtils.cpp File third_party/WebKit/Source/core/css/properties/CSSPropertyAlignmentUtils.cpp (right): https://codereview.chromium.org/2762623002/diff/40001/third_party/WebKit/Source/core/css/properties/CSSPropertyAlignmentUtils.cpp#newcode167 third_party/WebKit/Source/core/css/properties/CSSPropertyAlignmentUtils.cpp:167: if (CSSPropertyParserHelpers::identMatches<CSSValueNormal>(id) || On 2017/03/29 ...
3 years, 8 months ago (2017-03-29 22:03:51 UTC) #12
Bugs Nash
On 2017/03/29 at 22:03:51, jfernandez wrote: > Thanks for the review. > > https://codereview.chromium.org/2762623002/diff/40001/third_party/WebKit/Source/core/css/properties/CSSPropertyAlignmentUtils.cpp > ...
3 years, 8 months ago (2017-03-29 23:03:43 UTC) #13
jfernandez
On 2017/03/29 23:03:43, Bugs Nash wrote: > On 2017/03/29 at 22:03:51, jfernandez wrote: > > ...
3 years, 8 months ago (2017-03-30 00:01:32 UTC) #14
Bugs Nash
On 2017/03/30 at 00:01:32, jfernandez wrote: > On 2017/03/29 23:03:43, Bugs Nash wrote: > > ...
3 years, 8 months ago (2017-03-30 00:04:37 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2762623002/40001
3 years, 8 months ago (2017-03-30 00:17:48 UTC) #17
commit-bot: I haz the power
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the ...
3 years, 8 months ago (2017-03-30 00:17:50 UTC) #19
meade_UTC10
lgtm
3 years, 8 months ago (2017-03-30 00:38:26 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2762623002/40001
3 years, 8 months ago (2017-03-30 00:41:55 UTC) #22
commit-bot: I haz the power
3 years, 8 months ago (2017-03-30 02:53:02 UTC) #25
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/600d3038e72aa2a0b9bb94a4bd92...

Powered by Google App Engine
This is Rietveld 408576698