Ensure baseComputedStyle optimisation is cleared during registered custom property animations

Ensure baseComputedStyle optimisation is cleared during registered custom property animations

This change to StyleResolver ensures that the cached baseComputedStyle
on an animated element is removed while custom property animations are
active. This prevents stale baseComputedStyles from persisting and
failing the equality assertion in ElementAnimations::updateBaseComputedStyle().

The regression test for this bug needed to be a SimTest as it depended
on style recalcs being triggered by animations and not Javascript.
It's not possible to do this deterministically from Javascript as it
requires control over the frame times.

As part of making the SimTest ElementAnimation::animate() and
PropertyRegistration::registerProperty() required changes to reduce their
V8 dependencies. Additionally a test-only method was added to
AnimationClock to turn off generating synthetic frame times.

BUG=701250
Review-Url: https://codereview.chromium.org/2757543002
Cr-Commit-Position: refs/heads/master@{#461423}
Committed: https://chromium.googlesource.com/chromium/src/+/767e438649154d1985a803c93111e0860a29fa2f

[alancutter (OOO until 2018), 2017-03-16 04:02:51]

Description was changed from

==========
+animationStyleChangeSimTestPants

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

to

==========
Ensure baseComputedStyle optimisation is cleared during registered custom
property animations

This change ensures that the cached baseComputedStyle on an animated element is
removed while custom property animations are active. This prevents stale
baseComputedStyles from persisting and failing the equality assertion in
ElementAnimations::updateBaseComputedStyle().


The regression test for this bug needed to be a SimTest as it depended
on style recalcs being triggered by animations and not Javascript.
It's not possible to do this deterministically from Javascript as it
requires control over the frame times.

As part of making the SimTest ElementAnimation::animate() and
PropertyRegistration::registerProperty() required changes to reduce their V8
dependencies.

BUG=701250
==========

[alancutter (OOO until 2018), 2017-03-16 04:02:51]

alancutter@chromium.org changed reviewers:
+ esprehn@chromium.org, suzyh@chromium.org

[alancutter (OOO until 2018), 2017-03-16 04:03:24]

esprehn please review AnimationSimTest.cpp for SimTest coding style.

[suzyh_UTC10 (ex-contributor), 2017-03-16 23:46:41]

I'm afraid I do not understand this patch at all. The only behavioural change
seems to be changing the registerProperty argument from a ScriptState to an
ExecutionContext, and I don't know how that achieves the clearing of the
baseComputedStyle optimisation mentioned in the CL description. Please add
comments to the SimTest to explain what it's doing and how/why it tests what
it's testing.

[alancutter (OOO until 2018), 2017-03-17 00:01:47]

Description was changed from

==========
Ensure baseComputedStyle optimisation is cleared during registered custom
property animations

This change ensures that the cached baseComputedStyle on an animated element is
removed while custom property animations are active. This prevents stale
baseComputedStyles from persisting and failing the equality assertion in
ElementAnimations::updateBaseComputedStyle().


The regression test for this bug needed to be a SimTest as it depended
on style recalcs being triggered by animations and not Javascript.
It's not possible to do this deterministically from Javascript as it
requires control over the frame times.

As part of making the SimTest ElementAnimation::animate() and
PropertyRegistration::registerProperty() required changes to reduce their V8
dependencies.

BUG=701250
==========

to

==========
Ensure baseComputedStyle optimisation is cleared during registered custom
property animations

This change ensures that the cached baseComputedStyle on an animated element is
removed while custom property animations are active. This prevents stale
baseComputedStyles from persisting and failing the equality assertion in
ElementAnimations::updateBaseComputedStyle().


The regression test for this bug needed to be a SimTest as it depended
on style recalcs being triggered by animations and not Javascript.
It's not possible to do this deterministically from Javascript as it
requires control over the frame times.

As part of making the SimTest ElementAnimation::animate() and
PropertyRegistration::registerProperty() required changes to reduce their
V8 dependencies.

BUG=701250
==========

[alancutter (OOO until 2018), 2017-03-17 00:11:13]

Description was changed from

==========
Ensure baseComputedStyle optimisation is cleared during registered custom
property animations

This change ensures that the cached baseComputedStyle on an animated element is
removed while custom property animations are active. This prevents stale
baseComputedStyles from persisting and failing the equality assertion in
ElementAnimations::updateBaseComputedStyle().


The regression test for this bug needed to be a SimTest as it depended
on style recalcs being triggered by animations and not Javascript.
It's not possible to do this deterministically from Javascript as it
requires control over the frame times.

As part of making the SimTest ElementAnimation::animate() and
PropertyRegistration::registerProperty() required changes to reduce their
V8 dependencies.

BUG=701250
==========

to

==========
Ensure baseComputedStyle optimisation is cleared during registered custom
property animations

This change to StyleResolver ensures that the cached baseComputedStyle
on an animated element is removed while custom property animations are
active. This prevents stale baseComputedStyles from persisting and
failing the equality assertion in ElementAnimations::updateBaseComputedStyle().


The regression test for this bug needed to be a SimTest as it depended
on style recalcs being triggered by animations and not Javascript.
It's not possible to do this deterministically from Javascript as it
requires control over the frame times.

As part of making the SimTest ElementAnimation::animate() and
PropertyRegistration::registerProperty() required changes to reduce their
V8 dependencies.

BUG=701250
==========

[alancutter (OOO until 2018), 2017-03-17 00:29:16]

On 2017/03/16 at 23:46:41, suzyh wrote:
> I'm afraid I do not understand this patch at all. The only behavioural change
seems to be changing the registerProperty argument from a ScriptState to an
ExecutionContext, and I don't know how that achieves the clearing of the
baseComputedStyle optimisation mentioned in the CL description. Please add
comments to the SimTest to explain what it's doing and how/why it tests what
it's testing.

My mistake, I messed up git here and didn't actually include the fix. All that
was just adding the SimTest and tweaking interfaces so that they could be used
by C++.
Added fix (in StyleResolver.cpp) and a comment to the test.

[suzyh_UTC10 (ex-contributor), 2017-03-17 03:03:13]

OK, that makes much more sense ^.^

lgtm

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

https://codereview.chromium.org/2757543002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:621:
elementAnimations->clearBaseComputedStyle();
Sanity-check question: Is it possible instead to not set the baseComputedStyle
in the first place, rather than set it and clear it later?

[alancutter (OOO until 2018), 2017-03-17 03:36:50]

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

https://codereview.chromium.org/2757543002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:621:
elementAnimations->clearBaseComputedStyle();
On 2017/03/17 at 03:03:13, suzyh wrote:
> Sanity-check question: Is it possible instead to not set the baseComputedStyle
in the first place, rather than set it and clear it later?

I did think of that, to not set the baseComputedStyle on the exit frame of an
animation and make this a non-issue.
I think you'd need to plumb through more data and logic to make that happen.
Much more complexity that it'd be worth despite it making sense to do.

[alancutter (OOO until 2018), 2017-03-17 03:37:25]

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

[commit-bot: I haz the power, 2017-03-17 03:37:57]

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, 2017-03-17 04:33:48]

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

