Migrate GoogleUrlTracker to NetworkConnectionTracker

This migrates GoogleUrlTracker to use NetworkConnectionTracker instead
of NetworkChangeNotifier so it will work with the network service
enabled.

Bug: 868018
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: Ib8231dc6df2456ae6a0908b49e4626364158ba81
Reviewed-on: https://chromium-review.googlesource.com/1175486
Commit-Queue: Robbie McElrath <rmcelrath@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Kristi Park <kristipark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584289}

chrome/browser/google/google_url_tracker_factory.cc

@@ -14,6 +14,7 @@
 #include "components/google/core/browser/google_url_tracker.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
 #include "components/prefs/pref_service.h"
+#include "content/public/browser/network_service_instance.h"
 
 // static
 GoogleURLTracker* GoogleURLTrackerFactory::GetForProfile(Profile* profile) {
@@ -41,7 +42,8 @@ std::unique_ptr<KeyedService> BuildGoogleURLTracker(
       std::move(client),
       base::FeatureList::IsEnabled(GoogleURLTracker::kNoSearchDomainCheck)
           ? GoogleURLTracker::ALWAYS_DOT_COM_MODE
-          : GoogleURLTracker::NORMAL_MODE);
+          : GoogleURLTracker::NORMAL_MODE,
+      content::GetNetworkConnectionTracker());
 }
 
 }  // namespace

chrome/browser/search/one_google_bar/one_google_bar_loader_impl_unittest.cc

@@ -23,6 +23,7 @@
 #include "services/data_decoder/public/cpp/testing_json_parser.h"
 #include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h"
 #include "services/network/public/mojom/url_loader_factory.mojom.h"
+#include "services/network/test/test_network_connection_tracker.h"
 #include "services/network/test/test_url_loader_factory.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -70,11 +71,14 @@ class OneGoogleBarLoaderImplTest : public testing::Test {
       : OneGoogleBarLoaderImplTest(
             /*account_consistency_mirror_required=*/false) {}
 
-  explicit OneGoogleBarLoaderImplTest(
-      bool account_consistency_mirror_required)
+  explicit OneGoogleBarLoaderImplTest(bool account_consistency_mirror_required)
       : thread_bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP),
+        test_network_connection_tracker_(
+            true /* respond_synchronously */,
+            network::mojom::ConnectionType::CONNECTION_UNKNOWN),
         google_url_tracker_(std::make_unique<GoogleURLTrackerClientStub>(),
-                            GoogleURLTracker::ALWAYS_DOT_COM_MODE),
+                            GoogleURLTracker::ALWAYS_DOT_COM_MODE,
+                            &test_network_connection_tracker_),
         test_shared_loader_factory_(
             base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
                 &test_url_loader_factory_)),
