Refactor GestureNavigation to eliminate code redundancy

content/browser/web_contents/aura/overscroll_navigation_overlay.cc

Index: content/browser/web_contents/aura/overscroll_navigation_overlay.cc
diff --git a/content/browser/web_contents/aura/overscroll_navigation_overlay.cc b/content/browser/web_contents/aura/overscroll_navigation_overlay.cc
index a96b0d27ed66cd46912ebc7d55fdee0feb5e9f9c..50bf594a4846251446c259e40e336148248e2bde 100644
--- a/content/browser/web_contents/aura/overscroll_navigation_overlay.cc
+++ b/content/browser/web_contents/aura/overscroll_navigation_overlay.cc
@@ -6,6 +6,8 @@
 
 #include "content/browser/frame_host/navigation_entry_impl.h"
 #include "content/browser/renderer_host/render_view_host_impl.h"
+#include "content/browser/web_contents/aura/overscroll_layer_wrapper.h"
+#include "content/browser/web_contents/aura/overscroll_window_delegate.h"
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/common/view_messages.h"
 #include "content/public/browser/browser_thread.h"
@@ -18,7 +20,6 @@
 #include "ui/compositor/scoped_layer_animation_settings.h"
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/image/image_png_rep.h"
-#include "ui/gfx/image/image_skia.h"
 
 namespace content {
 namespace {
@@ -130,171 +131,201 @@ class OverlayDismissAnimator
 };
 
 OverscrollNavigationOverlay::OverscrollNavigationOverlay(
-    WebContentsImpl* web_contents)
-    : web_contents_(web_contents),
-      image_delegate_(NULL),
+    WebContentsImpl* web_contents,
+    aura::Window* web_contents_window)
+    : direction_(NONE),
+      web_contents_(web_contents),
+      window_(nullptr),
+      image_delegate_(nullptr),
       loading_complete_(false),
       received_paint_update_(false),
