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

chrome/browser/ui/search/instant_page.cc

Index: chrome/browser/ui/search/instant_page.cc
diff --git a/chrome/browser/ui/search/instant_page.cc b/chrome/browser/ui/search/instant_page.cc
index dbb5c2527307a041d6f6cd94f8aa48d0af85bd73..c773378273457a020ac558ee7536e853ae5a4974 100644
--- a/chrome/browser/ui/search/instant_page.cc
+++ b/chrome/browser/ui/search/instant_page.cc
@@ -7,7 +7,9 @@
 #include "base/utf_string_conversions.h"
 #include "chrome/common/render_messages.h"
 #include "chrome/common/url_constants.h"
+#include "content/public/browser/navigation_details.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/common/frame_navigate_params.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/font.h"
 
@@ -193,6 +195,16 @@ void InstantPage::DidCommitProvisionalLoadForFrame(
     delegate_->InstantPageAboutToNavigateMainFrame(contents(), url);
 }
 
+void InstantPage::DidFailProvisionalLoad(

[samarth, 2013/05/03 04:39:17]

nit: keep functions in same order as in the header file.

[David Black, 2013/05/03 06:14:44]

Done.

[sreeram, 2013/05/03 23:17:31]

Not done?

[David Black, 2013/05/04 00:49:23]

Done.

+    int64 /* frame_id */,
+    bool /* is_main_frame */,
+    const GURL& /* validated_url */,
+    int /* error_code */,
+    const string16& /* error_description */,
+    content::RenderViewHost* /* render_view_host */) {
+  delegate_->InstantPageLoadFailed(contents());

[sreeram, 2013/05/03 23:17:31]

Shouldn't you do this only for "if (is_main_frame)"?

[David Black, 2013/05/04 00:49:23]

Sure, I guess that sanity check makes sense.  Done.

+}
+
 void InstantPage::OnSetSuggestions(
     int page_id,
     const std::vector<InstantSuggestion>& suggestions) {
@@ -258,3 +270,13 @@ void InstantPage::OnUndoMostVisitedDeletion(InstantRestrictedID restricted_id) {
 void InstantPage::OnUndoAllMostVisitedDeletions() {
   delegate_->UndoAllMostVisitedDeletions();
 }
+
+void InstantPage::DidNavigateMainFrame(
+      const content::LoadCommittedDetails& details,
+      const content::FrameNavigateParams& /* params */) {

[sreeram, 2013/05/03 23:17:31]

Strange indent. Either 4 from beginning of line, or from beginning of
"InstantPage" or such.

[David Black, 2013/05/04 00:49:23]

Done.

+  // A 204 can be sent by the search provider as a lightweight signal
+  // to fall back to the local page, and we obviously want to fall back
+  // if we get any response code that indicates an error.
+  if (details.http_status_code == 204 || details.http_status_code >= 400)
+    delegate_->InstantPageLoadFailed(contents());

[sreeram, 2013/05/03 23:17:31]

DidNavigateMainFrame() is called for all main page navigations, including after
we have successfully loaded the server NTP and used it for searches, etc. It
doesn't seem that you should redirect to local NTP in those cases. Right?

There are some checks in InstantController where you return if the URL doesn't
match the instant_url of the template, etc., but what if those checks pass? Say
for example that there's a Back/Forward navigation to the homepage or a
history.pushState (say I clicked on the Google logo from the search results to
go back to the homepage) or some combination of the two. Is it possible for any
of these to return 204? (Not that the server actually returns a 204, but say
because of history.pushState semantics or Back/Forward cache?)

Basically, I'd like us to be more conservative when deciding that it's okay to
replace an InstantTab contents. For example, if we have not yet determined
instant_support for it.

[David Black, 2013/05/04 00:49:23]

This does not seem like a coherent concern to me.  A history.pushState won't
have an error http_status_code (if it even triggers this function at all, which
I doubt, as it's not a navigation), and everything else is an active navigation
to a homepage URL, which should always fall back on error in all cases.  If I
hit the back button to navigate to the NTP and I'm now offline then it should in
fact fall back to the local page.

A page that has failed to load and has an error status code will never have
instant_support because it hasn't loaded.

[David Black, 2013/05/04 00:53:51]

I have verified that history.pushState does not result in this function being
called.

+}