Reworks autocomplete result set processing by the edit/model necessitated by my last change (only one result set vs 2). There is now one notification when the results change. The edit is the sole listener and updates the popup after it does some processing.

My last change broke keyword state when switching tabs. This is because of the change I did to AutocompleteEditModel::SetUserText, specifically:

  keyword_.clear();
  is_keyword_hint_ = false;

When you restore a tab we end up in

void AutocompleteEditModel::RestoreState(const State& state) {
  ...
    keyword_ = state.keyword;
    is_keyword_hint_ = state.is_keyword_hint;
    view_->SetUserText(state.user_text,
        DisplayTextFromUserText(state.user_text), false);

Which ends up back in SetUserText and we because of my change the
keyword is effectively chucked.

I added the code to clear out the keyword because of the change of
notifications. Specifically because AutocompletePopupModel::Observe is
now notified before AutocompleteEditModel::Observe yet it expects the
AutocompleteEditModel to have been updated.

Additionally I need to update the keyword_ and keyword_hint_ immediately in the edit if the results go empty. This mirrors what the old code was doing.

BUG=72022
TEST=covered by tests

Review URL: http://codereview.chromium.org/6484015

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

9 files changed, 111 insertions, 113 deletions

diff --git a/chrome/browser/autocomplete/autocomplete.cc b/chrome/browser/autocomplete/autocomplete.cc
index bd8cf16..5f8f681 100644
--- a/chrome/browser/autocomplete/autocomplete.cc
+++ b/chrome/browser/autocomplete/autocomplete.cc
@@ -872,13 +872,9 @@ void AutocompleteController::Stop(bool clear_result) {
   done_ = true;
   if (clear_result && !result_.empty()) {
     result_.Reset();
-    NotificationService::current()->Notify(
-        NotificationType::AUTOCOMPLETE_CONTROLLER_RESULT_UPDATED,
-        Source<AutocompleteController>(this),
-        Details<const AutocompleteResult>(&result_));
-    // NOTE: We don't notify AUTOCOMPLETE_CONTROLLER_DEFAULT_MATCH_UPDATED since
-    // we're trying to only clear the popup, not touch the edit... this is all
-    // a mess and should be cleaned up :(
+    // 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);
   }
 }
 
@@ -951,16 +947,7 @@ void AutocompleteController::NotifyChanged(bool notify_default_match) {
   NotificationService::current()->Notify(
       NotificationType::AUTOCOMPLETE_CONTROLLER_RESULT_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_));
-  }
+      Details<bool>(&notify_default_match));
 }
 
 void AutocompleteController::CheckIfDone() {
diff --git a/chrome/browser/autocomplete/autocomplete.h b/chrome/browser/autocomplete/autocomplete.h
index ea6382c..148209f 100644
--- a/chrome/browser/autocomplete/autocomplete.h
+++ b/chrome/browser/autocomplete/autocomplete.h
@@ -615,12 +615,10 @@ class AutocompleteController : public ACProviderListener {
   // return matches which are synchronously available, which should mean that
   // all providers will be done immediately.
   //
-  // The controller will fire AUTOCOMPLETE_CONTROLLER_DEFAULT_MATCH_UPDATED from
-  // inside this call, and unless the query is stopped, will fire at least one
-  // (and perhaps more) AUTOCOMPLETE_CONTROLLER_RESULT_UPDATED later as more
-  // matches come in (even if the query completes synchronously).  Listeners
-  // should use the result set provided in the accompanying Details object to
-  // update themselves.
+  // The controller will fire AUTOCOMPLETE_CONTROLLER_RESULT_UPDATED from
+  // inside this call at least once. If matches are available later on that
+  // result in changing the result set AUTOCOMPLETE_CONTROLLER_RESULT_UPDATED
+  // is sent again.
   void Start(const string16& text,
              const string16& desired_tld,
              bool prevent_inline_autocomplete,
diff --git a/chrome/browser/autocomplete/autocomplete_edit.cc b/chrome/browser/autocomplete/autocomplete_edit.cc
index d8681d7..5f3aeef 100644
--- a/chrome/browser/autocomplete/autocomplete_edit.cc
+++ b/chrome/browser/autocomplete/autocomplete_edit.cc
@@ -15,6 +15,7 @@
 #include "chrome/browser/autocomplete/autocomplete_edit_view.h"
 #include "chrome/browser/autocomplete/autocomplete_match.h"
 #include "chrome/browser/autocomplete/autocomplete_popup_model.h"
+#include "chrome/browser/autocomplete/autocomplete_popup_view.h"
 #include "chrome/browser/autocomplete/keyword_provider.h"
 #include "chrome/browser/browser_list.h"
 #include "chrome/browser/command_updater.h"
@@ -82,7 +83,7 @@ AutocompleteEditModel::~AutocompleteEditModel() {
 void AutocompleteEditModel::SetPopupModel(AutocompletePopupModel* popup_model) {
   popup_ = popup_model;
   registrar_.Add(this,
-      NotificationType::AUTOCOMPLETE_CONTROLLER_DEFAULT_MATCH_UPDATED,
+      NotificationType::AUTOCOMPLETE_CONTROLLER_RESULT_UPDATED,
       Source<AutocompleteController>(popup_->autocomplete_controller()));
 }
 
@@ -147,8 +148,6 @@ bool AutocompleteEditModel::UpdatePermanentText(
 
 void AutocompleteEditModel::SetUserText(const string16& text) {
   SetInputInProgress(true);
-  keyword_.clear();
-  is_keyword_hint_ = false;
   InternalSetUserText(text);
   paste_state_ = NONE;
   has_temporary_text_ = false;
@@ -688,14 +687,6 @@ void AutocompleteEditModel::PopupBoundsChangedTo(const gfx::Rect& bounds) {
   controller_->OnPopupBoundsChanged(bounds);
 }
 
-void AutocompleteEditModel::OnPopupClosed() {
-  // Accepts the temporary text as the user text, because it makes little
-  // sense to have temporary text when the popup is closed.
-  InternalSetUserText(UserTextFromDisplayText(view_->GetText()));
-  has_temporary_text_ = false;
-  PopupBoundsChangedTo(gfx::Rect());
-}
-
 // Return true if the suggestion type warrants a TCP/IP preconnection.
 // i.e., it is now highly likely that the user will select the related domain.
 static bool IsPreconnectable(AutocompleteMatch::Type type) {
@@ -718,36 +709,53 @@ static bool IsPreconnectable(AutocompleteMatch::Type type) {
 void AutocompleteEditModel::Observe(NotificationType type,
                                     const NotificationSource& source,
                                     const NotificationDetails& details) {
-  DCHECK_EQ(NotificationType::AUTOCOMPLETE_CONTROLLER_DEFAULT_MATCH_UPDATED,
+  DCHECK_EQ(NotificationType::AUTOCOMPLETE_CONTROLLER_RESULT_UPDATED,
             type.value);
 
-  string16 inline_autocomplete_text;
-  string16 keyword;
-  bool is_keyword_hint = false;
-  const AutocompleteResult* result =
-      Details<const AutocompleteResult>(details).ptr();
-  const AutocompleteResult::const_iterator match(result->default_match());
-  if (match != result->end()) {
-    if ((match->inline_autocomplete_offset != string16::npos) &&
-        (match->inline_autocomplete_offset < match->fill_into_edit.length())) {
-      inline_autocomplete_text =
-          match->fill_into_edit.substr(match->inline_autocomplete_offset);
-    }
-
-    if (!match->destination_url.SchemeIs(chrome::kExtensionScheme)) {
-      // Warm up DNS Prefetch cache, or preconnect to a search service.
-      chrome_browser_net::AnticipateOmniboxUrl(match->destination_url,
-                                               IsPreconnectable(match->type));
+  const bool was_open = popup_->view()->IsOpen();
+  if (*(Details<bool>(details).ptr())) {
+    string16 inline_autocomplete_text;
+    string16 keyword;
+    bool is_keyword_hint = false;
+    const AutocompleteResult& result =
+        popup_->autocomplete_controller()->result();
+    const AutocompleteResult::const_iterator match(result.default_match());
+    if (match != result.end()) {
+      if ((match->inline_autocomplete_offset != string16::npos) &&
+          (match->inline_autocomplete_offset <
+           match->fill_into_edit.length())) {
+        inline_autocomplete_text =
+            match->fill_into_edit.substr(match->inline_autocomplete_offset);
+      }
+
+      if (!match->destination_url.SchemeIs(chrome::kExtensionScheme)) {
+        // Warm up DNS Prefetch cache, or preconnect to a search service.
+        chrome_browser_net::AnticipateOmniboxUrl(match->destination_url,
+                                                 IsPreconnectable(match->type));
+      }
+
+      // We could prefetch the alternate nav URL, if any, but because there
+      // can be many of these as a user types an initial series of characters,
+      // the OS DNS cache could suffer eviction problems for minimal gain.
+
+      is_keyword_hint = popup_->GetKeywordForMatch(*match, &keyword);
     }
-
-    // We could prefetch the alternate nav URL, if any, but because there
-    // can be many of these as a user types an initial series of characters,
-    // the OS DNS cache could suffer eviction problems for minimal gain.
-
-    is_keyword_hint = popup_->GetKeywordForMatch(*match, &keyword);
+    popup_->OnResultChanged();
+    OnPopupDataChanged(inline_autocomplete_text, NULL, keyword,
+                       is_keyword_hint);
+  } else {
+    popup_->OnResultChanged();
   }
 
-  OnPopupDataChanged(inline_autocomplete_text, NULL, keyword, is_keyword_hint);
+  if (popup_->view()->IsOpen()) {
+    PopupBoundsChangedTo(popup_->view()->GetTargetBounds());
+  } else if (was_open) {
+    // Accepts the temporary text as the user text, because it makes little
+    // sense to have temporary text when the popup is closed.
+    InternalSetUserText(UserTextFromDisplayText(view_->GetText()));
+    has_temporary_text_ = false;
+    PopupBoundsChangedTo(gfx::Rect());
+  }
 }
 
 void AutocompleteEditModel::InternalSetUserText(const string16& text) {
diff --git a/chrome/browser/autocomplete/autocomplete_edit.h b/chrome/browser/autocomplete/autocomplete_edit.h
index b407b11..8a94be4 100644
--- a/chrome/browser/autocomplete/autocomplete_edit.h
+++ b/chrome/browser/autocomplete/autocomplete_edit.h
@@ -324,9 +324,6 @@ class AutocompleteEditModel : public NotificationObserver {
   // Invoked when the popup is going to change its bounds to |bounds|.
   void PopupBoundsChangedTo(const gfx::Rect& bounds);
 
-  // Called when the popup is closed.
-  void OnPopupClosed();
-
  private:
   enum PasteState {
     NONE,           // Most recent edit was not a paste.
diff --git a/chrome/browser/autocomplete/autocomplete_edit_view_browsertest.cc b/chrome/browser/autocomplete/autocomplete_edit_view_browsertest.cc
index 0e9c8c2..0cb8fdf 100644
--- a/chrome/browser/autocomplete/autocomplete_edit_view_browsertest.cc
+++ b/chrome/browser/autocomplete/autocomplete_edit_view_browsertest.cc
@@ -994,6 +994,33 @@ class AutocompleteEditViewTest : public InProcessBrowserTest,
 
     ASSERT_FALSE(ui_test_utils::IsViewFocused(browser(), VIEW_ID_LOCATION_BAR));
   }
+
+  void PersistKeywordModeOnTabSwitch() {
+    AutocompleteEditView* edit_view = NULL;
+    ASSERT_NO_FATAL_FAILURE(GetAutocompleteEditView(&edit_view));
+
+    // Trigger keyword hint mode.
+    ASSERT_NO_FATAL_FAILURE(SendKeySequence(kSearchKeywordKeys));
+    ASSERT_TRUE(edit_view->model()->is_keyword_hint());
+    ASSERT_EQ(kSearchKeyword, UTF16ToUTF8(edit_view->model()->keyword()));
+
+    // Trigger keyword mode.
+    ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_TAB, false, false, false));
+    ASSERT_FALSE(edit_view->model()->is_keyword_hint());
+    ASSERT_EQ(kSearchKeyword, UTF16ToUTF8(edit_view->model()->keyword()));
+
+    // Input something as search text.
+    ASSERT_NO_FATAL_FAILURE(SendKeySequence(kSearchTextKeys));
+
+    // Create a new tab.
+    browser()->NewTab();
+
+    // Switch back to the first tab.
+    browser()->SelectTabContentsAt(0, true);
+
+    // Make sure we're still in keyword mode.
+    ASSERT_EQ(kSearchKeyword, UTF16ToUTF8(edit_view->model()->keyword()));
+  }
 };
 
 // Test if ctrl-* accelerators are workable in omnibox.
@@ -1059,6 +1086,11 @@ IN_PROC_BROWSER_TEST_F(AutocompleteEditViewTest, TabMoveCursorToEnd) {
   TabMoveCursorToEndTest();
 }
 
+IN_PROC_BROWSER_TEST_F(AutocompleteEditViewTest,
+                       PersistKeywordModeOnTabSwitch) {
+  PersistKeywordModeOnTabSwitch();
+}
+
 #if defined(OS_LINUX)
 // TODO(oshima): enable these tests for views-implmentation when
 // these featuers are supported.
@@ -1251,4 +1283,9 @@ IN_PROC_BROWSER_TEST_F(AutocompleteEditViewViewsTest,
   TabMoveCursorToEndTest();
 }
 
+IN_PROC_BROWSER_TEST_F(AutocompleteEditViewViewsTest,
+                       PersistKeywordModeOnTabSwitch) {
+  PersistKeywordModeOnTabSwitch();
+}
+
 #endif
diff --git a/chrome/browser/autocomplete/autocomplete_popup_model.cc b/chrome/browser/autocomplete/autocomplete_popup_model.cc
index 9e2d43b..6809d0c 100644
--- a/chrome/browser/autocomplete/autocomplete_popup_model.cc
+++ b/chrome/browser/autocomplete/autocomplete_popup_model.cc
@@ -18,8 +18,6 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search_engines/template_url.h"
 #include "chrome/browser/search_engines/template_url_model.h"
-#include "chrome/common/notification_details.h"
-#include "chrome/common/notification_source.h"
 #include "ui/gfx/rect.h"
 
 ///////////////////////////////////////////////////////////////////////////////
@@ -35,8 +33,6 @@ AutocompletePopupModel::AutocompletePopupModel(
       profile_(profile),
       hovered_line_(kNoMatch),
       selected_line_(kNoMatch) {
-  registrar_.Add(this, NotificationType::AUTOCOMPLETE_CONTROLLER_RESULT_UPDATED,
-                 Source<AutocompleteController>(controller_.get()));
 }
 
 AutocompletePopupModel::~AutocompletePopupModel() {
@@ -287,38 +283,27 @@ void AutocompletePopupModel::TryDeletingCurrentItem() {
   }
 }
 
-void AutocompletePopupModel::Observe(NotificationType type,
-                                     const NotificationSource& source,
-                                     const NotificationDetails& details) {
-  DCHECK_EQ(NotificationType::AUTOCOMPLETE_CONTROLLER_RESULT_UPDATED,
-            type.value);
+const SkBitmap* AutocompletePopupModel::GetSpecialIconForMatch(
+    const AutocompleteMatch& match) const {
+  if (!match.template_url || !match.template_url->IsExtensionKeyword())
+    return NULL;
 
-  const AutocompleteResult* result =
-      Details<const AutocompleteResult>(details).ptr();
-  selected_line_ = result->default_match() == result->end() ?
-      kNoMatch : static_cast<size_t>(result->default_match() - result->begin());
+  return &profile_->GetExtensionService()->GetOmniboxPopupIcon(
+      match.template_url->GetExtensionId());
+}
+
+void AutocompletePopupModel::OnResultChanged() {
+  const AutocompleteResult& result = controller_->result();
+  selected_line_ = result.default_match() == result.end() ?
+      kNoMatch : static_cast<size_t>(result.default_match() - result.begin());
   // There had better not be a nonempty result set with no default match.
-  CHECK((selected_line_ != kNoMatch) || result->empty());
+  CHECK((selected_line_ != kNoMatch) || result.empty());
   manually_selected_match_.Clear();
   // If we're going to trim the window size to no longer include the hovered
   // line, turn hover off.  Practically, this shouldn't happen, but it
   // doesn't hurt to be defensive.
-  if ((hovered_line_ != kNoMatch) && (result->size() <= hovered_line_))
+  if ((hovered_line_ != kNoMatch) && (result.size() <= hovered_line_))
     SetHoveredLine(kNoMatch);
 
-  const bool was_open = view_->IsOpen();
   view_->UpdatePopupAppearance();
-  if (view_->IsOpen())
-    edit_model_->PopupBoundsChangedTo(view_->GetTargetBounds());
-  else if (was_open)
-    edit_model_->OnPopupClosed();
-}
-
-const SkBitmap* AutocompletePopupModel::GetSpecialIconForMatch(
-    const AutocompleteMatch& match) const {
-  if (!match.template_url || !match.template_url->IsExtensionKeyword())
-    return NULL;
-
-  return &profile_->GetExtensionService()->GetOmniboxPopupIcon(
-      match.template_url->GetExtensionId());
 }
diff --git a/chrome/browser/autocomplete/autocomplete_popup_model.h b/chrome/browser/autocomplete/autocomplete_popup_model.h
index 37687a5..6402ff2 100644
--- a/chrome/browser/autocomplete/autocomplete_popup_model.h
+++ b/chrome/browser/autocomplete/autocomplete_popup_model.h
@@ -8,8 +8,6 @@
 
 #include "base/scoped_ptr.h"
 #include "chrome/browser/autocomplete/autocomplete.h"
-#include "chrome/common/notification_observer.h"
-#include "chrome/common/notification_registrar.h"
 
 class AutocompleteEditModel;
 class AutocompleteEditView;
@@ -18,7 +16,7 @@ class SkBitmap;
 
 class AutocompletePopupView;
 
-class AutocompletePopupModel : public NotificationObserver {
+class AutocompletePopupModel {
  public:
   AutocompletePopupModel(AutocompletePopupView* popup_view,
                          AutocompleteEditModel* edit_model,
@@ -128,23 +126,20 @@ class AutocompletePopupModel : public NotificationObserver {
 
   Profile* profile() const { return profile_; }
 
+  // Invoked from the edit model any time the result set of the controller
+  // changes.
+  void OnResultChanged();
+
   // The token value for selected_line_, hover_line_ and functions dealing with
   // a "line number" that indicates "no line".
   static const size_t kNoMatch = -1;
 
  private:
-  // NotificationObserver
-  virtual void Observe(NotificationType type,
-                       const NotificationSource& source,
-                       const NotificationDetails& details);
-
   AutocompletePopupView* view_;
 
   AutocompleteEditModel* edit_model_;
   scoped_ptr<AutocompleteController> controller_;
 
-  NotificationRegistrar registrar_;
-
   // Profile for current tab.
   Profile* profile_;
 
diff --git a/chrome/browser/autocomplete/autocomplete_unittest.cc b/chrome/browser/autocomplete/autocomplete_unittest.cc
index 332fedf..93ce9da 100644
--- a/chrome/browser/autocomplete/autocomplete_unittest.cc
+++ b/chrome/browser/autocomplete/autocomplete_unittest.cc
@@ -226,7 +226,7 @@ void AutocompleteProviderTest::Observe(NotificationType type,
                                        const NotificationSource& source,
                                        const NotificationDetails& details) {
   if (controller_->done()) {
-    result_.CopyFrom(*(Details<const AutocompleteResult>(details).ptr()));
+    result_.CopyFrom(controller_->result());
     MessageLoop::current()->Quit();
   }
 }
diff --git a/chrome/common/notification_type.h b/chrome/common/notification_type.h
index 174138c..0c390939 100644
--- a/chrome/common/notification_type.h
+++ b/chrome/common/notification_type.h
@@ -784,19 +784,10 @@ class NotificationType {
 
     // Autocomplete ------------------------------------------------------------
 
-    // Sent by the autocomplete controller at least once per query, each time
-    // new matches are available, subject to rate-limiting/coalescing to reduce
-    // the number of updates.  The details hold the AutocompleteResult that
-    // observers should use if they want to see the updated matches.
+    // Sent by the autocomplete controller each time the result set updates.
+    // The details is a boolean indicating if the default match has changed.
     AUTOCOMPLETE_CONTROLLER_RESULT_UPDATED,
 
-    // Sent by the autocomplete controller immediately after synchronous matches
-    // become available, and thereafter at the same time that
-    // AUTOCOMPLETE_CONTROLLER_RESULT_UPDATED is sent.  The details hold the
-    // AutocompleteResult that observers should use if they want to see the
-    // up-to-date matches.
-    AUTOCOMPLETE_CONTROLLER_DEFAULT_MATCH_UPDATED,
-
     // This is sent when an item of the Omnibox popup is selected. The source
     // is the profile.
     OMNIBOX_OPENED_URL,