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..8a17a2aeeb2bdfe132e4426afab7d2febb553830 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 {
@@ -43,6 +44,7 @@ bool DoesEntryMatchURL(NavigationEntry* entry, const GURL& url) {
 }  // namespace
 
 // A LayerDelegate that paints an image for the layer.
+// TODO move to overscroll_window.h???
 class ImageLayerDelegate : public ui::LayerDelegate {
  public:
   ImageLayerDelegate() {}
@@ -130,181 +132,211 @@ class OverlayDismissAnimator
 };
 
 OverscrollNavigationOverlay::OverscrollNavigationOverlay(
-    WebContentsImpl* web_contents)
-    : web_contents_(web_contents),
-      image_delegate_(NULL),
+    WebContentsImpl* web_contents,
+    OverscrollWindowAnimation* owa,
+    aura::Window* web_contents_window)
+    : direction_(OverscrollWindowAnimation::NONE),
+      web_contents_(web_contents),
+      window_(nullptr),
       loading_complete_(false),
-      received_paint_update_(false),
-      slide_direction_(SLIDE_UNKNOWN) {
+      observing_(false),
+      owa_(owa),
+      web_contents_window_(web_contents_window) {
 }
 
 OverscrollNavigationOverlay::~OverscrollNavigationOverlay() {
 }
 
 void OverscrollNavigationOverlay::StartObserving() {
+  LOG(ERROR) << "Starting to observe";
   loading_complete_ = false;
-  received_paint_update_ = false;
-  overlay_dismiss_layer_.reset();
+  observing_ = true;
   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;
-  } 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_)
-    return;
-
-  // 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();
-  window_.reset();
-  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();
-  }
+void OverscrollNavigationOverlay::SetupOverlayWindow() {
+  LOG(ERROR) << "ONO: Setting overlay window";
+  window_.reset(new aura::Window(
+      new OverscrollWindowDelegate(owa_, GetImageForDirection(direction_))));
+  window_->SetType(ui::wm::WINDOW_TYPE_CONTROL);
+  window_->SetTransparent(true);
+  window_->Init(aura::WINDOW_LAYER_TEXTURED);
+  window_->layer()->SetMasksToBounds(false);
+  window_->SetName("OverscrollOverlay");
+  window_->layer()->SetLayerBrightness(-0.1f);

[mfomitchev, 2015/03/06 01:36:43]

It doesn't seem like we are animating the brightness anymore, so I think we can
just get rid of this

[Nina, 2015/03/09 15:54:52]

Done.

+  web_contents_window_->AddChild(window_.get());
+  window_->Show();
 }
 
