Support compositing for active animations in SPv2.

Support compositing for active animations in SPv2.

Revise CompositorAnimations::canStartAnimationOnCompositor() to
consider direct compositing reasons rather than layer compositing
state.

BUG=674317
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/2bf83fd5b59fe18bc2a25e05f29a965a4887dc6b
Cr-Commit-Position: refs/heads/master@{#439657}

[wkorman, 2016-12-15 23:00:42]

Description was changed from

==========
Support compositing for active animations in SPv2.

Revise CompositorAnimations::canStartAnimationOnCompositor() to
consider direct compositing reasons rather than layer compositing
state.

BUG=674317
==========

to

==========
Support compositing for active animations in SPv2.

Revise CompositorAnimations::canStartAnimationOnCompositor() to
consider direct compositing reasons rather than layer compositing
state.

BUG=674317
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[wkorman, 2016-12-15 23:05:58]

wkorman@chromium.org changed reviewers:
+ chrishtr@chromium.org, pdr@chromium.org

[wkorman, 2016-12-15 23:06:00]

Will look at adding unit tests if approach passes your initial muster, as there
is yet much I likely do not know.

https://codereview.chromium.org/2578343002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/virtual/threaded/animations/composited-animation-style-update.html
(right):

https://codereview.chromium.org/2578343002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/virtual/threaded/animations/composited-animation-style-update.html:42:
testRunner.notifyDone();
This test now passes with SPv2 enabled. Can I do something to enable it as
passing in FlagExpectations/enable-slimming-paint-v2? What approach is
preferred?

https://codereview.chromium.org/2578343002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/animation/CompositorAnimations.cpp (right):

https://codereview.chromium.org/2578343002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/CompositorAnimations.cpp:350: //
TODO(wkorman): Consider effect node for opacity.
Are you adding direct compositing reasons to effect node, or should I do it?

https://codereview.chromium.org/2578343002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2578343002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/FrameView.cpp:2905: if
(RuntimeEnabledFeatures::slimmingPaintV2Enabled())
Will remove before landing, this is manual patch of
https://codereview.chromium.org/2570723002

https://codereview.chromium.org/2578343002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2578343002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:322: if
(CompositingReasonFinder::requiresCompositingForAnimation(
Are we not using CompositingReasonFinder::potentialCompositingReasonsFromStyle()
directly just so that we can be thoughtful and integrate these one at a time?

[chrishtr, 2016-12-16 00:31:07]

Looks legit. Still need to plumb ids for it to work right?

https://codereview.chromium.org/2578343002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/animation/CompositorAnimations.cpp (right):

https://codereview.chromium.org/2578343002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/CompositorAnimations.cpp:350: //
TODO(wkorman): Consider effect node for opacity.
On 2016/12/15 at 23:06:00, wkorman wrote:
> Are you adding direct compositing reasons to effect node, or should I do it?

You can add it. Follow the lead of the other patch and mimic the reasons
computed in CompositingReasonsFinder.

[wkorman, 2016-12-16 23:06:31]

On 2016/12/16 at 00:31:07, chrishtr wrote:
> Looks legit. Still need to plumb ids for it to work right?

The test updated in this change now passes since all it does is check that the
animation is on the compositor. Other currently failing for SPv2 composited
animation tests should require the rest of the plumbing to fix, though, yes.

[wkorman, 2016-12-16 23:08:00]

https://codereview.chromium.org/2578343002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/animation/CompositorAnimations.cpp (right):

https://codereview.chromium.org/2578343002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/CompositorAnimations.cpp:350: //
TODO(wkorman): Consider effect node for opacity.
On 2016/12/16 at 00:31:06, chrishtr wrote:
> On 2016/12/15 at 23:06:00, wkorman wrote:
> > Are you adding direct compositing reasons to effect node, or should I do it?
> 
> You can add it. Follow the lead of the other patch and mimic the reasons
computed in CompositingReasonsFinder.

Will do in subsequent change.

https://codereview.chromium.org/2578343002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/2578343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:1702:
FRIEND_TEST_ALL_PREFIXES(AnimationCompositorAnimationsTest,
I am not happy about having to do this, but it's just for testing. I'd like to
not have to use a whole RenderingTest in CompositorAnimations.cpp. Open to
thoughts on another way to essentially tack an explicit transform node onto a
LayoutObject's paint properties.

[chrishtr, 2016-12-17 00:05:43]

lgtm

https://codereview.chromium.org/2578343002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/2578343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:1702:
FRIEND_TEST_ALL_PREFIXES(AnimationCompositorAnimationsTest,
On 2016/12/16 at 23:08:00, wkorman wrote:
> I am not happy about having to do this, but it's just for testing. I'd like to
not have to use a whole RenderingTest in CompositorAnimations.cpp. Open to
thoughts on another way to essentially tack an explicit transform node onto a
LayoutObject's paint properties.

Which method do you need that is private?

[chrishtr, 2016-12-17 00:05:44]

lgtm

https://codereview.chromium.org/2578343002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/2578343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:1702:
FRIEND_TEST_ALL_PREFIXES(AnimationCompositorAnimationsTest,
On 2016/12/16 at 23:08:00, wkorman wrote:
> I am not happy about having to do this, but it's just for testing. I'd like to
not have to use a whole RenderingTest in CompositorAnimations.cpp. Open to
thoughts on another way to essentially tack an explicit transform node onto a
LayoutObject's paint properties.

Which method do you need that is private?

[wkorman, 2016-12-17 00:18:27]

https://codereview.chromium.org/2578343002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/2578343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:1702:
FRIEND_TEST_ALL_PREFIXES(AnimationCompositorAnimationsTest,
On 2016/12/17 at 00:05:43, chrishtr wrote:
> On 2016/12/16 at 23:08:00, wkorman wrote:
> > I am not happy about having to do this, but it's just for testing. I'd like
to not have to use a whole RenderingTest in CompositorAnimations.cpp. Open to
thoughts on another way to essentially tack an explicit transform node onto a
LayoutObject's paint properties.
> 
> Which method do you need that is private?

CompositorAnimationsTest has to be able to call:

layoutObject->getMutableForPainting().ensurePaintProperties()

so that we can use updateTransform() to set an explicit transform node. This
method is protected
and per comment below in line 1705 and the friend statement in line 1701, only
PaintPropertyTreeBuilder can
access it currently.

Some other options, none of which seem better IMO but I can be swayed:

- add a public getPaintPropertiesForTesting() method
- loosen restriction on access to these methods (just make them public)
- use a RenderingTest in CompositorAnimationsTest (would make the test added to
CompositorAnimationsTest more deviant than the other tests already there, and
heavier weight than needed, and may need to fiddle other animation infra that
using their existing unit test infra takes care of, but we could do it). I'm
somewhat sketched by our use of RenderingTest in unit tests as using them
produces something more like an integration test for cases like this.
- some more intrusive class extension/override approach, or via interfaces,
essentially, to allow customizing LayoutObject / PaintPropertyTreeBuilder
behavior for test. For example cc/ has a pattern for test code where they will
create TestXxx classes that extend the non-test class and override methods to
produce what they want, see cc/test/test_image_factory.h for example.

[wkorman, 2016-12-17 01:21:04]

The CQ bit was checked by wkorman@chromium.org

[wkorman, 2016-12-17 01:21:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/2578343002/#ps40001
(title: "Sync to head.")

[commit-bot: I haz the power, 2016-12-17 01:21:39]

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-12-17 03:33:46]

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

[commit-bot: I haz the power, 2016-12-17 03:33:47]

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

[wkorman, 2016-12-17 10:31:45]

On 2016/12/17 at 03:33:47, commit-bot wrote:
> Try jobs failed on following builders:
>   win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

Hmm, a couple of tests that look to have animations are failing with:

STDERR:
[4024:616:1216/182829.994:5769119:FATAL:findpropertiesneedingupdate.h(143)]
Check failed: !!m_originalProperties->transform() ==
!!objectProperties->transform(). Property was created or deleted without the
layout object ("LayoutBlockFlow DIV id='animated'") needing a paint property
update.

I haven't investigated yet but this check may need revision since animation
produces transform node and I am not sure whether we still need to mark the
object for a paint property update.

[wkorman, 2016-12-19 22:45:02]

The CQ bit was checked by wkorman@chromium.org

[wkorman, 2016-12-19 22:45:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/2578343002/#ps60001
(title: "setNeedsPaintPropertyUpdate on PaintLayer::styleDidChange.")

[commit-bot: I haz the power, 2016-12-19 22:45:39]

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-12-20 00:48:28]

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

[commit-bot: I haz the power, 2016-12-20 00:48:29]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[wkorman, 2016-12-20 00:56:44]

The CQ bit was checked by wkorman@chromium.org

[commit-bot: I haz the power, 2016-12-20 00:57:19]

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-12-20 01:37:15]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1482195404364450,
"parent_rev": "bc6130f835ba510955196798a764157782236048", "commit_rev":
"aee734d842680e55ae486a14c0d5bcc8ab51b2c3"}

[commit-bot: I haz the power, 2016-12-20 01:38:05]

Description was changed from

==========
Support compositing for active animations in SPv2.

Revise CompositorAnimations::canStartAnimationOnCompositor() to
consider direct compositing reasons rather than layer compositing
state.

BUG=674317
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Support compositing for active animations in SPv2.

Revise CompositorAnimations::canStartAnimationOnCompositor() to
consider direct compositing reasons rather than layer compositing
state.

BUG=674317
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2578343002
==========

[commit-bot: I haz the power, 2016-12-20 01:38:06]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-20 01:43:26]

Description was changed from

==========
Support compositing for active animations in SPv2.

Revise CompositorAnimations::canStartAnimationOnCompositor() to
consider direct compositing reasons rather than layer compositing
state.

BUG=674317
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2578343002
==========

to

==========
Support compositing for active animations in SPv2.

Revise CompositorAnimations::canStartAnimationOnCompositor() to
consider direct compositing reasons rather than layer compositing
state.

BUG=674317
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/2bf83fd5b59fe18bc2a25e05f29a965a4887dc6b
Cr-Commit-Position: refs/heads/master@{#439657}
==========

[commit-bot: I haz the power, 2016-12-20 01:43:28]

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