Refactor GestureNavigation to eliminate code redundancy

content/browser/web_contents/aura/overscroll_navigation_overlay.cc

 // Copyright 2014 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.
 
 #include "content/browser/web_contents/aura/overscroll_navigation_overlay.h"
 
 #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"
 #include "content/public/browser/render_widget_host_view.h"
 #include "ui/aura/window.h"
 #include "ui/aura_extra/image_window_delegate.h"
 #include "ui/base/layout.h"
 #include "ui/compositor/layer.h"
 #include "ui/compositor/layer_animation_observer.h"
 #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 {
 
 // Returns true if the entry's URL or any of the URLs in entry's redirect chain
 // match |url|.
 bool DoesEntryMatchURL(NavigationEntry* entry, const GURL& url) {
   if (!entry)
     return false;
   if (entry->GetURL() == url)
@@ skipping 91 matching lines @@
 
  private:
   ~OverlayDismissAnimator() override {}
 
   scoped_ptr<ui::Layer> layer_;
 
   DISALLOW_COPY_AND_ASSIGN(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();
-}
-
-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;
+  LOG(ERROR) << "URL: " << pending_entry_url_.GetContent();
+}
+
+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();
-  }
-}
-
-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_)
+    web_contents_window_->StackChildBelow(
+        window.get(), web_contents_->GetContentNativeView());
+  }
+  return scoped_ptr<OverscrollLayerWrapper>(
+      new OverscrollLayerWrapper(window.Pass()));
+}
+
+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();
+}
+
+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();
+}
+
+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 a slide is in progress, then do not destroy the window or the slide.
-  if (window_slider_.get() && window_slider_->IsSlideInProgress())
-    return;
-
-  // 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();
+  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();
-  }
-}
-
-ui::Layer* OverscrollNavigationOverlay::CreateSlideLayer(int offset) {
+  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.

+}
+
+const gfx::Image OverscrollNavigationOverlay::GetImageForDirection(
+    Direction direction) const {
   const NavigationControllerImpl& controller = web_contents_->GetController();
-  const NavigationEntryImpl* entry = controller.GetEntryAtOffset(offset);
-
-  gfx::Image image;
+  const NavigationEntryImpl* entry = NavigationEntryImpl::FromNavigationEntry(
+      controller.GetEntryAtOffset(direction == FORWARD ? 1 : -1));
+
+  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);
-  }
-  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;
-}
-
-ui::Layer* OverscrollNavigationOverlay::CreateBackLayer() {
+    return gfx::Image(image_reps);
+  }
+  return gfx::Image();
+}
+
+scoped_ptr<OverscrollLayerWrapper>
+OverscrollNavigationOverlay::CreateFrontLayerWrapper() {
+  LOG(ERROR) << "ONO: Create front layer";
+  if (!web_contents_->GetController().CanGoForward())
+    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();
+}
+
+scoped_ptr<OverscrollLayerWrapper>
+OverscrollNavigationOverlay::CreateBackLayerWrapper() {
+  LOG(ERROR) << "ONO: Create back layer";
   if (!web_contents_->GetController().CanGoBack())
-    return NULL;
-  slide_direction_ = SLIDE_BACK;
-  return CreateSlideLayer(-1);
-}
-
-ui::Layer* OverscrollNavigationOverlay::CreateFrontLayer() {
-  if (!web_contents_->GetController().CanGoForward())
-    return NULL;
-  slide_direction_ = SLIDE_FRONT;
-  return CreateSlideLayer(1);
-}
-
-void OverscrollNavigationOverlay::OnWindowSlideCompleting() {
-  if (slide_direction_ == SLIDE_UNKNOWN)
-    return;
-
-  // Perform the navigation.
-  if (slide_direction_ == SLIDE_BACK)
-    web_contents_->GetController().GoBack();
-  else if (slide_direction_ == SLIDE_FRONT)
+    return nullptr;
+  direction_ = BACK;
+  return CreateLayerWrapper();
+}
+
+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;
-  }
-
-  // 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();
+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()));
+    }
+  }
   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() ||
       DoesEntryMatchURL(visible_entry, pending_entry_url_)) {
     received_paint_update_ = true;
     StopObservingIfDone();
   }
 }
 
 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).
   loading_complete_ = true;
   StopObservingIfDone();
 }
 
 }  // namespace content