Added key-value storage for pending changes to OwnerSettingsServiceChromeOS. This CL is an alternative to the https://codereview.chromium.org/709763002/.

BUG=430042
TEST=manual tests on a falco device

Review URL: https://codereview.chromium.org/695193004

Cr-Commit-Position: refs/heads/master@{#303465}
(cherry picked from commit 45f512bda2a4f66d17f1f189624da245a5d16a06)

Review URL: https://codereview.chromium.org/723163002

Cr-Commit-Position: refs/branch-heads/2214@{#29}
Cr-Branched-From: 03655fd3f6d72165dc3c9bd2c89807305316fe6c-refs/heads/master@{#303346}

3 files changed, 56 insertions, 60 deletions

diff --git a/chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc b/chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
index f30a5fa..1912712 100644
--- a/chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
+++ b/chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
@@ -157,7 +157,6 @@ OwnerSettingsServiceChromeOS::OwnerSettingsServiceChromeOS(
       profile_(profile),
       waiting_for_profile_creation_(true),
       waiting_for_tpm_token_(true),
-      has_pending_changes_(false),
       weak_factory_(this),
       store_settings_factory_(this) {
   if (TPMTokenLoader::IsInitialized()) {
@@ -180,8 +179,6 @@ OwnerSettingsServiceChromeOS::OwnerSettingsServiceChromeOS(
   registrar_.Add(this,
                  chrome::NOTIFICATION_PROFILE_CREATED,
                  content::Source<Profile>(profile_));
-
-  UpdateFromService();
 }
 
 OwnerSettingsServiceChromeOS::~OwnerSettingsServiceChromeOS() {
@@ -215,15 +212,23 @@ bool OwnerSettingsServiceChromeOS::Set(const std::string& setting,
   if (!IsOwner() && !IsOwnerInTests(user_id_))
     return false;
 
-  UpdateDeviceSettings(setting, value, device_settings_);
+  pending_changes_.add(setting, make_scoped_ptr(value.DeepCopy()));
+
+  em::ChromeDeviceSettingsProto settings;
+  if (tentative_settings_.get()) {
+    settings = *tentative_settings_;
+  } else if (device_settings_service_->status() ==
+                 DeviceSettingsService::STORE_SUCCESS &&
+             device_settings_service_->device_settings()) {
+    settings = *device_settings_service_->device_settings();
+  }
+  UpdateDeviceSettings(setting, value, settings);
   em::PolicyData policy_data;
   policy_data.set_username(user_id_);
-  CHECK(device_settings_.SerializeToString(policy_data.mutable_policy_value()));
-  FOR_EACH_OBSERVER(OwnerSettingsService::Observer,
-                    observers_,
+  CHECK(settings.SerializeToString(policy_data.mutable_policy_value()));
+  FOR_EACH_OBSERVER(OwnerSettingsService::Observer, observers_,
                     OnTentativeChangesInPolicy(policy_data));
-  has_pending_changes_ = true;
-  StoreDeviceSettings();
+  StorePendingChanges();
   return true;
 }
 
@@ -236,12 +241,9 @@ bool OwnerSettingsServiceChromeOS::CommitTentativeDeviceSettings(
                << user_id_;
     return false;
   }
-  CHECK(device_settings_.ParseFromString(policy->policy_value()));
-  FOR_EACH_OBSERVER(OwnerSettingsService::Observer,
-                    observers_,
-                    OnTentativeChangesInPolicy(*policy));
-  has_pending_changes_ = true;
-  StoreDeviceSettings();
+  tentative_settings_.reset(new em::ChromeDeviceSettingsProto);
+  CHECK(tentative_settings_->ParseFromString(policy->policy_value()));
+  StorePendingChanges();
   return true;
 }
 
@@ -273,12 +275,12 @@ void OwnerSettingsServiceChromeOS::OwnerKeySet(bool success) {
 
 void OwnerSettingsServiceChromeOS::OwnershipStatusChanged() {
   DCHECK(thread_checker_.CalledOnValidThread());
-  StoreDeviceSettings();
+  StorePendingChanges();
 }
 
 void OwnerSettingsServiceChromeOS::DeviceSettingsUpdated() {
   DCHECK(thread_checker_.CalledOnValidThread());
-  StoreDeviceSettings();
+  StorePendingChanges();
 }
 
 void OwnerSettingsServiceChromeOS::OnDeviceSettingsServiceShutdown() {
@@ -586,17 +588,32 @@ void OwnerSettingsServiceChromeOS::ReloadKeypairImpl(const base::Callback<
                  callback));
 }
 
-void OwnerSettingsServiceChromeOS::StoreDeviceSettings() {
-  if (!has_pending_changes_ || store_settings_factory_.HasWeakPtrs())
+void OwnerSettingsServiceChromeOS::StorePendingChanges() {
+  if (!has_pending_changes() || store_settings_factory_.HasWeakPtrs() ||
+      !device_settings_service_) {
     return;
-  if (!UpdateFromService())
+  }
+
+  em::ChromeDeviceSettingsProto settings;
+  if (tentative_settings_.get()) {
+    settings.Swap(tentative_settings_.get());
+    tentative_settings_.reset();
+  } else if (device_settings_service_->status() ==
+                 DeviceSettingsService::STORE_SUCCESS &&
+             device_settings_service_->device_settings()) {
+    settings = *device_settings_service_->device_settings();
+  } else {
     return;
+  }
+
+  for (const auto& change : pending_changes_)
+    UpdateDeviceSettings(change.first, *change.second, settings);
+  pending_changes_.clear();
+
   scoped_ptr<em::PolicyData> policy = AssemblePolicy(
-      user_id_, device_settings_service_->policy_data(), &device_settings_);
-  has_pending_changes_ = false;
+      user_id_, device_settings_service_->policy_data(), &settings);
   bool rv = AssembleAndSignPolicyAsync(
-      content::BrowserThread::GetBlockingPool(),
-      policy.Pass(),
+      content::BrowserThread::GetBlockingPool(), policy.Pass(),
       base::Bind(&OwnerSettingsServiceChromeOS::OnPolicyAssembledAndSigned,
                  store_settings_factory_.GetWeakPtr()));
   if (!rv)
@@ -621,23 +638,7 @@ void OwnerSettingsServiceChromeOS::OnSignedPolicyStored(bool success) {
   FOR_EACH_OBSERVER(OwnerSettingsService::Observer,
                     observers_,
                     OnSignedPolicyStored(success));
-  StoreDeviceSettings();
-  if (!success)
-    has_pending_changes_ = true;
-}
-
-bool OwnerSettingsServiceChromeOS::UpdateFromService() {
-  if (!device_settings_service_ ||
-      device_settings_service_->status() !=
-          DeviceSettingsService::STORE_SUCCESS ||
-      !device_settings_service_->device_settings()) {
-    return false;
-  }
-  enterprise_management::ChromeDeviceSettingsProto settings =
-      *device_settings_service_->device_settings();
-  settings.MergeFrom(device_settings_);
-  device_settings_.Swap(&settings);
-  return true;
+  StorePendingChanges();
 }
 
 }  // namespace chromeos
diff --git a/chrome/browser/chromeos/ownership/owner_settings_service_chromeos.h b/chrome/browser/chromeos/ownership/owner_settings_service_chromeos.h
index 6b24bfe..1e6415f 100644
--- a/chrome/browser/chromeos/ownership/owner_settings_service_chromeos.h
+++ b/chrome/browser/chromeos/ownership/owner_settings_service_chromeos.h
@@ -9,7 +9,9 @@
 #include <vector>
 
 #include "base/callback_forward.h"
+#include "base/containers/scoped_ptr_hash_map.h"
 #include "base/macros.h"
+#include "base/values.h"
 #include "chrome/browser/chromeos/policy/proto/chrome_device_policy.pb.h"
 #include "chrome/browser/chromeos/settings/device_settings_service.h"
 #include "chromeos/dbus/session_manager_client.h"
@@ -83,7 +85,9 @@ class OwnerSettingsServiceChromeOS : public ownership::OwnerSettingsService,
       const base::Value& value,
       enterprise_management::ChromeDeviceSettingsProto& settings);
 
-  bool has_pending_changes() const { return has_pending_changes_; }
+  bool has_pending_changes() const {
+    return !pending_changes_.empty() || tentative_settings_.get();
+  }
 
  private:
   friend class OwnerSettingsServiceChromeOSFactory;
@@ -104,10 +108,8 @@ class OwnerSettingsServiceChromeOS : public ownership::OwnerSettingsService,
   // Possibly notifies DeviceSettingsService that owner's keypair is loaded.
   virtual void OnPostKeypairLoadedActions() override;
 
-  // Tries to sign store current device settings if there're pending
-  // changes in device settings and no active previous call to
-  // DeviceSettingsService::Store().
-  void StoreDeviceSettings();
+  // Tries to apply recent changes to device settings proto, sign it and store.
+  void StorePendingChanges();
 
   // Called when current device settings are successfully signed.
   // Sends signed settings for storage.
@@ -119,10 +121,6 @@ class OwnerSettingsServiceChromeOS : public ownership::OwnerSettingsService,
   // settings again.
   void OnSignedPolicyStored(bool success);
 
-  // Fetches device settings from DeviceSettingsService and merges
-  // them with local device settings.
-  bool UpdateFromService();
-
   DeviceSettingsService* device_settings_service_;
 
   // Profile this service instance belongs to.
@@ -137,15 +135,12 @@ class OwnerSettingsServiceChromeOS : public ownership::OwnerSettingsService,
   // Whether TPM token still needs to be initialized.
   bool waiting_for_tpm_token_;
 
-  // The device settings.  This may be different from the actual
-  // current device settings (which can be obtained from
-  // DeviceSettingsService) in case the device does not have an owner
-  // yet or there are pending changes that have not yet been written
-  // to session_manager.
-  enterprise_management::ChromeDeviceSettingsProto device_settings_;
+  // A set of pending changes to device settings.
+  base::ScopedPtrHashMap<std::string, base::Value> pending_changes_;
 
-  // True if some settings were changed but not stored.
-  bool has_pending_changes_;
+  // A protobuf containing pending changes to device settings.
+  scoped_ptr<enterprise_management::ChromeDeviceSettingsProto>
+      tentative_settings_;
 
   content::NotificationRegistrar registrar_;
 
diff --git a/components/ownership/owner_settings_service.h b/components/ownership/owner_settings_service.h
index a1d7c52..9be4a0a 100644
--- a/components/ownership/owner_settings_service.h
+++ b/components/ownership/owner_settings_service.h
@@ -41,8 +41,8 @@ class OWNERSHIP_EXPORT OwnerSettingsService : public KeyedService {
     // policy storage..
     virtual void OnSignedPolicyStored(bool success) {}
 
-    // Called when tentative changes were made to policy, but the policy still
-    // not signed and stored.
+    // Called when tentative changes were made to policy, but the
+    // policy is not signed and stored yet.
     //
     // TODO (ygorshenin@, crbug.com/230018): get rid of the method
     // since it creates DeviceSettingsService's dependency on