Recognise variable names when parsing CSS property names

Recognise variable names when parsing CSS property names

This patch moves the CSSVariableResolver::isValidVariableName call that
usually exists alongside calling cssPropertyId into the cssPropertyId
call (and similarly for unresolvedCSSPropertyId). Pretty much every
place which parses property names should be handling custom properties,
so it's easier to just return CSSPropertyVariable instead of needing to
manually check for a valid custom property name.

Committed: https://crrev.com/07846616d126fafc699b3c8e8edad379c16dfeda
Cr-Commit-Position: refs/heads/master@{#415230}

[Timothy Loh, 2016-08-29 04:01:36]

timloh@chromium.org changed reviewers:
+ ikilpatrick@chromium.org

[Timothy Loh, 2016-08-29 04:01:38]

https://codereview.chromium.org/2288633002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/2288633002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:168: if (length
>= 2 && propertyName[0] == '-' && propertyName[1] == '-')
I duplicated the logic because the argument to
CSSVariableParser::isValidVariableName didn't match what we have here, but the
logic is simple enough that I think it's fine.

[rune, 2016-08-29 07:48:41]

rune@opera.com changed reviewers:
+ rune@opera.com

[rune, 2016-08-29 07:48:42]

lgtm

https://codereview.chromium.org/2288633002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp (right):

https://codereview.chromium.org/2288633002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp:769: //
TODO(timloh): Shouldn't this only be for StyleRule::Style?
Yes, and it's observable as a bug. Reported 641873. Could you add the issue
number to the TODO?

[Timothy Loh, 2016-08-29 08:15:50]

https://codereview.chromium.org/2288633002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp (right):

https://codereview.chromium.org/2288633002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp:769: //
TODO(timloh): Shouldn't this only be for StyleRule::Style?
On 2016/08/29 07:48:42, rune wrote:
> Yes, and it's observable as a bug. Reported 641873. Could you add the issue
> number to the TODO?

Done.

[Timothy Loh, 2016-08-29 08:15:58]

The CQ bit was checked by timloh@chromium.org

[Timothy Loh, 2016-08-29 08:15:58]

The patchset sent to the CQ was uploaded after l-g-t-m from rune@opera.com
Link to the patchset: https://codereview.chromium.org/2288633002/#ps20001
(title: "tweak comment")

[commit-bot: I haz the power, 2016-08-29 08:16:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-29 08:22:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-29 08:22:14]

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...)

[haraken, 2016-08-29 08:22:47]

owner LGTM

[ikilpatrick, 2016-08-29 16:29:19]

lgtm

[ikilpatrick, 2016-08-29 16:30:59]

lgtm

https://codereview.chromium.org/2288633002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/DOMWindowCSS.cpp (right):

https://codereview.chromium.org/2288633002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/DOMWindowCSS.cpp:48: return
CSSParser::parseValueForCustomProperty(dummyStyle, "--valid", value, false, 0);
I assume once we have everything hooked up this will change from "--valid" to
property?

[ikilpatrick, 2016-08-29 16:31:03]

lgtm

[Timothy Loh, 2016-08-30 03:57:18]

https://codereview.chromium.org/2288633002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/DOMWindowCSS.cpp (right):

https://codereview.chromium.org/2288633002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/DOMWindowCSS.cpp:48: return
CSSParser::parseValueForCustomProperty(dummyStyle, "--valid", value, false, 0);
On 2016/08/29 16:30:59, ikilpatrick wrote:
> I assume once we have everything hooked up this will change from "--valid" to
> property?

I'm not really sure why this is "--valid" (I guess it's been like that since
custom props were implemented), property should work just as well. I'll probably
have to add logic here for custom props 2 (if CSS.supports is supposed to
change)

[Timothy Loh, 2016-08-30 03:57:22]

The CQ bit was checked by timloh@chromium.org

[Timothy Loh, 2016-08-30 03:57:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, ikilpatrick@chromium.org, rune@opera.com
Link to the patchset: https://codereview.chromium.org/2288633002/#ps40001
(title: "fix long var names")

[commit-bot: I haz the power, 2016-08-30 04:00:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-30 06:20:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-30 06:20:04]

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_...)

[Timothy Loh, 2016-08-30 06:25:04]

The CQ bit was checked by timloh@chromium.org

[commit-bot: I haz the power, 2016-08-30 06:25:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-30 07:26:48]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-30 07:28:36]

Description was changed from

==========
Recognise variable names when parsing CSS property names

This patch moves the CSSVariableResolver::isValidVariableName call that
usually exists alongside calling cssPropertyId into the cssPropertyId
call (and similarly for unresolvedCSSPropertyId). Pretty much every
place which parses property names should be handling custom properties,
so it's easier to just return CSSPropertyVariable instead of needing to
manually check for a valid custom property name.
==========

to

==========
Recognise variable names when parsing CSS property names

This patch moves the CSSVariableResolver::isValidVariableName call that
usually exists alongside calling cssPropertyId into the cssPropertyId
call (and similarly for unresolvedCSSPropertyId). Pretty much every
place which parses property names should be handling custom properties,
so it's easier to just return CSSPropertyVariable instead of needing to
manually check for a valid custom property name.

Committed: https://crrev.com/07846616d126fafc699b3c8e8edad379c16dfeda
Cr-Commit-Position: refs/heads/master@{#415230}
==========

[commit-bot: I haz the power, 2016-08-30 07:28:38]

Patchset 3 (id:??) landed as
https://crrev.com/07846616d126fafc699b3c8e8edad379c16dfeda
Cr-Commit-Position: refs/heads/master@{#415230}