Transform animations should not collapse by default when interpolating

Transform animations should not collapse by default when interpolating

Previously, keyframed transform animation curves would produce a
gfx::Transform for a given time fraction. Some clients, however,
would wish to retain the unbaked, interpolated TransformOperations.
With this CL, we no longer bake the TransformOperations by default.
Clients that wish this behavior can call TransformOperations::Apply
themselves.

BUG=738644
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2971503002
Cr-Commit-Position: refs/heads/master@{#484144}
Committed: https://chromium.googlesource.com/chromium/src/+/699fb5ef02326c0ae298c0f94b04cfd674da0668

[Ian Vollick, 2017-07-01 03:53:26]

Description was changed from

==========
Transform animations should not collapse by default when interpolating

Previously, keyframed transform animation curves would produce a
gfx::Transform for a given time fraction. Some clients, however,
would wish to retain the unbaked, interpolated TransformOperations.
With this CL, we no longer bake the TransformOperations by default.
Clients that wish this behavior can call TransformOperations::Apply
themselves.

BUG=738644
==========

to

==========
Transform animations should not collapse by default when interpolating

Previously, keyframed transform animation curves would produce a
gfx::Transform for a given time fraction. Some clients, however,
would wish to retain the unbaked, interpolated TransformOperations.
With this CL, we no longer bake the TransformOperations by default.
Clients that wish this behavior can call TransformOperations::Apply
themselves.

BUG=738644
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Ian Vollick, 2017-07-01 03:53:30]

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

[commit-bot: I haz the power, 2017-07-01 03:53:37]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ian Vollick, 2017-07-01 03:53:57]

Description was changed from

==========
Transform animations should not collapse by default when interpolating

Previously, keyframed transform animation curves would produce a
gfx::Transform for a given time fraction. Some clients, however,
would wish to retain the unbaked, interpolated TransformOperations.
With this CL, we no longer bake the TransformOperations by default.
Clients that wish this behavior can call TransformOperations::Apply
themselves.

BUG=738644
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Transform animations should not collapse by default when interpolating

Previously, keyframed transform animation curves would produce a
gfx::Transform for a given time fraction. Some clients, however,
would wish to retain the unbaked, interpolated TransformOperations.
With this CL, we no longer bake the TransformOperations by default.
Clients that wish this behavior can call TransformOperations::Apply
themselves.

BUG=738644
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Ian Vollick, 2017-07-01 03:53:58]

vollick@chromium.org changed reviewers:
+ ajuma@chromium.org, flackr@chromium.org

[Ian Vollick, 2017-07-01 03:54:38]

On 2017/07/01 03:53:58, Ian Vollick wrote:
> mailto:vollick@chromium.org changed reviewers:
> + mailto:ajuma@chromium.org, mailto:flackr@chromium.org

Ali, Rob, PTAL!

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

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

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

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

[Ian Vollick, 2017-07-01 04:25:04]

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

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

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

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

[commit-bot: I haz the power, 2017-07-01 04:43:18]

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

[Ian Vollick, 2017-07-01 13:46:32]

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

[commit-bot: I haz the power, 2017-07-01 13:46:34]

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-07-01 14:33:21]

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

[commit-bot: I haz the power, 2017-07-01 14:33:22]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Ian Vollick, 2017-07-01 14:46:30]

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

[commit-bot: I haz the power, 2017-07-01 14:46:34]

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-07-01 16:02:27]

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

[commit-bot: I haz the power, 2017-07-01 16:02:28]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Ian Vollick, 2017-07-02 21:14:21]

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

[commit-bot: I haz the power, 2017-07-02 21:14:27]

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-07-02 21:53:38]

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

[commit-bot: I haz the power, 2017-07-02 21:53:38]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[ajuma, 2017-07-04 14:50:56]

https://codereview.chromium.org/2971503002/diff/80001/cc/animation/transform_...
File cc/animation/transform_operation.cc (right):

https://codereview.chromium.org/2971503002/diff/80001/cc/animation/transform_...
cc/animation/transform_operation.cc:226: result->matrix = to_matrix;
We could get away with less matrix copying here by doing away with |to_matrix|
and |from_matrix|, and blending directly into result->matrix. Something like:
if (!IsOperationIdentity(to))
  result->matrix = to->matrix;
if (!result->matrix.Blend(from->matrix, progress))
  return false;

It might be slightly less readable though, so feel free to leave as-is if you
think it's not worth changing.

https://codereview.chromium.org/2971503002/diff/80001/cc/animation/transform_...
File cc/animation/transform_operations_unittest.cc (right):

https://codereview.chromium.org/2971503002/diff/80001/cc/animation/transform_...
cc/animation/transform_operations_unittest.cc:268: 
Please add a couple tests to verify
the unbaked TransformOperatoins produced by Blend (one case with matching
types and one case with non-matching type where we fallback to matrix blending).

https://codereview.chromium.org/2971503002/diff/80001/cc/test/geometry_test_u...
File cc/test/geometry_test_utils.cc (left):

