Simplify CSS variables resolution logic [1 of 2]

Simplify CSS variables resolution logic [1 of 2]

This patch removes the context object in variable resolution, moving the
success bool to be a more local concept. This fixes a bug where the flag
was being reset to true after fallback resolution in certain cases where
it shouldn't have been.

This patch also changes our storage for invalid custom properties from a
CSSVariableData with an empty vector to a null pointer. This doesn't end
up needing changes outside of the variable resolver as custom properties
are already returned as null in getVariable when they are invalid due to
being not set.

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

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

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

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

Split off part of the change from https://codereview.chromium.org/1694133002/

[shans, 2016-02-16 04:58:01]

lgtm

[dstockwell, 2016-02-16 07:14:33]

lgtm

[Timothy Loh, 2016-02-16 07:26:02]

Added a few more tests as discussed. I didn't comprehensively permute the
variables (none of the tests fail before the patch).

https://codereview.chromium.org/1698203002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/resolver/CSSVariableResolver.cpp
(right):

https://codereview.chromium.org/1698203002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/resolver/CSSVariableResolver.cpp:85:
m_cycleStartPoints.remove(variableName);
I un-added this line, it seems to not actually be needed (in fact it might be
wrong.. I'm not sure). I'm pretty sure it'll be right after the second patch
though, as that doesn't need to do anything special to handle cycles generated
by fallbacks.

[chromium-reviews, 2016-02-16 08:48:23]

Cool, thanks!

On Tue, 16 Feb 2016, 6:26 p.m.  <timloh@chromium.org> wrote:

>
> Added a few more tests as discussed. I didn't comprehensively permute the
> variables (none of the tests fail before the patch).
>
>
https://codereview.chromium.org/1698203002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/css/resolver/CSSVariableResolver.cpp
> (right):
>
https://codereview.chromium.org/1698203002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/css/resolver/CSSVariableResolver.cpp:85
<https://codereview.chromium.org/1698203002/diff/1/third_party/WebKit/Source/c...>:
> m_cycleStartPoints.remove(variableName);
> I un-added this line, it seems to not actually be needed (in fact it
> might be wrong.. I'm not sure). I'm pretty sure it'll be right after the
> second patch though, as that doesn't need to do anything special to
> handle cycles generated by fallbacks.
> https://codereview.chromium.org/1698203002/
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[blink-reviews, 2016-02-16 08:48:24]

Cool, thanks!

On Tue, 16 Feb 2016, 6:26 p.m.  <timloh@chromium.org> wrote:

>
> Added a few more tests as discussed. I didn't comprehensively permute the
> variables (none of the tests fail before the patch).
>
>
https://codereview.chromium.org/1698203002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/css/resolver/CSSVariableResolver.cpp
> (right):
>
https://codereview.chromium.org/1698203002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/css/resolver/CSSVariableResolver.cpp:85
<https://codereview.chromium.org/1698203002/diff/1/third_party/WebKit/Source/c...>:
> m_cycleStartPoints.remove(variableName);
> I un-added this line, it seems to not actually be needed (in fact it
> might be wrong.. I'm not sure). I'm pretty sure it'll be right after the
> second patch though, as that doesn't need to do anything special to
> handle cycles generated by fallbacks.
> https://codereview.chromium.org/1698203002/
>
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Timothy Loh, 2016-02-16 11:50:59]

The CQ bit was checked by timloh@chromium.org

[Timothy Loh, 2016-02-16 11:50:59]

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/1698203002/#ps20001
(title: "more tests")

[commit-bot: I haz the power, 2016-02-16 11:51:41]

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

[commit-bot: I haz the power, 2016-02-16 15:23:21]

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

[commit-bot: I haz the power, 2016-02-16 15:23:22]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Timothy Loh, 2016-02-16 22:34:36]

The CQ bit was checked by timloh@chromium.org

[commit-bot: I haz the power, 2016-02-16 22:39:04]

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

[commit-bot: I haz the power, 2016-02-16 23:52:44]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-02-16 23:54:33]

Description was changed from

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

This patch removes the context object in variable resolution, moving the
success bool to be a more local concept. This fixes a bug where the flag
was being reset to true after fallback resolution in certain cases where
it shouldn't have been.

This patch also changes our storage for invalid custom properties from a
CSSVariableData with an empty vector to a null pointer. This doesn't end
up needing changes outside of the variable resolver as custom properties
are already returned as null in getVariable when they are invalid due to
being not set.

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

to

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

This patch removes the context object in variable resolution, moving the
success bool to be a more local concept. This fixes a bug where the flag
was being reset to true after fallback resolution in certain cases where
it shouldn't have been.

This patch also changes our storage for invalid custom properties from a
CSSVariableData with an empty vector to a null pointer. This doesn't end
up needing changes outside of the variable resolver as custom properties
are already returned as null in getVariable when they are invalid due to
being not set.

BUG=586847

Committed: https://crrev.com/988acae8b91964c5e42eb01dac5cb041f26fbd7f
Cr-Commit-Position: refs/heads/master@{#375709}
==========

[commit-bot: I haz the power, 2016-02-16 23:54:33]

Patchset 2 (id:??) landed as
https://crrev.com/988acae8b91964c5e42eb01dac5cb041f26fbd7f
Cr-Commit-Position: refs/heads/master@{#375709}