ShillManagerClient: Use base::Value in Configure methods

Minor cleanup to help deprecate base::DictionaryValue in
chromeos/network and facilitate other cleanup.

The affected code is well covered by unit tests and browser tests.

Bug: None
Change-Id: I7c80587fe84011f3041c781e173644466230fcd8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2453129
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Pavol Marko <pmarko@chromium.org>
Reviewed-by: Azeem Arshad <azeemarshad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#815259}

chromeos/dbus/shill/fake_shill_manager_client.cc

@@ -64,6 +64,15 @@ int GetIntValue(const base::Value& dict, const char* key) {
   return value ? value->GetInt() : 0;
 }
 
+bool GetString(const base::Value& dict, const char* key, std::string* result) {
+  // Note: FindPath uses path expansion which is currently required for the
+  // fake shill implementations.
+  const base::Value* v = dict.FindPathOfType(key, base::Value::Type::STRING);
+  if (!v)
+    return false;
+  return v->GetAsString(result);
+}
+
 std::string GetStringValue(const base::Value& dict, const char* key) {
   const base::Value* value = dict.FindKeyOfType(key, base::Value::Type::STRING);
   return value ? value->GetString() : std::string();
@@ -300,9 +309,9 @@ void FakeShillManagerClient::RequestScan(const std::string& type,
 void FakeShillManagerClient::EnableTechnology(const std::string& type,
                                               base::OnceClosure callback,
                                               ErrorCallback error_callback) {
-  base::ListValue* enabled_list = nullptr;
-  if (!stub_properties_.GetListWithoutPathExpansion(
-          shill::kAvailableTechnologiesProperty, &enabled_list)) {
+  base::Value* enabled_list =
+      stub_properties_.FindListKey(shill::kAvailableTechnologiesProperty);
+  if (!enabled_list) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
                                                   std::move(callback));
     base::ThreadTaskRunnerHandle::Get()->PostTask(
@@ -321,9 +330,9 @@ void FakeShillManagerClient::EnableTechnology(const std::string& type,
 void FakeShillManagerClient::DisableTechnology(const std::string& type,
                                                base::OnceClosure callback,
                                                ErrorCallback error_callback) {
-  base::ListValue* enabled_list = nullptr;
-  if (!stub_properties_.GetListWithoutPathExpansion(
-          shill::kAvailableTechnologiesProperty, &enabled_list)) {
+  base::Value* enabled_list =
+      stub_properties_.FindListKey(shill::kAvailableTechnologiesProperty);
+  if (!enabled_list) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE, base::BindOnce(std::move(error_callback), "StubError",
                                   "Property not found"));
@@ -337,8 +346,7 @@ void FakeShillManagerClient::DisableTechnology(const std::string& type,
       interactive_delay_);
 }
 
-void FakeShillManagerClient::ConfigureService(
-    const base::DictionaryValue& properties,
+void FakeShillManagerClient::ConfigureService(const base::Value& properties,
                                               ObjectPathCallback callback,
                                               ErrorCallback error_callback) {
   switch (simulate_configuration_result_) {
@@ -360,8 +368,8 @@ void FakeShillManagerClient::ConfigureService(
   std::string guid;
   std::string type;
   std::string name;
-  if (!properties.GetString(shill::kGuidProperty, &guid) ||
-      !properties.GetString(shill::kTypeProperty, &type)) {
+  if (!GetString(properties, shill::kGuidProperty, &guid) ||
+      !GetString(properties, shill::kTypeProperty, &type)) {
     LOG(ERROR) << "ConfigureService requires GUID and Type to be defined";
     // If the properties aren't filled out completely, then just return an empty
     // object path.
@@ -371,11 +379,11 @@ void FakeShillManagerClient::ConfigureService(
   }
 
   if (type == shill::kTypeWifi) {
-    properties.GetString(shill::kSSIDProperty, &name);
+    GetString(properties, shill::kSSIDProperty, &name);
 
     if (name.empty()) {
       std::string hex_name;
-      properties.GetString(shill::kWifiHexSsid, &hex_name);
+      GetString(properties, shill::kWifiHexSsid, &hex_name);
       if (!hex_name.empty()) {
         std::vector<uint8_t> bytes;
         if (base::HexStringToBytes(hex_name, &bytes)) {
@@ -385,12 +393,12 @@ void FakeShillManagerClient::ConfigureService(
     }
   }
   if (name.empty())
-    properties.GetString(shill::kNameProperty, &name);
+    GetString(properties, shill::kNameProperty, &name);
   if (name.empty())
     name = guid;
 
   std::string ipconfig_path;
-  properties.GetString(shill::kIPConfigProperty, &ipconfig_path);
+  GetString(properties, shill::kIPConfigProperty, &ipconfig_path);
 
   std::string service_path = service_client->FindServiceMatchingGUID(guid);
   if (service_path.empty())
@@ -436,17 +444,16 @@ void FakeShillManagerClient::ConfigureService(
 
 void FakeShillManagerClient::ConfigureServiceForProfile(
     const dbus::ObjectPath& profile_path,
-    const base::DictionaryValue& properties,
+    const base::Value& properties,
     ObjectPathCallback callback,
     ErrorCallback error_callback) {
   std::string profile_property;
-  properties.GetStringWithoutPathExpansion(shill::kProfileProperty,
-                                           &profile_property);
+  GetString(properties, shill::kProfileProperty, &profile_property);
   CHECK(profile_property == profile_path.value());
   ConfigureService(properties, std::move(callback), std::move(error_callback));
 }
 
-void FakeShillManagerClient::GetService(const base::DictionaryValue& properties,
+void FakeShillManagerClient::GetService(const base::Value& properties,
                                         ObjectPathCallback callback,
                                         ErrorCallback error_callback) {
   base::ThreadTaskRunnerHandle::Get()->PostTask(
@@ -562,9 +569,8 @@ void FakeShillManagerClient::SetTechnologyProhibited(const std::string& type,
   CallNotifyObserversPropertyChanged(shill::kProhibitedTechnologiesProperty);
 }
 
-void FakeShillManagerClient::AddGeoNetwork(
-    const std::string& technology,
-    const base::DictionaryValue& network) {
+void FakeShillManagerClient::AddGeoNetwork(const std::string& technology,
+                                           const base::Value& network) {
   base::Value* list_value =
       stub_geo_networks_.FindKeyOfType(technology, base::Value::Type::LIST);
   if (!list_value) {
@@ -583,7 +589,7 @@ void FakeShillManagerClient::AddProfile(const std::string& profile_path) {
 }
 
 void FakeShillManagerClient::ClearProperties() {
-  stub_properties_.Clear();
+  stub_properties_ = base::Value(base::Value::Type::DICTIONARY);
 }
 
 void FakeShillManagerClient::SetManagerProperty(const std::string& key,
@@ -1073,8 +1079,8 @@ void FakeShillManagerClient::CallNotifyObserversPropertyChanged(
 void FakeShillManagerClient::NotifyObserversPropertyChanged(
     const std::string& property) {
   VLOG(1) << "NotifyObserversPropertyChanged: " << property;
-  base::Value* value = nullptr;
-  if (!stub_properties_.GetWithoutPathExpansion(property, &value)) {
+  base::Value* value = stub_properties_.FindKey(property);
+  if (!value) {
     LOG(ERROR) << "Notify for unknown property: " << property;
     return;
   }
@@ -1104,15 +1110,11 @@ bool FakeShillManagerClient::TechnologyEnabled(const std::string& type) const {
     return true;  // VPN is always "enabled" since there is no associated device
   if (type == shill::kTypeEthernetEap)
     return true;
-  bool enabled = false;
-  const base::ListValue* technologies;
-  if (stub_properties_.GetListWithoutPathExpansion(
-          shill::kEnabledTechnologiesProperty, &technologies)) {
-    base::Value type_value(type);
-    if (technologies->Find(type_value) != technologies->end())
-      enabled = true;
-  }
-  return enabled;
+  const base::Value* technologies =
+      stub_properties_.FindListKey(shill::kEnabledTechnologiesProperty);
+  if (technologies)
+    return base::Contains(technologies->GetList(), base::Value(type));
+  return false;
 }
 
 void FakeShillManagerClient::SetTechnologyEnabled(const std::string& type,
@@ -1132,16 +1134,13 @@ void FakeShillManagerClient::SetTechnologyEnabled(const std::string& type,
 
 base::Value FakeShillManagerClient::GetEnabledServiceList() const {
   base::Value new_service_list(base::Value::Type::LIST);
-  const base::ListValue* service_list;
-  if (stub_properties_.GetListWithoutPathExpansion(
-          shill::kServiceCompleteListProperty, &service_list)) {
+  const base::Value* service_list =
+      stub_properties_.FindListKey(shill::kServiceCompleteListProperty);
+  if (service_list) {
     ShillServiceClient::TestInterface* service_client =
         ShillServiceClient::Get()->GetTestInterface();
-    for (base::ListValue::const_iterator iter = service_list->begin();
-         iter != service_list->end(); ++iter) {
-      std::string service_path;
-      if (!iter->GetAsString(&service_path))
-        continue;
+    for (const base::Value& v : service_list->GetList()) {
+      std::string service_path = v.GetString();
       const base::DictionaryValue* properties =
           service_client->GetServiceProperties(service_path);
       if (!properties) {
@@ -1151,7 +1150,7 @@ base::Value FakeShillManagerClient::GetEnabledServiceList() const {
       std::string type;
       properties->GetString(shill::kTypeProperty, &type);
       if (TechnologyEnabled(type))
-        new_service_list.Append(iter->Clone());
+        new_service_list.Append(v.Clone());
     }
   }
   return new_service_list;

chromeos/dbus/shill/fake_shill_manager_client.h

@@ -47,14 +47,14 @@ class COMPONENT_EXPORT(SHILL_CLIENT) FakeShillManagerClient
   void DisableTechnology(const std::string& type,
                          base::OnceClosure callback,
                          ErrorCallback error_callback) override;
-  void ConfigureService(const base::DictionaryValue& properties,
+  void ConfigureService(const base::Value& properties,
                         ObjectPathCallback callback,
                         ErrorCallback error_callback) override;
   void ConfigureServiceForProfile(const dbus::ObjectPath& profile_path,
-                                  const base::DictionaryValue& properties,
+                                  const base::Value& properties,
                                   ObjectPathCallback callback,
                                   ErrorCallback error_callback) override;
-  void GetService(const base::DictionaryValue& properties,
+  void GetService(const base::Value& properties,
                   ObjectPathCallback callback,
                   ErrorCallback error_callback) override;
   void ConnectToBestServices(base::OnceClosure callback,
@@ -76,7 +76,7 @@ class COMPONENT_EXPORT(SHILL_CLIENT) FakeShillManagerClient
   void SetTechnologyProhibited(const std::string& type,
                                bool prohibited) override;
   void AddGeoNetwork(const std::string& technology,
-                     const base::DictionaryValue& network) override;
+                     const base::Value& network) override;
   void AddProfile(const std::string& profile_path) override;
   void ClearProperties() override;
   void SetManagerProperty(const std::string& key,
@@ -125,10 +125,10 @@ class COMPONENT_EXPORT(SHILL_CLIENT) FakeShillManagerClient
   std::string GetInitialStateForType(const std::string& type, bool* enabled);
 
   // Dictionary of property name -> property value
-  base::DictionaryValue stub_properties_;
+  base::Value stub_properties_{base::Value::Type::DICTIONARY};
 
   // Dictionary of technology -> list of property dictionaries
-  base::DictionaryValue stub_geo_networks_;
+  base::Value stub_geo_networks_{base::Value::Type::DICTIONARY};
 
   // Delay for interactive actions
   base::TimeDelta interactive_delay_;

chromeos/dbus/shill/shill_client_helper.cc

@@ -465,22 +465,20 @@ void ShillClientHelper::AppendValueDataAsVariant(dbus::MessageWriter* writer,
 // static
 void ShillClientHelper::AppendServicePropertiesDictionary(
     dbus::MessageWriter* writer,
-    const base::DictionaryValue& dictionary) {
-  dbus::MessageWriter array_writer(NULL);
+    const base::Value& dictionary) {
+  dbus::MessageWriter array_writer(nullptr);
   writer->OpenArray("{sv}", &array_writer);
-  for (base::DictionaryValue::Iterator it(dictionary); !it.IsAtEnd();
-       it.Advance()) {
-    dbus::MessageWriter entry_writer(NULL);
+  for (auto it : dictionary.DictItems()) {
+    dbus::MessageWriter entry_writer(nullptr);
     array_writer.OpenDictEntry(&entry_writer);
-    entry_writer.AppendString(it.key());
+    entry_writer.AppendString(it.first);
     // Shill expects Cellular.APN to be a string dictionary, a{ss}. All other
     // properties use a varient dictionary, a{sv}. TODO(stevenjb): Remove this
     // hack if/when we change Shill to accept a{sv} for Cellular.APN.
-    DictionaryType dictionary_type = (it.key() == shill::kCellularApnProperty)
+    DictionaryType dictionary_type = (it.first == shill::kCellularApnProperty)
                                          ? DICTIONARY_TYPE_STRING
                                          : DICTIONARY_TYPE_VARIANT;
-    AppendValueDataAsVariantInternal(&entry_writer, it.value(),
-                                     dictionary_type);
+    AppendValueDataAsVariantInternal(&entry_writer, it.second, dictionary_type);
     array_writer.CloseContainer(&entry_writer);
   }
   writer->CloseContainer(&array_writer);

chromeos/dbus/shill/shill_client_helper.h

@@ -138,7 +138,7 @@ class ShillClientHelper {
   // Appends a string-to-variant dictionary to the writer as an '{sv}' array.
   // Each value is written using AppendValueDataAsVariant.
   static void AppendServicePropertiesDictionary(dbus::MessageWriter* writer,
-                                                const base::DictionaryValue&);
+                                                const base::Value& dictionary);
 
  protected:
   // Reference / Ownership management. If the number of active refs (observers

chromeos/dbus/shill/shill_manager_client.cc

@@ -116,7 +116,7 @@ class ShillManagerClientImpl : public ShillManagerClient {
                                              std::move(error_callback));
   }
 
-  void ConfigureService(const base::DictionaryValue& properties,
+  void ConfigureService(const base::Value& properties,
                         ObjectPathCallback callback,
                         ErrorCallback error_callback) override {
     dbus::MethodCall method_call(shill::kFlimflamManagerInterface,
@@ -128,7 +128,7 @@ class ShillManagerClientImpl : public ShillManagerClient {
   }
 
   void ConfigureServiceForProfile(const dbus::ObjectPath& profile_path,
-                                  const base::DictionaryValue& properties,
+                                  const base::Value& properties,
                                   ObjectPathCallback callback,
                                   ErrorCallback error_callback) override {
     dbus::MethodCall method_call(shill::kFlimflamManagerInterface,
@@ -140,7 +140,7 @@ class ShillManagerClientImpl : public ShillManagerClient {
         &method_call, std::move(callback), std::move(error_callback));
   }
 
-  void GetService(const base::DictionaryValue& properties,
+  void GetService(const base::Value& properties,
                   ObjectPathCallback callback,
                   ErrorCallback error_callback) override {
     dbus::MethodCall method_call(shill::kFlimflamManagerInterface,

chromeos/dbus/shill/shill_manager_client.h

@@ -25,7 +25,10 @@ class ShillPropertyChangedObserver;
 
 // ShillManagerClient is used to communicate with the Shill Manager
 // service.  All methods should be called from the origin thread which
-// initializes the DBusThreadManager instance.
+// initializes the DBusThreadManager instance. Most methods that make Shill
+// Manager calls pass |callback| which will be invoked if the method call
+// succeeds, and |error_callback| which will be invoked if the method call fails
+// or returns an error response.
 class COMPONENT_EXPORT(SHILL_CLIENT) ShillManagerClient {
  public:
   typedef ShillClientHelper::DictionaryValueCallback DictionaryValueCallback;
@@ -56,8 +59,11 @@ class COMPONENT_EXPORT(SHILL_CLIENT) ShillManagerClient {
                                            bool initializing) = 0;
     virtual void SetTechnologyProhibited(const std::string& type,
                                          bool prohibited) = 0;
+    // |network| must be a dictionary describing a Shill network configuration
+    // which will be appended to the results returned from
+    // GetNetworksForGeolocation().
     virtual void AddGeoNetwork(const std::string& technology,
-                               const base::DictionaryValue& network) = 0;
+                               const base::Value& network) = 0;
 
     // Does not create an actual profile in the ProfileClient but update the
     // profiles list and sends a notification to observers. This should only be
@@ -141,55 +147,48 @@ class COMPONENT_EXPORT(SHILL_CLIENT) ShillManagerClient {
       ShillPropertyChangedObserver* observer) = 0;
 
   // Calls GetProperties method.
-  // |callback| is called after the method call succeeds.
   virtual void GetProperties(DictionaryValueCallback callback) = 0;
 
   // Calls GetNetworksForGeolocation method.
-  // |callback| is called after the method call succeeds.
   virtual void GetNetworksForGeolocation(DictionaryValueCallback callback) = 0;
 
   // Calls SetProperty method.
-  // |callback| is called after the method call succeeds.
   virtual void SetProperty(const std::string& name,
                            const base::Value& value,
                            base::OnceClosure callback,
                            ErrorCallback error_callback) = 0;
 
   // Calls RequestScan method.
-  // |callback| is called after the method call succeeds.
   virtual void RequestScan(const std::string& type,
                            base::OnceClosure callback,
                            ErrorCallback error_callback) = 0;
 
   // Calls EnableTechnology method.
-  // |callback| is called after the method call succeeds.
   virtual void EnableTechnology(const std::string& type,
                                 base::OnceClosure callback,
                                 ErrorCallback error_callback) = 0;
 
   // Calls DisableTechnology method.
-  // |callback| is called after the method call succeeds.
   virtual void DisableTechnology(const std::string& type,
                                  base::OnceClosure callback,
                                  ErrorCallback error_callback) = 0;
 
-  // Calls ConfigureService method.
-  // |callback| is called after the method call succeeds.
-  virtual void ConfigureService(const base::DictionaryValue& properties,
+  // Calls Manager.ConfigureService with |properties| which must be a
+  // dictionary value describing a Shill service.
+  virtual void ConfigureService(const base::Value& properties,
                                 ObjectPathCallback callback,
                                 ErrorCallback error_callback) = 0;
 
-  // Calls ConfigureServiceForProfile method.
-  // |callback| is called with the created service if the method call succeeds.
-  virtual void ConfigureServiceForProfile(
-      const dbus::ObjectPath& profile_path,
-      const base::DictionaryValue& properties,
+  // Calls Manager.ConfigureServiceForProfile for |profile_path| with
+  // |properties| which must be a dictionary value describing a Shill service.
+  virtual void ConfigureServiceForProfile(const dbus::ObjectPath& profile_path,
+                                          const base::Value& properties,
                                           ObjectPathCallback callback,
                                           ErrorCallback error_callback) = 0;
 
-  // Calls GetService method.
-  // |callback| is called after the method call succeeds.
-  virtual void GetService(const base::DictionaryValue& properties,
+  // Calls Manager.GetService with |properties| which must be a dictionary value
+  // describing a Service.
+  virtual void GetService(const base::Value& properties,
                           ObjectPathCallback callback,
                           ErrorCallback error_callback) = 0;