Remove ResourceRequestAllowedMigration flag

This flag was used in a canary finch experiment to make sure there was
not a regression in Variations.SeedFreshness. The experiment shows no
change, so enabling the new behavior:
https://uma.googleplex.com/variations?sid=5356f4f970495339c96d6af50868accf

Bug: 868021
Change-Id: I6f756be28a89226ef596c12cb09b032f2e672a75
Reviewed-on: https://chromium-review.googlesource.com/1222286
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592872}

chrome/browser/chrome_browser_main_browsertest.cc

@@ -9,7 +9,6 @@
 #include "base/command_line.h"
 #include "base/macros.h"
 #include "base/run_loop.h"
-#include "base/test/scoped_feature_list.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_browser_main_extra_parts.h"
 #include "chrome/test/base/in_process_browser_test.h"
@@ -19,8 +18,6 @@
 #include "content/public/common/service_manager_connection.h"
 #include "content/public/common/service_names.mojom.h"
 #include "content/public/test/browser_test_utils.h"
-#include "net/base/mock_network_change_notifier.h"
-#include "net/base/network_change_notifier_factory.h"
 #include "services/network/public/cpp/features.h"
 #include "services/network/public/mojom/network_service_test.mojom.h"
 #include "services/service_manager/public/cpp/connector.h"
@@ -62,67 +59,28 @@ void SimulateNetworkChange(network::mojom::ConnectionType type) {
       net::NetworkChangeNotifier::ConnectionType(type));
 }
 
-// ChromeBrowserMainExtraParts used to install a MockNetworkChangeNotifier.
+// ChromeBrowserMainExtraParts is used to initialize the network state.
 class ChromeBrowserMainExtraPartsNetFactoryInstaller
     : public ChromeBrowserMainExtraParts {
  public:
-  ChromeBrowserMainExtraPartsNetFactoryInstaller(bool migration_enabled)
-      : migration_enabled_(migration_enabled) {}
-  ~ChromeBrowserMainExtraPartsNetFactoryInstaller() override {
-    // |network_change_notifier_| needs to be destroyed before |net_installer_|.
-    network_change_notifier_.reset();
-  }
-
-  net::test::MockNetworkChangeNotifier* network_change_notifier() {
-    return network_change_notifier_.get();
-  }
+  ChromeBrowserMainExtraPartsNetFactoryInstaller() = default;
 
   // ChromeBrowserMainExtraParts:
   void PreEarlyInitialization() override {}
   void ServiceManagerConnectionStarted(
       content::ServiceManagerConnection* connection) override {
-    if (!migration_enabled_)
-      return;
     SimulateNetworkChange(network::mojom::ConnectionType::CONNECTION_NONE);
   }
-  void PostMainMessageLoopStart() override {
-    if (migration_enabled_)
-      return;
-    ASSERT_TRUE(net::NetworkChangeNotifier::HasNetworkChangeNotifier());
-    net_installer_ =
-        std::make_unique<net::NetworkChangeNotifier::DisableForTest>();
-    network_change_notifier_ =
-        std::make_unique<net::test::MockNetworkChangeNotifier>();
-    network_change_notifier_->SetConnectionType(
-        net::NetworkChangeNotifier::CONNECTION_NONE);
-  }
 
  private:
-  std::unique_ptr<net::test::MockNetworkChangeNotifier>
-      network_change_notifier_;
-  std::unique_ptr<net::NetworkChangeNotifier::DisableForTest> net_installer_;
-  bool migration_enabled_;
-
   DISALLOW_COPY_AND_ASSIGN(ChromeBrowserMainExtraPartsNetFactoryInstaller);
 };
 
-enum class MigrationState {
-  kEnabled,
-  kDisabled,
-};
-
 class ChromeBrowserMainBrowserTest
     : public InProcessBrowserTest,
