Use entry URLs instead of entry IDs to determine when to dismiss the overscroll overlay.

This is a band-aid fix for crbug.com/415167, which I'd like to try to get into M38.
Apparently it's possible to have "temporary" entry IDs set as pending and then
thrown away. This confuses OverscrollNavigationOverlay into thinking that the
page was never loaded and the overlay never gets dismissed as a result, which
makes the webpage unusable.
Using URLs is not as reliable avoids this problem. It can introduce false
positives in some corner cases, however a false positive doesn't have severe
consequences - we simply dismiss the overlay too soon, and the use may see an
empty page or a flicker.

BUG=415167

Review URL: https://codereview.chromium.org/575203002

Cr-Commit-Position: refs/heads/master@{#295811}

content/browser/web_contents/aura/overscroll_navigation_overlay.cc

@@ -21,6 +21,24 @@
 #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->GetURL() == url)
+    return true;
+  const std::vector<GURL>& redirect_chain = entry->GetRedirectChain();
+  for (std::vector<GURL>::const_iterator it = redirect_chain.begin();
+       it != redirect_chain.end();
+       it++) {
+    if (*it == url)
+      return true;
+  }
+  return false;
+}
+
+}  // namespace
 
 // A LayerDelegate that paints an image for the layer.
 class ImageLayerDelegate : public ui::LayerDelegate {
@@ -119,7 +137,6 @@ OverscrollNavigationOverlay::OverscrollNavigationOverlay(
       image_delegate_(NULL),
       loading_complete_(false),
       received_paint_update_(false),
-      pending_entry_id_(0),
       slide_direction_(SLIDE_UNKNOWN) {
 }
 
@@ -130,7 +147,6 @@ void OverscrollNavigationOverlay::StartObserving() {
   loading_complete_ = false;
   received_paint_update_ = false;
   overlay_dismiss_layer_.reset();
-  pending_entry_id_ = 0;
   Observe(web_contents_);
 
   // Make sure the overlay window is on top.
@@ -140,10 +156,10 @@ void OverscrollNavigationOverlay::StartObserving() {
   // Assumes the navigation has been initiated.
   NavigationEntry* pending_entry =
       web_contents_->GetController().GetPendingEntry();
-  // Save id of the pending entry to identify when it loads and paints later.
+  // 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_id_ = pending_entry ? pending_entry->GetUniqueID() : 0;
+  pending_entry_url_ = pending_entry ? pending_entry->GetURL() : GURL();
 }
 
 void OverscrollNavigationOverlay::SetOverlayWindow(
@@ -282,23 +298,21 @@ void OverscrollNavigationOverlay::OnWindowSliderDestroyed() {
 }
 
 void OverscrollNavigationOverlay::DidFirstVisuallyNonEmptyPaint() {
-  int visible_entry_id =
-      web_contents_->GetController().GetVisibleEntry()->GetUniqueID();
-  if (visible_entry_id == pending_entry_id_ || !pending_entry_id_) {
+  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) {
-  // Use the last committed entry rather than the active one, in case a
-  // pending entry has been created.
-  int committed_entry_id =
-      web_contents_->GetController().GetLastCommittedEntry()->GetUniqueID();
-  if (committed_entry_id == pending_entry_id_ || !pending_entry_id_) {
-    loading_complete_ = true;
-    StopObservingIfDone();
-  }
+  // 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

content/browser/web_contents/aura/overscroll_navigation_overlay.h

@@ -101,10 +101,10 @@ class CONTENT_EXPORT OverscrollNavigationOverlay
   bool loading_complete_;
   bool received_paint_update_;
 
-  // Unique ID of the NavigationEntry we are navigating to. This is needed to
+  // URL of the NavigationEntry we are navigating to. This is needed to
   // filter on WebContentsObserver callbacks and is used to dismiss the overlay
   // when the relevant page loads and paints.
-  int pending_entry_id_;
+  GURL pending_entry_url_;
 
   // The |WindowSlider| that allows sliding history layers while the page is
   // being reloaded.

content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc

@@ -157,7 +157,7 @@ TEST_F(OverscrollNavigationOverlayTest, MultiNavigation_PaintUpdate) {
   EXPECT_FALSE(GetOverlay()->received_paint_update_);
 
   ReceivePaintUpdate();
-  // Paint updates until the navigation is committed represent updates
+  // 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_);
 
@@ -179,17 +179,13 @@ TEST_F(OverscrollNavigationOverlayTest, MultiNavigation_LoadingUpdate) {
   // Navigation was started, so the loading status flag should be reset.
   EXPECT_FALSE(GetOverlay()->loading_complete_);
 
-  // Load updates until the navigation is committed represent updates for the
-  // previous page, so they shouldn't affect the flag.
+  // 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_FALSE(GetOverlay()->loading_complete_);
-
-  contents()->CommitPendingNavigation();
-  contents()->TestSetIsLoading(true);
-  contents()->TestSetIsLoading(false);
-  // Navigation was committed and the load update was received - the flag
-  // should now be updated.
   EXPECT_TRUE(GetOverlay()->loading_complete_);
 
   EXPECT_FALSE(GetOverlay()->web_contents());