InstantExtended: don't reset InstantTab if not ready.

InstantExtended: don't reset InstantTab if not ready.

Currently, we reset InstantTab if it's not ready by the time user starts
typing. With this change, we switch over to using the overlay, but leave the
tab connected so it has a chacne to finish loading. We'll try the tab again
when the overlay is hidden or committed.

BUG=236594
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=198460

[samarth, 2013-05-02 05:48:58]

This fixes the open-tab-and-type-quickly case.

Thanks,
Samarth

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

looks good; please add tests.

https://codereview.chromium.org/14843002/diff/3001/chrome/browser/ui/search/i...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/14843002/diff/3001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_controller.cc:266: return;
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]

Added test.  PTAL.  Thanks!
Samarth

https://codereview.chromium.org/14843002/diff/3001/chrome/browser/ui/search/i...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/14843002/diff/3001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_controller.cc:266: return;
On 2013/05/05 19:44:42, sreeram wrote:
> 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.

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

[sreeram, 2013-05-05 21:06:41]

https://codereview.chromium.org/14843002/diff/6002/chrome/browser/ui/search/i...
File chrome/browser/ui/search/instant_extended_interactive_uitest.cc (right):

https://codereview.chromium.org/14843002/diff/6002/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_extended_interactive_uitest.cc:2306:
instant()->instant_tab_->supports_instant_ = false;
Why do this? If you removed the observer and observer.Wait(), you should get the
same state, no?

[samarth, 2013-05-05 21:18:33]

https://codereview.chromium.org/14843002/diff/6002/chrome/browser/ui/search/i...
File chrome/browser/ui/search/instant_extended_interactive_uitest.cc (right):

https://codereview.chromium.org/14843002/diff/6002/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_extended_interactive_uitest.cc:2306:
instant()->instant_tab_->supports_instant_ = false;
On 2013/05/05 21:06:41, sreeram wrote:
> Why do this? If you removed the observer and observer.Wait(), you should get
the
> same state, no?

Ah yes, that's better.  Done.

[sreeram, 2013-05-05 21:24:07]

lgtm