-ui::Layer* OverscrollNavigationOverlay::CreateSlideLayer(int offset) {
+const gfx::Image OverscrollNavigationOverlay::GetImageForDirection(
+    OverscrollWindowAnimation::Direction direction) const {
   const NavigationControllerImpl& controller = web_contents_->GetController();
-  const NavigationEntryImpl* entry = controller.GetEntryAtOffset(offset);
+  const NavigationEntryImpl* entry = NavigationEntryImpl::FromNavigationEntry(
+      controller.GetEntryAtOffset(
+          direction == OverscrollWindowAnimation::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);
   }
+  return gfx::Image();
+}
+
+scoped_ptr<ui::Layer> OverscrollNavigationOverlay::CreateSlideLayer() {
+  LOG(ERROR) << "ONO: Creating slide layer";
   if (!layer_delegate_)
     layer_delegate_.reset(new ImageLayerDelegate());
-  layer_delegate_->SetImage(image);
-
-  ui::Layer* layer = new ui::Layer(ui::LAYER_TEXTURED);
+  layer_delegate_->SetImage(GetImageForDirection(direction_));
+  scoped_ptr<ui::Layer> layer(new ui::Layer(ui::LAYER_TEXTURED));
   layer->set_delegate(layer_delegate_.get());
-  return layer;
+  ui::Layer* parent = window_->layer()->parent();
+  parent->Add(layer.get());
+  if (direction_ == OverscrollWindowAnimation::FORWARD)

[mfomitchev, 2015/03/05 23:37:06]

I think somewhere here we should have LTR/RTL logic?

[Nina, 2015/03/09 15:54:52]

FORWARD and BACKWARD refer to history direction, not screen direction, so RTL
should have already been taken care of where this is set. Regardless of
direction, if we're going FORWARD we want to stack above the existing layer, and
vice versa.

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

Ok, cool

+    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();
 }
 
-ui::Layer* OverscrollNavigationOverlay::CreateBackLayer() {
-  if (!web_contents_->GetController().CanGoBack())
-    return NULL;
-  slide_direction_ = SLIDE_BACK;
-  return CreateSlideLayer(-1);
+scoped_ptr<OverscrollLayerWrapper> OverscrollNavigationOverlay::CreateLayer() {

[mfomitchev, 2015/03/05 23:37:05]

Method needs to be renamed - we are creating a wrapper (around window or layer),
not a layer

[Nina, 2015/03/09 15:54:52]

Agreed, had a TODO on the .h file.

+  LOG(ERROR) << "ONO: CreateLayer";
+  if (window_) {
+    LOG(ERROR) << "We have a window, returning slide layer";
+    return scoped_ptr<OverscrollLayerWrapper>(
+        new OverscrollLayerWrapper(CreateSlideLayer()));
+  }
+  SetupOverlayWindow();

[mfomitchev, 2015/03/05 23:37:06]

SetupOverlayWindow() should not inititalize window_. At this point ONO doesn't
own the overlay window - it is creating it for OWA, so it shouldn't set window_
to the overlay window, even intermittently for the duration of CreateLayer() -
it just makes the state/ownership confusing. window_ should only be set to the
overlay window in the "slide state".

[Nina, 2015/03/09 15:54:52]

Done.

+  if (direction_ == OverscrollWindowAnimation::FORWARD) {
+    web_contents_window_->StackChildAbove(
+        window_.get(), web_contents_->GetContentNativeView());
+  } else {
+    web_contents_window_->StackChildBelow(
+        window_.get(), web_contents_->GetContentNativeView());
+  }
+  return scoped_ptr<OverscrollLayerWrapper>(
+      new OverscrollLayerWrapper(window_.Pass()));
 }
 
-ui::Layer* OverscrollNavigationOverlay::CreateFrontLayer() {
+scoped_ptr<OverscrollLayerWrapper>
+OverscrollNavigationOverlay::CreateFrontLayer() {
+  LOG(ERROR) << "ONO: Create front layer";
   if (!web_contents_->GetController().CanGoForward())
-    return NULL;
-  slide_direction_ = SLIDE_FRONT;
-  return CreateSlideLayer(1);
+    return nullptr;
+  direction_ = OverscrollWindowAnimation::FORWARD;
+  return CreateLayer();
+}
+
+scoped_ptr<OverscrollLayerWrapper>
+OverscrollNavigationOverlay::CreateBackLayer() {
+  LOG(ERROR) << "ONO: Create back layer";
+  if (!web_contents_->GetController().CanGoBack())
+    return nullptr;
+  direction_ = OverscrollWindowAnimation::BACKWARD;
+  return CreateLayer();
 }
 
-void OverscrollNavigationOverlay::OnWindowSlideCompleting() {
-  if (slide_direction_ == SLIDE_UNKNOWN)
+void OverscrollNavigationOverlay::OnOverscrollCompleted(
+    scoped_ptr<OverscrollLayerWrapper> layer_wrapper) {
+  LOG(ERROR) << "ONO: OnOverscrollCompleted";
+
+  // The overscroll gesture has been cancelled.
+  if (!observing_ && !loading_complete_) {

[mfomitchev, 2015/03/05 23:37:05]

This seems like the only place we use observing_. So if we add a separate
callback for cancelled overscroll - we should be able to get rid of observing_.

[Nina, 2015/03/09 15:54:52]

Done.

+    LOG(ERROR) << "Not observing, the gesture has been cancelled";
+    LOG(ERROR) << "Observing:        " << observing_;
+    LOG(ERROR) << "Loading complete: " << loading_complete_;
+    window_.reset();
     return;
+  }
 
-  // Perform the navigation.
-  if (slide_direction_ == SLIDE_BACK)
-    web_contents_->GetController().GoBack();
-  else if (slide_direction_ == SLIDE_FRONT)
-    web_contents_->GetController().GoForward();
+  // TODO if the first iteration captures a dismiss layer and the second a
+  // window_, this code might not work.
+  if (layer_wrapper->has_window())
+    window_ = layer_wrapper->AcquireWindow();
   else
-    NOTREACHED();
+    dismiss_layer_ = layer_wrapper->AcquireLayer();
 
-  // Reset state and wait for the new navigation page to complete
-  // loading/painting.
-  StartObserving();
-}
+  // Make sure the overlay window is on top.
+  web_contents_window_->StackChildAtTop(window_.get());
+  //dismiss_layer_ = layer_wrapper->AcquireLayer();
+  owa_->set_live_window(window_.get());
 
-void OverscrollNavigationOverlay::OnWindowSlideCompleted(
-    scoped_ptr<ui::Layer> layer) {
-  if (slide_direction_ == SLIDE_UNKNOWN) {
-    window_slider_.reset();
-    StopObservingIfDone();
-    return;
-  }
+  // Reset the position of the contents window.
+  web_contents_->GetContentNativeView()->SetTransform(gfx::Transform());

[mfomitchev, 2015/03/06 01:36:43]

These three instructions should only be done for the overlay case
(layer_wrapper->has_window()), but not the slider case. Take a look at the old
OnWindowSlideCompleted() implementation to see what should be done for the
slider case

[Nina, 2015/03/09 15:54:52]

Done.
Agreed. This was not working at all.

 
-  // 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();
 }
 
-void OverscrollNavigationOverlay::OnWindowSlideAborted() {
-  StopObservingIfDone();
+void OverscrollNavigationOverlay::OnOverscrollCompleting() {
+  // We start the navigation as soon as we know the overscroll gesture is
+  // completing.
+  LOG(ERROR) << "ONO: Starting navigation";
+  if (direction_ == OverscrollWindowAnimation::FORWARD)
+    web_contents_->GetController().GoForward();
+  else
+    web_contents_->GetController().GoBack();
+  StartObserving();
 }
 
-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::FadeOutOverscrollWindow() {
+  LOG(ERROR) << "ONO: FadeOutOverscrollWindow";
+  // TODO maybe we do not need this check.
+  if (!dismiss_layer_ && !window_)
+    return;
+  if (!dismiss_layer_)
+    dismiss_layer_ = window_->AcquireLayer();
+  window_.reset();
+  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);
+  }
+  (new OverlayDismissAnimator(dismiss_layer_.Pass()))->Animate();
+}
+
+void OverscrollNavigationOverlay::StopObservingIfDone() {

[mfomitchev, 2015/03/06 01:36:43]

Order of the methods should be the same as in .h file. It's also best not to
change the order unless there's a good reason - makes it easier to see the diff
when reviewing.

[Nina, 2015/03/09 15:54:52]

Done. I also rearranged some of the methods in a way that IMHO makes more sense.

+  LOG(ERROR) << "ONO: Stop observing if done";
+  if (!loading_complete_ || owa_->is_active()) {
+    LOG(ERROR) << "Returning early";
+    return;
   }
+  FadeOutOverscrollWindow();
+  Observe(NULL);
+  observing_ = false;
+  loading_complete_ = false;
+  owa_->set_live_window(web_contents_window_);

[mfomitchev, 2015/03/06 01:36:43]

It's pretty unfortunate that ONO has to do this - it should have no business
setting up the relationship between web_contents_window_ and owa. Unfortunately
I don't see a good way for WCVA to do this.
Having two separate instances of OWA would solve this though. Is there any good
reason not to do it?

[Nina, 2015/03/09 15:54:52]

Yes
No. Done!

 }
 
 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;
+    loading_complete_ = 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).