Remove unused methods from NetworkConfigurationObserver

Apart from the OnConfigurationRemoved() method, all other methods from
the NetworkConfigurationObserver are only used in tests and should be
removed.

Bug: None
Change-Id: I6fb12c7437101f8f2755ddcafe1e8f70604764ea
Reviewed-on: https://chromium-review.googlesource.com/1104343
Commit-Queue: Alexander Hendrich <hendrich@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570688}

chrome/browser/extensions/api/vpn_provider/vpn_provider_apitest.cc

@@ -16,7 +16,6 @@
 #include "chromeos/dbus/fake_shill_service_client.h"
 #include "chromeos/dbus/fake_shill_third_party_vpn_driver_client.h"
 #include "chromeos/network/network_configuration_handler.h"
-#include "chromeos/network/network_configuration_observer.h"
 #include "chromeos/network/network_profile_handler.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/test/test_utils.h"
@@ -114,23 +113,11 @@ class TestShillThirdPartyVpnDriverClient
   std::vector<char> ip_packet_;
 };
 
-class VpnProviderApiTest : public extensions::ExtensionApiTest,
-                           public NetworkConfigurationObserver {
+class VpnProviderApiTest : public extensions::ExtensionApiTest {
  public:
   VpnProviderApiTest() {}
   ~VpnProviderApiTest() override {}
 
-  void SetUpOnMainThread() override {
-    extensions::ExtensionApiTest::SetUpOnMainThread();
-    NetworkHandler::Get()->network_configuration_handler()->AddObserver(this);
-  }
-
-  void TearDownOnMainThread() override {
-    extensions::ExtensionApiTest::TearDownOnMainThread();
-    NetworkHandler::Get()->network_configuration_handler()->RemoveObserver(
-        this);
-  }
-
   void SetUpInProcessBrowserTestFixture() override {
     extensions::ExtensionApiTest::SetUpInProcessBrowserTestFixture();
     test_client_ = new TestShillThirdPartyVpnDriverClient();
@@ -172,8 +159,9 @@ class VpnProviderApiTest : public extensions::ExtensionApiTest,
   }
 
   std::string GetSingleServicePath() {
-    EXPECT_FALSE(service_path_.empty());
-    return service_path_;
+    std::string service_path = service_->GetSingleServicepathForTesting();
+    EXPECT_FALSE(service_path.empty());
+    return service_path;
   }
 
   bool CreateConfigForTest(const std::string& name) {
@@ -200,25 +188,6 @@ class VpnProviderApiTest : public extensions::ExtensionApiTest,
         base::Bind(DoNothingFailureCallback));
   }
 
-  // NetworkConfigurationObserver:
-  void OnConfigurationCreated(
-      const std::string& service_path,
-      const std::string& profile_path,
-      const base::DictionaryValue& properties) override {
-    service_path_ = service_path;
-  }
-
-  void OnConfigurationRemoved(const std::string& service_path,
-                              const std::string& guid) override {}
-
-  void OnPropertiesSet(const std::string& service_path,
-                       const std::string& guid,
-                       const base::DictionaryValue& set_properties) override {}
-
-  void OnConfigurationProfileChanged(const std::string& service_path,
-                                     const std::string& profile_path) override {
-  }
-
  protected:
   TestShillThirdPartyVpnDriverClient* test_client_ = nullptr;
   VpnService* service_ = nullptr;

chromeos/network/network_configuration_handler.cc

@@ -503,13 +503,6 @@ void NetworkConfigurationHandler::ConfigurationCompleted(
   // the newly configured properties immediately, request an update here.
   network_state_handler_->RequestUpdateForNetwork(service_path.value());
 
-  // Notify observers immediately. (Note: Currently this is primarily used
-  // by tests).
-  for (auto& observer : observers_) {
-    observer.OnConfigurationCreated(service_path.value(), profile_path,
-                                    *configure_properties);
-  }
-
   if (callback.is_null())
     return;
 
@@ -540,8 +533,6 @@ void NetworkConfigurationHandler::SetNetworkProfileCompleted(
     const base::Closure& callback) {
   if (!callback.is_null())
     callback.Run();
-  for (auto& observer : observers_)
-    observer.OnConfigurationProfileChanged(service_path, profile_path);
 }
 
 void NetworkConfigurationHandler::GetPropertiesCallback(
@@ -594,10 +585,6 @@ void NetworkConfigurationHandler::SetPropertiesSuccessCallback(
   if (!network_state)
     return;  // Network no longer exists, do not notify or request update.
 
-  for (auto& observer : observers_) {
-    observer.OnPropertiesSet(service_path, network_state->guid(),
-                             *set_properties);
-  }
   network_state_handler_->RequestUpdateForNetwork(service_path);
 }
 

chromeos/network/network_configuration_handler_unittest.cc

@@ -93,62 +93,20 @@ class TestNetworkConfigurationObserver : public NetworkConfigurationObserver {
   TestNetworkConfigurationObserver() = default;
 
   // NetworkConfigurationObserver
-  void OnConfigurationCreated(
-      const std::string& service_path,
-      const std::string& profile_path,
-      const base::DictionaryValue& properties) override {
-    ASSERT_EQ(0u, configurations_.count(service_path));
-    configurations_[service_path] = properties.CreateDeepCopy();
-    profiles_[profile_path].insert(service_path);
-  }
-
   void OnConfigurationRemoved(const std::string& service_path,
                               const std::string& guid) override {
-    ASSERT_EQ(1u, configurations_.count(service_path));
-    configurations_.erase(service_path);
-    for (auto& p : profiles_) {
-      p.second.erase(service_path);
-    }
-  }
-
-  void OnConfigurationProfileChanged(const std::string& service_path,
-                                     const std::string& profile_path) override {
-    for (auto& p : profiles_) {
-      p.second.erase(service_path);
-    }
-    profiles_[profile_path].insert(service_path);
-  }
-
-  void OnPropertiesSet(const std::string& service_path,
-                       const std::string& guid,
-                       const base::DictionaryValue& set_properties) override {
-    configurations_[service_path]->MergeDictionary(&set_properties);
+    ASSERT_EQ(removed_configurations_.end(),
+              removed_configurations_.find(service_path));
+    removed_configurations_[service_path] = guid;
   }
 
-  bool HasConfiguration(const std::string& service_path) {
-    return configurations_.count(service_path) == 1;
-  }
-
-  std::string GetStringProperty(const std::string& service_path,
-                                const std::string& property) {
-    if (!HasConfiguration(service_path))
-      return "";
-    std::string result;
-    configurations_[service_path]->GetStringWithoutPathExpansion(property,
-                                                                 &result);
-    return result;
-  }
-
-  bool HasConfigurationInProfile(const std::string& service_path,
-                                 const std::string& profile_path) {
-    if (profiles_.count(profile_path) == 0)
-      return false;
-    return profiles_[profile_path].count(service_path) == 1;
+  bool HasRemovedConfiguration(const std::string& service_path) {
+    return removed_configurations_.find(service_path) !=
+           removed_configurations_.end();
   }
 
  private:
-  std::map<std::string, std::unique_ptr<base::DictionaryValue>> configurations_;
-  std::map<std::string, std::set<std::string>> profiles_;
+  std::map<std::string, std::string> removed_configurations_;
 
   DISALLOW_COPY_AND_ASSIGN(TestNetworkConfigurationObserver);
 };
@@ -669,7 +627,6 @@ TEST_F(NetworkConfigurationHandlerTest, StubCreateConfiguration) {
 
 TEST_F(NetworkConfigurationHandlerTest, NetworkConfigurationObserver) {
   const std::string service_path("/service/test_wifi");
-  const std::string test_passphrase("test_passphrase");
 
   auto network_configuration_observer =
       std::make_unique<TestNetworkConfigurationObserver>();
@@ -677,48 +634,15 @@ TEST_F(NetworkConfigurationHandlerTest, NetworkConfigurationObserver) {
       network_configuration_observer.get());
   CreateTestConfiguration(service_path, shill::kTypeWifi);
 
-  EXPECT_TRUE(network_configuration_observer->HasConfiguration(service_path));
-  EXPECT_TRUE(network_configuration_observer->HasConfigurationInProfile(
-      service_path, NetworkProfileHandler::GetSharedProfilePath()));
-  EXPECT_EQ(shill::kTypeWifi, network_configuration_observer->GetStringProperty(
-                                  service_path, shill::kTypeProperty));
-
-  base::DictionaryValue properties_to_set;
-  properties_to_set.SetKey(shill::kPassphraseProperty,
-                           base::Value(test_passphrase));
-  network_configuration_handler_->SetShillProperties(
-      service_path, properties_to_set, base::DoNothing(),
-      base::Bind(&ErrorCallback));
-  base::RunLoop().RunUntilIdle();
-  EXPECT_EQ(test_passphrase, network_configuration_observer->GetStringProperty(
-                                 service_path, shill::kPassphraseProperty));
-
-  std::string user_profile = "/profiles/user1";
-  std::string userhash = "user1";
-  DBusThreadManager::Get()
-      ->GetShillProfileClient()
-      ->GetTestInterface()
-      ->AddProfile(user_profile, userhash);
-
-  network_configuration_handler_->SetNetworkProfile(service_path, user_profile,
-                                                    base::DoNothing(),
-                                                    base::Bind(&ErrorCallback));
-  base::RunLoop().RunUntilIdle();
-  EXPECT_TRUE(network_configuration_observer->HasConfiguration(service_path));
-  EXPECT_FALSE(network_configuration_observer->HasConfigurationInProfile(
-      service_path, NetworkProfileHandler::GetSharedProfilePath()));
-  EXPECT_TRUE(network_configuration_observer->HasConfigurationInProfile(
-      service_path, user_profile));
+  EXPECT_FALSE(
+      network_configuration_observer->HasRemovedConfiguration(service_path));
 
   network_configuration_handler_->RemoveConfiguration(
       service_path, base::DoNothing(), base::Bind(&ErrorCallback));
   base::RunLoop().RunUntilIdle();
 
-  EXPECT_FALSE(network_configuration_observer->HasConfiguration(service_path));
-  EXPECT_FALSE(network_configuration_observer->HasConfigurationInProfile(
-      service_path, NetworkProfileHandler::GetSharedProfilePath()));
-  EXPECT_FALSE(network_configuration_observer->HasConfigurationInProfile(
-      service_path, user_profile));
+  EXPECT_TRUE(
+      network_configuration_observer->HasRemovedConfiguration(service_path));
 
   network_configuration_handler_->RemoveObserver(
       network_configuration_observer.get());

chromeos/network/network_configuration_observer.h

@@ -7,30 +7,11 @@
 
 #include <string>
 
-#include "base/macros.h"
-
-namespace base {
-class DictionaryValue;
-}
-
 namespace chromeos {
 
-// Observer class for network configuration events.
-// TODO(stevenjb/hendrich): This is only used in tests and should be removed.
+// Observer class for network configuration events (remove only).
 class NetworkConfigurationObserver {
  public:
-  // Called whenever a network configuration is created, or an existing
-  // configuration is replaced (see comment for CreateConfiguration).
-  // |service_path| provides the Shill current identifier for the network.
-  // Use properties[GUID] to get the global unique identifier. |profile_path|
-  // can be used to determine whether or not the network is shared.
-  // |properties| contains the Shill properties that were passed to
-  // NetworkConfigurationHandler::CreateConfiguration.
-  virtual void OnConfigurationCreated(
-      const std::string& service_path,
-      const std::string& profile_path,
-      const base::DictionaryValue& properties) = 0;
-
   // Called whenever a network configuration is removed. |service_path|
   // provides the Shill current identifier for the network. |guid| will be set
   // to the corresponding GUID for the network if known at the time of removal,
@@ -38,20 +19,6 @@ class NetworkConfigurationObserver {
   virtual void OnConfigurationRemoved(const std::string& service_path,
                                       const std::string& guid) = 0;
 
-  // Called whenever network properties are set. |service_path| provides the
-  // Shill current identifier for the network. |guid| will be set to the
-  // corresponding GUID for the network. |set_properties| contains the Shill
-  // properties that were passed to NetworkConfigurationHandler::SetProperties.
-  virtual void OnPropertiesSet(const std::string& service_path,
-                               const std::string& guid,
-                               const base::DictionaryValue& set_properties) = 0;
-
-  // Called whenever the profile (e.g. shared or user) that a configuration is
-  // associated with changes (see comment for OnConfigurationCreated).
-  virtual void OnConfigurationProfileChanged(
-      const std::string& service_path,
-      const std::string& profile_path) = 0;
-
  protected:
   virtual ~NetworkConfigurationObserver() {}
 

extensions/browser/api/vpn_provider/vpn_service.cc

@@ -278,11 +278,6 @@ std::string VpnService::GetKey(const std::string& extension_id,
   return base::HexEncode(key.data(), key.size());
 }
 
-void VpnService::OnConfigurationCreated(
-    const std::string& service_path,
-    const std::string& profile_path,
-    const base::DictionaryValue& properties) {}
-
 void VpnService::OnConfigurationRemoved(const std::string& service_path,
                                         const std::string& guid) {
   if (service_path_to_configuration_map_.find(service_path) ==
@@ -305,14 +300,6 @@ void VpnService::OnConfigurationRemoved(const std::string& service_path,
   DestroyConfigurationInternal(configuration);
 }
 
-void VpnService::OnPropertiesSet(const std::string& service_path,
-                                 const std::string& guid,
-                                 const base::DictionaryValue& set_properties) {}
-
-void VpnService::OnConfigurationProfileChanged(
-    const std::string& service_path,
-    const std::string& profile_path) {}
-
 void VpnService::OnGetPropertiesSuccess(
     const std::string& service_path,
     const base::DictionaryValue& dictionary) {
@@ -685,4 +672,10 @@ std::unique_ptr<content::VpnServiceProxy> VpnService::GetVpnServiceProxy() {
   return base::WrapUnique(new VpnServiceProxyImpl(weak_factory_.GetWeakPtr()));
 }
 
+const std::string VpnService::GetSingleServicepathForTesting() {
+  if (service_path_to_configuration_map_.size() == 1)
+    return service_path_to_configuration_map_.begin()->first;
+  return std::string();
+}
+
 }  // namespace chromeos

extensions/browser/api/vpn_provider/vpn_service.h

@@ -81,16 +81,8 @@ class VpnService : public KeyedService,
                          const std::string& error_message);
 
   // NetworkConfigurationObserver:
-  void OnConfigurationCreated(const std::string& service_path,
-                              const std::string& profile_path,
-                              const base::DictionaryValue& properties) override;
   void OnConfigurationRemoved(const std::string& service_path,
                               const std::string& guid) override;
-  void OnPropertiesSet(const std::string& service_path,
-                       const std::string& guid,
-                       const base::DictionaryValue& set_properties) override;
-  void OnConfigurationProfileChanged(const std::string& service_path,
-                                     const std::string& profile_path) override;
 
   // NetworkStateHandlerObserver:
   void NetworkListChanged() override;
@@ -163,6 +155,10 @@ class VpnService : public KeyedService,
   // valid to return nullptr.
   std::unique_ptr<content::VpnServiceProxy> GetVpnServiceProxy();
 
+  // Returns the single entry of |service_path_to_configuration_map_| for
+  // testing (see VpnProviderApiTest);
+  const std::string GetSingleServicepathForTesting();
+
  private:
   class VpnConfiguration;
   class VpnServiceProxyImpl;