Defer compositor keyframe snapshots until the next style resolve

Defer compositor keyframe snapshots until the next style resolve

Previously compositor keyframes were being captured as soon as an animation
was created. In the case of element.animate() there is not enough context
to know the correct parentStyle to use resulting in crashes
when computing "inherit" in corner case scenarios.

By deferring the compositor keyframe snapshotting until we are in a
style resolve we can pass through the correct parentStyle and avoid
the crashes.

Additionally by deferring we are able to avoid forcing a style recalc
during element.animate() of a composited property.

BUG=593252, 539793, 534122, 587257
Committed: https://crrev.com/3ebd5c3b08a5f861d8268402f3d9d76f5d7e306f
Cr-Commit-Position: refs/heads/master@{#403861}

[alancutter (OOO until 2018), 2016-06-08 07:21:13]

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

[suzyh_UTC10 (ex-contributor), 2016-06-08 07:29:56]

Alan, could you describe the performance and memory implications of the
ComputedStyle::clone and the style-resolve-within-style-resolve that you were
telling me about? I'm actually not sure where the
style-resolve-within-style-resolve occurs, so perhaps it should be highlighted
in the code with a comment/TODO.

[alancutter (OOO until 2018), 2016-06-08 07:52:11]

On 2016/06/08 at 07:29:56, suzyh wrote:
> Alan, could you describe the performance and memory implications of the
ComputedStyle::clone

ComputedStyle::clone() has been shifted from element.animate() to
StyleResolver::applyAnimatedProperties(), this patch doesn't introduce any calls
to clone() that weren't there before.
The clone happens once per composited animation keyframe, it doesn't occur
multiple times per frame except for specific changes in style that invalidate
the composited animation (see
CSSAnimations::calculateCompositorAnimationUpdate()).

> and the style-resolve-within-style-resolve that you were telling me about? I'm
actually not sure where the style-resolve-within-style-resolve occurs, so
perhaps it should be highlighted in the code with a comment/TODO.

Added TODO.

[alancutter (OOO until 2018), 2016-06-08 08:00:12]

On 2016/06/08 at 07:52:11, alancutter wrote:
> On 2016/06/08 at 07:29:56, suzyh wrote:
> > and the style-resolve-within-style-resolve that you were telling me about?
I'm actually not sure where the style-resolve-within-style-resolve occurs, so
perhaps it should be highlighted in the code with a comment/TODO.
> 
> Added TODO.

I should note that this patch isn't introducing the
style-resolve-within-style-resolve behaviour, it was already being done for CSS
animations previously.

[alancutter (OOO until 2018), 2016-06-08 11:12:12]

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

[commit-bot: I haz the power, 2016-06-08 11:12:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2043273002/40001

[commit-bot: I haz the power, 2016-06-08 11:56:02]

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

[commit-bot: I haz the power, 2016-06-08 11:56:03]

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

[alancutter (OOO until 2018), 2016-06-08 23:09:08]

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

[commit-bot: I haz the power, 2016-06-08 23:09:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2043273002/40001

[commit-bot: I haz the power, 2016-06-09 00:02:44]

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

[commit-bot: I haz the power, 2016-06-09 00:02:45]

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

[alancutter (OOO until 2018), 2016-06-09 00:32:33]

Description was changed from

==========
Defer compositor keyframe snapshots until the next style resolve

Previously compositor keyframes were being captured as soon as an animation
was created. In the case of element.animate() there is not enough context
to know the correct parentStyle to use resulting in crashes
when computing "inherit" in corner case scenarios.

By deferring the compositor keyframe snapshotting until we are in a
style resolve we can pass through the correct parentStyle and avoid
the crashes.

Additionally by deferring we are able to avoid forcing a style recalc
during element.animate() of a composited property.

BUG=593252, 539793, 534122
==========

to

==========
Defer compositor keyframe snapshots until the next style resolve

Previously compositor keyframes were being captured as soon as an animation
was created. In the case of element.animate() there is not enough context
to know the correct parentStyle to use resulting in crashes
when computing "inherit" in corner case scenarios.

By deferring the compositor keyframe snapshotting until we are in a
style resolve we can pass through the correct parentStyle and avoid
the crashes.

Additionally by deferring we are able to avoid forcing a style recalc
during element.animate() of a composited property.

BUG=593252, 539793, 534122, 587257
==========

[alancutter (OOO until 2018), 2016-06-09 05:51:45]

Interesting that the crash seen in
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...
has the stack trace that https://codereview.chromium.org/2047283002 is aiming to
fix.

[Timothy Loh, 2016-06-09 06:54:24]

lgtm

[alancutter (OOO until 2018), 2016-06-09 07:19:07]

The CQ bit was checked by alancutter@chromium.org

[alancutter (OOO until 2018), 2016-06-09 07:19:07]

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/2043273002/#ps80001
(title: "Rebased onto https://codereview.chromium.org/2039133002")

[commit-bot: I haz the power, 2016-06-09 07:19:30]

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

[commit-bot: I haz the power, 2016-06-09 07:31:52]

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

[commit-bot: I haz the power, 2016-06-09 07:31:53]

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

[alancutter (OOO until 2018), 2016-06-10 00:40:52]

The CQ bit was checked by alancutter@chromium.org

[commit-bot: I haz the power, 2016-06-10 00:41:44]

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

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

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

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

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

[alancutter (OOO until 2018), 2016-06-10 08:32:48]

On 2016/06/10 at 01:28:40, commit-bot wrote:
> Try jobs failed on following builders:
>   linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

This patch seems to be exposing crbug.com/616674 on ChromeOS.

[alancutter (OOO until 2018), 2016-06-10 08:47:13]

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

[commit-bot: I haz the power, 2016-06-10 08:47:23]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2043273002/100001

[commit-bot: I haz the power, 2016-06-10 10:38:02]

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

[commit-bot: I haz the power, 2016-06-10 10:38:04]

Dry run: This issue passed the CQ dry run.

[alancutter (OOO until 2018), 2016-07-06 04:29:18]

The CQ bit was checked by alancutter@chromium.org

[alancutter (OOO until 2018), 2016-07-06 04:29:18]

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/2043273002/#ps120001
(title: "Rebased")

[commit-bot: I haz the power, 2016-07-06 04:29:31]

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-07-06 07:12:58]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-07-06 07:13:07]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-06 07:15:50]