https://codereview.chromium.org/2971503002/diff/80001/cc/test/geometry_test_u...
cc/test/geometry_test_utils.cc:34: EXPECT_FLOAT_EQ((expected).matrix().get(3,
3), (actual).matrix().get(3, 3));
Thanks for cleaning this up!

https://codereview.chromium.org/2971503002/diff/80001/ui/compositor/transform...
File ui/compositor/transform_animation_curve_adapter.cc (right):

https://codereview.chromium.org/2971503002/diff/80001/ui/compositor/transform...
ui/compositor/transform_animation_curve_adapter.cc:14: static
cc::TransformOperations wrapTransform(const gfx::Transform& transform) {
nit: capitalize function name

https://codereview.chromium.org/2971503002/diff/80001/ui/compositor/transform...
ui/compositor/transform_animation_curve_adapter.cc:29:
target_value_(target_value),
If we call WrapTransform here so the stored intial_value_ and target_value_ are
TransformOperations, we save having to call WrapTransform on every call to
::GetValue.

[Ian Vollick, 2017-07-04 16:14:45]

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

[commit-bot: I haz the power, 2017-07-04 16:14:53]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ian Vollick, 2017-07-04 16:15:18]

https://codereview.chromium.org/2971503002/diff/80001/cc/animation/transform_...
File cc/animation/transform_operation.cc (right):

https://codereview.chromium.org/2971503002/diff/80001/cc/animation/transform_...
cc/animation/transform_operation.cc:226: result->matrix = to_matrix;
On 2017/07/04 14:50:56, ajuma wrote:
> We could get away with less matrix copying here by doing away with |to_matrix|
> and |from_matrix|, and blending directly into result->matrix. Something like:
> if (!IsOperationIdentity(to))
>   result->matrix = to->matrix;
> if (!result->matrix.Blend(from->matrix, progress))
>   return false;
> 
> It might be slightly less readable though, so feel free to leave as-is if you
> think it's not worth changing.

Done.

https://codereview.chromium.org/2971503002/diff/80001/cc/animation/transform_...
File cc/animation/transform_operations_unittest.cc (right):

https://codereview.chromium.org/2971503002/diff/80001/cc/animation/transform_...
cc/animation/transform_operations_unittest.cc:268: 
On 2017/07/04 14:50:56, ajuma wrote:
> Please add a couple tests to verify
> the unbaked TransformOperatoins produced by Blend (one case with matching
> types and one case with non-matching type where we fallback to matrix
blending).

Done.

https://codereview.chromium.org/2971503002/diff/80001/ui/compositor/transform...
File ui/compositor/transform_animation_curve_adapter.cc (right):

https://codereview.chromium.org/2971503002/diff/80001/ui/compositor/transform...
ui/compositor/transform_animation_curve_adapter.cc:14: static
cc::TransformOperations wrapTransform(const gfx::Transform& transform) {
On 2017/07/04 14:50:56, ajuma wrote:
> nit: capitalize function name

Done.

https://codereview.chromium.org/2971503002/diff/80001/ui/compositor/transform...
ui/compositor/transform_animation_curve_adapter.cc:29:
target_value_(target_value),
On 2017/07/04 14:50:56, ajuma wrote:
> If we call WrapTransform here so the stored intial_value_ and target_value_
are
> TransformOperations, we save having to call WrapTransform on every call to
> ::GetValue.

Done.

[ajuma, 2017-07-04 16:38:36]

Thanks, lgtm

[Ian Vollick, 2017-07-04 17:07:06]

The CQ bit was checked by vollick@chromium.org

[Ian Vollick, 2017-07-04 17:07:07]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/2971503002/#ps120001
(title: ".")

[commit-bot: I haz the power, 2017-07-04 17:07: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, 2017-07-04 19:02:39]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1499188026960140,
"parent_rev": "01d78a2724ab605aa5438743cef787e537632f03", "commit_rev":
"699fb5ef02326c0ae298c0f94b04cfd674da0668"}

[commit-bot: I haz the power, 2017-07-04 19:02:54]

Description was changed from

==========
Transform animations should not collapse by default when interpolating

Previously, keyframed transform animation curves would produce a
gfx::Transform for a given time fraction. Some clients, however,
would wish to retain the unbaked, interpolated TransformOperations.
With this CL, we no longer bake the TransformOperations by default.
Clients that wish this behavior can call TransformOperations::Apply
themselves.

BUG=738644
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Transform animations should not collapse by default when interpolating

Previously, keyframed transform animation curves would produce a
gfx::Transform for a given time fraction. Some clients, however,
would wish to retain the unbaked, interpolated TransformOperations.
With this CL, we no longer bake the TransformOperations by default.
Clients that wish this behavior can call TransformOperations::Apply
themselves.

BUG=738644
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2971503002
Cr-Commit-Position: refs/heads/master@{#484144}
Committed:
https://chromium.googlesource.com/chromium/src/+/699fb5ef02326c0ae298c0f94b04...
==========

[commit-bot: I haz the power, 2017-07-04 19:02:55]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/699fb5ef02326c0ae298c0f94b04...