@@ -127,6 +131,7 @@ class OneGoogleBarLoaderImplTest : public testing::Test {
 
   data_decoder::TestingJsonParser::ScopedFactoryOverride factory_override_;
 
+  network::TestNetworkConnectionTracker test_network_connection_tracker_;
   GoogleURLTracker google_url_tracker_;
   network::TestURLLoaderFactory test_url_loader_factory_;
   scoped_refptr<network::SharedURLLoaderFactory> test_shared_loader_factory_;

components/google/core/browser/google_url_tracker.cc

@@ -33,8 +33,10 @@ const base::Feature GoogleURLTracker::kNoSearchDomainCheck{
 
 GoogleURLTracker::GoogleURLTracker(
     std::unique_ptr<GoogleURLTrackerClient> client,
-    Mode mode)
+    Mode mode,
+    network::NetworkConnectionTracker* network_connection_tracker)
     : client_(std::move(client)),
+      network_connection_tracker_(network_connection_tracker),
       google_url_(
           mode == ALWAYS_DOT_COM_MODE
               ? kDefaultGoogleHomepage
@@ -43,7 +45,7 @@ GoogleURLTracker::GoogleURLTracker(
       already_loaded_(false),
       need_to_load_(false),
       weak_ptr_factory_(this) {
-  net::NetworkChangeNotifier::AddNetworkChangeObserver(this);
+  network_connection_tracker_->AddNetworkConnectionObserver(this);
   client_->set_google_url_tracker(this);
 
   // Because this function can be called during startup, when kicking off a URL
@@ -119,10 +121,10 @@ void GoogleURLTracker::OnURLLoaderComplete(
   }
 }
 
-void GoogleURLTracker::OnNetworkChanged(
-    net::NetworkChangeNotifier::ConnectionType type) {
+void GoogleURLTracker::OnConnectionChanged(
+    network::mojom::ConnectionType type) {
   // Ignore destructive signals.
-  if (type == net::NetworkChangeNotifier::CONNECTION_NONE)
+  if (type == network::mojom::ConnectionType::CONNECTION_NONE)
     return;
   already_loaded_ = false;
   StartLoadIfDesirable();
@@ -132,7 +134,7 @@ void GoogleURLTracker::Shutdown() {
   client_.reset();
   simple_loader_.reset();
   weak_ptr_factory_.InvalidateWeakPtrs();
-  net::NetworkChangeNotifier::RemoveNetworkChangeObserver(this);
+  network_connection_tracker_->RemoveNetworkConnectionObserver(this);
 }
 
 void GoogleURLTracker::SetNeedToLoad() {

components/google/core/browser/google_url_tracker.h

@@ -14,7 +14,7 @@
 #include "base/memory/weak_ptr.h"
 #include "components/google/core/browser/google_url_tracker_client.h"
 #include "components/keyed_service/core/keyed_service.h"
-#include "net/base/network_change_notifier.h"
+#include "services/network/public/cpp/network_connection_tracker.h"
 #include "url/gurl.h"
 
 namespace user_prefs {
@@ -35,7 +35,7 @@ class SimpleURLLoader;
 // performed (ever) unless at least one consumer registers interest by calling
 // RequestServerCheck().
 class GoogleURLTracker
-    : public net::NetworkChangeNotifier::NetworkChangeObserver,
+    : public network::NetworkConnectionTracker::NetworkConnectionObserver,
       public KeyedService {
  public:
   // Callback that is called when the Google URL is updated.
@@ -64,10 +64,15 @@ class GoogleURLTracker
   static const base::Feature kNoSearchDomainCheck;
 
   // Only the GoogleURLTrackerFactory and tests should call this.
+  // |network_connection_tracker| should be the global singleton instance and
+  // should outlive the GoogleURLTracker object.
   // Note: you *must* manually call Shutdown() before this instance gets
   // destroyed if you want to create another instance in the same binary
   // (e.g. in unit tests).
-  GoogleURLTracker(std::unique_ptr<GoogleURLTrackerClient> client, Mode mode);
+  GoogleURLTracker(
+      std::unique_ptr<GoogleURLTrackerClient> client,
+      Mode mode,
+      network::NetworkConnectionTracker* network_connection_tracker);
 
   ~GoogleURLTracker() override;
 
@@ -95,9 +100,8 @@ class GoogleURLTracker
 
   void OnURLLoaderComplete(std::unique_ptr<std::string> response_body);
 
-  // NetworkChangeNotifier::NetworkChangeObserver:
-  void OnNetworkChanged(
-      net::NetworkChangeNotifier::ConnectionType type) override;
+  // NetworkConnectionTracker::NetworkConnectionObserver:
+  void OnConnectionChanged(network::mojom::ConnectionType type) override;
 
   // KeyedService:
   void Shutdown() override;
@@ -116,6 +120,7 @@ class GoogleURLTracker
   CallbackList callback_list_;
 
   std::unique_ptr<GoogleURLTrackerClient> client_;
+  network::NetworkConnectionTracker* network_connection_tracker_;
 
   GURL google_url_;
   std::unique_ptr<network::SimpleURLLoader> simple_loader_;

components/google/core/browser/google_url_tracker_unittest.cc

@@ -21,6 +21,7 @@
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h"
 #include "services/network/public/mojom/url_loader_factory.mojom.h"
+#include "services/network/test/test_network_connection_tracker.h"
 #include "services/network/test/test_url_loader_factory.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -145,9 +146,8 @@ class GoogleURLTrackerTest : public testing::Test {
   TestingPrefServiceSimple prefs_;
   network::TestURLLoaderFactory test_url_loader_factory_;
 
-  // Creating this allows us to call
-  // net::NetworkChangeNotifier::NotifyObserversOfNetworkChangeForTests().
-  std::unique_ptr<net::NetworkChangeNotifier> network_change_notifier_;
+  std::unique_ptr<network::TestNetworkConnectionTracker>
+      test_network_connection_tracker_;
   GoogleURLTrackerClient* client_;
 
   std::unique_ptr<GoogleURLTracker> google_url_tracker_;
@@ -156,7 +156,11 @@ class GoogleURLTrackerTest : public testing::Test {
   bool handled_request_ = false;
 };
 
-GoogleURLTrackerTest::GoogleURLTrackerTest() {
+GoogleURLTrackerTest::GoogleURLTrackerTest()
+    : test_network_connection_tracker_(
+          new network::TestNetworkConnectionTracker(
+              true /* respond_synchronously */,
+              network::mojom::ConnectionType::CONNECTION_UNKNOWN)) {
   prefs_.registry()->RegisterStringPref(
       prefs::kLastKnownGoogleURL,
       GoogleURLTracker::kDefaultGoogleHomepage);
@@ -166,13 +170,13 @@ 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 client for its lifetime.
   client_ = new TestGoogleURLTrackerClient(&prefs_, &test_url_loader_factory_);
   std::unique_ptr<GoogleURLTrackerClient> client(client_);
   google_url_tracker_.reset(new GoogleURLTracker(
-      std::move(client), GoogleURLTracker::ALWAYS_DOT_COM_MODE));
+      std::move(client), GoogleURLTracker::ALWAYS_DOT_COM_MODE,
+      test_network_connection_tracker_.get()));
 }
 
 void GoogleURLTrackerTest::TearDown() {
@@ -203,10 +207,8 @@ void GoogleURLTrackerTest::FinishSleep() {
 }
 
 void GoogleURLTrackerTest::NotifyNetworkChanged() {
-  net::NetworkChangeNotifier::NotifyObserversOfNetworkChangeForTests(
-      net::NetworkChangeNotifier::CONNECTION_UNKNOWN);
-  // For thread safety, the NCN queues tasks to do the actual notifications, so
-  // we need to spin the message loop so the tracker will actually be notified.
+  test_network_connection_tracker_->SetConnectionType(
+      network::mojom::ConnectionType::CONNECTION_UNKNOWN);
   base::RunLoop().RunUntilIdle();
 }
 

ios/chrome/browser/google/google_url_tracker_factory.cc

@@ -49,7 +49,7 @@ std::unique_ptr<KeyedService> GoogleURLTrackerFactory::BuildServiceInstanceFor(
 
   return std::make_unique<GoogleURLTracker>(
       base::WrapUnique(new GoogleURLTrackerClientImpl(browser_state)),
-      GoogleURLTracker::NORMAL_MODE);
+      GoogleURLTracker::NORMAL_MODE, context->GetNetworkConnectionTracker());
 }
 
 web::BrowserState* GoogleURLTrackerFactory::GetBrowserStateToUse(

ios/web/DEPS

@@ -5,6 +5,7 @@ include_rules = [
   "+ios/web",
   "+mojo/public",
   "+net",
+  "+services/network/network_change_manager.h",
   "+services/network/network_context.h",
   "+services/network/public/cpp",
   "+services/network/public/mojom",

ios/web/browser_state.mm

@@ -23,7 +23,9 @@
 #include "mojo/public/cpp/bindings/interface_request.h"
 #include "net/url_request/url_request_context_getter.h"
 #include "net/url_request/url_request_context_getter_observer.h"
+#include "services/network/network_change_manager.h"
 #include "services/network/network_context.h"
+#include "services/network/public/cpp/network_connection_tracker.h"
 #include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h"
 #include "services/service_manager/public/cpp/connector.h"
 #include "services/service_manager/public/mojom/service.mojom.h"
@@ -107,6 +109,13 @@ class BrowserStateServiceManagerConnectionHolder
   DISALLOW_COPY_AND_ASSIGN(BrowserStateServiceManagerConnectionHolder);
 };
 
+// Passed to NetworkConnectionTracker to bind a NetworkChangeManagerRequest.
+void BindNetworkChangeManagerRequest(
+    network::NetworkChangeManager* network_change_manager,
+    network::mojom::NetworkChangeManagerRequest request) {
+  network_change_manager->AddRequest(std::move(request));
+}
+
 }  // namespace
 
 // static
@@ -189,6 +198,19 @@ network::mojom::CookieManager* BrowserState::GetCookieManager() {
   return cookie_manager_.get();
 }
 
+network::NetworkConnectionTracker* BrowserState::GetNetworkConnectionTracker() {
+  if (!network_connection_tracker_) {
+    DCHECK(!network_change_manager_);
+    network_change_manager_ =
+        std::make_unique<network::NetworkChangeManager>(nullptr);
+    network_connection_tracker_ =
+        std::make_unique<network::NetworkConnectionTracker>(base::BindRepeating(
+            &BindNetworkChangeManagerRequest,
+            base::Unretained(network_change_manager_.get())));
+  }
+  return network_connection_tracker_.get();
+}
+
 void BrowserState::GetProxyResolvingSocketFactory(
     network::mojom::ProxyResolvingSocketFactoryRequest request) {
   CreateNetworkContextIfNecessary();

ios/web/public/browser_state.h

@@ -23,6 +23,8 @@ class URLRequestContextGetter;
 }
 
 namespace network {
+class NetworkChangeManager;
+class NetworkConnectionTracker;
 class SharedURLLoaderFactory;
 class WeakWrapperSharedURLLoaderFactory;
 }  // namespace network
@@ -68,6 +70,9 @@ class BrowserState : public base::SupportsUserData {
   // Returns a CookieManager that is backed by GetRequestContext.
   network::mojom::CookieManager* GetCookieManager();
 
+  // Returns the NetworkConnectionTracker instance for this BrowserState.
+  network::NetworkConnectionTracker* GetNetworkConnectionTracker();
+
   // Binds a ProxyResolvingSocketFactory request to NetworkContext.
   void GetProxyResolvingSocketFactory(
       network::mojom::ProxyResolvingSocketFactoryRequest request);
@@ -129,6 +134,12 @@ class BrowserState : public base::SupportsUserData {
       shared_url_loader_factory_;
   network::mojom::NetworkContextPtr network_context_;
 
+  // Acts as a proxy between the NetworkChangeNotifier and
+  // NetworkConnectionTracker.
+  std::unique_ptr<network::NetworkChangeManager> network_change_manager_;
+  std::unique_ptr<network::NetworkConnectionTracker>
+      network_connection_tracker_;
+
   // Owns the network::NetworkContext that backs |url_loader_factory_|. Created
   // on the UI thread, destroyed on the IO thread.
   std::unique_ptr<NetworkContextOwner> network_context_owner_;