[commit-bot: I haz the power, 2017-03-17 04:33:49]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[alancutter (OOO until 2018), 2017-03-22 02:57:31]

Description was changed from

==========
Ensure baseComputedStyle optimisation is cleared during registered custom
property animations

This change to StyleResolver ensures that the cached baseComputedStyle
on an animated element is removed while custom property animations are
active. This prevents stale baseComputedStyles from persisting and
failing the equality assertion in ElementAnimations::updateBaseComputedStyle().


The regression test for this bug needed to be a SimTest as it depended
on style recalcs being triggered by animations and not Javascript.
It's not possible to do this deterministically from Javascript as it
requires control over the frame times.

As part of making the SimTest ElementAnimation::animate() and
PropertyRegistration::registerProperty() required changes to reduce their
V8 dependencies.

BUG=701250
==========

to

==========
Ensure baseComputedStyle optimisation is cleared during registered custom
property animations

This change to StyleResolver ensures that the cached baseComputedStyle
on an animated element is removed while custom property animations are
active. This prevents stale baseComputedStyles from persisting and
failing the equality assertion in ElementAnimations::updateBaseComputedStyle().


The regression test for this bug needed to be a SimTest as it depended
on style recalcs being triggered by animations and not Javascript.
It's not possible to do this deterministically from Javascript as it
requires control over the frame times.

