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 f63aeab6c07470428821cdb5c2a6fbee6c264dc8..a3d6b91fdb1d85bc40e6861dd08b5e9e4bfe8c26 100644
--- a/chrome/browser/ui/search/instant_controller.cc
+++ b/chrome/browser/ui/search/instant_controller.cc
@@ -933,6 +933,16 @@ void InstantController::ActiveTabChanged() {
 }
 
 void InstantController::TabDeactivated(content::WebContents* contents) {
+  std::string instant_url;
+  if ((!instant_tab_ || !instant_tab_->supports_instant()) &&
+      GetInstantURL(browser_->profile(), false, &instant_url) &&
+      chrome::MatchesOriginAndPath(contents->GetURL(),
+                                   GURL(instant_url))) {
+    // TODO(dcblack): verify this won't nuke loading SRPs and the like.

[samarth, 2013/04/30 17:10:33]

This definitely will. In general, I don't think we need to worry much about the
ctrl+TTTT... case.  I really doubt that is a normal use case except for Chrome
engineers.

[David Black, 2013/04/30 23:09:08]

As I mentioned in person, this is also necessary to prevent such Ts from
actually showing error pages.

I'm not actually convinced it'll affect anything we don't want it to.  It'll
only trigger on instant_urls that don't have Instant support determined. 
Instant_urls are always the homepage, and if support hasn't been determined yet
then there's no way to get a query or anything like that in there.  I think this
might be ok, but would certainly appreciate others reasoning through it as well.

+    // (It probably will - likely need to check the mode or something.)
+    RedirectToLocalNTP(contents);
+  }
+
   LOG_INSTANT_DEBUG_EVENT(this, "TabDeactivated");
   if (extended_enabled_ && !contents->IsBeingDestroyed())
     CommitIfPossible(INSTANT_COMMIT_FOCUS_LOST);
@@ -1074,6 +1084,29 @@ void InstantController::UndoAllMostVisitedDeletions() {
   top_sites->ClearBlacklistedURLs();
 }
 
+void InstantController::InstantPageLoadFailed(content::WebContents* contents) {
+  GURL local_fallback_url = chrome::GetLocalInstantURL(browser_->profile());
+
+  if (instant_tab_ && IsContentsFrom(instant_tab(), contents)) {
+    if (instant_tab_->IsLocal())
+      return;
+    SearchTabHelper::FromWebContents(contents)->RedirectingToLocal();
+    RedirectToLocalNTP(contents);
+  }
+
+  if (ntp_ && IsContentsFrom(ntp(), contents)) {

[samarth, 2013/04/30 17:10:33]

Shouldn't this get handled normally via InstantSupportDetermined?

[David Black, 2013/04/30 23:09:08]

Whether it *should* or not is not something I'm sure about, but I am sure it
*doesn't* get handled by it.  If I comment this out, creating a new NTP when GWS
returns an error results in the error page.

+    if (ntp_->IsLocal())
+      return;
+    ResetNTP(false, true);
+  }
+
+  if (overlay_ && IsContentsFrom(overlay(), contents)) {
+    if (overlay_->IsLocal())
+      return;
+    CreateOverlay(local_fallback_url.spec(), contents);
+  }
+}
+
 void InstantController::Observe(int type,
                                 const content::NotificationSource& source,
                                 const content::NotificationDetails& details) {
@@ -1706,12 +1739,6 @@ bool InstantController::ShouldSwitchToLocalNTP() const {
   if (ntp_->supports_instant())
     return false;
 
-  // If this is not window startup, switch.
-  // TODO(shishir): This is not completely reliable. Find a better way to detect
-  // startup time.
-  if (browser_->GetActiveWebContents())
-    return true;
-
   return chrome::IsAggressiveLocalNTPFallbackEnabled();
 }
 
@@ -1719,3 +1746,13 @@ 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.
+  // Remove extraneous history entry.
+  contents->GetController().PruneAllButActive();

[samarth, 2013/04/30 17:10:33]

This isn't right in the general case.  For example, let's say you use the home
button or bookmark to navigate to chrome://newtab in an existing tab. This will
nuke all your history.

[David Black, 2013/04/30 23:09:08]

Yes, I know.  However, I was unable to find a way of removing or replacing the
most recent committed history state with the current active entry.  I could do
it by adding a new function to the controller, but that'll require LGTMs from
outside the team, so I'd prefer to leave that as a followup.  Added a TODO.

+}