Make ChromeSigninClient use content::NetworkConnectionTracker

This CL makes ChromeSigninClient use content::NetworkConnectionTracker
instead of net::NetworkChangeNotifier.

This CL additionally modifies NetworkConnectionTracker's
constructor so we can mock it in TestingBrowserProcess and tests.

Bug: 754709
Change-Id: Ic05662830839403d5b327793f8faf7284a0da0bd
Reviewed-on: https://chromium-review.googlesource.com/757583Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Helen Li <xunjieli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515965}

chrome/browser/browser_process_impl.cc

@@ -576,8 +576,9 @@ BrowserProcessImpl::network_connection_tracker() {
   DCHECK(io_thread_);
   if (!network_connection_tracker_) {
     network_connection_tracker_ =
-        std::make_unique<content::NetworkConnectionTracker>(
-            io_thread_->GetNetworkServiceOnUIThread());
+        std::make_unique<content::NetworkConnectionTracker>();
+    network_connection_tracker_->Initialize(
+        io_thread_->GetNetworkServiceOnUIThread());
   }
   return network_connection_tracker_.get();
 }

chrome/browser/signin/chrome_signin_client.cc

@@ -68,10 +68,12 @@ ChromeSigninClient::ChromeSigninClient(
     SigninErrorController* signin_error_controller)
     : OAuth2TokenService::Consumer("chrome_signin_client"),
       profile_(profile),
