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..5a124b90852a86ac84f502d224a2ca91fca7fc7d 100644
--- a/chrome/browser/chromeos/user_cros_settings_provider.cc
+++ b/chrome/browser/chromeos/user_cros_settings_provider.cc
@@ -27,6 +27,7 @@
 #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 +59,80 @@ 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 - stored the settings in the policy blob
+// immediately if the current user is the owner. Uses the
+// SignedSettingsTempStorage if there is no owner yet. Or

[rkc, 2011/08/31 11:18:11]

Nit: Incomplete comment?

[pastarmovj, 2011/08/31 11:50:58]

Ooops true.

+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 value is not thread safe so take care if you call this function
+    // while MigrateValues is running. You can make sure this is not the case if
+    // You only call this function on the FILE thread.
+    migration_values_[path] = value;
+  }
+
+  void MigrateValues(void) {
+    // The check we do below should be done on the file thread so if we are not
+    // there jump on the right thread.
+    if (!BrowserThread::CurrentlyOn(BrowserThread::FILE)) {
+      BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
+                              base::Bind(&MigrationHelper::MigrateValues,
+                                         base::Unretained(this)));
+      return;
+    }
+
+    OwnershipService* service = OwnershipService::GetSharedInstance();
+    if (service->CurrentUserIsOwner() ||
+        service->GetStatus(true) != OwnershipService::OWNERSHIP_TAKEN) {
+      std::map<std::string, std::string>::const_iterator i =
+          migration_values_.begin();
+      for (; i != migration_values_.end(); ++i) {
+        // This is needed to avoid loosing the value after the clear below.
+        const std::string value = i->second;
+        // Queue all values for storing.
+        BrowserThread::PostTask(
+            BrowserThread::UI, FROM_HERE,
+            base::Bind(
+                &SignedSettingsHelper::StartStorePropertyOp,
+                base::Unretained(SignedSettingsHelper::Get()),
+                i->first.c_str(), value,

[rkc, 2011/08/31 11:18:11]

Why the difference between the passing of these two parameters? They seem to be
identical in the function declaration and the map template.

[pastarmovj, 2011/08/31 11:50:58]

Underneath they are not the same because the key(first) is stored as const
string and the value second as a non-const string. And the compiler is clever
enough to extend the lifetime of the const object due to the const ref argument
but the value is not and the clear below deletes it.

For the sake of clarity I decided to go for unifying them.

+                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) {
+      if (!migration_values_.empty())
+        MigrateValues();
+    }
+  }
+
+ private:
+  NotificationRegistrar registrar_;
+  std::map<std::string, std::string> migration_values_;
+  SignedSettingsHelper::Callback* callback_;
+
+  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 +349,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 +413,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 +548,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.