Web Animations: Allow animations with delay to start on compositor

Web Animations: Allow animations with delay to start on compositor

This patch ensures layers are created for any current animation (an
animation that has yet to play, or is playing) which should also
composite.

Previously layers were only created for animations once they had applied
an effect to the style (opacity, transform, ...), but this is too late
to allow a compositor animation to start when the animation specifies a
delay.

This change appeared to allow several hacks to be cleaned up as it's no
longer necessary to massage the interpolation output for opacity and
transform in order to ensure layer creation. However this is actually
required for transforms[1]. Tests and comments updated to clarify.

[1] http://www.w3.org/TR/css-transforms-1/#none-transform-animation
BUG=321926, 344809
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169448

[Julien - ping for review, 2014-03-10 18:11:59]

https://codereview.chromium.org/189883003/diff/1/LayoutTests/compositing/laye...
File LayoutTests/compositing/layer-creation/animation-overlap-with-children.html
(right):

https://codereview.chromium.org/189883003/diff/1/LayoutTests/compositing/laye...
LayoutTests/compositing/layer-creation/animation-overlap-with-children.html:51:
from { -webkit-transform: translateX(0px); }
Could you explain what the rationale for the test changes are?

https://codereview.chromium.org/189883003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBox.h (right):

https://codereview.chromium.org/189883003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.h:75: if (isRoot() || isPositioned() ||
createsGroup() || hasClipPath() || hasTransform() || hasHiddenBackface() ||
hasReflection() || style()->specifiesColumns() || !style()->hasAutoZIndex() ||
style()->hasWillChangeCompositingHint() ||
shouldCompositeForActiveAnimations(*this))
Note that other renderers override this (e.g. RenderTableRow, RenderInline,
RenderBoxModelObject, ...) and they would need to be updated too.

[dstockwell, 2014-03-11 03:21:30]

https://codereview.chromium.org/189883003/diff/1/LayoutTests/compositing/laye...
File LayoutTests/compositing/layer-creation/animation-overlap-with-children.html
(right):

https://codereview.chromium.org/189883003/diff/1/LayoutTests/compositing/laye...
LayoutTests/compositing/layer-creation/animation-overlap-with-children.html:51:
from { -webkit-transform: translateX(0px); }
On 2014/03/10 18:11:59, Julien Chaffraix - PST wrote:
> Could you explain what the rationale for the test changes are?

Previously an interpolation from none -> translate would produce the identity
transform at time 0, now we produce 'none'. Added to description.

https://codereview.chromium.org/189883003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBox.h (right):

https://codereview.chromium.org/189883003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.h:75: if (isRoot() || isPositioned() ||
createsGroup() || hasClipPath() || hasTransform() || hasHiddenBackface() ||
hasReflection() || style()->specifiesColumns() || !style()->hasAutoZIndex() ||
style()->hasWillChangeCompositingHint() ||
shouldCompositeForActiveAnimations(*this))
On 2014/03/10 18:11:59, Julien Chaffraix - PST wrote:
> Note that other renderers override this (e.g. RenderTableRow, RenderInline,
> RenderBoxModelObject, ...) and they would need to be updated too.

Added. Looked at all the overrides, seems to be just the ones you listed that
are necessary.

[dstockwell, 2014-03-13 23:41:15]

Ping.

[Julien - ping for review, 2014-03-14 17:42:00]

I can't really comment on the core/animation/ bits (they look OK to me but I am
not fluent enough in this code to say).

https://codereview.chromium.org/189883003/diff/40001/LayoutTests/compositing/...
File LayoutTests/compositing/layer-creation/animation-overlap-with-children.html
(right):

https://codereview.chromium.org/189883003/diff/40001/LayoutTests/compositing/...
LayoutTests/compositing/layer-creation/animation-overlap-with-children.html:51:
from { -webkit-transform: translateX(0px); }
It seemed like the test expected this animation to generate a RenderLayer but it
won't anymore so it looks like this is regressing the original test.

While I agree that the old logic was wonky, it seems like a regression as it
should this generate some flickering around time = 0 as we switch from being
hardware accelerated to not being.

