Do not accept CSS-wide keywords for descriptors

Do not accept CSS-wide keywords for descriptors

Do not accept CSS-wide keywords for descriptors, since they are
only defined to work on properties[1]. In order to test for
descriptors, add a new keyword 'descriptor' to CSSProperties.in
and generate the function isDescriptor based on that.

BUG=448567
[1] http://www.w3.org/TR/css3-values/#common-keywords

Committed: https://crrev.com/cc83aeab5eb7a294fc8583bbbd3c8b4ee4c0281f
Cr-Commit-Position: refs/heads/master@{#396870}

[rwlbuis, 2016-05-06 20:28:26]

Description was changed from

==========
448567 WIP

BUG=
==========

to

==========
448567 WIP

BUG=448567
==========

[rwlbuis, 2016-05-16 01:52:08]

Description was changed from

==========
448567 WIP

BUG=448567
==========

to

==========
Do not accept CSS-wide identifiers for descriptors

Do not accept CSS-wide identifiers for descriptors.

BUG=448567
==========

[rwlbuis, 2016-05-16 01:53:11]

Description was changed from

==========
Do not accept CSS-wide identifiers for descriptors

Do not accept CSS-wide identifiers for descriptors.

BUG=448567
==========

to

==========
Do not accept CSS-wide keywords for descriptors

Do not accept CSS-wide keywords for descriptors.

BUG=448567
==========

[rwlbuis, 2016-05-16 02:04:30]

Description was changed from

==========
Do not accept CSS-wide keywords for descriptors

Do not accept CSS-wide keywords for descriptors.

BUG=448567
==========

to

==========
Do not accept CSS-wide keywords for descriptors

Do not accept CSS-wide keywords for descriptors, since they are only defined to
work on properties[1]. In order to test for descriptors, add a new keyword
descriptor to CSSProperties.in and change the 

BUG=448567

[1] http://www.w3.org/TR/css3-values/#common-keywords
==========

[rwlbuis, 2016-05-16 02:05:34]

Description was changed from

==========
Do not accept CSS-wide keywords for descriptors

Do not accept CSS-wide keywords for descriptors, since they are only defined to
work on properties[1]. In order to test for descriptors, add a new keyword
descriptor to CSSProperties.in and change the 

BUG=448567

[1] http://www.w3.org/TR/css3-values/#common-keywords
==========

to

==========
Do not accept CSS-wide keywords for descriptors

Do not accept CSS-wide keywords for descriptors, since they are
only defined to work on properties[1]. In order to test for
descriptors, add a new keyword descriptor to CSSProperties.in
and generate the function isDescriptor based on that.

BUG=448567

[1] http://www.w3.org/TR/css3-values/#common-keywords
==========

[rwlbuis, 2016-05-16 02:07:57]

Description was changed from

==========
Do not accept CSS-wide keywords for descriptors

Do not accept CSS-wide keywords for descriptors, since they are
only defined to work on properties[1]. In order to test for
descriptors, add a new keyword descriptor to CSSProperties.in
and generate the function isDescriptor based on that.

BUG=448567

[1] http://www.w3.org/TR/css3-values/#common-keywords
==========

to

==========
Do not accept CSS-wide keywords for descriptors

Do not accept CSS-wide keywords for descriptors, since they are
only defined to work on properties[1]. In order to test for
descriptors, add a new keyword 'descriptor' to CSSProperties.in
and generate the function isDescriptor based on that.

BUG=448567

[1] http://www.w3.org/TR/css3-values/#common-keywords
==========

[rwlbuis, 2016-05-16 02:08:52]

rob.buis@samsung.com changed reviewers:
+ timloh@chromium.org

[rwlbuis, 2016-05-16 02:08:52]

PTAL.

[Timothy Loh, 2016-05-19 05:12:22]

lgtm

[Timothy Loh, 2016-05-19 05:12:40]

On 2016/05/19 05:12:22, Timothy Loh wrote:
> lgtm

oops finger slipped, meant to be for a different patch

[Timothy Loh, 2016-05-19 05:44:42]

https://codereview.chromium.org/1386733003/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/css-parser/css-wide-keywords-descriptors.html
(right):

https://codereview.chromium.org/1386733003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/css-parser/css-wide-keywords-descriptors.html:8:
assert_invalid_value("orientation", "initial");
I'd swap some initials to inherit/unset and try different capitalizations.

https://codereview.chromium.org/1386733003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSProperties.in (right):

https://codereview.chromium.org/1386733003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSProperties.in:445: font-display
builder_skip, runtime_flag=CSSFontDisplay
this one is a @font-face descriptor

https://codereview.chromium.org/1386733003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSProperties.in:450: max-zoom descriptor,
builder_skip
descriptor should be something like descriptor_only since we aren't tagging
stuff like width and font-family. Also let's remove builder_skip from these and
make descriptor_only imply should_declare_functions in make_style_builder.py and
have these assert in StyleBuilder.cpp.tmpl if they're reached.

https://codereview.chromium.org/1386733003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSParserFastPaths.cpp (right):

https://codereview.chromium.org/1386733003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSParserFastPaths.cpp:849: if
(CSSPropertyMetadata::isDescriptor(propertyId))
Maybe this check should just be at the top of the function. font-display is
apparently handled by the keyword fast path currently.

https://codereview.chromium.org/1386733003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1386733003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:309: if
(CSSPropertyMetadata::isDescriptor(property))
I think it's better to be at the very top of parseValueStart, so it's clear that
these non-properties are never successfully parsed.

[rwlbuis, 2016-05-19 17:12:00]

Patchset #8 (id:140001) has been deleted

[rwlbuis, 2016-05-19 18:38:02]

Patchset #8 (id:160001) has been deleted

[rwlbuis, 2016-05-19 21:38:17]

Patchset #9 (id:200001) has been deleted

[rwlbuis, 2016-05-20 00:51:11]

PTAL.

https://codereview.chromium.org/1386733003/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/css-parser/css-wide-keywords-descriptors.html
(right):

https://codereview.chromium.org/1386733003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/css-parser/css-wide-keywords-descriptors.html:8:
assert_invalid_value("orientation", "initial");
On 2016/05/19 05:44:41, Timothy Loh wrote:
> I'd swap some initials to inherit/unset and try different capitalizations.

Done.

https://codereview.chromium.org/1386733003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSProperties.in (right):

https://codereview.chromium.org/1386733003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSProperties.in:445: font-display
builder_skip, runtime_flag=CSSFontDisplay
On 2016/05/19 05:44:42, Timothy Loh wrote:
> this one is a @font-face descriptor

Done.

https://codereview.chromium.org/1386733003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSProperties.in:450: max-zoom descriptor,
builder_skip
On 2016/05/19 05:44:42, Timothy Loh wrote:
> descriptor should be something like descriptor_only since we aren't tagging
> stuff like width and font-family. Also let's remove builder_skip from these
and
> make descriptor_only imply should_declare_functions in make_style_builder.py
and
> have these assert in StyleBuilder.cpp.tmpl if they're reached.

Done.

https://codereview.chromium.org/1386733003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSParserFastPaths.cpp (right):

https://codereview.chromium.org/1386733003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSParserFastPaths.cpp:849: if
(CSSPropertyMetadata::isDescriptor(propertyId))
On 2016/05/19 05:44:42, Timothy Loh wrote:
> Maybe this check should just be at the top of the function. font-display is
> apparently handled by the keyword fast path currently.

This was placed here to minimize calls to isDescriptor. I can still move it if
you want.

https://codereview.chromium.org/1386733003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1386733003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:309: if
(CSSPropertyMetadata::isDescriptor(property))
On 2016/05/19 05:44:42, Timothy Loh wrote:
> I think it's better to be at the very top of parseValueStart, so it's clear
that
> these non-properties are never successfully parsed.

Same as before, done for efficiency, can move if you want. I was not sure if an
UNLIKELY would help either.

[rwlbuis, 2016-05-25 20:49:10]

On 2016/05/20 00:51:11, rwlbuis wrote:
> 
[ bunch of stuff fixed]
>

PTAP (please take a peek).

[rwlbuis, 2016-05-25 20:49:23]

On 2016/05/20 00:51:11, rwlbuis wrote:
> 
[ bunch of stuff fixed]
>

PTAP (please take a peek).

[Timothy Loh, 2016-05-31 03:24:05]

lgtm, sorry for the delay. Please update the description too.

https://codereview.chromium.org/1386733003/diff/220001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/build/scripts/templates/CSSPropertyMetadata.cpp.tmpl
(right):

https://codereview.chromium.org/1386733003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/templates/CSSPropertyMetadata.cpp.tmpl:65:
bool CSSPropertyMetadata::isDescriptor(CSSPropertyID property)
let's rename this to isDecriptorOnly (similar to the flag)

[Timothy Loh, 2016-05-31 07:35:10]

https://codereview.chromium.org/1386733003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSProperties.in (right):

https://codereview.chromium.org/1386733003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSProperties.in:16: 
// - descriptor_only
// These are actually descriptors and not CSS properties. Properties with the
same name as a descriptor do not get this flag.

https://codereview.chromium.org/1386733003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSProperties.in:449: // descriptors go here
blank line before comment

[rwlbuis, 2016-05-31 16:08:18]

The CQ bit was checked by rob.buis@samsung.com

[rwlbuis, 2016-05-31 16:08:18]

The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org
Link to the patchset: https://codereview.chromium.org/1386733003/#ps240001
(title: "Patch for landing")

[commit-bot: I haz the power, 2016-05-31 16:08:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1386733003/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1386733003/240001

[commit-bot: I haz the power, 2016-05-31 18:26:46]

Description was changed from

==========
Do not accept CSS-wide keywords for descriptors

Do not accept CSS-wide keywords for descriptors, since they are
only defined to work on properties[1]. In order to test for
descriptors, add a new keyword 'descriptor' to CSSProperties.in
and generate the function isDescriptor based on that.

BUG=448567

[1] http://www.w3.org/TR/css3-values/#common-keywords
==========

to

==========
Do not accept CSS-wide keywords for descriptors

Do not accept CSS-wide keywords for descriptors, since they are
only defined to work on properties[1]. In order to test for
descriptors, add a new keyword 'descriptor' to CSSProperties.in
and generate the function isDescriptor based on that.

BUG=448567

[1] http://www.w3.org/TR/css3-values/#common-keywords
==========

[commit-bot: I haz the power, 2016-05-31 18:26:47]

Committed patchset #10 (id:240001)

[commit-bot: I haz the power, 2016-05-31 18:27:47]

Description was changed from

==========
Do not accept CSS-wide keywords for descriptors

Do not accept CSS-wide keywords for descriptors, since they are
only defined to work on properties[1]. In order to test for
descriptors, add a new keyword 'descriptor' to CSSProperties.in
and generate the function isDescriptor based on that.

BUG=448567

[1] http://www.w3.org/TR/css3-values/#common-keywords
==========

to

==========
Do not accept CSS-wide keywords for descriptors

Do not accept CSS-wide keywords for descriptors, since they are
only defined to work on properties[1]. In order to test for
descriptors, add a new keyword 'descriptor' to CSSProperties.in
and generate the function isDescriptor based on that.

BUG=448567

[1] http://www.w3.org/TR/css3-values/#common-keywords

Committed: https://crrev.com/cc83aeab5eb7a294fc8583bbbd3c8b4ee4c0281f
Cr-Commit-Position: refs/heads/master@{#396870}
==========

[commit-bot: I haz the power, 2016-05-31 18:27:48]

Patchset 10 (id:??) landed as
https://crrev.com/cc83aeab5eb7a294fc8583bbbd3c8b4ee4c0281f
Cr-Commit-Position: refs/heads/master@{#396870}