Description was changed from

==========
Defer compositor keyframe snapshots until the next style resolve

Previously compositor keyframes were being captured as soon as an animation
was created. In the case of element.animate() there is not enough context
to know the correct parentStyle to use resulting in crashes
when computing "inherit" in corner case scenarios.

By deferring the compositor keyframe snapshotting until we are in a
style resolve we can pass through the correct parentStyle and avoid
the crashes.

Additionally by deferring we are able to avoid forcing a style recalc
during element.animate() of a composited property.

BUG=593252, 539793, 534122, 587257
==========

to

==========
Defer compositor keyframe snapshots until the next style resolve

Previously compositor keyframes were being captured as soon as an animation
was created. In the case of element.animate() there is not enough context
to know the correct parentStyle to use resulting in crashes
when computing "inherit" in corner case scenarios.

By deferring the compositor keyframe snapshotting until we are in a
style resolve we can pass through the correct parentStyle and avoid
the crashes.

Additionally by deferring we are able to avoid forcing a style recalc
during element.animate() of a composited property.

BUG=593252, 539793, 534122, 587257

Committed: https://crrev.com/3ebd5c3b08a5f861d8268402f3d9d76f5d7e306f
Cr-Commit-Position: refs/heads/master@{#403861}
==========

[commit-bot: I haz the power, 2016-07-06 07:15:52]

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