Added a CallbackLayerAnimationObserver.

ui/compositor/layer_animation_observer.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef UI_COMPOSITOR_LAYER_ANIMATION_OBSERVER_H_
 #define UI_COMPOSITOR_LAYER_ANIMATION_OBSERVER_H_
 
 #include <map>
 #include <set>
 
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
 #include "ui/compositor/compositor_export.h"
 #include "ui/compositor/layer_animation_element.h"
 
 namespace ui {
 
+namespace test {
+class LayerAnimationObserverTestApi;
+}  // namespace test
+
 class LayerAnimationSequence;
 class ScopedLayerAnimationSettings;
 class ImplicitAnimationObserver;
 
 // LayerAnimationObservers are notified when animations complete.
 class COMPOSITOR_EXPORT LayerAnimationObserver  {
  public:
   // Called when the |sequence| starts.
   virtual void OnLayerAnimationStarted(LayerAnimationSequence* sequence);
 
@@ skipping 11 matching lines @@
       LayerAnimationSequence* sequence) = 0;
 
  protected:
   typedef std::set<LayerAnimationSequence*> AttachedSequences;
 
   LayerAnimationObserver();
   virtual ~LayerAnimationObserver();
 
   // If the animator is destroyed during an animation, the animations are
   // aborted. The resulting NotifyAborted notifications will NOT be sent to
-  // this observer if this function returns false. NOTE: IF YOU override THIS
-  // FUNCTION TO RETURN TRUE, YOU MUST REMEMBER TO REMOVE YOURSELF AS AN
-  // OBSERVER WHEN YOU ARE DESTROYED.
+  // this observer if this function returns false. An observer who wants to
+  // receive the NotifyAborted notifications during destruction can override
+  // this function to return true.
+  //
+  // *** IMPORTANT ***: If you override this function to return true AND you are
+  // using the pattern described below, then make sure you explicity remove

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

I would say "... and you are the owner of the observer ..." rather than
referring to later text.

[bruthig, 2015/10/13 19:41:42]

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

+  // yourself as an observer before the animator is destroyed!
+  //
+  // This opt-in pattern is used because it is a common pattern for a class to
+  // be the observer of an animation that it itself owns. In such a case the
+  // animator will be destroyed as the owning class is destructed and we don't
+  // want the NotifyAbort being invoked on a partially destroyed observer.
   virtual bool RequiresNotificationWhenAnimatorDestroyed() const;
 
   // Called when |this| is added to |sequence|'s observer list.
   virtual void OnAttachedToSequence(LayerAnimationSequence* sequence);
 
   // Called when |this| is removed to |sequence|'s observer list.
   virtual void OnDetachedFromSequence(LayerAnimationSequence* sequence);
 
   // Detaches this observer from all sequences it is currently observing.
   void StopObserving();
 
   const AttachedSequences& attached_sequences() const {
     return attached_sequences_;
   }
 
  private:
   friend class LayerAnimationSequence;
+  friend class test::LayerAnimationObserverTestApi;

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

What's the advantage to this instead of just making a LayerAnimationSequence in
the unittest?

[bruthig, 2015/10/13 19:41:42]

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.

 
   // Called when |this| is added to |sequence|'s observer list.
   void AttachedToSequence(LayerAnimationSequence* sequence);
 
   // Called when |this| is removed to |sequence|'s observer list.
   // This will only result in notifications if |send_notification| is true.
   void DetachedFromSequence(LayerAnimationSequence* sequence,
                             bool send_notification);
 
   AttachedSequences attached_sequences_;
@@ skipping 67 matching lines @@
                    AnimationStatus> PropertyAnimationStatusMap;
   PropertyAnimationStatusMap property_animation_status_;
 
   // True if OnLayerAnimationScheduled() has been called at least once.
   bool first_sequence_scheduled_;
 };
 
 }  // namespace ui
 
 #endif  // UI_COMPOSITOR_LAYER_ANIMATION_OBSERVER_H_