Persist the Instant API to committed search result pages.

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 0c0f54706186ed867be074c5b94ac6579ce32d6f..293fd04beef5aec6148193b75babb129acb58d3f 100644
--- a/chrome/browser/instant/instant_controller.cc
+++ b/chrome/browser/instant/instant_controller.cc
@@ -176,11 +176,12 @@ bool InstantController::Update(const AutocompleteMatch& match,
   DCHECK(!omnibox_popup_is_open || user_input_in_progress);
 
   // If the popup is closed, there should be no inline autocompletion.
-  DCHECK(omnibox_popup_is_open || user_text == full_text) << user_text << "|"
-                                                          << full_text;
+  DCHECK(omnibox_popup_is_open || user_text.empty() || user_text == full_text)
+      << user_text << "|" << full_text;
 
   // If there's inline autocompletion, the query has to be verbatim.
-  DCHECK(user_text == full_text || verbatim) << user_text << "|" << full_text;
+  DCHECK(user_text.empty() || user_text == full_text || verbatim)
+      << user_text << "|" << full_text;
 
   // If there's no text in the omnibox, the user can't have typed any.
   DCHECK(!full_text.empty() || user_text.empty()) << user_text;
@@ -204,6 +205,21 @@ bool InstantController::Update(const AutocompleteMatch& match,
     return false;
   }
 
+  // Ensure we have a loader that can process this match. First, try to use the
+  // TemplateURL of the |match|. If that's invalid, in non-extended mode, stop.
+  // In extended mode, try using the default search engine, but only when the
+  // match is for a URL (i.e., not some other kind of non-Instant search).
+  // A completely blank query shows up as a search, and we do want to allow
+  // that, hence the "!user_text.empty()" clause.
+  Profile* const profile = active_tab->profile();
+  const bool match_is_search = AutocompleteMatch::IsSearchType(match.type);
+  if (!ResetLoader(match.GetTemplateURL(profile, false), active_tab) &&

[Jered, 2012/11/27 19:21:57]

Why reset the loader here if client_ is set?

[sreeram, 2012/11/29 07:33:19]

Done.

+      (!extended_enabled_ || (match_is_search && !user_text.empty()) ||
+       (!client_ && !CreateDefaultLoader()))) {
+    Hide(true);
+    return false;
+  }
+
   // Legend: OPIO == |omnibox_popup_is_open|, UIIP = |user_input_in_progress|.
   //
   //  #  OPIO UIIP full_text  Notes
@@ -226,40 +242,47 @@ bool InstantController::Update(const AutocompleteMatch& match,
   //
   //  In extended mode, #2 and #4 call Hide(). #1 doesn't Hide() as the preview
   //  may be showing custom NTP content, but doesn't Update() either. #3 and #7
-  //  don't Hide(), but send a blank query to Update(). #8 calls Update().
+  //  don't Hide(), and send a blank query to Update(). #8 calls Update().
 
   if (extended_enabled_) {
     if (!omnibox_popup_is_open) {
-      if (!full_text.empty()) {
-        Hide(true);
-        return false;
+      if (!user_input_in_progress) {
+        if (!full_text.empty()) {
+          Hide(true);  // #2

[samarth, 2012/11/27 18:09:43]

Mostly for my understanding: Hide() will be a no-op here if client_ is set,
right? If so, it might be clearer to make that explicit by checking if
(!client_) before calling it.

[sreeram, 2012/11/29 07:33:19]

No, Hide() won't be a no-op. For one, it will actually hide the preview (i.e.,
owned by |loader_|). It will also reset last_full_text_, which also isn't a
no-op.

+          // If the user hit Escape when on a search results page, restore the
+          // original results by resending the query. If the user was not on a
+          // search results page, SearchModeChanged() would've caught it, and
+          // |client_| will be NULL, so we won't accidentally send non-query
+          // |full_text| to |client_|. Except if the user just switched tabs.
+          // Hence the comparison of WebContents, to catch that case as well.
+          if (client_ && client_->contents() == active_tab->web_contents())
+            client_->Update(full_text, true);
+        }
+        return false;  // #1
+      } else {
+        if (!full_text.empty()) {
+          Hide(true);  // #4
+          if (match_is_search) {
+            // The user just switched to a tab with a partial query already in
+            // the omnibox. This tab may or may not be a search results page
+            // (SearchModeChanged() hasn't been called yet). So, assume that it
+            // could be, and store |full_text| so that if the user then hits
+            // Enter, we'll send the correct query in client_->Submit().
+            last_full_text_ = full_text;
+            last_match_was_search_ = true;
+          }
+          return false;
+        }
       }
-      if (!user_input_in_progress && full_text.empty())
-        return false;
     }
   } else if (!omnibox_popup_is_open || full_text.empty()) {
     // Update() can be called if the user clicks the preview while composing
     // text with an IME. If so, we should commit on mouse up, so don't Hide().
-    if (!GetPreviewContents() || !loader_->IsPointerDownFromActivate())
+    if (!loader_ || !loader_->IsPointerDownFromActivate())
       Hide(true);
     return false;
   }
 
-  // Ensure we have a loader that can process this match. First, try to use the
-  // TemplateURL of the |match|. If that's invalid, in non-extended mode, stop.
-  // In extended mode, try using the default search engine, but only when the
-  // match is for a URL (i.e., not some other kind of non-Instant search).
-  // A completely blank query shows up as a search, and we do want to allow
-  // that, hence the "!full_text.empty()" clause.
-  Profile* const profile = active_tab->profile();
-  const bool match_is_search = AutocompleteMatch::IsSearchType(match.type);
-  if (!ResetLoader(match.GetTemplateURL(profile, false), active_tab) &&
-      (!extended_enabled_ || (match_is_search && !full_text.empty()) ||
-       !CreateDefaultLoader())) {
-    Hide(true);
-    return false;
-  }
-
   // If the user continues typing the same query as the suggested text is
   // showing, reuse the suggestion (but only for INSTANT_COMPLETE_NEVER).
   bool reused_suggestion = false;
@@ -288,7 +311,7 @@ bool InstantController::Update(const AutocompleteMatch& match,
   url_for_history_ = match.destination_url;
 
   // Store the first interaction time for use with latency histograms.
-  if (first_interaction_time_.is_null())
+  if (loader_ == CurrentClient() && first_interaction_time_.is_null())

[Jered, 2012/11/27 19:21:57]

Please extract a method like IsCommitted() for loader_ == CurrentClient() and
client_ == CurrentClient().

[sreeram, 2012/11/29 07:33:19]

I've gone with the simpler "if (instant_tab_)" idiom everywhere. I think this
reads reasonably well ("if [there's] an instant tab, ...").

     first_interaction_time_ = base::Time::Now();
 
   // Allow search suggestions. In extended mode, SearchModeChanged() will set
@@ -296,7 +319,10 @@ bool InstantController::Update(const AutocompleteMatch& match,
   if (!extended_enabled_)
     search_mode_.mode = chrome::search::Mode::MODE_SEARCH_SUGGESTIONS;
 
-  loader_->Update(extended_enabled_ ? user_text : full_text, verbatim);
+  CurrentClient()->Update(extended_enabled_ ? user_text : full_text, verbatim);
+
+  if (loader_ == CurrentClient())
+    loader_->DidNavigate(history::HistoryAddPageArgs());
 
   content::NotificationService::current()->Notify(
       chrome::NOTIFICATION_INSTANT_CONTROLLER_UPDATED,
@@ -338,9 +364,6 @@ void InstantController::HandleAutocompleteResults(
   if (!extended_enabled_)
     return;
 

[sky, 2012/11/27 01:07:51]

Shouldn't we early out if no client?

[sreeram, 2012/11/29 07:33:19]

Done.

-  if (!GetPreviewContents())
-    return;
-
   DVLOG(1) << "AutocompleteResults:";
   std::vector<InstantAutocompleteResult> results;
   for (ACProviders::const_iterator provider = providers.begin();
@@ -360,18 +383,20 @@ void InstantController::HandleAutocompleteResults(
     }
   }
 
-  loader_->SendAutocompleteResults(results);
+  if (InstantClient* client = CurrentClient())

[samarth, 2012/11/27 18:09:43]

nit: does the Chrome style guide allow this? I haven't seen assignment in
conditionals elsewhere.

[sreeram, 2012/11/29 07:33:19]

The style guide doesn't mention it. @sky hates it. I like this style, but am
cognizant that I might be overdoing it. I think it helps in some cases; for
example, see InstantLoader::SetupPreviewContents().

+    client->SendAutocompleteResults(results);
 }
 
 bool InstantController::OnUpOrDownKeyPressed(int count) {
   if (!extended_enabled_)
     return false;
 
-  if (!GetPreviewContents())
-    return false;
+  if (InstantClient* client = CurrentClient()) {
+    client->UpOrDownKeyPressed(count);
+    return true;
+  }
 
-  loader_->OnUpOrDownKeyPressed(count);
-  return true;
+  return false;
 }
 
 TabContents* InstantController::GetPreviewContents() const {
@@ -386,11 +411,24 @@ bool InstantController::CommitIfCurrent(InstantCommitType type) {
   if (!extended_enabled_ && !instant_enabled_)
     return false;
 
+  if (client_ == CurrentClient() && last_match_was_search_ &&

[Jered, 2012/11/27 19:21:57]

I am very confused. Why doesn't this cancel sometimes? What is the business
about loader_ below?

[sreeram, 2012/11/29 07:33:19]

Cancel() isn't applicable in the case of a committed tab. I've added a short
comment about why this block exists here; let me know if it is still unclear.

+      type == INSTANT_COMMIT_PRESSED_ENTER) {
+    client_->Submit(last_full_text_);
+    return true;
+  }
+
   if (!IsCurrent())
     return false;
 
   DVLOG(1) << "CommitIfCurrent";
-  TabContents* preview = loader_->ReleasePreviewContents(type, last_full_text_);
+
+  if (type == INSTANT_COMMIT_FOCUS_LOST)
+    loader_->Cancel(last_full_text_);
+  else
+    loader_->Submit(last_full_text_);
+  loader_->CleanupPreviewContents();
+
+  TabContents* preview = loader_->release_preview_contents();
 
   if (extended_enabled_) {
     // Consider what's happening:
@@ -494,7 +532,7 @@ void InstantController::OmniboxLostFocus(gfx::NativeView view_gaining_focus) {
     Hide(true);
 #else
   if (IsViewInContents(GetViewGainingFocus(view_gaining_focus),
-                       GetPreviewContents()->web_contents()))
+                       loader_->contents()))
     CommitIfCurrent(INSTANT_COMMIT_FOCUS_LOST);
   else
     Hide(true);
@@ -508,7 +546,7 @@ void InstantController::OmniboxGotFocus() {
   if (!extended_enabled_ && !instant_enabled_)
     return;
 
-  if (!GetPreviewContents())
+  if (!client_ && !loader_)
     CreateDefaultLoader();
 }
 
@@ -531,8 +569,10 @@ void InstantController::SearchModeChanged(
     Hide(true);
   }
 
-  if (GetPreviewContents())
+  if (loader_)
     loader_->SearchModeChanged(new_mode);
+
+  ResetClient();
 }
 
 void InstantController::ActiveTabChanged() {
@@ -548,6 +588,9 @@ void InstantController::ActiveTabChanged() {
   if (search_mode_.is_search_suggestions() &&
       model_.mode().is_search_suggestions())
     Hide(false);
+
+  if (extended_enabled_)
+    ResetClient();
 }
 
 void InstantController::SetInstantEnabled(bool instant_enabled) {
@@ -560,7 +603,7 @@ void InstantController::ThemeChanged(const ThemeBackgroundInfo& theme_info) {
   if (!extended_enabled_)
     return;
 
-  if (GetPreviewContents())
+  if (loader_)
     loader_->SendThemeBackgroundInfo(theme_info);
 }
 
@@ -568,15 +611,15 @@ void InstantController::ThemeAreaHeightChanged(int height) {
   if (!extended_enabled_)
     return;
 
-  if (GetPreviewContents())
+  if (loader_)
     loader_->SendThemeAreaHeight(height);
 }
 
 void InstantController::SetSuggestions(
-    InstantLoader* loader,
+    InstantClient* client,
     const std::vector<InstantSuggestion>& suggestions) {
   DVLOG(1) << "SetSuggestions";
-  if (loader_ != loader || !search_mode_.is_search_suggestions())
+  if (client != CurrentClient() || !search_mode_.is_search_suggestions())
     return;
 
   InstantSuggestion suggestion;
@@ -659,8 +702,18 @@ void InstantController::ShowInstantPreview(InstantLoader* loader,
     Show(reason, height, units);
 }
 
-void InstantController::InstantSupportDetermined(InstantLoader* loader,
+void InstantController::InstantSupportDetermined(InstantClient* client,
                                                  bool supports_instant) {
+  if (client_ == client) {
+    if (!supports_instant) {
+      // TODO(sreeram): This should be scheduled for later deletion, but we need
+      // to disconnect the observer before doing so.
+      client_.reset();
+    }
+    return;
+  }
+
+  InstantLoader* loader = static_cast<InstantLoader*>(client);
   if (supports_instant) {
     blacklisted_urls_.erase(loader->instant_url());
   } else {
@@ -689,16 +742,34 @@ void InstantController::InstantLoaderContentsFocused(InstantLoader* loader) {
 #endif
 }
 
+void InstantController::ResetClient() {
+  if (search_mode_.is_origin_search()) {
+    content::WebContents* contents =
+        browser_->GetActiveTabContents()->web_contents();
+    if (!client_ || contents != client_->contents()) {
+      client_.reset(new InstantClient(this, contents));
+      client_->DetermineIfPageSupportsInstant();
+    }
+    // We are now using |client_| instead of |loader_|, so Hide() the latter. We
+    // want to call Hide(true) to clear old query results on the |loader_|, but
+    // that would also clear |last_full_text_|, which is bad if the user then
+    // immediately tries to commit the query on |client_|.
+    Hide(!search_mode_.is_search_suggestions());
+  } else {
+    client_.reset();
+  }
+}
+
 bool InstantController::ResetLoader(const TemplateURL* template_url,
                                     const TabContents* active_tab) {
   std::string instant_url;
   if (!GetInstantURL(template_url, &instant_url))
     return false;
 
-  if (GetPreviewContents() && loader_->instant_url() != instant_url)
+  if (loader_ && loader_->instant_url() != instant_url)
     DeleteLoader();
 
-  if (!GetPreviewContents()) {
+  if (!loader_) {
     loader_.reset(new InstantLoader(this, instant_url, active_tab));
     loader_->Init();
 
@@ -735,7 +806,7 @@ void InstantController::OnStaleLoader() {
   // If the preview is showing or the omnibox has focus, don't delete the
   // loader. It will get refreshed the next time the preview is hidden or the
   // omnibox loses focus.
-  if (!stale_loader_timer_.IsRunning() && !is_omnibox_focused_ &&
+  if (!stale_loader_timer_.IsRunning() && !is_omnibox_focused_ && !client_ &&
       model_.mode().is_default()) {
     DeleteLoader();
     CreateDefaultLoader();
@@ -796,6 +867,9 @@ void InstantController::Hide(bool clear_query) {
 void InstantController::Show(InstantShownReason reason,
                              int height,
                              InstantSizeUnits units) {
+  if (client_ == CurrentClient())
+    return;
+
   DVLOG(1) << "Show: reason=" << reason << " height=" << height << " units="
            << units;
 
@@ -819,8 +893,8 @@ void InstantController::Show(InstantShownReason reason,
 }
 
 void InstantController::SendBoundsToPage() {
-  if (last_omnibox_bounds_ == omnibox_bounds_ ||
-      !GetPreviewContents() || loader_->IsPointerDownFromActivate())
+  if (last_omnibox_bounds_ == omnibox_bounds_ || !loader_ ||
+      loader_->IsPointerDownFromActivate())
     return;
 
   last_omnibox_bounds_ = omnibox_bounds_;
@@ -896,3 +970,7 @@ bool InstantController::GetInstantURL(const TemplateURL* template_url,
 
   return true;
 }
+
+InstantClient* InstantController::CurrentClient() const {
+  return client_ ? client_.get() : loader_.get();
+}