Instant: Move SearchModel and SearchDelegate out of Browser

...and into BrowserInstantController. This reduces the number of
"Instant things" that Browser has to know about from 4 to 2.

Bug: 627747
Change-Id: I1d9dbfe37bda88e59ce6fa4759f8b3a8428eaacc
Reviewed-on: https://chromium-review.googlesource.com/678502Reviewed-by: Michael Wasserman <msw@chromium.org>
Reviewed-by: Chris Pickel <sfiera@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504309}

chrome/browser/ui/browser.cc

@@ -126,8 +126,6 @@
 #include "chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.h"
 #include "chrome/browser/ui/location_bar/location_bar.h"
 #include "chrome/browser/ui/permission_bubble/chooser_bubble_delegate.h"
-#include "chrome/browser/ui/search/search_delegate.h"
-#include "chrome/browser/ui/search/search_model.h"
 #include "chrome/browser/ui/search/search_tab_helper.h"
 #include "chrome/browser/ui/singleton_tabs.h"
 #include "chrome/browser/ui/status_bubble.h"
@@ -409,8 +407,6 @@ Browser::Browser(const CreateParams& params)
 
   toolbar_model_.reset(new ToolbarModelImpl(toolbar_model_delegate_.get(),
                                             content::kMaxURLDisplayChars));
-  search_model_.reset(new SearchModel());
-  search_delegate_.reset(new SearchDelegate(search_model_.get()));
 
   extension_registry_observer_.Add(
       extensions::ExtensionRegistry::Get(profile_));
