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

chrome/browser/autocomplete/autocomplete_controller.cc

Index: chrome/browser/autocomplete/autocomplete_controller.cc
diff --git a/chrome/browser/autocomplete/autocomplete_controller.cc b/chrome/browser/autocomplete/autocomplete_controller.cc
index 173dd62f01f6dae23595a47e1cd8afe8d2acefc1..637ea370e53404b5216e36eec5c2081e6fee55ad 100644
--- a/chrome/browser/autocomplete/autocomplete_controller.cc
+++ b/chrome/browser/autocomplete/autocomplete_controller.cc
@@ -27,6 +27,7 @@
 #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"
@@ -118,12 +119,6 @@ void AppendAvailableAutocompletion(size_t type,
     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;
@@ -137,6 +132,8 @@ AutocompleteController::AutocompleteController(
       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),
@@ -236,6 +233,7 @@ void AutocompleteController::Start(const AutocompleteInput& input) {
       (input_.matches_requested() == old_matches_requested);
 
   expire_timer_.Stop();
+  stop_timer_.Stop();
 
   // Start the new query.
   in_zero_suggest_ = false;
@@ -273,8 +271,10 @@ void AutocompleteController::Start(const AutocompleteInput& input) {
   // need the edit model to update the display.
   UpdateResult(false, true);
 
-  if (!done_)
+  if (!done_) {
     StartExpireTimer();
+    StartStopTimer();
+  }
 }
 
 void AutocompleteController::Stop(bool clear_result) {
@@ -284,6 +284,7 @@ void AutocompleteController::Stop(bool clear_result) {
   }
 
   expire_timer_.Stop();
+  stop_timer_.Stop();
   done_ = true;
   if (clear_result && !result_.empty()) {
     result_.Reset();
@@ -588,8 +589,42 @@ void AutocompleteController::CheckIfDone() {
 }
 
 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));
+  }
+}