-      public testing::WithParamInterface<MigrationState>,
-      public network::NetworkConnectionTracker::NetworkConnectionObserver {
+      network::NetworkConnectionTracker::NetworkConnectionObserver {
  public:
-  ChromeBrowserMainBrowserTest() {
-    if (GetParam() == MigrationState::kEnabled) {
-      feature_list_.InitAndEnableFeature(
-          web_resource::kResourceRequestAllowedMigration);
-    }
-  }
-
+  ChromeBrowserMainBrowserTest() = default;
   ~ChromeBrowserMainBrowserTest() override {
     content::GetNetworkConnectionTracker()->RemoveNetworkConnectionObserver(
         this);
@@ -144,8 +102,7 @@ class ChromeBrowserMainBrowserTest
         static_cast<ChromeBrowserMainParts*>(browser_main_parts);
     ChromeBrowserMainPartsTestApi(chrome_browser_main_parts)
         .EnableVariationsServiceInit();
-    extra_parts_ = new ChromeBrowserMainExtraPartsNetFactoryInstaller(
-        GetParam() == MigrationState::kEnabled);
+    extra_parts_ = new ChromeBrowserMainExtraPartsNetFactoryInstaller();
     chrome_browser_main_parts->AddParts(extra_parts_);
   }
 
@@ -172,7 +129,6 @@ class ChromeBrowserMainBrowserTest
       run_loop_->Quit();
   }
 
-  base::test::ScopedFeatureList feature_list_;
   network::mojom::ConnectionType expected_connection_type_ =
       network::mojom::ConnectionType::CONNECTION_UNKNOWN;
   network::mojom::ConnectionType connection_type_ =
@@ -184,20 +140,13 @@ class ChromeBrowserMainBrowserTest
 
 // Verifies VariationsService does a request when network status changes from
 // none to connected. This is a regression test for https://crbug.com/826930.
-IN_PROC_BROWSER_TEST_P(ChromeBrowserMainBrowserTest,
+IN_PROC_BROWSER_TEST_F(ChromeBrowserMainBrowserTest,
                        VariationsServiceStartsRequestOnNetworkChange) {
   const int initial_request_count =
       g_browser_process->variations_service()->request_count();
   ASSERT_TRUE(extra_parts_);
-  if (GetParam() == MigrationState::kEnabled) {
-    SimulateNetworkChange(network::mojom::ConnectionType::CONNECTION_WIFI);
-    WaitForConnectionType(network::mojom::ConnectionType::CONNECTION_WIFI);
-  } else {
-    extra_parts_->network_change_notifier()->SetConnectionType(
-        net::NetworkChangeNotifier::CONNECTION_WIFI);
-    net::NetworkChangeNotifier::NotifyObserversOfNetworkChangeForTests(
-        net::NetworkChangeNotifier::CONNECTION_WIFI);
-  }
+  SimulateNetworkChange(network::mojom::ConnectionType::CONNECTION_WIFI);
+  WaitForConnectionType(network::mojom::ConnectionType::CONNECTION_WIFI);
   // NotifyObserversOfNetworkChangeForTests uses PostTask, so run the loop until
   // idle to ensure VariationsService processes the network change.
   base::RunLoop().RunUntilIdle();
@@ -206,9 +155,4 @@ IN_PROC_BROWSER_TEST_P(ChromeBrowserMainBrowserTest,
   EXPECT_EQ(initial_request_count + 1, final_request_count);
 }
 
-INSTANTIATE_TEST_CASE_P(,
-                        ChromeBrowserMainBrowserTest,
-                        testing::Values(MigrationState::kEnabled,
-                                        MigrationState::kDisabled));
-
 }  // namespace

components/web_resource/resource_request_allowed_notifier.cc

@@ -8,11 +8,6 @@
 
 namespace web_resource {
 
-// This feature enables network change notifications from the
-// NetworkConnectionTracker instead of NetworkChangeNotifier.
-const base::Feature kResourceRequestAllowedMigration{
-    "ResourceRequestAllowedMigration", base::FEATURE_DISABLED_BY_DEFAULT};
-
 ResourceRequestAllowedNotifier::ResourceRequestAllowedNotifier(
     PrefService* local_state,
     const char* disable_network_switch,
@@ -27,12 +22,8 @@ ResourceRequestAllowedNotifier::ResourceRequestAllowedNotifier(
       weak_factory_(this) {}
 
 ResourceRequestAllowedNotifier::~ResourceRequestAllowedNotifier() {
-  if (observer_) {
-    if (base::FeatureList::IsEnabled(kResourceRequestAllowedMigration))
-      network_connection_tracker_->RemoveNetworkConnectionObserver(this);
-    else
-      net::NetworkChangeNotifier::RemoveNetworkChangeObserver(this);
-  }
+  if (observer_)
+    network_connection_tracker_->RemoveNetworkConnectionObserver(this);
 }
 
 void ResourceRequestAllowedNotifier::Init(Observer* observer, bool leaky) {
@@ -40,23 +31,18 @@ void ResourceRequestAllowedNotifier::Init(Observer* observer, bool leaky) {
   DCHECK(observer);
   observer_ = observer;
 
-  if (base::FeatureList::IsEnabled(kResourceRequestAllowedMigration)) {
-    DCHECK(network_connection_tracker_getter_);
-    network_connection_tracker_ =
-        std::move(network_connection_tracker_getter_).Run();
-
-    if (leaky)
-      network_connection_tracker_->AddLeakyNetworkConnectionObserver(this);
-    else
-      network_connection_tracker_->AddNetworkConnectionObserver(this);
-    if (network_connection_tracker_->GetConnectionType(
-            &connection_type_,
-            base::BindOnce(&ResourceRequestAllowedNotifier::SetConnectionType,
-                           weak_factory_.GetWeakPtr()))) {
-      connection_initialized_ = true;
-    }
-  } else {
-    net::NetworkChangeNotifier::AddNetworkChangeObserver(this);
+  DCHECK(network_connection_tracker_getter_);
+  network_connection_tracker_ =
+      std::move(network_connection_tracker_getter_).Run();
+
+  if (leaky)
+    network_connection_tracker_->AddLeakyNetworkConnectionObserver(this);
+  else
+    network_connection_tracker_->AddNetworkConnectionObserver(this);
+  if (network_connection_tracker_->GetConnectionType(
+          &connection_type_,
+          base::BindOnce(&ResourceRequestAllowedNotifier::SetConnectionType,
+                         weak_factory_.GetWeakPtr()))) {
     connection_initialized_ = true;
   }
 
@@ -90,11 +76,8 @@ ResourceRequestAllowedNotifier::GetResourceRequestsAllowedState() {
 }
 
 bool ResourceRequestAllowedNotifier::IsOffline() {
-  if (base::FeatureList::IsEnabled(kResourceRequestAllowedMigration)) {
-    return !connection_initialized_ ||
-           connection_type_ == network::mojom::ConnectionType::CONNECTION_NONE;
-  }
-  return net::NetworkChangeNotifier::IsOffline();
+  return !connection_initialized_ ||
+         connection_type_ == network::mojom::ConnectionType::CONNECTION_NONE;
 }
 
 bool ResourceRequestAllowedNotifier::ResourceRequestsAllowed() {
@@ -139,9 +122,10 @@ void ResourceRequestAllowedNotifier::OnEulaAccepted() {
   MaybeNotifyObserver();
 }
 
-void ResourceRequestAllowedNotifier::OnNetworkChanged(
-    net::NetworkChangeNotifier::ConnectionType type) {
-  if (type != net::NetworkChangeNotifier::CONNECTION_NONE) {
+void ResourceRequestAllowedNotifier::OnConnectionChanged(
+    network::mojom::ConnectionType type) {
+  SetConnectionType(type);
+  if (type != network::mojom::ConnectionType::CONNECTION_NONE) {
     DVLOG(1) << "Network came online.";
     // MaybeNotifyObserver() internally guarantees that it will only notify the
     // observer if it's currently waiting for the network to come online.
@@ -149,12 +133,6 @@ void ResourceRequestAllowedNotifier::OnNetworkChanged(
   }
 }
 
-void ResourceRequestAllowedNotifier::OnConnectionChanged(
-    network::mojom::ConnectionType type) {
-  SetConnectionType(type);
-  OnNetworkChanged(net::NetworkChangeNotifier::ConnectionType(type));
-}
-
 void ResourceRequestAllowedNotifier::SetConnectionType(
     network::mojom::ConnectionType connection_type) {
   connection_type_ = connection_type;

components/web_resource/resource_request_allowed_notifier.h

@@ -10,7 +10,6 @@
 #include "base/feature_list.h"
 #include "base/macros.h"
 #include "components/web_resource/eula_accepted_notifier.h"
-#include "net/base/network_change_notifier.h"
 #include "services/network/public/cpp/network_connection_tracker.h"
 
 class PrefService;
@@ -39,8 +38,7 @@ namespace web_resource {
 // global instance.
 class ResourceRequestAllowedNotifier
     : public EulaAcceptedNotifier::Observer,
-      public network::NetworkConnectionTracker::NetworkConnectionObserver,
-      public net::NetworkChangeNotifier::NetworkChangeObserver {
+      public network::NetworkConnectionTracker::NetworkConnectionObserver {
  public:
   // Observes resource request allowed state changes.
   class Observer {
@@ -108,10 +106,6 @@ class ResourceRequestAllowedNotifier
   // EulaAcceptedNotifier::Observer overrides:
   void OnEulaAccepted() override;
 
-  // net::NetworkChangeNotifier::NetworkChangeObserver overrides:
-  void OnNetworkChanged(
-      net::NetworkChangeNotifier::ConnectionType type) override;
-
   // network::NetworkConnectionTracker::NetworkConnectionObserver overrides:
   void OnConnectionChanged(network::mojom::ConnectionType type) override;
 

components/web_resource/resource_request_allowed_notifier_unittest.cc

@@ -6,7 +6,6 @@
 #include "base/memory/ptr_util.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
-#include "base/test/scoped_feature_list.h"
 #include "components/prefs/testing_pref_service.h"
 #include "components/web_resource/eula_accepted_notifier.h"
 #include "components/web_resource/resource_request_allowed_notifier_test_util.h"
@@ -15,34 +14,6 @@
 
 namespace web_resource {
 
-// Override NetworkChangeNotifier to simulate connection type changes for tests.
-class TestNetworkChangeNotifier : public net::NetworkChangeNotifier {
- public:
-  TestNetworkChangeNotifier()
-      : connection_type_(net::NetworkChangeNotifier::CONNECTION_UNKNOWN) {}
-
-  // Simulates a change of the connection type to |type|. This will notify any
-  // objects that are NetworkChangeNotifiers.
-  void SimulateNetworkConnectionChange(
-      net::NetworkChangeNotifier::ConnectionType type) {
-    connection_type_ = type;
-    net::NetworkChangeNotifier::NotifyObserversOfNetworkChangeForTests(
-        connection_type_);
-    base::RunLoop().RunUntilIdle();
-  }
-
- private:
-  ConnectionType GetCurrentConnectionType() const override {
-    return connection_type_;
-  }
-
-  // The currently simulated network connection type. If this is set to
-  // CONNECTION_NONE, then NetworkChangeNotifier::IsOffline will return true.
-  net::NetworkChangeNotifier::ConnectionType connection_type_;
-
-  DISALLOW_COPY_AND_ASSIGN(TestNetworkChangeNotifier);
-};
-
 // EulaAcceptedNotifier test class that allows mocking the EULA accepted state
 // and issuing simulated notifications.
 class TestEulaAcceptedNotifier : public EulaAcceptedNotifier {
@@ -74,23 +45,13 @@ enum class ConnectionTrackerResponseMode {
   kAsynchronous,
 };
 
-enum class MigrationState {
-  kEnabled,
-  kDisabled,
-};
-
-struct TestCase {
-  ConnectionTrackerResponseMode response_mode;
-  MigrationState migration_state;
-};
-
 // A test fixture class for ResourceRequestAllowedNotifier tests that require
 // network state simulations. This also acts as the service implementing the
 // ResourceRequestAllowedNotifier::Observer interface.
 class ResourceRequestAllowedNotifierTest
     : public testing::Test,
       public ResourceRequestAllowedNotifier::Observer,
-      public testing::WithParamInterface<TestCase> {
+      public testing::WithParamInterface<ConnectionTrackerResponseMode> {
  public:
   ResourceRequestAllowedNotifierTest()
       : resource_request_allowed_notifier_(
@@ -100,12 +61,9 @@ class ResourceRequestAllowedNotifierTest
         was_notified_(false) {
     auto* tracker = network::TestNetworkConnectionTracker::GetInstance();
     tracker->SetRespondSynchronously(
-        GetParam().response_mode ==
-        ConnectionTrackerResponseMode::kSynchronous);
+        GetParam() == ConnectionTrackerResponseMode::kSynchronous);
     tracker->SetConnectionType(network::mojom::ConnectionType::CONNECTION_WIFI);
 
-    if (GetParam().migration_state == MigrationState::kEnabled)
-      feature_list_.InitAndEnableFeature(kResourceRequestAllowedMigration);
     resource_request_allowed_notifier_.InitWithEulaAcceptNotifier(
         this, base::WrapUnique(eula_notifier_));
   }
@@ -117,14 +75,9 @@ class ResourceRequestAllowedNotifierTest
   void OnResourceRequestsAllowed() override { was_notified_ = true; }
 
   void SimulateNetworkConnectionChange(network::mojom::ConnectionType type) {
-    if (GetParam().migration_state == MigrationState::kEnabled) {
-      network::TestNetworkConnectionTracker::GetInstance()->SetConnectionType(
-          type);
-      base::RunLoop().RunUntilIdle();
-    } else {
-      network_notifier_.SimulateNetworkConnectionChange(
-          net::NetworkChangeNotifier::ConnectionType(type));
-    }
+    network::TestNetworkConnectionTracker::GetInstance()->SetConnectionType(
+        type);
+    base::RunLoop().RunUntilIdle();
   }
 
   // Simulate a resource request from the test service. It returns true if
@@ -166,10 +119,8 @@ class ResourceRequestAllowedNotifierTest
   }
 
  private:
-  base::test::ScopedFeatureList feature_list_;
   base::MessageLoopForUI message_loop_;
   TestRequestAllowedNotifier resource_request_allowed_notifier_;
-  TestNetworkChangeNotifier network_notifier_;
   TestingPrefServiceSimple prefs_;
   TestEulaAcceptedNotifier* eula_notifier_;  // Weak, owned by RRAN.
   bool was_notified_;
@@ -178,7 +129,7 @@ class ResourceRequestAllowedNotifierTest
 };
 
 TEST_P(ResourceRequestAllowedNotifierTest, NotifyOnInitialNetworkState) {
-  if (GetParam().response_mode == ConnectionTrackerResponseMode::kSynchronous) {
+  if (GetParam() == ConnectionTrackerResponseMode::kSynchronous) {
     EXPECT_TRUE(SimulateResourceRequest());
   } else {
     EXPECT_FALSE(SimulateResourceRequest());
@@ -347,11 +298,7 @@ TEST_P(ResourceRequestAllowedNotifierTest, NoRequestNoNotifyEula) {
 INSTANTIATE_TEST_CASE_P(
     ,
     ResourceRequestAllowedNotifierTest,
-    testing::Values(TestCase({ConnectionTrackerResponseMode::kSynchronous,
-                              MigrationState::kEnabled}),
-                    TestCase({ConnectionTrackerResponseMode::kAsynchronous,
-                              MigrationState::kEnabled}),
-                    TestCase({ConnectionTrackerResponseMode::kSynchronous,
-                              MigrationState::kDisabled})));
+    testing::Values(ConnectionTrackerResponseMode::kSynchronous,
+                    ConnectionTrackerResponseMode::kAsynchronous));
 
 }  // namespace web_resource