Add expansion of shorthands with custom properties to longhands using a pending substition value.

Expand custom property shorthands to longhand with pending value.

Introduces CSSPendingSubstitutionValue for each longhand, containing a
reference back to the shorthand property.

Changes CSSVariableResolver to just resolve variables, rather than
resolving and applying them (a task that StyleBuilderCustom can pretty
easily take care of).

Introduces a property cache on StyleResolverState to limit the amount
of parsing required during resolution. This is somewhat unattractive to
me, but there was a lack of better alternatives.

BUG=612634
Committed: https://crrev.com/6b045cf7e0fe8f7eb94dc94508d75dbde7e63365
Cr-Commit-Position: refs/heads/master@{#402620}

[Andy Mutton, 2016-06-21 04:55:56]

Description was changed from

==========
Add expansion of shorthands with custom properties to longhands using a pending
substition value.

Introduces CSSPendingSubstitutionValue for each longhand, containing a reference
back to the shorthand property.
Changes CSSVariableResolver to just resolve variables, rather than resolving and
applying them (a task that StyleBuilderCustom can pretty easily take care of).
Introduces a property cache on StyleResolverState to limit the amount of parsing
required during resolution. This is somewhat unattractive to me, but there was a
lack of better alternatives.
BUG=612634
==========

to

==========
Add expansion of shorthands with custom properties to longhands using a pending
substition value.

Introduces CSSPendingSubstitutionValue for each longhand, containing a reference
back to the shorthand property.
Changes CSSVariableResolver to just resolve variables, rather than resolving and
applying them (a task that StyleBuilderCustom can pretty easily take care of).
Introduces a property cache on StyleResolverState to limit the amount of parsing
required during resolution. This is somewhat unattractive to me, but there was a
lack of better alternatives.
BUG=612634
==========

[Andy Mutton, 2016-06-21 04:55:56]

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

[Timothy Loh, 2016-06-22 00:06:12]

(please wrap patch description to 72 chars)

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

https://codereview.chromium.org/2089593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/StylePropertySerializer.cpp:406: if
(value.isInheritedValue())
Probably need to also check for pending substitution values here. This is for a
case like:

-> e.style.cssText = 'margin: var(--x); margin-top: 10px'

-> console.log(e.style.cssText)
(firefox outputs the following, looks crazy but I think it's spec compliant)
margin-right: ; margin-bottom: ; margin-left: ; margin-top: 10px;

-> console.log($0.style.marginLeft)
-> console.log($0.style.margin)
(empty string for both as per spec)

Let's also have some sort of test for this stuff.

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

https://codereview.chromium.org/2089593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/CSSVariableResolver.cpp:150: //
Longhand variable references follow this path.
Longhand -> Non-shorthand

https://codereview.chromium.org/2089593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/CSSVariableResolver.cpp:158: if
(value) {
Braces { } are not required in Blink for single-line if statements (I prefer
omitting it generally, but it's up to you)

https://codereview.chromium.org/2089593002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/CSSVariableResolver.h (right):

https://codereview.chromium.org/2089593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/CSSVariableResolver.h:26: const
static CSSValue* resolveVariableReferences(StyleResolverState&, CSSPropertyID,
const CSSVariableReferenceValue&);
static const

https://codereview.chromium.org/2089593002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleResolverState.h (right):

https://codereview.chromium.org/2089593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleResolverState.h:163: void
setCssValueForLonghandPropertyId(CSSPropertyID, const CSSValue*);
Css -> CSS

https://codereview.chromium.org/2089593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleResolverState.h:193:
HeapHashMap<CSSPropertyID, Member<const CSSValue>>
m_parsedLonghandCssValueForVariableResolver;
I think this is insufficient for cases like:

$0.style.cssText = '--b1: 10px solid grey; --b2: 20px dashed green; border:
var(--b1); border-right: var(--b2);'

Firefox correctly renders (grey border, green dashed for right border), but I
think we'll get this wrong with this patch.

[Andy Mutton, 2016-06-22 01:06:17]

Description was changed from

==========
Add expansion of shorthands with custom properties to longhands using a pending
substition value.

Introduces CSSPendingSubstitutionValue for each longhand, containing a reference
back to the shorthand property.
Changes CSSVariableResolver to just resolve variables, rather than resolving and
applying them (a task that StyleBuilderCustom can pretty easily take care of).
Introduces a property cache on StyleResolverState to limit the amount of parsing
required during resolution. This is somewhat unattractive to me, but there was a
lack of better alternatives.
BUG=612634
==========

to

==========
Expand custom property shorthands to longhand with pending value.

Introduces CSSPendingSubstitutionValue for each longhand, containing a
reference back to the shorthand property.

Changes CSSVariableResolver to just resolve variables, rather than
resolving and applying them (a task that StyleBuilderCustom can pretty
easily take care of).

Introduces a property cache on StyleResolverState to limit the amount
of parsing required during resolution. This is somewhat unattractive to
me, but there was a lack of better alternatives.

BUG=612634
==========

[Andy Mutton, 2016-06-24 04:52:43]

On 2016/06/22 00:06:12, Timothy Loh wrote:
> (please wrap patch description to 72 chars)
> 
>
https://codereview.chromium.org/2089593002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/css/StylePropertySerializer.cpp (right):
> 
>
https://codereview.chromium.org/2089593002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/css/StylePropertySerializer.cpp:406: if
> (value.isInheritedValue())
> Probably need to also check for pending substitution values here. This is for
a
> case like:
> 
> -> e.style.cssText = 'margin: var(--x); margin-top: 10px'
> 
> -> console.log(e.style.cssText)
> (firefox outputs the following, looks crazy but I think it's spec compliant)
> margin-right: ; margin-bottom: ; margin-left: ; margin-top: 10px;
> 
> -> console.log($0.style.marginLeft)
> -> console.log($0.style.margin)
> (empty string for both as per spec)
> 
> Let's also have some sort of test for this stuff.
> 
>
https://codereview.chromium.org/2089593002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/css/resolver/CSSVariableResolver.cpp
> (right):
> 
>
https://codereview.chromium.org/2089593002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/css/resolver/CSSVariableResolver.cpp:150: //
> Longhand variable references follow this path.
> Longhand -> Non-shorthand
> 
>
https://codereview.chromium.org/2089593002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/css/resolver/CSSVariableResolver.cpp:158: if
> (value) {
> Braces { } are not required in Blink for single-line if statements (I prefer
> omitting it generally, but it's up to you)
> 
>
https://codereview.chromium.org/2089593002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/css/resolver/CSSVariableResolver.h
(right):
> 
>
https://codereview.chromium.org/2089593002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/css/resolver/CSSVariableResolver.h:26: const
> static CSSValue* resolveVariableReferences(StyleResolverState&, CSSPropertyID,
> const CSSVariableReferenceValue&);
> static const
> 
>
https://codereview.chromium.org/2089593002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/css/resolver/StyleResolverState.h (right):
> 
>
https://codereview.chromium.org/2089593002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/css/resolver/StyleResolverState.h:163: void
> setCssValueForLonghandPropertyId(CSSPropertyID, const CSSValue*);
> Css -> CSS
> 
>
https://codereview.chromium.org/2089593002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/css/resolver/StyleResolverState.h:193:
> HeapHashMap<CSSPropertyID, Member<const CSSValue>>
> m_parsedLonghandCssValueForVariableResolver;
> I think this is insufficient for cases like:
> 
> $0.style.cssText = '--b1: 10px solid grey; --b2: 20px dashed green; border:
> var(--b1); border-right: var(--b2);'
> 
> Firefox correctly renders (grey border, green dashed for right border), but I
> think we'll get this wrong with this patch.

All done. PTAL.

[Andy Mutton, 2016-06-24 04:53:26]

All done. PTAL.

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

https://codereview.chromium.org/2089593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/StylePropertySerializer.cpp:406: if
(value.isInheritedValue())
On 2016/06/22 00:06:11, Timothy Loh wrote:
> Probably need to also check for pending substitution values here. This is for
a
> case like:
> 
> -> e.style.cssText = 'margin: var(--x); margin-top: 10px'
> 
> -> console.log(e.style.cssText)
> (firefox outputs the following, looks crazy but I think it's spec compliant)
> margin-right: ; margin-bottom: ; margin-left: ; margin-top: 10px;
> 
> -> console.log($0.style.marginLeft)
> -> console.log($0.style.margin)
> (empty string for both as per spec)
> 
> Let's also have some sort of test for this stuff.

Done.

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

https://codereview.chromium.org/2089593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/CSSVariableResolver.cpp:150: //
Longhand variable references follow this path.
On 2016/06/22 00:06:12, Timothy Loh wrote:
> Longhand -> Non-shorthand

Done.

https://codereview.chromium.org/2089593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/CSSVariableResolver.cpp:158: if
(value) {
On 2016/06/22 00:06:11, Timothy Loh wrote:
> Braces { } are not required in Blink for single-line if statements (I prefer
> omitting it generally, but it's up to you)

I prefer keeping, but I also don't really care about my preferences vs. matching
what everyone else does. Done.

https://codereview.chromium.org/2089593002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/CSSVariableResolver.h (right):

https://codereview.chromium.org/2089593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/CSSVariableResolver.h:26: const
static CSSValue* resolveVariableReferences(StyleResolverState&, CSSPropertyID,
const CSSVariableReferenceValue&);
On 2016/06/22 00:06:12, Timothy Loh wrote:
> static const

Done.

https://codereview.chromium.org/2089593002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleResolverState.h (right):

https://codereview.chromium.org/2089593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleResolverState.h:163: void
setCssValueForLonghandPropertyId(CSSPropertyID, const CSSValue*);
On 2016/06/22 00:06:12, Timothy Loh wrote:
> Css -> CSS

Blurgh! Done.

https://codereview.chromium.org/2089593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleResolverState.h:193:
HeapHashMap<CSSPropertyID, Member<const CSSValue>>
m_parsedLonghandCssValueForVariableResolver;
On 2016/06/22 00:06:12, Timothy Loh wrote:
> I think this is insufficient for cases like:
> 
> $0.style.cssText = '--b1: 10px solid grey; --b2: 20px dashed green; border:
> var(--b1); border-right: var(--b2);'
> 
> Firefox correctly renders (grey border, green dashed for right border), but I
> think we'll get this wrong with this patch.

Done. Weird, more complicated caching structure added. Happy to change any bits
of it that look funky!

[Timothy Loh, 2016-06-27 04:19:10]

lgtm

https://codereview.chromium.org/2089593002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css/variables/longhand-pending-shorthand-substitution.html
(right):

https://codereview.chromium.org/2089593002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/variables/longhand-pending-shorthand-substitution.html:9:
}
just use a single block for the two #testElem cases?

https://codereview.chromium.org/2089593002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/variables/longhand-pending-shorthand-substitution.html:25:
var te = getComputedStyle(testElem);
this (and similar below) is actually a style not an element, maybe better to
name it style :)

https://codereview.chromium.org/2089593002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/StylePropertySerializer.cpp (right):

https://codereview.chromium.org/2089593002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/StylePropertySerializer.cpp:683: 
blank line not related :)

https://codereview.chromium.org/2089593002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/CSSVariableResolver.cpp
(right):

https://codereview.chromium.org/2089593002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/CSSVariableResolver.cpp:13: #include
"core/css/CSSValuePool.h"
unneeded

https://codereview.chromium.org/2089593002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/CSSVariableResolver.cpp:172: //
Property cache is unset. Let's fill it.
This would alternatively mean that we already got in here for a different
longhand but the value with variables resolved was invalid. We probably don't
want to be doing that (at least as fixing it isn't going to make the code more
complicated) but I guess it's ok for now. Maybe document this though,

// TODO(timloh): We shouldn't retry this for all longhands if the shorthand ends
up invalid

https://codereview.chromium.org/2089593002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleResolverState.cpp (right):

https://codereview.chromium.org/2089593002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleResolverState.cpp:89:
HeapHashMap<CSSPropertyID, Member<const CSSValue>>&
StyleResolverState::parsedPropertiesForPendingSubstitution(const CSSValue& psv)
no abbreviations in blink :)

https://codereview.chromium.org/2089593002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleResolverState.h (right):

https://codereview.chromium.org/2089593002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleResolverState.h:192:
HeapHashMap<Member<const CSSValue>, Member<HeapHashMap<CSSPropertyID,
Member<const CSSValue>>>> m_parsedPropertiesForPendingSubstitution;
nicer to key on CSSPendingSubstitutionValue

[Andy Mutton, 2016-06-28 03:18:51]

All done. Thanks.

https://codereview.chromium.org/2089593002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css/variables/longhand-pending-shorthand-substitution.html
(right):

https://codereview.chromium.org/2089593002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/variables/longhand-pending-shorthand-substitution.html:9:
}
On 2016/06/27 04:19:09, Timothy Loh wrote:
> just use a single block for the two #testElem cases?

Done.

https://codereview.chromium.org/2089593002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/variables/longhand-pending-shorthand-substitution.html:25:
var te = getComputedStyle(testElem);
On 2016/06/27 04:19:09, Timothy Loh wrote:
> this (and similar below) is actually a style not an element, maybe better to
> name it style :)

Done.

https://codereview.chromium.org/2089593002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/StylePropertySerializer.cpp (right):

https://codereview.chromium.org/2089593002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/StylePropertySerializer.cpp:683: 
On 2016/06/27 04:19:09, Timothy Loh wrote:
> blank line not related :)

Done.

https://codereview.chromium.org/2089593002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/CSSVariableResolver.cpp
(right):

https://codereview.chromium.org/2089593002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/CSSVariableResolver.cpp:13: #include
"core/css/CSSValuePool.h"
On 2016/06/27 04:19:09, Timothy Loh wrote:
> unneeded

Done.

https://codereview.chromium.org/2089593002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/CSSVariableResolver.cpp:172: //
Property cache is unset. Let's fill it.
On 2016/06/27 04:19:09, Timothy Loh wrote:
> This would alternatively mean that we already got in here for a different
> longhand but the value with variables resolved was invalid. We probably don't
> want to be doing that (at least as fixing it isn't going to make the code more
> complicated) but I guess it's ok for now. Maybe document this though,
> 
> // TODO(timloh): We shouldn't retry this for all longhands if the shorthand
ends
> up invalid

Done. (Happy for you to have a TODO)

https://codereview.chromium.org/2089593002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleResolverState.cpp (right):

https://codereview.chromium.org/2089593002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleResolverState.cpp:89:
HeapHashMap<CSSPropertyID, Member<const CSSValue>>&
StyleResolverState::parsedPropertiesForPendingSubstitution(const CSSValue& psv)
On 2016/06/27 04:19:10, Timothy Loh wrote:
> no abbreviations in blink :)

Done.

https://codereview.chromium.org/2089593002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleResolverState.h (right):

https://codereview.chromium.org/2089593002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleResolverState.h:192:
HeapHashMap<Member<const CSSValue>, Member<HeapHashMap<CSSPropertyID,
Member<const CSSValue>>>> m_parsedPropertiesForPendingSubstitution;
On 2016/06/27 04:19:10, Timothy Loh wrote:
> nicer to key on CSSPendingSubstitutionValue

Yeah... I debated this a little, since really this key is just any old pointer
and I tend to like less compile-time dependencies. However, I see the arguments
on the other side and I'm happy to make it explicit. Done.

[Andy Mutton, 2016-06-28 03:20:12]

The CQ bit was checked by andymutton@chromium.org to run a CQ dry run

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

Dry run: 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-06-28 06:10:30]

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

[commit-bot: I haz the power, 2016-06-28 06:10:31]

Dry run: This issue passed the CQ dry run.

[Andy Mutton, 2016-06-29 00:30:41]

The CQ bit was checked by andymutton@chromium.org

[Andy Mutton, 2016-06-29 00:30:41]

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/2089593002/#ps60001
(title: "Renames, signature update and master merge")

[commit-bot: I haz the power, 2016-06-29 00:32:34]

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-06-29 00:42:59]

