Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2888283006:
CSS: Use count position values with 3 parts (Closed)
|
Created:
3 years, 7 months ago by Eric Willigers Modified:
3 years, 7 months ago CC:
darktears, apavlov+blink_chromium.org, asvitkine+watch_chromium.org, blink-reviews, blink-reviews-css, blink-reviews-frames_chromium.org, chromium-reviews, dglazkov+blink, rwlbuis Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionCSS: Use count position values with 3 parts
The CSS Working Group recently changed the grammar of <position> such
that 3-value syntax is no longer supported.
https://lists.w3.org/Archives/Public/www-style/2017Mar/0054.html
We update parsing logic so that the following now use count three value
positions, in preparation for future deprecation and removal discussions:
object-position https://drafts.csswg.org/css-images-3/#the-object-position
perspective-origin
https://drafts.csswg.org/css-transforms-2/#perspective-origin-property
scroll-snap-coordinate (not shipped, not in current spec)
https://www.w3.org/TR/2015/WD-css-snappoints-1-20150326/#scroll-snap-coordinate
scroll-snap-destination (not shipped, not in current spec)
https://www.w3.org/TR/2015/WD-css-snappoints-1-20150326/#scroll-snap-destination
center position for circle and ellipse basic shapes
https://drafts.csswg.org/css-shapes/#supported-basic-shapes
(used in https://www.w3.org/TR/css-shapes-1/#shape-outside-property and
https://drafts.fxtf.org/css-masking-1/#the-clip-path and
https://drafts.fxtf.org/motion-1/#offset-path-property )
position for radial and (not shipped) conic gradients
https://drafts.csswg.org/css-images-3/#radial-gradients
https://drafts.csswg.org/css-images-4/#conic-gradients
(used in https://drafts.csswg.org/css-backgrounds-3/#the-background-image )
The following no longer accept three value positions:
offset-anchor (property not yet shipped, expecting to ship soon)
https://drafts.fxtf.org/motion-1/#offset-anchor-property
offset-position (property not yet shipped, expecting to ship soon)
https://drafts.fxtf.org/motion-1/#offset-position-property
The following are not affected:
background-position
https://drafts.csswg.org/css-backgrounds-3/#background-position
(continues to support three values)
transform-origin
https://drafts.csswg.org/css-transforms/#transform-origin-property
(has never used <position>, instead defines its own grammar)
BUG=717833
Review-Url: https://codereview.chromium.org/2888283006
Cr-Commit-Position: refs/heads/master@{#474358}
Committed: https://chromium.googlesource.com/chromium/src/+/a951d7b5e8647b077be37f420254b73063a30b4c
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : distinct use counters #
Total comments: 4
Patch Set 4 : rebase, add use counter for backgroud-position #Patch Set 5 : use count in the property APIs #
Total comments: 5
Patch Set 6 : use counter feature as argument #
Total comments: 2
Patch Set 7 : two color-stop for conic-gradient example #
Total comments: 10
Patch Set 8 : kDisallowThreeValuedPosition #Patch Set 9 : rebase #
Total comments: 2
Patch Set 10 : extra argument #
Total comments: 4
Patch Set 11 : rebase #
Total comments: 1
Patch Set 12 : optional #Patch Set 13 : rebase #Patch Set 14 : CSSPropertyAPIScrollSnapCoordinate #Patch Set 15 : rebase #
Total comments: 1
Messages
Total messages: 99 (61 generated)
Description was changed from ========== CSS: Use count position values with 3 parts The CSS Working Group recently changed the grammar of <position> such that 3-value syntax is no longer supported. We update parsing logic so that the following now use count three value positions, in preparation for future deprecation and removal discussions: object-position https://drafts.csswg.org/css-images-3/#the-object-position perspective-origin https://drafts.csswg.org/css-transforms-2/#perspective-origin-property scroll-snap-coordinate (not shipped, not in current spec) https://www.w3.org/TR/2015/WD-css-snappoints-1-20150326/#scroll-snap-coordinate scroll-snap-destination (not shipped, not in current spec) https://www.w3.org/TR/2015/WD-css-snappoints-1-20150326/#scroll-snap-destination center position for circle and ellipse basic shapes https://drafts.csswg.org/css-shapes/#supported-basic-shapes (used in https://www.w3.org/TR/css-shapes-1/#shape-outside-property and https://drafts.fxtf.org/css-masking-1/#the-clip-path and https://drafts.fxtf.org/motion-1/#offset-path-property ) position for radial and (not shipped) conic gradients https://drafts.csswg.org/css-images-3/#radial-gradients https://drafts.csswg.org/css-images-4/#conic-gradients (used in https://drafts.csswg.org/css-backgrounds-3/#the-background-image ) The following no longer accept three value positions: offset-anchor (not shipped, expecting to ship soon) https://drafts.fxtf.org/motion-1/#offset-anchor-property offset-position (not shipped, expecting to ship soon) https://drafts.fxtf.org/motion-1/#offset-position-property The following are not affected: background-position https://drafts.csswg.org/css-backgrounds-3/#background-position (continues to support three values) transform-origin https://drafts.csswg.org/css-transforms/#transform-origin-property (has never used <position>, instead defines its own grammar) BUG=717833 ========== to ========== CSS: Use count position values with 3 parts The CSS Working Group recently changed the grammar of <position> such that 3-value syntax is no longer supported. https://lists.w3.org/Archives/Public/www-style/2017Mar/0054.html We update parsing logic so that the following now use count three value positions, in preparation for future deprecation and removal discussions: object-position https://drafts.csswg.org/css-images-3/#the-object-position perspective-origin https://drafts.csswg.org/css-transforms-2/#perspective-origin-property scroll-snap-coordinate (not shipped, not in current spec) https://www.w3.org/TR/2015/WD-css-snappoints-1-20150326/#scroll-snap-coordinate scroll-snap-destination (not shipped, not in current spec) https://www.w3.org/TR/2015/WD-css-snappoints-1-20150326/#scroll-snap-destination center position for circle and ellipse basic shapes https://drafts.csswg.org/css-shapes/#supported-basic-shapes (used in https://www.w3.org/TR/css-shapes-1/#shape-outside-property and https://drafts.fxtf.org/css-masking-1/#the-clip-path and https://drafts.fxtf.org/motion-1/#offset-path-property ) position for radial and (not shipped) conic gradients https://drafts.csswg.org/css-images-3/#radial-gradients https://drafts.csswg.org/css-images-4/#conic-gradients (used in https://drafts.csswg.org/css-backgrounds-3/#the-background-image ) The following no longer accept three value positions: offset-anchor (not shipped, expecting to ship soon) https://drafts.fxtf.org/motion-1/#offset-anchor-property offset-position (not shipped, expecting to ship soon) https://drafts.fxtf.org/motion-1/#offset-position-property The following are not affected: background-position https://drafts.csswg.org/css-backgrounds-3/#background-position (continues to support three values) transform-origin https://drafts.csswg.org/css-transforms/#transform-origin-property (has never used <position>, instead defines its own grammar) BUG=717833 ==========
ericwilligers@chromium.org changed reviewers: + bugsnash@chromium.org
The CQ bit was checked by ericwilligers@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ericwilligers@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by ericwilligers@chromium.org to run a CQ dry run
There are currently few parsing tests for offset-position and offset-anchor. More will be added with the landing of https://codereview.chromium.org/2886703002/
Dry run: 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
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2888283006/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/css3/motion-path/offset-position.html (right): https://codereview.chromium.org/2888283006/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/css3/motion-path/offset-position.html:50: }, 'offset-position may not use three values'); The CL description says that offset-position hasn't shipped disallowing 3 values yet, will this test pass? https://codereview.chromium.org/2888283006/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp (right): https://codereview.chromium.org/2888283006/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:769: switch (syntax) { This switch statement kind of introduces property specific logic where we're trying to remove that (from shared methods like this). Why not avoid introducing a new switch and do the use counting in the property APIs themselves? I.e inside the case statements in ParseSingleValue and ParseShorthand as this is what will be ported to the APIs. This would also avoid the need to pass more objects around.
Description was changed from ========== CSS: Use count position values with 3 parts The CSS Working Group recently changed the grammar of <position> such that 3-value syntax is no longer supported. https://lists.w3.org/Archives/Public/www-style/2017Mar/0054.html We update parsing logic so that the following now use count three value positions, in preparation for future deprecation and removal discussions: object-position https://drafts.csswg.org/css-images-3/#the-object-position perspective-origin https://drafts.csswg.org/css-transforms-2/#perspective-origin-property scroll-snap-coordinate (not shipped, not in current spec) https://www.w3.org/TR/2015/WD-css-snappoints-1-20150326/#scroll-snap-coordinate scroll-snap-destination (not shipped, not in current spec) https://www.w3.org/TR/2015/WD-css-snappoints-1-20150326/#scroll-snap-destination center position for circle and ellipse basic shapes https://drafts.csswg.org/css-shapes/#supported-basic-shapes (used in https://www.w3.org/TR/css-shapes-1/#shape-outside-property and https://drafts.fxtf.org/css-masking-1/#the-clip-path and https://drafts.fxtf.org/motion-1/#offset-path-property ) position for radial and (not shipped) conic gradients https://drafts.csswg.org/css-images-3/#radial-gradients https://drafts.csswg.org/css-images-4/#conic-gradients (used in https://drafts.csswg.org/css-backgrounds-3/#the-background-image ) The following no longer accept three value positions: offset-anchor (not shipped, expecting to ship soon) https://drafts.fxtf.org/motion-1/#offset-anchor-property offset-position (not shipped, expecting to ship soon) https://drafts.fxtf.org/motion-1/#offset-position-property The following are not affected: background-position https://drafts.csswg.org/css-backgrounds-3/#background-position (continues to support three values) transform-origin https://drafts.csswg.org/css-transforms/#transform-origin-property (has never used <position>, instead defines its own grammar) BUG=717833 ========== to ========== CSS: Use count position values with 3 parts The CSS Working Group recently changed the grammar of <position> such that 3-value syntax is no longer supported. https://lists.w3.org/Archives/Public/www-style/2017Mar/0054.html We update parsing logic so that the following now use count three value positions, in preparation for future deprecation and removal discussions: object-position https://drafts.csswg.org/css-images-3/#the-object-position perspective-origin https://drafts.csswg.org/css-transforms-2/#perspective-origin-property scroll-snap-coordinate (not shipped, not in current spec) https://www.w3.org/TR/2015/WD-css-snappoints-1-20150326/#scroll-snap-coordinate scroll-snap-destination (not shipped, not in current spec) https://www.w3.org/TR/2015/WD-css-snappoints-1-20150326/#scroll-snap-destination center position for circle and ellipse basic shapes https://drafts.csswg.org/css-shapes/#supported-basic-shapes (used in https://www.w3.org/TR/css-shapes-1/#shape-outside-property and https://drafts.fxtf.org/css-masking-1/#the-clip-path and https://drafts.fxtf.org/motion-1/#offset-path-property ) position for radial and (not shipped) conic gradients https://drafts.csswg.org/css-images-3/#radial-gradients https://drafts.csswg.org/css-images-4/#conic-gradients (used in https://drafts.csswg.org/css-backgrounds-3/#the-background-image ) The following no longer accept three value positions: offset-anchor (property not yet shipped, expecting to ship soon) https://drafts.fxtf.org/motion-1/#offset-anchor-property offset-position (property not yet shipped, expecting to ship soon) https://drafts.fxtf.org/motion-1/#offset-position-property The following are not affected: background-position https://drafts.csswg.org/css-backgrounds-3/#background-position (continues to support three values) transform-origin https://drafts.csswg.org/css-transforms/#transform-origin-property (has never used <position>, instead defines its own grammar) BUG=717833 ==========
The CQ bit was checked by ericwilligers@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Patchset #5 (id:80001) has been deleted
https://codereview.chromium.org/2888283006/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/css3/motion-path/offset-position.html (right): https://codereview.chromium.org/2888283006/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/css3/motion-path/offset-position.html:50: }, 'offset-position may not use three values'); On 2017/05/22 01:12:56, Bugs Nash wrote: > The CL description says that offset-position hasn't shipped disallowing 3 values > yet, will this test pass? I have clarified the CL description. The property has not yet shipped, on any browsers. So there is no compatibility risk in never supporting 3 value syntax. https://codereview.chromium.org/2888283006/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp (right): https://codereview.chromium.org/2888283006/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:769: switch (syntax) { On 2017/05/22 01:12:56, Bugs Nash wrote: > This switch statement kind of introduces property specific logic where we're > trying to remove that (from shared methods like this). Why not avoid introducing > a new switch and do the use counting in the property APIs themselves? I.e inside > the case statements in ParseSingleValue and ParseShorthand as this is what will > be ported to the APIs. This would also avoid the need to pass more objects > around. Done.
On 2017/05/22 at 03:52:06, ericwilligers wrote: > https://codereview.chromium.org/2888283006/diff/40001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/css3/motion-path/offset-position.html (right): > > https://codereview.chromium.org/2888283006/diff/40001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/css3/motion-path/offset-position.html:50: }, 'offset-position may not use three values'); > On 2017/05/22 01:12:56, Bugs Nash wrote: > > The CL description says that offset-position hasn't shipped disallowing 3 values > > yet, will this test pass? > > I have clarified the CL description. The property has not yet shipped, on any browsers. So there is no compatibility risk in never supporting 3 value syntax. Ah I see > > https://codereview.chromium.org/2888283006/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp (right): > > https://codereview.chromium.org/2888283006/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:769: switch (syntax) { > On 2017/05/22 01:12:56, Bugs Nash wrote: > > This switch statement kind of introduces property specific logic where we're > > trying to remove that (from shared methods like this). Why not avoid introducing > > a new switch and do the use counting in the property APIs themselves? I.e inside > > the case statements in ParseSingleValue and ParseShorthand as this is what will > > be ported to the APIs. This would also avoid the need to pass more objects > > around. > > Done.
https://codereview.chromium.org/2888283006/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp (right): https://codereview.chromium.org/2888283006/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:720: const CSSParserContext& context, this can remain as just the parser mode https://codereview.chromium.org/2888283006/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:773: case PositionSyntax::kCountThreeValues: will this case ever be hit? if not, perhaps an if/else is more appropriate https://codereview.chromium.org/2888283006/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:799: const CSSParserContext& context, can remain as parser mode https://codereview.chromium.org/2888283006/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h (right): https://codereview.chromium.org/2888283006/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h:43: kCountThreeValues the purpose of this enum is unclear to me, it seems to be confounded as both an input of whether 3 values are allowed, and an output of whether 3 values were read. if that's the case a comment to that effect would be useful.
On 2017/05/22 at 04:36:10, Bugs Nash wrote: > https://codereview.chromium.org/2888283006/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp (right): > > https://codereview.chromium.org/2888283006/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:720: const CSSParserContext& context, > this can remain as just the parser mode > > https://codereview.chromium.org/2888283006/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:773: case PositionSyntax::kCountThreeValues: > will this case ever be hit? if not, perhaps an if/else is more appropriate > > https://codereview.chromium.org/2888283006/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:799: const CSSParserContext& context, > can remain as parser mode > > https://codereview.chromium.org/2888283006/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h (right): > > https://codereview.chromium.org/2888283006/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h:43: kCountThreeValues > the purpose of this enum is unclear to me, it seems to be confounded as both an input of whether 3 values are allowed, and an output of whether 3 values were read. if that's the case a comment to that effect would be useful. The previous design with the property specific switch did seem a lot cleaner than the new design. Although it's not ideal creating a new enum that mirrors properties, it's ok with me if you want to stick with that.
https://codereview.chromium.org/2888283006/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h (right): https://codereview.chromium.org/2888283006/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h:43: kCountThreeValues On 2017/05/22 04:36:10, Bugs Nash wrote: > the purpose of this enum is unclear to me, it seems to be confounded as both an > input of whether 3 values are allowed, and an output of whether 3 values were > read. if that's the case a comment to that effect would be useful. Changed to new approach where the use counter feature is passed in. I've changed the scroll snap points properties to reject 3 value positions. This is consistent with the spec, and has no compatibility impact as these properties have not shipped.
The CQ bit was checked by ericwilligers@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
yiorsi@gmail.com changed reviewers: + Yiorsi@gmail.com
https://codereview.chromium.org/2888283006/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/css/usecounter-position3value-conic-gradient.html (right): https://codereview.chromium.org/2888283006/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/css/usecounter-position3value-conic-gradient.html:19: div.style = 'background-image: conic-gradient(at left 10% top, black);'; conic-gradient not support one color-stop.
ericwilligers@chromium.org changed reviewers: + yiorsi@gmail.com - Yiorsi@gmail.com
https://codereview.chromium.org/2888283006/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/css/usecounter-position3value-conic-gradient.html (right): https://codereview.chromium.org/2888283006/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/css/usecounter-position3value-conic-gradient.html:19: div.style = 'background-image: conic-gradient(at left 10% top, black);'; On 2017/05/22 07:15:19, Yiorsi wrote: > conic-gradient not support one color-stop. Changed.
yeahya! i like this version a lot more. lgtm with nits https://codereview.chromium.org/2888283006/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/css/scroll-snap-parsing.html (right): https://codereview.chromium.org/2888283006/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/css/scroll-snap-parsing.html:48: <div class="test" property="scroll-snap-destination" style="scroll-snap-destination: left top 50%;" expected="0px 0px" desc="3 piece percentage destination" ></div> why is this expected to change? isn't scroll-snap-destination 3 value position still valid? https://codereview.chromium.org/2888283006/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/css/scroll-snap-parsing.html:53: <div class="test" property="scroll-snap-coordinate" style="scroll-snap-coordinate: left top 50%;" expected="none" desc="3 piece percentage coordinate" ></div> as above https://codereview.chromium.org/2888283006/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/css/usecounter-position3value-background-position.html (right): https://codereview.chromium.org/2888283006/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/css/usecounter-position3value-background-position.html:8: let ThreeValuedPositionBackgroundPosition = 1983; // From UseCounter.h as above https://codereview.chromium.org/2888283006/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/css/usecounter-position3value-background.html (right): https://codereview.chromium.org/2888283006/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/css/usecounter-position3value-background.html:8: let ThreeValuedPositionBackgroundPosition = 1983; // From UseCounter.h should this be named differently from UseCounter.h? https://codereview.chromium.org/2888283006/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/css/usecounter-position3value-background.html:12: some tests make sure that other properties with their own use counters don't trigger the use counter in question, some don't (presumably because this would be a redundant check with some other test on the same use counter). the inconsistency is a bit confusing, I would suggest one of the following 1) comment in the tests that do not include this check linking the test that does do this check 2) split out the negative tests into a separate file 3) not sure if this one is possible - bring all the tests for a single use counter into the one file. would require ability to clear the use counter, which may not be possible? https://codereview.chromium.org/2888283006/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/2888283006/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1652: UseCounter::kNumberOfFeatures); for readability, could we create a named constant for this called disallowThreeValuedPosition or similar? It could live in CSSPropertyParserHelpers with ConsumePosition
https://codereview.chromium.org/2888283006/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/css/scroll-snap-parsing.html (right): https://codereview.chromium.org/2888283006/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/css/scroll-snap-parsing.html:48: <div class="test" property="scroll-snap-destination" style="scroll-snap-destination: left top 50%;" expected="0px 0px" desc="3 piece percentage destination" ></div> On 2017/05/22 23:07:08, Bugs Nash wrote: > why is this expected to change? isn't scroll-snap-destination 3 value position > still valid? We would have needed to add a use counter or misuse another use counter, and I don't either is worthwhile. We hope the two properties (that never shipped) will be deleted from the codebase in a couple of weeks. https://codereview.chromium.org/2888283006/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/css/usecounter-position3value-background.html (right): https://codereview.chromium.org/2888283006/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/css/usecounter-position3value-background.html:8: let ThreeValuedPositionBackgroundPosition = 1983; // From UseCounter.h On 2017/05/22 23:07:09, Bugs Nash wrote: > should this be named differently from UseCounter.h? I have copied the convention from other use counter tests. cd third_party/WebKit/LayoutTests ; git grep "UseCounter.h" shows many similar cases. git grep " k[A-Z]" `git grep -H isUseCounted | cut -d':' -f1 | sort -u` shows only one example where the name from UseCounter.h was used. https://codereview.chromium.org/2888283006/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/css/usecounter-position3value-background.html:12: On 2017/05/22 23:07:09, Bugs Nash wrote: > some tests make sure that other properties with their own use counters don't > trigger the use counter in question, some don't (presumably because this would > be a redundant check with some other test on the same use counter). the > inconsistency is a bit confusing, I would suggest one of the following > 1) comment in the tests that do not include this check linking the test that > does do this check > 2) split out the negative tests into a separate file > 3) not sure if this one is possible - bring all the tests for a single use > counter into the one file. would require ability to clear the use counter, which > may not be possible? Comments added. clearing the use counter is not possible AFAIK. https://codereview.chromium.org/2888283006/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/2888283006/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1652: UseCounter::kNumberOfFeatures); On 2017/05/22 23:07:09, Bugs Nash wrote: > for readability, could we create a named constant for this called > disallowThreeValuedPosition or similar? It could live in > CSSPropertyParserHelpers with ConsumePosition Done.
The CQ bit was checked by ericwilligers@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by ericwilligers@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bugsnash@chromium.org Link to the patchset: https://codereview.chromium.org/2888283006/#ps180001 (title: "rebase")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
ericwilligers@chromium.org changed reviewers: + meade@chromium.org
+meade for OWNERS
https://codereview.chromium.org/2898133002 is similar.
suzyh@chromium.org changed reviewers: + suzyh@chromium.org
I think you're using UseCounter::Feature for two different purposes here. I am worried that the resulting code is unclear and highly fragile, particularly about using UseCounter::Feature to determine when three values is disallowed. I appreciate that you need to get the UseCounters in ASAP but am reluctant to give an OWNERS approval to this as it is. I think you need a separate enum that captures the parsing-side concept, and then some code to determine what UseCounter to increment when parsing. I have not carefully reviewed the tests. https://codereview.chromium.org/2888283006/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h (right): https://codereview.chromium.org/2888283006/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h:82: UseCounter::Feature::kNumberOfFeatures; I don't understand this, and it seems fragile because it is going to change every time someone adds a new feature to UseCounter. What's going on here?
https://codereview.chromium.org/2888283006/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h (right): https://codereview.chromium.org/2888283006/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h:82: UseCounter::Feature::kNumberOfFeatures; On 2017/05/23 23:26:54, suzyh_UTC10 wrote: > I don't understand this, and it seems fragile because it is going to change > every time someone adds a new feature to UseCounter. What's going on here? A recap of this code review: Version #3 of this CL defined an enum to specify the type of position being parsed, and a switch to select the appropriate use counter: https://codereview.chromium.org/2888283006/patch/40001/50009 https://codereview.chromium.org/2888283006/patch/40001/50008 This was unfortunately in the opposite direction from moving property specific logic out of CSSPropertyParserHelpers. Version #5 moved to simply indicating if 3 values are permitted or not, and letting the caller update the use counter https://codereview.chromium.org/2888283006/patch/100001/110012 That received "The previous design with the property specific switch did seem a lot cleaner than the new design." Version #6 lets the caller pass in the specific use counter for when 3 value is received. If there is no use counter, because 3 value is not valid, we pass the non-use-counter kNumberOfFeatures https://codereview.chromium.org/2888283006/patch/140001/150014 https://codereview.chromium.org/2888283006/diff/140001/third_party/WebKit/Sou... "for readability, could we create a named constant for this called disallowThreeValuedPosition or similar? It could live in CSSPropertyParserHelpers with ConsumePosition" Hence the alias kDisallowThreeValuedPosition. We can add a two value enum class, retire the alias, and DCHECK that kNumberOfFeatures is passed IFF the enum indicates 3 value positions are not supported: enum class ThreeValuePositionQuirk { kAllow, kForbid };
ericwilligers@chromium.org changed reviewers: - meade@chromium.org
Patchset #10 (id:200001) has been deleted
On 2017/05/24 at 01:27:17, ericwilligers wrote: > https://codereview.chromium.org/2888283006/diff/180001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h (right): > > https://codereview.chromium.org/2888283006/diff/180001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h:82: UseCounter::Feature::kNumberOfFeatures; > On 2017/05/23 23:26:54, suzyh_UTC10 wrote: > > I don't understand this, and it seems fragile because it is going to change > > every time someone adds a new feature to UseCounter. What's going on here? > > A recap of this code review: > > Version #3 of this CL defined an enum to specify > the type of position being parsed, and a switch to select > the appropriate use counter: > https://codereview.chromium.org/2888283006/patch/40001/50009 > https://codereview.chromium.org/2888283006/patch/40001/50008 > > This was unfortunately in the opposite direction from moving > property specific logic out of CSSPropertyParserHelpers. > > > Version #5 moved to simply indicating if 3 values are > permitted or not, and letting the caller update the use counter > https://codereview.chromium.org/2888283006/patch/100001/110012 > > That received > "The previous design with the property specific switch did seem a lot cleaner > than the new design." > > > Version #6 lets the caller pass in the specific use counter for when 3 value is received. > If there is no use counter, because 3 value is not valid, we pass the non-use-counter kNumberOfFeatures > https://codereview.chromium.org/2888283006/patch/140001/150014 > > > > https://codereview.chromium.org/2888283006/diff/140001/third_party/WebKit/Sou... > "for readability, could we create a named constant for this called > disallowThreeValuedPosition or similar? It could live in > CSSPropertyParserHelpers with ConsumePosition" > > Hence the alias kDisallowThreeValuedPosition. > > > We can add a two value enum class, retire the alias, and DCHECK that kNumberOfFeatures is passed IFF the enum indicates 3 value positions are not supported: > enum class ThreeValuePositionQuirk { kAllow, kForbid }; What we really want is some way to pass null in as the use counter. We are currently using kNumberOfFeatures to act like null here, since it doesn't correspond to any real use counter. Although the underlying number associated with kNumberOfFeatures will change, it won't impact the code here. It just so happens that whenever we don't want to use count, we also don't want to allow the feature. Those 2 things are a little confounded, as disallowing is theoretically independent from use counting. we could split this into 2 args, a 'do we allow' bool type arg and a 'which use counter to use' type arg. we still need a way to pass null as the use counter whenever we're not allowing the feature and I can't see a better option than kNumberOfFeatures (since 0 is taken).
Aha! OK, thanks for the explanation of how we got to this point. I think this latest version is preferable, although it would be so much nicer if we could actually provide effectively a null UseCounter::Feature value to pass in instead. I'd strongly suggest opening a bug to investigate a nicer way of doing this, and looping in rbyers for his opinion. core/css OWNERS lgtm https://codereview.chromium.org/2888283006/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/2888283006/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1657: UseCounter::Feature::kNumberOfFeatures); I'm curious if this should be UseCounter::kNumberOfFeatures to be consistent with the two below. (Or vice versa, have the ones below be UseCounter::Feature::kThreeValued*.) WDYT? https://codereview.chromium.org/2888283006/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp (right): https://codereview.chromium.org/2888283006/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:783: context.Count(threeValuePosition); I'd rather see this guarded by a check, because we don't want to be accidentally trying to increment the counter for kNumberOfFeatures. An if-statement might be appropriate, or a DCHECK that threeValuePosition is not kNumberOfFeatures at this point.
On 2017/05/24 04:24:18, suzyh_UTC10 wrote: > Aha! OK, thanks for the explanation of how we got to this point. I think this > latest version is preferable, although it would be so much nicer if we could > actually provide effectively a null UseCounter::Feature value to pass in > instead. I'd strongly suggest opening a bug to investigate a nicer way of doing > this, and looping in rbyers for his opinion. There is Optional, pioneered in https://theboostcpplibraries.com/boost.optional and now existing in std::optional (C++17) and base::Optional and WTF::Optional but not used AFAIK in style code. WTF::Optional is used in layout/ng.
https://codereview.chromium.org/2888283006/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/2888283006/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1657: UseCounter::Feature::kNumberOfFeatures); On 2017/05/24 04:24:18, suzyh_UTC10 wrote: > I'm curious if this should be UseCounter::kNumberOfFeatures to be consistent > with the two below. (Or vice versa, have the ones below be > UseCounter::Feature::kThreeValued*.) WDYT? scroll-snap-destination has not shipped, will not ship, and we hope will be retired from the code base within weeks. https://bugs.chromium.org/p/chromium/issues/detail?id=724912 Hence there is no need to support three value positions or add a use counter. The other two have shipped, so we have added use counters. https://codereview.chromium.org/2888283006/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp (right): https://codereview.chromium.org/2888283006/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:783: context.Count(threeValuePosition); On 2017/05/24 04:24:18, suzyh_UTC10 wrote: > I'd rather see this guarded by a check, because we don't want to be accidentally > trying to increment the counter for kNumberOfFeatures. An if-statement might be > appropriate, or a DCHECK that threeValuePosition is not kNumberOfFeatures at > this point. This is guarded by a DCHECK, at the top of the function.
The CQ bit was checked by ericwilligers@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bugsnash@chromium.org Link to the patchset: https://codereview.chromium.org/2888283006/#ps220001 (title: "extra argument")
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
Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
On 2017/05/24 at 04:44:48, ericwilligers wrote: > https://codereview.chromium.org/2888283006/diff/220001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): > > https://codereview.chromium.org/2888283006/diff/220001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1657: UseCounter::Feature::kNumberOfFeatures); > On 2017/05/24 04:24:18, suzyh_UTC10 wrote: > > I'm curious if this should be UseCounter::kNumberOfFeatures to be consistent > > with the two below. (Or vice versa, have the ones below be > > UseCounter::Feature::kThreeValued*.) WDYT? > > scroll-snap-destination has not shipped, will not ship, and we hope will be retired from the code base within weeks. > https://bugs.chromium.org/p/chromium/issues/detail?id=724912 > Hence there is no need to support three value positions or add a use counter. > > The other two have shipped, so we have added use counters. To clarify: I meant the inclusion of "::Feature" in the string. That is, the contrast between "UseCounter::Feature::kXXXX" and "UseCounter::kXXXX" -- you seem to have been inconsistent in which you use. It is a minor point. > https://codereview.chromium.org/2888283006/diff/220001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp (right): > > https://codereview.chromium.org/2888283006/diff/220001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:783: context.Count(threeValuePosition); > On 2017/05/24 04:24:18, suzyh_UTC10 wrote: > > I'd rather see this guarded by a check, because we don't want to be accidentally > > trying to increment the counter for kNumberOfFeatures. An if-statement might be > > appropriate, or a DCHECK that threeValuePosition is not kNumberOfFeatures at > > this point. > > This is guarded by a DCHECK, at the top of the function. I was a little worried that the DCHECK was too far removed from this point and that it could get out of sync or be otherwise unclear when reading the code that we can never accidentally try to use-count kNumberOfFeatures. I'll leave this to your judgement.
The CQ bit was checked by ericwilligers@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bugsnash@chromium.org, suzyh@chromium.org Link to the patchset: https://codereview.chromium.org/2888283006/#ps240001 (title: "rebase")
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 ericwilligers@chromium.org
> To clarify: I meant the inclusion of "::Feature" in the string. That is, the > contrast between "UseCounter::Feature::kXXXX" and "UseCounter::kXXXX" -- you > seem to have been inconsistent in which you use. It is a minor point. Got it, changed.
The CQ bit was checked by ericwilligers@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bugsnash@chromium.org, suzyh@chromium.org Link to the patchset: https://codereview.chromium.org/2888283006/#ps250001 (title: "rebase")
Patchset #11 (id:240001) has been deleted
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
ericwilligers@chromium.org changed reviewers: + alancutter@chromium.org
https://codereview.chromium.org/2888283006/diff/250001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h (right): https://codereview.chromium.org/2888283006/diff/250001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h:89: UseCounter::Feature threeValuePosition); How about accepting a UseCounter::Feature* to allow call sites to pass in nullptr instead of kNumberOfFeatures? Passing in non-null values will be a tad uglier but I think it's worth it to avoid this use of kNumberOfFeatures.
lgtm after changing the use counter enum to be wtf::Optional instead of using kNumberOfFeatures.
The CQ bit was checked by ericwilligers@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by ericwilligers@chromium.org to run a CQ dry run
Dry run: 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 ericwilligers@chromium.org
The CQ bit was checked by ericwilligers@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bugsnash@chromium.org, suzyh@chromium.org, alancutter@chromium.org Link to the patchset: https://codereview.chromium.org/2888283006/#ps290001 (title: "rebase")
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
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ericwilligers@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bugsnash@chromium.org, suzyh@chromium.org, alancutter@chromium.org Link to the patchset: https://codereview.chromium.org/2888283006/#ps310001 (title: "CSSPropertyAPIScrollSnapCoordinate")
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
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ericwilligers@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bugsnash@chromium.org, suzyh@chromium.org, alancutter@chromium.org Link to the patchset: https://codereview.chromium.org/2888283006/#ps330001 (title: "rebase")
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": 330001, "attempt_start_ts": 1495643377982060,
"parent_rev": "cf887b6f97e0762a83e1e054a9d4aeb14fa0f046", "commit_rev":
"a951d7b5e8647b077be37f420254b73063a30b4c"}
Message was sent while issue was closed.
Description was changed from ========== CSS: Use count position values with 3 parts The CSS Working Group recently changed the grammar of <position> such that 3-value syntax is no longer supported. https://lists.w3.org/Archives/Public/www-style/2017Mar/0054.html We update parsing logic so that the following now use count three value positions, in preparation for future deprecation and removal discussions: object-position https://drafts.csswg.org/css-images-3/#the-object-position perspective-origin https://drafts.csswg.org/css-transforms-2/#perspective-origin-property scroll-snap-coordinate (not shipped, not in current spec) https://www.w3.org/TR/2015/WD-css-snappoints-1-20150326/#scroll-snap-coordinate scroll-snap-destination (not shipped, not in current spec) https://www.w3.org/TR/2015/WD-css-snappoints-1-20150326/#scroll-snap-destination center position for circle and ellipse basic shapes https://drafts.csswg.org/css-shapes/#supported-basic-shapes (used in https://www.w3.org/TR/css-shapes-1/#shape-outside-property and https://drafts.fxtf.org/css-masking-1/#the-clip-path and https://drafts.fxtf.org/motion-1/#offset-path-property ) position for radial and (not shipped) conic gradients https://drafts.csswg.org/css-images-3/#radial-gradients https://drafts.csswg.org/css-images-4/#conic-gradients (used in https://drafts.csswg.org/css-backgrounds-3/#the-background-image ) The following no longer accept three value positions: offset-anchor (property not yet shipped, expecting to ship soon) https://drafts.fxtf.org/motion-1/#offset-anchor-property offset-position (property not yet shipped, expecting to ship soon) https://drafts.fxtf.org/motion-1/#offset-position-property The following are not affected: background-position https://drafts.csswg.org/css-backgrounds-3/#background-position (continues to support three values) transform-origin https://drafts.csswg.org/css-transforms/#transform-origin-property (has never used <position>, instead defines its own grammar) BUG=717833 ========== to ========== CSS: Use count position values with 3 parts The CSS Working Group recently changed the grammar of <position> such that 3-value syntax is no longer supported. https://lists.w3.org/Archives/Public/www-style/2017Mar/0054.html We update parsing logic so that the following now use count three value positions, in preparation for future deprecation and removal discussions: object-position https://drafts.csswg.org/css-images-3/#the-object-position perspective-origin https://drafts.csswg.org/css-transforms-2/#perspective-origin-property scroll-snap-coordinate (not shipped, not in current spec) https://www.w3.org/TR/2015/WD-css-snappoints-1-20150326/#scroll-snap-coordinate scroll-snap-destination (not shipped, not in current spec) https://www.w3.org/TR/2015/WD-css-snappoints-1-20150326/#scroll-snap-destination center position for circle and ellipse basic shapes https://drafts.csswg.org/css-shapes/#supported-basic-shapes (used in https://www.w3.org/TR/css-shapes-1/#shape-outside-property and https://drafts.fxtf.org/css-masking-1/#the-clip-path and https://drafts.fxtf.org/motion-1/#offset-path-property ) position for radial and (not shipped) conic gradients https://drafts.csswg.org/css-images-3/#radial-gradients https://drafts.csswg.org/css-images-4/#conic-gradients (used in https://drafts.csswg.org/css-backgrounds-3/#the-background-image ) The following no longer accept three value positions: offset-anchor (property not yet shipped, expecting to ship soon) https://drafts.fxtf.org/motion-1/#offset-anchor-property offset-position (property not yet shipped, expecting to ship soon) https://drafts.fxtf.org/motion-1/#offset-position-property The following are not affected: background-position https://drafts.csswg.org/css-backgrounds-3/#background-position (continues to support three values) transform-origin https://drafts.csswg.org/css-transforms/#transform-origin-property (has never used <position>, instead defines its own grammar) BUG=717833 Review-Url: https://codereview.chromium.org/2888283006 Cr-Commit-Position: refs/heads/master@{#474358} Committed: https://chromium.googlesource.com/chromium/src/+/a951d7b5e8647b077be37f420254... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:330001) as https://chromium.googlesource.com/chromium/src/+/a951d7b5e8647b077be37f420254...
Message was sent while issue was closed.
https://codereview.chromium.org/2888283006/diff/330001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp (right): https://codereview.chromium.org/2888283006/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:797: WTF::Optional<UseCounter::Feature> threeValuePosition) { I think this should have a comment such that if this argument is missing, then 3 values are not permitted. This could easily be misread as if this value is missing then there is no use counting |