As part of making the SimTest ElementAnimation::animate() and
PropertyRegistration::registerProperty() required changes to reduce their
V8 dependencies. Additionally a test-only method was added to AnimationClock
to turn off generating synthetic frame times.

BUG=701250
==========

[alancutter (OOO until 2018), 2017-03-22 02:57:52]

The CQ bit was checked by alancutter@chromium.org

[alancutter (OOO until 2018), 2017-03-22 02:57:52]

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

[commit-bot: I haz the power, 2017-03-22 02:58:35]

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, 2017-03-22 03:07:30]

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

[commit-bot: I haz the power, 2017-03-22 03:07:31]

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

[alancutter (OOO until 2018), 2017-03-22 03:11:43]

alancutter@chromium.org changed reviewers:
+ haraken@chromium.org

[alancutter (OOO until 2018), 2017-03-22 03:11:43]

On 2017/03/22 at 03:07:31, commit-bot wrote:
> 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 for web/BUILD.gn.

[alancutter (OOO until 2018), 2017-03-22 03:13:21]

Description was changed from

==========
Ensure baseComputedStyle optimisation is cleared during registered custom
property animations

This change to StyleResolver ensures that the cached baseComputedStyle
on an animated element is removed while custom property animations are
active. This prevents stale baseComputedStyles from persisting and
failing the equality assertion in ElementAnimations::updateBaseComputedStyle().


The regression test for this bug needed to be a SimTest as it depended
on style recalcs being triggered by animations and not Javascript.
It's not possible to do this deterministically from Javascript as it
requires control over the frame times.

As part of making the SimTest ElementAnimation::animate() and
PropertyRegistration::registerProperty() required changes to reduce their
V8 dependencies. Additionally a test-only method was added to AnimationClock
to turn off generating synthetic frame times.

BUG=701250
==========

to

==========
Ensure baseComputedStyle optimisation is cleared during registered custom
property animations

This change to StyleResolver ensures that the cached baseComputedStyle
on an animated element is removed while custom property animations are
active. This prevents stale baseComputedStyles from persisting and
failing the equality assertion in ElementAnimations::updateBaseComputedStyle().


The regression test for this bug needed to be a SimTest as it depended
on style recalcs being triggered by animations and not Javascript.
It's not possible to do this deterministically from Javascript as it
requires control over the frame times.

As part of making the SimTest ElementAnimation::animate() and
PropertyRegistration::registerProperty() required changes to reduce their
V8 dependencies. Additionally a test-only method was added to
AnimationClock to turn off generating synthetic frame times.

BUG=701250
==========

[haraken, 2017-03-22 03:35:41]

On 2017/03/22 03:11:43, alancutter wrote:
> On 2017/03/22 at 03:07:31, commit-bot wrote:
> > 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 for web/BUILD.gn.

LGTM

[esprehn, 2017-03-22 03:45:32]

Description was changed from

==========
Ensure baseComputedStyle optimisation is cleared during registered custom
property animations

This change to StyleResolver ensures that the cached baseComputedStyle
on an animated element is removed while custom property animations are
active. This prevents stale baseComputedStyles from persisting and
failing the equality assertion in ElementAnimations::updateBaseComputedStyle().


The regression test for this bug needed to be a SimTest as it depended
on style recalcs being triggered by animations and not Javascript.
It's not possible to do this deterministically from Javascript as it
requires control over the frame times.

As part of making the SimTest ElementAnimation::animate() and
PropertyRegistration::registerProperty() required changes to reduce their
V8 dependencies. Additionally a test-only method was added to
AnimationClock to turn off generating synthetic frame times.

BUG=701250
==========

to

==========
Ensure baseComputedStyle optimisation is cleared during registered custom
property animations

This change to StyleResolver ensures that the cached baseComputedStyle
on an animated element is removed while custom property animations are
active. This prevents stale baseComputedStyles from persisting and
failing the equality assertion in ElementAnimations::updateBaseComputedStyle().

The regression test for this bug needed to be a SimTest as it depended
on style recalcs being triggered by animations and not Javascript.
It's not possible to do this deterministically from Javascript as it
requires control over the frame times.

As part of making the SimTest ElementAnimation::animate() and
PropertyRegistration::registerProperty() required changes to reduce their
V8 dependencies. Additionally a test-only method was added to
AnimationClock to turn off generating synthetic frame times.

