Create GoogleURLTrackerClient interface and //chrome implementation

chrome/browser/google/google_url_tracker_unittest.cc

Index: chrome/browser/google/google_url_tracker_unittest.cc
diff --git a/chrome/browser/google/google_url_tracker_unittest.cc b/chrome/browser/google/google_url_tracker_unittest.cc
index 1ebe61369cfad3fc5d09681fe5f7d55e12707313..81ddb444f2b013a7c90a851d8f560f2cf61bba3d 100644
--- a/chrome/browser/google/google_url_tracker_unittest.cc
+++ b/chrome/browser/google/google_url_tracker_unittest.cc
@@ -15,6 +15,7 @@
 #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"
@@ -94,6 +95,46 @@ void TestNotificationObserver::Observe(
 }
 
 
+// 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
@@ -103,8 +144,6 @@ class 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;
@@ -118,7 +157,6 @@ class TestGoogleURLTrackerNavigationHelper
 
  private:
   GoogleURLTracker* tracker_;
-  bool observe_nav_start_;
   std::set<const content::NavigationController*>
       nav_controller_commit_listeners_;
   std::set<const content::NavigationController*>
@@ -126,8 +164,7 @@ class TestGoogleURLTrackerNavigationHelper
 };
 
 TestGoogleURLTrackerNavigationHelper::TestGoogleURLTrackerNavigationHelper()
-    : tracker_(NULL),
-      observe_nav_start_(false) {
+    : tracker_(NULL) {
 }
 
 TestGoogleURLTrackerNavigationHelper::
@@ -139,15 +176,6 @@ void TestGoogleURLTrackerNavigationHelper::SetGoogleURLTracker(
   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) {
@@ -253,6 +281,7 @@ class GoogleURLTrackerTest : public testing::Test {
   net::TestURLFetcherFactory fetcher_factory_;
   content::NotificationRegistrar registrar_;
   TestNotificationObserver observer_;
+  GoogleURLTrackerClient* client_;
   GoogleURLTrackerNavigationHelper* nav_helper_;
   TestingProfile profile_;
   scoped_ptr<GoogleURLTracker> google_url_tracker_;
@@ -291,12 +320,14 @@ 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));
@@ -306,6 +337,7 @@ 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:

@@ -367,7 +399,7 @@ void GoogleURLTrackerTest::SetNavigationPending(intptr_t unique_id,
     // 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);