Search state transitions not working

This change codifies and fixes the state transitions used in
the Search UI.  It documents the transitions in tabular
form in search_types.h detailing the user interaction that
results in either animated or non-animated transitions.
The pending navigation signal is removed, so all transitions now
occur on the navigation "committed" signal.

BUG=133529
TEST=Manual
R=sky@chromium.org, kuan@chromium.org


Review URL: https://chromiumcodereview.appspot.com/10837240

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

chrome/browser/ui/omnibox/omnibox_edit_model.cc

@@ -979,6 +979,7 @@ void OmniboxEditModel::OnResultChanged(bool default_match_changed) {
     InternalSetUserText(UserTextFromDisplayText(view_->GetText()));
     has_temporary_text_ = false;
     PopupBoundsChangedTo(gfx::Rect());
+    NotifySearchTabHelper();
   }
 }
 
@@ -1101,7 +1102,7 @@ bool OmniboxEditModel::CreatedKeywordSearchByInsertingSpaceInMiddle(
 void OmniboxEditModel::NotifySearchTabHelper() {
   if (controller_->GetTabContents()) {
     controller_->GetTabContents()->search_tab_helper()->
-        OmniboxEditModelChanged(this);
+        OmniboxEditModelChanged(user_input_in_progress_, !in_revert_);
   }
 }
 

chrome/browser/ui/search/search_delegate_unittest.cc

@@ -37,7 +37,7 @@ TEST_F(SearchDelegateTest, SearchModel) {
   chrome::ActivateTabAt(browser(), 1, true);
   contents = chrome::GetTabContentsAt(browser(), 1);
   contents->search_tab_helper()->model()->SetMode(
-      Mode(Mode::MODE_SEARCH, false));
+      Mode(Mode::MODE_SEARCH_RESULTS, false));
   EXPECT_TRUE(browser()->search_model()->mode().is_search());
 
   // The first tab is not active so changes should not propagate.

chrome/browser/ui/search/search_model.cc

@@ -39,13 +39,6 @@ void SearchModel::SetMode(const Mode& new_mode) {
   mode_.animate = false;
 }
 
-void SearchModel::MaybeChangeMode(Mode::Type from_mode, Mode::Type to_mode) {
-  if (mode_.mode == from_mode) {
-    Mode mode(to_mode, true);
-    SetMode(mode);
-  }
-}
-
 void SearchModel::AddObserver(SearchModelObserver* observer) {
   observers_.AddObserver(observer);
 }

chrome/browser/ui/search/search_model.h

@@ -36,9 +36,6 @@ class SearchModel {
   // Get the active mode.
   const Mode& mode() const { return mode_; }
 
-  // Change the mode to |to_mode| only if |from_mode| is the active mode.
-  void MaybeChangeMode(Mode::Type from_mode, Mode::Type to_mode);
-
   // Add and remove observers.
   void AddObserver(SearchModelObserver* observer);
   void RemoveObserver(SearchModelObserver* observer);

chrome/browser/ui/search/search_tab_helper.cc

@@ -48,24 +48,15 @@ SearchTabHelper::SearchTabHelper(
 SearchTabHelper::~SearchTabHelper() {
 }
 
-void SearchTabHelper::OmniboxEditModelChanged(OmniboxEditModel* edit_model) {
+void SearchTabHelper::OmniboxEditModelChanged(bool user_input_in_progress,
+                                              bool cancelling) {
   if (!is_search_enabled_)
     return;
 
-  if (model_.mode().is_ntp()) {
-    if (edit_model->user_input_in_progress())
-      model_.SetMode(Mode(Mode::MODE_SEARCH, true));
-    return;
-  }
-
-  Mode::Type mode = Mode::MODE_DEFAULT;
-  GURL url(web_contents()->GetURL());
-  // TODO(kuan): revisit this condition when zero suggest becomes available.
-  if (google_util::IsInstantExtendedAPIGoogleSearchUrl(url.spec()) ||
-      (edit_model->has_focus() && edit_model->user_input_in_progress())) {
-    mode = Mode::MODE_SEARCH;
-  }
-  model_.SetMode(Mode(mode, true));
+  if (user_input_in_progress)
+    model_.SetMode(Mode(Mode::MODE_SEARCH_SUGGESTIONS, true));
+  else if (cancelling)
+    UpdateModelBasedOnURL(web_contents()->GetURL(), true);
 }
 
 void SearchTabHelper::NavigateToPendingEntry(
@@ -75,7 +66,14 @@ void SearchTabHelper::NavigateToPendingEntry(
     return;
 
   // Do not animate if this url is the very first navigation for the tab.
-  UpdateModel(url, !web_contents()->GetController().IsInitialNavigation());
+  // NTP mode changes are initiated at "pending", all others are initiated
+  // when "committed".  This is because NTP is rendered natively so is faster
+  // to render than the web contents and we need to coordinate the animations.
+  if (IsNTP(url)) {
+    UpdateModelBasedOnURL(
+        url,
+        !web_contents()->GetController().IsInitialNavigation());
+  }
 }
 
 void SearchTabHelper::Observe(
@@ -85,22 +83,29 @@ void SearchTabHelper::Observe(
   DCHECK_EQ(content::NOTIFICATION_NAV_ENTRY_COMMITTED, type);
   content::LoadCommittedDetails* committed_details =
       content::Details<content::LoadCommittedDetails>(details).ptr();
-  // TODO(dhollowa): NavigationController::IsInitialNavigation() is always false
-  // by the time NOTIFICATION_NAV_ENTRY_COMMITTED is received, so please handle
-  // it appropriately when restructuring NavigateToPendingEntry() and this
-  // methods.
-  UpdateModel(committed_details->entry->GetURL(),
+
+  // TODO(dhollowa): Fix |NavigationController::IsInitialNavigation()| so that
+  // it spans the |NOTIFICATION_NAV_ENTRY_COMMITTED| notification.
+  // See comment in |NavigateToPendingEntry()| about why |!IsNTP()| is used.
+  if (!IsNTP(committed_details->entry->GetURL())) {
+    UpdateModelBasedOnURL(
+        committed_details->entry->GetURL(),
         !web_contents()->GetController().IsInitialNavigation());
+  }
 }
 
-void SearchTabHelper::UpdateModel(const GURL& url, bool animate) {
+void SearchTabHelper::UpdateModelBasedOnURL(const GURL& url, bool animate) {
   Mode::Type type = Mode::MODE_DEFAULT;
   if (IsNTP(url))
     type = Mode::MODE_NTP;
   else if (google_util::IsInstantExtendedAPIGoogleSearchUrl(url.spec()))
-    type = Mode::MODE_SEARCH;
+    type = Mode::MODE_SEARCH_RESULTS;
   model_.SetMode(Mode(type, animate));
 }
 
+content::WebContents* SearchTabHelper::web_contents() {
+  return model_.tab_contents()->web_contents();
+}
+
 }  // namespace search
 }  // namespace chrome

chrome/browser/ui/search/search_tab_helper.h

@@ -14,6 +14,10 @@
 class OmniboxEditModel;
 class TabContents;
 
+namespace content {
+class WebContents;
+}
+
 namespace chrome {
 namespace search {
 
@@ -31,9 +35,9 @@ class SearchTabHelper : public content::WebContentsObserver,
 
   // Invoked when the OmniboxEditModel changes state in some way that might
   // affect the search mode.
-  void OmniboxEditModelChanged(OmniboxEditModel* edit_model);
+  void OmniboxEditModelChanged(bool user_input_in_progress, bool cancelling);
 
-  // content::WebContentsObserver overrides:
+  // Overridden from contents::WebContentsObserver:
   virtual void NavigateToPendingEntry(
       const GURL& url,
       content::NavigationController::ReloadType reload_type) OVERRIDE;
@@ -44,9 +48,11 @@ class SearchTabHelper : public content::WebContentsObserver,
                        const content::NotificationDetails& details) OVERRIDE;
 
  private:
-  // Sets the mode of the model based on |url|.
-  // |animate| is set in the Mode passed to the model.
-  void UpdateModel(const GURL& url, bool animate);
+  // Sets the mode of the model based on |url|.  |animate| is based on initial
+  // navigation and used for the mode change on the |model_|.
+  void UpdateModelBasedOnURL(const GURL& url, bool animate);
+
+  content::WebContents* web_contents();
 
   const bool is_search_enabled_;
 

chrome/browser/ui/search/search_types.h

@@ -11,6 +11,36 @@ namespace search {
 // The Mode structure encodes the visual states encountered when interacting
 // with the NTP and the Omnibox.  State changes can be animated depending on the
 // context.
+//
+// *** Legend ***
+// |--------------+------------------------------------------------|
+// | Abbreviation | User Action                                    |
+// |--------------+------------------------------------------------|
+// | S            | Start.  First navigation to NTP                |
+// | N            | Navigation: Back/Forward/Direct                |
+// | T            | Tab switch                                     |
+// | I            | User input in Omnibox                          |
+// | D            | Defocus the Omnibox                            |
+// | E            | Escape out of Omnibox with suggestions showing |
+// | C            | Choose a suggestion from suggestion list       |
+//
+//
+// *** Transitions ***
+// |----------+---------+-----+---------+-----+----------+-----+---------+-----|
+// |          | To      |     |         |     |          |     |         |     |
+// |----------+---------+-----+---------+-----+----------+-----+---------+-----|
+// |          | NTP     |     | Search* |     | Search** |     | Default |     |
+// |----------+---------+-----+---------+-----+----------+-----+---------+-----|
+// | From     | Animate | Pop | Animate | Pop | Animate  | Pop | Animate | Pop |
+// |----------+---------+-----+---------+-----+----------+-----+---------+-----|
+// | Start    |         | S   | -       | -   | -        | -   | -       | -   |
+// | NTP      | -       | -   | I       |     | N        | T   | N       | T   |
+// | Search*  | N D E   | T   | -       | -   | D E C    | T   | N D E C | T   |
+// | Search** | N       | T   | I       |     | -        | -   | N D E C | T   |
+// | Default  | N       | T   | I       |     | N        | T   | -       | -   |
+//
+// * Search with suggestions showing.
+// ** Search without suggestions showing.
 struct Mode {
   enum Type {
     // The default state means anything but the following states.
@@ -19,11 +49,11 @@ struct Mode {
     // On the NTP page and the NTP is ready to be displayed.
     MODE_NTP,
 
-    // Any of the following:
-    // . on the NTP page and the Omnibox is modified in some way.
-    // . on a search results page.
-    // . the Omnibox has focus.
-    MODE_SEARCH,
+    // On the NTP page and the Omnibox is modified in some way.
+    MODE_SEARCH_SUGGESTIONS,
+
+    // On a search results page.
+    MODE_SEARCH_RESULTS,
   };
 
   Mode() : mode(MODE_DEFAULT), animate(false) {
@@ -46,7 +76,7 @@ struct Mode {
   }
 
   bool is_search() const {
-    return mode == MODE_SEARCH;
+    return mode == MODE_SEARCH_SUGGESTIONS || mode == MODE_SEARCH_RESULTS;
   }
 
   Type mode;

chrome/browser/ui/views/frame/browser_view.cc

@@ -867,7 +867,8 @@ SkColor BrowserView::GetToolbarBackgroundColor(
       return theme_provider->GetColor(
           ThemeService::COLOR_SEARCH_NTP_BACKGROUND);
 
-    case chrome::search::Mode::MODE_SEARCH:
+    case chrome::search::Mode::MODE_SEARCH_SUGGESTIONS:
+    case chrome::search::Mode::MODE_SEARCH_RESULTS:
       return theme_provider->GetColor(
           ThemeService::COLOR_SEARCH_SEARCH_BACKGROUND);
 
@@ -889,7 +890,8 @@ gfx::ImageSkia* BrowserView::GetToolbarBackgroundImage(
     case chrome::search::Mode::MODE_NTP:
       return theme_provider->GetImageSkiaNamed(IDR_THEME_NTP_BACKGROUND);
 
-    case chrome::search::Mode::MODE_SEARCH:
+    case chrome::search::Mode::MODE_SEARCH_SUGGESTIONS:
+    case chrome::search::Mode::MODE_SEARCH_RESULTS:
     case chrome::search::Mode::MODE_DEFAULT:
     default:
       return theme_provider->GetImageSkiaNamed(IDR_THEME_TOOLBAR_SEARCH);

chrome/browser/ui/views/search_view_controller.cc

@@ -299,8 +299,8 @@ void SearchViewController::ModeChanged(const chrome::search::Mode& old_mode,
 }
 
 void SearchViewController::OnImplicitAnimationsCompleted() {
-  DCHECK_EQ(STATE_ANIMATING, state_);
-  state_ = STATE_SEARCH;
+  DCHECK_EQ(STATE_NTP_ANIMATING, state_);
+  state_ = STATE_SUGGESTIONS;
   ntp_view_->SetVisible(false);
   // While |ntp_view_| was fading out, location bar was animating from the
   // middle of the NTP page to the top toolbar, at the same rate.
@@ -326,16 +326,15 @@ void SearchViewController::UpdateState() {
       new_state = STATE_NTP;
       break;
 
-    case chrome::search::Mode::MODE_SEARCH:
-      if (search_model()->mode().animate && state_ == STATE_NTP) {
-        new_state = STATE_ANIMATING;
-      } else {
-        // Only enter into MODE_SEARCH if the omnibox is visible.
-        if (omnibox_popup_view_parent_->is_child_visible())
-          new_state = STATE_SEARCH;
-        else
+    case chrome::search::Mode::MODE_SEARCH_SUGGESTIONS:
+      if (search_model()->mode().animate && state_ == STATE_NTP)
+        new_state = STATE_NTP_ANIMATING;
+      else if (omnibox_popup_view_parent_->is_child_visible())
+        new_state = STATE_SUGGESTIONS;
+      break;
+
+    case chrome::search::Mode::MODE_SEARCH_RESULTS:
       new_state = STATE_NOT_VISIBLE;
-      }
       break;
   }
   SetState(new_state);
@@ -357,13 +356,13 @@ void SearchViewController::SetState(State state) {
       CreateViews();
       break;
 
-    case STATE_ANIMATING:
+    case STATE_NTP_ANIMATING:
       // Should only animate from the ntp.
       DCHECK_EQ(STATE_NTP, old_state);
       StartAnimation();
       break;
 
-    case STATE_SEARCH:
+    case STATE_SUGGESTIONS:
       DestroyViews();
       CreateViews();
       ntp_view_->SetVisible(false);
@@ -477,7 +476,7 @@ void SearchViewController::DestroyViews() {
 void SearchViewController::PopupVisibilityChanged() {
   // Don't do anything while animating if the child is visible. Otherwise we'll
   // prematurely cancel the animation.
-  if (state_ != STATE_ANIMATING ||
+  if (state_ != STATE_NTP_ANIMATING ||
       !omnibox_popup_view_parent_->is_child_visible()) {
     UpdateState();
   }

chrome/browser/ui/views/search_view_controller.h

@@ -72,11 +72,11 @@ class SearchViewController
     // Layout for the new tab page.
     STATE_NTP,
 
-    // Animating between STATE_NTP and STATE_SEARCH.
-    STATE_ANIMATING,
+    // Animating between STATE_NTP and STATE_SUGGESTIONS.
+    STATE_NTP_ANIMATING,
 
-    // Search layout. This is only used when the omnibox is visible.
-    STATE_SEARCH,
+    // Search layout. This is only used when the suggestions UI is visible.
+    STATE_SUGGESTIONS,
   };
 
   class OmniboxPopupViewParent;

chrome/browser/ui/views/tabs/tab.cc

@@ -508,7 +508,8 @@ gfx::ImageSkia* Tab::GetTabBackgroundImage(
     case chrome::search::Mode::MODE_NTP:
       return tp->GetImageSkiaNamed(IDR_THEME_NTP_BACKGROUND);
 
-    case chrome::search::Mode::MODE_SEARCH:
+    case chrome::search::Mode::MODE_SEARCH_SUGGESTIONS:
+    case chrome::search::Mode::MODE_SEARCH_RESULTS:
     case chrome::search::Mode::MODE_DEFAULT:
     default:
       return tp->GetImageSkiaNamed(IDR_THEME_TOOLBAR_SEARCH);