Turn GoogleURLTrackerNavigationHelper(Impl) into a per-tab object.

chrome/browser/google/google_url_tracker_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/google/google_url_tracker.h"
 
+#include <map>
 #include <set>
 #include <string>
 
 #include "base/message_loop/message_loop.h"
 #include "base/prefs/pref_service.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/google/google_url_tracker_factory.h"
 #include "chrome/browser/google/google_url_tracker_infobar_delegate.h"
 #include "chrome/browser/google/google_url_tracker_navigation_helper.h"
 #include "chrome/common/pref_names.h"
@@ skipping 70 matching lines @@
 TestNotificationObserver::~TestNotificationObserver() {
 }
 
 void TestNotificationObserver::Observe(
     int type,
     const content::NotificationSource& source,
     const content::NotificationDetails& details) {
   notified_ = true;
 }
 
-

[Peter Kasting, 2014/05/15 21:29:52]

Nit: Don't remove this blank line (really, TestGoogleURLTrackerNavigationHelper
should have another blank above it too)

[blundell, 2014/05/16 11:54:18]

Done.

 // TestGoogleURLTrackerClient -------------------------------------------------
 
 class TestGoogleURLTrackerClient : public GoogleURLTrackerClient {
  public:
   TestGoogleURLTrackerClient();
   virtual ~TestGoogleURLTrackerClient();
 
   virtual void SetListeningForNavigationStart(bool listen) OVERRIDE;
   virtual bool IsListeningForNavigationStart() OVERRIDE;
 
  private:
   bool observe_nav_start_;
 
   DISALLOW_COPY_AND_ASSIGN(TestGoogleURLTrackerClient);
 };
 
 TestGoogleURLTrackerClient::TestGoogleURLTrackerClient()
     : observe_nav_start_(false) {
 }
 
 TestGoogleURLTrackerClient::~TestGoogleURLTrackerClient() {
 }
 
