InstantExtended: don't reset InstantTab if not ready.

chrome/browser/ui/search/instant_controller.cc

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/search/instant_controller.h"
 
 #include "base/metrics/histogram.h"
 #include "base/string_util.h"
 #include "base/stringprintf.h"
 #include "base/utf_string_conversions.h"
@@ skipping 222 matching lines @@
 
 }  // namespace
 
 InstantController::InstantController(BrowserInstantController* browser,
                                      bool extended_enabled)
     : browser_(browser),
       extended_enabled_(extended_enabled),
       instant_enabled_(false),
       use_local_page_only_(true),
       model_(this),
+      use_tab_for_suggestions_(false),
       last_omnibox_text_has_inline_autocompletion_(false),
       last_verbatim_(false),
       last_transition_type_(content::PAGE_TRANSITION_LINK),
       last_match_was_search_(false),
       omnibox_focus_state_(OMNIBOX_FOCUS_NONE),
       omnibox_bounds_(-1, -1, 0, 0),
       allow_overlay_to_show_search_suggestions_(false),
       weak_ptr_factory_(this) {
 
   // When the InstantController lives, the InstantService should live.
   // InstantService sets up profile-level facilities such as the ThemeSource for
   // the NTP.
   InstantServiceFactory::GetForProfile(browser_->profile());
 }
 
 InstantController::~InstantController() {
 }
 
 void InstantController::OnAutocompleteStart() {
   if (instant_tab_ && instant_tab_->supports_instant()) {
     LOG_INSTANT_DEBUG_EVENT(
         this, "OnAutocompleteStart: using InstantTab");
     return;

[sreeram, 2013/05/05 19:44:42]

I wrote this long comment below, but then realized that line 362 does cause
HideInternal() to be called, so all's well.

<obsolete>
I think you should set use_tab_for_suggestions_ = true here.

Generally speaking, whenever the overlay is showing, and it gets hidden later,
you set use_tab_for_suggestions_ = true. However, if the overlay never shows,
but gets deleted, we may end up in limbo.

Consider: You hit Ctrl-N (new window). instant_tab_ is set to non-NULL (local
NTP). overlay_ has just started loading the server page. Type a char quickly.
Line 269 below sets use_tab_for_suggestions_ = false. The instant_tab_ finishes
loading, and supports instant. The overlay_ finishes loading and does NOT
support instant. So, InstantSupportDetermined() sets overlay_ to NULL. Now, you
can keep typing as slowly as you want, but keystrokes are sent to neither,
because we'll keep hitting this return statement here (so
use_tab_for_suggestions_ is always false, but we never recreate the overlay).
Until finally you focus away from the omnibox or Backspace away or Escape or do
something else to cause HideInternal() to be called. (If possible, please write
a test to catch this scenario. And verify that your current patch fails, and
that after the fix, it passes.)

So, in general, either do the "if (instant_tab_) use_tab_for_suggestions_ =
true;"  not only when hiding the overlay, but also whenever we do
overlay_.reset(), or just set "use_tab_for_suggestions_ = true" here.
</obsolete>

It also turns out that we always do "if (!UseTabForSuggestions() && !overlay_)
return;" in all the critical places, so we won't end up dereferencing a NULL
overlay_ as per the scenario above.

[samarth, 2013/05/05 21:00:08]

Ack. Yeah this is kind of ugly but works.  I'll rip this out this week and move
more stuff to SearchTabHelper.

   }
 
-  if (instant_tab_) {
-    // If we have an |instant_tab_| but it doesn't support Instant yet, sever
-    // the connection to it so we use the overlay instead. This ensures that the
-    // user interaction will be responsive and handles cases where
-    // |instant_tab_| never responds about whether it supports Instant.
-    instant_tab_.reset();
-    LOG_INSTANT_DEBUG_EVENT(
-        this, "OnAutocompleteStart: reset InstantTab");
-  }
+  use_tab_for_suggestions_ = false;
 
   // Not using |instant_tab_|. Check if overlay is OK to use.
   if (ShouldSwitchToLocalOverlay()) {
     ResetOverlay(GetLocalInstantURL());
     LOG_INSTANT_DEBUG_EVENT(
         this, "OnAutocompleteStart: switching to local overlay");
   } else {
     LOG_INSTANT_DEBUG_EVENT(
         this, "OnAutocompleteStart: using existing overlay");
   }
