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

content/browser/web_contents/aura/window_slider.h

 // Copyright (c) 2013 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 CONTENT_BROWSER_WEB_CONTENTS_AURA_WINDOW_SLIDER_H_
 #define CONTENT_BROWSER_WEB_CONTENTS_AURA_WINDOW_SLIDER_H_
 
 #include "base/compiler_specific.h"
 #include "base/memory/scoped_ptr.h"
 #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 {
 class Layer;
 }
 
 namespace content {
 
 class ShadowLayerDelegate;
 
 // A class for sliding the layer in a Window on top of other layers.
 class CONTENT_EXPORT WindowSlider : public ui::EventHandler,
                                     public aura::WindowObserver {
  public:
   class CONTENT_EXPORT Delegate {
    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;
   };
 
   // The WindowSlider slides the layers in the |owner| window. It starts
   // intercepting scroll events on |event_window|, and uses those events to
   // control the layer-slide. The lifetime of the slider is managed by the
   // lifetime of |owner|, i.e. if |owner| is destroyed, then the slider also
   // destroys itself.
   WindowSlider(Delegate* delegate,
                aura::Window* event_window,
                aura::Window* owner);
 
   virtual ~WindowSlider();
 
   // Changes the owner of the slider.
   void ChangeOwner(aura::Window* new_owner);
 
   bool IsSlideInProgress() const;
 
  private:
   // Sets up the slider layer correctly (sets the correct bounds of the layer,
   // parents it to the right layer, and sets up the correct stacking order).
   void SetupSliderLayer();
 
   void UpdateForScroll(float x_offset, float y_offset);
 
   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;
   virtual void OnMouseEvent(ui::MouseEvent* event) OVERRIDE;
   virtual void OnScrollEvent(ui::ScrollEvent* event) OVERRIDE;
   virtual void OnGestureEvent(ui::GestureEvent* event) OVERRIDE;
 
   // Overridden from aura::WindowObserver:
   virtual void OnWindowRemovingFromRootWindow(aura::Window* window) OVERRIDE;
 
   Delegate* delegate_;
 
   // The slider intercepts scroll events from this window. The slider does not
   // own |event_window_|. If |event_window_| is destroyed, then the slider stops
   // listening for events, but it doesn't destroy itself.
   aura::Window* event_window_;
 
   // The window the slider operates on. The lifetime of the slider is bound to
   // this window (i.e. if |owner_| does, the slider destroys itself). The slider
   // can also delete itself when a slide gesture is completed. This does not
   // 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_;
 
   // This keeps track of the layer created by the delegate.
   scoped_ptr<ui::Layer> slider_;
 
   // This manages the shadow for the layers.
   scoped_ptr<ShadowLayerDelegate> shadow_;
 
   base::WeakPtrFactory<WindowSlider> weak_factory_;
 
   float active_start_threshold_;
 
   const float start_threshold_touchscreen_;
   const float start_threshold_touchpad_;
   const float complete_threshold_;
 
   DISALLOW_COPY_AND_ASSIGN(WindowSlider);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_WEB_CONTENTS_AURA_WINDOW_SLIDER_H_