Delay the metrics policy migration call to make sure ownership has been taken.

chrome/browser/chromeos/user_cros_settings_provider.cc

Index: chrome/browser/chromeos/user_cros_settings_provider.cc
diff --git a/chrome/browser/chromeos/user_cros_settings_provider.cc b/chrome/browser/chromeos/user_cros_settings_provider.cc
index c50e429895399c73c5e8519a08951ff99de3f5b3..557d8d22df404f8bbd56bdb0b1368e336c1c3372 100644
--- a/chrome/browser/chromeos/user_cros_settings_provider.cc
+++ b/chrome/browser/chromeos/user_cros_settings_provider.cc
@@ -7,8 +7,6 @@
 #include <map>
 #include <set>
 
-#include "base/bind.h"
-#include "base/callback.h"
 #include "base/hash_tables.h"
 #include "base/logging.h"
 #include "base/memory/singleton.h"
@@ -22,11 +20,13 @@
 #include "chrome/browser/chromeos/cros_settings.h"
 #include "chrome/browser/chromeos/cros_settings_names.h"
 #include "chrome/browser/chromeos/login/ownership_service.h"
+#include "chrome/browser/chromeos/login/ownership_status_checker.h"
 #include "chrome/browser/chromeos/login/user_manager.h"
 #include "chrome/browser/policy/browser_policy_connector.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/prefs/scoped_user_pref_update.h"
 #include "chrome/browser/ui/options/options_util.h"
+#include "chrome/common/chrome_notification_types.h"
 #include "chrome/installer/util/google_update_settings.h"
 #include "content/browser/browser_thread.h"
 
@@ -58,21 +58,72 @@ const char* kListSettings[] = {
   kAccountsPrefUsers
 };
 
