Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2330893002:
CSS Properties and Values API: Support more syntax strings (Closed)
|
Created:
4 years, 3 months ago by Timothy Loh Modified:
4 years, 3 months ago Reviewers:
meade_UTC10 CC:
chromium-reviews, blink-reviews-css, dglazkov+blink, apavlov+blink_chromium.org, darktears, blink-reviews, rwlbuis Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCSS Properties and Values API: Support more syntax strings
This patch implements proper parsing of syntax strings in the P&V API,
as well parsing of the actual values. In particular, we add support for
parsing of syntax strings like "<length> | <percentage>" and "<number>+".
- Whitespace is not mentioned in the spec but used in examples. This
patch allows whitespace in most places.
- The <ident> production is possibly not the best choice (see github
issue below), so until that is resolved we only allow name code points.
- <resolution> is not yet used in any CSS properties in Blink, so value
parsing support will be added separately.
- <transform-function> isn't factored to be trivially supported, so value
parsing support will also be added separately. There's also an
outstanding issue re. Typed OM integration which might result in changes
here.
https://drafts.css-houdini.org/css-properties-values-api/#supported-syntax-strings
https://github.com/w3c/css-houdini-drafts/issues/265
BUG=641877
Committed: https://crrev.com/f3c212113fa3578799a243e1940e533ac6eac927
Cr-Commit-Position: refs/heads/master@{#419425}
Patch Set 1 #
Total comments: 9
Patch Set 2 : move some syntax parsing logic into helpers #
Dependent Patchsets: Messages
Total messages: 24 (15 generated)
Description was changed from ========== ---------------------This line is 72 characters long-------------------- BUG= ========== to ========== CSS Properties and Values API: Support more syntax strings This patch implements proper parsing of syntax strings in the P&V API, as well parsing of the actual values. - Whitespace is not mentioned in the spec but used in examples. This patch allows whitespace anywhere. - The <ident> production is possibly not the best choice (see github issue below), so until that is resolved we only allow name code points. - <resolution> is not yet used in any CSS properties in Blink, so value parsing support will be added separately. - <transform-function> isn't factored to easily be supported, so value parsing support will also be added separately. There's also an outstanding issue re. Typed OM integration which might result in changes here. https://drafts.css-houdini.org/css-properties-values-api/#supported-syntax-st... https://github.com/w3c/css-houdini-drafts/issues/265 BUG=641877 ==========
Description was changed from ========== CSS Properties and Values API: Support more syntax strings This patch implements proper parsing of syntax strings in the P&V API, as well parsing of the actual values. - Whitespace is not mentioned in the spec but used in examples. This patch allows whitespace anywhere. - The <ident> production is possibly not the best choice (see github issue below), so until that is resolved we only allow name code points. - <resolution> is not yet used in any CSS properties in Blink, so value parsing support will be added separately. - <transform-function> isn't factored to easily be supported, so value parsing support will also be added separately. There's also an outstanding issue re. Typed OM integration which might result in changes here. https://drafts.css-houdini.org/css-properties-values-api/#supported-syntax-st... https://github.com/w3c/css-houdini-drafts/issues/265 BUG=641877 ========== to ========== CSS Properties and Values API: Support more syntax strings This patch implements proper parsing of syntax strings in the P&V API, as well parsing of the actual values. - Whitespace is not mentioned in the spec but used in examples. This patch allows whitespace in most places. - The <ident> production is possibly not the best choice (see github issue below), so until that is resolved we only allow name code points. - <resolution> is not yet used in any CSS properties in Blink, so value parsing support will be added separately. - <transform-function> isn't factored to easily be supported, so value parsing support will also be added separately. There's also an outstanding issue re. Typed OM integration which might result in changes here. https://drafts.css-houdini.org/css-properties-values-api/#supported-syntax-st... https://github.com/w3c/css-houdini-drafts/issues/265 BUG=641877 ==========
The CQ bit was checked by timloh@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...
Description was changed from ========== CSS Properties and Values API: Support more syntax strings This patch implements proper parsing of syntax strings in the P&V API, as well parsing of the actual values. - Whitespace is not mentioned in the spec but used in examples. This patch allows whitespace in most places. - The <ident> production is possibly not the best choice (see github issue below), so until that is resolved we only allow name code points. - <resolution> is not yet used in any CSS properties in Blink, so value parsing support will be added separately. - <transform-function> isn't factored to easily be supported, so value parsing support will also be added separately. There's also an outstanding issue re. Typed OM integration which might result in changes here. https://drafts.css-houdini.org/css-properties-values-api/#supported-syntax-st... https://github.com/w3c/css-houdini-drafts/issues/265 BUG=641877 ========== to ========== CSS Properties and Values API: Support more syntax strings This patch implements proper parsing of syntax strings in the P&V API, as well parsing of the actual values. - Whitespace is not mentioned in the spec but used in examples. This patch allows whitespace in most places. - The <ident> production is possibly not the best choice (see github issue below), so until that is resolved we only allow name code points. - <resolution> is not yet used in any CSS properties in Blink, so value parsing support will be added separately. - <transform-function> isn't factored to be trivially supported, so value parsing support will also be added separately. There's also an outstanding issue re. Typed OM integration which might result in changes here. https://drafts.css-houdini.org/css-properties-values-api/#supported-syntax-st... https://github.com/w3c/css-houdini-drafts/issues/265 BUG=641877 ==========
timloh@chromium.org changed reviewers: + meade@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== CSS Properties and Values API: Support more syntax strings This patch implements proper parsing of syntax strings in the P&V API, as well parsing of the actual values. - Whitespace is not mentioned in the spec but used in examples. This patch allows whitespace in most places. - The <ident> production is possibly not the best choice (see github issue below), so until that is resolved we only allow name code points. - <resolution> is not yet used in any CSS properties in Blink, so value parsing support will be added separately. - <transform-function> isn't factored to be trivially supported, so value parsing support will also be added separately. There's also an outstanding issue re. Typed OM integration which might result in changes here. https://drafts.css-houdini.org/css-properties-values-api/#supported-syntax-st... https://github.com/w3c/css-houdini-drafts/issues/265 BUG=641877 ========== to ========== CSS Properties and Values API: Support more syntax strings This patch implements proper parsing of syntax strings in the P&V API, as well parsing of the actual values. In particular, we add support for parsing of syntax strings like "<length> | <percentage>" and "<number>+". - Whitespace is not mentioned in the spec but used in examples. This patch allows whitespace in most places. - The <ident> production is possibly not the best choice (see github issue below), so until that is resolved we only allow name code points. - <resolution> is not yet used in any CSS properties in Blink, so value parsing support will be added separately. - <transform-function> isn't factored to be trivially supported, so value parsing support will also be added separately. There's also an outstanding issue re. Typed OM integration which might result in changes here. https://drafts.css-houdini.org/css-properties-values-api/#supported-syntax-st... https://github.com/w3c/css-houdini-drafts/issues/265 BUG=641877 ==========
Sorry for super slow review, too many things happening all at once. This is basically lgtm to me - my comments are mostly stylistic things. I wouldn't mind a second opinion from someone more experienced with the CSS parser though. https://codereview.chromium.org/2330893002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/custom-properties/register-property-syntax-parsing.html (right): https://codereview.chromium.org/2330893002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/custom-properties/register-property-syntax-parsing.html:15: try { CSS.unregisterProperty('--syntax-test'); } catch(e) { } Having empty catch blocks in JavaScript seems like a code smell to me, as they'll catch everything including syntax errors. Is this really necessary? https://codereview.chromium.org/2330893002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/CSSSyntaxDescriptor.cpp (right): https://codereview.chromium.org/2330893002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/CSSSyntaxDescriptor.cpp:37: if (type == "length") Would it be better to use a switch statement here? Then if you do need to make type lowercase for comparison, you only need to do it once. https://codereview.chromium.org/2330893002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/CSSSyntaxDescriptor.cpp:87: offset++; This ends up with a lot of nesting, would it be clearer to move these inner loops into helper functions? e.g. consumeUntilChar, consumeIdent? Even better, is there a way to use the other parsing helpers here, like CSSParserToken etc? https://codereview.chromium.org/2330893002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/CSSSyntaxDescriptor.h (right): https://codereview.chromium.org/2330893002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/CSSSyntaxDescriptor.h:29: CustomIdent, Should these be alphabetised?
https://codereview.chromium.org/2330893002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/custom-properties/register-property-syntax-parsing.html (right): https://codereview.chromium.org/2330893002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/custom-properties/register-property-syntax-parsing.html:15: try { CSS.unregisterProperty('--syntax-test'); } catch(e) { } On 2016/09/14 12:54:13, Eddy wrote: > Having empty catch blocks in JavaScript seems like a code smell to me, as > they'll catch everything including syntax errors. Is this really necessary? I figured this was nicer than either generating different property names for each test or unregistering after registration. https://codereview.chromium.org/2330893002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/CSSSyntaxDescriptor.cpp (right): https://codereview.chromium.org/2330893002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/CSSSyntaxDescriptor.cpp:37: if (type == "length") On 2016/09/14 12:54:13, Eddy wrote: > Would it be better to use a switch statement here? Then if you do need to make > type lowercase for comparison, you only need to do it once. C++ doesn't support switch statements over strings. This function is only called when registering a string so I don't think we need to add any complexity to make it more efficient. https://codereview.chromium.org/2330893002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/CSSSyntaxDescriptor.cpp:87: offset++; On 2016/09/14 12:54:13, Eddy wrote: > This ends up with a lot of nesting, would it be clearer to move these inner > loops into helper functions? e.g. consumeUntilChar, consumeIdent? I moved some of this logic into helper functions. > Even better, is there a way to use the other parsing helpers here, like > CSSParserToken etc? No, because the meta-syntax is not CSS (which allows comments everywhere and has escape processing). https://codereview.chromium.org/2330893002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/CSSSyntaxDescriptor.h (right): https://codereview.chromium.org/2330893002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/CSSSyntaxDescriptor.h:29: CustomIdent, On 2016/09/14 12:54:13, Eddy wrote: > Should these be alphabetised? These (Length->CustomIdent) are ordered the same as in the spec. I don't have any strong preference for order here -- I just left them as is but LMK if you'd prefer I changed it.
https://codereview.chromium.org/2330893002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/CSSSyntaxDescriptor.cpp (right): https://codereview.chromium.org/2330893002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/CSSSyntaxDescriptor.cpp:37: if (type == "length") On 2016/09/19 07:14:14, Timothy Loh wrote: > On 2016/09/14 12:54:13, Eddy wrote: > > Would it be better to use a switch statement here? Then if you do need to make > > type lowercase for comparison, you only need to do it once. > > C++ doesn't support switch statements over strings. This function is only called > when registering a string so I don't think we need to add any complexity to make > it more efficient. registering a property*
The CQ bit was checked by timloh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from meade@chromium.org Link to the patchset: https://codereview.chromium.org/2330893002/#ps20001 (title: "move some syntax parsing logic into helpers")
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_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by timloh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== CSS Properties and Values API: Support more syntax strings This patch implements proper parsing of syntax strings in the P&V API, as well parsing of the actual values. In particular, we add support for parsing of syntax strings like "<length> | <percentage>" and "<number>+". - Whitespace is not mentioned in the spec but used in examples. This patch allows whitespace in most places. - The <ident> production is possibly not the best choice (see github issue below), so until that is resolved we only allow name code points. - <resolution> is not yet used in any CSS properties in Blink, so value parsing support will be added separately. - <transform-function> isn't factored to be trivially supported, so value parsing support will also be added separately. There's also an outstanding issue re. Typed OM integration which might result in changes here. https://drafts.css-houdini.org/css-properties-values-api/#supported-syntax-st... https://github.com/w3c/css-houdini-drafts/issues/265 BUG=641877 ========== to ========== CSS Properties and Values API: Support more syntax strings This patch implements proper parsing of syntax strings in the P&V API, as well parsing of the actual values. In particular, we add support for parsing of syntax strings like "<length> | <percentage>" and "<number>+". - Whitespace is not mentioned in the spec but used in examples. This patch allows whitespace in most places. - The <ident> production is possibly not the best choice (see github issue below), so until that is resolved we only allow name code points. - <resolution> is not yet used in any CSS properties in Blink, so value parsing support will be added separately. - <transform-function> isn't factored to be trivially supported, so value parsing support will also be added separately. There's also an outstanding issue re. Typed OM integration which might result in changes here. https://drafts.css-houdini.org/css-properties-values-api/#supported-syntax-st... https://github.com/w3c/css-houdini-drafts/issues/265 BUG=641877 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== CSS Properties and Values API: Support more syntax strings This patch implements proper parsing of syntax strings in the P&V API, as well parsing of the actual values. In particular, we add support for parsing of syntax strings like "<length> | <percentage>" and "<number>+". - Whitespace is not mentioned in the spec but used in examples. This patch allows whitespace in most places. - The <ident> production is possibly not the best choice (see github issue below), so until that is resolved we only allow name code points. - <resolution> is not yet used in any CSS properties in Blink, so value parsing support will be added separately. - <transform-function> isn't factored to be trivially supported, so value parsing support will also be added separately. There's also an outstanding issue re. Typed OM integration which might result in changes here. https://drafts.css-houdini.org/css-properties-values-api/#supported-syntax-st... https://github.com/w3c/css-houdini-drafts/issues/265 BUG=641877 ========== to ========== CSS Properties and Values API: Support more syntax strings This patch implements proper parsing of syntax strings in the P&V API, as well parsing of the actual values. In particular, we add support for parsing of syntax strings like "<length> | <percentage>" and "<number>+". - Whitespace is not mentioned in the spec but used in examples. This patch allows whitespace in most places. - The <ident> production is possibly not the best choice (see github issue below), so until that is resolved we only allow name code points. - <resolution> is not yet used in any CSS properties in Blink, so value parsing support will be added separately. - <transform-function> isn't factored to be trivially supported, so value parsing support will also be added separately. There's also an outstanding issue re. Typed OM integration which might result in changes here. https://drafts.css-houdini.org/css-properties-values-api/#supported-syntax-st... https://github.com/w3c/css-houdini-drafts/issues/265 BUG=641877 Committed: https://crrev.com/f3c212113fa3578799a243e1940e533ac6eac927 Cr-Commit-Position: refs/heads/master@{#419425} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/f3c212113fa3578799a243e1940e533ac6eac927 Cr-Commit-Position: refs/heads/master@{#419425} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
