FakeShillServiceClient: Elim remaining use of DictionaryValue

Bug: 1137487
Change-Id: I6164788dc2f84fa670027c41d48f8997c5dbc053
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2466823
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Azeem Arshad <azeemarshad@chromium.org>
Reviewed-by: Pavol Marko <pmarko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#817750}

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

@@ -190,6 +190,14 @@ class VpnProviderApiTest : public extensions::ExtensionApiTest {
         base::DoNothing(), base::Bind(DoNothingFailureCallback));
   }
 
+  bool HasService(const std::string& service_path) const {
+    std::string profile_path;
+    base::Value properties =
+        ShillProfileClient::Get()->GetTestInterface()->GetService(
+            service_path, &profile_path);
+    return properties.is_dict();
+  }
+
  protected:
   TestShillThirdPartyVpnDriverClient* test_client_ = nullptr;  // Unowned
   VpnService* service_ = nullptr;
@@ -214,11 +222,7 @@ IN_PROC_BROWSER_TEST_F(VpnProviderApiTest, CreateConfig) {
   AddNetworkProfileForUser();
   EXPECT_TRUE(RunExtensionTest("createConfigSuccess"));
   EXPECT_TRUE(DoesConfigExist(kTestConfig));
-  const std::string service_path = GetSingleServicePath();
-  std::string profile_path;
-  base::DictionaryValue properties;
-  EXPECT_TRUE(ShillProfileClient::Get()->GetTestInterface()->GetService(
-      service_path, &profile_path, &properties));
+  EXPECT_TRUE(HasService(GetSingleServicePath()));
 }
 
 IN_PROC_BROWSER_TEST_F(VpnProviderApiTest, DestroyConfig) {
@@ -227,15 +231,11 @@ IN_PROC_BROWSER_TEST_F(VpnProviderApiTest, DestroyConfig) {
   EXPECT_TRUE(CreateConfigForTest(kTestConfig));
   EXPECT_TRUE(DoesConfigExist(kTestConfig));
   const std::string service_path = GetSingleServicePath();
-  std::string profile_path;
-  base::DictionaryValue properties;
-  EXPECT_TRUE(ShillProfileClient::Get()->GetTestInterface()->GetService(
-      service_path, &profile_path, &properties));
+  EXPECT_TRUE(HasService(service_path));
 
   EXPECT_TRUE(RunExtensionTest("destroyConfigSuccess"));
   EXPECT_FALSE(DoesConfigExist(kTestConfig));
-  EXPECT_FALSE(ShillProfileClient::Get()->GetTestInterface()->GetService(
-      service_path, &profile_path, &properties));
+  EXPECT_FALSE(HasService(service_path));
 }
 
 IN_PROC_BROWSER_TEST_F(VpnProviderApiTest, DestroyConnectedConfig) {
@@ -245,10 +245,7 @@ IN_PROC_BROWSER_TEST_F(VpnProviderApiTest, DestroyConnectedConfig) {
   EXPECT_TRUE(CreateConfigForTest(kTestConfig));
   EXPECT_TRUE(DoesConfigExist(kTestConfig));
   const std::string service_path = GetSingleServicePath();
-  std::string profile_path;
-  base::DictionaryValue properties;
-  EXPECT_TRUE(ShillProfileClient::Get()->GetTestInterface()->GetService(
-      service_path, &profile_path, &properties));
+  EXPECT_TRUE(HasService(service_path));
   EXPECT_FALSE(IsConfigConnected());
 
   const std::string object_path = shill::kObjectPathBase + GetKey(kTestConfig);
@@ -262,8 +259,7 @@ IN_PROC_BROWSER_TEST_F(VpnProviderApiTest, DestroyConnectedConfig) {
 
   EXPECT_TRUE(DestroyConfigForTest(kTestConfig));
   EXPECT_FALSE(DoesConfigExist(kTestConfig));
-  EXPECT_FALSE(ShillProfileClient::Get()->GetTestInterface()->GetService(
-      service_path, &profile_path, &properties));
+  EXPECT_FALSE(HasService(service_path));
 
   ASSERT_TRUE(catcher.GetNextResult());
 }
@@ -273,11 +269,7 @@ IN_PROC_BROWSER_TEST_F(VpnProviderApiTest, VpnSuccess) {
   AddNetworkProfileForUser();
   EXPECT_TRUE(RunExtensionTest("createConfigConnectAndDisconnect"));
   EXPECT_TRUE(DoesConfigExist(kTestConfig));
-  const std::string service_path = GetSingleServicePath();
-  std::string profile_path;
-  base::DictionaryValue properties;
-  EXPECT_TRUE(ShillProfileClient::Get()->GetTestInterface()->GetService(
-      service_path, &profile_path, &properties));
+  EXPECT_TRUE(HasService(GetSingleServicePath()));
   EXPECT_FALSE(IsConfigConnected());
 
   const std::string object_path = shill::kObjectPathBase + GetKey(kTestConfig);
@@ -371,16 +363,12 @@ IN_PROC_BROWSER_TEST_F(VpnProviderApiTest, CreateUninstall) {
   EXPECT_TRUE(DoesConfigExist(kTestConfig));
 
   const std::string service_path = GetSingleServicePath();
-  std::string profile_path;
-  base::DictionaryValue properties;
-  EXPECT_TRUE(ShillProfileClient::Get()->GetTestInterface()->GetService(
-      service_path, &profile_path, &properties));
+  EXPECT_TRUE(HasService(service_path));
 
   UninstallExtension(extension_id_);
   content::RunAllPendingInMessageLoop();
   EXPECT_FALSE(DoesConfigExist(kTestConfig));
-  EXPECT_FALSE(ShillProfileClient::Get()->GetTestInterface()->GetService(
-      service_path, &profile_path, &properties));
+  EXPECT_FALSE(HasService(service_path));
 }
 
 IN_PROC_BROWSER_TEST_F(VpnProviderApiTest, CreateDisable) {
@@ -390,10 +378,7 @@ IN_PROC_BROWSER_TEST_F(VpnProviderApiTest, CreateDisable) {
   EXPECT_TRUE(DoesConfigExist(kTestConfig));
 
   const std::string service_path = GetSingleServicePath();
-  std::string profile_path;
-  base::DictionaryValue properties;
-  EXPECT_TRUE(ShillProfileClient::Get()->GetTestInterface()->GetService(
-      service_path, &profile_path, &properties));
+  EXPECT_TRUE(HasService(service_path));
 
   extensions::ExtensionService* extension_service =
       extensions::ExtensionSystem::Get(profile())->extension_service();
@@ -401,8 +386,7 @@ IN_PROC_BROWSER_TEST_F(VpnProviderApiTest, CreateDisable) {
       extension_id_, extensions::disable_reason::DISABLE_USER_ACTION);
   content::RunAllPendingInMessageLoop();
   EXPECT_FALSE(DoesConfigExist(kTestConfig));
-  EXPECT_FALSE(ShillProfileClient::Get()->GetTestInterface()->GetService(
-      service_path, &profile_path, &properties));
+  EXPECT_FALSE(HasService(service_path));
 }
 
 IN_PROC_BROWSER_TEST_F(VpnProviderApiTest, CreateBlocklist) {
@@ -412,18 +396,14 @@ IN_PROC_BROWSER_TEST_F(VpnProviderApiTest, CreateBlocklist) {
   EXPECT_TRUE(DoesConfigExist(kTestConfig));
 
   const std::string service_path = GetSingleServicePath();
-  std::string profile_path;
-  base::DictionaryValue properties;
-  EXPECT_TRUE(ShillProfileClient::Get()->GetTestInterface()->GetService(
-      service_path, &profile_path, &properties));
+  EXPECT_TRUE(HasService(service_path));
 
   extensions::ExtensionService* extension_service =
       extensions::ExtensionSystem::Get(profile())->extension_service();
   extension_service->BlocklistExtensionForTest(extension_id_);
   content::RunAllPendingInMessageLoop();
   EXPECT_FALSE(DoesConfigExist(kTestConfig));
-  EXPECT_FALSE(ShillProfileClient::Get()->GetTestInterface()->GetService(
-      service_path, &profile_path, &properties));
+  EXPECT_FALSE(HasService(service_path));
 }
 
 }  // namespace chromeos

chromeos/dbus/shill/fake_shill_profile_client.cc

@@ -233,24 +233,19 @@ void FakeShillProfileClient::GetProfilePathsContainingService(
   }
 }
 
-bool FakeShillProfileClient::GetService(const std::string& service_path,
-                                        std::string* profile_path,
-                                        base::DictionaryValue* properties) {
+base::Value FakeShillProfileClient::GetService(const std::string& service_path,
+                                               std::string* profile_path) {
   DCHECK(profile_path);
-  DCHECK(properties);
 
-  properties->Clear();
   // Returns the entry added latest.
   for (const auto& profile : base::Reversed(profiles_)) {
-    const base::Value* entry = profile->entries.FindKeyOfType(
-        service_path, base::Value::Type::DICTIONARY);
-    if (entry) {
-      *profile_path = profile->path;
-      *properties = static_cast<base::DictionaryValue&&>(entry->Clone());
-      return true;
-    }
+    const base::Value* entry = profile->entries.FindDictKey(service_path);
+    if (!entry)
+      continue;
+    *profile_path = profile->path;
+    return entry->Clone();
   }
-  return false;
+  return base::Value();
 }
 
 bool FakeShillProfileClient::HasService(const std::string& service_path) {

chromeos/dbus/shill/fake_shill_profile_client.h

@@ -58,9 +58,8 @@ class COMPONENT_EXPORT(SHILL_CLIENT) FakeShillProfileClient
   void GetProfilePathsContainingService(
       const std::string& service_path,
       std::vector<std::string>* profiles) override;
-  bool GetService(const std::string& service_path,
-                  std::string* profile_path,
-                  base::DictionaryValue* properties) override;
+  base::Value GetService(const std::string& service_path,
+                         std::string* profile_path) override;
   bool HasService(const std::string& service_path) override;
   void ClearProfiles() override;
   void SetSimulateDeleteResult(FakeShillSimulatedResult delete_result) override;

chromeos/dbus/shill/fake_shill_service_client.h

@@ -116,16 +116,15 @@ class COMPONENT_EXPORT(SHILL_CLIENT) FakeShillServiceClient
 
   void NotifyObserversPropertyChanged(const dbus::ObjectPath& service_path,
                                       const std::string& property);
-  base::DictionaryValue* GetModifiableServiceProperties(
-      const std::string& service_path,
-      bool create_if_missing);
+  base::Value* GetModifiableServiceProperties(const std::string& service_path,
+                                              bool create_if_missing);
   PropertyObserverList& GetObserverList(const dbus::ObjectPath& device_path);
   void SetOtherServicesOffline(const std::string& service_path);
   void SetCellularActivated(const dbus::ObjectPath& service_path,
                             ErrorCallback error_callback);
   void ContinueConnect(const std::string& service_path);
 
-  base::DictionaryValue stub_services_;
+  base::Value stub_services_{base::Value::Type::DICTIONARY};
 
   // Per network service, stores a closure that is executed on each connection
   // attempt. The callback can for example modify the services properties in

chromeos/dbus/shill/shill_profile_client.h

@@ -14,10 +14,6 @@
 #include "chromeos/dbus/shill/fake_shill_simulated_result.h"
 #include "chromeos/dbus/shill/shill_client_helper.h"
 
-namespace base {
-class DictionaryValue;
-}  // namespace base
-
 namespace dbus {
 class Bus;
 class ObjectPath;
@@ -74,12 +70,12 @@ class COMPONENT_EXPORT(SHILL_CLIENT) ShillProfileClient {
         const std::string& service_path,
         std::vector<std::string>* profiles) = 0;
 
-    // Sets |properties| to the entry for |service_path|, sets |profile_path|
-    // to the path of the profile with the entry, and returns true if the
-    // service exists in any profile.
-    virtual bool GetService(const std::string& service_path,
-                            std::string* profile_path,
-                            base::DictionaryValue* properties) = 0;
+    // Returns the entry for |service_path| if it exists in any profile and sets
+    // |profile_path| to the path of the profile the service was found in.
+    // Profiles are searched starting with the most recently added profile.
+    // If the service does not exist in any profile, an empty Value is returned.
+    virtual base::Value GetService(const std::string& service_path,
+                                   std::string* profile_path) = 0;
 
     // Returns true iff an entry sepcified via |service_path| exists in
     // any profile.

extensions/browser/api/networking_private/networking_private_chromeos_unittest.cc

@@ -276,8 +276,7 @@ class NetworkingPrivateApiTest : public ApiUnitTest {
 
   bool GetServiceProfile(const std::string& service_path,
                          std::string* profile_path) {
-    base::DictionaryValue properties;
-    return profile_test_->GetService(service_path, profile_path, &properties);
+    return profile_test_->GetService(service_path, profile_path).is_dict();
   }
 
   std::unique_ptr<base::DictionaryValue> GetNetworkProperties(