-// Only write the property if the owner is the current logged on user.
-void StartStorePropertyOpIfOwner(const std::string& name,
-                                 const std::string& value,
-                                 SignedSettingsHelper::Callback* callback) {
-  if (OwnershipService::GetSharedInstance()->CurrentUserIsOwner()) {
-    BrowserThread::PostTask(BrowserThread::UI,
-                            FROM_HERE,
-                            base::Bind(
-                                &SignedSettingsHelper::StartStorePropertyOp,
-                                base::Unretained(SignedSettingsHelper::Get()),
-                                name,
-                                value,
-                                callback));
+// This class provides the means to migrate settings to the signed settings
+// store. It does one of three things - store the settings in the policy blob
+// immediately if the current user is the owner. Uses the
+// SignedSettingsTempStorage if there is no owner yet, or waits for an
+// OWNERSHIP_CHECKED notification to delay the storing until the owner has
+// logged in.
+class MigrationHelper : public NotificationObserver {
+ public:
+  explicit MigrationHelper() : callback_(NULL) {
+    registrar_.Add(this, chrome::NOTIFICATION_OWNERSHIP_CHECKED,
+                   NotificationService::AllSources());
   }
-}
+
+  void set_callback(SignedSettingsHelper::Callback* callback) {
+    callback_ = callback;
+  }
+
+  void AddMigrationValue(const std::string& path, const std::string& value) {
+    migration_values_[path] = value;
+  }
+
+  void MigrateValues(void) {
+    ownership_checker_.reset(new OwnershipStatusChecker(NewCallback(
+        this, &MigrationHelper::DoMigrateValues)));
+  }
+
+  void DoMigrateValues(OwnershipService::Status status,

[Mattias Nissler (ping if slow), 2011/08/31 15:08:33]

make this private

[pastarmovj, 2011/08/31 15:26:36]

Done.

+                       bool current_user_is_owner) {
+    ownership_checker_.reset(NULL);
+
+    if (current_user_is_owner || status != OwnershipService::OWNERSHIP_TAKEN) {

[Mattias Nissler (ping if slow), 2011/08/31 15:08:33]

Put a comment why we do this if ownership hasn't been taken yet (maybe move the
comment below in the empty else block up here)?

[pastarmovj, 2011/08/31 15:26:36]

Done.

+      std::map<std::string, std::string>::const_iterator i;
+      for (i = migration_values_.begin(); i != migration_values_.end(); ++i) {
+        // This is needed to avoid loosing the value after the clear below.

[Mattias Nissler (ping if slow), 2011/08/31 15:08:33]

Nope, this is not needed. A copy is created when constructing the
StorePropertyOpContext.

[pastarmovj, 2011/08/31 15:26:36]

Before I did need it because I was passing the result of a c_str instead of the
object and it was destroyed but not done anymore so removed. (Dunno why I
thought I need to pass pure char* instead of a string object).

On 2011/08/31 15:08:33, Mattias Nissler wrote:

+        // Temp objects passed by const ref will have the life time of the ref.
+        const std::string name = i->first;
+        const std::string value = i->second;
+        // Queue all values for storing.
+        SignedSettingsHelper::Get()->StartStorePropertyOp(name, value,
+                                                          callback_);
+      }
+      migration_values_.clear();
+    } else {
+      // Either we are not yet logged in or the user currently logged in is not
+      // the owner. So we should wait for user change. (Actually only the first
+      // case is interesting for us.)
+    }
+  }
+
+  // NotificationObserver overrides:
+  virtual void Observe(int type,
+                       const NotificationSource& source,
+                       const NotificationDetails& details) OVERRIDE {
+    if (type == chrome::NOTIFICATION_OWNERSHIP_CHECKED)
+      MigrateValues();
+  }
+
+ private:
+  NotificationRegistrar registrar_;
+  scoped_ptr<OwnershipStatusChecker> ownership_checker_;
+  SignedSettingsHelper::Callback* callback_;
+
+  std::map<std::string, std::string> migration_values_;
+
+  DISALLOW_COPY_AND_ASSIGN(MigrationHelper);
+};
 
 bool IsControlledBooleanSetting(const std::string& pref_path) {
   // TODO(nkostylev): Using std::find for 4 value array generates this warning
@@ -289,6 +340,7 @@ class UserCrosSettingsTrust : public SignedSettingsHelper::Callback {
   UserCrosSettingsTrust()
       : ownership_service_(OwnershipService::GetSharedInstance()),
         retries_left_(kNumRetriesLimit) {
+    migration_helper_.set_callback(this);
     // Start prefetching Boolean and String preferences.
     Reload();
   }
@@ -352,14 +404,10 @@ class UserCrosSettingsTrust : public SignedSettingsHelper::Callback {
         // Temporarily allow it until we fix http://crbug.com/62626
         base::ThreadRestrictions::ScopedAllowIO allow_io;
         stats_consent = GoogleUpdateSettings::GetCollectStatsConsent();
-        // Only store settings if the owner is logged on, otherwise the write
-        // will fail, triggering another read and we'll end up in an infinite
-        // loop. Owner check needs to be done on the FILE thread.
-        BrowserThread::PostTask(BrowserThread::FILE,
-                                FROM_HERE,
-                                base::Bind(&StartStorePropertyOpIfOwner, path,
-                                           stats_consent ? "true" : "false",
-                                           this));
+        // Make sure the values will get eventually written to the policy file.
+        migration_helper_.AddMigrationValue(
+            path, stats_consent ? "true" : "false");
+        migration_helper_.MigrateValues();
         UpdateCacheBool(path, stats_consent, USE_VALUE_SUPPLIED);
         LOG(WARNING) << "No metrics policy set will revert to checking "
                      << "consent file which is "
@@ -491,6 +539,7 @@ class UserCrosSettingsTrust : public SignedSettingsHelper::Callback {
   base::hash_map< std::string, std::vector< Task* > > callbacks_;
 
   OwnershipService* ownership_service_;
+  MigrationHelper migration_helper_;
 
   // In order to guard against occasional failure to fetch a property
   // we allow for some number of retries.