[SPv2] Decomposite otherwise-compositable animations that paint nothing.

[SPv2] Decomposite otherwise-compositable animations that paint nothing.

In SPv1 we create cc layers for composited animations even if they
paint no content.

In SPv2, we only create cc layers when something is painted,
a.k.a., there is a PaintChunk.

This means that in SPv1 we could be assured that a composited
animation that painted no content would still have a cc layer
with the animation's compositor element id set on it, but in
SPv2 such an animation will have no cc layer and accordingly
it is impossible to run it as a composited animation.

In SPv2, we choose to not composite such animations.

To do otherwise would require:

1. code complexity to identify when this is the case,
through a dummy PaintChunk or similar, and ensure that
we create a cc layer even though there's nothing in need
of painting.

2. compositor resource overhead to maintain a layer
solely for the purpose of executing a composited animation
even though it is not producing any visible content.

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

Review-Url: https://codereview.chromium.org/2724083002
Cr-Commit-Position: refs/heads/master@{#462748}
Committed: https://chromium.googlesource.com/chromium/src/+/efeb13b3ba0f7604fe7f61a69d83870d1a5f51fa

[wkorman, 2017-03-01 18:43:24]

Description was changed from

==========
[SPv2] Decomposite otherwise-compositable animations that paint nothing.

BUG=692310
==========

to

==========
[SPv2] Decomposite otherwise-compositable animations that paint nothing.

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

[wkorman, 2017-03-15 08:49:54]

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

[commit-bot: I haz the power, 2017-03-15 08:50:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[wkorman, 2017-03-15 08:57:10]

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

[wkorman, 2017-03-15 08:57:11]

Needs cleanup and add'l tests but I wanted feedback on general approach. More
background in linked bug.

In particular:

- Most of change is just passing param with element id set down into the two
Animation methods where it's required. If there were somewhere we thought
suitable to store the set and avoid passing it down could make change smaller.

- The compositable animation constraint methods are quite unintuitive to someone
new to the code base. I documented my investigation including a call graph, see
http://crbug.com/692310#c7. I mention just so we can keep in mind, could be
worth method rename/refactor, perhaps this grew organically over time.

- Calling out my note in http://crbug.com/692310#c8 for your review as it
pertains to the goals this change is progressive toward.

[commit-bot: I haz the power, 2017-03-15 09:29:24]

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

[commit-bot: I haz the power, 2017-03-15 09:29:24]

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

[pdr., 2017-03-16 21:20:05]

https://codereview.chromium.org/2724083002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/Animation.h (right):

https://codereview.chromium.org/2724083002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/Animation.h:34: #include <memory>
It took me a little while to page this back in. Can you add a summary to the
description?

The big points seem to be:
* In SPV1, layers are always created for composited animations so we are sure
the compositor gets the associated element id off the layer.
* In SPV2 we can optimize out even creating a paint chunk for opacity=0
animations, and the compositor never finds out about opacity=0 animations.
* This change ensures SPV2 only starts compositor animations with element ids
that were composited, effectively forcing non-composited animations back to the
main thread.

https://codereview.chromium.org/2724083002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2724083002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:3107:
compositedAnimationElementIds.emplace();
Is "animation" correct here? Are we returning composited animation element ids
or just a list of all element ids with composited effects?

https://codereview.chromium.org/2724083002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:3108:
pushPaintArtifactToCompositor(compositedAnimationElementIds.value());
Idea for not needing to pass this Optional around so much:
It looks like the Document has access to all Animations through the
AnimationTimeline it owns. Could we add an ElementId to Animation and then add a
function call here to
"suppressNonCompositedAnimations(compositedAnimationElementIds)". This function
would iterate through all Animations and suppress any where the ElementId is not
in compositedAnimationElementIds. Animation::canStartAnimationOnCompositor then
returns false for any suppressed animation.

https://codereview.chromium.org/2724083002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:3117:
DocumentAnimations::updateAnimations(layoutView()->document(),
Can this call be moved right after pushPaintArtifactToCompositor to avoid
needing an Optional outside the SPV2 codepath?

https://codereview.chromium.org/2724083002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h (right):

https://codereview.chromium.org/2724083002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h:113: // Returns
a stable compositor element id for the given layout
Nit: remove?

[wkorman, 2017-03-16 21:53:32]

Thanks for feedback, replies:

https://codereview.chromium.org/2724083002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/Animation.h (right):

https://codereview.chromium.org/2724083002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/Animation.h:34: #include <memory>
On 2017/03/16 21:20:04, pdr. wrote:
> It took me a little while to page this back in. Can you add a summary to the
> description?

Will do.

> The big points seem to be:
> * In SPV1, layers are always created for composited animations so we are sure
> the compositor gets the associated element id off the layer.
> * In SPV2 we can optimize out even creating a paint chunk for opacity=0
> animations, and the compositor never finds out about opacity=0 animations.

Rather than being about opacity = 0, this patch is aimed at the issue that, for
animated elements that paint no content at all, we never create a layer, thus
there is no layer to associated with a compositor element id in cc, and cc can't
run the composited animation.

The opacity = 0 issue you may be recalling is the one we addressed previously in
http://crrev.com/2652273004 but that case still involved elements that do
actually paint something (for example, they have a background with some color).
For elements that literally end up painting nothing at all because they have no
content, background or other painting-required style, we hit the case this bug
is about and this patch is trying to address.

> * This change ensures SPV2 only starts compositor animations with element ids
> that were composited, effectively forcing non-composited animations back to
the
> main thread.

Correct, I would rephrase the "element ids that were composited" bit slightly
for clarity. Something like, "...only starts compositor animations with element
ids for which we created a cc layer."

https://codereview.chromium.org/2724083002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2724083002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:3107:
compositedAnimationElementIds.emplace();
On 2017/03/16 21:20:05, pdr. wrote:
> Is "animation" correct here? Are we returning composited animation element ids
> or just a list of all element ids with composited effects?

Name needs improvement, agreed, let's come up with something better.

This is populated with the set of compositor element ids for all cc layers
created that had a compositor element id set on *either* the transform or effect
node.

Perhaps compositedLayerElementIds? I hate long names. compositedElementIds?

https://codereview.chromium.org/2724083002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:3108:
pushPaintArtifactToCompositor(compositedAnimationElementIds.value());
On 2017/03/16 21:20:05, pdr. wrote:
> Idea for not needing to pass this Optional around so much:
> It looks like the Document has access to all Animations through the
> AnimationTimeline it owns. Could we add an ElementId to Animation and then add
a
> function call here to
> "suppressNonCompositedAnimations(compositedAnimationElementIds)". This
function
> would iterate through all Animations and suppress any where the ElementId is
not
> in compositedAnimationElementIds. Animation::canStartAnimationOnCompositor
then
> returns false for any suppressed animation.

I like this general idea. How do you envision marking an animation as
"suppressed"? Add a bool to Animation? Some angst at that idea given:

1. how much complexity is already around whether an animation is compositable,
see my diagram and notes at http://crbug.com/692310#c7

2. the lifecycle of an animation, which can include being cancelled and such,
and the 'suppressed' bool would only be relevant when the animation is first
coming into existence, but would then hang around thereafter taking up space and
adding risk of bugs, potential misunderstanding/misuse by other eng, etc.

https://codereview.chromium.org/2724083002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:3117:
DocumentAnimations::updateAnimations(layoutView()->document(),
On 2017/03/16 21:20:05, pdr. wrote:
> Can this call be moved right after pushPaintArtifactToCompositor to avoid
> needing an Optional outside the SPV2 codepath?

Yes, though I think we'd want to duplicate the two DCHECKs above within that
block, and put the ones here in an 'else' block for non-SPv2, since we want them
to precede the call to updateAnimations. Still seems worth moving it as you
suggest to avoid constructing Optional for non-SPv2.

https://codereview.chromium.org/2724083002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h (right):

https://codereview.chromium.org/2724083002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h:113: // Returns
a stable compositor element id for the given layout
On 2017/03/16 21:20:05, pdr. wrote:
> Nit: remove?

Will fix this as part of cleanup, I think we want to share this method's logic
in some manner, exposing it like I have commented out here may be one way, more
soon.

[wkorman, 2017-03-16 22:06:02]

Description was changed from

==========
[SPv2] Decomposite otherwise-compositable animations that paint nothing.

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

to

==========
[SPv2] Decomposite otherwise-compositable animations that paint nothing.

In SPv1 we create cc layers for composited animations even if they
paint no content.

In SPv2, we only create cc layers when something is painted,
a.k.a., there is a PaintChunk.

This means that in SPv1 we could be assured that a composited
animation that painted no content would still have a cc layer
with the animation's compositor element id set on it, but in
SPv2 such an animation will have no cc layer and accordingly
it is impossible to run it as a composited animation.

In SPv2, we choose to not composite such animations.

To do otherwise would require (1) code complexity to identify
when this is the case, through a dummy PaintChunk or
similar, and (2) compositor resource overhead to maintain
a layer solely for the purpose of executing a composited
animation even though it is not producing any visible content.

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

[wkorman, 2017-03-16 22:07:35]

Description was changed from

==========
[SPv2] Decomposite otherwise-compositable animations that paint nothing.

In SPv1 we create cc layers for composited animations even if they
paint no content.

In SPv2, we only create cc layers when something is painted,
a.k.a., there is a PaintChunk.

This means that in SPv1 we could be assured that a composited
animation that painted no content would still have a cc layer
with the animation's compositor element id set on it, but in
SPv2 such an animation will have no cc layer and accordingly
it is impossible to run it as a composited animation.

In SPv2, we choose to not composite such animations.

To do otherwise would require (1) code complexity to identify
when this is the case, through a dummy PaintChunk or
similar, and (2) compositor resource overhead to maintain
a layer solely for the purpose of executing a composited
animation even though it is not producing any visible content.

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

to

==========
[SPv2] Decomposite otherwise-compositable animations that paint nothing.

In SPv1 we create cc layers for composited animations even if they
paint no content.

In SPv2, we only create cc layers when something is painted,
a.k.a., there is a PaintChunk.

This means that in SPv1 we could be assured that a composited
animation that painted no content would still have a cc layer
with the animation's compositor element id set on it, but in
SPv2 such an animation will have no cc layer and accordingly
it is impossible to run it as a composited animation.

In SPv2, we choose to not composite such animations.

To do otherwise would require:

1. code complexity to identify when this is the case,
through a dummy PaintChunk or similar, and ensure that
we create a cc layer even though there's nothing in need
of painting.

2. compositor resource overhead to maintain a layer
solely for the purpose of executing a composited animation
even though it is not producing any visible content.

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

[pdr., 2017-03-16 22:16:01]

https://codereview.chromium.org/2724083002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2724083002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:3107:
compositedAnimationElementIds.emplace();
On 2017/03/16 at 21:53:31, wkorman wrote:
> On 2017/03/16 21:20:05, pdr. wrote:
> > Is "animation" correct here? Are we returning composited animation element
ids
> > or just a list of all element ids with composited effects?
> 
> Name needs improvement, agreed, let's come up with something better.
> 
> This is populated with the set of compositor element ids for all cc layers
created that had a compositor element id set on *either* the transform or effect
node.
> 
> Perhaps compositedLayerElementIds? I hate long names. compositedElementIds?

compositedElementIds SGTM

short is beautiful

https://codereview.chromium.org/2724083002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:3108:
pushPaintArtifactToCompositor(compositedAnimationElementIds.value());
On 2017/03/16 at 21:53:31, wkorman wrote:
> On 2017/03/16 21:20:05, pdr. wrote:
> > Idea for not needing to pass this Optional around so much:
> > It looks like the Document has access to all Animations through the
> > AnimationTimeline it owns. Could we add an ElementId to Animation and then
add a
> > function call here to
> > "suppressNonCompositedAnimations(compositedAnimationElementIds)". This
function
> > would iterate through all Animations and suppress any where the ElementId is
not
> > in compositedAnimationElementIds. Animation::canStartAnimationOnCompositor
then
> > returns false for any suppressed animation.
> 
> I like this general idea. How do you envision marking an animation as
"suppressed"? Add a bool to Animation? Some angst at that idea given:
> 
> 1. how much complexity is already around whether an animation is compositable,
see my diagram and notes at http://crbug.com/692310#c7
> 
> 2. the lifecycle of an animation, which can include being cancelled and such,
and the 'suppressed' bool would only be relevant when the animation is first
coming into existence, but would then hang around thereafter taking up space and
adding risk of bugs, potential misunderstanding/misuse by other eng, etc.

I was thinking about re-using setEffectSuppressed but you're right that that
isn't a great approach. Would a new bool be so bad? We could completely control
it (e.g.,
animation.setCompositorDeclinedAnimationResponsibility(!compositedElementIds.contains(animation.elementId)))
and include a little comment describing the use. I agree that it would add
complexity to the overall lifecycle, but isn't a bool just making explicit what
is implicitly happening when canStartAnimationOnCompositor returns false due to
compositing?

https://codereview.chromium.org/2724083002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:3117:
DocumentAnimations::updateAnimations(layoutView()->document(),
On 2017/03/16 at 21:53:31, wkorman wrote:
> On 2017/03/16 21:20:05, pdr. wrote:
> > Can this call be moved right after pushPaintArtifactToCompositor to avoid
> > needing an Optional outside the SPV2 codepath?
> 
> Yes, though I think we'd want to duplicate the two DCHECKs above within that
block, and put the ones here in an 'else' block for non-SPv2, since we want them
to precede the call to updateAnimations. Still seems worth moving it as you
suggest to avoid constructing Optional for non-SPv2.

Do the checks actually need to come before updateAnimations? I thought not, but
if they do then I could go either way (my original suggestion, or your original
code).

[wkorman, 2017-03-16 22:32:25]

https://codereview.chromium.org/2724083002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2724083002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:3108:
pushPaintArtifactToCompositor(compositedAnimationElementIds.value());
On 2017/03/16 22:16:01, pdr. wrote:
> On 2017/03/16 at 21:53:31, wkorman wrote:
> > On 2017/03/16 21:20:05, pdr. wrote:
> > > Idea for not needing to pass this Optional around so much:
> > > It looks like the Document has access to all Animations through the
> > > AnimationTimeline it owns. Could we add an ElementId to Animation and then
> add a
> > > function call here to
> > > "suppressNonCompositedAnimations(compositedAnimationElementIds)". This
> function
> > > would iterate through all Animations and suppress any where the ElementId
is
> not
> > > in compositedAnimationElementIds. Animation::canStartAnimationOnCompositor
> then
> > > returns false for any suppressed animation.
> > 
> > I like this general idea. How do you envision marking an animation as
> "suppressed"? Add a bool to Animation? Some angst at that idea given:
> > 
> > 1. how much complexity is already around whether an animation is
compositable,
> see my diagram and notes at http://crbug.com/692310#c7
> > 
> > 2. the lifecycle of an animation, which can include being cancelled and
such,
> and the 'suppressed' bool would only be relevant when the animation is first
> coming into existence, but would then hang around thereafter taking up space
and
> adding risk of bugs, potential misunderstanding/misuse by other eng, etc.
> 
> I was thinking about re-using setEffectSuppressed but you're right that that
> isn't a great approach. Would a new bool be so bad? We could completely
control
> it (e.g.,
>
animation.setCompositorDeclinedAnimationResponsibility(!compositedElementIds.contains(animation.elementId)))
> and include a little comment describing the use. I agree that it would add
> complexity to the overall lifecycle, but isn't a bool just making explicit
what
> is implicitly happening when canStartAnimationOnCompositor returns false due
to
> compositing?

Yes, but the overhead of an omnipresent bool field is easier to rationalize
when it's expected to be reused for more of the lifetime of the containing
object.

In this case we actually have the needed data from the very start
of the required code path (after call into PAC::update through the entire
code path that handles starting any composited animations), and we know
(well, we currently believe that) we'll never need to reference that data
again after the animation is started.

Having talked through it, it still feels to me more like we should just
pass it down, but I can be convinced otherwise. It's one bit on each
animation and we've already got similar bits like m_effectSuppressed
(I forgot about that one). Yeah, repurposing that seems risky as it's
intended to be used by the inspector (which is not documented anywhere
on the method or field, apparent only through code search, alas).

[wkorman, 2017-03-17 20:58:51]

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

[commit-bot: I haz the power, 2017-03-17 20:59:16]

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 22:09:27]

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

[commit-bot: I haz the power, 2017-03-17 22:09:28]

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

[alancutter (OOO until 2018), 2017-03-20 06:21:09]

core/animation/ lgtm after comments addressed.

https://codereview.chromium.org/2724083002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/Animation.cpp (right):

https://codereview.chromium.org/2724083002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/Animation.cpp:739: // which case we
trust the compositing logic will create a layer if needed.
We should assert the relevant RuntimeEnabledFeature status if it determines
has_value().

https://codereview.chromium.org/2724083002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/Animation.cpp:744:
createCompositorElementId(DOMNodeIds::idForNode(effect->target()),
Need null check for target.

https://codereview.chromium.org/2724083002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/Animation.cpp:751: playing();
Put the new check after this check, this way m_content and its type is
guaranteed to be what you're expecting.

[wkorman, 2017-03-20 22:32:33]

Still looking at unit tests but wanted to reply to alan's comments.

https://codereview.chromium.org/2724083002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2
(left):

https://codereview.chromium.org/2724083002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2:1344:
crbug.com/674317 virtual/threaded/transitions/opacity-transition-zindex.html [
Timeout ]
alancutter@ -- are these tests (this and the one below marked as removed)
actually aiming to test composited behavior? I want to make sure they aren't
just passing now due to being de-composited even when compositing feature is
enabled if their purpose is explicitly to make sure they do composite when
feature is enabled. If that makes sense.

https://codereview.chromium.org/2724083002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/Animation.cpp (right):

https://codereview.chromium.org/2724083002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/Animation.cpp:739: // which case we
trust the compositing logic will create a layer if needed.
On 2017/03/20 06:21:09, alancutter wrote:
> We should assert the relevant RuntimeEnabledFeature status if it determines
> has_value().

Done.

https://codereview.chromium.org/2724083002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/Animation.cpp:744:
createCompositorElementId(DOMNodeIds::idForNode(effect->target()),
On 2017/03/20 06:21:09, alancutter wrote:
> Need null check for target.

What cases might lead it to be null, and if it's null, would we ever want to
allow compositing? I'm assuming not.

Most callsites to target() don't do any null checks:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/animation...

though I do see some null checks internally on KeyframeEffectReadOnly for
example:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/animation...

https://codereview.chromium.org/2724083002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/Animation.cpp:751: playing();
On 2017/03/20 06:21:09, alancutter wrote:
> Put the new check after this check, this way m_content and its type is
> guaranteed to be what you're expecting.

Done.

[wkorman, 2017-03-20 22:37:00]

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

[commit-bot: I haz the power, 2017-03-20 22:37:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

https://codereview.chromium.org/2724083002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2
(left):

https://codereview.chromium.org/2724083002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2:1344:
crbug.com/674317 virtual/threaded/transitions/opacity-transition-zindex.html [
Timeout ]
On 2017/03/20 at 22:32:33, wkorman wrote:
> alancutter@ -- are these tests (this and the one below marked as removed)
actually aiming to test composited behavior? I want to make sure they aren't
just passing now due to being de-composited even when compositing feature is
enabled if their purpose is explicitly to make sure they do composite when
feature is enabled. If that makes sense.

Thanks for the prompt. Looks like this test does depend on running on the
compositor for the purpose of synchronising with CC. I suspect changing
waitForCompositor() to box.animate({transform: ['scale(0.5)', 'scale(0.5)']})
should get it composited again though the expectations will need rebaselining.

The other timeout expectation removals look fine to me.

https://codereview.chromium.org/2724083002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/Animation.cpp (right):

https://codereview.chromium.org/2724083002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/Animation.cpp:744:
createCompositorElementId(DOMNodeIds::idForNode(effect->target()),
On 2017/03/20 at 22:32:33, wkorman wrote:
> On 2017/03/20 06:21:09, alancutter wrote:
> > Need null check for target.
> 
> What cases might lead it to be null, and if it's null, would we ever want to
allow compositing? I'm assuming not.
> 
> Most callsites to target() don't do any null checks:
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/animation...
> 
> though I do see some null checks internally on KeyframeEffectReadOnly for
example:
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/animation...

Both the target element of an effect and the effect of an animation are nullable
in the Web Animations API. Both must be non-null for compositing (or any visible
effect at all) to occur.
Thanks for pointing out the iffy call sites, that's something animations team
should fix.

[commit-bot: I haz the power, 2017-03-21 02:56:58]

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

[commit-bot: I haz the power, 2017-03-21 02:56:59]

Dry run: Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)

[wkorman, 2017-03-23 21:44:10]

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

[commit-bot: I haz the power, 2017-03-23 21:44:41]

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-23 23:24:25]

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

[commit-bot: I haz the power, 2017-03-23 23:24:26]

Dry run: This issue passed the CQ dry run.

[wkorman, 2017-03-24 19:24:55]

pdr@ -- PTAL, added unit tests to AnimationTest and PaintArtifactCompositorTest.

https://codereview.chromium.org/2724083002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2
(left):

https://codereview.chromium.org/2724083002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2:1344:
crbug.com/674317 virtual/threaded/transitions/opacity-transition-zindex.html [
Timeout ]
On 2017/03/21 00:32:12, alancutter wrote:
> On 2017/03/20 at 22:32:33, wkorman wrote:
> > alancutter@ -- are these tests (this and the one below marked as removed)
> actually aiming to test composited behavior? I want to make sure they aren't
> just passing now due to being de-composited even when compositing feature is
> enabled if their purpose is explicitly to make sure they do composite when
> feature is enabled. If that makes sense.
> 
> Thanks for the prompt. Looks like this test does depend on running on the
> compositor for the purpose of synchronising with CC. I suspect changing
> waitForCompositor() to box.animate({transform: ['scale(0.5)', 'scale(0.5)']})
> should get it composited again though the expectations will need rebaselining.
> 
> The other timeout expectation removals look fine to me.

I double checked this and it is creating a compositor animation as-is in SPv2,
so we're good. I'm not sure why I thought it wasn't.

[pdr., 2017-03-24 20:37:47]

LGTM

https://codereview.chromium.org/2724083002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/animations/resources/composited-animation-test.js
(right):

https://codereview.chromium.org/2724083002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/animations/resources/composited-animation-test.js:113:
// console.log('Creating test instance [duration=' + duration + ', delay=' +
(-duration * sample.at) + ', easing=' + test.data.easing + '].');
nit: did you mean to leave this in?

[wkorman, 2017-03-24 20:59:26]

https://codereview.chromium.org/2724083002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/animations/resources/composited-animation-test.js
(right):

https://codereview.chromium.org/2724083002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/animations/resources/composited-animation-test.js:113:
// console.log('Creating test instance [duration=' + duration + ', delay=' +
(-duration * sample.at) + ', easing=' + test.data.easing + '].');
On 2017/03/24 20:37:47, pdr. wrote:
> nit: did you mean to leave this in?

Ah, good catch, removed.

[wkorman, 2017-03-24 21:01:12]

The CQ bit was checked by wkorman@chromium.org

[wkorman, 2017-03-24 21:01:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
alancutter@chromium.org, pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/2724083002/#ps160001
(title: "Remove logging.")

[commit-bot: I haz the power, 2017-03-24 21:02:50]

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-24 22:24:00]

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

[commit-bot: I haz the power, 2017-03-24 22:24:01]

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

[wkorman, 2017-03-24 23:34:12]

The CQ bit was checked by wkorman@chromium.org

[commit-bot: I haz the power, 2017-03-24 23:34:59]

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-25 00:39:22]

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

[commit-bot: I haz the power, 2017-03-25 00:39:24]

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

[wkorman, 2017-04-07 01:12:00]

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

[commit-bot: I haz the power, 2017-04-07 01:12:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[wkorman, 2017-04-07 01:26:47]

The CQ bit was checked by wkorman@chromium.org

[wkorman, 2017-04-07 01:26:48]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
alancutter@chromium.org
Link to the patchset: https://codereview.chromium.org/2724083002/#ps200001
(title: "Update expectations.")

[commit-bot: I haz the power, 2017-04-07 01:27:25]

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-07 02:34:52]

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

[commit-bot: I haz the power, 2017-04-07 02:34:53]

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

[wkorman, 2017-04-07 03:24:25]

The CQ bit was checked by wkorman@chromium.org

[commit-bot: I haz the power, 2017-04-07 03:25:04]

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-07 04:43:50]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1491535465113880,
"parent_rev": "1dedf46ece745194dd618cbedc84f402acf53d6e", "commit_rev":
"efeb13b3ba0f7604fe7f61a69d83870d1a5f51fa"}

[commit-bot: I haz the power, 2017-04-07 04:44:44]

Description was changed from

==========
[SPv2] Decomposite otherwise-compositable animations that paint nothing.

In SPv1 we create cc layers for composited animations even if they
paint no content.

In SPv2, we only create cc layers when something is painted,
a.k.a., there is a PaintChunk.

This means that in SPv1 we could be assured that a composited
animation that painted no content would still have a cc layer
with the animation's compositor element id set on it, but in
SPv2 such an animation will have no cc layer and accordingly
it is impossible to run it as a composited animation.

In SPv2, we choose to not composite such animations.

To do otherwise would require:

1. code complexity to identify when this is the case,
through a dummy PaintChunk or similar, and ensure that
we create a cc layer even though there's nothing in need
of painting.

2. compositor resource overhead to maintain a layer
solely for the purpose of executing a composited animation
even though it is not producing any visible content.

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

to

==========
[SPv2] Decomposite otherwise-compositable animations that paint nothing.

In SPv1 we create cc layers for composited animations even if they
paint no content.

In SPv2, we only create cc layers when something is painted,
a.k.a., there is a PaintChunk.

This means that in SPv1 we could be assured that a composited
animation that painted no content would still have a cc layer
with the animation's compositor element id set on it, but in
SPv2 such an animation will have no cc layer and accordingly
it is impossible to run it as a composited animation.

In SPv2, we choose to not composite such animations.

To do otherwise would require:

1. code complexity to identify when this is the case,
through a dummy PaintChunk or similar, and ensure that
we create a cc layer even though there's nothing in need
of painting.

2. compositor resource overhead to maintain a layer
solely for the purpose of executing a composited animation
even though it is not producing any visible content.

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

Review-Url: https://codereview.chromium.org/2724083002
Cr-Commit-Position: refs/heads/master@{#462748}
Committed:
https://chromium.googlesource.com/chromium/src/+/efeb13b3ba0f7604fe7f61a69d83...
==========

[commit-bot: I haz the power, 2017-04-07 04:44:46]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/efeb13b3ba0f7604fe7f61a69d83...