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 f3146a5d324ce0429e460109f88df97bf6c3657a..781873ea36ce721377b5c715221aab17daa0a86d 100644
--- a/chrome/browser/instant/instant_controller.cc
+++ b/chrome/browser/instant/instant_controller.cc
@@ -14,12 +14,12 @@
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/history/history_tab_helper.h"
 #include "chrome/browser/instant/instant_loader.h"
+#include "chrome/browser/instant/instant_tab.h"
 #include "chrome/browser/platform_util.h"
 #include "chrome/browser/search_engines/template_url_service.h"
 #include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "chrome/browser/ui/browser_instant_controller.h"
 #include "chrome/browser/ui/search/search_tab_helper.h"
-#include "chrome/browser/ui/tab_contents/tab_contents.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/chrome_switches.h"
 #include "content/public/browser/navigation_entry.h"
@@ -48,16 +48,16 @@ const int kMaxInstantSupportFailures = 10;
 const int kStaleLoaderTimeoutMS = 3 * 3600 * 1000;
 
 void AddSessionStorageHistogram(bool extended_enabled,
-                                const TabContents* tab1,
-                                const TabContents* tab2) {
+                                const content::WebContents* tab1,
+                                const content::WebContents* tab2) {
   base::Histogram* histogram = base::BooleanHistogram::FactoryGet(
       std::string("Instant.SessionStorageNamespace") +
           (extended_enabled ? "_Extended" : "_Instant"),
       base::Histogram::kUmaTargetedHistogramFlag);
   const content::SessionStorageNamespaceMap& session_storage_map1 =
-      tab1->web_contents()->GetController().GetSessionStorageNamespaceMap();
+      tab1->GetController().GetSessionStorageNamespaceMap();
   const content::SessionStorageNamespaceMap& session_storage_map2 =
-      tab2->web_contents()->GetController().GetSessionStorageNamespaceMap();
+      tab2->GetController().GetSessionStorageNamespaceMap();
   bool is_session_storage_the_same =
       session_storage_map1.size() == session_storage_map2.size();
   if (is_session_storage_the_same) {
@@ -90,7 +90,7 @@ string16 Normalize(const string16& str) {
 }
 
 bool NormalizeAndStripPrefix(string16* text, const string16& prefix) {
-  const string16 norm_prefix = Normalize(prefix);
+  string16 norm_prefix = Normalize(prefix);

[samarth, 2012/12/03 19:46:03]

FWIW, I find this kind of const usage useful (as opposed to say, const bool
function parameters) but your call.

[sreeram, 2012/12/04 08:10:52]

Ack. I do vacillate between too much and too little const, but for now, I'll
keep my change.

   string16 norm_text = Normalize(*text);
   if (norm_prefix.size() <= norm_text.size() &&
       norm_text.compare(0, norm_prefix.size(), norm_prefix) == 0) {
@@ -111,9 +111,8 @@ gfx::NativeView GetViewGainingFocus(gfx::NativeView view_gaining_focus) {
     views::FocusManager* focus_manager = widget->GetFocusManager();
     if (focus_manager && focus_manager->is_changing_focus() &&
         focus_manager->GetFocusedView() &&
-        focus_manager->GetFocusedView()->GetWidget()) {
+        focus_manager->GetFocusedView()->GetWidget())
       return focus_manager->GetFocusedView()->GetWidget()->GetNativeView();
-    }
   }
 #endif
   return view_gaining_focus;
@@ -161,9 +160,9 @@ InstantController::~InstantController() {
 bool InstantController::Update(const AutocompleteMatch& match,
                                const string16& user_text,
                                const string16& full_text,
-                               const bool verbatim,
-                               const bool user_input_in_progress,
-                               const bool omnibox_popup_is_open) {
+                               bool verbatim,
+                               bool user_input_in_progress,
+                               bool omnibox_popup_is_open) {
   if (!extended_enabled_ && !instant_enabled_)
     return false;
 
@@ -176,11 +175,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;
@@ -198,9 +198,34 @@ bool InstantController::Update(const AutocompleteMatch& match,
     search_mode_.mode = chrome::search::Mode::MODE_DEFAULT;
 
   // If there's no active tab, the browser is closing.
-  const TabContents* const active_tab = browser_->GetActiveTabContents();
+  const content::WebContents* active_tab = browser_->GetActiveWebContents();
   if (!active_tab) {
-    Hide(true);
+    Hide();
+    return false;
+  }
+
+  const TemplateURL* template_url = match.GetTemplateURL(
+      Profile::FromBrowserContext(active_tab->GetBrowserContext()), false);
+  bool match_is_search = AutocompleteMatch::IsSearchType(match.type);
+
+  // Ensure we have a loader or tab that can process this |match|.
+  if (extended_enabled_) {
+    // In extended mode, use the first among the following that succeeds:
+    // 1. A committed search results page.
+    // 2. An Instant loader for |template_url|.
+    // 3. If the |match| is a URL or a blank search query, the Instant loader
+    //    for the default search engine.
+    // TODO(sreeram): If |template_url| doesn't match the |instant_tab_| URL,
+    // we shouldn't blindly use the committed tab.
+    if (!instant_tab_ && !ResetLoader(template_url, active_tab) &&
+        ((match_is_search && !user_text.empty()) || !CreateDefaultLoader())) {

[Jered, 2012/11/29 19:57:05]

Should this check full_text.empty() as it used to?

[Jered, 2012/11/29 19:57:05]

It took me a second to unwind this if. Maybe
bool PrepareToUpdate(...) {
  if (instant_tab_)
    return true;
  if (!ResetLoader(template_url, active_tab))
    return false;
  if (!match_is_search || full_text.empty())
    return !CreateDefaultLoader();
  return true;
}

then if (!PrepareToUpdate(...)) ...

or fold in the other branch below

[sreeram, 2012/12/04 08:10:52]

No, user_text is the right thing here. If we are in case #2, full_text won't be
empty, but user_text will be, and match_is_search will be true. We don't want to
create a loader in that case.

Previously, full_text worked because case #2 never triggered (i.e., recall the
changes in this CL to use view_->GetText() instead of user_text +
inline_autocomplete_text_ in omnibox_edit_model.cc).

[sreeram, 2012/12/04 08:10:52]

Done.

+      Hide();
+      return false;
+    }
+  } else if (!ResetLoader(template_url, active_tab)) {
+    // In non-extended mode, if the TemplateURL of the |match| doesn't have a
+    // valid Instant URL, do not proceed.
+    Hide();
     return false;
   }
 
@@ -208,7 +233,7 @@ bool InstantController::Update(const AutocompleteMatch& match,
   //
   //  #  OPIO UIIP full_text  Notes
   //  -  ---- ---- ---------  -----
-  //  1   no   no    blank    } Navigation, or user hit Escape. |full_text| is
+  //  1   no   no    blank    } Navigation, tab-switch or Escape. |full_text| is
   //  2   no   no  non-blank  } blank if the page is NTP, non-blank otherwise.
   //
   //  3   no  yes    blank    User backspaced away all omnibox text.
@@ -226,37 +251,44 @@ 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();  // #2
+          // 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

[Jered, 2012/11/29 19:57:05]

What "original" results? Do you mean the committed results? I don't understand
why full_text is the correct query.

[sreeram, 2012/12/04 08:10:52]

I've tried to explain this better in the new patch. PTAL.

+          // search results page, SearchModeChanged() would've caught it, and
+          // |instant_tab_| will be NULL, so we won't accidentally send a URL
+          // to |instant_tab_|. Except if the user just switched tabs. Hence the
+          // comparison of WebContents, to catch that case as well.
+          if (instant_tab_ && instant_tab_->contents() == active_tab)
+            instant_tab_->Update(full_text, true);
+        }
+        return false;  // #1

[Jered, 2012/11/29 19:57:05]

Note this is both #1 and #2.

[sreeram, 2012/12/04 08:10:52]

Ack. See new patch.

+      } else {
+        if (!full_text.empty()) {
+          Hide();  // #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 instant_tab_->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())
-      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);
+    if (!loader_ || !loader_->is_pointer_down_from_activate())
+      Hide();
     return false;
   }
 
@@ -287,8 +319,9 @@ bool InstantController::Update(const AutocompleteMatch& match,
   last_match_was_search_ = match_is_search;
   url_for_history_ = match.destination_url;
 
-  // Store the first interaction time for use with latency histograms.
-  if (first_interaction_time_.is_null())
+  // Store the first interaction time for use with latency histograms (but only
+  // if we are talking to the |loader_| and not a committed tab).
+  if (!instant_tab_ && first_interaction_time_.is_null())
     first_interaction_time_ = base::Time::Now();
 
   // Allow search suggestions. In extended mode, SearchModeChanged() will set
@@ -296,7 +329,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);
+  if (instant_tab_)
+    instant_tab_->Update(user_text, verbatim);
+  else
+    loader_->Update(extended_enabled_ ? user_text : full_text, verbatim);
 
   content::NotificationService::current()->Notify(
       chrome::NOTIFICATION_INSTANT_CONTROLLER_UPDATED,
@@ -338,7 +374,7 @@ void InstantController::HandleAutocompleteResults(
   if (!extended_enabled_)
     return;
 
-  if (!GetPreviewContents())
+  if (!instant_tab_ && !loader_)
     return;
 
   DVLOG(1) << "AutocompleteResults:";
@@ -360,22 +396,29 @@ void InstantController::HandleAutocompleteResults(
     }
   }
 
-  loader_->SendAutocompleteResults(results);
+  if (instant_tab_)
+    instant_tab_->SendAutocompleteResults(results);
+  else
+    loader_->SendAutocompleteResults(results);
 }
 
 bool InstantController::OnUpOrDownKeyPressed(int count) {
   if (!extended_enabled_)
     return false;
 
-  if (!GetPreviewContents())
+  if (!instant_tab_ && !loader_)
     return false;
 
-  loader_->OnUpOrDownKeyPressed(count);
-  return true;
+  if (instant_tab_)
+    instant_tab_->UpOrDownKeyPressed(count);
+  else
+    loader_->UpOrDownKeyPressed(count);
+
+  return false;

[Jered, 2012/11/29 19:57:05]

Should this be return true;?

[sreeram, 2012/12/04 08:10:52]

Indeed. Done. ('Twas a typo.)

 }
 
-TabContents* InstantController::GetPreviewContents() const {
-  return loader_ ? loader_->preview_contents() : NULL;
+content::WebContents* InstantController::GetPreviewContents() const {
+  return loader_ ? loader_->contents() : NULL;
 }
 
 bool InstantController::IsCurrent() const {
@@ -386,11 +429,25 @@ bool InstantController::CommitIfCurrent(InstantCommitType type) {
   if (!extended_enabled_ && !instant_enabled_)
     return false;
 
+  // If we are on a committed search results page, send a submit event to the
+  // page, but otherwise, nothing else to do.

[Jered, 2012/11/29 19:57:05]

From the "nothing else to do" part, I would expect:
if (instant_tab_) {
  if (last_match_was_search_ && type == INSTANT_COMMIT_PRESSED_ENTER) {
    instant_tab->Submit(last_full_text_);
    return true;
  }
  return false;
}

[sreeram, 2012/12/04 08:10:52]

Done. It worked the way it was before, since IsCurrent() would return false, but
I've made it more explicit, the way you suggested.

+  if (instant_tab_ && last_match_was_search_ &&
+      type == INSTANT_COMMIT_PRESSED_ENTER) {
+    instant_tab_->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_);
+
+  content::WebContents* preview = loader_->ReleaseContents();
 
   if (extended_enabled_) {
     // Consider what's happening:
@@ -405,7 +462,7 @@ bool InstantController::CommitIfCurrent(InstantCommitType type) {
     // TODO(samarth,beaudoin): Instead of this hack, we should add a new field
     // to NavigationEntry to keep track of what the correct query, if any, is.
     content::NavigationEntry* entry =
-        preview->web_contents()->GetController().GetVisibleEntry();
+        preview->GetController().GetVisibleEntry();
     std::string url = entry->GetVirtualURL().spec();
     if (!google_util::IsInstantExtendedAPIGoogleSearchUrl(url) &&
         google_util::IsGoogleDomainUrl(url, google_util::ALLOW_SUBDOMAIN,
@@ -413,8 +470,8 @@ bool InstantController::CommitIfCurrent(InstantCommitType type) {
       entry->SetVirtualURL(GURL(
           url + "#q=" +
           net::EscapeQueryParamValue(UTF16ToUTF8(last_full_text_), true)));
-      chrome::search::SearchTabHelper::FromWebContents(
-          preview->web_contents())->NavigationEntryUpdated();
+      chrome::search::SearchTabHelper::FromWebContents(preview)->
+          NavigationEntryUpdated();
     }
   }
 
@@ -424,13 +481,12 @@ bool InstantController::CommitIfCurrent(InstantCommitType type) {
   const history::HistoryAddPageArgs& last_navigation =
       loader_->last_navigation();
   if (!last_navigation.url.is_empty()) {
-    content::NavigationEntry* entry =
-        preview->web_contents()->GetController().GetActiveEntry();
+    content::NavigationEntry* entry = preview->GetController().GetActiveEntry();
     DCHECK_EQ(last_navigation.url, entry->GetURL());
 
     // Add the page to history.
     HistoryTabHelper* history_tab_helper =
-        HistoryTabHelper::FromWebContents(preview->web_contents());
+        HistoryTabHelper::FromWebContents(preview);
     history_tab_helper->UpdateHistoryForNavigation(last_navigation);
 
     // Update the page title.
@@ -439,34 +495,38 @@ bool InstantController::CommitIfCurrent(InstantCommitType type) {
 
   // Add a fake history entry with a non-Instant search URL, so that search
   // terms extraction (for autocomplete history matches) works.
-  if (HistoryService* history = HistoryServiceFactory::GetForProfile(
-          preview->profile(), Profile::EXPLICIT_ACCESS)) {
+  HistoryService* history = HistoryServiceFactory::GetForProfile(
+      Profile::FromBrowserContext(preview->GetBrowserContext()),
+      Profile::EXPLICIT_ACCESS);
+  if (history) {
     history->AddPage(url_for_history_, base::Time::Now(), NULL, 0, GURL(),
                      history::RedirectList(), last_transition_type_,
                      history::SOURCE_BROWSED, false);
   }
 
-  preview->web_contents()->GetController().PruneAllButActive();
+  preview->GetController().PruneAllButActive();
 
   if (type != INSTANT_COMMIT_PRESSED_ALT_ENTER) {
-    const TabContents* active_tab = browser_->GetActiveTabContents();
+    content::WebContents* active_tab = browser_->GetActiveWebContents();
     AddSessionStorageHistogram(extended_enabled_, active_tab, preview);
-    preview->web_contents()->GetController().CopyStateFromAndPrune(
-        &active_tab->web_contents()->GetController());
+    preview->GetController().CopyStateFromAndPrune(
+        &active_tab->GetController());
   }
 
-  DeleteLoader();
-
   // Browser takes ownership of the preview.
   browser_->CommitInstant(preview, type == INSTANT_COMMIT_PRESSED_ALT_ENTER);
 
   content::NotificationService::current()->Notify(
       chrome::NOTIFICATION_INSTANT_COMMITTED,
-      content::Source<content::WebContents>(preview->web_contents()),
+      content::Source<content::WebContents>(preview),
       content::NotificationService::NoDetails());
 
+  // Hide explicitly. See comments in Hide() for why calling it won't work.
   model_.SetPreviewState(chrome::search::Mode(), 0, INSTANT_SIZE_PERCENT);
 
+  // Delay deletion as we could've gotten here from an InstantLoader method.
+  MessageLoop::current()->DeleteSoon(FROM_HERE, loader_.release());
+
   // Try to create another loader immediately so that it is ready for the next
   // user interaction.
   CreateDefaultLoader();
@@ -490,14 +550,14 @@ void InstantController::OmniboxLostFocus(gfx::NativeView view_gaining_focus) {
   }
 
 #if defined(OS_MACOSX)
-  if (!loader_->IsPointerDownFromActivate())
-    Hide(true);
+  if (!loader_->is_pointer_down_from_activate())
+    Hide();
 #else
   if (IsViewInContents(GetViewGainingFocus(view_gaining_focus),
-                       GetPreviewContents()->web_contents()))
+                       loader_->contents()))
     CommitIfCurrent(INSTANT_COMMIT_FOCUS_LOST);
   else
-    Hide(true);
+    Hide();
 #endif
 }
 
@@ -508,8 +568,7 @@ void InstantController::OmniboxGotFocus() {
   if (!extended_enabled_ && !instant_enabled_)
     return;
 
-  if (!GetPreviewContents())
-    CreateDefaultLoader();
+  CreateDefaultLoader();
 }
 
 void InstantController::SearchModeChanged(
@@ -525,14 +584,21 @@ void InstantController::SearchModeChanged(
 
   if (new_mode.is_search_suggestions()) {
     // The preview is showing NTP content, but it's not appropriate anymore.
-    if (model_.mode().is_ntp() && !new_mode.is_origin_ntp())
-      Hide(false);
+    if (model_.mode().is_ntp() && !new_mode.is_origin_ntp()) {
+      // Hide() clears |last_full_text_|. Restore it so that if the user just
+      // switched to a tab with a partial query, we don't lose it.
+      string16 text = last_full_text_;
+      Hide();
+      last_full_text_ = text;
+    }
   } else {
-    Hide(true);
+    Hide();
   }
 
-  if (GetPreviewContents())
+  if (loader_)
     loader_->SearchModeChanged(new_mode);
+
+  ResetInstantTab();
 }
 
 void InstantController::ActiveTabChanged() {
@@ -546,21 +612,31 @@ void InstantController::ActiveTabChanged() {
   // going from search_suggestions to search_suggestions (i.e., partial queries
   // on both old and new tabs).
   if (search_mode_.is_search_suggestions() &&
-      model_.mode().is_search_suggestions())
-    Hide(false);
+      model_.mode().is_search_suggestions()) {
+    // Hide() clears |last_full_text_|. Restore it so that if the user just
+    // switched to a tab with a partial query, we don't lose it.
+    string16 text = last_full_text_;
+    Hide();
+    last_full_text_ = text;
+  }
+
+  if (extended_enabled_)
+    ResetInstantTab();
 }
 
 void InstantController::SetInstantEnabled(bool instant_enabled) {
   instant_enabled_ = instant_enabled;
-  if (!extended_enabled_ && !instant_enabled_)
-    DeleteLoader();
+  HideInternal();
+  loader_.reset();
+  if (extended_enabled_ || instant_enabled_)
+    CreateDefaultLoader();
 }
 
 void InstantController::ThemeChanged(const ThemeBackgroundInfo& theme_info) {
   if (!extended_enabled_)
     return;
 
-  if (GetPreviewContents())
+  if (loader_)
     loader_->SendThemeBackgroundInfo(theme_info);
 }
 
@@ -568,15 +644,20 @@ void InstantController::ThemeAreaHeightChanged(int height) {
   if (!extended_enabled_)
     return;
 
-  if (GetPreviewContents())
+  if (loader_)
     loader_->SendThemeAreaHeight(height);
 }
 
 void InstantController::SetSuggestions(
-    InstantLoader* loader,
+    const content::WebContents* contents,
     const std::vector<InstantSuggestion>& suggestions) {
   DVLOG(1) << "SetSuggestions";
-  if (loader_ != loader || !search_mode_.is_search_suggestions())
+
+  // Ignore if we are not currently accepting search suggestions, or if they are
+  // coming from the wrong loader.
+  if (!search_mode_.is_search_suggestions() ||
+      (instant_tab_ && instant_tab_->contents() != contents) ||
+      (!instant_tab_ && loader_ && loader_->contents() != contents))

[samarth, 2012/12/03 19:46:03]

Yikes! The last two clauses appear all over the place--worth making utilities
for them for clarity? For example, IsLoaderActive() and IsTabActive()? (There's
probably better names you could use here.)

[sreeram, 2012/12/04 08:10:52]

Actually, it's just in two places - the only two callbacks that are applicable
to both InstantTab and InstantLoader, viz., SetSuggestions() and
InstantSupportDetermined(). Until this proliferates, I think it's simpler to
just keep them as they are.

The other similar checks, such as the ones in Update(), are conceptually
different. They are not checking whether instant_tab_ or loader_ match the
caller's contents; rather that the instant_tab_ contents match the active tab
contents. I think it might lead to more confusion if we tried to share code
across them.

     return;
 
   InstantSuggestion suggestion;
@@ -646,87 +727,88 @@ void InstantController::SetSuggestions(
   Show(INSTANT_SHOWN_QUERY_SUGGESTIONS, 100, INSTANT_SIZE_PERCENT);
 }
 
-void InstantController::CommitInstantLoader(InstantLoader* loader) {
-  if (loader_ == loader)
-    CommitIfCurrent(INSTANT_COMMIT_FOCUS_LOST);
+void InstantController::InstantSupportDetermined(
+    const content::WebContents* contents,
+    bool supports_instant) {
+  if (instant_tab_ && instant_tab_->contents() == contents) {
+    if (!supports_instant)

[Jered, 2012/11/29 19:57:05]

Wow, I hope not. Do we not want to update the blacklist in this case?

[sreeram, 2012/12/04 08:10:52]

No. The blacklist is to make sure that we are not causing unnecessary and
repeated page loads. In the case of a committed tab, we are not actually causing
any loads. We are just using what's already there. No resources wasted, so no
need to blacklist.

+      MessageLoop::current()->DeleteSoon(FROM_HERE, instant_tab_.release());
+    return;
+  }
+
+  if (loader_ && loader_->contents() == contents) {
+    if (supports_instant) {
+      blacklisted_urls_.erase(loader_->instant_url());
+    } else {
+      ++blacklisted_urls_[loader_->instant_url()];
+      HideInternal();
+      delete loader_->ReleaseContents();
+      MessageLoop::current()->DeleteSoon(FROM_HERE, loader_.release());
+      CreateDefaultLoader();

[Jered, 2012/11/29 19:57:05]

Is this CreateDefaultLoader() call new? I think this is safe because of
blacklisted_urls_, but initially I was worried this might result in an infinite
loop of creating and deleting the default loader... that can't happen, right?

[sreeram, 2012/12/04 08:10:52]

Yeah, the call is new. The infinite loop can't happen because of
kMaxInstantSupportFailures, but this code will eventually be overhauled when
http://crbug.com/143722 is fixed. So, I'll keep this change for now.

+    }
+    content::NotificationService::current()->Notify(
+        chrome::NOTIFICATION_INSTANT_SUPPORT_DETERMINED,
+        content::Source<InstantController>(this),
+        content::NotificationService::NoDetails());
+  }
 }
 
-void InstantController::ShowInstantPreview(InstantLoader* loader,
-                                           InstantShownReason reason,
+void InstantController::ShowInstantPreview(InstantShownReason reason,
                                            int height,
                                            InstantSizeUnits units) {
-  if (loader_ == loader && extended_enabled_)
+  if (extended_enabled_)
     Show(reason, height, units);
 }
 
-void InstantController::InstantSupportDetermined(InstantLoader* loader,
-                                                 bool supports_instant) {
-  if (supports_instant) {
-    blacklisted_urls_.erase(loader->instant_url());
-  } else {
-    ++blacklisted_urls_[loader->instant_url()];
-    if (loader_ == loader)
-      DeleteLoader();
-  }
-
-  content::NotificationService::current()->Notify(
-      chrome::NOTIFICATION_INSTANT_SUPPORT_DETERMINED,
-      content::Source<InstantController>(this),
-      content::NotificationService::NoDetails());
-}
-
-void InstantController::SwappedTabContents(InstantLoader* loader) {
-  if (loader_ == loader)
-    model_.SetPreviewContents(GetPreviewContents());
+void InstantController::SwappedWebContents() {
+  model_.SetPreviewContents(GetPreviewContents());
 }
 
-void InstantController::InstantLoaderContentsFocused(InstantLoader* loader) {
+void InstantController::InstantLoaderContentsFocused() {
 #if defined(USE_AURA)
   // On aura the omnibox only receives a focus lost if we initiate the focus
   // change. This does that.
-  if (loader_ == loader && !model_.mode().is_default())
+  if (!model_.mode().is_default())
     browser_->InstantPreviewFocused();
 #endif
 }
 
 bool InstantController::ResetLoader(const TemplateURL* template_url,
-                                    const TabContents* active_tab) {
+                                    const content::WebContents* active_tab) {
   std::string instant_url;
   if (!GetInstantURL(template_url, &instant_url))
     return false;
 
-  if (GetPreviewContents() && loader_->instant_url() != instant_url)
-    DeleteLoader();
-
-  if (!GetPreviewContents()) {
-    loader_.reset(new InstantLoader(this, instant_url, active_tab));
-    loader_->Init();
+  if (loader_ && loader_->instant_url() == instant_url)
+    return true;
 
-    // Ensure the searchbox API has the correct theme-related info and context.
-    if (extended_enabled_) {
-      browser_->UpdateThemeInfoForPreview();
-      loader_->SearchModeChanged(search_mode_);
-    }
+  HideInternal();
+  loader_.reset(new InstantLoader(this, instant_url));
+  loader_->InitContents(active_tab);
 
-    // Reset the loader timer.
-    stale_loader_timer_.Start(
-        FROM_HERE,
-        base::TimeDelta::FromMilliseconds(kStaleLoaderTimeoutMS), this,
-        &InstantController::OnStaleLoader);
+  // Ensure the searchbox API has the correct theme-related info and context.
+  if (extended_enabled_) {
+    browser_->UpdateThemeInfoForPreview();
+    loader_->SearchModeChanged(search_mode_);
   }
 
+  // Restart the stale loader timer.
+  stale_loader_timer_.Start(FROM_HERE,
+      base::TimeDelta::FromMilliseconds(kStaleLoaderTimeoutMS), this,
+      &InstantController::OnStaleLoader);
+
   return true;
 }
 
 bool InstantController::CreateDefaultLoader() {
   // If there's no active tab, the browser is closing.
-  const TabContents* active_tab = browser_->GetActiveTabContents();
+  const content::WebContents* active_tab = browser_->GetActiveWebContents();
   if (!active_tab)
     return false;
 
-  const TemplateURL* template_url =
-      TemplateURLServiceFactory::GetForProfile(active_tab->profile())->
-                                 GetDefaultSearchProvider();
+  const TemplateURL* template_url = TemplateURLServiceFactory::GetForProfile(
+      Profile::FromBrowserContext(active_tab->GetBrowserContext()))->
+      GetDefaultSearchProvider();
 
   return ResetLoader(template_url, active_tab);
 }
@@ -737,65 +819,59 @@ void InstantController::OnStaleLoader() {
   // omnibox loses focus.
   if (!stale_loader_timer_.IsRunning() && !is_omnibox_focused_ &&
       model_.mode().is_default()) {
-    DeleteLoader();
+    loader_.reset();
     CreateDefaultLoader();
   }
 }
 
-void InstantController::DeleteLoader() {
-  // Clear all state, except |last_transition_type_| as it's used during commit.
-  last_user_text_.clear();
-  last_full_text_.clear();
-  last_verbatim_ = false;
-  last_suggestion_ = InstantSuggestion();
-  last_match_was_search_ = false;
-  if (!extended_enabled_)
-    search_mode_.mode = chrome::search::Mode::MODE_DEFAULT;
-  omnibox_bounds_ = gfx::Rect();
-  last_omnibox_bounds_ = gfx::Rect();
-  update_bounds_timer_.Stop();
-  stale_loader_timer_.Stop();
-  url_for_history_ = GURL();
-  first_interaction_time_ = base::Time();
-  if (GetPreviewContents()) {
-    model_.SetPreviewState(chrome::search::Mode(), 0, INSTANT_SIZE_PERCENT);
-    loader_->CleanupPreviewContents();
+void InstantController::ResetInstantTab() {
+  if (search_mode_.is_origin_search()) {
+    content::WebContents* active_tab = browser_->GetActiveWebContents();
+    if (!instant_tab_ || active_tab != instant_tab_->contents())
+      instant_tab_.reset(new InstantTab(this, active_tab));
+
+    // We are now using |instant_tab_| instead of |loader_|, so Hide() the
+    // latter, but preserve |last_full_text_|.

[Jered, 2012/11/29 19:57:05]

This happens in 3 places. Perhaps extract a method.

[sreeram, 2012/12/04 08:10:52]

Ack. No longer repeated in the new patch.

+    string16 text = last_full_text_;
+    Hide();
+    last_full_text_ = text;
+  } else {
+    instant_tab_.reset();
   }
+}
 
-  // Schedule the deletion for later, since we may have gotten here from a call
-  // within a |loader_| method (i.e., it's still on the stack). If we deleted
-  // the loader immediately, things would still be fine so long as the caller
-  // doesn't access any instance members after we return, but why rely on that?
-  MessageLoop::current()->DeleteSoon(FROM_HERE, loader_.release());
+void InstantController::Hide() {
+  HideInternal();
+  OnStaleLoader();
 }
 
-void InstantController::Hide(bool clear_query) {
-  DVLOG(1) << "Hide: clear_query=" << clear_query;
+void InstantController::HideInternal() {
+  DVLOG(1) << "Hide";
 
-  // The only time when the preview is not already in the desired MODE_DEFAULT
-  // state and GetPreviewContents() returns NULL is when we are in the commit
-  // path. In that case, don't change the state just yet; otherwise we may
-  // cause the preview to hide unnecessarily. Instead, the state will be set
-  // correctly after the commit is done.
-  if (GetPreviewContents())
+  // If GetPreviewContents() returns NULL, either we're already in the desired
+  // MODE_DEFAULT state, or we're in the commit path. For the latter, don't
+  // change the state just yet; else we may hide the preview unnecessarily.
+  // Instead, the state will be set correctly after the commit is done.
+  if (GetPreviewContents()) {
     model_.SetPreviewState(chrome::search::Mode(), 0, INSTANT_SIZE_PERCENT);
 
-  // Clear the first interaction timestamp for later use.
-  first_interaction_time_ = base::Time();
-
-  if (clear_query) {
-    if (GetPreviewContents() && !last_full_text_.empty())
-      loader_->Update(string16(), true);
-    last_user_text_.clear();
-    last_full_text_.clear();
+    // Send a message asking the preview to clear out old results.
+    if (!last_full_text_.empty()) {
+      last_full_text_.clear();
+      loader_->Update(last_full_text_, true);
+    }
   }
 
-  OnStaleLoader();
+  // Clear the first interaction timestamp for later use.
+  first_interaction_time_ = base::Time();
 }
 
 void InstantController::Show(InstantShownReason reason,
                              int height,
                              InstantSizeUnits units) {
+  if (instant_tab_)
+    return;
+
   DVLOG(1) << "Show: reason=" << reason << " height=" << height << " units="
            << units;
 
@@ -808,8 +884,8 @@ void InstantController::Show(InstantShownReason reason,
       !search_mode_.is_search_suggestions())
     return;
 
-  // If the preview is being shown because of the first set of suggestions to
-  // arrive for this query editing session, record a histogram value.
+  // If the preview is being shown for the first time since the user started
+  // typing, record a histogram value.
   if (!first_interaction_time_.is_null() && model_.mode().is_default()) {
     base::TimeDelta delta = base::Time::Now() - first_interaction_time_;
     UMA_HISTOGRAM_TIMES("Instant.TimeToFirstShow", delta);
@@ -820,8 +896,8 @@ void InstantController::Show(InstantShownReason reason,
   // - The page wants to show custom NTP content.
   // - The page is over a website other than search or an NTP, and is not
   //   already showing at 100% height.
-  const bool is_full_height =
-      model_.height() == 100 && model_.height_units() == INSTANT_SIZE_PERCENT;
+  bool is_full_height = model_.height() == 100 &&
+                        model_.height_units() == INSTANT_SIZE_PERCENT;
   if (height == 0 ||
       reason == INSTANT_SHOWN_CUSTOM_NTP_CONTENT ||
       (search_mode_.is_origin_default() && !is_full_height))
@@ -831,8 +907,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_->is_pointer_down_from_activate())
     return;
 
   last_omnibox_bounds_ = omnibox_bounds_;
@@ -886,8 +962,8 @@ bool InstantController::GetInstantURL(const TemplateURL* template_url,
       return false;
 
     if (!url_obj.SchemeIsSecure()) {
-      const std::string new_scheme = "https";
-      const std::string new_port = "443";
+      std::string new_scheme = "https";

[Jered, 2012/11/29 19:57:05]

There is probably a constant for this somewhere.

[sreeram, 2012/12/04 08:10:52]

I'd think so too, but I couldn't find one!

+      std::string new_port = "443";

[Jered, 2012/11/29 19:57:05]

This too I'd expect.

[sreeram, 2012/12/04 08:10:52]

Likewise. (Note that this is worse, since it needs to be a string; 443 as an int
doesn't cut it.)

       GURL::Replacements secure;
       secure.SetSchemeStr(new_scheme);
       secure.SetPortStr(new_port);