Fix a number of problems caused by the AutocompleteController purposefully committing out-of-date results after the edit had changed. Simply not doing this commit would make the popup appear less responsive, so instead we're more careful to not try and update the edit with the out-of-date data, and instead force the popup to get newer results in cases where it might have been out-of-date.

BUG=46315
TEST=Type a letter that inline autocompletes, then immediately hit ctrl-a.  You should get everything selected, not nothing.  Also, typing a letter that inline autocompletes, then immediately a random other letter and the down arrow key, should show a popup with results for the two letters you typed, not just the first.
Review URL: http://codereview.chromium.org/3047041

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@54649 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 43 insertions, 20 deletions

diff --git a/chrome/browser/autocomplete/autocomplete.cc b/chrome/browser/autocomplete/autocomplete.cc
index 5ae9d82..0693ca2 100644
--- a/chrome/browser/autocomplete/autocomplete.cc
+++ b/chrome/browser/autocomplete/autocomplete.cc
@@ -813,9 +813,12 @@ void AutocompleteController::Start(const std::wstring& text,
   // If we're interrupting an old query, and committing its result won't shrink
   // the visible set (which would probably re-expand soon, thus looking very
   // flickery), then go ahead and commit what we've got, in order to feel more
-  // responsive when the user is typing rapidly.
+  // responsive when the user is typing rapidly.  In this case it's important
+  // that we don't update the edit, as the user has already changed its contents
+  // and anything we might do with it (e.g. inline autocomplete) likely no
+  // longer applies.
   if (!minimal_changes && !done_ && (latest_result_.size() >= result_.size()))
-    CommitResult();
+    CommitResult(false);
 
   // If the timer is already running, it could fire shortly after starting this
   // query, when we're likely to only have the synchronous results back, thus
@@ -866,9 +869,14 @@ void AutocompleteController::DeleteMatch(const AutocompleteMatch& match) {
   DCHECK(match.deletable);
   match.provider->DeleteMatch(match);  // This may synchronously call back to
                                        // OnProviderUpdate().
-  CommitResult();  // Ensure any new result gets committed immediately.  If it
-                   // was committed already or hasn't been modified, this is
-                   // harmless.
+  CommitResult(true);  // Ensure any new result gets committed immediately.  If
+                       // it was committed already or hasn't been modified, this
+                       // is harmless.
+}
+
+void AutocompleteController::CommitIfQueryHasNeverBeenCommitted() {
+  if (!have_committed_during_this_query_)
+    CommitResult(true);
 }
 
 void AutocompleteController::OnProviderUpdate(bool updated_matches) {
@@ -917,15 +925,15 @@ void AutocompleteController::UpdateLatestResult(bool is_synchronous_pass) {
   // last commit, to minimize flicker.
   if (result_.empty() || (done_ && !have_committed_during_this_query_) ||
       delay_interval_has_passed_)
-    CommitResult();
+    CommitResult(true);
 }
 
 void AutocompleteController::DelayTimerFired() {
   delay_interval_has_passed_ = true;
-  CommitResult();
+  CommitResult(true);
 }
 
-void AutocompleteController::CommitResult() {
+void AutocompleteController::CommitResult(bool notify_default_match) {
   if (done_) {
     update_delay_timer_.Stop();
     delay_interval_has_passed_ = false;
@@ -943,13 +951,15 @@ void AutocompleteController::CommitResult() {
       NotificationType::AUTOCOMPLETE_CONTROLLER_RESULT_UPDATED,
       Source<AutocompleteController>(this),
       Details<const AutocompleteResult>(&result_));
-  // This notification must be sent after the other so the popup has time to
-  // update its state before the edit calls into it.
-  // TODO(pkasting): Eliminate this ordering requirement.
-  NotificationService::current()->Notify(
-      NotificationType::AUTOCOMPLETE_CONTROLLER_DEFAULT_MATCH_UPDATED,
-      Source<AutocompleteController>(this),
-      Details<const AutocompleteResult>(&result_));
+  if (notify_default_match) {
+    // This notification must be sent after the other so the popup has time to
+    // update its state before the edit calls into it.
+    // TODO(pkasting): Eliminate this ordering requirement.
+    NotificationService::current()->Notify(
+        NotificationType::AUTOCOMPLETE_CONTROLLER_DEFAULT_MATCH_UPDATED,
+        Source<AutocompleteController>(this),
+        Details<const AutocompleteResult>(&result_));
+  }
   if (!done_)
     update_delay_timer_.Reset();
 }
diff --git a/chrome/browser/autocomplete/autocomplete.h b/chrome/browser/autocomplete/autocomplete.h
index 623ab7f..b6448df 100644
--- a/chrome/browser/autocomplete/autocomplete.h
+++ b/chrome/browser/autocomplete/autocomplete.h
@@ -769,6 +769,10 @@ class AutocompleteController : public ACProviderListener {
   // no query is running.
   void DeleteMatch(const AutocompleteMatch& match);
 
+  // Commits the results for the current query if they've never been committed.
+  // This is used by the popup to ensure it's not showing an out-of-date query.
+  void CommitIfQueryHasNeverBeenCommitted();
+
   // Getters
   const AutocompleteInput& input() const { return input_; }
   const AutocompleteResult& result() const { return result_; }
@@ -791,7 +795,12 @@ class AutocompleteController : public ACProviderListener {
   void DelayTimerFired();
 
   // Copies |latest_result_| to |result_| and notifies observers of updates.
-  void CommitResult();
+  // |notify_default_match| should normally be true; if it's false, we don't
+  // send an AUTOCOMPLETE_CONTROLLER_DEFAULT_MATCH_UPDATED notification.  This
+  // is a hack to avoid updating the edit with out-of-date data.
+  // TODO(pkasting): Don't hardcode assumptions about who listens to these
+  // notificiations.
+  void CommitResult(bool notify_default_match);
 
   // Returns the matches from |provider| whose destination urls are not in
   // |latest_result_|.
diff --git a/chrome/browser/autocomplete/autocomplete_popup_model.cc b/chrome/browser/autocomplete/autocomplete_popup_model.cc
index f4f7a82..eb99c3d 100644
--- a/chrome/browser/autocomplete/autocomplete_popup_model.cc
+++ b/chrome/browser/autocomplete/autocomplete_popup_model.cc
@@ -82,6 +82,9 @@ void AutocompletePopupModel::SetHoveredLine(size_t line) {
 
 void AutocompletePopupModel::SetSelectedLine(size_t line,
                                              bool reset_to_default) {
+  // We should at least be dealing with the results of the current query.
+  controller_->CommitIfQueryHasNeverBeenCommitted();
+
   const AutocompleteResult& result = controller_->result();
   CHECK(line < result.size());
   if (result.empty())
@@ -154,6 +157,10 @@ void AutocompletePopupModel::InfoForCurrentSelection(
   DCHECK(match != NULL);
   const AutocompleteResult* result;
   if (!controller_->done()) {
+    // NOTE: Using latest_result() is important here since not only could it
+    // contain newer results than result() for the current query, it could even
+    // refer to an entirely different query (e.g. if the user is typing rapidly
+    // and the controller is purposefully delaying updates to avoid flicker).
     result = &controller_->latest_result();
     // It's technically possible for |result| to be empty if no provider returns
     // a synchronous result but the query has not completed synchronously;
diff --git a/chrome/browser/autocomplete/autocomplete_unittest.cc b/chrome/browser/autocomplete/autocomplete_unittest.cc
index f8081e7..e4f3b92 100644
--- a/chrome/browser/autocomplete/autocomplete_unittest.cc
+++ b/chrome/browser/autocomplete/autocomplete_unittest.cc
@@ -123,9 +123,6 @@ class AutocompleteProviderTest : public testing::Test,
 void AutocompleteProviderTest::SetUp() {
   registrar_.Add(this, NotificationType::AUTOCOMPLETE_CONTROLLER_RESULT_UPDATED,
                  NotificationService::AllSources());
-  registrar_.Add(this,
-      NotificationType::AUTOCOMPLETE_CONTROLLER_DEFAULT_MATCH_UPDATED,
-      NotificationService::AllSources());
   ResetController(false);
 }
 
diff --git a/chrome/browser/extensions/extension_omnibox_apitest.cc b/chrome/browser/extensions/extension_omnibox_apitest.cc
index bba834d..5b799e4 100644
--- a/chrome/browser/extensions/extension_omnibox_apitest.cc
+++ b/chrome/browser/extensions/extension_omnibox_apitest.cc
@@ -58,7 +58,7 @@ class OmniboxApiTest : public ExtensionApiTest {
   void WaitForAutocompleteDone(AutocompleteController* controller) {
     while (!controller->done()) {
       ui_test_utils::WaitForNotification(
-          NotificationType::AUTOCOMPLETE_CONTROLLER_DEFAULT_MATCH_UPDATED);
+          NotificationType::AUTOCOMPLETE_CONTROLLER_RESULT_UPDATED);
     }
   }
 };