Refactor GestureNavigation to eliminate code redundancy

content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc

Index: content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc
diff --git a/content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc b/content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc
index bd9ab089c1c2f788b0bd666b99a807d112fd6dd5..8861d11dd1c101f6a8b79fb5a02a9b254c348c0c 100644
--- a/content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc
+++ b/content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc
@@ -4,6 +4,7 @@
 
 #include "content/browser/web_contents/aura/overscroll_navigation_overlay.h"
 
+#include <vector>
 #include "content/browser/frame_host/navigation_entry_impl.h"
 #include "content/browser/web_contents/web_contents_view.h"
 #include "content/common/frame_messages.h"
@@ -15,13 +16,57 @@
 #include "ui/aura/test/test_windows.h"
 #include "ui/aura/window.h"
 #include "ui/aura_extra/image_window_delegate.h"
+#include "ui/compositor/scoped_animation_duration_scale_mode.h"
+#include "ui/compositor/scoped_layer_animation_settings.h"
+#include "ui/compositor/test/layer_animator_test_controller.h"
+#include "ui/events/test/event_generator.h"
 #include "ui/gfx/codec/png_codec.h"
+#include "ui/gfx/frame_time.h"
 
 namespace content {
 
+// A subclass of TestWebContents that offers a fake content window.
+class OverscrollTestWebContents : public TestWebContents {

[mfomitchev, 2015/03/27 00:14:05]

Why not just add a SetContentsNativeView method to TestWebContents?

[Nina, 2015/03/27 17:52:36]

This was a good suggestion, but I had to add another modification to it, and I
would rather have them isolated for this test only.

[mfomitchev, 2015/03/31 22:16:31]

What is the reason we need to set the "fake" native view? Is it something unique
to our tests or an issue with the default behavior that could cause problems to
other tests as well? If the latter - why not also add SetNativeView to
TestWebContents?

[Nina, 2015/04/01 18:10:00]

It's unique to our case AFAICT. We need a window with a parent (for the clipping
code), and the test infrastructure provides a window without a parent. I can't
find a subclass of TestWebContents in CS, so I assume other tests do not need
it.

[mfomitchev, 2015/04/02 22:50:36]

Gotcha.

+ public:
+  ~OverscrollTestWebContents() override {}
+
+  static OverscrollTestWebContents* Create(
+      BrowserContext* browser_context,
+      SiteInstance* instance,
+      scoped_ptr<aura::Window> fake_contents_window) {
+    OverscrollTestWebContents*  web_contents =
+        new OverscrollTestWebContents(browser_context,
+                                      fake_contents_window.Pass());
+    web_contents->Init(WebContents::CreateParams(browser_context, instance));
+    web_contents->RenderFrameCreated(web_contents->GetMainFrame());
+    return web_contents;
+  }
+
+  gfx::NativeView GetContentNativeView() override {
+    return fake_contents_window_.get();
+  }
+
+ protected:
+  explicit OverscrollTestWebContents(
+      BrowserContext* browser_context,
+      scoped_ptr<aura::Window> fake_contents_window)
+      : TestWebContents(browser_context),
+        fake_contents_window_(fake_contents_window.Pass()) {
+  }
+
+ private:
+  scoped_ptr<aura::Window> fake_contents_window_;
+};
+
 class OverscrollNavigationOverlayTest : public RenderViewHostImplTestHarness {
  public:
-  OverscrollNavigationOverlayTest() {}
+  OverscrollNavigationOverlayTest()
+      : first_("https://www.google.com"),
+        second_("http://www.chromium.org"),
+        third_("https://www.kernel.org/"),
+        fourth_("https://github.com/") {
+  }
+
   ~OverscrollNavigationOverlayTest() override {}
 
   gfx::Image CreateDummyScreenshot() {
@@ -51,29 +96,52 @@ class OverscrollNavigationOverlayTest : public RenderViewHostImplTestHarness {
   }
 
   void PerformBackNavigationViaSliderCallbacks() {
-    // Sets slide direction to SLIDE_BACK, sets screenshot from NavEntry at
+    // Sets slide direction to BACK, sets screenshot from NavEntry at
     // offset -1  on layer_delegate_.
-    delete GetOverlay()->CreateBackLayer();
+    scoped_ptr<aura::Window> window(GetOverlay()->CreateBackWindow());
     // Performs BACK navigation, sets image from layer_delegate_ on
     // image_delegate_.
-    GetOverlay()->OnWindowSlideCompleting();
-    GetOverlay()->OnWindowSlideCompleted(scoped_ptr<ui::Layer>());
+    GetOverlay()->OnOverscrollCompleting();
+    GetOverlay()->OnOverscrollCompleted(window.Pass());
   }
 
+  // Tests URLs.
+  const GURL first_;
+  const GURL second_;
+  const GURL third_;
+  const GURL fourth_;
+
  protected:
   // RenderViewHostImplTestHarness:
   void SetUp() override {
     RenderViewHostImplTestHarness::SetUp();
 
-    const GURL first("https://www.google.com");
-    contents()->NavigateAndCommit(first);
+    // Set up the fake contents window.
+    scoped_ptr<aura::Window> fake_contents_window(new aura::Window(nullptr));
+    fake_contents_window->Init(aura::WINDOW_LAYER_SOLID_COLOR);
+    root_window()->AddChild(fake_contents_window.get());
+    fake_contents_window->SetBounds(gfx::Rect(root_window()->bounds().size()));
+
+    // Replace the default test web contents with our custom class.
+    SetContents(OverscrollTestWebContents::Create(
+        browser_context(),
+        SiteInstance::Create(browser_context()),
+        fake_contents_window.Pass()));
+
+    contents()->NavigateAndCommit(first_);
     EXPECT_TRUE(controller().GetVisibleEntry());
     EXPECT_FALSE(controller().CanGoBack());
 
-    const GURL second("http://www.chromium.org");
-    contents()->NavigateAndCommit(second);
+    contents()->NavigateAndCommit(second_);
+    EXPECT_TRUE(controller().CanGoBack());
+
+    contents()->NavigateAndCommit(third_);
     EXPECT_TRUE(controller().CanGoBack());
 
+    contents()->NavigateAndCommit(fourth_);
+    EXPECT_TRUE(controller().CanGoBack());
+    EXPECT_FALSE(controller().CanGoForward());
+
     // Receive a paint update. This is necessary to make sure the size is set
     // correctly in RenderWidgetHostImpl.
     ViewHostMsg_UpdateRect_Params params;
@@ -86,22 +154,8 @@ class OverscrollNavigationOverlayTest : public RenderViewHostImplTestHarness {
     test_rvh()->ResetSizeAndRepaintPendingFlags();
 
     // Create the overlay, and set the contents of the overlay window.
-    overlay_.reset(new OverscrollNavigationOverlay(contents()));
-    aura_extra::ImageWindowDelegate* image_delegate =
-        new aura_extra::ImageWindowDelegate();
-    scoped_ptr<aura::Window> overlay_window(
-      aura::test::CreateTestWindowWithDelegate(
-          image_delegate,
-          0,
-          gfx::Rect(root_window()->bounds().size()),
-          root_window()));
-
-    overlay_->SetOverlayWindow(overlay_window.Pass(), image_delegate);
-    overlay_->StartObserving();
-
-    EXPECT_TRUE(overlay_->web_contents());
-    EXPECT_FALSE(overlay_->loading_complete_);
-    EXPECT_FALSE(overlay_->received_paint_update_);
+    overlay_.reset(new OverscrollNavigationOverlay(
+        contents(), root_window()));
   }
 
   void TearDown() override {
@@ -119,67 +173,121 @@ class OverscrollNavigationOverlayTest : public RenderViewHostImplTestHarness {
   DISALLOW_COPY_AND_ASSIGN(OverscrollNavigationOverlayTest);
 };
 
-TEST_F(OverscrollNavigationOverlayTest, FirstVisuallyNonEmptyPaint_NoImage) {
-  ReceivePaintUpdate();
-  EXPECT_TRUE(GetOverlay()->received_paint_update_);
-  EXPECT_FALSE(GetOverlay()->loading_complete_);
-  // The paint update will hide the overlay.
-  EXPECT_FALSE(GetOverlay()->web_contents());
+// Tests that if a screenshot is available, it is set in the overlay window
+// delegate.
+TEST_F(OverscrollNavigationOverlayTest, WithScreenshot) {
+  SetDummyScreenshotOnNavEntry(controller().GetEntryAtOffset(-1));
+  PerformBackNavigationViaSliderCallbacks();
+  // Screenshot was set on NavEntry at offset -1.
+  EXPECT_TRUE(static_cast<aura_extra::ImageWindowDelegate*>(
+                  GetOverlay()->window_->delegate())->has_image());

[mfomitchev, 2015/03/27 00:14:05]

Verify contents()->GetURL()?

[Nina, 2015/03/27 17:52:36]

I don't think we need to re-test this (we do it later), as the point of the test
is to make sure ONO works correctly with / without screenshot.

[mfomitchev, 2015/03/31 22:16:31]

Ok, well, maybe put the check for cross_navigation_pending() after Completing()
is called into PerformBackNavigationViaSliderCallbacks(). This is the first,
most basic test, and requesting navigation in response to the callback is part
of ONO's most basic functionality. So it seems like that should be tested here.

[Nina, 2015/04/01 18:10:00]

Done.
So are you OK with testing cross_navigation_pending() in
PerformBackBavigationViaSliderCallbacks()?

+}
+
+// Tests that if a screenshot is not available, no image is set in the overlay
+// window delegate.
+TEST_F(OverscrollNavigationOverlayTest, WithoutScreenshot) {
+  PerformBackNavigationViaSliderCallbacks();
+  // No screenshot was set on NavEntry at offset -1.
+  EXPECT_FALSE(static_cast<aura_extra::ImageWindowDelegate*>(
+                   GetOverlay()->window_->delegate())->has_image());

[mfomitchev, 2015/03/27 00:14:05]

Verify contents()->GetURL()?

[Nina, 2015/03/27 17:52:36]

Ditto

[mfomitchev, 2015/03/31 22:16:31]

same as above

+}
+
+// Tests that if a navigation is attempted but there is nothing to navigate to,
+// we return a null window.
+TEST_F(OverscrollNavigationOverlayTest, CannotNavigate) {
+  EXPECT_EQ(GetOverlay()->CreateFrontWindow(), nullptr);

[mfomitchev, 2015/03/27 00:14:05]

Verify contents()->GetURL()?

[Nina, 2015/03/27 17:52:36]

Ditto. And in this case we are not navigating.

+}
+
+// Tests that if a navigation is aborted, no navigation is performed and the
+// state is restored.
+TEST_F(OverscrollNavigationOverlayTest, AbortNavigation) {
+  scoped_ptr<aura::Window> window = GetOverlay()->CreateBackWindow();
+  EXPECT_NE(GetOverlay()->direction_, OverscrollNavigationOverlay::NONE);
+
+  GetOverlay()->OnOverscrollAborted();
+  // Make sure that if we started a navigation (we shouldn't), we commit it.
+  EXPECT_FALSE(contents()->cross_navigation_pending());

[mfomitchev, 2015/03/27 00:14:05]

Verify contents()->GetURL()?

[Nina, 2015/03/27 17:52:36]

I really don't think we need to test this - it means we have to commit the
navigation in the test side, when cross_navigation_pending() should do the
trick.

[mfomitchev, 2015/03/31 22:16:31]

Acknowledged.

+}
+
+// Tests that if a second navigation is aborted, the first one still happens.
+TEST_F(OverscrollNavigationOverlayTest, AbortAfterSuccessfulNavigation) {
+  PerformBackNavigationViaSliderCallbacks();
+  scoped_ptr<aura::Window> wrapper = GetOverlay()->CreateBackWindow();
+  EXPECT_NE(GetOverlay()->direction_, OverscrollNavigationOverlay::NONE);
+
+  GetOverlay()->OnOverscrollAborted();
+  EXPECT_EQ(GetOverlay()->direction_, OverscrollNavigationOverlay::NONE);
+
+  EXPECT_TRUE(contents()->cross_navigation_pending());
+  contents()->CommitPendingNavigation();
+  EXPECT_EQ(contents()->GetURL(), third_);
 }
 
-TEST_F(OverscrollNavigationOverlayTest, FirstVisuallyNonEmptyPaint_WithImage) {
-  GetOverlay()->image_delegate_->SetImage(CreateDummyScreenshot());
+// Tests that an overscroll navigation that receives a paint update actually
+// stops observing.
+TEST_F(OverscrollNavigationOverlayTest, Navigation_PaintUpdate) {
+  PerformBackNavigationViaSliderCallbacks();
+  ReceivePaintUpdate();
+
+  // Paint updates until the navigation is committed typically represent updates
+  // for the previous page, so we should still be observing.
+  EXPECT_TRUE(GetOverlay()->web_contents());
 
+  contents()->CommitPendingNavigation();
   ReceivePaintUpdate();
-  EXPECT_TRUE(GetOverlay()->received_paint_update_);
-  EXPECT_FALSE(GetOverlay()->loading_complete_);
-  // The paint update will hide the overlay.
+
+  // Navigation was committed and the paint update was received - we should no
+  // longer be observing.
   EXPECT_FALSE(GetOverlay()->web_contents());
+  EXPECT_EQ(contents()->GetURL(), third_);
 }
 
-TEST_F(OverscrollNavigationOverlayTest, LoadUpdateWithoutNonEmptyPaint) {
-  GetOverlay()->image_delegate_->SetImage(CreateDummyScreenshot());
-  process()->sink().ClearMessages();
-
+// Tests that an overscroll navigation that receives a loading update actually
+// stops observing.
+TEST_F(OverscrollNavigationOverlayTest, Navigation_LoadingUpdate) {
+  PerformBackNavigationViaSliderCallbacks();
+  EXPECT_TRUE(GetOverlay()->web_contents());
+  // DidStopLoading for any navigation should always reset the load flag and
+  // dismiss the overlay even if the pending navigation wasn't committed -
+  // this is a "safety net" in case we mis-identify the destination webpage
+  // (which can happen if a new navigation is performed while while a GestureNav
+  // navigation is in progress).
+  contents()->TestSetIsLoading(true);
   contents()->TestSetIsLoading(false);
-  EXPECT_TRUE(GetOverlay()->loading_complete_);
-  EXPECT_FALSE(GetOverlay()->received_paint_update_);
-  // The page load should hide the overlay.
   EXPECT_FALSE(GetOverlay()->web_contents());
+  contents()->CommitPendingNavigation();
+  EXPECT_EQ(contents()->GetURL(), third_);
 }
 
+// Tests that an overscroll gesture followed after another one before the first
+// one finishes loading works with a paint update.
 TEST_F(OverscrollNavigationOverlayTest, MultiNavigation_PaintUpdate) {
-  GetOverlay()->image_delegate_->SetImage(CreateDummyScreenshot());
-  SetDummyScreenshotOnNavEntry(controller().GetEntryAtOffset(-1));
-
   PerformBackNavigationViaSliderCallbacks();
-  // Screenshot was set on NavEntry at offset -1.
-  EXPECT_TRUE(GetOverlay()->image_delegate_->has_image());
-  EXPECT_FALSE(GetOverlay()->received_paint_update_);
-
+  EXPECT_TRUE(GetOverlay()->web_contents());
+  PerformBackNavigationViaSliderCallbacks();
+  EXPECT_TRUE(GetOverlay()->web_contents());
   ReceivePaintUpdate();
+
   // Paint updates until the navigation is committed typically represent updates
-  // for the previous page, so they shouldn't affect the flag.
-  EXPECT_FALSE(GetOverlay()->received_paint_update_);
+  // for the previous page, so we should still be observing.
+  EXPECT_TRUE(GetOverlay()->web_contents());
 
   contents()->CommitPendingNavigation();
   ReceivePaintUpdate();
-  // Navigation was committed and the paint update was received - the flag
-  // should now be updated.
-  EXPECT_TRUE(GetOverlay()->received_paint_update_);
 
+  // Navigation was committed and the paint update was received - we should no
+  // longer be observing.
   EXPECT_FALSE(GetOverlay()->web_contents());
+  EXPECT_EQ(contents()->GetURL(), second_);
 }
 
+// Tests that an overscroll gesture followed after another one before the first
+// one finishes loading works with a loading update.
 TEST_F(OverscrollNavigationOverlayTest, MultiNavigation_LoadingUpdate) {
-  GetOverlay()->image_delegate_->SetImage(CreateDummyScreenshot());
-
   PerformBackNavigationViaSliderCallbacks();
-  // No screenshot was set on NavEntry at offset -1.
-  EXPECT_FALSE(GetOverlay()->image_delegate_->has_image());
-  // Navigation was started, so the loading status flag should be reset.
-  EXPECT_FALSE(GetOverlay()->loading_complete_);
-
+  EXPECT_TRUE(GetOverlay()->web_contents());
+  PerformBackNavigationViaSliderCallbacks();
+  EXPECT_TRUE(GetOverlay()->web_contents());
   // DidStopLoading for any navigation should always reset the load flag and
   // dismiss the overlay even if the pending navigation wasn't committed -
   // this is a "safety net" in case we mis-identify the destination webpage
@@ -187,9 +295,68 @@ TEST_F(OverscrollNavigationOverlayTest, MultiNavigation_LoadingUpdate) {
   // navigation is in progress).
   contents()->TestSetIsLoading(true);
   contents()->TestSetIsLoading(false);
-  EXPECT_TRUE(GetOverlay()->loading_complete_);
-
   EXPECT_FALSE(GetOverlay()->web_contents());
+  contents()->CommitPendingNavigation();
+  EXPECT_EQ(contents()->GetURL(), second_);
+}
+
+// Tests that swapping the overlay window at the end of a gesture caused by the
+// start of a new overscroll does not crash and the events still reach the new
+// overlay window.
+TEST_F(OverscrollNavigationOverlayTest, OverlayWindowSwap) {
+  PerformBackNavigationViaSliderCallbacks();
+  aura::Window* first_overlay_window = GetOverlay()->window_.get();
+  EXPECT_TRUE(GetOverlay()->web_contents());
+  EXPECT_TRUE(first_overlay_window);
+
+
+  // At this stage, the overlay window is covering the web contents. Configure
+  // the animator of the overlay window for the test.
+  ui::ScopedAnimationDurationScaleMode normal_duration(
+            ui::ScopedAnimationDurationScaleMode::NORMAL_DURATION);
+  ui::LayerAnimator* animator = GetOverlay()->window_->layer()->GetAnimator();
+  animator->set_disable_timer_for_test(true);
+  ui::LayerAnimatorTestController test_controller(animator);
+
+  // Start and complete a back navigation via a gesture.
+  ui::test::EventGenerator generator(root_window());
+  generator.GestureScrollSequence(
+      gfx::Point(10, 10),
+      gfx::Point(400, 10),
+      base::TimeDelta::FromMilliseconds(10),
+      10);
+
+  ui::ScopedLayerAnimationSettings settings(animator);
+  base::TimeDelta duration = settings.GetTransitionDuration();
+  test_controller.StartThreadedAnimationsIfNeeded();
+  base::TimeTicks start_time = gfx::FrameTime::Now();
+
+  // The overlay window should now be being animated to the edge of the screen.
+  animator->Step(start_time + duration / 2);
+  EXPECT_EQ(GetOverlay()->window_.get(), first_overlay_window);
+
+  // The overlay window is halfway through, start another animation that will
+  // cancel the first one. The first event that cancels the animation will go to

[mfomitchev, 2015/03/27 00:14:05]

So this comment is incorrect, right? The events go to the new window, not the
old overlay window from the beginning.. 

[Nina, 2015/03/27 17:52:36]

Right, fixed comment.

+  // the old overlay window, which will be destroyed, and subsequent events
+  // should be routed to the new overlay window.
+  generator.GestureScrollSequence(
+      gfx::Point(10, 10),

[mfomitchev, 2015/03/27 00:14:05]

Can we make  scroll gesture begin on the old overlay window? When we pass 10, I
think it is positioned on the new window.. 

[Nina, 2015/03/27 17:52:36]

I noticed this, and modified it.

+      gfx::Point(400, 10),
+      base::TimeDelta::FromMilliseconds(10),
+      10);
+  EXPECT_TRUE(GetOverlay()->window_.get());
+  // The overlay window should be a new window.
+  EXPECT_NE(GetOverlay()->window_.get(), first_overlay_window);
+
+  // Complete the animation.
+  GetOverlay()->window_->layer()->GetAnimator()->StopAnimating();
+  EXPECT_TRUE(GetOverlay()->window_.get());
+
+  // Load the page.
+  contents()->CommitPendingNavigation();
+  ReceivePaintUpdate();
+  EXPECT_FALSE(GetOverlay()->window_.get());
+  EXPECT_EQ(contents()->GetURL(), first_);

[mfomitchev, 2015/03/27 00:14:05]

We start with fourth_ and do two back gestures, shouldn't we be at second_ now?

[Nina, 2015/03/27 17:52:36]

We did three gestures, L307, L323 and L342.

[mfomitchev, 2015/03/31 22:16:31]

Acknowledged.

 }
 
 }  // namespace content