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

This solved the issue that the UI does not reflect the OOBE screen setting
on new machines. There will be now a window of 30secs where this value will
still be wrong but this is still better than not migrating correctly at all.

BUG=chromium-os:19427, 19942
TEST=Check the metrics reporting on the EULA screen and see if the settings UI is checked too.


Review URL: http://codereview.chromium.org/7741045

TBR=pastarmovj@chromium.org
Review URL: http://codereview.chromium.org/7830020

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@99242 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 39 insertions, 91 deletions

diff --git a/chrome/browser/chromeos/login/existing_user_controller.cc b/chrome/browser/chromeos/login/existing_user_controller.cc
index 63e56ce..c06b1a0 100644
--- a/chrome/browser/chromeos/login/existing_user_controller.cc
+++ b/chrome/browser/chromeos/login/existing_user_controller.cc
@@ -250,8 +250,7 @@ void ExistingUserController::OnStartEnterpriseEnrollment() {
 }
 
 void ExistingUserController::OnEnrollmentOwnershipCheckCompleted(
-    OwnershipService::Status status,
-    bool current_user_is_owner) {
+    OwnershipService::Status status) {
   if (status == OwnershipService::OWNERSHIP_NONE) {
     host_->StartWizard(WizardController::kEnterpriseEnrollmentScreenName,
                        GURL());
diff --git a/chrome/browser/chromeos/login/existing_user_controller.h b/chrome/browser/chromeos/login/existing_user_controller.h
index e857456..342d53e 100644
--- a/chrome/browser/chromeos/login/existing_user_controller.h
+++ b/chrome/browser/chromeos/login/existing_user_controller.h
@@ -136,8 +136,7 @@ class ExistingUserController : public LoginDisplay::Delegate,
 
   // Handles result of ownership check and starts enterprise enrollment if
   // applicable.
-  void OnEnrollmentOwnershipCheckCompleted(OwnershipService::Status status,
-                                           bool current_user_is_owner);
+  void OnEnrollmentOwnershipCheckCompleted(OwnershipService::Status status);
 
   void set_login_performer_delegate(LoginPerformer::Delegate* d) {
     login_performer_delegate_.reset(d);
diff --git a/chrome/browser/chromeos/login/ownership_status_checker.cc b/chrome/browser/chromeos/login/ownership_status_checker.cc
index 7e7a7e1..f7d67ab 100644
--- a/chrome/browser/chromeos/login/ownership_status_checker.cc
+++ b/chrome/browser/chromeos/login/ownership_status_checker.cc
@@ -28,16 +28,13 @@ void OwnershipStatusChecker::Core::Check() {
   DCHECK(origin_loop_->BelongsToCurrentThread());
   OwnershipService::Status status =
       OwnershipService::GetSharedInstance()->GetStatus(false);
-  // We can only report the OWNERSHIP_NONE status without executing code on the
-  // file thread because checking whether the current user is owner needs file
-  // access.
-  if (status == OwnershipService::OWNERSHIP_NONE) {
+  if (status != OwnershipService::OWNERSHIP_UNKNOWN) {
     // Take a spin on the message loop in order to avoid reentrancy in callers.
     origin_loop_->PostTask(
         FROM_HERE,
         NewRunnableMethod(this,
                           &OwnershipStatusChecker::Core::ReportResult,
-                          status, false));
+                          status));
   } else {
     // Switch to the file thread to make the blocking call.
     BrowserThread::PostTask(
@@ -56,20 +53,18 @@ void OwnershipStatusChecker::Core::CheckOnFileThread() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   OwnershipService::Status status =
       OwnershipService::GetSharedInstance()->GetStatus(true);
-  bool current_user_is_owner =
-      OwnershipService::GetSharedInstance()->CurrentUserIsOwner();
   origin_loop_->PostTask(
       FROM_HERE,
       NewRunnableMethod(this,
                         &OwnershipStatusChecker::Core::ReportResult,
-                        status, current_user_is_owner));
+                        status));
 }
 
 void OwnershipStatusChecker::Core::ReportResult(
-    OwnershipService::Status status, bool current_user_is_owner) {
+    OwnershipService::Status status) {
   DCHECK(origin_loop_->BelongsToCurrentThread());
   if (callback_.get()) {
-    callback_->Run(status, current_user_is_owner);
+    callback_->Run(status);
     callback_.reset();
   }
 }
diff --git a/chrome/browser/chromeos/login/ownership_status_checker.h b/chrome/browser/chromeos/login/ownership_status_checker.h
index d94bdef..fb726da 100644
--- a/chrome/browser/chromeos/login/ownership_status_checker.h
+++ b/chrome/browser/chromeos/login/ownership_status_checker.h
@@ -23,9 +23,8 @@ namespace chromeos {
 class OwnershipStatusChecker {
  public:
   // Callback function type. The status code is guaranteed to be different from
-  // OWNERSHIP_UNKNOWN. The bool parameter is true iff the current logged in
-  // user is the owner.
-  typedef Callback2<OwnershipService::Status, bool>::Type Callback;
+  // OWNERSHIP_UNKNOWN.
+  typedef Callback1<OwnershipService::Status>::Type Callback;
 
   explicit OwnershipStatusChecker(Callback* callback);
   ~OwnershipStatusChecker();
@@ -45,8 +44,7 @@ class OwnershipStatusChecker {
 
    private:
     void CheckOnFileThread();
-    void ReportResult(OwnershipService::Status status,
-                      bool current_user_is_owner);
+    void ReportResult(OwnershipService::Status status);
 
     scoped_ptr<Callback> callback_;
     scoped_refptr<base::MessageLoopProxy> origin_loop_;
diff --git a/chrome/browser/chromeos/login/signed_settings.cc b/chrome/browser/chromeos/login/signed_settings.cc
index 3e25a9c..9bc98b8 100644
--- a/chrome/browser/chromeos/login/signed_settings.cc
+++ b/chrome/browser/chromeos/login/signed_settings.cc
@@ -772,9 +772,12 @@ std::string RetrievePropertyOp::LookUpInPolicy(const std::string& prop) {
     return pol.release_channel().release_channel();
 
   } else if (prop == kStatsReportingPref) {
-    if (pol.has_metrics_enabled()) {
-      return kVeritas[pol.metrics_enabled().metrics_enabled()];
+    if (!pol.has_metrics_enabled() ||
+        !pol.metrics_enabled().metrics_enabled()) {
+      return kVeritas[0];  // Default to not collecting metrics.
     }
+    return kVeritas[pol.metrics_enabled().metrics_enabled()];
+
   }
   return std::string();
 }
diff --git a/chrome/browser/chromeos/user_cros_settings_provider.cc b/chrome/browser/chromeos/user_cros_settings_provider.cc
index 56f3f84..c50e4298 100644
--- a/chrome/browser/chromeos/user_cros_settings_provider.cc
+++ b/chrome/browser/chromeos/user_cros_settings_provider.cc
@@ -7,6 +7,8 @@
 #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"
@@ -20,13 +22,11 @@
 #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,69 +58,21 @@ const char* kListSettings[] = {
   kAccountsPrefUsers
 };
 
-// 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;
+// 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));
   }
-
-  void MigrateValues(void) {
-    ownership_checker_.reset(new OwnershipStatusChecker(NewCallback(
-        this, &MigrationHelper::DoMigrateValues)));
-  }
-
-  // NotificationObserver overrides:
-  virtual void Observe(int type,
-                       const NotificationSource& source,
-                       const NotificationDetails& details) OVERRIDE {
-    if (type == chrome::NOTIFICATION_OWNERSHIP_CHECKED)
-      MigrateValues();
-  }
-
- private:
-  void DoMigrateValues(OwnershipService::Status status,
-                       bool current_user_is_owner) {
-    ownership_checker_.reset(NULL);
-
-    // We can call StartStorePropertyOp in two cases - either if the owner is
-    // currently logged in and the policy can be updated immediately or if there
-    // is no owner yet in which case the value will be temporarily stored in the
-    // SignedSettingsTempStorage until the device is owned. If none of these
-    // cases is met then we will wait for user change notification and retry.
-    if (current_user_is_owner || status != OwnershipService::OWNERSHIP_TAKEN) {
-      std::map<std::string, std::string>::const_iterator i;
-      for (i = migration_values_.begin(); i != migration_values_.end(); ++i) {
-        // Queue all values for storing.
-        SignedSettingsHelper::Get()->StartStorePropertyOp(i->first, i->second,
-                                                          callback_);
-      }
-      migration_values_.clear();
-    }
-  }
-
-  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
@@ -337,7 +289,6 @@ 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();
   }
@@ -401,10 +352,14 @@ class UserCrosSettingsTrust : public SignedSettingsHelper::Callback {
         // Temporarily allow it until we fix http://crbug.com/62626
         base::ThreadRestrictions::ScopedAllowIO allow_io;
         stats_consent = GoogleUpdateSettings::GetCollectStatsConsent();
-        // Make sure the values will get eventually written to the policy file.
-        migration_helper_.AddMigrationValue(
-            path, stats_consent ? "true" : "false");
-        migration_helper_.MigrateValues();
+        // 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));
         UpdateCacheBool(path, stats_consent, USE_VALUE_SUPPLIED);
         LOG(WARNING) << "No metrics policy set will revert to checking "
                      << "consent file which is "
@@ -536,7 +491,6 @@ 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.