@@ skipping 20 matching lines @@
       UTF16ToUTF8(user_text).c_str(), UTF16ToUTF8(full_text).c_str(),
       static_cast<int>(selection_start), static_cast<int>(selection_end),
       verbatim, user_input_in_progress, omnibox_popup_is_open, escape_pressed,
       is_keyword_search));
 
   // TODO(dhollowa): Complete keyword match UI.  For now just hide suggestions.
   // http://crbug.com/153932.  Note, this early escape is happens prior to the
   // DCHECKs below because |user_text| and |full_text| have different semantics
   // when keyword search is in effect.
   if (is_keyword_search) {
-    if (instant_tab_)
+    if (UseTabForSuggestions())
       instant_tab_->Update(string16(), 0, 0, true);
     else
       HideOverlay();
     last_match_was_search_ = false;
     return false;
   }
 
   // Ignore spurious updates when the omnibox is blurred; otherwise click
   // targets on the page may vanish before a click event arrives.
   if (omnibox_focus_state_ == OMNIBOX_FOCUS_NONE)
@@ skipping 30 matching lines @@
                            !user_text.empty();
 
   // In non extended mode, Instant is disabled for URLs and keyword mode.
   if (!extended_enabled_ &&
       (!last_match_was_search_ ||
        match.type == AutocompleteMatch::SEARCH_OTHER_ENGINE)) {
     HideOverlay();
     return false;
   }
 
