Compositor animations for Independent CSS Transform Properties

Compositor animations for Independent CSS Transform Properties

Design Doc :

https://docs.google.com/document/d/1Rbwj36VVhGuI7C3PQGuoEqJ2U9nAfzKXjwBV2N41JlQ/edit?usp=sharing

Link to Intent to implement:

https://groups.google.com/a/chromium.org/d/msg/blink-dev/-GfE73tlLX8/hv7Qitys6j8J

BUG=496550
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=198362

[soonm, 2015-06-30 02:42:04]

soonm@google.com changed reviewers:
+ alancutter@chromium.org, dstockwell@chromium.org, ericwilligers@chromium.org,
shans@chromium.org, timloh@chromium.org

[Eric Willigers, 2015-06-30 03:18:23]

https://codereview.chromium.org/1218943002/diff/1/Source/core/animation/Compo...
File Source/core/animation/CompositorAnimations.cpp (right):

https://codereview.chromium.org/1218943002/diff/1/Source/core/animation/Compo...
Source/core/animation/CompositorAnimations.cpp:100: ||
effectToAdd.affects(PropertyHandle(CSSPropertyScale))
indent four spaces. Also in cancelIncompatibleAnimationsOnCompositor and
isCandidateForAnimationOnCompositor.

https://codereview.chromium.org/1218943002/diff/1/Source/core/animation/Docum...
File Source/core/animation/DocumentAnimations.cpp (right):

https://codereview.chromium.org/1218943002/diff/1/Source/core/animation/Docum...
Source/core/animation/DocumentAnimations.cpp:74: || (property ==
CSSPropertyRotate && style->isRunningTransformAnimationOnCompositor())
Perhaps introduce a bool representing property == CSSPropertyRotate || property
== ... .

Then style->isRunningTransformAnimationOnCompositor() only needs to be checked
once:

if ((...  && style->isRunningTransformAnimationOnCompositor())
|| (property == CSSPropertyOpacity &&
style->isRunningOpacityAnimationOnCompositor())
|| (property == CSSPropertyWebkitFilter &&
    style->isRunningFilterAnimationOnCompositor())) {

[Eric Willigers, 2015-06-30 03:19:36]

ericwilligers@chromium.org changed reviewers:
+ loyso@chromium.org

[soonm, 2015-06-30 05:01:09]

https://codereview.chromium.org/1218943002/diff/1/Source/core/animation/Compo...
File Source/core/animation/CompositorAnimations.cpp (right):

https://codereview.chromium.org/1218943002/diff/1/Source/core/animation/Compo...
Source/core/animation/CompositorAnimations.cpp:100: ||
effectToAdd.affects(PropertyHandle(CSSPropertyScale))
On 2015/06/30 at 03:18:23, Eric Willigers wrote:
> indent four spaces. Also in cancelIncompatibleAnimationsOnCompositor and
isCandidateForAnimationOnCompositor.

Done.

https://codereview.chromium.org/1218943002/diff/1/Source/core/animation/Docum...
File Source/core/animation/DocumentAnimations.cpp (right):

https://codereview.chromium.org/1218943002/diff/1/Source/core/animation/Docum...
Source/core/animation/DocumentAnimations.cpp:74: || (property ==
CSSPropertyRotate && style->isRunningTransformAnimationOnCompositor())
On 2015/06/30 at 03:18:23, Eric Willigers wrote:
> Perhaps introduce a bool representing property == CSSPropertyRotate ||
property == ... .
> 
> Then style->isRunningTransformAnimationOnCompositor() only needs to be checked
once:
> 
> if ((...  && style->isRunningTransformAnimationOnCompositor())
> || (property == CSSPropertyOpacity &&
style->isRunningOpacityAnimationOnCompositor())
> || (property == CSSPropertyWebkitFilter &&
>     style->isRunningFilterAnimationOnCompositor())) {

Done.

[alancutter (OOO until 2018), 2015-07-01 03:35:47]

https://codereview.chromium.org/1218943002/diff/60001/LayoutTests/virtual/thr...
File
LayoutTests/virtual/threaded/animations/compositor-transform-with-rotate.html
(right):

https://codereview.chromium.org/1218943002/diff/60001/LayoutTests/virtual/thr...
LayoutTests/virtual/threaded/animations/compositor-transform-with-rotate.html:6:
position: absolute;
If you don't use position: absolute can you remove the top & left property
settings?

https://codereview.chromium.org/1218943002/diff/60001/LayoutTests/virtual/thr...
LayoutTests/virtual/threaded/animations/compositor-transform-with-rotate.html:12:
#human-text{
Use a div instead of span and remove this rule. Use CSS classes for the test
boxes.

https://codereview.chromium.org/1218943002/diff/60001/LayoutTests/virtual/thr...
LayoutTests/virtual/threaded/animations/compositor-transform-with-rotate.html:16:
#divBlue {
Use test descriptions like transform property names instead of colours and add
it as text in the boxes.

https://codereview.chromium.org/1218943002/diff/60001/LayoutTests/virtual/thr...
LayoutTests/virtual/threaded/animations/compositor-transform-with-rotate.html:37:
<p id="text"></p>
Inconsistent indentation.

https://codereview.chromium.org/1218943002/diff/60001/LayoutTests/virtual/thr...
LayoutTests/virtual/threaded/animations/compositor-transform-with-rotate.html:68:
window.testRunner.waitUntilDone();
No need to call window.testRunner methods if you're using testharness.js.

https://codereview.chromium.org/1218943002/diff/60001/LayoutTests/virtual/thr...
LayoutTests/virtual/threaded/animations/compositor-transform-with-rotate.html:72:
test(function() {
Use async_test().

https://codereview.chromium.org/1218943002/diff/60001/Source/core/animation/C...
File Source/core/animation/CompositorAnimations.cpp (right):

https://codereview.chromium.org/1218943002/diff/60001/Source/core/animation/C...
Source/core/animation/CompositorAnimations.cpp:260: return false;
Add TODO for supporting multiple transform property animations on the
compositor.

https://codereview.chromium.org/1218943002/diff/60001/Source/core/animation/C...
File Source/core/animation/CompositorAnimations.h (right):

https://codereview.chromium.org/1218943002/diff/60001/Source/core/animation/C...
Source/core/animation/CompositorAnimations.h:50: static bool
isCompositableProperty(CSSPropertyID property) { return property ==
CSSPropertyRotate || property == CSSPropertyOpacity || property ==
CSSPropertyScale || property == CSSPropertyTransform || property ==
CSSPropertyTranslate || property == CSSPropertyWebkitFilter; }
We should just loop over CompositableProperties at this point.

https://codereview.chromium.org/1218943002/diff/60001/Source/core/animation/C...
Source/core/animation/CompositorAnimations.h:51: static CSSPropertyID
CompositableProperties[6];
Clean up nit, I think this should be named compositableProperties.

[soonm, 2015-07-01 07:18:51]

https://codereview.chromium.org/1218943002/diff/60001/LayoutTests/virtual/thr...
File
LayoutTests/virtual/threaded/animations/compositor-transform-with-rotate.html
(right):

https://codereview.chromium.org/1218943002/diff/60001/LayoutTests/virtual/thr...
LayoutTests/virtual/threaded/animations/compositor-transform-with-rotate.html:6:
position: absolute;
On 2015/07/01 at 03:35:47, alancutter wrote:
> If you don't use position: absolute can you remove the top & left property
settings?

Done

https://codereview.chromium.org/1218943002/diff/60001/LayoutTests/virtual/thr...
LayoutTests/virtual/threaded/animations/compositor-transform-with-rotate.html:12:
#human-text{
On 2015/07/01 at 03:35:47, alancutter wrote:
> Use a div instead of span and remove this rule. Use CSS classes for the test
boxes.

Done

https://codereview.chromium.org/1218943002/diff/60001/LayoutTests/virtual/thr...
LayoutTests/virtual/threaded/animations/compositor-transform-with-rotate.html:16:
#divBlue {
On 2015/07/01 at 03:35:46, alancutter wrote:
> Use test descriptions like transform property names instead of colours and add
it as text in the boxes.

Done

https://codereview.chromium.org/1218943002/diff/60001/LayoutTests/virtual/thr...
LayoutTests/virtual/threaded/animations/compositor-transform-with-rotate.html:37:
<p id="text"></p>
On 2015/07/01 at 03:35:46, alancutter wrote:
> Inconsistent indentation.

Done - Fixed indentation.

https://codereview.chromium.org/1218943002/diff/60001/LayoutTests/virtual/thr...
LayoutTests/virtual/threaded/animations/compositor-transform-with-rotate.html:68:
window.testRunner.waitUntilDone();
On 2015/07/01 at 03:35:47, alancutter wrote:
> No need to call window.testRunner methods if you're using testharness.js.

Done - Removed.

https://codereview.chromium.org/1218943002/diff/60001/LayoutTests/virtual/thr...
LayoutTests/virtual/threaded/animations/compositor-transform-with-rotate.html:72:
test(function() {
On 2015/07/01 at 03:35:46, alancutter wrote:
> Use async_test().

Done.

https://codereview.chromium.org/1218943002/diff/60001/Source/core/animation/C...
File Source/core/animation/CompositorAnimations.cpp (right):

https://codereview.chromium.org/1218943002/diff/60001/Source/core/animation/C...
Source/core/animation/CompositorAnimations.cpp:260: return false;
On 2015/07/01 at 03:35:47, alancutter wrote:
> Add TODO for supporting multiple transform property animations on the
compositor.

Done

https://codereview.chromium.org/1218943002/diff/60001/Source/core/animation/C...
File Source/core/animation/CompositorAnimations.h (right):

https://codereview.chromium.org/1218943002/diff/60001/Source/core/animation/C...
Source/core/animation/CompositorAnimations.h:50: static bool
isCompositableProperty(CSSPropertyID property) { return property ==
CSSPropertyRotate || property == CSSPropertyOpacity || property ==
CSSPropertyScale || property == CSSPropertyTransform || property ==
CSSPropertyTranslate || property == CSSPropertyWebkitFilter; }
On 2015/07/01 at 03:35:47, alancutter wrote:
> We should just loop over CompositableProperties at this point.

Done - Loopity loop

https://codereview.chromium.org/1218943002/diff/60001/Source/core/animation/C...
Source/core/animation/CompositorAnimations.h:51: static CSSPropertyID
CompositableProperties[6];
On 2015/07/01 at 03:35:47, alancutter wrote:
> Clean up nit, I think this should be named compositableProperties.

Changed at a call site at KeyframeEffectModel as well

[Eric Willigers, 2015-07-02 01:25:53]

Optionally in a subsequent CL, it would be great to have tests where an
animation starts on the compositor, and then moves off it when another transform
property is defined or animated.

https://codereview.chromium.org/1218943002/diff/80001/Source/core/animation/C...
File Source/core/animation/CompositorAnimations.h (right):

https://codereview.chromium.org/1218943002/diff/80001/Source/core/animation/C...
Source/core/animation/CompositorAnimations.h:51: static CSSPropertyID
compositableProperties[6];
static const CSSPropertyID compositableProperties[6];

(also change CompositorAnimations.cpp)

[alancutter (OOO until 2018), 2015-07-02 01:34:15]

You should add a test that uses multiple individual transform properties.
I recommend refactoring your tests into a single file that reads like:
assertIsComposited(transform);
assertIsComposited(translate);
assertIsNotComposited(transform, translate);
assertIsNotComposited(rotate, translate);

https://codereview.chromium.org/1218943002/diff/80001/LayoutTests/virtual/thr...
File
LayoutTests/virtual/threaded/animations/compositor-transform-with-rotate.html
(right):

https://codereview.chromium.org/1218943002/diff/80001/LayoutTests/virtual/thr...
LayoutTests/virtual/threaded/animations/compositor-transform-with-rotate.html:21:
}
Giving these boxes different colours seems unnecessary (you could give them a
border instead) but it's not a big deal.

https://codereview.chromium.org/1218943002/diff/80001/LayoutTests/virtual/thr...
LayoutTests/virtual/threaded/animations/compositor-transform-with-rotate.html:63:
assert_true(internals.isCompositedAnimation(transformOnlyAnimation), 'Only
Transform should be composited');
"Only X" sounds misleading, it should be "X only"

[soonm, 2015-07-02 03:28:56]

https://codereview.chromium.org/1218943002/diff/80001/LayoutTests/virtual/thr...
File
LayoutTests/virtual/threaded/animations/compositor-transform-with-rotate.html
(right):

https://codereview.chromium.org/1218943002/diff/80001/LayoutTests/virtual/thr...
LayoutTests/virtual/threaded/animations/compositor-transform-with-rotate.html:21:
}
On 2015/07/02 at 01:34:15, alancutter wrote:
> Giving these boxes different colours seems unnecessary (you could give them a
border instead) but it's not a big deal.

Change to using borders but kept the colors

https://codereview.chromium.org/1218943002/diff/80001/LayoutTests/virtual/thr...
LayoutTests/virtual/threaded/animations/compositor-transform-with-rotate.html:63:
assert_true(internals.isCompositedAnimation(transformOnlyAnimation), 'Only
Transform should be composited');
On 2015/07/02 at 01:34:15, alancutter wrote:
> "Only X" sounds misleading, it should be "X only"

Done

[soonm, 2015-07-02 03:37:04]

On 2015/07/02 at 01:25:53, ericwilligers wrote:
> Optionally in a subsequent CL, it would be great to have tests where an
animation starts on the compositor, and then moves off it when another transform
property is defined or animated.
> 
>
https://codereview.chromium.org/1218943002/diff/80001/Source/core/animation/C...
> File Source/core/animation/CompositorAnimations.h (right):
> 
>
https://codereview.chromium.org/1218943002/diff/80001/Source/core/animation/C...
> Source/core/animation/CompositorAnimations.h:51: static CSSPropertyID
compositableProperties[6];
> static const CSSPropertyID compositableProperties[6];
> 
> (also change CompositorAnimations.cpp)

Done.

[Eric Willigers, 2015-07-02 03:44:58]

lgtm

[alancutter (OOO until 2018), 2015-07-02 04:44:47]

lgtm with nits.

https://codereview.chromium.org/1218943002/diff/120001/LayoutTests/virtual/th...
File
LayoutTests/virtual/threaded/animations/compositor-independent-transform-properties.html
(right):

https://codereview.chromium.org/1218943002/diff/120001/LayoutTests/virtual/th...
LayoutTests/virtual/threaded/animations/compositor-independent-transform-properties.html:13:
var obj = {};
s/obj/keyframe/

https://codereview.chromium.org/1218943002/diff/120001/LayoutTests/virtual/th...
LayoutTests/virtual/threaded/animations/compositor-independent-transform-properties.html:16:
obj[property] = 'initial';
Nice.

https://codereview.chromium.org/1218943002/diff/120001/LayoutTests/virtual/th...
LayoutTests/virtual/threaded/animations/compositor-independent-transform-properties.html:28:
assert_equals(internals.isCompositedAnimation(animation), isComposited,
properties + (isComposited ? " should " : " should not ") + "be composited");
You need to wrap this in asyncHandle.step(function(){ ... });

[soonm, 2015-07-02 04:50:40]

The CQ bit was checked by soonm@google.com

[soonm, 2015-07-02 04:50:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
ericwilligers@chromium.org, alancutter@chromium.org
Link to the patchset: https://codereview.chromium.org/1218943002/#ps140001
(title: "Clean up test")

[commit-bot: I haz the power, 2015-07-02 04:50:59]

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

[commit-bot: I haz the power, 2015-07-02 04:56:48]

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

[commit-bot: I haz the power, 2015-07-02 04:56:50]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/3...)

[soonm, 2015-07-03 01:36:38]

On 2015/07/02 at 04:44:47, alancutter wrote:
> lgtm with nits.
> 
>
https://codereview.chromium.org/1218943002/diff/120001/LayoutTests/virtual/th...
> File
LayoutTests/virtual/threaded/animations/compositor-independent-transform-properties.html
(right):
> 
>
https://codereview.chromium.org/1218943002/diff/120001/LayoutTests/virtual/th...
>
LayoutTests/virtual/threaded/animations/compositor-independent-transform-properties.html:13:
var obj = {};
> s/obj/keyframe/
> 
>
https://codereview.chromium.org/1218943002/diff/120001/LayoutTests/virtual/th...
>
LayoutTests/virtual/threaded/animations/compositor-independent-transform-properties.html:16:
obj[property] = 'initial';
> Nice.
> 
>
https://codereview.chromium.org/1218943002/diff/120001/LayoutTests/virtual/th...
>
LayoutTests/virtual/threaded/animations/compositor-independent-transform-properties.html:28:
assert_equals(internals.isCompositedAnimation(animation), isComposited,
properties + (isComposited ? " should " : " should not ") + "be composited");
> You need to wrap this in asyncHandle.step(function(){ ... });

Done.

[Timothy Loh, 2015-07-03 01:52:29]

lgtm

[dstockwell, 2015-07-03 01:53:24]

taking a look

https://codereview.chromium.org/1218943002/diff/140001/Source/core/animation/...
File Source/core/animation/CompositorAnimations.cpp (right):

https://codereview.chromium.org/1218943002/diff/140001/Source/core/animation/...
Source/core/animation/CompositorAnimations.cpp:101: ||
effectToAdd.affects(PropertyHandle(CSSPropertyTransform))
transform should be first (as most common)

https://codereview.chromium.org/1218943002/diff/140001/Source/core/animation/...
Source/core/animation/CompositorAnimations.cpp:119: || (affectsTransform &&
attachedAnimation->affects(targetElement, CSSPropertyTransform))
transform should come first

[dstockwell, 2015-07-03 01:58:41]

https://codereview.chromium.org/1218943002/diff/140001/LayoutTests/virtual/th...
File
LayoutTests/virtual/threaded/animations/compositor-independent-transform-properties.html
(right):

https://codereview.chromium.org/1218943002/diff/140001/LayoutTests/virtual/th...
LayoutTests/virtual/threaded/animations/compositor-independent-transform-properties.html:1:
<!DOCTYPE html>
for now we need manual tests too, to verify that this actually animates on the
compositor

https://codereview.chromium.org/1218943002/diff/140001/Source/core/animation/...
File Source/core/animation/CompositorAnimations.cpp (right):

https://codereview.chromium.org/1218943002/diff/140001/Source/core/animation/...
Source/core/animation/CompositorAnimations.cpp:226: unsigned
transformOperationsCount = 0;
transformPropertyCount?

https://codereview.chromium.org/1218943002/diff/140001/Source/core/animation/...
Source/core/animation/CompositorAnimations.cpp:287: ||
effectToAdd.affects(PropertyHandle(CSSPropertyTransform))
transform first

https://codereview.chromium.org/1218943002/diff/140001/Source/core/animation/...
Source/core/animation/CompositorAnimations.cpp:305: || (affectsTransform &&
attachedAnimation->affects(targetElement, CSSPropertyTransform))
transform first

https://codereview.chromium.org/1218943002/diff/140001/Source/core/animation/...
File Source/core/animation/ElementAnimations.cpp (right):

https://codereview.chromium.org/1218943002/diff/140001/Source/core/animation/...
Source/core/animation/ElementAnimations.cpp:65: ||
effect.affects(PropertyHandle(CSSPropertyTransform))
transform first

https://codereview.chromium.org/1218943002/diff/140001/Source/core/animation/...
File Source/core/animation/KeyframeEffect.cpp (right):

https://codereview.chromium.org/1218943002/diff/140001/Source/core/animation/...
Source/core/animation/KeyframeEffect.cpp:140: // rotate or scale operation has
been introduced on the element.
comment needs update

https://codereview.chromium.org/1218943002/diff/140001/Source/core/animation/...
Source/core/animation/KeyframeEffect.cpp:251: // Do not put transforms on
compositor if more than one of them are defined
I'm not sure the above is correct now (in applyEffects), does this work
correctly if we animate transform, then set the rotate property later?

[loyso (OOO), 2015-07-03 03:50:07]

https://codereview.chromium.org/1218943002/diff/140001/Source/core/animation/...
File Source/core/animation/CompositorAnimations.cpp (right):

https://codereview.chromium.org/1218943002/diff/140001/Source/core/animation/...
Source/core/animation/CompositorAnimations.cpp:231: if (property.cssProperty()
== CSSPropertyRotate
You have similar patterns repeated everywhere. Could we "extract common helper
method(s)" to avoid redundancy?

[soonm, 2015-07-03 06:34:30]

https://codereview.chromium.org/1218943002/diff/140001/Source/core/animation/...
File Source/core/animation/CompositorAnimations.cpp (right):

https://codereview.chromium.org/1218943002/diff/140001/Source/core/animation/...
Source/core/animation/CompositorAnimations.cpp:101: ||
effectToAdd.affects(PropertyHandle(CSSPropertyTransform))
On 2015/07/03 at 01:53:23, dstockwell wrote:
> transform should be first (as most common)

Done.

https://codereview.chromium.org/1218943002/diff/140001/Source/core/animation/...
Source/core/animation/CompositorAnimations.cpp:119: || (affectsTransform &&
attachedAnimation->affects(targetElement, CSSPropertyTransform))
On 2015/07/03 at 01:53:23, dstockwell wrote:
> transform should come first

Done - As well for the same code below.

https://codereview.chromium.org/1218943002/diff/140001/Source/core/animation/...
Source/core/animation/CompositorAnimations.cpp:226: unsigned
transformOperationsCount = 0;
On 2015/07/03 at 01:58:40, dstockwell wrote:
> transformPropertyCount?

Yep that sounds better.

https://codereview.chromium.org/1218943002/diff/140001/Source/core/animation/...
Source/core/animation/CompositorAnimations.cpp:231: if (property.cssProperty()
== CSSPropertyRotate
On 2015/07/03 at 03:50:07, loyso wrote:
> You have similar patterns repeated everywhere. Could we "extract common helper
method(s)" to avoid redundancy?

Did some clean up and abstracted out common checks.

https://codereview.chromium.org/1218943002/diff/140001/Source/core/animation/...
Source/core/animation/CompositorAnimations.cpp:287: ||
effectToAdd.affects(PropertyHandle(CSSPropertyTransform))
On 2015/07/03 at 01:58:40, dstockwell wrote:
> transform first

Done.

https://codereview.chromium.org/1218943002/diff/140001/Source/core/animation/...
Source/core/animation/CompositorAnimations.cpp:305: || (affectsTransform &&
attachedAnimation->affects(targetElement, CSSPropertyTransform))
On 2015/07/03 at 01:58:40, dstockwell wrote:
> transform first

Done.

https://codereview.chromium.org/1218943002/diff/140001/Source/core/animation/...
File Source/core/animation/ElementAnimations.cpp (right):

https://codereview.chromium.org/1218943002/diff/140001/Source/core/animation/...
Source/core/animation/ElementAnimations.cpp:65: ||
effect.affects(PropertyHandle(CSSPropertyTransform))
On 2015/07/03 at 01:58:40, dstockwell wrote:
> transform first

Done.

https://codereview.chromium.org/1218943002/diff/140001/Source/core/animation/...
File Source/core/animation/KeyframeEffect.cpp (right):

https://codereview.chromium.org/1218943002/diff/140001/Source/core/animation/...
Source/core/animation/KeyframeEffect.cpp:140: // rotate or scale operation has
been introduced on the element.
On 2015/07/03 at 01:58:41, dstockwell wrote:
> comment needs update

Done.

https://codereview.chromium.org/1218943002/diff/140001/Source/core/animation/...
Source/core/animation/KeyframeEffect.cpp:251: // Do not put transforms on
compositor if more than one of them are defined
On 2015/07/03 at 01:58:40, dstockwell wrote:
> I'm not sure the above is correct now (in applyEffects), does this work
correctly if we animate transform, then set the rotate property later?

Add tests to check for cancellations happening when new animation/style is added
with animating transform/translate/rotate/scale

[Eric Willigers, 2015-07-05 23:03:29]

https://codereview.chromium.org/1218943002/diff/160001/Source/core/animation/...
File Source/core/animation/css/CSSAnimatableValueFactory.cpp (left):

https://codereview.chromium.org/1218943002/diff/160001/Source/core/animation/...
Source/core/animation/css/CSSAnimatableValueFactory.cpp:533: case
CSSPropertyTranslate:
CSSPropertyTranslate: {
with closing brace before the next case.

Same for the next two cases.

[soonm, 2015-07-05 23:40:45]

https://codereview.chromium.org/1218943002/diff/160001/Source/core/animation/...
File Source/core/animation/css/CSSAnimatableValueFactory.cpp (left):

https://codereview.chromium.org/1218943002/diff/160001/Source/core/animation/...
Source/core/animation/css/CSSAnimatableValueFactory.cpp:533: case
CSSPropertyTranslate:
On 2015/07/05 at 23:03:29, Eric Willigers wrote:
> CSSPropertyTranslate: {
> with closing brace before the next case.
> 
> Same for the next two cases.

These changes should not have been in this patch. 

It resolves a separate issue and submitted to another cl - Thanks for picking
this bug up :)

Going to be fixed with rebase master.

[alancutter (OOO until 2018), 2015-07-06 04:36:15]

https://codereview.chromium.org/1218943002/diff/180001/LayoutTests/virtual/th...
File
LayoutTests/virtual/threaded/animations/composited-animation-independent-transform-cancel.html
(right):

https://codereview.chromium.org/1218943002/diff/180001/LayoutTests/virtual/th...
LayoutTests/virtual/threaded/animations/composited-animation-independent-transform-cancel.html:9:
cancelProperty is animated on the element */
This documentation should be next to the function it's documenting.

https://codereview.chromium.org/1218943002/diff/180001/LayoutTests/virtual/th...
LayoutTests/virtual/threaded/animations/composited-animation-independent-transform-cancel.html:10:
var varMap = {
keyframeValueMap?

[soonm, 2015-07-06 04:49:56]

https://codereview.chromium.org/1218943002/diff/180001/LayoutTests/virtual/th...
File
LayoutTests/virtual/threaded/animations/composited-animation-independent-transform-cancel.html
(right):

https://codereview.chromium.org/1218943002/diff/180001/LayoutTests/virtual/th...
LayoutTests/virtual/threaded/animations/composited-animation-independent-transform-cancel.html:9:
cancelProperty is animated on the element */
On 2015/07/06 at 04:36:14, alancutter wrote:
> This documentation should be next to the function it's documenting.

Done - Moved down to assertAnimationComposited

https://codereview.chromium.org/1218943002/diff/180001/LayoutTests/virtual/th...
LayoutTests/virtual/threaded/animations/composited-animation-independent-transform-cancel.html:10:
var varMap = {
On 2015/07/06 at 04:36:14, alancutter wrote:
> keyframeValueMap?

Changed - Yep that's a better name

[Eric Willigers, 2015-07-06 06:03:28]

https://codereview.chromium.org/1218943002/diff/200001/Source/core/animation/...
File Source/core/animation/ElementAnimations.cpp (right):

https://codereview.chromium.org/1218943002/diff/200001/Source/core/animation/...
Source/core/animation/ElementAnimations.cpp:63: if
(effect.affects(PropertyHandle(CSSPropertyTransform))
[Optional] Can we call isTransformRelatedEffect?  The method could perhaps move
from CompositorAnimations.cpp to EffectModel.

[soonm, 2015-07-06 23:36:50]

https://codereview.chromium.org/1218943002/diff/140001/LayoutTests/virtual/th...
File
LayoutTests/virtual/threaded/animations/compositor-independent-transform-properties.html
(right):

https://codereview.chromium.org/1218943002/diff/140001/LayoutTests/virtual/th...
LayoutTests/virtual/threaded/animations/compositor-independent-transform-properties.html:1:
<!DOCTYPE html>
On 2015/07/03 at 01:58:40, dstockwell wrote:
> for now we need manual tests too, to verify that this actually animates on the
compositor

Done - Added manual tests for the 3 transform properties

https://codereview.chromium.org/1218943002/diff/200001/Source/core/animation/...
File Source/core/animation/ElementAnimations.cpp (right):

https://codereview.chromium.org/1218943002/diff/200001/Source/core/animation/...
Source/core/animation/ElementAnimations.cpp:63: if
(effect.affects(PropertyHandle(CSSPropertyTransform))
On 2015/07/06 at 06:03:28, Eric Willigers wrote:
> [Optional] Can we call isTransformRelatedEffect?  The method could perhaps
move from CompositorAnimations.cpp to EffectModel.

Yep that seems sound since the function only deals with the EffectModel

https://codereview.chromium.org/1218943002/diff/200001/Source/core/animation/...
Source/core/animation/ElementAnimations.cpp:63: if
(effect.affects(PropertyHandle(CSSPropertyTransform))
On 2015/07/06 at 06:03:28, Eric Willigers wrote:
> [Optional] Can we call isTransformRelatedEffect?  The method could perhaps
move from CompositorAnimations.cpp to EffectModel.

Done - Yep that seems sound since the function only deals with the EffectModel

[soonm, 2015-07-07 04:56:27]

The CQ bit was checked by soonm@google.com

[soonm, 2015-07-07 04:56:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
alancutter@chromium.org, timloh@chromium.org, ericwilligers@chromium.org
Link to the patchset: https://codereview.chromium.org/1218943002/#ps220001
(title: "Add Manual Tests and fix bug with cancel tests")

[commit-bot: I haz the power, 2015-07-07 04:56:38]

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

[commit-bot: I haz the power, 2015-07-07 05:00:21]

Committed patchset #12 (id:220001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=198362