Basic support for animating window properties in Mus.

Basic support for animating window properties in Mus.

- Adding Mojo types for specifying opacity and transform animations.
- Adding support for scheduling and Tick()ing the animations on the Mus side.
- Unit tests.

BUG=548396
Committed: https://crrev.com/f29c7b9ceb70fab3a19f2e0ea18aa30757188236
Cr-Commit-Position: refs/heads/master@{#402939}

[mfomitchev, 2016-06-15 21:45:26]

Description was changed from

==========
Basic support for animating window properties in Mus.

- Adding Mojo types for specifying opacity and transform animations.
- Adding support for scheduling and Tick()ing the animations on the Mus side.

BUG=548396
==========

to

==========
Basic support for animating window properties in Mus.

- Adding Mojo types for specifying opacity and transform animations.
- Adding support for scheduling and Tick()ing the animations on the Mus side.
- Unit tests.

BUG=548396
==========

[mfomitchev, 2016-06-15 21:45:43]

mfomitchev@chromium.org changed reviewers:
+ sky@chromium.org

[sky, 2016-06-16 16:36:33]

https://codereview.chromium.org/2072573002/diff/1/components/mus/public/inter...
File components/mus/public/interfaces/animations.mojom (right):

https://codereview.chromium.org/2072573002/diff/1/components/mus/public/inter...
components/mus/public/interfaces/animations.mojom:15: //import
"ui/gfx/geometry/mojo/geometry.mojom";
?

https://codereview.chromium.org/2072573002/diff/1/components/mus/public/inter...
components/mus/public/interfaces/animations.mojom:26: gfx.mojom.Transform
transform;
Shouldn't this be a ? as it isn't applicable to all properties?

https://codereview.chromium.org/2072573002/diff/1/components/mus/ws/animation...
File components/mus/ws/animation_runner.cc (right):

https://codereview.chromium.org/2072573002/diff/1/components/mus/ws/animation...
components/mus/ws/animation_runner.cc:55: ScopedVector<ScheduledAnimationGroup>
groups;
ScopedVector is deprecated.

https://codereview.chromium.org/2072573002/diff/1/components/mus/ws/animation...
File components/mus/ws/animation_runner.h (right):

https://codereview.chromium.org/2072573002/diff/1/components/mus/ws/animation...
components/mus/ws/animation_runner.h:91: base::ScopedPtrHashMap<ServerWindow*,
ScopedPtrHashMap is deprecated now.

https://codereview.chromium.org/2072573002/diff/1/components/mus/ws/scheduled...
File components/mus/ws/scheduled_animation_group.cc (right):

https://codereview.chromium.org/2072573002/diff/1/components/mus/ws/scheduled...
components/mus/ws/scheduled_animation_group.cc:10: //#include
"mojo/converters/geometry/geometry_type_converters.h"
?

https://codereview.chromium.org/2072573002/diff/1/components/mus/ws/scheduled...
components/mus/ws/scheduled_animation_group.cc:134:
!IsAnimationValueValid(element.property, *element.start_value))
I think it would be useful to DVLOG invalid data. Hopefully that will help folks
figure out if they did something wrong.

https://codereview.chromium.org/2072573002/diff/1/components/mus/ws/scheduled...
components/mus/ws/scheduled_animation_group.cc:153: if
(transport_group.sequences.size() == 0u)
empty

[sky, 2016-06-16 16:36:54]

Also, you'll need a security review for the new mojom.

[mfomitchev, 2016-06-22 21:59:36]

https://codereview.chromium.org/2072573002/diff/1/components/mus/public/inter...
File components/mus/public/interfaces/animations.mojom (right):

https://codereview.chromium.org/2072573002/diff/1/components/mus/public/inter...
components/mus/public/interfaces/animations.mojom:15: //import
"ui/gfx/geometry/mojo/geometry.mojom";
On 2016/06/16 16:36:33, sky wrote:
> ?

Done.

https://codereview.chromium.org/2072573002/diff/1/components/mus/public/inter...
components/mus/public/interfaces/animations.mojom:26: gfx.mojom.Transform
transform;
On 2016/06/16 16:36:33, sky wrote:
> Shouldn't this be a ? as it isn't applicable to all properties?

Done. Note that we still always return true for transforms in
IsAnimationValueValid() - we can't differentiate between the true Identity
transform and the transform that wasn't specified (and was thus initialized to
Identity). I think it's ok for now, but I am not sure if that's what we want
longer term. On one hand it's marginally less "correct". OTOH not having to pass
Identity transforms over IPC lets us save on IPC traffic, so it might be
actually be a good thing.

https://codereview.chromium.org/2072573002/diff/1/components/mus/ws/animation...
File components/mus/ws/animation_runner.cc (right):

https://codereview.chromium.org/2072573002/diff/1/components/mus/ws/animation...
components/mus/ws/animation_runner.cc:55: ScopedVector<ScheduledAnimationGroup>
groups;
On 2016/06/16 16:36:33, sky wrote:
> ScopedVector is deprecated.

Done.

https://codereview.chromium.org/2072573002/diff/1/components/mus/ws/animation...
File components/mus/ws/animation_runner.h (right):