-      signin_error_controller_(signin_error_controller) {
+      signin_error_controller_(signin_error_controller),
+      weak_ptr_factory_(this) {
   signin_error_controller_->AddObserver(this);
 #if !defined(OS_CHROMEOS)
-  net::NetworkChangeNotifier::AddNetworkChangeObserver(this);
+  g_browser_process->network_connection_tracker()->AddNetworkConnectionObserver(
+      this);
 #else
   // UserManager may not exist in unit_tests.
   if (!user_manager::UserManager::IsInitialized())
@@ -103,11 +105,9 @@ ChromeSigninClient::ChromeSigninClient(
 
 ChromeSigninClient::~ChromeSigninClient() {
   signin_error_controller_->RemoveObserver(this);
-}
-
-void ChromeSigninClient::Shutdown() {
 #if !defined(OS_CHROMEOS)
-  net::NetworkChangeNotifier::RemoveNetworkChangeObserver(this);
+  g_browser_process->network_connection_tracker()
+      ->RemoveNetworkConnectionObserver(this);
 #endif
 }
 
@@ -378,9 +378,9 @@ void ChromeSigninClient::OnGetTokenFailure(
 }
 
 #if !defined(OS_CHROMEOS)
-void ChromeSigninClient::OnNetworkChanged(
-    net::NetworkChangeNotifier::ConnectionType type) {
-  if (type >= net::NetworkChangeNotifier::ConnectionType::CONNECTION_NONE)
+void ChromeSigninClient::OnConnectionChanged(
+    content::mojom::ConnectionType type) {
+  if (type == content::mojom::ConnectionType::CONNECTION_NONE)
     return;
 
   for (const base::Closure& callback : delayed_callbacks_)
@@ -398,7 +398,13 @@ void ChromeSigninClient::DelayNetworkCall(const base::Closure& callback) {
   return;
 #else
   // Don't bother if we don't have any kind of network connection.
-  if (net::NetworkChangeNotifier::IsOffline()) {
+  content::mojom::ConnectionType type;
+  bool sync =
+      g_browser_process->network_connection_tracker()->GetConnectionType(
+          &type, base::BindOnce(&ChromeSigninClient::OnConnectionChanged,
+                                weak_ptr_factory_.GetWeakPtr()));
+  if (!sync || type == content::mojom::ConnectionType::CONNECTION_NONE) {
+    // Connection type cannot be retrieved synchronously so delay the callback.
     delayed_callbacks_.push_back(callback);
   } else {
     callback.Run();

chrome/browser/signin/chrome_signin_client.h

@@ -7,14 +7,16 @@
 
 #include "base/compiler_specific.h"
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "build/build_config.h"
 #include "components/signin/core/browser/signin_client.h"
 #include "components/signin/core/browser/signin_error_controller.h"
+#include "content/public/common/network_change_manager.mojom.h"
 #include "google_apis/gaia/gaia_oauth_client.h"
 #include "google_apis/gaia/oauth2_token_service.h"
 
 #if !defined(OS_CHROMEOS)
-#include "net/base/network_change_notifier.h"
+#include "content/public/common/network_connection_tracker.h"
 #endif
 
 #if !defined(OS_ANDROID) && !defined(OS_CHROMEOS)
@@ -25,7 +27,7 @@ class Profile;
 class ChromeSigninClient
     : public SigninClient,
 #if !defined(OS_CHROMEOS)
-      public net::NetworkChangeNotifier::NetworkChangeObserver,
+      public content::NetworkConnectionTracker::NetworkConnectionObserver,
 #endif
       public SigninErrorController::Observer,
       public gaia::GaiaOAuthClient::Delegate,
@@ -34,7 +36,6 @@ class ChromeSigninClient
   explicit ChromeSigninClient(
       Profile* profile, SigninErrorController* signin_error_controller);
   ~ChromeSigninClient() override;
-  void Shutdown() override;
   void DoFinalInit() override;
 
   // Utility method.
@@ -97,9 +98,9 @@ class ChromeSigninClient
                          const GoogleServiceAuthError& error) override;
 
 #if !defined(OS_CHROMEOS)
-  // net::NetworkChangeController::NetworkChangeObserver implementation.
-  void OnNetworkChanged(net::NetworkChangeNotifier::ConnectionType type)
-      override;
+  // content::NetworkConnectionTracker::NetworkConnectionObserver
+  // implementation.
+  void OnConnectionChanged(content::mojom::ConnectionType type) override;
 #endif
 
   void AfterCredentialsCopied() override;
@@ -132,6 +133,8 @@ class ChromeSigninClient
   std::unique_ptr<gaia::GaiaOAuthClient> oauth_client_;
   std::unique_ptr<OAuth2TokenService::Request> oauth_request_;
 
+  base::WeakPtrFactory<ChromeSigninClient> weak_ptr_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(ChromeSigninClient);
 };
 

chrome/browser/signin/chrome_signin_client_unittest.cc

@@ -18,8 +18,8 @@
 #include "chrome/test/base/browser_with_test_window_test.h"
 #include "chrome/test/base/testing_browser_process.h"
 #include "chrome/test/base/testing_profile.h"
+#include "content/public/common/network_connection_tracker.h"
 #include "content/public/test/test_browser_thread_bundle.h"
-#include "net/base/network_change_notifier.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -28,31 +28,52 @@
 
 namespace {
 
-class MockNetworkChangeNotifierNeverOffline :
-    public net::NetworkChangeNotifier {
+class MockNetworkConnectionTrackerNeverOffline
+    : public content::NetworkConnectionTracker {
  public:
-  net::NetworkChangeNotifier::ConnectionType GetCurrentConnectionType() const
-      override {
-    return NetworkChangeNotifier::CONNECTION_3G;
+  MockNetworkConnectionTrackerNeverOffline()
+      : content::NetworkConnectionTracker() {}
+  ~MockNetworkConnectionTrackerNeverOffline() override {}
+
+  bool GetConnectionType(content::mojom::ConnectionType* type,
+                         ConnectionTypeCallback callback) override {
+    *type = content::mojom::ConnectionType::CONNECTION_3G;
+    return true;
   }
 };
 
-class MockNetworkChangeNotifierOfflineUntilChange :
-    public net::NetworkChangeNotifier {
+class MockNetworkConnectionTrackerGetConnectionTypeAsync
+    : public content::NetworkConnectionTracker {
  public:
-  MockNetworkChangeNotifierOfflineUntilChange() : online_(false) {}
-  net::NetworkChangeNotifier::ConnectionType GetCurrentConnectionType() const
-      override {
-    return online_ ? net::NetworkChangeNotifier::CONNECTION_3G
-                   : net::NetworkChangeNotifier::CONNECTION_NONE;
+  MockNetworkConnectionTrackerGetConnectionTypeAsync()
+      : content::NetworkConnectionTracker() {}
+  ~MockNetworkConnectionTrackerGetConnectionTypeAsync() override {}
+
+  void CompleteCallback() {
+    OnInitialConnectionType(content::mojom::ConnectionType::CONNECTION_3G);
   }
+};
 
+class MockNetworkConnectionTrackerOfflineUntilChange
+    : public content::NetworkConnectionTracker {
+ public:
+  MockNetworkConnectionTrackerOfflineUntilChange()
+      : content::NetworkConnectionTracker(), online_(false) {}
+  ~MockNetworkConnectionTrackerOfflineUntilChange() override {}
+
+  bool GetConnectionType(content::mojom::ConnectionType* type,
+                         ConnectionTypeCallback callback) override {
+    if (online_) {
+      *type = content::mojom::ConnectionType::CONNECTION_3G;
+    } else {
+      *type = content::mojom::ConnectionType::CONNECTION_NONE;
+    }
+    return true;
+  }
   void GoOnline() {
     online_ = true;
-    net::NetworkChangeNotifier::NotifyObserversOfNetworkChangeForTests(
-        net::NetworkChangeNotifier::CONNECTION_3G);
+    OnNetworkChanged(content::mojom::ConnectionType::CONNECTION_3G);
   }
-
  private:
   bool online_;
 };
@@ -81,7 +102,16 @@ bool CallbackTester::WasCalledExactlyOnce() {
 class ChromeSigninClientTest : public testing::Test {
  public:
   ChromeSigninClientTest() {}
-  void SetUp() override;
+
+  void Initialize(std::unique_ptr<content::NetworkConnectionTracker> tracker) {
+    TestingBrowserProcess::GetGlobal()->SetNetworkConnectionTracker(
+        std::move(tracker));
+    // Create a signed-in profile.
+    TestingProfile::Builder builder;
+    profile_ = builder.Build();
+
+    signin_client_ = ChromeSigninClientFactory::GetForProfile(profile());
+  }
 
   Profile* profile() { return profile_.get(); }
   SigninClient* signin_client() { return signin_client_; }
@@ -92,28 +122,34 @@ class ChromeSigninClientTest : public testing::Test {
   SigninClient* signin_client_;
 };
 
-void ChromeSigninClientTest::SetUp() {
-  // Create a signed-in profile.
-  TestingProfile::Builder builder;
-  profile_ = builder.Build();
-
-  signin_client_ = ChromeSigninClientFactory::GetForProfile(profile());
+TEST_F(ChromeSigninClientTest, DelayNetworkCallRunsImmediatelyWithNetwork) {
+  Initialize(std::make_unique<MockNetworkConnectionTrackerNeverOffline>());
+  CallbackTester tester;
+  signin_client()->DelayNetworkCall(
+      base::Bind(&CallbackTester::Increment, base::Unretained(&tester)));
+  ASSERT_TRUE(tester.WasCalledExactlyOnce());
 }
 
-TEST_F(ChromeSigninClientTest, DelayNetworkCallRunsImmediatelyWithNetwork) {
-  std::unique_ptr<net::NetworkChangeNotifier> mock(
-      new MockNetworkChangeNotifierNeverOffline);
+TEST_F(ChromeSigninClientTest, DelayNetworkCallRunsAfterGetConnectionType) {
+  auto tracker =
+      std::make_unique<MockNetworkConnectionTrackerGetConnectionTypeAsync>();
+  MockNetworkConnectionTrackerGetConnectionTypeAsync* mock = tracker.get();
+  Initialize(std::move(tracker));
+
   CallbackTester tester;
   signin_client()->DelayNetworkCall(base::Bind(&CallbackTester::Increment,
                                                base::Unretained(&tester)));
+  ASSERT_FALSE(tester.WasCalledExactlyOnce());
+  mock->CompleteCallback();
+  base::RunLoop().RunUntilIdle();
   ASSERT_TRUE(tester.WasCalledExactlyOnce());
 }
 
-TEST_F(ChromeSigninClientTest, DelayNetworkCallRunsAfterNetworkFound) {
-  std::unique_ptr<MockNetworkChangeNotifierOfflineUntilChange> mock(
-      new MockNetworkChangeNotifierOfflineUntilChange());
-  // Install a SigninClient after the NetworkChangeNotifier's created.
-  SetUp();
+TEST_F(ChromeSigninClientTest, DelayNetworkCallRunsAfterNetworkChange) {
+  auto tracker =
+      std::make_unique<MockNetworkConnectionTrackerOfflineUntilChange>();
+  MockNetworkConnectionTrackerOfflineUntilChange* mock = tracker.get();
+  Initialize(std::move(tracker));
 
   CallbackTester tester;
   signin_client()->DelayNetworkCall(base::Bind(&CallbackTester::Increment,

chrome/test/base/testing_browser_process.cc

@@ -27,6 +27,7 @@
 #include "components/prefs/pref_service.h"
 #include "components/subresource_filter/content/browser/content_ruleset_service.h"
 #include "content/public/browser/notification_service.h"
+#include "content/public/common/network_connection_tracker.h"
 #include "extensions/features/features.h"
 #include "media/media_features.h"
 #include "net/url_request/url_request_context_getter.h"
@@ -55,6 +56,25 @@
 #include "components/keep_alive_registry/keep_alive_registry.h"
 #endif
 
+namespace {
+
+class MockNetworkConnectionTracker : public content::NetworkConnectionTracker {
+ public:
+  MockNetworkConnectionTracker() : content::NetworkConnectionTracker() {}
+  ~MockNetworkConnectionTracker() override {}
+
+  bool GetConnectionType(content::mojom::ConnectionType* type,
+                         ConnectionTypeCallback callback) override {
+    *type = content::mojom::ConnectionType::CONNECTION_UNKNOWN;
+    return true;
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(MockNetworkConnectionTracker);
+};
+
+}  // namespace
+
 // static
 TestingBrowserProcess* TestingBrowserProcess::GetGlobal() {
   return static_cast<TestingBrowserProcess*>(g_browser_process);
@@ -145,7 +165,11 @@ TestingBrowserProcess::system_network_context_manager() {
 
 content::NetworkConnectionTracker*
 TestingBrowserProcess::network_connection_tracker() {
-  return nullptr;
+  if (!network_connection_tracker_) {
+    network_connection_tracker_ =
+        std::make_unique<MockNetworkConnectionTracker>();
+  }
+  return network_connection_tracker_.get();
 }
 
 WatchDogThread* TestingBrowserProcess::watchdog_thread() {
@@ -435,6 +459,11 @@ void TestingBrowserProcess::SetSystemRequestContext(
   system_request_context_ = context_getter;
 }
 
+void TestingBrowserProcess::SetNetworkConnectionTracker(
+    std::unique_ptr<content::NetworkConnectionTracker> tracker) {
+  network_connection_tracker_ = std::move(tracker);
+}
+
 void TestingBrowserProcess::SetNotificationUIManager(
     std::unique_ptr<NotificationUIManager> notification_ui_manager) {
   notification_ui_manager_.swap(notification_ui_manager);

chrome/test/base/testing_browser_process.h

@@ -147,6 +147,8 @@ class TestingBrowserProcess : public BrowserProcess {
       std::unique_ptr<optimization_guide::OptimizationGuideService>
           optimization_guide_service);
   void SetSystemRequestContext(net::URLRequestContextGetter* context_getter);
+  void SetNetworkConnectionTracker(
+      std::unique_ptr<content::NetworkConnectionTracker> tracker);
   void SetNotificationUIManager(
       std::unique_ptr<NotificationUIManager> notification_ui_manager);
   void SetNotificationPlatformBridge(
@@ -166,6 +168,8 @@ class TestingBrowserProcess : public BrowserProcess {
 
   std::unique_ptr<policy::BrowserPolicyConnector> browser_policy_connector_;
   bool created_browser_policy_connector_ = false;
+  std::unique_ptr<content::NetworkConnectionTracker>
+      network_connection_tracker_;
   std::unique_ptr<ProfileManager> profile_manager_;
   std::unique_ptr<NotificationUIManager> notification_ui_manager_;
   std::unique_ptr<NotificationPlatformBridge> notification_platform_bridge_;

content/public/common/network_connection_tracker.cc

@@ -32,15 +32,18 @@ static const int32_t kConnectionTypeInvalid = -1;
 
 }  // namespace
 
-NetworkConnectionTracker::NetworkConnectionTracker(
-    mojom::NetworkService* network_service)
+NetworkConnectionTracker::NetworkConnectionTracker()
     : task_runner_(base::ThreadTaskRunnerHandle::Get()),
       connection_type_(kConnectionTypeInvalid),
       network_change_observer_list_(
           new base::ObserverListThreadSafe<NetworkConnectionObserver>(
               base::ObserverListBase<
                   NetworkConnectionObserver>::NOTIFY_EXISTING_ONLY)),
-      binding_(this) {
+      binding_(this) {}
+
+void NetworkConnectionTracker::Initialize(
+    mojom::NetworkService* network_service) {
+  DCHECK(!binding_.is_bound());
   DCHECK(network_service);
   // Get NetworkChangeManagerPtr.
   mojom::NetworkChangeManagerPtr manager_ptr;

content/public/common/network_connection_tracker.h

@@ -45,17 +45,25 @@ class CONTENT_EXPORT NetworkConnectionTracker
     DISALLOW_COPY_AND_ASSIGN(NetworkConnectionObserver);
   };
 
-  explicit NetworkConnectionTracker(mojom::NetworkService* network_service);
+  NetworkConnectionTracker();
 
   ~NetworkConnectionTracker() override;
 
+  // Starts listening for connection notifications from |network_service|.
+  // Observers may be added and GetConnectionType called, but no network
+  // information will be provided until this method is called. For unit tests,
+  // this class can be subclassed, and OnInitialConnectionType /
+  // OnNetworkChanged may be called directly, instead of providing a
+  // NetworkService.
+  void Initialize(mojom::NetworkService* network_service);
+
   // If connection type can be retrieved synchronously, returns true and |type|
   // will contain the current connection type; Otherwise, returns false, in
   // which case, |callback| will be called on the calling thread when connection
   // type is ready. This method is thread safe. Please also refer to
   // net::NetworkChangeNotifier::GetConnectionType() for documentation.
-  bool GetConnectionType(mojom::ConnectionType* type,
-                         ConnectionTypeCallback callback);
+  virtual bool GetConnectionType(mojom::ConnectionType* type,
+                                 ConnectionTypeCallback callback);
 
   // Returns true if |type| is a cellular connection.
   // Returns false if |type| is CONNECTION_UNKNOWN, and thus, depending on the
@@ -74,13 +82,14 @@ class CONTENT_EXPORT NetworkConnectionTracker
   // All observers must be unregistred before |this| is destroyed.
   void RemoveNetworkConnectionObserver(NetworkConnectionObserver* observer);
 
- private:
-  FRIEND_TEST_ALL_PREFIXES(NetworkGetConnectionTest,
-                           GetConnectionTypeOnDifferentThread);
-  // NetworkChangeManagerClient implementation:
+ protected:
+  // NetworkChangeManagerClient implementation. Protected for testing.
   void OnInitialConnectionType(mojom::ConnectionType type) override;
   void OnNetworkChanged(mojom::ConnectionType type) override;
 
+ private:
+  FRIEND_TEST_ALL_PREFIXES(NetworkGetConnectionTest,
+                           GetConnectionTypeOnDifferentThread);
   // The task runner that |this| lives on.
   scoped_refptr<base::SequencedTaskRunner> task_runner_;
 

content/public/common/network_connection_tracker_unittest.cc

@@ -129,8 +129,8 @@ class NetworkConnectionTrackerTest : public testing::Test {
     network_service_ =
         NetworkService::Create(std::move(network_service_request),
                                /*netlog=*/nullptr);
-    tracker_ =
-        std::make_unique<NetworkConnectionTracker>(network_service_.get());
+    tracker_ = std::make_unique<NetworkConnectionTracker>();
+    tracker_->Initialize(network_service_.get());
     observer_ = std::make_unique<TestNetworkConnectionObserver>(tracker_.get());
   }
 
@@ -219,7 +219,8 @@ TEST_F(NetworkConnectionTrackerTest, GetConnectionType) {
       mojo::MakeRequest(&network_service_ptr);
   std::unique_ptr<NetworkService> network_service =
       NetworkService::Create(std::move(network_service_request), nullptr);
-  NetworkConnectionTracker tracker(network_service_ptr.get());
+  NetworkConnectionTracker tracker;
+  tracker.Initialize(network_service_ptr.get());
 
   ConnectionTypeGetter getter1(&tracker), getter2(&tracker);
   // These two GetConnectionType() will finish asynchonously because network