Fall back to local page if online NTP fails to load.

chrome/browser/ui/search/instant_controller.cc

Index: chrome/browser/ui/search/instant_controller.cc
diff --git a/chrome/browser/ui/search/instant_controller.cc b/chrome/browser/ui/search/instant_controller.cc
index e721a26e45e908da47226ad37f3c128f8caf0cf6..197d56157fa6eb20f05b2e85b9f301e860e2c13e 100644
--- a/chrome/browser/ui/search/instant_controller.cc
+++ b/chrome/browser/ui/search/instant_controller.cc
@@ -712,6 +712,44 @@ void InstantController::OmniboxNavigateToURL() {
     instant_tab_->Submit(string16());
 }
 
+void InstantController::InstantPageLoadFailed(content::WebContents* contents) {
+  if (!chrome::ShouldPreferRemoteNTPOnStartup() || !extended_enabled_) {
+    // We only need to fall back on errors if we're showing the online page
+    // at startup, as otherwise we fall back correctly when trying to show
+    // a page that hasn't yet indicated that it supports the InstantExtended
+    // API.
+    return;
+  }
+
+  if (IsContentsFrom(instant_tab(), contents)) {
+    // Verify we're not already on a local page and that the URL precisely
+    // equals the instant_url (minus the query params, as those will be filled
+    // in by template values).  This check is necessary to make sure we don't
+    // inadvertently redirect to the local NTP if someone, say, reloads a SRP
+    // while offline, as a committed results page still counts as an instant
+    // url.
+    const GURL current_url = contents->GetURL();

[sreeram, 2013/05/05 08:40:39]

const GURL&

[David Black, 2013/05/05 20:02:42]

Done.

+    if (instant_tab_->IsLocal() ||
+        !chrome::MatchesOriginAndPath(GURL(GetInstantURL()), current_url) ||
+        !current_url.ref().empty())
+      return;
+    LOG_INSTANT_DEBUG_EVENT(this, "InstantPageLoadFailed: instant_tab");

[Jered, 2013/05/05 15:08:00]

So when do we get in here exactly? I think just on a remote NTP or
google.com/webhp if it fails to load (instant_url has /webhp right)? It would be
moderately weird if visiting google.com when offline took me to the local NTP
(since for instance, I might visit google.com to try to figure out if my
internet works).

[sreeram, 2013/05/05 16:38:13]

Yes, instant_url has /webhp. But, it's possible that when users start typing
"google.com", a history autocompletion might lead them to
"https://www.google.com/webhp".

As it turns out, there's another small layer of protection. In general, we only
hook up |instant_tab_| when the |search_mode_| is correct. So, if you start from
a non-NTP/SERP (so instant_tab_ is NULL to begin with), when you navigate to
"https://www.google.com/webhp", search_mode_ won't be updated to become NTP
(because it lacks the "espv" param). So, instant_tab_ will remain NULL, and no
swapping shenanigans will take place.

If you start from a local NTP, we won't swap, so we are still good. (So, if you
are offline, and you open a new tab to check out google.com, there still won't
be a swap, because you started from a local NTP).

There is a risk that you might start from a Google NTP/SERP and type
"https://www.google.com/webhp". But I think this risk is small. Moreover, given
that you were looking at a valid Google page before, it shouldn't be a surprise
that you continued seeing a valid Google-like page (the local NTP), even if you
were actually offline.

It is also a risk if you happen to type "https://www.google.com/webhp?espv=2"
(perhaps aided by an autocompletion). If you are offline or there's a 404 or
such, we will nevertheless hook up instant_tab_ (since search_mode_ will be
NTP). But search_mode_ is only updated after the provisional load commits
(including failures), so the instant_tab_ hookup happens after the failure, so
we won't observe any _new_ failure (and so, no swapping). Also,
OnInstantSupportDetermined() will fire, and reset instant_tab_ to NULL again.

So overall, I think the risk of this confusion is small. Of course, I haven't
actually tested any of my claims / theories above. Somebody should.

This points to a different concern with the local NTP though. If you are offline
and you open a new tab, you see what looks like the google.com page. Many users
might simply conclude therefore that Google is reachable and the problem is with
whatever website they were trying to access.

[Jered, 2013/05/05 16:59:13]

Wow, ok, thanks for the explanation. Wheels within wheels. I will patch this and
try it Monday.

+    RedirectToLocalNTP(contents);
+  } else if (IsContentsFrom(ntp(), contents)) {
+    LOG_INSTANT_DEBUG_EVENT(this, "InstantPageLoadFailed: ntp");
+    bool is_local = ntp_->IsLocal();
+    DeletePageSoon(ntp_.Pass());
+    if (!is_local)
+      ResetNTP(GetLocalInstantURL());
+  } else if (IsContentsFrom(overlay(), contents)) {
+    LOG_INSTANT_DEBUG_EVENT(this, "InstantPageLoadFailed: overlay");
+    bool is_local = overlay_->IsLocal();
+    DeletePageSoon(overlay_.Pass());
+    if (!is_local)
+      ResetOverlay(GetLocalInstantURL());
+  }
+}
+
 content::WebContents* InstantController::GetOverlayContents() const {
   return overlay_ ? overlay_->contents() : NULL;
 }
