Prevent networkingPrivate.forgetNetwork from removing shared configs

chromeos/dbus/fake_shill_profile_client.cc

Index: chromeos/dbus/fake_shill_profile_client.cc
diff --git a/chromeos/dbus/fake_shill_profile_client.cc b/chromeos/dbus/fake_shill_profile_client.cc
index 9a5fe1a3c0522195735e597ff2458e1fabf3e7de..3e107ec6d9237112cdf38e9f83d61eef7410f04c 100644
--- a/chromeos/dbus/fake_shill_profile_client.cc
+++ b/chromeos/dbus/fake_shill_profile_client.cc
@@ -215,22 +215,37 @@ void FakeShillProfileClient::GetProfilePaths(
     profiles->push_back(iter->first);
 }
 
+void FakeShillProfileClient::GetProfilePathsContainingService(
+    const std::string& service_path,
+    std::vector<std::string>* profiles) {
+  for (auto iter = profiles_.begin(); iter != profiles_.end(); ++iter) {
+    std::string profile_path;
+    if (GetServiceDataFromProfile(iter, service_path, &profile_path, nullptr))
+      profiles->push_back(profile_path);
+  }
+}

[stevenjb, 2017/03/29 00:06:45]

blank line

[tbarzic, 2017/03/29 01:48:50]

Done.

 bool FakeShillProfileClient::GetService(const std::string& service_path,
                                         std::string* profile_path,
                                         base::DictionaryValue* properties) {
   properties->Clear();
+
+  auto shared = profiles_.find(GetSharedProfilePath());
+  bool found_shared = false;
+  if (shared != profiles_.end()) {
+    found_shared = GetServiceDataFromProfile(shared, service_path, profile_path,
+                                             properties);
+  }

[stevenjb, 2017/03/29 00:06:45]

So, I kind of hate to ask this, but this is ugly and confusing and I worry about
the next person who comes along and looks at this code.

We should make profiles_ a list instead of a map, and make the path part of
ProfileProperties. It will always be a short list, so iterating or using
std::find() will be fast, and it will simplify this code which should just
iterate through the list and apply the profiles in order (which is what Shill
does IIRC). Then we just need to make sure the shared profile gets added first.

[tbarzic, 2017/03/29 01:48:50]

Sounds reasonable. Done.

+
   for (auto iter = profiles_.begin(); iter != profiles_.end(); ++iter) {
-    const ProfileProperties* profile = iter->second.get();
-    const base::DictionaryValue* entry;
-    if (!profile->entries.GetDictionaryWithoutPathExpansion(
-            service_path, &entry)) {
+    // Shared profile properties have already been applied before the for loop.
+    if (iter->first == GetSharedProfilePath())
       continue;
+    if (GetServiceDataFromProfile(iter, service_path, profile_path,
+                                  properties)) {
+      return true;
     }
-    *profile_path = iter->first;
-    properties->MergeDictionary(entry);
-    return true;
   }
-  return false;
+  return found_shared;
 }
 
 void FakeShillProfileClient::ClearProfiles() {
@@ -250,4 +265,21 @@ FakeShillProfileClient::ProfileProperties* FakeShillProfileClient::GetProfile(
   return found->second.get();
 }
 
+bool FakeShillProfileClient::GetServiceDataFromProfile(
+    const ProfileMap::iterator& iter,

[stevenjb, 2017/03/29 00:06:45]

I would rather pass the profile path and const ProfileProperties* separately
than pass an iterator.

[tbarzic, 2017/03/29 01:48:50]

Done in sense that now only ProfileProperties are needed.

+    const std::string& service_path,
+    std::string* profile_path,
+    base::DictionaryValue* properties) {
+  const ProfileProperties* profile = iter->second.get();
+  const base::DictionaryValue* entry;
+  if (!profile->entries.GetDictionaryWithoutPathExpansion(service_path,
+                                                          &entry)) {
+    return false;
+  }
+  *profile_path = iter->first;
+  if (properties)
+    properties->MergeDictionary(entry);
+  return true;
+}
+
 }  // namespace chromeos