Create GoogleURLTrackerClient interface and //chrome implementation

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 <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"
 #include "chrome/test/base/testing_profile.h"
+#include "components/google/core/browser/google_url_tracker_client.h"
 #include "components/infobars/core/infobar.h"
 #include "components/infobars/core/infobar_delegate.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "net/url_request/test_url_fetcher_factory.h"
 #include "net/url_request/url_fetcher.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 class GoogleURLTrackerTest;
 
@@ skipping 59 matching lines @@
 }
 
 void TestNotificationObserver::Observe(
     int type,
     const content::NotificationSource& source,
     const content::NotificationDetails& details) {
   notified_ = true;
 }
 
 
+// TestGoogleURLTrackerClient -------------------------------------

[Peter Kasting, 2014/05/14 23:30:28]

Tiny nit: Can you make all the lines of dashes in this file go out to 79 chars
like the first two lines (above) do?

[blundell, 2014/05/15 07:58:03]

Done.

+
+class TestGoogleURLTrackerClient
+    : public GoogleURLTrackerClient {

[Peter Kasting, 2014/05/14 23:30:28]

Nit: Linebreak not necessary

[blundell, 2014/05/15 07:58:03]

Done.

+ public:
+  TestGoogleURLTrackerClient();
+  virtual ~TestGoogleURLTrackerClient();
+
+  virtual void SetGoogleURLTracker(GoogleURLTracker* tracker) OVERRIDE;
+  virtual void SetListeningForNavigationStart(bool listen) OVERRIDE;
+  virtual bool IsListeningForNavigationStart() OVERRIDE;
+
+ private:
+  GoogleURLTracker* tracker_;
+  bool observe_nav_start_;
+};

[Peter Kasting, 2014/05/14 23:30:28]

Nit: DISALLOW_COPY_AND_ASSIGN

[blundell, 2014/05/15 07:58:03]

Done.

+
+TestGoogleURLTrackerClient::TestGoogleURLTrackerClient()
+    : tracker_(NULL),
+      observe_nav_start_(false) {
+}
+
+TestGoogleURLTrackerClient::
+    ~TestGoogleURLTrackerClient() {

[Peter Kasting, 2014/05/14 23:30:28]

Nit: Linebreak not necessary

[blundell, 2014/05/15 07:58:03]

Done.

+}
+
+void TestGoogleURLTrackerClient::SetGoogleURLTracker(
+    GoogleURLTracker* tracker) {
+  tracker_ = tracker;

[Peter Kasting, 2014/05/14 23:30:28]

Since both GoogleURLTrackerClient implementations seem to want to do this, maybe
the base class should have a GoogleURLTracker* member, and define a non-virtual
cheap public setter ("void set_google_url_tracker(GoogleURLTracker*
google_url_tracker);") as well as a protected getter ("GoogleURLTracker*
google_url_tracker();") for the implementations to use.

I realize this will make the base class non-pure, but I don't think that's a
problem.  If being able to refer to the owning tracker is a core concept that
GoogleURLTrackerClients need to do, it makes sense to go ahead and put that
concrete implementation in the base class.

[blundell, 2014/05/15 07:58:03]

Done.

+}
+
+void TestGoogleURLTrackerClient::SetListeningForNavigationStart(
+    bool listen) {
+  observe_nav_start_ = listen;
+}
+
+bool TestGoogleURLTrackerClient::IsListeningForNavigationStart() {
+  return observe_nav_start_;
+}
+
 // TestGoogleURLTrackerNavigationHelper -------------------------------------
 
 class TestGoogleURLTrackerNavigationHelper
     : public GoogleURLTrackerNavigationHelper {
  public:
   TestGoogleURLTrackerNavigationHelper();
   virtual ~TestGoogleURLTrackerNavigationHelper();
 
   virtual void SetGoogleURLTracker(GoogleURLTracker* tracker) OVERRIDE;
-  virtual void SetListeningForNavigationStart(bool listen) OVERRIDE;
-  virtual bool IsListeningForNavigationStart() OVERRIDE;
   virtual void SetListeningForNavigationCommit(
       const content::NavigationController* nav_controller,
       bool listen) OVERRIDE;
   virtual bool IsListeningForNavigationCommit(
       const content::NavigationController* nav_controller) OVERRIDE;
   virtual void SetListeningForTabDestruction(
       const content::NavigationController* nav_controller,
       bool listen) OVERRIDE;
   virtual bool IsListeningForTabDestruction(
       const content::NavigationController* nav_controller) OVERRIDE;
 
  private:
   GoogleURLTracker* tracker_;
-  bool observe_nav_start_;
   std::set<const content::NavigationController*>
       nav_controller_commit_listeners_;
   std::set<const content::NavigationController*>
       nav_controller_tab_close_listeners_;
 };
 
 TestGoogleURLTrackerNavigationHelper::TestGoogleURLTrackerNavigationHelper()
-    : tracker_(NULL),
-      observe_nav_start_(false) {
+    : tracker_(NULL) {
 }
 
 TestGoogleURLTrackerNavigationHelper::
     ~TestGoogleURLTrackerNavigationHelper() {
 }
 
 void TestGoogleURLTrackerNavigationHelper::SetGoogleURLTracker(
     GoogleURLTracker* tracker) {
   tracker_ = tracker;
 }
 
-void TestGoogleURLTrackerNavigationHelper::SetListeningForNavigationStart(
-    bool listen) {
-  observe_nav_start_ = listen;
-}
-
-bool TestGoogleURLTrackerNavigationHelper::IsListeningForNavigationStart() {
-  return observe_nav_start_;
-}
-
 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(
@@ skipping 85 matching lines @@
 
   // 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_;
   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,
@@ skipping 18 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 a weak pointer is kept;
-  // this is safe since GoogleURLTracker keeps the observer for its lifetime.
+  // Ownership is passed to google_url_tracker_, but weak pointers are kept;
+  // this is safe since GoogleURLTracker keeps these objects 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_, nav_helper.Pass(),
+      new GoogleURLTracker(&profile_, client.Pass(), nav_helper.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());
 
+  client_ = NULL;
   nav_helper_ = NULL;
   google_url_tracker_.reset();
   network_change_notifier_.reset();

[Peter Kasting, 2014/05/14 23:30:28]

Why are any of these four lines necessary?

[blundell, 2014/05/15 07:58:03]

Removed.

On 2014/05/14 23:30:28, Peter Kasting wrote:

 }
 
 net::TestURLFetcher* GoogleURLTrackerTest::GetFetcher() {
   // This will return the last fetcher created.  If no fetchers have been
   // created, we'll pass GetFetcherByID() "-1", and it will return NULL.
   return fetcher_factory_.GetFetcherByID(google_url_tracker_->fetcher_id_ - 1);
 }
 
 void GoogleURLTrackerTest::MockSearchDomainCheckResponse(
     const std::string& domain) {
@@ skipping 38 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 (nav_helper_->IsListeningForNavigationStart()) {
+  if (client_->IsListeningForNavigationStart()) {
     google_url_tracker_->OnNavigationPending(
         reinterpret_cast<content::NavigationController*>(unique_id),
         reinterpret_cast<InfoBarService*>(unique_id), unique_id);
   }
 }
 
 void GoogleURLTrackerTest::CommitNonSearch(intptr_t unique_id) {
   GoogleURLTrackerMapEntry* map_entry = GetMapEntry(unique_id);
   if (!map_entry)
     return;
@@ skipping 696 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));
 }