Implemented OwnerSettingsService::Set() method.

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 037df4ea963351ab9601afbabaf385dfaae5f594..b7b1e1cdec67a510b661acee55530d9df4029f34 100644
--- a/chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
+++ b/chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
@@ -10,11 +10,13 @@
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/command_line.h"
+#include "base/message_loop/message_loop.h"
 #include "base/prefs/pref_service.h"
 #include "base/threading/thread_checker.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
+#include "chrome/browser/chromeos/settings/device_settings_provider.h"
 #include "chrome/browser/chromeos/settings/session_manager_operation.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
@@ -43,8 +45,6 @@ namespace chromeos {
 
 namespace {
 
-DeviceSettingsService* g_device_settings_service_for_testing = NULL;
-
 bool IsOwnerInTests(const std::string& user_id) {
   if (user_id.empty() ||
       !CommandLine::ForCurrentProcess()->HasSwitch(::switches::kTestType) ||
@@ -136,23 +136,46 @@ void DoesPrivateKeyExistAsync(
       callback);
 }
 
-DeviceSettingsService* GetDeviceSettingsService() {
-  if (g_device_settings_service_for_testing)
-    return g_device_settings_service_for_testing;
-  return DeviceSettingsService::IsInitialized() ? DeviceSettingsService::Get()
-                                                : NULL;
+}  // namespace
+
+class OwnerSettingsServiceChromeOS::ProcessingLoopAutoBlocker {
+ public:
+  explicit ProcessingLoopAutoBlocker(
+      DeviceSettingsService* device_settings_service)
+      : device_settings_service_(device_settings_service) {
+    device_settings_service_->EnableProcessingLoop(false /* enabled */);
+  }
+
+  ~ProcessingLoopAutoBlocker() {
+    device_settings_service_->EnableProcessingLoop(true /* enabled */);
+  }
+
+ private:
+  DeviceSettingsService* device_settings_service_;
+
+  DISALLOW_COPY_AND_ASSIGN(ProcessingLoopAutoBlocker);
+};
+
+OwnerSettingsServiceChromeOS::SetRequest::SetRequest(
+    const std::string& setting,
+    linked_ptr<base::Value> value)
+    : setting(setting), value(value) {
 }
 
-}  // namespace
+OwnerSettingsServiceChromeOS::SetRequest::~SetRequest() {
+}
 
 OwnerSettingsServiceChromeOS::OwnerSettingsServiceChromeOS(
+    DeviceSettingsService* device_settings_service,
     Profile* profile,
     const scoped_refptr<OwnerKeyUtil>& owner_key_util)
     : ownership::OwnerSettingsService(owner_key_util),
+      device_settings_service_(device_settings_service),
       profile_(profile),
       waiting_for_profile_creation_(true),
       waiting_for_tpm_token_(true),
-      weak_factory_(this) {
+      weak_factory_(this),
+      set_requests_callback_factory_(this) {
   if (TPMTokenLoader::IsInitialized()) {
     TPMTokenLoader::TPMTokenStatus tpm_token_status =
         TPMTokenLoader::Get()->IsTPMTokenEnabled(
@@ -167,6 +190,8 @@ OwnerSettingsServiceChromeOS::OwnerSettingsServiceChromeOS(
     DBusThreadManager::Get()->GetSessionManagerClient()->AddObserver(this);
   }
 
+  device_settings_service_->AddObserver(this);
+
   registrar_.Add(this,
                  chrome::NOTIFICATION_PROFILE_CREATED,
                  content::Source<Profile>(profile_));
@@ -178,6 +203,8 @@ OwnerSettingsServiceChromeOS::~OwnerSettingsServiceChromeOS() {
       DBusThreadManager::Get()->GetSessionManagerClient()) {
     DBusThreadManager::Get()->GetSessionManagerClient()->RemoveObserver(this);
   }
+
+  device_settings_service_->RemoveObserver(this);
 }
 
 void OwnerSettingsServiceChromeOS::OnTPMTokenReady(
@@ -190,19 +217,17 @@ void OwnerSettingsServiceChromeOS::OnTPMTokenReady(
   ReloadKeypair();
 }
 
-void OwnerSettingsServiceChromeOS::SignAndStorePolicyAsync(
-    scoped_ptr<em::PolicyData> policy,
-    const base::Closure& callback) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  SignAndStoreSettingsOperation* operation = new SignAndStoreSettingsOperation(
-      base::Bind(&OwnerSettingsServiceChromeOS::HandleCompletedOperation,
-                 weak_factory_.GetWeakPtr(),
-                 callback),
-      policy.Pass());
-  operation->set_owner_settings_service(weak_factory_.GetWeakPtr());
-  pending_operations_.push_back(operation);
-  if (pending_operations_.front() == operation)
-    StartNextOperation();
+bool OwnerSettingsServiceChromeOS::HandlesSetting(const std::string& setting) {
+  return DeviceSettingsProvider::IsDeviceSetting(setting);
+}
+
+void OwnerSettingsServiceChromeOS::Set(const std::string& setting,
+                                       const base::Value& value) {
+  if (!IsOwner() && !IsOwnerInTests(user_id_))
+    return;
+  set_requests_.push(

[Mattias Nissler (ping if slow), 2014/10/20 12:41:30]

It just occurred to me that instead of keeping a queue of individual settings to
updated, we could just as well keep a ChromeDeviceSettingsProto that we write
all tentative device settings to. Then, when we get the chance to call
DeviceSettingsService::Store() we can send the updated device settings in one
go. WDYT?

This would also conveniently give us the right settings to send to the proposed
DeviceSettingsProvider call to update tentative settings.

[ygorshenin1, 2014/10/20 13:35:16]

But we always can call DeviceSettingsService::Store(), right? Or, you mean, that
if DeviceSettingsService is currently busy with load/store, we should wait until
it'll complete it's operations, then merge our device settings with
DeviceSettingsService::device_settings_, sign them and send for storage?

[Mattias Nissler (ping if slow), 2014/10/20 13:46:49]

DeviceSettingsService doesn't impose any restrictions on when we can call Store,
it'll happily queue up multiple Store operations for us. However, that'd make it
hard for OwnerSettingsServiceChromeOS to keep track of what to do in
DeviceSettingsUpdated(), as we'd have to wait for all Store() operations to
finish until we'd get a consistent picture again. And if we'd generate more
Store() operations using the current device settings published by
DeviceSettingsService while we still have Store()s in progress, we might
inadvertently roll back the effect of the pending Store()s because our fresh
Store() would pick up old values.

Hence, I think OwnerSettingsServiceChromeOS should have at most one pending
Store() operation at a given point in time. We can accumulate all changes that
come in via Set() in a local ChromeDeviceSettingsProto instance and fire off a
new Store() operation once the currently-pending Store() has finished. Makes
sense?

[ygorshenin1, 2014/10/20 13:49:06]

Sure, thanks for the explanation!

+      SetRequest(setting, linked_ptr<base::Value>(value.DeepCopy())));
+  ProcessNextSetRequest();
 }
 
 void OwnerSettingsServiceChromeOS::Observe(
@@ -231,6 +256,16 @@ void OwnerSettingsServiceChromeOS::OwnerKeySet(bool success) {
     ReloadKeypair();
 }
 
+void OwnerSettingsServiceChromeOS::OwnershipStatusChanged() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  ProcessNextSetRequestAsync();
+}
+
+void OwnerSettingsServiceChromeOS::DeviceSettingsUpdated() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  ProcessNextSetRequestAsync();
+}
+
 // static
 void OwnerSettingsServiceChromeOS::IsOwnerForSafeModeAsync(
     const std::string& user_hash,
@@ -249,11 +284,13 @@ void OwnerSettingsServiceChromeOS::IsOwnerForSafeModeAsync(
       base::Bind(&DoesPrivateKeyExistAsync, owner_key_util, callback));
 }
 
-// static
-void OwnerSettingsServiceChromeOS::SetDeviceSettingsServiceForTesting(
-    DeviceSettingsService* device_settings_service) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  g_device_settings_service_for_testing = device_settings_service;
+void OwnerSettingsServiceChromeOS::AddObserver(Observer* observer) {
+  if (observer)
+    observers_.AddObserver(observer);
+}
+
+void OwnerSettingsServiceChromeOS::RemoveObserver(Observer* observer) {
+  observers_.RemoveObserver(observer);
 }
 
 void OwnerSettingsServiceChromeOS::OnPostKeypairLoadedActions() {
@@ -261,8 +298,8 @@ void OwnerSettingsServiceChromeOS::OnPostKeypairLoadedActions() {
 
   user_id_ = profile_->GetProfileName();
   const bool is_owner = IsOwner() || IsOwnerInTests(user_id_);
-  if (is_owner && GetDeviceSettingsService())
-    GetDeviceSettingsService()->InitOwner(user_id_, weak_factory_.GetWeakPtr());
+  if (is_owner)
+    device_settings_service_->InitOwner(user_id_, weak_factory_.GetWeakPtr());
 }
 
 void OwnerSettingsServiceChromeOS::ReloadKeypairImpl(const base::Callback<
@@ -283,44 +320,68 @@ void OwnerSettingsServiceChromeOS::ReloadKeypairImpl(const base::Callback<
                  callback));
 }
 
-void OwnerSettingsServiceChromeOS::StartNextOperation() {
-  DeviceSettingsService* service = GetDeviceSettingsService();
-  if (!pending_operations_.empty() && service &&
-      service->session_manager_client()) {
-    pending_operations_.front()->Start(
-        service->session_manager_client(), owner_key_util_, public_key_);
-  }
+void OwnerSettingsServiceChromeOS::ProcessNextSetRequestAsync() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  base::MessageLoop::current()->PostTask(
+      FROM_HERE,
+      base::Bind(&OwnerSettingsServiceChromeOS::ProcessNextSetRequest,
+                 weak_factory_.GetWeakPtr()));
 }
 
-void OwnerSettingsServiceChromeOS::HandleCompletedOperation(
-    const base::Closure& callback,
-    SessionManagerOperation* operation,
-    DeviceSettingsService::Status status) {
-  DCHECK_EQ(operation, pending_operations_.front());
-
-  DeviceSettingsService* service = GetDeviceSettingsService();
-  if (status == DeviceSettingsService::STORE_SUCCESS) {
-    service->set_policy_data(operation->policy_data().Pass());
-    service->set_device_settings(operation->device_settings().Pass());
+void OwnerSettingsServiceChromeOS::ProcessNextSetRequest() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (set_requests_.empty() || set_requests_callback_factory_.HasWeakPtrs() ||
+      device_settings_service_->HasPendingOperations()) {
+    return;
   }
 
-  if ((operation->public_key().get() && !public_key_.get()) ||
-      (operation->public_key().get() && public_key_.get() &&
-       operation->public_key()->data() != public_key_->data())) {
-    // Public part changed so we need to reload private part too.
-    ReloadKeypair();
-    content::NotificationService::current()->Notify(
-        chrome::NOTIFICATION_OWNERSHIP_STATUS_CHANGED,
-        content::Source<OwnerSettingsServiceChromeOS>(this),
-        content::NotificationService::NoDetails());
+  scoped_ptr<ProcessingLoopAutoBlocker> lock(
+      new ProcessingLoopAutoBlocker(device_settings_service_));
+
+  // TODO (ygorshenin@): notify somehow DeviceSettingsProvider about tentative
+  // change.
+  // crbug.com/230018

[Mattias Nissler (ping if slow), 2014/10/20 12:41:30]

Shouldn't this already happen in Set()?

[ygorshenin1, 2014/10/22 09:20:11]

Done.

+  SetRequest request = set_requests_.front();
+  set_requests_.pop();
+
+  em::ChromeDeviceSettingsProto new_settings(
+      *device_settings_service_->device_settings());
+  DeviceSettingsService::UpdateDeviceSettings(request.setting, *request.value,
+                                              new_settings);
+  scoped_ptr<em::PolicyData> new_policy = DeviceSettingsService::AssemblePolicy(
+      user_id_, device_settings_service_->policy_data(), &new_settings);
+  bool rv = AssembleAndSignPolicyAsync(
+      content::BrowserThread::GetBlockingPool(),
+      new_policy.Pass(),
+      base::Bind(&OwnerSettingsServiceChromeOS::OnPolicyAssembledAndSigned,
+                 set_requests_callback_factory_.GetWeakPtr(),
+                 Passed(&lock)));
+  if (!rv)
+    HandleError();
+}
+
+void OwnerSettingsServiceChromeOS::OnPolicyAssembledAndSigned(
+    scoped_ptr<ProcessingLoopAutoBlocker> blocker,
+    scoped_ptr<em::PolicyFetchResponse> policy_response) {
+  blocker.reset();
+  if (!policy_response.get()) {
+    HandleError();
+    return;
   }
-  service->OnSignAndStoreOperationCompleted(status);
-  if (!callback.is_null())
-    callback.Run();
+  device_settings_service_->Store(
+      policy_response.Pass(),
+      base::Bind(&OwnerSettingsServiceChromeOS::OnSignedPolicyStored,
+                 weak_factory_.GetWeakPtr()));
+}
+
+void OwnerSettingsServiceChromeOS::OnSignedPolicyStored() {
+  FOR_EACH_OBSERVER(Observer, observers_, OnSetCompleted(true));
+  ProcessNextSetRequestAsync();
+}
 
-  pending_operations_.pop_front();
-  delete operation;
-  StartNextOperation();
+void OwnerSettingsServiceChromeOS::HandleError() {
+  FOR_EACH_OBSERVER(Observer, observers_, OnSetCompleted(false));
+  ProcessNextSetRequestAsync();
 }
 
 }  // namespace chromeos