Commit instant loader when the instant page navigates away from instant URL.

chrome/browser/instant/instant_controller.cc

Index: chrome/browser/instant/instant_controller.cc
diff --git a/chrome/browser/instant/instant_controller.cc b/chrome/browser/instant/instant_controller.cc
index 1e63886b896da48c173bbbb985d178fc240b5439..090ead61754be99672fdcb26918ecfc8e92d4c81 100644
--- a/chrome/browser/instant/instant_controller.cc
+++ b/chrome/browser/instant/instant_controller.cc
@@ -438,12 +438,12 @@ bool InstantController::CommitIfCurrent(InstantCommitType type) {
     return false;
   }
 
-  if (!IsCurrent())
+  if (!IsCurrent() && type != INSTANT_COMMIT_NAVIGATED)

[samarth, 2012/12/07 01:14:34]

This goes against the name of this function :)

How about changing IsCurrent to take a reason?  And while we're at it, I would
rename it to something like CanCommit or CanCommitForReason.  (The only other
place it gets used is in OmniboxLostFocus I think.)

[Shishir, 2012/12/07 21:01:25]

It seemed better to rename IsCurrent to IsCurrentSearch. CanCommit seems more
general that current status to me.Take a look.

     return false;
 
   if (type == INSTANT_COMMIT_FOCUS_LOST)
     loader_->Cancel(last_omnibox_text_);
-  else
+  else if (type != INSTANT_COMMIT_NAVIGATED)
     loader_->Submit(last_omnibox_text_);
 
   content::WebContents* preview = loader_->ReleaseContents();
@@ -481,7 +481,15 @@ bool InstantController::CommitIfCurrent(InstantCommitType type) {
       loader_->last_navigation();
   if (!last_navigation.url.is_empty()) {
     content::NavigationEntry* entry = preview->GetController().GetActiveEntry();
-    DCHECK_EQ(last_navigation.url, entry->GetURL());
+
+    // The last navigation should be the same as the active entry if the loader
+    // is in search mode. During navigation, the active entry could have
+    // changed since DidCommitProvisionalLoadForFrame is called after the entry
+    // is changed.
+    // TODO(shishir): Should we commit the last navigation for

[samarth, 2012/12/07 01:14:34]

I'm a little confused by this.  Is the situation you're describing where I'm on
the NTP with instant URL A, it navigates to A' (also an instant URL) and then to
B?

[Shishir, 2012/12/07 21:01:25]

Yes its the case you describe.

+    // INSTANT_COMMIT_NAVIGATED.
+    DCHECK(type == INSTANT_COMMIT_NAVIGATED ||
+           last_navigation.url == entry->GetURL());
 
     // Add the page to history.
     HistoryTabHelper* history_tab_helper =
@@ -766,6 +774,20 @@ void InstantController::InstantLoaderContentsFocused() {
 #endif
 }
 
+void InstantController::InstantLoaderAboutToNavigateMainFrame(const GURL& url) {
+  GURL instant_url(loader_->instant_url());
+
+  // Might be navigating to the instant URL.
+  if (url == instant_url)

[samarth, 2012/12/07 01:14:34]

Why is this check necessary given the check below?

[Shishir, 2012/12/07 21:01:25]

Not really necessary, just makes it clear that navigating to the instant URL is
also intercepted here. Removing.

+    return;
+
+  // Don't commit if the URL being navigated to has the same host and path as
+  // the instant URL. This enables the instant page to change the query
+  // parameters and fragments of the URL without it navigating.
+  if (url.host() != instant_url.host() || url.path() != instant_url.path())
+    CommitIfCurrent(INSTANT_COMMIT_NAVIGATED);
+}
+
 bool InstantController::ResetLoader(const TemplateURL* template_url,
                                     const content::WebContents* active_tab) {
   std::string instant_url;