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..3039845018c6cbbcf683c04b0f9693fc0c569ef9 100644
--- a/chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
+++ b/chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
@@ -15,6 +15,7 @@
 #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"
@@ -145,6 +146,22 @@ DeviceSettingsService* GetDeviceSettingsService() {
 
 }  // namespace
 
+class OwnerSettingsServiceChromeOS::DeviceSettingsServiceLock {

[Mattias Nissler (ping if slow), 2014/10/17 12:05:26]

Calling this a lock is bit misleading, nothing gets blocked actually.

[ygorshenin1, 2014/10/20 11:36:10]

Renamed to ProcessingLoopAutoBlocker. Not sure that this is a good name.

+ public:
+  DeviceSettingsServiceLock() {
+    if (GetDeviceSettingsService())
+      GetDeviceSettingsService()->EnableProcessingLoop(false /* enabled */);
+  }
+
+  ~DeviceSettingsServiceLock() {
+    if (GetDeviceSettingsService())
+      GetDeviceSettingsService()->EnableProcessingLoop(true /* enabled */);
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(DeviceSettingsServiceLock);
+};
+
 OwnerSettingsServiceChromeOS::OwnerSettingsServiceChromeOS(
     Profile* profile,
     const scoped_refptr<OwnerKeyUtil>& owner_key_util)
@@ -152,7 +169,8 @@ OwnerSettingsServiceChromeOS::OwnerSettingsServiceChromeOS(
       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 +185,9 @@ OwnerSettingsServiceChromeOS::OwnerSettingsServiceChromeOS(
     DBusThreadManager::Get()->GetSessionManagerClient()->AddObserver(this);
   }
 
+  if (DeviceSettingsService::IsInitialized())

[Mattias Nissler (ping if slow), 2014/10/17 12:05:27]

I assume this is just for testing? A better way might be to pass in the
DeviceSettingsService pointer via the constructor, so you can pass mocks for
tests.

[ygorshenin1, 2014/10/20 11:36:10]

Done.

+    GetDeviceSettingsService()->AddObserver(this);
+
   registrar_.Add(this,
                  chrome::NOTIFICATION_PROFILE_CREATED,
                  content::Source<Profile>(profile_));
@@ -178,6 +199,9 @@ OwnerSettingsServiceChromeOS::~OwnerSettingsServiceChromeOS() {
       DBusThreadManager::Get()->GetSessionManagerClient()) {
     DBusThreadManager::Get()->GetSessionManagerClient()->RemoveObserver(this);
   }
+
+  if (DeviceSettingsService::IsInitialized())
+    GetDeviceSettingsService()->RemoveObserver(this);
 }
 
 void OwnerSettingsServiceChromeOS::OnTPMTokenReady(
@@ -190,19 +214,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(
+      SetRequest(setting, linked_ptr<base::Value>(value.DeepCopy())));
+  ProcessNextSetRequest();
 }
 
 void OwnerSettingsServiceChromeOS::Observe(
@@ -231,6 +253,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,
@@ -256,6 +288,15 @@ void OwnerSettingsServiceChromeOS::SetDeviceSettingsServiceForTesting(
   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() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
@@ -283,44 +324,72 @@ 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());
+  BrowserThread::PostTask(

[Mattias Nissler (ping if slow), 2014/10/17 12:05:27]

If you're on the right thread anyways, you can just use
MessageLoop::current()->PostTask(). Is there a good reason to not just call this
directly?

[ygorshenin1, 2014/10/20 11:36:10]

The reason is that the method is called when DeviceSettingsService notifies
their observers about changes in settings or ownership status while
DeviceSettingsService::weak_factory_ still have active weak pointers. So direct
call to ProcessNextSetRequest() will have no effect because
DeviceSettingsService::HasPendingOperations() will return true.

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

Is there a good reason DeviceSettingsService is implemented that way? Sounds
like clearing the weak pointers earlier might be a better fix that doesn't
require quirks in consuming code.

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

Done.

+      BrowserThread::UI,
+      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())
+    return;
+  if (set_requests_callback_factory_.HasWeakPtrs())

[Mattias Nissler (ping if slow), 2014/10/17 12:05:27]

Could just unify the conditionals in lines 338, 340, 342-343 into a single if
statement.

[ygorshenin1, 2014/10/20 11:36:10]

Done.

+    return;
+  if (!GetDeviceSettingsService() ||
+      GetDeviceSettingsService()->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<DeviceSettingsServiceLock> lock(new DeviceSettingsServiceLock());
+
+  // TODO (ygorshenin@): notify somehow DeviceSettingsProvider about tentative
+  // change.
+  // crbug.com/230018

[Mattias Nissler (ping if slow), 2014/10/17 12:05:27]

Have you tested how the device behaves (in particular, owner settings in the
settings page) with just the changes you have in this CL? I'm curious :)

[ygorshenin1, 2014/10/20 11:36:10]

Yes, I've tested the CL with small additional change -
CoreChromeOSOptionsHandler::SetPref() used OwnerSettingsServiceChromeOS instead
of CrosSettings - and didn't found any regressions. But I'm going to make the
latter change in an individual CL.

+  std::string setting = set_requests_.front().first;
+  linked_ptr<base::Value> value = set_requests_.front().second;
+  set_requests_.pop();
+
+  em::ChromeDeviceSettingsProto new_settings(
+      *GetDeviceSettingsService()->device_settings());
+  DeviceSettingsService::UpdateDeviceSettings(setting, *value, new_settings);

[Mattias Nissler (ping if slow), 2014/10/17 12:05:26]

Is there a good reason to have the UpdateDeviceSettings and AssemblePolicy
functions in DeviceSettingsService and not here in OwnerSettingsServiceChromeOS?

[ygorshenin1, 2014/10/20 11:36:10]

The reason is to not introduce dependency on
chrome/browser/chromeos/ownership/*. If it's not a big issue to introduce such
dependencies in chrome/browser/chromeos/settings/*, I'll move AssemblePolicy and
UpdateDeviceSettings to OwnerSettingsServiceChromeOS.

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

What dependencies are we talking about? IMHO, the two functions logically belong
in OwnerSettingsServiceChromeOS, and I'd be willing to accept additional
dependencies for that.

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

Done.

+  scoped_ptr<em::PolicyData> new_policy = DeviceSettingsService::AssemblePolicy(
+      user_id_, GetDeviceSettingsService()->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<DeviceSettingsServiceLock> lock,
+    scoped_ptr<em::PolicyFetchResponse> policy_response) {
+  lock.reset();
+  if (!policy_response.get()) {
+    HandleError();
+    return;
   }
-  service->OnSignAndStoreOperationCompleted(status);
-  if (!callback.is_null())
-    callback.Run();
+  GetDeviceSettingsService()->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