[dstockwell, 2014-03-17 03:47:34]

On 2014/03/14 17:42:00, Julien Chaffraix - PST wrote:
> I can't really comment on the core/animation/ bits (they look OK to me but I
am
> not fluent enough in this code to say).

+shans
 
>
https://codereview.chromium.org/189883003/diff/40001/LayoutTests/compositing/...
> File
LayoutTests/compositing/layer-creation/animation-overlap-with-children.html
> (right):
> 
>
https://codereview.chromium.org/189883003/diff/40001/LayoutTests/compositing/...
>
LayoutTests/compositing/layer-creation/animation-overlap-with-children.html:51:
> from { -webkit-transform: translateX(0px); }
> It seemed like the test expected this animation to generate a RenderLayer but
it
> won't anymore so it looks like this is regressing the original test.

The difference wasn't actually related to layer creation, but was due to 'none'
not
producing a new stacking context. This prompted me to take another look at
how interpolation is specified for transforms. I've reverted the changes there,
updated a comment and tests to clarify that it is actually required that we
treat
'none' as the identity transform function.

[shans, 2014-03-17 04:03:46]

lgtm once these are resolved.

https://codereview.chromium.org/189883003/diff/50001/Source/core/animation/Ac...
File Source/core/animation/ActiveAnimations.cpp (right):

https://codereview.chromium.org/189883003/diff/50001/Source/core/animation/Ac...
Source/core/animation/ActiveAnimations.cpp:78: bool
ActiveAnimations::hasActiveAnimations(CSSPropertyID property) const
This name is no longer correct.

https://codereview.chromium.org/189883003/diff/50001/Source/core/animation/Ac...
Source/core/animation/ActiveAnimations.cpp:89: return false;
Can m_defaultStack reflect animations that are pending?

[dstockwell, 2014-03-17 04:16:34]

https://codereview.chromium.org/189883003/diff/50001/Source/core/animation/Ac...
File Source/core/animation/ActiveAnimations.cpp (right):

https://codereview.chromium.org/189883003/diff/50001/Source/core/animation/Ac...
Source/core/animation/ActiveAnimations.cpp:78: bool
ActiveAnimations::hasActiveAnimations(CSSPropertyID property) const
On 2014/03/17 04:03:47, shans wrote:
> This name is no longer correct.

FIXME, I think we'll want to consider renaming most of this class to be about
'current' rather than 'active' animations.

https://codereview.chromium.org/189883003/diff/50001/Source/core/animation/Ac...
Source/core/animation/ActiveAnimations.cpp:89: return false;
On 2014/03/17 04:03:47, shans wrote:
> Can m_defaultStack reflect animations that are pending?

Possibly, or perhaps we should create a separate collection of current (but not
active) animations for a given element. We'll need to do something here when I
add groups.

[Julien - ping for review, 2014-03-17 22:26:16]

lgtm2

https://codereview.chromium.org/189883003/diff/70001/Source/core/css/resolver...
File Source/core/css/resolver/AnimatedStyleBuilder.cpp (right):

https://codereview.chromium.org/189883003/diff/70001/Source/core/css/resolver...
Source/core/css/resolver/AnimatedStyleBuilder.cpp:583: // FIXME: This
normalization (handling of 'none' should be performed at input).
Is the ending parenthesis badly placed?

Also not sure what "performed at input" means: are we talking parsing time?

[dstockwell, 2014-03-18 00:13:55]

https://codereview.chromium.org/189883003/diff/70001/Source/core/css/resolver...
File Source/core/css/resolver/AnimatedStyleBuilder.cpp (right):

https://codereview.chromium.org/189883003/diff/70001/Source/core/css/resolver...
Source/core/css/resolver/AnimatedStyleBuilder.cpp:583: // FIXME: This
normalization (handling of 'none' should be performed at input).
On 2014/03/17 22:26:16, Julien Chaffraix - PST wrote:
> Is the ending parenthesis badly placed?
> 
> Also not sure what "performed at input" means: are we talking parsing time?

