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..c668553343168b4c423e269d3e3899bdc8575370 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"
@@ -21,7 +22,11 @@ namespace content {
 
 class OverscrollNavigationOverlayTest : public RenderViewHostImplTestHarness {
  public:
-  OverscrollNavigationOverlayTest() {}
+  OverscrollNavigationOverlayTest()
+      : first_("https://www.google.com"),
+        second_("http://www.chromium.org"),
+        third_("https://www.kernel.org/") {}
+
   ~OverscrollNavigationOverlayTest() override {}
 
   gfx::Image CreateDummyScreenshot() {
@@ -51,28 +56,35 @@ 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_;
+
  protected:
   // RenderViewHostImplTestHarness:
   void SetUp() override {
     RenderViewHostImplTestHarness::SetUp();
 
-    const GURL first("https://www.google.com");
-    contents()->NavigateAndCommit(first);
+    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());
+    EXPECT_FALSE(controller().CanGoForward());
 
     // Receive a paint update. This is necessary to make sure the size is set
     // correctly in RenderWidgetHostImpl.
@@ -86,22 +98,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(), contents()->GetNativeView()));
   }
 
   void TearDown() override {
@@ -119,67 +117,122 @@ 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());
+}
+
+// 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());
 }
 
-TEST_F(OverscrollNavigationOverlayTest, FirstVisuallyNonEmptyPaint_WithImage) {
-  GetOverlay()->image_delegate_->SetImage(CreateDummyScreenshot());
+// 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);
+}
+
+// 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);

[mfomitchev, 2015/03/26 15:36:00]

Why don't we test that the direction is BACK instead of testing it's not NONE?
Applies to other tests as well.

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

Done.

+
+  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/26 15:36:00]

Also confirm direction is NONE?

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

Done.

+}
+
+// Tests that if a second navigation is aborted before, the first one still

[mfomitchev, 2015/03/26 15:36:00]

"a second navigation is aborted before" is not clear

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

Done.

+// 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(), second_);
+}
+
+// Tests that an overscroll navigation that receives a paint update actually
+// stops observing.
+TEST_F(OverscrollNavigationOverlayTest, Navigation_PaintUpdate) {
+  PerformBackNavigationViaSliderCallbacks();
   ReceivePaintUpdate();
-  EXPECT_TRUE(GetOverlay()->received_paint_update_);
-  EXPECT_FALSE(GetOverlay()->loading_complete_);
-  // The paint update will hide the overlay.
+
+  // 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();
+
+  // 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_);
 }
 
-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(), second_);
 }
 
+// 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();

[mfomitchev, 2015/03/26 15:36:00]

Do you think we still need the multinavigation tests? It was a different flow
for the second navigation before since the slider callbacks were only used for
the second navigation. Now it's the same callbacks, so I am not sure these tests
are still useful.

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

Yeah... it does not make that much sense to have these. The codepath is the
same. Removed.

-  // 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(), first_);
 }
 
+// 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 +240,9 @@ 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(), first_);
 }
 
 }  // namespace content