https://codereview.chromium.org/2072573002/diff/1/components/mus/ws/animation...
components/mus/ws/animation_runner.h:91: base::ScopedPtrHashMap<ServerWindow*,
On 2016/06/16 16:36:33, sky wrote:
> ScopedPtrHashMap is deprecated now.

Done.

https://codereview.chromium.org/2072573002/diff/1/components/mus/ws/scheduled...
File components/mus/ws/scheduled_animation_group.cc (right):

https://codereview.chromium.org/2072573002/diff/1/components/mus/ws/scheduled...
components/mus/ws/scheduled_animation_group.cc:10: //#include
"mojo/converters/geometry/geometry_type_converters.h"
On 2016/06/16 16:36:33, sky wrote:
> ?

Done.

https://codereview.chromium.org/2072573002/diff/1/components/mus/ws/scheduled...
components/mus/ws/scheduled_animation_group.cc:134:
!IsAnimationValueValid(element.property, *element.start_value))
On 2016/06/16 16:36:33, sky wrote:
> I think it would be useful to DVLOG invalid data. Hopefully that will help
folks
> figure out if they did something wrong.

Added in a few places.

https://codereview.chromium.org/2072573002/diff/1/components/mus/ws/scheduled...
components/mus/ws/scheduled_animation_group.cc:153: if
(transport_group.sequences.size() == 0u)
On 2016/06/16 16:36:33, sky wrote:
> empty

Done.

[sky, 2016-06-22 23:21:28]

https://codereview.chromium.org/2072573002/diff/20001/components/mus/public/i...
File components/mus/public/interfaces/animations.mojom (right):

https://codereview.chromium.org/2072573002/diff/20001/components/mus/public/i...
components/mus/public/interfaces/animations.mojom:40: AnimationValue
start_value;
Don't you need the ? here? You kept the logic that has the null check.

[mfomitchev, 2016-06-23 14:54:28]

https://codereview.chromium.org/2072573002/diff/20001/components/mus/public/i...
File components/mus/public/interfaces/animations.mojom (right):

https://codereview.chromium.org/2072573002/diff/20001/components/mus/public/i...
components/mus/public/interfaces/animations.mojom:40: AnimationValue
start_value;
On 2016/06/22 23:21:28, sky wrote:
> Don't you need the ? here? You kept the logic that has the null check.

Oops, sorry! I was toggling "?" for gfx.mojom.Transform in AnimationValue back
and forth to see what effect it had on the generated cpp code (only
MOJO_INTERNAL_DLOG_SERIALIZATION_WARNING and ValidatePointerNonNullable), and I
must've changed this by accident. Fixed.

[sky, 2016-06-23 15:55:36]

LGTM

[mfomitchev, 2016-06-23 16:52:54]

mfomitchev@chromium.org changed reviewers:
+ meacer@chromium.org

[mfomitchev, 2016-06-23 16:52:54]

+meacer@chromium.org
for the security review

[mfomitchev, 2016-06-27 14:11:29]

meacer: ping
Can you PTAL at components/mus/public/interfaces/animations.mojom?

[meacer, 2016-06-27 19:24:44]

Sorry for the delay. IPC Lgtm.

[mfomitchev, 2016-06-27 19:32:00]

The CQ bit was checked by mfomitchev@chromium.org

[commit-bot: I haz the power, 2016-06-27 19:32:54]

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

[commit-bot: I haz the power, 2016-06-27 21:04:56]

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

[commit-bot: I haz the power, 2016-06-27 21:04:58]

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

[mfomitchev, 2016-06-29 20:03:41]

The CQ bit was checked by mfomitchev@chromium.org

[mfomitchev, 2016-06-29 20:03:42]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
meacer@chromium.org
Link to the patchset: https://codereview.chromium.org/2072573002/#ps60001
(title: "Replacing .empty() with .size() == 0 to account for is_null case in 
mojo::Array.")

[commit-bot: I haz the power, 2016-06-29 20:06:29]

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

[commit-bot: I haz the power, 2016-06-29 21:38:25]

Description was changed from

==========
Basic support for animating window properties in Mus.

- Adding Mojo types for specifying opacity and transform animations.
- Adding support for scheduling and Tick()ing the animations on the Mus side.
- Unit tests.

BUG=548396
==========

to

==========
Basic support for animating window properties in Mus.

- Adding Mojo types for specifying opacity and transform animations.
- Adding support for scheduling and Tick()ing the animations on the Mus side.
- Unit tests.

BUG=548396
==========

[commit-bot: I haz the power, 2016-06-29 21:38:26]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-29 21:38:31]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-06-29 21:40:31]

Description was changed from

==========
Basic support for animating window properties in Mus.

- Adding Mojo types for specifying opacity and transform animations.
- Adding support for scheduling and Tick()ing the animations on the Mus side.
- Unit tests.

BUG=548396
==========

to

==========
Basic support for animating window properties in Mus.

- Adding Mojo types for specifying opacity and transform animations.
- Adding support for scheduling and Tick()ing the animations on the Mus side.
- Unit tests.

BUG=548396

Committed: https://crrev.com/f29c7b9ceb70fab3a19f2e0ea18aa30757188236
Cr-Commit-Position: refs/heads/master@{#402939}
==========

[commit-bot: I haz the power, 2016-06-29 21:40:32]

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