I'll rephrase this and fix the parenthesis. By 'input' I meant input to the
animation system, this happens in AnimatableValueFactory currently.

[dstockwell, 2014-03-18 00:42:01]

The CQ bit was checked by dstockwell@chromium.org

[commit-bot: I haz the power, 2014-03-18 00:42:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dstockwell@chromium.org/189883003/90001

[commit-bot: I haz the power, 2014-03-18 00:42:16]

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

[commit-bot: I haz the power, 2014-03-18 00:42:17]

Failed to apply patch for Source/core/animation/Player.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: Source/core/animation/Player.h
  |diff --git a/Source/core/animation/Player.h b/Source/core/animation/Player.h
  |index
737ebda7fcd5b8884ba6ac4c3ca30a1207e9a339..ccebee1d4a5890666a1c8737ba4a3ef1c2521cba
100644
  |--- a/Source/core/animation/Player.h
  |+++ b/Source/core/animation/Player.h
  --------------------------
  No file to patch.  Skipping patch.
  1 out of 1 hunk ignored

Patch:       Source/core/animation/Player.h
Index: Source/core/animation/Player.h
diff --git a/Source/core/animation/Player.h b/Source/core/animation/Player.h
index
737ebda7fcd5b8884ba6ac4c3ca30a1207e9a339..ccebee1d4a5890666a1c8737ba4a3ef1c2521cba
100644
--- a/Source/core/animation/Player.h
+++ b/Source/core/animation/Player.h
@@ -77,6 +77,7 @@ public:
     double startTime() const { return m_startTime; }
     void setStartTime(double);
 
+    const TimedItem* source() const { return m_content.get(); }
     TimedItem* source() { return m_content.get(); }
     TimedItem* source(bool& isNull) { isNull = !m_content; return
m_content.get(); }
     void setSource(TimedItem*);

[dstockwell, 2014-03-18 00:50:27]

The CQ bit was checked by dstockwell@chromium.org

[commit-bot: I haz the power, 2014-03-18 00:50:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dstockwell@chromium.org/189883003/110001

[commit-bot: I haz the power, 2014-03-18 00:53:29]

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

[commit-bot: I haz the power, 2014-03-18 00:53:30]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[dstockwell, 2014-03-18 01:04:14]

The CQ bit was checked by dstockwell@chromium.org

[commit-bot: I haz the power, 2014-03-18 01:04:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dstockwell@chromium.org/189883003/110001

[commit-bot: I haz the power, 2014-03-18 01:07:53]

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

[commit-bot: I haz the power, 2014-03-18 01:07:54]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[dstockwell, 2014-03-18 01:32:54]

The CQ bit was checked by dstockwell@chromium.org

[commit-bot: I haz the power, 2014-03-18 01:32:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dstockwell@chromium.org/189883003/130001

[commit-bot: I haz the power, 2014-03-18 01:36:28]

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

[commit-bot: I haz the power, 2014-03-18 01:36:29]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_rel

[dstockwell, 2014-03-18 01:51:45]

The CQ bit was checked by dstockwell@chromium.org

[commit-bot: I haz the power, 2014-03-18 03:17:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dstockwell@chromium.org/189883003/130001

[commit-bot: I haz the power, 2014-03-18 04:25:36]

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

[commit-bot: I haz the power, 2014-03-18 04:25:37]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[dstockwell, 2014-03-18 08:11:11]

The CQ bit was checked by dstockwell@chromium.org

[commit-bot: I haz the power, 2014-03-18 08:11:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dstockwell@chromium.org/189883003/140001

[commit-bot: I haz the power, 2014-03-18 09:22:56]

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

[commit-bot: I haz the power, 2014-03-18 09:22:57]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[dstockwell, 2014-03-18 10:44:25]

The CQ bit was checked by dstockwell@chromium.org

[commit-bot: I haz the power, 2014-03-18 10:44:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dstockwell@chromium.org/189883003/140001

[commit-bot: I haz the power, 2014-03-18 14:04:35]

Change committed as 169448