BUG=701250
==========

[esprehn, 2017-03-22 03:46:17]

It should be possible to use the baseComputedStyle for custom property
animations though? I'm not sure I understand how custom properties animating are
different from non-custom ones.

[alancutter (OOO until 2018), 2017-03-23 00:26:40]

On 2017/03/22 at 03:46:17, esprehn wrote:
> It should be possible to use the baseComputedStyle for custom property
animations though? I'm not sure I understand how custom properties animating are
different from non-custom ones.

Custom properties (animated and not) are computed before non-custom properties
are applied so that var() references can be resolved correctly. The
baseComputedStyle snapshot occurs after non-custom property style rules are
applied which is after custom property animations have applied and would
erroneously include animation state if we didn't disable it.

Here's a short doc I wrote up about the complication of custom property
animation and style resolver optimisations.
https://docs.google.com/document/d/154t9CUycSKGKeEjEVwgtZzRYizeZxzIOX5YE5JOfR...

[alancutter (OOO until 2018), 2017-03-23 00:26:45]

The CQ bit was checked by alancutter@chromium.org

[commit-bot: I haz the power, 2017-03-23 00:27:40]

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, 2017-03-23 02:43:09]

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

[commit-bot: I haz the power, 2017-03-23 02:43:11]

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

[esprehn, 2017-03-28 21:46:23]

On mac it says AnimationSimTest.CustomPropertyBaseComputedStyle is failing, what
happened that it passed on the other bots?

[alancutter (OOO until 2018), 2017-04-03 12:12:03]

On 2017/03/28 at 21:46:23, esprehn wrote:
> On mac it says AnimationSimTest.CustomPropertyBaseComputedStyle is failing,
what happened that it passed on the other bots?

The test failure should be fixed by: https://codereview.chromium.org/2790083002
Seems like Mac has more checks in its executables than the other platforms.

[alancutter (OOO until 2018), 2017-04-03 12:15:35]

The CQ bit was checked by alancutter@chromium.org

[commit-bot: I haz the power, 2017-04-03 12:15:44]

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, 2017-04-03 14:08:50]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491221735639540,
"parent_rev": "50148353a87f81026afab0ea2647ad66eb25cf8c", "commit_rev":
"767e438649154d1985a803c93111e0860a29fa2f"}

[commit-bot: I haz the power, 2017-04-03 14:09:41]

Description was changed from

==========
Ensure baseComputedStyle optimisation is cleared during registered custom
property animations

This change to StyleResolver ensures that the cached baseComputedStyle
on an animated element is removed while custom property animations are
active. This prevents stale baseComputedStyles from persisting and
failing the equality assertion in ElementAnimations::updateBaseComputedStyle().

The regression test for this bug needed to be a SimTest as it depended
on style recalcs being triggered by animations and not Javascript.
It's not possible to do this deterministically from Javascript as it
requires control over the frame times.

As part of making the SimTest ElementAnimation::animate() and
PropertyRegistration::registerProperty() required changes to reduce their
V8 dependencies. Additionally a test-only method was added to
AnimationClock to turn off generating synthetic frame times.

BUG=701250
==========

to

==========
Ensure baseComputedStyle optimisation is cleared during registered custom
property animations

This change to StyleResolver ensures that the cached baseComputedStyle
on an animated element is removed while custom property animations are
active. This prevents stale baseComputedStyles from persisting and
failing the equality assertion in ElementAnimations::updateBaseComputedStyle().

The regression test for this bug needed to be a SimTest as it depended
on style recalcs being triggered by animations and not Javascript.
It's not possible to do this deterministically from Javascript as it
requires control over the frame times.

As part of making the SimTest ElementAnimation::animate() and
PropertyRegistration::registerProperty() required changes to reduce their
V8 dependencies. Additionally a test-only method was added to
AnimationClock to turn off generating synthetic frame times.

BUG=701250

Review-Url: https://codereview.chromium.org/2757543002
Cr-Commit-Position: refs/heads/master@{#461423}
Committed:
https://chromium.googlesource.com/chromium/src/+/767e438649154d1985a803c93111...
==========

[commit-bot: I haz the power, 2017-04-03 14:09:42]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/767e438649154d1985a803c93111...