Added a CallbackLayerAnimationObserver.

Added a CallbackLayerAnimationObserver.

This is based off of the CL here: https://codereview.chromium.org/1381463002/

This is some preliminary work to add callbacks to the InkDropAnimation
that will notify observers when an animation is started and ended. It was
decided to add a general CallbackLayerAnimationObserver that will
re-usuable by other things such as the CallbackAnimationObserver in
ash/wm/session_state_animator_impl.cc.

TEST=CallbackLayerAnimationObserverTest.VerifyInitialState
TEST=CallbackLayerAnimationObserverTest.ExplicitlyDeleteObserverInAnimationStartedCallbackWithNoSequencesAttached
TEST=CallbackLayerAnimationObserverTest.ExplicitlyDeleteObserverInAnimationStartedCallbackWithSomeSequencesAttached
TEST=CallbackLayerAnimationObserverTest.IgnoreTrueReturnValueForAnimationEndedCallbackIfExplicitlyDeleted
TEST=CallbackLayerAnimationObserverTest.SetActiveWhenNoSequencesWereAttached
TEST=CallbackLayerAnimationObserverTest.SetActiveWhenAllSequencesAreAttachedButNoneWereStarted
TEST=CallbackLayerAnimationObserverTest.SetActiveWhenAllSequencesAreAttachedAndOnlySomeWereStarted
TEST=CallbackLayerAnimationObserverTest.SetActiveWhenAllSequencesAreAttachedAndOnlySomeWereCompleted
TEST=CallbackLayerAnimationObserverTest.SetActiveAfterAllSequencesWereStartedButNoneWereCompleted
TEST=CallbackLayerAnimationObserverTest.SetActiveWhenAllSequencesAreStartedAndOnlySomeWereCompleted
TEST=CallbackLayerAnimationObserverTest.SetActiveWhenAllSequencesWereCompleted
TEST=CallbackLayerAnimationObserverTest.SetActiveAgainAfterAllSequencesWereCompleted

