Simplify CSS variables resolution logic [2 of 2]

Simplify CSS variables resolution logic [2 of 2]

This patch restructures the CSS variable resolution logic. The main goal
is to extract from resolveVariableTokensRecursive the recursive
resolution logic so that other potential features like @apply might be
able to recurse without needing to copy this logic.

This recursive resolution logic is split into two new functions
valueForCustomProperty and resolveCustomProperty (these are two functions
so we can write the loop in resolveVariableDefinitions without needing
extra hash lookups). These are the only functions which should handle the
cycle-detection logic.

BUG=586847
Committed: https://crrev.com/a6223fa39f086ab3483320014dce8b0b7a868886
Cr-Commit-Position: refs/heads/master@{#376687}

[Timothy Loh, 2016-02-15 04:28:19]

Description was changed from

==========
Simplify CSS variables resolution logic

This patch restructures the CSS variable resolution logic. The main goal
is to extract from resolveVariableTokensRecursive the recursive
resolution logic so that other potential features like @apply might be
able to recurse without needing to copy this logic.

This is split into two new functions, valueForCustomProperty and
resolveCustomProperty.

[todo: finish writing this]
==========

to

==========
Simplify CSS variables resolution logic

This patch restructures the CSS variable resolution logic. The main goal
is to extract from resolveVariableTokensRecursive the recursive
resolution logic so that other potential features like @apply might be
able to recurse without needing to copy this logic.

This recursive resolution logic is split into two new functions
valueForCustomProperty and resolveCustomProperty (these are two functions
so we can write the loop in resolveVariableDefinitions without needing
extra hash lookups).

The success bool on the context object is moved to return values. This
makes it easier to verify they are being handled correctly since they
operate at a local level only now. We also fixed a bug where the flag
would be reset to true when it should not have been reset.

Lastly this patch changes how we store invalid custom properties from
a CSSVariableData with an empty vector to a null pointer.

BUG=586847
==========

[Timothy Loh, 2016-02-15 04:28:20]

timloh@chromium.org changed reviewers:
+ dstockwell@chromium.org, shans@chromium.org

[Timothy Loh, 2016-02-15 04:32:20]

Description was changed from

==========
Simplify CSS variables resolution logic

This patch restructures the CSS variable resolution logic. The main goal
is to extract from resolveVariableTokensRecursive the recursive
resolution logic so that other potential features like @apply might be
able to recurse without needing to copy this logic.

This recursive resolution logic is split into two new functions
valueForCustomProperty and resolveCustomProperty (these are two functions
so we can write the loop in resolveVariableDefinitions without needing
extra hash lookups).

The success bool on the context object is moved to return values. This
makes it easier to verify they are being handled correctly since they
operate at a local level only now. We also fixed a bug where the flag
would be reset to true when it should not have been reset.

Lastly this patch changes how we store invalid custom properties from
a CSSVariableData with an empty vector to a null pointer.

BUG=586847
==========

to

==========
Simplify CSS variables resolution logic

This patch restructures the CSS variable resolution logic. The main goal
is to extract from resolveVariableTokensRecursive the recursive
resolution logic so that other potential features like @apply might be
able to recurse without needing to copy this logic.

The success bool on the context object is moved to return values. This
makes it easier to verify they are being handled correctly since they
operate at a local level only now. We also fixed a bug where the flag
would be reset to true when it should not have been reset.

This recursive resolution logic is split into two new functions
valueForCustomProperty and resolveCustomProperty (these are two functions
so we can write the loop in resolveVariableDefinitions without needing
extra hash lookups). These are the only functions which should handle the
cycle-detection logic.

Lastly this patch changes how we store invalid custom properties from
a CSSVariableData with an empty vector to a null pointer.

BUG=586847
==========

[Timothy Loh, 2016-02-15 04:34:54]

I tried to split it up but I introduced some cycle detection bugs when doing so.
So I'm leaving this as a single patch :/

[Timothy Loh, 2016-02-16 03:27:53]

Description was changed from

==========
Simplify CSS variables resolution logic

This patch restructures the CSS variable resolution logic. The main goal
is to extract from resolveVariableTokensRecursive the recursive
resolution logic so that other potential features like @apply might be
able to recurse without needing to copy this logic.

The success bool on the context object is moved to return values. This
makes it easier to verify they are being handled correctly since they
operate at a local level only now. We also fixed a bug where the flag
would be reset to true when it should not have been reset.

This recursive resolution logic is split into two new functions
valueForCustomProperty and resolveCustomProperty (these are two functions
so we can write the loop in resolveVariableDefinitions without needing
extra hash lookups). These are the only functions which should handle the
cycle-detection logic.

Lastly this patch changes how we store invalid custom properties from
a CSSVariableData with an empty vector to a null pointer.

BUG=586847
==========

to

==========
Simplify CSS variables resolution logic [2 of 2]

This patch restructures the CSS variable resolution logic. The main goal
is to extract from resolveVariableTokensRecursive the recursive
resolution logic so that other potential features like @apply might be
able to recurse without needing to copy this logic.

This recursive resolution logic is split into two new functions
valueForCustomProperty and resolveCustomProperty (these are two functions
so we can write the loop in resolveVariableDefinitions without needing
extra hash lookups). These are the only functions which should handle the
cycle-detection logic.

BUG=586847
==========

[Timothy Loh, 2016-02-16 03:34:56]

Split off part of the change into https://codereview.chromium.org/1698203002/,
hopefully that makes it easier to review.

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

https://codereview.chromium.org/1694133002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/CSSVariableResolver.cpp:127: //
TODO(leviw): This should be a stack
I assume this means to unroll the recursion into iteration and use an explicit
stack. I don't think it'd be worthwhile to consider this unless lots of websites
started having custom property dependency chains which were thousands of custom
properties long...

[shans, 2016-02-17 04:04:16]

lgtm

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

https://codereview.chromium.org/1694133002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/CSSVariableResolver.cpp:55:
Vector<CSSParserToken> tokens;
ASSERT(variableData->needsVariableResolution());

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

https://codereview.chromium.org/1694133002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/CSSVariableResolver.h:35: bool
resolveVariableReference(CSSParserTokenRange, Vector<CSSParserToken>& result);
This is a good opportunity to document what these functions do. Just a
once-sentence description.

https://codereview.chromium.org/1694133002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/CSSVariableResolver.h:40:
PassRefPtr<CSSVariableData> resolveCustomProperty(AtomicString name, const
CSSVariableData&);
(and these)

[Timothy Loh, 2016-02-22 03:24:55]

The CQ bit was checked by timloh@chromium.org

[Timothy Loh, 2016-02-22 03:24:56]

The patchset sent to the CQ was uploaded after l-g-t-m from shans@chromium.org
Link to the patchset: https://codereview.chromium.org/1694133002/#ps80001
(title: " ")

[commit-bot: I haz the power, 2016-02-22 03:24:58]

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

[commit-bot: I haz the power, 2016-02-22 03:36:21]

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

[commit-bot: I haz the power, 2016-02-22 03:36:22]

Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Timothy Loh, 2016-02-22 03:44:57]

