Fixing race conditions in ui::content::WindowSlider which could cause the overscroll overlay to nev…

content/browser/web_contents/aura/window_slider.h

Index: content/browser/web_contents/aura/window_slider.h
diff --git a/content/browser/web_contents/aura/window_slider.h b/content/browser/web_contents/aura/window_slider.h
index 0ebe79239fd35277d712dba0163d84629c51e506..a329b2f08fde23c4568ed75391fd42a608e31588 100644
--- a/content/browser/web_contents/aura/window_slider.h
+++ b/content/browser/web_contents/aura/window_slider.h
@@ -10,6 +10,7 @@
 #include "base/memory/weak_ptr.h"
 #include "content/common/content_export.h"
 #include "ui/aura/window_observer.h"
+#include "ui/compositor/layer_animator.h"
 #include "ui/events/event_handler.h"
 
 namespace ui {
@@ -28,25 +29,33 @@ class CONTENT_EXPORT WindowSlider : public ui::EventHandler,
    public:
     virtual ~Delegate() {}
 
-    // Creates a layer to show in the background, as the window-layer slides
-    // with the scroll gesture.
-    // The WindowSlider takes ownership of the created layer.
-    virtual ui::Layer* CreateBackLayer() = 0;
-
-    // Creates a layer to slide on top of the window-layer with the scroll
+    // Called when the slide in the backward direction starts. Returns a layer
+    // to show in the background, as the window-layer slides with the scroll
     // gesture.
     // The WindowSlider takes ownership of the created layer.
-    virtual ui::Layer* CreateFrontLayer() = 0;
+    virtual ui::Layer* OnSlideBackStartedCreateLayer() = 0;

[sadrul, 2014/03/19 20:21:18]

I want the window-slider to work in terms of back/front, and not necessarily in
terms of back/forward.

[mfomitchev, 2014/03/20 15:10:06]

Ah, do you mean back/front in a sense of below/above?
ONO sets slide_direction_ based on these callbacks, which is why I chose to make
it explicit in the callback names. If you don't like this, should we also rename
slide_direction_ to something else?

[sadrul, 2014/03/24 20:20:46]

Yes.
ONO is aware of back/forward navigation. So it should maintain state that
corresponds to back/forward navigation. WindowSlider doesn't deal with
navigation. Its responsibility is to manage the action of sliding a window above
another one. For this slide operation, it needs to know about the top/bottom (or
below/above, front/back etc.) layers. So it should maintain state that
corresponds to this. That's how I am thinking.

[mfomitchev, 2014/03/24 22:36:47]

Ok, that makes sense. I am still not a big fan of the old names, because these
callbacks are not just about the layer creation, they are also about updating
the 'target'. E.g. looking at the old names, you can imagine that you are
allowed to save front/back layers in WindowSlider rather than request a new
layer every time the slide direction changes. Perhaps call them
CreateFrontLayerAndSetAsTarget(), CreateBackLayerAndSetAsTarget()? (Or
CreateXxxLayerAndSetAsSlideTarget()?) 

[sadrul, 2014/03/25 23:01:56]

I don't understand what you mean by 'updating the target'. The ONO
implementation maintains internal state to decide what to do when the
window-slide completes. If we don't want that, then we can include that
information in the OnWindowSlideComplet*() callbacks. But I still don't see any
value in changing that.

I am not convinced there is an issue with the name as they are. I think in the
context of the WindowSlider, the names make good sense. You can update the doc
to explain that it gets called every time the direction changes if you think
that'd be useful.

 
-    // Called when the slide is complete. Note that at the end of a completed
-    // slide, the window-layer may have been transformed. The callback here
-    // should reset the transform if necessary.
-    virtual void OnWindowSlideComplete() = 0;
+    // Called when the slide in the forward direction starts. Returns a layer
+    // to slide on top of the window-layer with the scroll gesture.
+    // The WindowSlider takes ownership of the created layer.
+    virtual ui::Layer* OnSlideForwardStartedCreateLayer() = 0;
 
     // Called when the slide is aborted. Note that when the slide is aborted,
     // the WindowSlider resets any transform it applied on the window-layer.
     virtual void OnWindowSlideAborted() = 0;
 
+    // Called when the sliding window is flinged off-screen (in the direction
+    // it was sliding).
+    // This callback is always preceeded by one of the OnSlide***Started()

[sadrul, 2014/03/19 20:21:18]

What are the OnSlide***Started() callbacks?

[mfomitchev, 2014/03/20 15:10:06]

Done.

+    // callbacks, and is guaranteed to be followed by the
+    // OnWindowFlingCompleted() callback.
+    virtual void OnWindowFlingStarted() = 0;
+
+    // Called when the window fling animation is complete. Note that at the end
+    // the window-layer may have been transformed. The callback here should
+    // reset the transform if necessary.
+    virtual void OnWindowFlingCompleted() = 0;

[sadrul, 2014/03/19 20:21:18]

What if the gesture ends without a fling?

Can we call these OnWindowSlideWillCompleteSoon() and OnWindowSlideComplete()
instead? The delegate doesn't need to know if the slide completed because of a
fling or not. It just needs to know when the slide-complete animation starts,
and ends (so something like OnWindowSlideCompleteAnimtionStart()
OnWindowSlideCompleteAnimationEnd() also work).

[mfomitchev, 2014/03/20 15:10:06]

Done.

+
     // Called when the slider is destroyed.
     virtual void OnWindowSliderDestroyed() = 0;
   };
@@ -76,16 +85,20 @@ class CONTENT_EXPORT WindowSlider : public ui::EventHandler,
 
   void UpdateForFling(float x_velocity, float y_velocity);
 
-  // Resets any in-progress slide.
-  void ResetScroll();
+  // Stops all slider-owned animations, progressing them to their end-points.
+  // Note that depending on the sate of the Delegate and the WindowSlider, this
+  // may destroy the WindowSlider through animation callbacks.
+  void CompleteActiveAnimations();
 
-  // Cancels any scroll/animation in progress.
-  void CancelScroll();
+  // Resets in-progress slide if any, and starts the animation of the slidden
+  // window to its original position.
+  void ResetSlide();
 
   // The following callbacks are triggered after an animation.
-  void CompleteWindowSlideAfterAnimation();
+  void FlingAnimationCompleted(ui::Layer* layer, ShadowLayerDelegate* shadow);
 
-  void AbortWindowSlideAfterAnimation();
+  void ResetSlideAnimationCompleted(ui::Layer* layer,
+                                    ShadowLayerDelegate* shadow);
 
   // Overridden from ui::EventHandler:
   virtual void OnKeyEvent(ui::KeyEvent* event) OVERRIDE;
@@ -109,6 +122,10 @@ class CONTENT_EXPORT WindowSlider : public ui::EventHandler,
   // destroy |owner_|.
   aura::Window* owner_;
 
+  // Set to the Animator of the currently active animation. If no animation is
+  // active, this is set to NULL.
+  ui::LayerAnimator* active_animator_;
+
   // The accumulated amount of horizontal scroll.
   float delta_x_;