@@ -1027,7 +1023,8 @@ void Browser::TabDetachedAt(WebContents* contents, int index) {
 
 void Browser::TabDeactivated(WebContents* contents) {
   exclusive_access_manager_->OnTabDeactivated(contents);
-  search_delegate_->OnTabDeactivated(contents);
+  if (instant_controller_)
+    instant_controller_->OnTabDeactivated(contents);
   SearchTabHelper::FromWebContents(contents)->OnTabDeactivated();
 
   // Save what the user's currently typing, so it can be restored when we
@@ -1082,8 +1079,8 @@ void Browser::ActiveTabChanged(WebContents* old_contents,
   // Propagate the profile to the location bar.
   UpdateToolbar((reason & CHANGE_REASON_REPLACED) == 0);
 
-  if (search::IsInstantExtendedAPIEnabled())
-    search_delegate_->OnTabActivated(new_contents);
+  if (instant_controller_)
+    instant_controller_->OnTabActivated(new_contents);
 
   // Update reload/stop state.
   command_controller_->LoadingStateChanged(new_contents->IsLoading(), true);
@@ -1121,10 +1118,6 @@ void Browser::ActiveTabChanged(WebContents* old_contents,
                                        base::TimeTicks::Now());
   }
 
-  // This needs to be called after notifying SearchDelegate.
-  if (instant_controller_)
-    instant_controller_->ActiveTabChanged();
-
   SearchTabHelper::FromWebContents(new_contents)->OnTabActivated();
 }
 
@@ -2392,8 +2385,8 @@ void Browser::TabDetachedAtImpl(content::WebContents* contents,
     find_bar_controller_->ChangeWebContents(NULL);
   }
 
-  // Stop observing search model changes for this tab.
-  search_delegate_->OnTabDetached(contents);
+  if (instant_controller_)
+    instant_controller_->OnTabDetached(contents);
 
   for (size_t i = 0; i < interstitial_observers_.size(); i++) {
     if (interstitial_observers_[i]->web_contents() != contents)

chrome/browser/ui/browser.h

@@ -70,8 +70,6 @@ class BrowserWindow;
 class FindBarController;
 class Profile;
 class ScopedKeepAlive;
-class SearchDelegate;
-class SearchModel;
 class StatusBubble;
 class TabStripModel;
 class TabStripModelDelegate;
@@ -264,13 +262,6 @@ class Browser : public TabStripModelObserver,
   chrome::BrowserCommandController* command_controller() {
     return command_controller_.get();
   }
-  SearchModel* search_model() { return search_model_.get(); }
-  const SearchModel* search_model() const {
-      return search_model_.get();
-  }
-  SearchDelegate* search_delegate() {
-    return search_delegate_.get();
-  }
   const SessionID& session_id() const { return session_id_; }
   BrowserContentSettingBubbleModelDelegate*
       content_setting_bubble_model_delegate() {
@@ -918,13 +909,6 @@ class Browser : public TabStripModelObserver,
   // The model for the toolbar view.
   std::unique_ptr<ToolbarModel> toolbar_model_;
 
-  // The model for the "active" search state.  There are per-tab search models
-  // as well.  When a tab is active its model is kept in sync with this one.
-  // When a new tab is activated its model state is propagated to this active
-  // model.  This way, observers only have to attach to this single model for
-  // updates, and don't have to worry about active tab changes directly.
-  std::unique_ptr<SearchModel> search_model_;
-
   // UI update coalescing and handling ////////////////////////////////////////
 
   typedef std::map<const content::WebContents*, int> UpdateMap;
@@ -982,10 +966,6 @@ class Browser : public TabStripModelObserver,
   // Helper which implements the ToolbarModelDelegate interface.
   std::unique_ptr<BrowserToolbarModelDelegate> toolbar_model_delegate_;
 
-  // A delegate that handles the details of updating the "active"
-  // |search_model_| state with the tab's state.
-  std::unique_ptr<SearchDelegate> search_delegate_;
-
   // Helper which implements the LiveTabContext interface.
   std::unique_ptr<BrowserLiveTabContext> live_tab_context_;
 

chrome/browser/ui/browser_instant_controller.cc

@@ -78,10 +78,7 @@ DEFINE_WEB_CONTENTS_USER_DATA_KEY(TabReloader);
 // BrowserInstantController ---------------------------------------------------
 
 BrowserInstantController::BrowserInstantController(Browser* browser)
-    : browser_(browser),
-      instant_(this) {
-  browser_->search_model()->AddObserver(this);
-
+    : browser_(browser), search_delegate_(&search_model_), instant_(this) {
   TemplateURLService* template_url_service =
       TemplateURLServiceFactory::GetForProfile(profile());
   // TemplateURLService can be null in tests.
@@ -95,9 +92,7 @@ BrowserInstantController::BrowserInstantController(Browser* browser)
   }
 }
 
-BrowserInstantController::~BrowserInstantController() {
-  browser_->search_model()->RemoveObserver(this);
-}
+BrowserInstantController::~BrowserInstantController() = default;
 
 Profile* BrowserInstantController::profile() const {
   return browser_->profile();
@@ -107,13 +102,21 @@ content::WebContents* BrowserInstantController::GetActiveWebContents() const {
   return browser_->tab_strip_model()->GetActiveWebContents();
 }
 
-void BrowserInstantController::ActiveTabChanged() {
+void BrowserInstantController::OnTabActivated(
+    content::WebContents* web_contents) {
+  // Note: Order matters, |search_delegate_| must be notified first.
+  search_delegate_.OnTabActivated(web_contents);
   instant_.ActiveTabChanged();
 }
 
-void BrowserInstantController::ModelChanged(SearchModel::Origin old_origin,
-                                            SearchModel::Origin new_origin) {
-  instant_.SearchModeChanged(old_origin, new_origin);
+void BrowserInstantController::OnTabDeactivated(
+    content::WebContents* web_contents) {
+  search_delegate_.OnTabDeactivated(web_contents);
+}
+
+void BrowserInstantController::OnTabDetached(
+    content::WebContents* web_contents) {
+  search_delegate_.OnTabDetached(web_contents);
 }
 
 void BrowserInstantController::OnSearchEngineBaseURLChanged(

chrome/browser/ui/browser_instant_controller.h

@@ -11,7 +11,8 @@
 #include "base/macros.h"
 #include "chrome/browser/search/search_engine_base_url_tracker.h"
 #include "chrome/browser/ui/search/instant_controller.h"
-#include "chrome/browser/ui/search/search_model_observer.h"
+#include "chrome/browser/ui/search/search_delegate.h"
+#include "chrome/browser/ui/search/search_model.h"
 
 class Browser;
 class Profile;
@@ -20,32 +21,50 @@ namespace content {
 class WebContents;
 }
 
-class BrowserInstantController : public SearchModelObserver {
+// BrowserInstantController is responsible for reloading any Instant tabs (which
+// today just means NTPs) when the default search provider changes. This can
+// happen when the user chooses a different default search engine, or when the
+// Google base URL changes while Google is the default search engine. Also owns
+// a SearchModel instance that corresponds to the currently-active tab.
+class BrowserInstantController {
  public:
   explicit BrowserInstantController(Browser* browser);
-  ~BrowserInstantController() override;
+  ~BrowserInstantController();
 
   // Returns the Profile associated with the Browser that owns this object.
   Profile* profile() const;
 
   InstantController* instant() { return &instant_; }
 
+  SearchModel* search_model() { return &search_model_; }
+
   // Invoked by |instant_| to get the currently active tab.
   content::WebContents* GetActiveWebContents() const;
 
   // Invoked by |browser_| when the active tab changes.
-  void ActiveTabChanged();
+  // TODO(treib): Implement TabStripModelObserver instead of relying on custom
+  // callbacks from Browser.
+  void OnTabActivated(content::WebContents* web_contents);
+  void OnTabDeactivated(content::WebContents* web_contents);
+  void OnTabDetached(content::WebContents* web_contents);
 
  private:
-  // SearchModelObserver:
-  void ModelChanged(SearchModel::Origin old_origin,
-                    SearchModel::Origin new_origin) override;
-
   void OnSearchEngineBaseURLChanged(
       SearchEngineBaseURLTracker::ChangeReason change_reason);
 
   Browser* const browser_;
 
+  // The model for the "active" search state.  There are per-tab search models
+  // as well.  When a tab is active its model is kept in sync with this one.
+  // When a new tab is activated its model state is propagated to this active
+  // model.  This way, observers only have to attach to this single model for
+  // updates, and don't have to worry about active tab changes directly.
+  SearchModel search_model_;
+
+  // A delegate that handles the details of updating the "active"
+  // |search_model_| state with the tab's state.
+  SearchDelegate search_delegate_;
+
   InstantController instant_;
 
   std::unique_ptr<SearchEngineBaseURLTracker> search_engine_base_url_tracker_;

chrome/browser/ui/search/instant_controller.cc

@@ -15,6 +15,7 @@
 #include "chrome/browser/search/instant_service.h"
 #include "chrome/browser/search/instant_service_factory.h"
 #include "chrome/browser/ui/browser_instant_controller.h"
+#include "chrome/browser/ui/search/search_tab_helper.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_observer.h"
@@ -40,17 +41,26 @@ class InstantController::TabObserver : public content::WebContentsObserver {
   DISALLOW_COPY_AND_ASSIGN(TabObserver);
 };
 
-InstantController::InstantController(BrowserInstantController* browser)
-    : browser_(browser), search_origin_(SearchModel::Origin::DEFAULT) {}
+InstantController::InstantController(
+    BrowserInstantController* browser_instant_controller)
+    : browser_instant_controller_(browser_instant_controller) {
+  browser_instant_controller_->search_model()->AddObserver(this);
+}
 
-InstantController::~InstantController() = default;
+InstantController::~InstantController() {
+  browser_instant_controller_->search_model()->RemoveObserver(this);
+}
 
-void InstantController::SearchModeChanged(SearchModel::Origin old_origin,
-                                          SearchModel::Origin new_origin) {
-  LogDebugEvent(base::StringPrintf("SearchModeChanged: %d to %d", old_origin,
-                                   new_origin));
+void InstantController::ModelChanged(SearchModel::Origin old_origin,
+                                     SearchModel::Origin new_origin) {
+  // The search mode in the active tab has changed. Bind |instant_tab_observer_|
+  // if the |new_origin| reflects an Instant NTP.
+  // Note: This can be called either because the SearchMode changed within the
+  // current tab, or because the active tab changed. In the latter case, this
+  // gets called before ActiveTabChanged().
+  LogDebugEvent(
+      base::StringPrintf("ModelChanged: %d to %d", old_origin, new_origin));
 
-  search_origin_ = new_origin;
   ResetInstantTab();
 }
 
@@ -85,8 +95,13 @@ void InstantController::InstantTabAboutToNavigateMainFrame() {
 }
 
 void InstantController::ResetInstantTab() {
-  if (search_origin_ == SearchModel::Origin::NTP) {
-    content::WebContents* active_tab = browser_->GetActiveWebContents();
+  content::WebContents* active_tab =
+      browser_instant_controller_->GetActiveWebContents();
+  if (active_tab &&
+      SearchTabHelper::FromWebContents(active_tab)->model()->origin() ==
+          SearchModel::Origin::NTP) {
+    // The active tab is an NTP. If we're not already tracking it, do so and
+    // also update the required info.
     if (!instant_tab_observer_ ||
         active_tab != instant_tab_observer_->web_contents()) {
       instant_tab_observer_ = base::MakeUnique<TabObserver>(
@@ -96,14 +111,14 @@ void InstantController::ResetInstantTab() {
       UpdateInfoForInstantTab();
     }
   } else {
-    instant_tab_observer_.reset();
+    instant_tab_observer_ = nullptr;
   }
 }
 
 void InstantController::UpdateInfoForInstantTab() {
   DCHECK(instant_tab_observer_);
-  InstantService* instant_service =
-      InstantServiceFactory::GetForProfile(browser_->profile());
+  InstantService* instant_service = InstantServiceFactory::GetForProfile(
+      browser_instant_controller_->profile());
   if (instant_service) {
     instant_service->UpdateThemeInfo();
     instant_service->UpdateMostVisitedItemsInfo();

chrome/browser/ui/search/instant_controller.h

@@ -14,7 +14,7 @@
 
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
-#include "chrome/browser/ui/search/search_model.h"
+#include "chrome/browser/ui/search/search_model_observer.h"
 
 class BrowserInstantController;
 
@@ -24,15 +24,15 @@ class BrowserInstantController;
 // * an open tab navigates to an NTP.
 //
 // InstantController is owned by Browser via BrowserInstantController.
-class InstantController {
+class InstantController : public SearchModelObserver {
  public:
-  explicit InstantController(BrowserInstantController* browser);
-  ~InstantController();
+  explicit InstantController(
+      BrowserInstantController* browser_instant_controller);
+  ~InstantController() override;
 
-  // The search mode in the active tab has changed. Bind |instant_tab_observer_|
-  // if the |new_origin| reflects an Instant NTP.
-  void SearchModeChanged(SearchModel::Origin old_origin,
-                         SearchModel::Origin new_origin);
+  // SearchModelObserver:
+  void ModelChanged(SearchModel::Origin old_origin,
+                    SearchModel::Origin new_origin) override;
 
   // The user switched tabs. Bind |instant_tab_observer_| if the newly active
   // tab is an Instant NTP.
@@ -69,14 +69,11 @@ class InstantController {
   // Sends theme info and most visited items to the Instant renderer process.
   void UpdateInfoForInstantTab();
 
-  BrowserInstantController* const browser_;
+  BrowserInstantController* const browser_instant_controller_;
 
   // Only non-null if the current tab is an Instant tab, i.e. an NTP.
   std::unique_ptr<TabObserver> instant_tab_observer_;
 
-  // The search model mode for the active tab.
-  SearchModel::Origin search_origin_;
-
   // List of events and their timestamps, useful in debugging Instant behaviour.
   mutable std::list<std::pair<int64_t, std::string>> debug_events_;
 

chrome/browser/ui/search/search_delegate_unittest.cc

@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_instant_controller.h"
 #include "chrome/browser/ui/search/search_model.h"
 #include "chrome/browser/ui/search/search_tab_helper.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
@@ -13,8 +14,11 @@ typedef BrowserWithTestWindowTest SearchDelegateTest;
 // Test the propagation of search "mode" changes from the tab's search model to
 // the browser's search model.
 TEST_F(SearchDelegateTest, SearchModel) {
+  const SearchModel& browser_search_model =
+      *browser()->instant_controller()->search_model();
+
   // Initial state.
-  EXPECT_EQ(SearchModel::Origin::DEFAULT, browser()->search_model()->origin());
+  EXPECT_EQ(SearchModel::Origin::DEFAULT, browser_search_model.origin());
 
   // Propagate change from tab's search model to browser's search model.
   AddTab(browser(), GURL("http://foo/0"));
@@ -22,7 +26,7 @@ TEST_F(SearchDelegateTest, SearchModel) {
       browser()->tab_strip_model()->GetWebContentsAt(0);
   SearchTabHelper::FromWebContents(first_tab)->model()->SetOrigin(
       SearchModel::Origin::NTP);
-  EXPECT_EQ(SearchModel::Origin::NTP, browser()->search_model()->origin());
+  EXPECT_EQ(SearchModel::Origin::NTP, browser_search_model.origin());
 
   // Add second tab (it gets inserted at index 0), make it active, and make sure
   // its mode changes propagate to the browser's search model.
@@ -31,14 +35,14 @@ TEST_F(SearchDelegateTest, SearchModel) {
       browser()->tab_strip_model()->GetWebContentsAt(0);
   ASSERT_NE(first_tab, second_tab);
   browser()->tab_strip_model()->ActivateTabAt(0, true);
-  EXPECT_EQ(SearchModel::Origin::DEFAULT, browser()->search_model()->origin());
+  EXPECT_EQ(SearchModel::Origin::DEFAULT, browser_search_model.origin());
   SearchTabHelper::FromWebContents(second_tab)
       ->model()
       ->SetOrigin(SearchModel::Origin::NTP);
-  EXPECT_EQ(SearchModel::Origin::NTP, browser()->search_model()->origin());
+  EXPECT_EQ(SearchModel::Origin::NTP, browser_search_model.origin());
 
   // The first tab is not active so changes should not propagate.
   SearchTabHelper::FromWebContents(first_tab)->model()->SetOrigin(
       SearchModel::Origin::DEFAULT);
-  EXPECT_EQ(SearchModel::Origin::NTP, browser()->search_model()->origin());
+  EXPECT_EQ(SearchModel::Origin::NTP, browser_search_model.origin());
 }