Fixed merge of device settings.

chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc

Index: chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
diff --git a/chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc b/chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
index f30a5fa61b485dee1cf72513c399510bd704de24..4be1cc7276ee6305ae08946c86f97440e0bdf418 100644
--- a/chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
+++ b/chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
@@ -146,6 +146,132 @@ void DoesPrivateKeyExistAsync(
       callback);
 }
 
+template <typename E>
+bool Eq(const E& lhs, const E& rhs) {
+  return lhs == rhs;
+}
+
+template <>
+bool Eq<em::AppPackEntryProto>(const em::AppPackEntryProto& lhs,
+                               const em::AppPackEntryProto& rhs) {
+  if (lhs.has_extension_id() != rhs.has_extension_id())
+    return false;
+  if (lhs.has_extension_id() && lhs.extension_id() != rhs.extension_id())
+    return false;
+  if (lhs.has_update_url() != rhs.has_update_url())
+    return false;
+  if (lhs.has_update_url() && lhs.update_url() != rhs.update_url())
+    return false;
+  if (lhs.has_obsolete_online_only() != rhs.has_obsolete_online_only())
+    return false;
+  if (lhs.has_obsolete_online_only() &&
+      lhs.obsolete_online_only() != rhs.obsolete_online_only())
+    return false;
+  return true;
+}
+
+template <>
+bool Eq<em::KioskAppInfoProto>(const em::KioskAppInfoProto& lhs,
+                               const em::KioskAppInfoProto& rhs) {
+  if (lhs.has_app_id() != rhs.has_app_id())
+    return false;
+  if (lhs.has_app_id() && lhs.app_id() != rhs.app_id())
+    return false;
+  if (lhs.has_update_url() != rhs.has_update_url())
+    return false;
+  if (lhs.has_update_url() && lhs.update_url() != rhs.update_url())
+    return false;
+  return true;
+}
+
+template <>
+bool Eq<em::DeviceLocalAccountInfoProto>(
+    const em::DeviceLocalAccountInfoProto& lhs,
+    const em::DeviceLocalAccountInfoProto& rhs) {
+  if (lhs.has_deprecated_public_session_id() !=
+      rhs.has_deprecated_public_session_id()) {
+    return false;
+  }
+  if (lhs.has_deprecated_public_session_id() &&
+      lhs.deprecated_public_session_id() !=
+          rhs.deprecated_public_session_id()) {
+    return false;
+  }
+  if (lhs.has_account_id() != rhs.has_account_id())
+    return false;
+  if (lhs.has_account_id() && lhs.account_id() != rhs.account_id())
+    return false;
+  if (lhs.has_type() != rhs.has_type())
+    return false;
+  if (lhs.has_type() && lhs.type() != rhs.type())
+    return false;
+  if (lhs.has_kiosk_app() != rhs.has_kiosk_app())
+    return false;
+  if (lhs.has_kiosk_app() && !Eq(lhs.kiosk_app(), rhs.kiosk_app()))
+    return false;
+  return true;
+}
+
+template <template <typename> class R, typename E>

[Mattias Nissler (ping if slow), 2014/11/07 16:07:35]

Could use better names for R and E to indicate what they are.

+bool Exists(const R<E>& collection, const E& elem) {
+  for (const auto& e : collection) {
+    if (Eq(e, elem))
+      return true;
+  }
+  return false;
+}
+
+template <template <typename> class R, typename E>
+void Merge(const R<E>& from, R<E>* to) {
+  for (const auto& f : from) {
+    if (!Exists(*to, f))
+      *to->Add() = f;

[Mattias Nissler (ping if slow), 2014/11/07 16:07:35]

If merging only ever adds fields, how would I actually remove an entry from a
list? If I'm not mistaken, updating for example the user whitelist by removing
an entry will actually trigger a full write of the list - the expectation is
that the data in device settings will be replaced by this new version of the
list instead of being merged. Am I missing something?

[ygorshenin1, 2014/11/07 17:24:32]

You're right, it's impossible to remove values from a list. It seems that that
solution doesn't work. I think it's better to implement a key-value storage in
OwnerSettingsService for pending changes. I'll upload a changelist for your
review soon.

+  }
+}
+
+void MergeSettings(const em::ChromeDeviceSettingsProto& from,
+                   em::ChromeDeviceSettingsProto* to) {
+  ::google::protobuf::RepeatedPtrField<std::string> user_whitelist(
+      to->user_whitelist().user_whitelist());
+  Merge(from.user_whitelist().user_whitelist(), &user_whitelist);
+
+  ::google::protobuf::RepeatedPtrField<em::AppPackEntryProto> app_pack(
+      to->app_pack().app_pack());
+  Merge(from.app_pack().app_pack(), &app_pack);
+
+  ::google::protobuf::RepeatedPtrField<std::string> app_id(
+      to->pinned_apps().app_id());
+  Merge(from.pinned_apps().app_id(), &app_id);
+
+  ::google::protobuf::RepeatedField<int> connection_types(
+      to->auto_update_settings().allowed_connection_types());
+  Merge(from.auto_update_settings().allowed_connection_types(),
+        &connection_types);
+
+  ::google::protobuf::RepeatedPtrField<std::string> start_up_urls(
+      to->start_up_urls().start_up_urls());
+  Merge(from.start_up_urls().start_up_urls(), &start_up_urls);
+
+  ::google::protobuf::RepeatedPtrField<em::DeviceLocalAccountInfoProto> account(
+      to->device_local_accounts().account());
+  Merge(from.device_local_accounts().account(), &account);
+
+  em::ChromeDeviceSettingsProto settings = from;
+  settings.MergeFrom(*to);
+  settings.mutable_user_whitelist()->mutable_user_whitelist()->CopyFrom(
+      user_whitelist);
+  settings.mutable_app_pack()->mutable_app_pack()->CopyFrom(app_pack);
+  settings.mutable_pinned_apps()->mutable_app_id()->CopyFrom(app_id);
+  settings.mutable_auto_update_settings()
+      ->mutable_allowed_connection_types()
+      ->CopyFrom(connection_types);
+  settings.mutable_start_up_urls()->mutable_start_up_urls()->CopyFrom(
+      start_up_urls);
+  settings.mutable_device_local_accounts()->mutable_account()->CopyFrom(
+      account);
+  to->Swap(&settings);
+}
+
 }  // namespace
 
 OwnerSettingsServiceChromeOS::OwnerSettingsServiceChromeOS(
@@ -633,10 +759,8 @@ bool OwnerSettingsServiceChromeOS::UpdateFromService() {
       !device_settings_service_->device_settings()) {
     return false;
   }
-  enterprise_management::ChromeDeviceSettingsProto settings =
-      *device_settings_service_->device_settings();
-  settings.MergeFrom(device_settings_);
-  device_settings_.Swap(&settings);
+  MergeSettings(*device_settings_service_->device_settings(),
+                &device_settings_);
   return true;
 }