[commit-bot: I haz the power, 2013-05-05 21:24:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/samarth@chromium.org/14843002/11001

[commit-bot: I haz the power, 2013-05-05 21:35:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/samarth@chromium.org/14843002/10003

[commit-bot: I haz the power, 2013-05-06 08:09:45]

Failed to apply patch for chrome/browser/ui/search/instant_controller.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/ui/search/instant_controller.cc
  Hunk #1 succeeded at 239 (offset -1 lines).
  Hunk #2 succeeded at 265 (offset -1 lines).
  Hunk #3 succeeded at 306 (offset -1 lines).
  Hunk #4 succeeded at 357 (offset -1 lines).
  Hunk #5 succeeded at 367 (offset -1 lines).
  Hunk #6 succeeded at 400 (offset -1 lines).
  Hunk #7 succeeded at 430 (offset -1 lines).
  Hunk #8 succeeded at 485 (offset -1 lines).
  Hunk #9 succeeded at 575 (offset -1 lines).
  Hunk #10 succeeded at 627 (offset -1 lines).
  Hunk #11 succeeded at 655 (offset -1 lines).
  Hunk #12 succeeded at 672 (offset -1 lines).
  Hunk #13 succeeded at 687 (offset -1 lines).
  Hunk #14 FAILED at 701.
  Hunk #15 succeeded at 720 (offset -1 lines).
  Hunk #16 succeeded at 851 (offset -1 lines).
  Hunk #17 succeeded at 1201 (offset -1 lines).
  Hunk #18 succeeded at 1213 (offset -1 lines).
  Hunk #19 succeeded at 1458 (offset 1 line).
  Hunk #20 succeeded at 1490 (offset 1 line).
  Hunk #21 succeeded at 1511 (offset 1 line).
  Hunk #22 succeeded at 1693 (offset 1 line).
  1 out of 22 hunks FAILED -- saving rejects to file
chrome/browser/ui/search/instant_controller.cc.rej

Patch:       chrome/browser/ui/search/instant_controller.cc
Index: chrome/browser/ui/search/instant_controller.cc
diff --git a/chrome/browser/ui/search/instant_controller.cc
b/chrome/browser/ui/search/instant_controller.cc
index
e721a26e45e908da47226ad37f3c128f8caf0cf6..7be330a2929e251cc3b01caef4dc89786d01fd98
100644
--- a/chrome/browser/ui/search/instant_controller.cc
+++ b/chrome/browser/ui/search/instant_controller.cc
@@ -240,6 +240,7 @@
InstantController::InstantController(BrowserInstantController* browser,
       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),
@@ -265,15 +266,7 @@ void InstantController::OnAutocompleteStart() {
     return;
   }
 
-  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()) {
@@ -314,7 +307,7 @@ bool InstantController::Update(const AutocompleteMatch&
match,
   // 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();
@@ -365,7 +358,7 @@ bool InstantController::Update(const AutocompleteMatch&
match,
     return false;
   }
 
-  if (!instant_tab_ && !overlay_) {
+  if (!UseTabForSuggestions() && !overlay_) {
     HideOverlay();
     return false;
   }
@@ -375,7 +368,7 @@ bool InstantController::Update(const AutocompleteMatch&
match,
       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.
@@ -408,7 +401,7 @@ bool InstantController::Update(const AutocompleteMatch&
match,
         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()) {
@@ -438,7 +431,7 @@ bool InstantController::Update(const AutocompleteMatch&
match,
       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);
@@ -493,7 +486,7 @@ bool InstantController::Update(const AutocompleteMatch&
match,
   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())
@@ -583,7 +576,7 @@ void InstantController::HandleAutocompleteResults(
   if (!extended_enabled_)
     return;
 
-  if (!instant_tab_ && !overlay_)
+  if (!UseTabForSuggestions() && !overlay_)
     return;
 
   // The omnibox sends suggestions when its possibly imaginary popup closes
@@ -635,7 +628,7 @@ void InstantController::HandleAutocompleteResults(
       "HandleAutocompleteResults: total_results=%d",
       static_cast<int>(results.size())));
 
-  if (instant_tab_)
+  if (UseTabForSuggestions())
     instant_tab_->SendAutocompleteResults(results);
   else
     overlay_->SendAutocompleteResults(results);
@@ -663,10 +656,10 @@ 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);
@@ -680,7 +673,7 @@ void InstantController::OnCancel(const AutocompleteMatch&
match,
   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.
@@ -695,7 +688,7 @@ void InstantController::OnCancel(const AutocompleteMatch&
match,
   // 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 {
@@ -708,7 +701,7 @@ void InstantController::OmniboxNavigateToURL() {
   if (!extended_enabled_)
     return;
   RecordNavigationHistogram(UsingLocalPage(), false);
-  if (instant_tab_)
+  if (UseTabForSuggestions())
     instant_tab_->Submit(string16());
 }
 
@@ -728,13 +721,13 @@ bool InstantController::CommitIfPossible(InstantCommitType
type) {
 
   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_ ||
@@ -859,6 +852,9 @@ bool InstantController::CommitIfPossible(InstantCommitType
type) {
   // user interaction.
   ResetOverlay(GetInstantURL());
 
+  if (instant_tab_)
+    use_tab_for_suggestions_ = true;
+
   LOG_INSTANT_DEBUG_EVENT(this, "Committed");
   return true;
 }
@@ -1206,7 +1202,7 @@ void InstantController::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_)
@@ -1218,7 +1214,8 @@ void InstantController::SetSuggestions(
 
   // 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)
@@ -1460,6 +1457,7 @@ void InstantController::ResetInstantTab() {
       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.
@@ -1491,11 +1489,14 @@ void InstantController::HideInternal() {
 
   // 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_)
+  i…
(message too large)

[commit-bot: I haz the power, 2013-05-06 08:19:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/samarth@chromium.org/14843002/26001

[commit-bot: I haz the power, 2013-05-06 15:32:21]

Change committed as 198460