Omnibox: Call Stop() on all Providers After Certain Amount of Time

chrome/browser/autocomplete/autocomplete_controller.cc

 // Copyright (c) 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/autocomplete/autocomplete_controller.h"
 
 #include <set>
 #include <string>
 
 #include "base/command_line.h"
 #include "base/format_macros.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/stringprintf.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/time.h"
 #include "chrome/browser/autocomplete/autocomplete_controller_delegate.h"
 #include "chrome/browser/autocomplete/bookmark_provider.h"
 #include "chrome/browser/autocomplete/builtin_provider.h"
 #include "chrome/browser/autocomplete/extension_app_provider.h"
 #include "chrome/browser/autocomplete/history_contents_provider.h"
 #include "chrome/browser/autocomplete/history_quick_provider.h"
 #include "chrome/browser/autocomplete/history_url_provider.h"
 #include "chrome/browser/autocomplete/keyword_provider.h"
 #include "chrome/browser/autocomplete/search_provider.h"
 #include "chrome/browser/autocomplete/shortcuts_provider.h"
 #include "chrome/browser/autocomplete/zero_suggest_provider.h"
 #include "chrome/browser/omnibox/omnibox_field_trial.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/search/search.h"
 #include "chrome/browser/search_engines/template_url.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/chrome_switches.h"
 #include "content/public/browser/notification_service.h"
 #include "grit/generated_resources.h"
 #include "grit/theme_resources.h"
 #include "ui/base/l10n/l10n_util.h"
 
 #if defined(OS_CHROMEOS)
 #include "chrome/browser/autocomplete/contact_provider_chromeos.h"
@@ skipping 71 matching lines @@
   if (!autocompletions->empty())
     autocompletions->append("j");
   base::StringAppendF(autocompletions, "%" PRIuS, type);
   // Subtype is optional - string16::npos indicates no subtype.
   if (subtype != string16::npos)
     base::StringAppendF(autocompletions, "i%" PRIuS, subtype);
   if (count > 1)
     base::StringAppendF(autocompletions, "l%d", count);
 }
 
-// Amount of time (in ms) between when the user stops typing and when we remove
-// any copied entries. We do this from the time the user stopped typing as some
-// providers (such as SearchProvider) wait for the user to stop typing before
-// they initiate a query.
-const int kExpireTimeMS = 500;
-
 }  // namespace
 
 const int AutocompleteController::kNoItemSelected = -1;
 
 AutocompleteController::AutocompleteController(
     Profile* profile,
     AutocompleteControllerDelegate* delegate,
     int provider_types)
     : delegate_(delegate),
       history_url_provider_(NULL),
       keyword_provider_(NULL),
       search_provider_(NULL),
       zero_suggest_provider_(NULL),
+      in_stop_timer_field_trial_(
+          OmniboxFieldTrial::InStopTimerFieldTrialExperimentGroup()),
       done_(true),
       in_start_(false),
       in_zero_suggest_(false),
       profile_(profile) {
   // AND with the disabled providers, if any.
   provider_types &= ~OmniboxFieldTrial::GetDisabledProviderTypes();
   bool use_hqp = !!(provider_types & AutocompleteProvider::TYPE_HISTORY_QUICK);
   // TODO(mrossetti): Permanently modify the HistoryURLProvider to not search
   // titles once HQP is turned on permanently.
   // History quick provider can be used on all platforms other than Android.
@@ skipping 79 matching lines @@
   // may no longer be prevented.  In both these cases the text itself hasn't
   // changed since the last query, and some providers can do much less work (and
   // get matches back more quickly).  Taking advantage of this reduces flicker.
   //
   // NOTE: This comes after constructing |input_| above since that construction
   // can change the text string (e.g. by stripping off a leading '?').
   const bool minimal_changes = (input_.text() == old_input_text) &&
       (input_.matches_requested() == old_matches_requested);
 
   expire_timer_.Stop();
+  stop_timer_.Stop();
 
   // Start the new query.
   in_zero_suggest_ = false;
   in_start_ = true;
   base::TimeTicks start_time = base::TimeTicks::Now();
   for (ACProviders::iterator i(providers_.begin()); i != providers_.end();
        ++i) {
     (*i)->Start(input_, minimal_changes);
     if (input.matches_requested() != AutocompleteInput::ALL_MATCHES)
       DCHECK((*i)->done());
@@ skipping 17 matching lines @@
   // even if the current match is for the same URL as the last run's
   // default match.  Likewise, the controller doesn't know what's
   // happened in the edit since the last time it ran autocomplete.
   // The user might have selected all the text and hit delete, then
   // typed a new character.  The selection and delete won't send any
   // signals to the controller so it doesn't realize that anything was
   // cleared or changed.  Even if the default match hasn't changed, we
   // need the edit model to update the display.
   UpdateResult(false, true);
 
-  if (!done_)
+  if (!done_) {
     StartExpireTimer();
+    StartStopTimer();
+  }
 }
 
 void AutocompleteController::Stop(bool clear_result) {
   for (ACProviders::const_iterator i(providers_.begin()); i != providers_.end();
        ++i) {
     (*i)->Stop(clear_result);
   }
 
   expire_timer_.Stop();
+  stop_timer_.Stop();
   done_ = true;
   if (clear_result && !result_.empty()) {
     result_.Reset();
     // NOTE: We pass in false since we're trying to only clear the popup, not
     // touch the edit... this is all a mess and should be cleaned up :(
     NotifyChanged(false);
   }
 }
 
 void AutocompleteController::StartZeroSuggest(
@@ skipping 284 matching lines @@
        ++i) {
     if (!(*i)->done()) {
       done_ = false;
       return;
     }
   }
   done_ = true;
 }
 
 void AutocompleteController::StartExpireTimer() {
+  // Amount of time (in ms) between when the user stops typing and
+  // when we remove any copied entries. We do this from the time the
+  // user stopped typing as some providers (such as SearchProvider)
+  // wait for the user to stop typing before they initiate a query.
+  const int kExpireTimeMS = 500;
+
   if (result_.HasCopiedMatches())
     expire_timer_.Start(FROM_HERE,
                         base::TimeDelta::FromMilliseconds(kExpireTimeMS),
                         this, &AutocompleteController::ExpireCopiedEntries);
 }