BUG=522175, 537614
Committed: https://crrev.com/3f35acd6de9138929e8a5e993c08b3c049d8b653
Cr-Commit-Position: refs/heads/master@{#353855}

[bruthig, 2015-09-28 20:12:30]

bruthig@chromium.org changed reviewers:
+ danakj@chromium.org

[bruthig, 2015-09-28 20:12:30]

danakj@, Can you PTAL? I've added a new re-useable LayerAnimationObserver that
is preliminary work for this CL: https://codereview.chromium.org/1373983002/ .

Note that, I'm still planning on adding some unittests to this review but just
wanted to make sure there wasn't any reason to not add this new observer.

[bruthig, 2015-10-05 18:01:47]

bruthig@chromium.org changed reviewers:
+ vollick@chromium.org

[bruthig, 2015-10-05 18:01:47]

danakj@ I've added the necessary tests, can you PTAL?

vollick@, can you take a look at the re-worded documentation for
LayerAnimationObserver::RequiresNotificationWhenAnimatorDestroyed() in
layer_animation_observer.h?

[bruthig, 2015-10-06 21:30:37]

https://codereview.chromium.org/1369393002/diff/40001/ui/compositor/callback_...
File ui/compositor/callback_layer_animation_observer.h (right):

https://codereview.chromium.org/1369393002/diff/40001/ui/compositor/callback_...
ui/compositor/callback_layer_animation_observer.h:70: explicit
CallbackLayerAnimationObserver(
TODO: Remove explicit.

[danakj, 2015-10-07 15:11:09]

danakj@chromium.org changed reviewers:
+ ajuma@chromium.org

[danakj, 2015-10-07 15:11:10]

+ajuma

https://codereview.chromium.org/1369393002/diff/40001/ui/compositor/callback_...
File ui/compositor/callback_layer_animation_observer.h (right):

https://codereview.chromium.org/1369393002/diff/40001/ui/compositor/callback_...
ui/compositor/callback_layer_animation_observer.h:86: void
OnLayerAnimationStarted(ui::LayerAnimationSequence* sequence) override;
This isn't a member of LayerAnimationObserver?

[bruthig, 2015-10-07 15:21:23]

https://codereview.chromium.org/1369393002/diff/40001/ui/compositor/callback_...
File ui/compositor/callback_layer_animation_observer.h (right):

https://codereview.chromium.org/1369393002/diff/40001/ui/compositor/callback_...
ui/compositor/callback_layer_animation_observer.h:86: void
OnLayerAnimationStarted(ui::LayerAnimationSequence* sequence) override;
On 2015/10/07 15:11:10, danakj_OOO_til_10-12 wrote:
> This isn't a member of LayerAnimationObserver?

It's being added in my pending CL here:
https://codereview.chromium.org/1381463002

[ajuma, 2015-10-07 20:14:55]

https://codereview.chromium.org/1369393002/diff/40001/ui/compositor/callback_...
File ui/compositor/callback_layer_animation_observer.h (right):

https://codereview.chromium.org/1369393002/diff/40001/ui/compositor/callback_...
ui/compositor/callback_layer_animation_observer.h:39: //      ui::LayerAnimator*
animator_2 = layer_->GetAnimator();
Should these by layer_1 and layer_2, not the same layer_ on both lines?
(Otherwise, animator_1 == animator_2.)

https://codereview.chromium.org/1369393002/diff/40001/ui/compositor/callback_...
File ui/compositor/callback_layer_animation_observer_unittest.cc (right):

https://codereview.chromium.org/1369393002/diff/40001/ui/compositor/callback_...
ui/compositor/callback_layer_animation_observer_unittest.cc:162: //
|callbacks_|.
I'm not sure I follow this part. The observer's destructor doesn't seem to
detach it from sequences (or maybe i'm just missing where that happens), so I'm
confused about what deleting it first actually does.

[bruthig, 2015-10-07 21:37:29]

ajuma@, can you please have another look?

https://codereview.chromium.org/1369393002/diff/40001/ui/compositor/callback_...
File ui/compositor/callback_layer_animation_observer.h (right):

https://codereview.chromium.org/1369393002/diff/40001/ui/compositor/callback_...
ui/compositor/callback_layer_animation_observer.h:39: //      ui::LayerAnimator*
animator_2 = layer_->GetAnimator();
On 2015/10/07 20:14:55, ajuma wrote:
> Should these by layer_1 and layer_2, not the same layer_ on both lines?
> (Otherwise, animator_1 == animator_2.)

Yes, good catch!

https://codereview.chromium.org/1369393002/diff/40001/ui/compositor/callback_...
ui/compositor/callback_layer_animation_observer.h:70: explicit
CallbackLayerAnimationObserver(
On 2015/10/06 21:30:37, bruthig wrote:
> TODO: Remove explicit.

Done.

https://codereview.chromium.org/1369393002/diff/40001/ui/compositor/callback_...
File ui/compositor/callback_layer_animation_observer_unittest.cc (right):

https://codereview.chromium.org/1369393002/diff/40001/ui/compositor/callback_...
ui/compositor/callback_layer_animation_observer_unittest.cc:162: //
|callbacks_|.
On 2015/10/07 20:14:55, ajuma wrote:
> I'm not sure I follow this part. The observer's destructor doesn't seem to
> detach it from sequences (or maybe i'm just missing where that happens), so
I'm
> confused about what deleting it first actually does.

Observers are detached from in
LayerAnimationObserver::~LayerAnimationObserver().  Does that clarify?

[ajuma, 2015-10-07 21:50:38]

lgtm

https://codereview.chromium.org/1369393002/diff/40001/ui/compositor/callback_...
File ui/compositor/callback_layer_animation_observer_unittest.cc (right):

https://codereview.chromium.org/1369393002/diff/40001/ui/compositor/callback_...
ui/compositor/callback_layer_animation_observer_unittest.cc:162: //
|callbacks_|.
On 2015/10/07 21:37:29, bruthig wrote:
> On 2015/10/07 20:14:55, ajuma wrote:
> > I'm not sure I follow this part. The observer's destructor doesn't seem to
> > detach it from sequences (or maybe i'm just missing where that happens), so
> I'm
> > confused about what deleting it first actually does.
> 
> Observers are detached from in
> LayerAnimationObserver::~LayerAnimationObserver().  Does that clarify?

Ah, yes, that's what I was missing. Thanks!

[bruthig, 2015-10-07 21:59:34]

The CQ bit was checked by bruthig@chromium.org

[commit-bot: I haz the power, 2015-10-07 22:02:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1369393002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1369393002/80001

[commit-bot: I haz the power, 2015-10-07 22:11:46]

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

[commit-bot: I haz the power, 2015-10-07 22:11:47]

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

[sadrul, 2015-10-08 00:51:52]

Can you explain a little how CallbackLayerAnimationObserver is different from
ImplicitAnimationObserver/ClosureAnimationObserver?

https://codereview.chromium.org/1369393002/diff/80001/ui/compositor/callback_...
File ui/compositor/callback_layer_animation_observer.h (right):

https://codereview.chromium.org/1369393002/diff/80001/ui/compositor/callback_...
ui/compositor/callback_layer_animation_observer.h:132:
DISALLOW_COPY_AND_ASSIGN(CallbackLayerAnimationObserver);
Like ImplicitAnimationObserver, does this need to track whether it is getting
destroyed during invoking callbacks
(https://code.google.com/p/chromium/codesearch#chromium/src/ui/compositor/laye...

[bruthig, 2015-10-08 20:32:18]

sadrul@, can you PTAL?

vollick@, do you have time to look at the files listed below in danakj@'s
absence or should I add piman@?
 - ui/compositor/test/*
 - ui/compositor/layer_animation_observer.h - re-worded documentation for
LayerAnimationObserver::RequiresNotificationWhenAnimatorDestroyed()
 - ui/compositor/BUILD.gn
 - ui/compositor/compositor.gyp

On 2015/10/08 00:51:52, sadrul wrote:
> Can you explain a little how CallbackLayerAnimationObserver is different from
> ImplicitAnimationObserver/ClosureAnimationObserver?

FTR I wasn't aware of the ClosureAnimationObserver because
ImplicitAnimationObserver (IAO) didn't appear to do what I wanted so I didn't
look at subclasses of it.
I opted not to use the IAO for two reasons. 1. It appeared to be very heavy
weight for what I wanted and 2. it didn't appear to be written with multiple
sequences animating the same property in mind. i.e. the last call to
UpdatePropertyAnimationStatus() is going to 'win'.  Having said that, I can
totally foresee the  IAO being redesigned to subclass from the
CallbackLayerAnimationObserver (CLAO) but I think that would be a considerable
amount of effort and outside the scope of this CL.

Some other subtle differences between the IAO and CLAO are:

- The CLAO notifies after all animations are completed (aka ended or aborted)
versus all being detached.
- The CLAO notifies after all animations are started versus the first one
scheduled.
- The CLAO will be notified of sequences aborted due to the animator being
destroyed, thus guaranteeing the callbacks to be invoked unless the CLAO itself
is destroyed pre-maturely.  Albeit it would be a minor change to parameterize
this.

https://codereview.chromium.org/1369393002/diff/80001/ui/compositor/callback_...
File ui/compositor/callback_layer_animation_observer.h (right):

https://codereview.chromium.org/1369393002/diff/80001/ui/compositor/callback_...
ui/compositor/callback_layer_animation_observer.h:132:
DISALLOW_COPY_AND_ASSIGN(CallbackLayerAnimationObserver);
On 2015/10/08 00:51:52, sadrul wrote:
> Like ImplicitAnimationObserver, does this need to track whether it is getting
> destroyed during invoking callbacks
>
(https://code.google.com/p/chromium/codesearch#chromium/src/ui/compositor/laye...

Done.

[bruthig, 2015-10-08 20:34:20]

ajuma@, can you PTAL at the diff against patch set 5?

[ajuma, 2015-10-08 20:44:12]

On 2015/10/08 20:34:20, bruthig wrote:
> ajuma@, can you PTAL at the diff against patch set 5?

lgtm % sadrul

[sadrul, 2015-10-09 20:27:06]

On 2015/10/08 20:32:18, bruthig wrote:
> sadrul@, can you PTAL?
> 
> vollick@, do you have time to look at the files listed below in danakj@'s
> absence or should I add piman@?
>  - ui/compositor/test/*
>  - ui/compositor/layer_animation_observer.h - re-worded documentation for
> LayerAnimationObserver::RequiresNotificationWhenAnimatorDestroyed()
>  - ui/compositor/BUILD.gn
>  - ui/compositor/compositor.gyp
> 
> On 2015/10/08 00:51:52, sadrul wrote:
> > Can you explain a little how CallbackLayerAnimationObserver is different
from
> > ImplicitAnimationObserver/ClosureAnimationObserver?
> 
> FTR I wasn't aware of the ClosureAnimationObserver because
> ImplicitAnimationObserver (IAO) didn't appear to do what I wanted so I didn't
> look at subclasses of it.
> I opted not to use the IAO for two reasons. 1. It appeared to be very heavy
> weight for what I wanted and 2. it didn't appear to be written with multiple
> sequences animating the same property in mind. i.e. the last call to
> UpdatePropertyAnimationStatus() is going to 'win'.  Having said that, I can
> totally foresee the  IAO being redesigned to subclass from the
> CallbackLayerAnimationObserver (CLAO) but I think that would be a considerable
> amount of effort and outside the scope of this CL.
> 
> Some other subtle differences between the IAO and CLAO are:
> 
> - The CLAO notifies after all animations are completed (aka ended or aborted)
> versus all being detached.
> - The CLAO notifies after all animations are started versus the first one
> scheduled.
> - The CLAO will be notified of sequences aborted due to the animator being
> destroyed, thus guaranteeing the callbacks to be invoked unless the CLAO
itself
> is destroyed pre-maturely.  Albeit it would be a minor change to parameterize
> this.

Thanks for the detailed explanation. As discussed offline, it would be useful to
try to unify the two into one and make the subtle differences more obvious and
explicit (e.g. have one kind of observer with explicit callbacks for each subtly
different events). It'd be neat if you could file a bug to make sure this work
is tracked.

> 
>
https://codereview.chromium.org/1369393002/diff/80001/ui/compositor/callback_...
> File ui/compositor/callback_layer_animation_observer.h (right):
> 
>
https://codereview.chromium.org/1369393002/diff/80001/ui/compositor/callback_...
> ui/compositor/callback_layer_animation_observer.h:132:
> DISALLOW_COPY_AND_ASSIGN(CallbackLayerAnimationObserver);
> On 2015/10/08 00:51:52, sadrul wrote:
> > Like ImplicitAnimationObserver, does this need to track whether it is
getting
> > destroyed during invoking callbacks
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/ui/compositor/laye...
> 
> Done.

[danakj, 2015-10-12 21:33:56]

https://codereview.chromium.org/1369393002/diff/140001/ui/compositor/layer_an...
File ui/compositor/layer_animation_observer.h (right):

https://codereview.chromium.org/1369393002/diff/140001/ui/compositor/layer_an...
ui/compositor/layer_animation_observer.h:58: // using the pattern described
below, then make sure you explicity remove
I would say "... and you are the owner of the observer ..." rather than
referring to later text.

https://codereview.chromium.org/1369393002/diff/140001/ui/compositor/layer_an...
ui/compositor/layer_animation_observer.h:82: friend class
test::LayerAnimationObserverTestApi;
What's the advantage to this instead of just making a LayerAnimationSequence in
the unittest?

https://codereview.chromium.org/1369393002/diff/140001/ui/compositor/test/lay...
File ui/compositor/test/layer_animation_observer_test_api.h (right):

https://codereview.chromium.org/1369393002/diff/140001/ui/compositor/test/lay...
ui/compositor/test/layer_animation_observer_test_api.h:19: // Wrappers for
LayerAnimationObserver:
s/:/./

[bruthig, 2015-10-13 19:41:43]

FYI I've created www.crbug.com/542825 to track the work to combine the
ImplicitAnimationObserver and the CallbackLayerAnimationObserver.

sadrul@, let me know if 542825 doesn't capture what's required.

danakj@, can you PTAL?

https://codereview.chromium.org/1369393002/diff/140001/ui/compositor/layer_an...
File ui/compositor/layer_animation_observer.h (right):

https://codereview.chromium.org/1369393002/diff/140001/ui/compositor/layer_an...
ui/compositor/layer_animation_observer.h:58: // using the pattern described
below, then make sure you explicity remove
On 2015/10/12 21:33:56, danakj wrote:
> I would say "... and you are the owner of the observer ..." rather than
> referring to later text.

I've tried to re-word this. PTAL.

https://codereview.chromium.org/1369393002/diff/140001/ui/compositor/layer_an...
ui/compositor/layer_animation_observer.h:82: friend class
test::LayerAnimationObserverTestApi;
On 2015/10/12 21:33:56, danakj wrote:
> What's the advantage to this instead of just making a LayerAnimationSequence
in
> the unittest?

There are a few advantages to testing the CallbackLayerAnimationObserver in
isolation which we can discuss offline if you are interested but I will outline
the 2 most compelling ones here.

(1) The test author doesn't need to know the implementation details of the
LayerAnimationSequence just so they can manipulate the SUT as desired.  This is
also very helpful to the test reader because they won't have to sift through
relevant and irrelevant details where irrelevant details are the setup and
configuration of the test collaborator, aka LayerAnimationSequence.  A more
concrete example is to think about the code required to setup and manipulate a
LayerAnimationSequence just to get
LayerAnimationObserver::OnLayerAnimationStarted() called.  The test would have
to instantiate and manage a LayerAnimationSequence.  It would have to configure
the LayerAnimationSequence to have a non-empty |elements_| vector which would
require creating a LayerAnimationElement. And the test would have to poke the
LayerAnimationSequence::Start() method so that the
LayerAnimationObserver::OnLayerAnimationStarted() method is called.  That's a
lot of work and indirection that will more easily confuse test authors and
readers.

(2) By using the real LayerAnimationSequence in a test for the
LayerAnimationObserver you are making the tests more coupled and thus brittle.
i.e. if the implementation of the LayerAnimationSequence were to change, but not
it's interaction contract with the observer, then you may have to update/modify
the LayerAnimationObserver tests just so they would still work.  If the scope of
the test is high, and the details/code required to manipulate the SUT or
collaborators is high then the test becomes brittle and there starts to be a
high overhead to test maintenance when changing the collaborators.

https://codereview.chromium.org/1369393002/diff/140001/ui/compositor/test/lay...
File ui/compositor/test/layer_animation_observer_test_api.h (right):

https://codereview.chromium.org/1369393002/diff/140001/ui/compositor/test/lay...
ui/compositor/test/layer_animation_observer_test_api.h:19: // Wrappers for
LayerAnimationObserver:
On 2015/10/12 21:33:56, danakj wrote:
> s/:/./

I'm not 100% what you meant here, assuming you meant change the ':' to a '.'.

[danakj, 2015-10-13 20:49:54]

OK I'm not sure about the test-api-as-friend thing, cuz then the state in the
tested class is being poked at in ways that it does not expect which can leave
it in an inconsistent state.

I've heard of this as a pattern elsewhere in chrome and maybe it's just less
familiar to me. It looks fine here though I'm still not totally convinced about
the value for these specific tests.

LGTM % ajuma

[bruthig, 2015-10-13 21:00:18]

The CQ bit was checked by bruthig@chromium.org

[bruthig, 2015-10-13 21:00:18]

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/1369393002/#ps160001
(title: "Addressed concerns from patch set 8.")

[commit-bot: I haz the power, 2015-10-13 21:05:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1369393002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1369393002/160001

[commit-bot: I haz the power, 2015-10-13 21:22:23]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-10-13 21:23:43]

Patchset 9 (id:??) landed as
https://crrev.com/3f35acd6de9138929e8a5e993c08b3c049d8b653
Cr-Commit-Position: refs/heads/master@{#353855}