-  if (!instant_tab_ && !overlay_) {
+  if (!UseTabForSuggestions() && !overlay_) {
     HideOverlay();
     return false;
   }
 
   if (extended_enabled_) {
     if (!omnibox_popup_is_open) {
       if (!user_input_in_progress) {
         // If the user isn't typing and the omnibox popup is closed, it means a
         // regular navigation, tab-switch or the user hitting Escape.
-        if (instant_tab_) {
+        if (UseTabForSuggestions()) {
           // The user is on a search results page. It may be showing results for
           // a partial query the user typed before they hit Escape. Send the
           // omnibox text to the page to restore the original results.
           //
           // In a tab switch, |instant_tab_| won't have updated yet, so it may
           // be pointing to the previous tab (which was a search results page).
           // Ensure we don't send the omnibox text to a random webpage (the new
           // tab), by comparing the old and new WebContents.
           if (escape_pressed &&
               instant_tab_->contents() == browser_->GetActiveWebContents()) {
@@ skipping 12 matching lines @@
           // If |full_text| is empty, the user is on the NTP. The overlay may
           // be showing custom NTP content; hide only if that's not the case.
           HideOverlay();
         }
       } else if (full_text.empty()) {
         // The user is typing, and backspaced away all omnibox text. Clear
         // |last_omnibox_text_| so that we don't attempt to set suggestions.
         last_omnibox_text_.clear();
         last_user_text_.clear();
         last_suggestion_ = InstantSuggestion();
-        if (instant_tab_) {
+        if (UseTabForSuggestions()) {
           // On a search results page, tell it to clear old results.
           instant_tab_->Update(string16(), 0, 0, true);
         } else if (search_mode_.is_origin_ntp()) {
           // On the NTP, tell the overlay to clear old results. Don't hide the
           // overlay so it can show a blank page or logo if it wants.
           overlay_->Update(string16(), 0, 0, true);
         } else {
           HideOverlay();
         }
       } else {
         // The user switched to a tab with partial text already in the omnibox.
         HideOverlay();
 
         // The new tab may or may not be a search results page; we don't know
         // since SearchModeChanged() hasn't been called yet. If it later turns
         // out to be, we should store |full_text| now, so that if the user hits
         // Enter, we'll send the correct query to instant_tab_->Submit(). If the
         // partial text is not a query (|last_match_was_search_| is false), we
         // won't Submit(), so no need to worry about that.
         last_omnibox_text_ = full_text;
         last_user_text_ = user_text;
         last_suggestion_ = InstantSuggestion();
       }
       return false;
     } else if (full_text.empty()) {
       // The user typed a solitary "?". Same as the backspace case above.
       last_omnibox_text_.clear();
       last_user_text_.clear();
       last_suggestion_ = InstantSuggestion();
-      if (instant_tab_)
+      if (UseTabForSuggestions())
         instant_tab_->Update(string16(), 0, 0, true);
       else if (search_mode_.is_origin_ntp())
         overlay_->Update(string16(), 0, 0, true);
       else
         HideOverlay();
       return false;
     }
   } else if (!omnibox_popup_is_open || full_text.empty()) {
     // In the non-extended case, hide the overlay as long as the user isn't
     // actively typing a non-empty query.
@@ skipping 34 matching lines @@
   last_verbatim_ = verbatim;
 
   last_transition_type_ = match.transition;
   url_for_history_ = match.destination_url;
 
   // Allow search suggestions. In extended mode, SearchModeChanged() will set
   // this, but it's not called in non-extended mode, so fake it.
   if (!extended_enabled_)
     search_mode_.mode = SearchMode::MODE_SEARCH_SUGGESTIONS;
 
-  if (instant_tab_) {
+  if (UseTabForSuggestions()) {
     instant_tab_->Update(user_text, selection_start, selection_end, verbatim);
   } else {
     if (first_interaction_time_.is_null())
       first_interaction_time_ = base::Time::Now();
     allow_overlay_to_show_search_suggestions_ = true;
 
     overlay_->Update(extended_enabled_ ? user_text : full_text,
                      selection_start, selection_end, verbatim);
   }
 
@@ skipping 69 matching lines @@
     ntp_->SetOmniboxBounds(omnibox_bounds_);
   if (instant_tab_)
     instant_tab_->SetOmniboxBounds(omnibox_bounds_);
 }
 
 void InstantController::HandleAutocompleteResults(
     const std::vector<AutocompleteProvider*>& providers) {
   if (!extended_enabled_)
     return;
 
-  if (!instant_tab_ && !overlay_)
+  if (!UseTabForSuggestions() && !overlay_)
     return;
 
   // The omnibox sends suggestions when its possibly imaginary popup closes
   // as it stops autocomplete. Ignore these.
   if (omnibox_focus_state_ == OMNIBOX_FOCUS_NONE)
     return;
 
   DVLOG(1) << "AutocompleteResults:";
   std::vector<InstantAutocompleteResult> results;
   for (ACProviders::const_iterator provider = providers.begin();
@@ skipping 31 matching lines @@
                << result.provider << " " << result.destination_url << " '"
                << result.description << "' '" << result.search_query << "' "
                << result.transition;
       results.push_back(result);
     }
   }
   LOG_INSTANT_DEBUG_EVENT(this, base::StringPrintf(
       "HandleAutocompleteResults: total_results=%d",
       static_cast<int>(results.size())));
 
-  if (instant_tab_)
+  if (UseTabForSuggestions())
     instant_tab_->SendAutocompleteResults(results);
   else
     overlay_->SendAutocompleteResults(results);
 }
 
 void InstantController::OnDefaultSearchProviderChanged() {
   if (ntp_ && extended_enabled_) {
     ntp_.reset();
     if (!use_local_page_only_)
       ResetNTP(GetInstantURL());
   }
 
   // Do not reload the overlay if it's actually the local overlay.
   if (overlay_ && !overlay_->IsLocal()) {
     overlay_.reset();
     if (extended_enabled_ || instant_enabled_) {
       // Try to create another overlay immediately so that it is ready for the
       // next user interaction.
       ResetOverlay(GetInstantURL());
     }
   }
 }
 
 bool InstantController::OnUpOrDownKeyPressed(int count) {
   if (!extended_enabled_)
     return false;
 
-  if (!instant_tab_ && !overlay_)
+  if (!UseTabForSuggestions() && !overlay_)
     return false;
 
-  if (instant_tab_)
+  if (UseTabForSuggestions())
     instant_tab_->UpOrDownKeyPressed(count);
   else
     overlay_->UpOrDownKeyPressed(count);
 
   return true;
 }
 
 void InstantController::OnCancel(const AutocompleteMatch& match,
                                  const string16& user_text,
                                  const string16& full_text) {
   if (!extended_enabled_)
     return;
 
-  if (!instant_tab_ && !overlay_)
+  if (!UseTabForSuggestions() && !overlay_)
     return;
 
   // We manually reset the state here since the JS is not expected to do it.
   // TODO(sreeram): Handle the case where user_text is now a URL
   last_match_was_search_ = AutocompleteMatch::IsSearchType(match.type) &&
                            !full_text.empty();
   last_omnibox_text_ = full_text;
   last_user_text_ = user_text;
   last_suggestion_ = InstantSuggestion();
 
   // Say |full_text| is "amazon.com" and |user_text| is "ama". This means the
   // inline autocompletion is "zon.com"; so the selection should span from
   // user_text.size() to full_text.size(). The selection bounds are inverted
   // because the caret is at the end of |user_text|, not |full_text|.
-  if (instant_tab_) {
+  if (UseTabForSuggestions()) {
     instant_tab_->CancelSelection(user_text, full_text.size(), user_text.size(),
                                   last_verbatim_);
   } else {
     overlay_->CancelSelection(user_text, full_text.size(), user_text.size(),
                               last_verbatim_);
   }
 }
 
 void InstantController::OmniboxNavigateToURL() {
   if (!extended_enabled_)
     return;
   RecordNavigationHistogram(UsingLocalPage(), false);
-  if (instant_tab_)
+  if (UseTabForSuggestions())
     instant_tab_->Submit(string16());
 }
 
 content::WebContents* InstantController::GetOverlayContents() const {
   return overlay_ ? overlay_->contents() : NULL;
 }
 
 bool InstantController::IsOverlayingSearchResults() const {
   return model_.mode().is_search_suggestions() && IsFullHeight(model_) &&
          (last_match_was_search_ ||
           last_suggestion_.behavior == INSTANT_COMPLETE_NEVER);
 }
 
 bool InstantController::CommitIfPossible(InstantCommitType type) {
   if (!extended_enabled_ && !instant_enabled_)
     return false;
 
   LOG_INSTANT_DEBUG_EVENT(this, base::StringPrintf(
       "CommitIfPossible: type=%d last_omnibox_text_='%s' "
-      "last_match_was_search_=%d instant_tab_=%d", type,
+      "last_match_was_search_=%d use_tab_for_suggestions=%d", type,
       UTF16ToUTF8(last_omnibox_text_).c_str(), last_match_was_search_,
-      instant_tab_ != NULL));
+      UseTabForSuggestions()));
 
   // If we are on an already committed search results page, send a submit event
   // to the page, but otherwise, nothing else to do.
-  if (instant_tab_) {
+  if (UseTabForSuggestions()) {
     if (type == INSTANT_COMMIT_PRESSED_ENTER &&
         !instant_tab_->IsLocal() &&
         (last_match_was_search_ ||
          last_suggestion_.behavior == INSTANT_COMPLETE_NEVER)) {
       last_suggestion_.text.clear();
       instant_tab_->Submit(last_omnibox_text_);
       instant_tab_->contents()->GetView()->Focus();
       EnsureSearchTermsAreSet(instant_tab_->contents(), last_omnibox_text_);
       return true;
     }
@@ skipping 104 matching lines @@
   // Hide explicitly. See comments in HideOverlay() for why.
   model_.SetOverlayState(SearchMode(), 0, INSTANT_SIZE_PERCENT);
 
   // Delay deletion as we could've gotten here from an InstantOverlay method.
   DeletePageSoon(overlay_.Pass());
 
   // Try to create another overlay immediately so that it is ready for the next
   // user interaction.
   ResetOverlay(GetInstantURL());
 
+  if (instant_tab_)
+    use_tab_for_suggestions_ = true;
+
   LOG_INSTANT_DEBUG_EVENT(this, "Committed");
   return true;
 }
 
 void InstantController::OmniboxFocusChanged(
     OmniboxFocusState state,
     OmniboxFocusChangeReason reason,
     gfx::NativeView view_gaining_focus) {
   LOG_INSTANT_DEBUG_EVENT(this, base::StringPrintf(
       "OmniboxFocusChanged: %d to %d for reason %d", omnibox_focus_state_,
@@ skipping 327 matching lines @@
 }
 
 void InstantController::SetSuggestions(
     const content::WebContents* contents,
     const std::vector<InstantSuggestion>& suggestions) {
   LOG_INSTANT_DEBUG_EVENT(this, "SetSuggestions");
 
   // Ignore if the message is from an unexpected source.
   if (IsContentsFrom(ntp(), contents))
     return;
-  if (instant_tab_ && !IsContentsFrom(instant_tab(), contents))
+  if (UseTabForSuggestions() && !IsContentsFrom(instant_tab(), contents))
     return;
   if (IsContentsFrom(overlay(), contents) &&
       !allow_overlay_to_show_search_suggestions_)
     return;
 
   InstantSuggestion suggestion;
   if (!suggestions.empty())
     suggestion = suggestions[0];
 
   // TODO(samarth): allow InstantTabs to call SetSuggestions() from the NTP once
   // that is better supported.
-  bool can_use_instant_tab = instant_tab_ && search_mode_.is_search();
+  bool can_use_instant_tab = UseTabForSuggestions() &&
+      search_mode_.is_search();
   bool can_use_overlay = search_mode_.is_search_suggestions() &&
       !last_omnibox_text_.empty();
   if (!can_use_instant_tab && !can_use_overlay)
     return;
 
   if (suggestion.behavior == INSTANT_COMPLETE_REPLACE) {
     // We don't get an Update() when changing the omnibox due to a REPLACE
     // suggestion (so that we don't inadvertently cause the overlay to change
     // what it's showing, as the user arrows up/down through the page-provided
     // suggestions). So, update these state variables here.
@@ skipping 221 matching lines @@
       instant_tab_.reset(new InstantTab(this));
       instant_tab_->Init(active_tab);
       // Update theme info for this tab.
       browser_->UpdateThemeInfo();
       instant_tab_->SetDisplayInstantResults(instant_enabled_);
       instant_tab_->SetOmniboxBounds(omnibox_bounds_);
       instant_tab_->InitializeFonts();
       StartListeningToMostVisitedChanges();
       instant_tab_->KeyCaptureChanged(
           omnibox_focus_state_ == OMNIBOX_FOCUS_INVISIBLE);
+      use_tab_for_suggestions_ = true;
     }
 
     // Hide the |overlay_| since we are now using |instant_tab_| instead.
     HideOverlay();
   } else {
     instant_tab_.reset();
   }
 }
 
 void InstantController::HideOverlay() {
@@ skipping 11 matching lines @@
   if (GetOverlayContents()) {
     model_.SetOverlayState(SearchMode(), 0, INSTANT_SIZE_PERCENT);
     allow_overlay_to_show_search_suggestions_ = false;
 
     // Send a message asking the overlay to clear out old results.
     overlay_->Update(string16(), 0, 0, true);
   }
 
   // Clear the first interaction timestamp for later use.
   first_interaction_time_ = base::Time();
+
+  if (instant_tab_)
+    use_tab_for_suggestions_ = true;
 }
 
 void InstantController::ShowOverlay(int height, InstantSizeUnits units) {
   // If we are on a committed search results page, the |overlay_| is not in use.
-  if (instant_tab_)
+  if (UseTabForSuggestions())
     return;
 
   LOG_INSTANT_DEBUG_EVENT(this, base::StringPrintf(
       "Show: height=%d units=%d", height, units));
 
   // Must have updated omnibox after the last HideOverlay() to show suggestions.
   if (!allow_overlay_to_show_search_suggestions_)
     return;
 
   // The page is trying to hide itself. Hide explicitly (i.e., don't use
   // HideOverlay()) so that it can change its mind.
   if (height == 0) {
     model_.SetOverlayState(SearchMode(), 0, INSTANT_SIZE_PERCENT);
+    if (instant_tab_)
+      use_tab_for_suggestions_ = true;
     return;
   }
 
   // If the overlay 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);
   }
 
@@ skipping 160 matching lines @@
       // suggest nothing.
       // TODO(samarth|jered): revisit this logic. http://crbug.com/196572.
       return true;
     }
   }
 
   return false;
 }
 
 bool InstantController::UsingLocalPage() const {
-  return (instant_tab_ && instant_tab_->IsLocal()) ||
-      (!instant_tab_ && overlay_ && overlay_->IsLocal());
+  return (UseTabForSuggestions() && instant_tab_->IsLocal()) ||
+      (!UseTabForSuggestions() && overlay_ && overlay_->IsLocal());
 }
+
+bool InstantController::UseTabForSuggestions() const {
+  return instant_tab_ && use_tab_for_suggestions_;
+}