Description was changed from

==========
Expand custom property shorthands to longhand with pending value.

Introduces CSSPendingSubstitutionValue for each longhand, containing a
reference back to the shorthand property.

Changes CSSVariableResolver to just resolve variables, rather than
resolving and applying them (a task that StyleBuilderCustom can pretty
easily take care of).

Introduces a property cache on StyleResolverState to limit the amount
of parsing required during resolution. This is somewhat unattractive to
me, but there was a lack of better alternatives.

BUG=612634
==========

to

==========
Expand custom property shorthands to longhand with pending value.

Introduces CSSPendingSubstitutionValue for each longhand, containing a
reference back to the shorthand property.

Changes CSSVariableResolver to just resolve variables, rather than
resolving and applying them (a task that StyleBuilderCustom can pretty
easily take care of).

Introduces a property cache on StyleResolverState to limit the amount
of parsing required during resolution. This is somewhat unattractive to
me, but there was a lack of better alternatives.

BUG=612634
==========

[commit-bot: I haz the power, 2016-06-29 00:43:00]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-29 00:44:11]

Description was changed from

==========
Expand custom property shorthands to longhand with pending value.

Introduces CSSPendingSubstitutionValue for each longhand, containing a
reference back to the shorthand property.

Changes CSSVariableResolver to just resolve variables, rather than
resolving and applying them (a task that StyleBuilderCustom can pretty
easily take care of).

Introduces a property cache on StyleResolverState to limit the amount
of parsing required during resolution. This is somewhat unattractive to
me, but there was a lack of better alternatives.

BUG=612634
==========

to

==========
Expand custom property shorthands to longhand with pending value.

Introduces CSSPendingSubstitutionValue for each longhand, containing a
reference back to the shorthand property.

Changes CSSVariableResolver to just resolve variables, rather than
resolving and applying them (a task that StyleBuilderCustom can pretty
easily take care of).

Introduces a property cache on StyleResolverState to limit the amount
of parsing required during resolution. This is somewhat unattractive to
me, but there was a lack of better alternatives.

BUG=612634

Committed: https://crrev.com/6b045cf7e0fe8f7eb94dc94508d75dbde7e63365
Cr-Commit-Position: refs/heads/master@{#402620}
==========

[commit-bot: I haz the power, 2016-06-29 00:44:12]

Patchset 4 (id:??) landed as
https://crrev.com/6b045cf7e0fe8f7eb94dc94508d75dbde7e63365
Cr-Commit-Position: refs/heads/master@{#402620}