-      slide_direction_(SLIDE_UNKNOWN) {
+      owa_(new OverscrollWindowAnimation(this, nullptr)),
+      web_contents_window_(web_contents_window) {
 }
 
 OverscrollNavigationOverlay::~OverscrollNavigationOverlay() {
 }
 
 void OverscrollNavigationOverlay::StartObserving() {
+  LOG(ERROR) << "Starting to observe";
   loading_complete_ = false;
   received_paint_update_ = false;

[mfomitchev, 2015/03/10 19:25:59]

overlay_dismiss_layer_.reset();

[Nina, 2015/03/12 22:21:28]

Done.

-  overlay_dismiss_layer_.reset();
   Observe(web_contents_);
 
-  // Make sure the overlay window is on top.
-  if (window_.get() && window_->parent())
-    window_->parent()->StackChildAtTop(window_.get());
-
   // Assumes the navigation has been initiated.
   NavigationEntry* pending_entry =
       web_contents_->GetController().GetPendingEntry();
+
   // Save url of the pending entry to identify when it loads and paints later.
   // Under some circumstances navigation can leave a null pending entry -
   // see comments in NavigationControllerImpl::NavigateToPendingEntry().
   pending_entry_url_ = pending_entry ? pending_entry->GetURL() : GURL();
+  LOG(ERROR) << "URL: " << pending_entry_url_.GetContent();
 }
 
-void OverscrollNavigationOverlay::SetOverlayWindow(
-    scoped_ptr<aura::Window> window,
-    aura_extra::ImageWindowDelegate* delegate) {
-  window_ = window.Pass();
-  if (window_.get() && window_->parent())
-    window_->parent()->StackChildAtTop(window_.get());
-  image_delegate_ = delegate;
-
-  if (window_.get() && delegate->has_image()) {
-    window_slider_.reset(new WindowSlider(this,
-                                          window_->parent(),
-                                          window_.get()));
-    slide_direction_ = SLIDE_UNKNOWN;
+void OverscrollNavigationOverlay::StopObservingIfDone() {
+  LOG(ERROR) << "ONO: Stop observing if done";
+  if (!window_ ||
+      !(loading_complete_ || received_paint_update_) ||

[mfomitchev, 2015/03/10 19:25:59]

Can you please copy the comments for loading_complete_, received_paint_update_
from the old implementation?

[Nina, 2015/03/12 22:21:28]

Done.

+      owa_->is_active()) {
+    LOG(ERROR) << "Returning early";
+    return;
+  }
+  FadeOutOverscrollWindow();
+  Observe(NULL);
+  received_paint_update_ = false;
+  loading_complete_ = false;
+}
+
+scoped_ptr<OverscrollLayerWrapper>
+OverscrollNavigationOverlay::CreateLayerWrapper() {
+  LOG(ERROR) << "ONO: CreateLayerWrapper";
+  if (window_) {
+    LOG(ERROR) << "We have a window, returning slide layer";
+    return scoped_ptr<OverscrollLayerWrapper>(
+        new OverscrollLayerWrapper(CreateSlideLayer()));
+  }
+  scoped_ptr<aura::Window> window = CreateOverlayWindow();
+  if (direction_ == FORWARD) {

[mfomitchev, 2015/03/10 19:25:59]

It's not very logical IMO that stacking is done in CreateSlideLayer for the
layer while it's done right here in CreateLayerWrapper for the window. We should
be consistent about that. 

It might even make sense to get rid of CreateSlideLayer and CreateOverlayWindow
altogether and just do everything in CreateLayerWrapper.

[Nina, 2015/03/12 22:21:28]

Done.

+    web_contents_window_->StackChildAbove(
+        window.get(), web_contents_->GetContentNativeView());
   } else {
-    window_slider_.reset();
+    web_contents_window_->StackChildBelow(
+        window.get(), web_contents_->GetContentNativeView());
   }
+  return scoped_ptr<OverscrollLayerWrapper>(
+      new OverscrollLayerWrapper(window.Pass()));
 }
 
-void OverscrollNavigationOverlay::StopObservingIfDone() {
-  // Normally we dismiss the overlay once we receive a paint update, however
-  // for in-page navigations DidFirstVisuallyNonEmptyPaint() does not get
-  // called, and we rely on loading_complete_ for those cases.
-  if (!received_paint_update_ && !loading_complete_)
-    return;
+scoped_ptr<aura::Window> OverscrollNavigationOverlay::CreateOverlayWindow() {
+  LOG(ERROR) << "ONO: Setting overlay window";
+  image_delegate_ =
+      new OverscrollWindowDelegate(owa_.get(),
+                                   GetImageForDirection(direction_));
+  scoped_ptr<aura::Window> window(new aura::Window(image_delegate_));
+  window->SetTransparent(true);
+  window->Init(aura::WINDOW_LAYER_TEXTURED);
+  window->layer()->SetMasksToBounds(false);
+  window->SetName("OverscrollOverlay");
+  web_contents_window_->AddChild(window.get());
+  window->Show();
+  owa_->set_overlay_window(window.get());

[mfomitchev, 2015/03/10 19:25:59]

It would be better to do this in OnOverscrollCompleted() - this is where we
transition from the overscroll state to the slider state, so I think this is
where it would make sense to set up that up.

[Nina, 2015/03/12 22:21:28]

That function was deleted so now we handle it differently.

+  return window.Pass();
+}
 
-  // If a slide is in progress, then do not destroy the window or the slide.
-  if (window_slider_.get() && window_slider_->IsSlideInProgress())
-    return;
+scoped_ptr<ui::Layer> OverscrollNavigationOverlay::CreateSlideLayer() {
+  LOG(ERROR) << "ONO: Creating slide layer";
+  if (!layer_delegate_)
+    layer_delegate_.reset(new ImageLayerDelegate());
+  layer_delegate_->SetImage(GetImageForDirection(direction_));
+  scoped_ptr<ui::Layer> layer(new ui::Layer(ui::LAYER_TEXTURED));
+  layer->set_delegate(layer_delegate_.get());
+  ui::Layer* parent = window_->layer()->parent();
+  parent->Add(layer.get());
+  if (direction_ == FORWARD)
+    parent->StackAbove(layer.get(), window_->layer());
+  else
+    parent->StackBelow(layer.get(), window_->layer());
+  gfx::Rect bounds = gfx::Rect(layer->parent()->bounds().size());
+  layer->SetBounds(bounds);
+  return layer.Pass();
+}
 
-  // The layer to be animated by OverlayDismissAnimator
-  scoped_ptr<ui::Layer> overlay_dismiss_layer;
-  if (overlay_dismiss_layer_)
-    overlay_dismiss_layer = overlay_dismiss_layer_.Pass();
-  else if (window_.get())
-    overlay_dismiss_layer = window_->AcquireLayer();
-  Observe(NULL);
-  window_slider_.reset();
+void OverscrollNavigationOverlay::FadeOutOverscrollWindow() {
+  LOG(ERROR) << "ONO: FadeOutOverscrollWindow";
+  if (!dismiss_layer_ && !window_)
+    return;

[mfomitchev, 2015/03/10 19:25:59]

I don't think you can get here if !window_

[Nina, 2015/03/12 22:21:28]

Right!

+  if (!dismiss_layer_)
+    dismiss_layer_ = window_->AcquireLayer();
   window_.reset();

[mfomitchev, 2015/03/10 19:25:59]

We should probably also reset the overlay window on OWA.
set_overlay_window(NULL)? It would probably also make sense to do
set_overlay_window(NULL) and window_.reset() outside of
FadeOutOverscrollWindow() - perhaps just after calling it. 

[Nina, 2015/03/12 22:21:28]

Done.

-  image_delegate_ = NULL;
-  if (overlay_dismiss_layer.get()) {
-    // OverlayDismissAnimator deletes overlay_dismiss_layer and itself when the
-    // animation completes.
-    (new OverlayDismissAnimator(overlay_dismiss_layer.Pass()))->Animate();
+  aura::Window* contents = web_contents_->GetContentNativeView();
+  contents->layer()->SetLayerBrightness(1.f);
+  {
+    ui::ScopedLayerAnimationSettings settings(contents->layer()->GetAnimator());
+    settings.SetPreemptionStrategy(
+        ui::LayerAnimator::IMMEDIATELY_ANIMATE_TO_NEW_TARGET);
+    settings.SetTweenType(gfx::Tween::EASE_OUT);
+    contents->layer()->SetLayerBrightness(0.f);

[mfomitchev, 2015/03/10 19:25:59]

Why are we fiddling with the brightness of web_contents_ layer here? If we get
rid of this, we probably don't really need FadeOutOverscrollWindow()?

[Nina, 2015/03/12 22:21:28]

Got rid of this.

   }
+  (new OverlayDismissAnimator(dismiss_layer_.Pass()))->Animate();

[mfomitchev, 2015/03/10 19:25:59]

keep comment

[Nina, 2015/03/12 22:21:28]

Done.

 }
 
-ui::Layer* OverscrollNavigationOverlay::CreateSlideLayer(int offset) {
+const gfx::Image OverscrollNavigationOverlay::GetImageForDirection(
+    Direction direction) const {
   const NavigationControllerImpl& controller = web_contents_->GetController();
-  const NavigationEntryImpl* entry = controller.GetEntryAtOffset(offset);
+  const NavigationEntryImpl* entry = NavigationEntryImpl::FromNavigationEntry(
+      controller.GetEntryAtOffset(direction == FORWARD ? 1 : -1));
 
-  gfx::Image image;
+  LOG(ERROR) << "ONO: Image for entry - " << entry->GetTitle();
   if (entry && entry->screenshot().get()) {
     std::vector<gfx::ImagePNGRep> image_reps;
     image_reps.push_back(gfx::ImagePNGRep(entry->screenshot(), 1.0f));
-    image = gfx::Image(image_reps);
+    return gfx::Image(image_reps);
   }
-  if (!layer_delegate_)
-    layer_delegate_.reset(new ImageLayerDelegate());
-  layer_delegate_->SetImage(image);
-
-  ui::Layer* layer = new ui::Layer(ui::LAYER_TEXTURED);
-  layer->set_delegate(layer_delegate_.get());
-  return layer;
+  return gfx::Image();
 }
 
-ui::Layer* OverscrollNavigationOverlay::CreateBackLayer() {
-  if (!web_contents_->GetController().CanGoBack())
-    return NULL;
-  slide_direction_ = SLIDE_BACK;
-  return CreateSlideLayer(-1);
-}
-
-ui::Layer* OverscrollNavigationOverlay::CreateFrontLayer() {
+scoped_ptr<OverscrollLayerWrapper>
+OverscrollNavigationOverlay::CreateFrontLayerWrapper() {
+  LOG(ERROR) << "ONO: Create front layer";
   if (!web_contents_->GetController().CanGoForward())
-    return NULL;
-  slide_direction_ = SLIDE_FRONT;
-  return CreateSlideLayer(1);
+    return nullptr;

[mfomitchev, 2015/03/10 19:25:59]

Not sure if the convention is to return a scoped_ptr wrapping nullptr in these
cases instead - can you check how it's done in similar cases?

[Nina, 2015/03/12 22:21:28]

I don't know what the convention usually is, but I think in general if the
constructor is not annotated by the explicit keyword, you can use the implicit
version. Also, we are still returning a scoped_ptr.
Adding explicit to the scoped_ptr constructor and trying to compile yielded some
results where this idiom is used:

https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/discar...

https://code.google.com/p/chromium/codesearch#chromium/src/ipc/mojo/ipc_chann...

https://code.google.com/p/chromium/codesearch#chromium/src/net/disk_cache/mem...

https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/geometry/r_...

https://code.google.com/p/chromium/codesearch#chromium/src/net/base/sdch_mana...

+  direction_ = FORWARD;
+  return CreateLayerWrapper();
 }
 
-void OverscrollNavigationOverlay::OnWindowSlideCompleting() {
-  if (slide_direction_ == SLIDE_UNKNOWN)
-    return;
+scoped_ptr<OverscrollLayerWrapper>
+OverscrollNavigationOverlay::CreateBackLayerWrapper() {
+  LOG(ERROR) << "ONO: Create back layer";
+  if (!web_contents_->GetController().CanGoBack())
+    return nullptr;
+  direction_ = BACK;
+  return CreateLayerWrapper();
+}
 
-  // Perform the navigation.
-  if (slide_direction_ == SLIDE_BACK)
-    web_contents_->GetController().GoBack();
-  else if (slide_direction_ == SLIDE_FRONT)
+void OverscrollNavigationOverlay::OnOverscrollCompleting() {
+  // We start the navigation as soon as we know the overscroll gesture is
+  // completing.
+  LOG(ERROR) << "ONO: Starting navigation";
+  if (direction_ == FORWARD)
     web_contents_->GetController().GoForward();
   else
-    NOTREACHED();
-
-  // Reset state and wait for the new navigation page to complete
-  // loading/painting.
+    web_contents_->GetController().GoBack();

[mfomitchev, 2015/03/10 19:25:59]

NOTREACHED() for direction == NONE

[Nina, 2015/03/12 22:21:28]

Done.

   StartObserving();
 }
 
-void OverscrollNavigationOverlay::OnWindowSlideCompleted(
-    scoped_ptr<ui::Layer> layer) {
-  if (slide_direction_ == SLIDE_UNKNOWN) {
-    window_slider_.reset();
-    StopObservingIfDone();
-    return;
+void OverscrollNavigationOverlay::OnOverscrollCompleted(
+    scoped_ptr<OverscrollLayerWrapper> layer_wrapper) {
+  LOG(ERROR) << "ONO: OnOverscrollCompleted";
+  if (layer_wrapper->has_window()) {

[mfomitchev, 2015/03/10 19:26:00]

I think checking if (!window_) would be better to determine the state. And then
do DCHECK(layer_wrapper->has_window())

[Nina, 2015/03/12 22:21:29]

Done, but I removed has_window()

+    LOG(ERROR) << "The layer wrapper has a window";
+    window_ = layer_wrapper->AcquireWindow();
+    // Make sure the overlay window is on top.
+    web_contents_window_->StackChildAtTop(window_.get());
+    // Reset the position of the contents window.
+    web_contents_->GetContentNativeView()->SetTransform(gfx::Transform());
+  } else {
+    LOG(ERROR) << "The layer wrapper has a layer";
+    dismiss_layer_ = layer_wrapper->AcquireLayer();

[mfomitchev, 2015/03/10 19:25:59]

Move this to after schedulepaint and add back the comment

[Nina, 2015/03/12 22:21:28]

Done.

+    // If this overscroll was started too quickly, there is a chance we've

[mfomitchev, 2015/03/10 19:25:59]

Huh? We shouldn't be dismissing the window while OWA is active, and it just
ceases to be active when it calls OnOverscrollCompleted, so it seems window_
should always be true here.. (if so - DCHECK)

[Nina, 2015/03/12 22:21:29]

Removed.

+    // already dismissed the window_;
+    if (window_) {
+      // Change the image used for the overlay window.
+      image_delegate_->SetImage(layer_delegate_->image());
+      // Reset the position of its layer.
+      window_->layer()->SetTransform(gfx::Transform());
+      window_->SchedulePaintInRect(gfx::Rect(window_->bounds().size()));
+    }
   }
-
-  // Change the image used for the overlay window.
-  image_delegate_->SetImage(layer_delegate_->image());
-  window_->layer()->SetTransform(gfx::Transform());
-  window_->SchedulePaintInRect(gfx::Rect(window_->bounds().size()));
-  slide_direction_ = SLIDE_UNKNOWN;
-  // We may end up dismissing the overlay before it has a chance to repaint, so
-  // set the slider layer to be the one animated by OverlayDismissAnimator.
-  if (layer.get())
-    overlay_dismiss_layer_ = layer.Pass();
   StopObservingIfDone();

[mfomitchev, 2015/03/10 19:25:59]

We should be resetting direction_ to NONE when the animation completes. So here
and in OnOverscrollAborted().

[Nina, 2015/03/12 22:21:28]

Done.

 }
 
-void OverscrollNavigationOverlay::OnWindowSlideAborted() {
+void OverscrollNavigationOverlay::OnOverscrollAborted() {
   StopObservingIfDone();
 }
 
-void OverscrollNavigationOverlay::OnWindowSliderDestroyed() {
-  // We only want to take an action here if WindowSlider is being destroyed
-  // outside of OverscrollNavigationOverlay. If window_slider_.get() is NULL,
-  // then OverscrollNavigationOverlay is the one destroying WindowSlider, and
-  // we don't need to do anything.
-  // This check prevents StopObservingIfDone() being called multiple times
-  // (including recursively) for a single event.
-  if (window_slider_.get()) {
-    // The slider has just been destroyed. Release the ownership.
-    ignore_result(window_slider_.release());
-    StopObservingIfDone();
-  }
-}
-
 void OverscrollNavigationOverlay::DidFirstVisuallyNonEmptyPaint() {
+  LOG(ERROR) << "Did first visually non empty paint";
   NavigationEntry* visible_entry =
       web_contents_->GetController().GetVisibleEntry();
   if (pending_entry_url_.is_empty() ||
@@ -305,6 +336,7 @@ void OverscrollNavigationOverlay::DidFirstVisuallyNonEmptyPaint() {
 }
 
 void OverscrollNavigationOverlay::DidStopLoading(RenderViewHost* host) {
+  LOG(ERROR) << "Did stop loading";
   // Don't compare URLs in this case - it's possible they won't match if
   // a gesture-nav initiated navigation was interrupted by some other in-site
   // navigation ((e.g., from a script, or from a bookmark).