Break up UserPolicyTokenCache in order to make the disk IO bits reusable.

BUG=none
TEST=unit tests


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

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

9 files changed, 114 insertions, 78 deletions

diff --git a/chrome/browser/policy/browser_policy_connector.cc b/chrome/browser/policy/browser_policy_connector.cc
index a672e7a..6449376 100644
--- a/chrome/browser/policy/browser_policy_connector.cc
+++ b/chrome/browser/policy/browser_policy_connector.cc
@@ -114,8 +114,8 @@ BrowserPolicyConnector::~BrowserPolicyConnector() {
   if (user_cloud_policy_subsystem_.get())
     user_cloud_policy_subsystem_->Shutdown();
   user_cloud_policy_subsystem_.reset();
+  user_policy_token_cache_.reset();
   user_data_store_.reset();
-  user_policy_token_cache_ = NULL;
 }
 
 ConfigurationPolicyProvider*
@@ -239,8 +239,8 @@ void BrowserPolicyConnector::InitializeUserPolicy(const std::string& user_name,
 
   // Throw away the old backend.
   user_cloud_policy_subsystem_.reset();
+  user_policy_token_cache_.reset();
   user_data_store_.reset();
-  user_policy_token_cache_ = NULL;
   registrar_.RemoveAll();
 
   CommandLine* command_line = CommandLine::ForCurrentProcess();
@@ -254,9 +254,9 @@ void BrowserPolicyConnector::InitializeUserPolicy(const std::string& user_name,
     UserPolicyCache* user_policy_cache =
         new UserPolicyCache(policy_cache_dir.Append(kPolicyCacheFile));
     user_data_store_.reset(CloudPolicyDataStore::CreateForUserPolicies());
-    user_policy_token_cache_ = new UserPolicyTokenCache(
-        user_data_store_->GetWeakPtr(),
-        policy_cache_dir.Append(kTokenCacheFile));
+    user_policy_token_cache_.reset(
+        new UserPolicyTokenCache(user_data_store_.get(),
+                                 policy_cache_dir.Append(kTokenCacheFile)));
 
     // Prepending user caches meaning they will take precedence of device policy
     // caches.
diff --git a/chrome/browser/policy/browser_policy_connector.h b/chrome/browser/policy/browser_policy_connector.h
index edf434e..780a1c8 100644
--- a/chrome/browser/policy/browser_policy_connector.h
+++ b/chrome/browser/policy/browser_policy_connector.h
@@ -137,7 +137,7 @@ class BrowserPolicyConnector : public NotificationObserver {
   scoped_ptr<EnterpriseInstallAttributes> install_attributes_;
 #endif
 
-  scoped_refptr<UserPolicyTokenCache> user_policy_token_cache_;
+  scoped_ptr<UserPolicyTokenCache> user_policy_token_cache_;
   scoped_ptr<CloudPolicyDataStore> user_data_store_;
   scoped_ptr<CloudPolicySubsystem> user_cloud_policy_subsystem_;
 
diff --git a/chrome/browser/policy/cloud_policy_controller.cc b/chrome/browser/policy/cloud_policy_controller.cc
index 2e7de65..24fbbd84 100644
--- a/chrome/browser/policy/cloud_policy_controller.cc
+++ b/chrome/browser/policy/cloud_policy_controller.cc
@@ -183,10 +183,6 @@ void CloudPolicyController::OnCredentialsChanged() {
   }
 }
 
-void CloudPolicyController::OnDataStoreGoingAway() {
-  NOTREACHED();
-}
-
 CloudPolicyController::CloudPolicyController(
     DeviceManagementService* service,
     CloudPolicyCacheBase* cache,
diff --git a/chrome/browser/policy/cloud_policy_controller.h b/chrome/browser/policy/cloud_policy_controller.h
index 9aa25cb..04e5e29 100644
--- a/chrome/browser/policy/cloud_policy_controller.h
+++ b/chrome/browser/policy/cloud_policy_controller.h
@@ -53,7 +53,6 @@ class CloudPolicyController
   // CloudPolicyDataStore::Observer implementation:
   virtual void OnDeviceTokenChanged() OVERRIDE;
   virtual void OnCredentialsChanged() OVERRIDE;
-  virtual void OnDataStoreGoingAway() OVERRIDE;
 
  private:
   // Indicates the current state the controller is in.
diff --git a/chrome/browser/policy/cloud_policy_data_store.cc b/chrome/browser/policy/cloud_policy_data_store.cc
index b3d7a84b..afbcf10 100644
--- a/chrome/browser/policy/cloud_policy_data_store.cc
+++ b/chrome/browser/policy/cloud_policy_data_store.cc
@@ -22,6 +22,8 @@ const char kMachineInfoSerialNumber[] = "serial_number";
 
 namespace policy {
 
+CloudPolicyDataStore::~CloudPolicyDataStore() {}
+
 // static
 CloudPolicyDataStore* CloudPolicyDataStore::CreateForUserPolicies() {
   return new CloudPolicyDataStore(em::DeviceRegisterRequest::USER,
@@ -60,16 +62,7 @@ CloudPolicyDataStore::CloudPolicyDataStore(
       policy_type_(policy_type),
       machine_model_(machine_model),
       machine_id_(machine_id),
-      token_cache_loaded_(false),
-      ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {}
-
-CloudPolicyDataStore::~CloudPolicyDataStore() {
-  FOR_EACH_OBSERVER(Observer, observer_list_, OnDataStoreGoingAway());
-}
-
-base::WeakPtr<CloudPolicyDataStore> CloudPolicyDataStore::GetWeakPtr() {
-  return weak_ptr_factory_.GetWeakPtr();
-}
+      token_cache_loaded_(false) {}
 
 void CloudPolicyDataStore::SetDeviceToken(const std::string& device_token,
                                           bool from_cache) {
diff --git a/chrome/browser/policy/cloud_policy_data_store.h b/chrome/browser/policy/cloud_policy_data_store.h
index 74327cb..0fed36d 100644
--- a/chrome/browser/policy/cloud_policy_data_store.h
+++ b/chrome/browser/policy/cloud_policy_data_store.h
@@ -8,7 +8,6 @@
 
 #include <string>
 
-#include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
 #include "chrome/browser/policy/proto/device_management_backend.pb.h"
 
@@ -33,11 +32,10 @@ class CloudPolicyDataStore {
     // Authentication credentials for talking to the device management service
     // (gaia_token_) changed.
     virtual void OnCredentialsChanged() = 0;
-
-    // Called from the destructor of CloudPolicyData.
-    virtual void OnDataStoreGoingAway() = 0;
   };
 
+  ~CloudPolicyDataStore();
+
   // Create CloudPolicyData with constants initialized for fetching user
   // policies.
   static CloudPolicyDataStore* CreateForUserPolicies();
@@ -46,10 +44,6 @@ class CloudPolicyDataStore {
   // policies.
   static CloudPolicyDataStore* CreateForDevicePolicies();
 
-  virtual ~CloudPolicyDataStore();
-
-  base::WeakPtr<CloudPolicyDataStore> GetWeakPtr();
-
   // Sets the device token, and token_policy_cache_loaded and sends out
   // notifications. Also ensures that setting the token should first happen
   // from the cache.
@@ -116,8 +110,6 @@ class CloudPolicyDataStore {
 
   ObserverList<Observer, true> observer_list_;
 
-  base::WeakPtrFactory<CloudPolicyDataStore> weak_ptr_factory_;
-
   DISALLOW_COPY_AND_ASSIGN(CloudPolicyDataStore);
 };
 
diff --git a/chrome/browser/policy/device_token_fetcher_unittest.cc b/chrome/browser/policy/device_token_fetcher_unittest.cc
index 04b23ca..3b7decb 100644
--- a/chrome/browser/policy/device_token_fetcher_unittest.cc
+++ b/chrome/browser/policy/device_token_fetcher_unittest.cc
@@ -20,8 +20,8 @@ namespace policy {
 
 const char kTestToken[] = "device_token_fetcher_test_auth_token";
 
-using testing::_;
 using testing::Mock;
+using testing::_;
 
 class MockTokenAvailableObserver : public CloudPolicyDataStore::Observer {
  public:
@@ -30,7 +30,6 @@ class MockTokenAvailableObserver : public CloudPolicyDataStore::Observer {
 
   MOCK_METHOD0(OnDeviceTokenChanged, void());
   MOCK_METHOD0(OnCredentialsChanged, void());
-  MOCK_METHOD0(OnDataStoreGoingAway, void());
 
  private:
   DISALLOW_COPY_AND_ASSIGN(MockTokenAvailableObserver);
diff --git a/chrome/browser/policy/user_policy_token_cache.cc b/chrome/browser/policy/user_policy_token_cache.cc
index f8894ab..ce1403e 100644
--- a/chrome/browser/policy/user_policy_token_cache.cc
+++ b/chrome/browser/policy/user_policy_token_cache.cc
@@ -32,38 +32,36 @@ namespace policy {
 
 namespace em = enterprise_management;
 
-UserPolicyTokenCache::UserPolicyTokenCache(
-    const base::WeakPtr<CloudPolicyDataStore>& data_store,
+UserPolicyTokenLoader::Delegate::~Delegate() {}
+
+UserPolicyTokenLoader::UserPolicyTokenLoader(
+    const base::WeakPtr<Delegate>& delegate,
     const FilePath& cache_file)
-    : data_store_(data_store),
-      cache_file_(cache_file) {
-  data_store_->AddObserver(this);
-}
+    : delegate_(delegate),
+      cache_file_(cache_file) {}
 
-void UserPolicyTokenCache::Load() {
+void UserPolicyTokenLoader::Load() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
-      NewRunnableMethod(this, &UserPolicyTokenCache::LoadOnFileThread));
+      NewRunnableMethod(this, &UserPolicyTokenLoader::LoadOnFileThread));
 }
 
-void UserPolicyTokenCache::Store(const std::string& token,
+void UserPolicyTokenLoader::Store(const std::string& token,
                                  const std::string& device_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
       NewRunnableMethod(this,
-                        &UserPolicyTokenCache::StoreOnFileThread,
+                        &UserPolicyTokenLoader::StoreOnFileThread,
                         token,
                         device_id));
 }
 
-UserPolicyTokenCache::~UserPolicyTokenCache() {
-  if (data_store_)
-    data_store_->RemoveObserver(this);
+UserPolicyTokenLoader::~UserPolicyTokenLoader() {
 }
 
-void UserPolicyTokenCache::LoadOnFileThread() {
+void UserPolicyTokenLoader::LoadOnFileThread() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   std::string device_token;
   std::string device_id;
@@ -84,22 +82,20 @@ void UserPolicyTokenCache::LoadOnFileThread() {
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       NewRunnableMethod(this,
-                        &UserPolicyTokenCache::NotifyOnUIThread,
+                        &UserPolicyTokenLoader::NotifyOnUIThread,
                         device_token,
                         device_id));
 }
 
-void UserPolicyTokenCache::NotifyOnUIThread(const std::string& token,
-                                            const std::string& device_id) {
+void UserPolicyTokenLoader::NotifyOnUIThread(const std::string& token,
+                                             const std::string& device_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  if (data_store_) {
-    data_store_->set_device_id(device_id);
-    data_store_->SetDeviceToken(token, true);
-  }
+  if (delegate_.get())
+    delegate_->OnTokenLoaded(token, device_id);
 }
 
-void UserPolicyTokenCache::StoreOnFileThread(const std::string& token,
-                                             const std::string& device_id) {
+void UserPolicyTokenLoader::StoreOnFileThread(const std::string& token,
+                                              const std::string& device_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   em::DeviceCredentials device_credentials;
   device_credentials.set_device_token(token);
@@ -129,19 +125,46 @@ void UserPolicyTokenCache::StoreOnFileThread(const std::string& token,
   SampleUMA(kMetricTokenStoreSucceeded);
 }
 
-void UserPolicyTokenCache::OnDeviceTokenChanged() {
-  if (!data_store_->device_token().empty() &&
-      !data_store_->device_id().empty()) {
-    // This will also happen after loading the cache.
-    Store(data_store_->device_token(), data_store_->device_id());
+UserPolicyTokenCache::UserPolicyTokenCache(
+    CloudPolicyDataStore* data_store,
+    const FilePath& cache_file)
+    : ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)),
+      data_store_(data_store) {
+  loader_ = new UserPolicyTokenLoader(weak_ptr_factory_.GetWeakPtr(),
+                                      cache_file);
+  data_store_->AddObserver(this);
+}
+
+UserPolicyTokenCache::~UserPolicyTokenCache() {
+  if (data_store_)
+    data_store_->RemoveObserver(this);
+}
+
+void UserPolicyTokenCache::Load() {
+  loader_->Load();
+}
+
+void UserPolicyTokenCache::OnTokenLoaded(const std::string& token,
+                                         const std::string& device_id) {
+  token_ = token;
+  if (data_store_) {
+    data_store_->set_device_id(device_id);
+    data_store_->SetDeviceToken(token, true);
   }
 }
 
-void UserPolicyTokenCache::OnCredentialsChanged() {
+void UserPolicyTokenCache::OnDeviceTokenChanged() {
+  const std::string& new_token(data_store_->device_token());
+  if (!new_token.empty() && !data_store_->device_id().empty()) {
+    if (token_ == new_token)
+      return;
+
+    token_ = new_token;
+    loader_->Store(new_token, data_store_->device_id());
+  }
 }
 
-void UserPolicyTokenCache::OnDataStoreGoingAway() {
-  data_store_->RemoveObserver(this);
+void UserPolicyTokenCache::OnCredentialsChanged() {
 }
 
 }  // namespace policy
diff --git a/chrome/browser/policy/user_policy_token_cache.h b/chrome/browser/policy/user_policy_token_cache.h
index e7eeed5..a7ce971 100644
--- a/chrome/browser/policy/user_policy_token_cache.h
+++ b/chrome/browser/policy/user_policy_token_cache.h
@@ -16,13 +16,20 @@
 
 namespace policy {
 
-// Responsible for managing the on-disk token cache.
-class UserPolicyTokenCache
-    : public base::RefCountedThreadSafe<UserPolicyTokenCache>,
-      public CloudPolicyDataStore::Observer {
+// Handles disk access and threading details for loading and storing tokens.
+class UserPolicyTokenLoader
+    : public base::RefCountedThreadSafe<UserPolicyTokenLoader> {
  public:
-  UserPolicyTokenCache(const base::WeakPtr<CloudPolicyDataStore>& data_store,
-                       const FilePath& cache_file);
+  // Callback interface for reporting a successfull load.
+  class Delegate {
+   public:
+    virtual ~Delegate();
+    virtual void OnTokenLoaded(const std::string& token,
+                               const std::string& device_id) = 0;
+  };
+
+  UserPolicyTokenLoader(const base::WeakPtr<Delegate>& delegate,
+                        const FilePath& cache_file);
 
   // Starts loading the disk cache. After the load is finished, the result is
   // reported through the delegate.
@@ -31,14 +38,9 @@ class UserPolicyTokenCache
   // Stores credentials asynchronously to disk.
   void Store(const std::string& token, const std::string& device_id);
 
-  // CloudPolicyData::Observer implementation:
-  virtual void OnDeviceTokenChanged() OVERRIDE;
-  virtual void OnCredentialsChanged() OVERRIDE;
-  virtual void OnDataStoreGoingAway() OVERRIDE;
-
  private:
-  friend class base::RefCountedThreadSafe<UserPolicyTokenCache>;
-  virtual ~UserPolicyTokenCache();
+  friend class base::RefCountedThreadSafe<UserPolicyTokenLoader>;
+  ~UserPolicyTokenLoader();
 
   void LoadOnFileThread();
   void NotifyOnUIThread(const std::string& token,
@@ -46,9 +48,41 @@ class UserPolicyTokenCache
   void StoreOnFileThread(const std::string& token,
                          const std::string& device_id);
 
-  const base::WeakPtr<CloudPolicyDataStore> data_store_;
+  const base::WeakPtr<Delegate> delegate_;
   const FilePath cache_file_;
 
+  DISALLOW_COPY_AND_ASSIGN(UserPolicyTokenLoader);
+};
+
+// Responsible for managing the on-disk token cache.
+class UserPolicyTokenCache : public CloudPolicyDataStore::Observer,
+                             public UserPolicyTokenLoader::Delegate {
+ public:
+  UserPolicyTokenCache(CloudPolicyDataStore* data_store,
+                       const FilePath& cache_file);
+  virtual ~UserPolicyTokenCache();
+
+  // Starts loading the disk cache and installs the token in the data store.
+  void Load();
+
+  // UserPolicyTokenLoader::Delegate implementation:
+  virtual void OnTokenLoaded(const std::string& token,
+                             const std::string& device_id) OVERRIDE;
+
+  // CloudPolicyDataStore::Observer implementation:
+  virtual void OnDeviceTokenChanged() OVERRIDE;
+  virtual void OnCredentialsChanged() OVERRIDE;
+
+ private:
+  base::WeakPtrFactory<UserPolicyTokenLoader::Delegate> weak_ptr_factory_;
+
+  CloudPolicyDataStore* data_store_;
+  scoped_refptr<UserPolicyTokenLoader> loader_;
+
+  // The current token in the cache. Upon new token notifications, we compare
+  // against this in order to avoid writing the file when there's no change.
+  std::string token_;
+
   DISALLOW_COPY_AND_ASSIGN(UserPolicyTokenCache);
 };