The CQ bit was checked by timloh@chromium.org

[Timothy Loh, 2016-02-22 03:44:57]

The patchset sent to the CQ was uploaded after l-g-t-m from shans@chromium.org
Link to the patchset: https://codereview.chromium.org/1694133002/#ps100001
(title: " ")

[commit-bot: I haz the power, 2016-02-22 03:45:06]

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

[commit-bot: I haz the power, 2016-02-22 04:47:00]

Description was changed from

==========
Simplify CSS variables resolution logic [2 of 2]

This patch restructures the CSS variable resolution logic. The main goal
is to extract from resolveVariableTokensRecursive the recursive
resolution logic so that other potential features like @apply might be
able to recurse without needing to copy this logic.

This recursive resolution logic is split into two new functions
valueForCustomProperty and resolveCustomProperty (these are two functions
so we can write the loop in resolveVariableDefinitions without needing
extra hash lookups). These are the only functions which should handle the
cycle-detection logic.

BUG=586847
==========

to

==========
Simplify CSS variables resolution logic [2 of 2]

This patch restructures the CSS variable resolution logic. The main goal
is to extract from resolveVariableTokensRecursive the recursive
resolution logic so that other potential features like @apply might be
able to recurse without needing to copy this logic.

This recursive resolution logic is split into two new functions
valueForCustomProperty and resolveCustomProperty (these are two functions
so we can write the loop in resolveVariableDefinitions without needing
extra hash lookups). These are the only functions which should handle the
cycle-detection logic.

BUG=586847
==========

[commit-bot: I haz the power, 2016-02-22 04:47:01]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-02-22 04:48:07]

Description was changed from

==========
Simplify CSS variables resolution logic [2 of 2]

This patch restructures the CSS variable resolution logic. The main goal
is to extract from resolveVariableTokensRecursive the recursive
resolution logic so that other potential features like @apply might be
able to recurse without needing to copy this logic.

This recursive resolution logic is split into two new functions
valueForCustomProperty and resolveCustomProperty (these are two functions
so we can write the loop in resolveVariableDefinitions without needing
extra hash lookups). These are the only functions which should handle the
cycle-detection logic.

BUG=586847
==========

to

==========
Simplify CSS variables resolution logic [2 of 2]

This patch restructures the CSS variable resolution logic. The main goal
is to extract from resolveVariableTokensRecursive the recursive
resolution logic so that other potential features like @apply might be
able to recurse without needing to copy this logic.

This recursive resolution logic is split into two new functions
valueForCustomProperty and resolveCustomProperty (these are two functions
so we can write the loop in resolveVariableDefinitions without needing
extra hash lookups). These are the only functions which should handle the
cycle-detection logic.

BUG=586847

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

[commit-bot: I haz the power, 2016-02-22 04:48:08]

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