Implemented OwnerSettingsService::Set() method.

chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/ownership/owner_settings_service_chromeos.h"
 
 #include <string>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/command_line.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"
 #include "chromeos/tpm_token_loader.h"
 #include "components/ownership/owner_key_util.h"
 #include "components/policy/core/common/cloud/cloud_policy_constants.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_details.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_source.h"
@@ skipping 110 matching lines @@
 
 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::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)
     : ownership::OwnerSettingsService(owner_key_util),
       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(
             base::Bind(&OwnerSettingsServiceChromeOS::OnTPMTokenReady,
                        weak_factory_.GetWeakPtr()));
     waiting_for_tpm_token_ =
         tpm_token_status == TPMTokenLoader::TPM_TOKEN_STATUS_UNDETERMINED;
   }
 
   if (DBusThreadManager::IsInitialized() &&
       DBusThreadManager::Get()->GetSessionManagerClient()) {
     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_));
 }
 
 OwnerSettingsServiceChromeOS::~OwnerSettingsServiceChromeOS() {
   DCHECK(thread_checker_.CalledOnValidThread());
   if (DBusThreadManager::IsInitialized() &&
       DBusThreadManager::Get()->GetSessionManagerClient()) {
     DBusThreadManager::Get()->GetSessionManagerClient()->RemoveObserver(this);
   }
+
+  if (DeviceSettingsService::IsInitialized())
+    GetDeviceSettingsService()->RemoveObserver(this);
 }
 
 void OwnerSettingsServiceChromeOS::OnTPMTokenReady(
     bool /* tpm_token_enabled */) {
   DCHECK(thread_checker_.CalledOnValidThread());
   waiting_for_tpm_token_ = false;
 
   // TPMTokenLoader initializes the TPM and NSS database which is necessary to
   // determine ownership. Force a reload once we know these are initialized.
   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(
     int type,
     const content::NotificationSource& source,
     const content::NotificationDetails& details) {
   DCHECK(thread_checker_.CalledOnValidThread());
   if (type != chrome::NOTIFICATION_PROFILE_CREATED) {
     NOTREACHED();
     return;
   }
 
   Profile* profile = content::Source<Profile>(source).ptr();
   if (profile != profile_) {
     NOTREACHED();
     return;
   }
 
   waiting_for_profile_creation_ = false;
   ReloadKeypair();
 }
 
 void OwnerSettingsServiceChromeOS::OwnerKeySet(bool success) {
   DCHECK(thread_checker_.CalledOnValidThread());
   if (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,
     const scoped_refptr<OwnerKeyUtil>& owner_key_util,
     const IsOwnerCallback& callback) {
   CHECK(chromeos::LoginState::Get()->IsInSafeMode());
 
   // Make sure NSS is initialized and NSS DB is loaded for the user before
   // searching for the owner key.
   BrowserThread::PostTaskAndReply(
       BrowserThread::IO,
       FROM_HERE,
       base::Bind(base::IgnoreResult(&crypto::InitializeNSSForChromeOSUser),
                  user_hash,
                  ProfileHelper::GetProfilePathByUserIdHash(user_hash)),
       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() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   user_id_ = profile_->GetProfileName();
   const bool is_owner = IsOwner() || IsOwnerInTests(user_id_);
   if (is_owner && GetDeviceSettingsService())
     GetDeviceSettingsService()->InitOwner(user_id_, weak_factory_.GetWeakPtr());
 }
 
 void OwnerSettingsServiceChromeOS::ReloadKeypairImpl(const base::Callback<
     void(const scoped_refptr<PublicKey>& public_key,
          const scoped_refptr<PrivateKey>& private_key)>& callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   if (waiting_for_profile_creation_ || waiting_for_tpm_token_)
     return;
   scoped_refptr<base::TaskRunner> task_runner =
       BrowserThread::GetBlockingPool()->GetTaskRunnerWithShutdownBehavior(
           base::SequencedWorkerPool::SKIP_ON_SHUTDOWN);
   task_runner->PostTask(
       FROM_HERE,
       base::Bind(&LoadPrivateKey,
                  owner_key_util_,
                  ProfileHelper::GetUserIdHashFromProfile(profile_),
                  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()));
+}
 
-  pending_operations_.pop_front();
-  delete operation;
-  StartNextOperation();
+void OwnerSettingsServiceChromeOS::OnSignedPolicyStored() {
+  FOR_EACH_OBSERVER(Observer, observers_, OnSetCompleted(true));
+  ProcessNextSetRequestAsync();
+}
+
+void OwnerSettingsServiceChromeOS::HandleError() {
+  FOR_EACH_OBSERVER(Observer, observers_, OnSetCompleted(false));
+  ProcessNextSetRequestAsync();
 }
 
 }  // namespace chromeos