-void TestGoogleURLTrackerClient::SetListeningForNavigationStart(
-    bool listen) {
+void TestGoogleURLTrackerClient::SetListeningForNavigationStart(bool listen) {
   observe_nav_start_ = listen;
 }
 
 bool TestGoogleURLTrackerClient::IsListeningForNavigationStart() {
   return observe_nav_start_;
 }
 
 // TestGoogleURLTrackerNavigationHelper ---------------------------------------
 
 class TestGoogleURLTrackerNavigationHelper
     : public GoogleURLTrackerNavigationHelper {
  public:
-  TestGoogleURLTrackerNavigationHelper();
+  TestGoogleURLTrackerNavigationHelper(GoogleURLTracker* tracker);

[Peter Kasting, 2014/05/15 21:29:52]

Nit: Explicit

[blundell, 2014/05/16 11:54:18]

Done.

   virtual ~TestGoogleURLTrackerNavigationHelper();
 
-  virtual void SetGoogleURLTracker(GoogleURLTracker* tracker) OVERRIDE;
   virtual void SetListeningForNavigationCommit(
-      const content::NavigationController* nav_controller,
       bool listen) OVERRIDE;
-  virtual bool IsListeningForNavigationCommit(
-      const content::NavigationController* nav_controller) OVERRIDE;
+  virtual bool IsListeningForNavigationCommit() OVERRIDE;
   virtual void SetListeningForTabDestruction(
-      const content::NavigationController* nav_controller,
       bool listen) OVERRIDE;
-  virtual bool IsListeningForTabDestruction(
-      const content::NavigationController* nav_controller) OVERRIDE;
+  virtual bool IsListeningForTabDestruction() OVERRIDE;
 
  private:
-  GoogleURLTracker* tracker_;
-  std::set<const content::NavigationController*>
-      nav_controller_commit_listeners_;
-  std::set<const content::NavigationController*>
-      nav_controller_tab_close_listeners_;
+  bool listening_for_nav_commit_;
+  bool listening_for_tab_destruction_;
 };

[Peter Kasting, 2014/05/15 21:29:52]

Nit: While here: DISALLOW_COPY_AND_ASSIGN

[blundell, 2014/05/16 11:54:18]

Done.

 
-TestGoogleURLTrackerNavigationHelper::TestGoogleURLTrackerNavigationHelper()
-    : tracker_(NULL) {
+TestGoogleURLTrackerNavigationHelper::TestGoogleURLTrackerNavigationHelper(
+    GoogleURLTracker* tracker)
+    : GoogleURLTrackerNavigationHelper(tracker),
+      listening_for_nav_commit_(false),
+      listening_for_tab_destruction_(false) {
 }
 
 TestGoogleURLTrackerNavigationHelper::
     ~TestGoogleURLTrackerNavigationHelper() {
 }
 
-void TestGoogleURLTrackerNavigationHelper::SetGoogleURLTracker(
-    GoogleURLTracker* tracker) {
-  tracker_ = tracker;
+void TestGoogleURLTrackerNavigationHelper::SetListeningForNavigationCommit(
+    bool listen) {
+  listening_for_nav_commit_ = listen;
 }
 
-void TestGoogleURLTrackerNavigationHelper::SetListeningForNavigationCommit(
-    const content::NavigationController* nav_controller,
-    bool listen) {
-  if (listen)
-    nav_controller_commit_listeners_.insert(nav_controller);
-  else
-    nav_controller_commit_listeners_.erase(nav_controller);
-}
-
-bool TestGoogleURLTrackerNavigationHelper::IsListeningForNavigationCommit(
-    const content::NavigationController* nav_controller) {
-  return nav_controller_commit_listeners_.count(nav_controller) > 0;
+bool TestGoogleURLTrackerNavigationHelper::IsListeningForNavigationCommit() {
+  return listening_for_nav_commit_;
 }
 
 void TestGoogleURLTrackerNavigationHelper::SetListeningForTabDestruction(
-    const content::NavigationController* nav_controller,
     bool listen) {
-  if (listen)
-    nav_controller_tab_close_listeners_.insert(nav_controller);
-  else
-    nav_controller_tab_close_listeners_.erase(nav_controller);
+  listening_for_tab_destruction_ = listen;
 }
 
-bool TestGoogleURLTrackerNavigationHelper::IsListeningForTabDestruction(
-    const content::NavigationController* nav_controller) {
-  return nav_controller_tab_close_listeners_.count(nav_controller) > 0;
+bool TestGoogleURLTrackerNavigationHelper::IsListeningForTabDestruction() {
+  return listening_for_tab_destruction_;
 }
 
 }  // namespace
 
 
 // GoogleURLTrackerTest -------------------------------------------------------
 
 // Ths class exercises GoogleURLTracker.  In order to avoid instantiating more
 // of the Chrome infrastructure than necessary, the GoogleURLTracker functions
 // are carefully written so that many of the functions which take
@@ skipping 36 matching lines @@
   }
   GURL google_url() const { return google_url_tracker_->google_url(); }
   void SetLastPromptedGoogleURL(const GURL& url);
   GURL GetLastPromptedGoogleURL();
   void SetNavigationPending(intptr_t unique_id, bool is_search);
   void CommitNonSearch(intptr_t unique_id);
   void CommitSearch(intptr_t unique_id, const GURL& search_url);
   void CloseTab(intptr_t unique_id);
   GoogleURLTrackerMapEntry* GetMapEntry(intptr_t unique_id);
   GoogleURLTrackerInfoBarDelegate* GetInfoBarDelegate(intptr_t unique_id);
+  GoogleURLTrackerNavigationHelper* GetNavigationHelper(intptr_t unique_id);
   void ExpectDefaultURLs() const;
   void ExpectListeningForCommit(intptr_t unique_id, bool listening);
   bool observer_notified() const { return observer_.notified(); }
   void clear_observer_notified() { observer_.clear_notified(); }
 
  private:
   // Since |infobar_service| is really a magic number rather than an actual
   // object, we don't add the created infobar to it.  Instead we will simulate
   // any helper<->infobar interaction necessary.  The returned object will be
   // cleaned up in OnInfoBarClosed().
   infobars::InfoBar* CreateTestInfoBar(InfoBarService* infobar_service,
                                        GoogleURLTracker* google_url_tracker,
                                        const GURL& search_url);
 
   // These are required by the TestURLFetchers GoogleURLTracker will create (see
   // test_url_fetcher_factory.h).
   content::TestBrowserThreadBundle thread_bundle_;
   // Creating this allows us to call
   // net::NetworkChangeNotifier::NotifyObserversOfIPAddressChangeForTests().
   scoped_ptr<net::NetworkChangeNotifier> network_change_notifier_;
   net::TestURLFetcherFactory fetcher_factory_;
   content::NotificationRegistrar registrar_;
   TestNotificationObserver observer_;
   GoogleURLTrackerClient* client_;
-  GoogleURLTrackerNavigationHelper* nav_helper_;
+

[Peter Kasting, 2014/05/15 21:29:52]

Nit: No blank line here

[blundell, 2014/05/16 11:54:18]

Code removed.

On 2014/05/15 21:29:52, Peter Kasting wrote:

+  // Owns created TestGoogleURLTrackerNavigationHelpers to avoid leakage.
+  ScopedVector<GoogleURLTrackerNavigationHelper> nav_helpers_;
   TestingProfile profile_;
   scoped_ptr<GoogleURLTracker> google_url_tracker_;
   // This tracks the different "tabs" a test has "opened", so we can close them
   // properly before shutting down |google_url_tracker_|, which expects that.
   std::set<int> unique_ids_seen_;
 };
 
 void GoogleURLTrackerTest::OnInfoBarClosed(
     scoped_ptr<infobars::InfoBar> infobar,
     InfoBarService* infobar_service) {
@@ skipping 17 matching lines @@
     : thread_bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP) {
   GoogleURLTrackerFactory::GetInstance()->
       RegisterUserPrefsOnBrowserContextForTest(&profile_);
 }
 
 GoogleURLTrackerTest::~GoogleURLTrackerTest() {
 }
 
 void GoogleURLTrackerTest::SetUp() {
   network_change_notifier_.reset(net::NetworkChangeNotifier::CreateMock());
-  // Ownership is passed to google_url_tracker_, but weak pointers are kept;
-  // this is safe since GoogleURLTracker keeps these objects for its lifetime.
+  // Ownership is passed to google_url_tracker_, but a weak pointer is kept;
+  // this is safe since GoogleURLTracker keeps the client for its lifetime.
   client_ = new TestGoogleURLTrackerClient();
-  nav_helper_ = new TestGoogleURLTrackerNavigationHelper();
   scoped_ptr<GoogleURLTrackerClient> client(client_);
-  scoped_ptr<GoogleURLTrackerNavigationHelper> nav_helper(nav_helper_);
-  google_url_tracker_.reset(
-      new GoogleURLTracker(&profile_, client.Pass(), nav_helper.Pass(),
-                           GoogleURLTracker::UNIT_TEST_MODE));
+  google_url_tracker_.reset(new GoogleURLTracker(
+      &profile_, client.Pass(), GoogleURLTracker::UNIT_TEST_MODE));
   google_url_tracker_->infobar_creator_ = base::Bind(
       &GoogleURLTrackerTest::CreateTestInfoBar, base::Unretained(this));
 }
 
 void GoogleURLTrackerTest::TearDown() {
   while (!unique_ids_seen_.empty())
     CloseTab(*unique_ids_seen_.begin());
 }
 
 net::TestURLFetcher* GoogleURLTrackerTest::GetFetcher() {
@@ skipping 46 matching lines @@
 
 void GoogleURLTrackerTest::SetNavigationPending(intptr_t unique_id,
                                                 bool is_search) {
   if (is_search) {
     google_url_tracker_->SearchCommitted();
     // Note that the call above might not have actually registered a listener
     // for navigation starts if the searchdomaincheck response was bogus.
   }
   unique_ids_seen_.insert(unique_id);
   if (client_->IsListeningForNavigationStart()) {
+    GoogleURLTrackerNavigationHelper* nav_helper =
+        GetNavigationHelper(unique_id);
+    if (!nav_helper) {
+      nav_helper =
+          new TestGoogleURLTrackerNavigationHelper(google_url_tracker_.get());
+      nav_helpers_.push_back(nav_helper);
+    }
     google_url_tracker_->OnNavigationPending(
-        reinterpret_cast<content::NavigationController*>(unique_id),
-        reinterpret_cast<InfoBarService*>(unique_id), unique_id);
+        nav_helper, reinterpret_cast<InfoBarService*>(unique_id), unique_id);
   }
 }
 
 void GoogleURLTrackerTest::CommitNonSearch(intptr_t unique_id) {
   GoogleURLTrackerMapEntry* map_entry = GetMapEntry(unique_id);
   if (!map_entry)
     return;
 
   ExpectListeningForCommit(unique_id, false);
 
   // The infobar should be showing; otherwise the pending non-search should
   // have closed it.
   ASSERT_TRUE(map_entry->has_infobar_delegate());
 
   // The pending_id should have been reset to 0 when the non-search became
   // pending.
   EXPECT_EQ(0, map_entry->infobar_delegate()->pending_id());
 
   // Committing the navigation would close the infobar.
   map_entry->infobar_delegate()->Close(false);
 }
 
 void GoogleURLTrackerTest::CommitSearch(intptr_t unique_id,
                                         const GURL& search_url) {
   DCHECK(search_url.is_valid());
-  if (nav_helper_->IsListeningForNavigationCommit(
-      reinterpret_cast<content::NavigationController*>(unique_id))) {
+  if (GetNavigationHelper(unique_id) &&

[Peter Kasting, 2014/05/15 21:29:52]

Nit: Factor GetNavigationHelper() calls out to a temp

[blundell, 2014/05/16 11:54:18]

Done.

+      GetNavigationHelper(unique_id)->IsListeningForNavigationCommit()) {
     google_url_tracker_->OnNavigationCommitted(
         reinterpret_cast<InfoBarService*>(unique_id), search_url);
   }
 }
 
 void GoogleURLTrackerTest::CloseTab(intptr_t unique_id) {
   unique_ids_seen_.erase(unique_id);
-  content::NavigationController* nav_controller =
-      reinterpret_cast<content::NavigationController*>(unique_id);
-  if (nav_helper_->IsListeningForTabDestruction(nav_controller)) {
-    google_url_tracker_->OnTabClosed(nav_controller);
+  if (GetNavigationHelper(unique_id) &&

[Peter Kasting, 2014/05/15 21:29:52]

Nit: Factor GetNavigationHelper() calls out to a temp

[blundell, 2014/05/16 11:54:18]

Done.

+      GetNavigationHelper(unique_id)->IsListeningForTabDestruction()) {
+    google_url_tracker_->OnTabClosed(GetNavigationHelper(unique_id));
   } else {
     // Closing a tab with an infobar showing would close the infobar.
     GoogleURLTrackerInfoBarDelegate* delegate = GetInfoBarDelegate(unique_id);
     if (delegate)
       delegate->Close(false);
   }
 }
 
 GoogleURLTrackerMapEntry* GoogleURLTrackerTest::GetMapEntry(
     intptr_t unique_id) {
   GoogleURLTracker::EntryMap::const_iterator i =
       google_url_tracker_->entry_map_.find(
           reinterpret_cast<InfoBarService*>(unique_id));
   return (i == google_url_tracker_->entry_map_.end()) ? NULL : i->second;
 }
 
 GoogleURLTrackerInfoBarDelegate* GoogleURLTrackerTest::GetInfoBarDelegate(
     intptr_t unique_id) {
   GoogleURLTrackerMapEntry* map_entry = GetMapEntry(unique_id);
   return map_entry ? map_entry->infobar_delegate() : NULL;
 }
 
+GoogleURLTrackerNavigationHelper* GoogleURLTrackerTest::GetNavigationHelper(
+    intptr_t unique_id) {
+  GoogleURLTrackerMapEntry* map_entry = GetMapEntry(unique_id);
+  return map_entry ? map_entry->navigation_helper() : NULL;
+}
+
 void GoogleURLTrackerTest::ExpectDefaultURLs() const {
   EXPECT_EQ(GURL(GoogleURLTracker::kDefaultGoogleHomepage), google_url());
   EXPECT_EQ(GURL(), fetched_google_url());
 }
 
 void GoogleURLTrackerTest::ExpectListeningForCommit(intptr_t unique_id,
                                                     bool listening) {
   GoogleURLTrackerMapEntry* map_entry = GetMapEntry(unique_id);
   if (map_entry) {
-    EXPECT_EQ(listening, nav_helper_->IsListeningForNavigationCommit(
-        map_entry->navigation_controller()));
+    EXPECT_EQ(listening,
+              map_entry->navigation_helper()->IsListeningForNavigationCommit());
   } else {
     EXPECT_FALSE(listening);
   }
 }
 
 infobars::InfoBar* GoogleURLTrackerTest::CreateTestInfoBar(
     InfoBarService* infobar_service,
     GoogleURLTracker* google_url_tracker,
     const GURL& search_url) {
   return TestInfoBarDelegate::Create(this, infobar_service, google_url_tracker,
@@ skipping 622 matching lines @@
   CommitSearch(2, GURL("http://www.google.co.uk/search?q=test2"));
   EXPECT_FALSE(GetInfoBarDelegate(1) == NULL);
   GoogleURLTrackerInfoBarDelegate* delegate2 = GetInfoBarDelegate(2);
   ASSERT_FALSE(delegate2 == NULL);
   SetNavigationPending(1, true);
   ASSERT_NO_FATAL_FAILURE(ExpectListeningForCommit(1, true));
   delegate2->Close(false);
   SetNavigationPending(1, false);
   ASSERT_NO_FATAL_FAILURE(ExpectListeningForCommit(1, false));
 }