+
+void AutocompleteController::StartStopTimer() {
+  if (!in_stop_timer_field_trial_)
+    return;
+
+  // Amount of time (in ms) between when the user stops typing and
+  // when we send Stop() to every provider.  This is intended to avoid
+  // the disruptive effect of belated omnibox updates, updates that
+  // come after the user has had to time to read the whole dropdown
+  // and doesn't expect it to change.  This doesn't entirely prohibit
+  // late updates--some providers may ignore the Stop() call--but it

[Peter Kasting, 2013/04/19 19:48:53]

This last sentence worries me.  No one should be ignoring Stop().  Even if, say,
the HQP can't halt its in-process SQL queries, it should ignore the results they
return if things have been Stop()ed.

[Mark P, 2013/04/19 20:20:40]

HCP cannot halt its in-processe SQL queries.  It currently does not have any
special logic to prevent updating its results after stop is called.  Should I
file a bug for this?

Actually, it looks like HCP clears its results unconditionally (even if it is
done) when stop is called.  This looks like a bug to me.  Do you agree?

There are likely problems with other providers too.  In keyword provider I see
extensions that handle omnibox input get routed the OnInputCancelled event, but
I don't see any code that enforces that the keyword extension obeys that and
doesn't allow changes after the fact.  Should I file a bug for this one in
particular too?

Do you want me to revise this comment given that it's true at the moment (even
though you're not happy that it's true)?

[Peter Kasting, 2013/04/19 20:26:17]

Yes.  Once Stop() is called we should not make any changes.  This can cause
problems with cursor navigation.
Yeah, that sounds wrong.
Maybe; I'm less certain here because I don't know if such enforcement is
possible.
Can we fix any issues here so that we can falsify the comment, before
proceeding?  If not, we should make this comment be something like "NOTE:
Currently not all providers correctly obey Stop(); this is a bug that should be
fixed" or something like that.

[Mark P, 2013/04/19 21:11:25]

Filed bug.
https://code.google.com/p/chromium/issues/detail?id=233727
Then, as we discussed on the bug, HCP is probably okay.
Fixed with https://codereview.chromium.org/14267032/
Actually, given our discussion thus far, it sounds like things may be okay. 
Removed the worrying sentence in the comment.

+  // does a fairly good job.
+  const int kStopTimeMS = 1500;
+
+  // Only use the timer if Instant/InstantExtended is disabled.
+  // InstantExtended has its own logic for when to stop updating the
+  // dropdown.  Furthermore, both Instant and InstantExtended expect
+  // all results they inject (regardless of how long they took) to make
+  // it to the edit model / dropdown display code.
+  if (!chrome::IsInstantExtendedAPIEnabled() &&
+      !chrome::IsInstantEnabled(profile_)) {
+    stop_timer_.Start(FROM_HERE,
+                      base::TimeDelta::FromMilliseconds(kStopTimeMS),
+                      base::Bind(&AutocompleteController::Stop,
+                                 base::Unretained(this),
+                                 false));
+  }
+}