@@ -1136,8 +1174,13 @@ void InstantController::InstantSupportDetermined(
         content::Source<InstantController>(this),
         content::NotificationService::NoDetails());
   } else if (IsContentsFrom(ntp(), contents)) {
-    if (!supports_instant)
+    if (!supports_instant) {
+      bool is_local = ntp_->IsLocal();
       DeletePageSoon(ntp_.Pass());
+      // Preload a local NTP in place of the broken online one.
+      if (!is_local)
+        ResetNTP(GetLocalInstantURL());
+    }
 
     content::NotificationService::current()->Notify(
         chrome::NOTIFICATION_INSTANT_NTP_SUPPORT_DETERMINED,
@@ -1147,7 +1190,11 @@ void InstantController::InstantSupportDetermined(
   } else if (IsContentsFrom(overlay(), contents)) {
     if (!supports_instant) {
       HideInternal();
+      bool is_local = overlay_->IsLocal();
       DeletePageSoon(overlay_.Pass());
+      // Preload a local overlay in place of the broken online one.
+      if (!is_local && extended_enabled_)
+        ResetOverlay(GetLocalInstantURL());
     }
 
     content::NotificationService::current()->Notify(
@@ -1172,29 +1219,36 @@ void InstantController::InstantPageRenderViewGone(
 void InstantController::InstantPageAboutToNavigateMainFrame(
     const content::WebContents* contents,
     const GURL& url) {
-  DCHECK(IsContentsFrom(overlay(), contents));
-
-  // If the page does not yet support Instant, we allow redirects and other
-  // navigations to go through since the Instant URL can redirect - e.g. to
-  // country specific pages.
-  if (!overlay_->supports_instant())
-    return;
+  if (IsContentsFrom(overlay(), contents)) {
+    // If the page does not yet support Instant, we allow redirects and other
+    // navigations to go through since the Instant URL can redirect - e.g. to
+    // country specific pages.
+    if (!overlay_->supports_instant())
+      return;
 
-  GURL instant_url(overlay_->instant_url());
+    GURL instant_url(overlay_->instant_url());
 
-  // If we are navigating to the Instant URL, do nothing.
-  if (url == instant_url)
-    return;
+    // If we are navigating to the Instant URL, do nothing.
+    if (url == instant_url)
+      return;
 
-  // Commit the navigation if either:
-  //  - The page is in NTP mode (so it could only navigate on a user click) or
-  //  - The page is not in NTP mode and we are navigating to a URL with a
-  //    different host or path than the Instant URL. This enables the instant
-  //    page when it is showing search results to change the query parameters
-  //    and fragments of the URL without it navigating.
-  if (model_.mode().is_ntp() ||
-      (url.host() != instant_url.host() || url.path() != instant_url.path())) {
-    CommitIfPossible(INSTANT_COMMIT_NAVIGATED);
+    // Commit the navigation if either:
+    //  - The page is in NTP mode (so it could only navigate on a user click) or
+    //  - The page is not in NTP mode and we are navigating to a URL with a
+    //    different host or path than the Instant URL. This enables the instant
+    //    page when it is showing search results to change the query parameters
+    //    and fragments of the URL without it navigating.
+    if (model_.mode().is_ntp() ||
+        (url.host() != instant_url.host() ||
+         url.path() != instant_url.path())) {
+      CommitIfPossible(INSTANT_COMMIT_NAVIGATED);
+    }
+  } else if (IsContentsFrom(instant_tab(), contents)) {
+    // The Instant tab navigated.  Send it the data it needs to display
+    // properly.
+    UpdateInfoForInstantTab();
+  } else {
+    NOTREACHED();
   }
 }
 
@@ -1452,14 +1506,7 @@ void InstantController::ResetInstantTab() {
     if (!instant_tab_ || active_tab != instant_tab_->contents()) {
       instant_tab_.reset(new InstantTab(this));
       instant_tab_->Init(active_tab);
-      // Update theme info for this tab.
-      browser_->UpdateThemeInfo();
-      instant_tab_->SetDisplayInstantResults(instant_enabled_);
-      instant_tab_->SetOmniboxBounds(omnibox_bounds_);
-      instant_tab_->InitializeFonts();
-      StartListeningToMostVisitedChanges();
-      instant_tab_->KeyCaptureChanged(
-          omnibox_focus_state_ == OMNIBOX_FOCUS_INVISIBLE);
+      UpdateInfoForInstantTab();
     }
 
     // Hide the |overlay_| since we are now using |instant_tab_| instead.
@@ -1469,6 +1516,18 @@ void InstantController::ResetInstantTab() {
   }
 }
 
+void InstantController::UpdateInfoForInstantTab() {
+  if (instant_tab_) {
+    browser_->UpdateThemeInfo();
+    instant_tab_->SetDisplayInstantResults(instant_enabled_);
+    instant_tab_->SetOmniboxBounds(omnibox_bounds_);
+    instant_tab_->InitializeFonts();
+    StartListeningToMostVisitedChanges();
+    instant_tab_->KeyCaptureChanged(
+        omnibox_focus_state_ == OMNIBOX_FOCUS_INVISIBLE);
+  }
+}
+
 void InstantController::HideOverlay() {
   HideInternal();
   ReloadOverlayIfStale();
@@ -1692,3 +1751,14 @@ bool InstantController::UsingLocalPage() const {
   return (instant_tab_ && instant_tab_->IsLocal()) ||
       (!instant_tab_ && overlay_ && overlay_->IsLocal());
 }
+
+void InstantController::RedirectToLocalNTP(content::WebContents* contents) {
+  contents->GetController().LoadURL(
+    chrome::GetLocalInstantURL(browser_->profile()),
+    content::Referrer(),
+    content::PAGE_TRANSITION_SERVER_REDIRECT,
+    std::string());  // No extra headers.
+  // TODO(dcblack): Remove extraneous history entry caused by 404s.
+  // Note that the base case of a 204 being returned doesn't push a history
+  // entry.

[sreeram, 2013/05/05 08:40:39]

Won't this nuke the "Forward" history in the Back/Forward nav stack? For
example, if the user hits "Back" to go back to the Google NTP, and it 404s or
the user is offline. It's fine to switch to the local NTP, but we shouldn't nuke
the nav stack.

Also, PAGE_TRANSITION_SERVER_REDIRECT isn't strictly true, but we can let that
one slide for now.

[David Black, 2013/05/05 20:02:42]

Yes, good point.  Added a check to not redirect instant_tab_ if there exists
forward history.

+}