Cache force-installed apps/extensions in device-local accounts

chrome/browser/chromeos/policy/device_local_account_policy_service.h

Index: chrome/browser/chromeos/policy/device_local_account_policy_service.h
diff --git a/chrome/browser/chromeos/policy/device_local_account_policy_service.h b/chrome/browser/chromeos/policy/device_local_account_policy_service.h
index 02b82f4de26ffff2b6532db9d5338a852df83a96..9e8527840eeb8e0341f6f902fd0769bf2610f90a 100644
--- a/chrome/browser/chromeos/policy/device_local_account_policy_service.h
+++ b/chrome/browser/chromeos/policy/device_local_account_policy_service.h
@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_CHROMEOS_POLICY_DEVICE_LOCAL_ACCOUNT_POLICY_SERVICE_H_
 
 #include <map>
+#include <set>
 #include <string>
 
 #include "base/basictypes.h"
@@ -14,8 +15,8 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
+#include "chrome/browser/chromeos/extensions/device_local_account_external_policy_loader.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
-#include "chrome/browser/policy/cloud/cloud_policy_core.h"
 #include "chrome/browser/policy/cloud/cloud_policy_store.h"
 
 namespace base {
@@ -30,7 +31,8 @@ class SessionManagerClient;
 
 namespace policy {
 
-class CloudPolicyClient;
+class CloudPolicyCore;
+struct DeviceLocalAccount;
 class DeviceLocalAccountPolicyStore;
 class DeviceManagementService;
 
@@ -39,19 +41,26 @@ class DeviceManagementService;
 class DeviceLocalAccountPolicyBroker {
  public:
   // |task_runner| is the runner for policy refresh tasks.
-  explicit DeviceLocalAccountPolicyBroker(
-      const std::string& user_id,
+  DeviceLocalAccountPolicyBroker(
+      const DeviceLocalAccount& account,
       scoped_ptr<DeviceLocalAccountPolicyStore> store,
       const scoped_refptr<base::SequencedTaskRunner>& task_runner);
   ~DeviceLocalAccountPolicyBroker();
 
+  const std::string& account_id() const { return account_id_; }
   const std::string& user_id() const { return user_id_; }

[Joao da Silva, 2013/10/17 14:57:54]

Document these two, it's not clear when each should be used. Or alternatively,
document these fields in DeviceLocalAccount and document here that these fields
refer to those others.

[bartfab (slow), 2013/10/18 12:58:39]

Done.

 
-  CloudPolicyCore* core() { return &core_; }
-  const CloudPolicyCore* core() const { return &core_; }
+  scoped_refptr<chromeos::DeviceLocalAccountExternalPolicyLoader>
+      extension_loader() const { return extension_loader_; }
 
-  // Establish a cloud connection for the service.
-  void Connect(scoped_ptr<CloudPolicyClient> client);
+  CloudPolicyCore* core() { return core_.get(); }
+  const CloudPolicyCore* core() const { return core_.get(); }
+
+  // Fire up the cloud connection for fetching policy for the account from the
+  // cloud if this is an enterprise-managed device.
+  void ConnectIfPossible(
+      chromeos::DeviceSettingsService* device_settings_service,
+      DeviceManagementService* device_management_service);
 
   // Destroy the cloud connection, stopping policy refreshes.
   void Disconnect();
@@ -64,9 +73,12 @@ class DeviceLocalAccountPolicyBroker {
   std::string GetDisplayName() const;
 
  private:
+  const std::string account_id_;
   const std::string user_id_;
-  scoped_ptr<DeviceLocalAccountPolicyStore> store_;
-  CloudPolicyCore core_;
+  const scoped_ptr<DeviceLocalAccountPolicyStore> store_;
+  scoped_refptr<chromeos::DeviceLocalAccountExternalPolicyLoader>
+      extension_loader_;
+  scoped_ptr<CloudPolicyCore> core_;
 
   DISALLOW_COPY_AND_ASSIGN(DeviceLocalAccountPolicyBroker);
 };
@@ -92,7 +104,8 @@ class DeviceLocalAccountPolicyService : public CloudPolicyStore::Observer {
   DeviceLocalAccountPolicyService(
       chromeos::SessionManagerClient* session_manager_client,
       chromeos::DeviceSettingsService* device_settings_service,
-      chromeos::CrosSettings* cros_settings);
+      chromeos::CrosSettings* cros_settings,
+      scoped_refptr<base::SequencedTaskRunner> extension_cache_task_runner);
   virtual ~DeviceLocalAccountPolicyService();
 
   // Initializes the cloud policy service connection.
@@ -117,29 +130,32 @@ class DeviceLocalAccountPolicyService : public CloudPolicyStore::Observer {
   virtual void OnStoreError(CloudPolicyStore* store) OVERRIDE;
 
  private:
-  struct PolicyBrokerWrapper {
-    PolicyBrokerWrapper();
+  typedef std::map<std::string, DeviceLocalAccountPolicyBroker*>
+      PolicyBrokerMap;
 
-    // Return the |broker|, creating it first if necessary.
-    DeviceLocalAccountPolicyBroker* GetBroker();
+  // Returns |true| if the directory in which force-installed extensions are
+  // cached for |account_id| is busy, either because a broker that was using
+  // this directory has not shut down completely yet or because the directory is
+  // being deleted.
+  bool IsExtensionCacheDirectoryBusy(const std::string& account_id);
 
-    // Fire up the cloud connection for fetching policy for the account from the
-    // cloud if this is an enterprise-managed device.
-    void ConnectIfPossible();
+  // Starts any extension caches that are not running yet but can be started now
+  // because their cache directories are no longer busy.
+  void StartExtensionCachesIfPossible();
 
-    // Destroy the cloud connection.
-    void Disconnect();
+  // Checks whether a broker exists for |account_id|. If so, starts the broker's
+  // extension cache and returns |true|. Otherwise, returns |false|.
+  bool StartExtensionCacheForAccountIfPresent(const std::string& account_id);
 
-    // Delete the broker.
-    void DeleteBroker();
+  // Called back when any extension caches belonging to device-local accounts
+  // that no longer exist have been removed at start-up.
+  void OnOrphanedExtensionCachesDeleted();
 
-    std::string user_id;
-    std::string account_id;
-    DeviceLocalAccountPolicyService* parent;
-    DeviceLocalAccountPolicyBroker* broker;
-  };
+  // Called back when the extension cache for |account_id| has been shut down.
+  void OnObsoleteExensionCacheShutdown(const std::string& account_id);

[Joao da Silva, 2013/10/17 14:57:54]

OnObsoleteExtensionCacheShutdown (missing "t")

[bartfab (slow), 2013/10/18 12:58:39]

Done.

 
-  typedef std::map<std::string, PolicyBrokerWrapper> PolicyBrokerMap;
+  // Called back when the extension cache for |account_id| has been removed.
+  void OnObsoleteExtensionCacheDeleted(const std::string& account_id);
 
   // Re-queries the list of defined device-local accounts from device settings
   // and updates |policy_brokers_| to match that list.
@@ -163,14 +179,32 @@ class DeviceLocalAccountPolicyService : public CloudPolicyStore::Observer {
   // The device-local account policy brokers, keyed by user ID.
   PolicyBrokerMap policy_brokers_;
 
+  // Whether a call to UpdateAccountList() is pending because |cros_settings_|
+  // are not trusted yet).

[Joao da Silva, 2013/10/17 14:57:54]

remove )

[bartfab (slow), 2013/10/18 12:58:39]

Done.

+  bool waiting_for_cros_settings_;
+
+  // Whether the process of removing orphaned extension caches at startup has
+  // been started.
+  bool orphaned_cache_deletion_started_;
+  // Whether the process of removing orphaned extension caches at startup has
+  // been completed.
+  bool orphaned_cache_deletion_finished_;
+
+  // Account IDs whose extension cache directories are busy, either because a
+  // broker for the account has not shut down completely yet or because the
+  // directory is being deleted.
+  std::set<std::string> busy_extension_cache_directories_;

[Joao da Silva, 2013/10/17 14:57:54]

I think you got the logic right but I'm wary of having the set and 3 bools; it's
quite a bit of state to track, and very easy to get wrong in a subsequent
extension/refactoring to this class.

It'd be extra nice if this could be simplified, but I'm not stopping this change
because of this :-)

[bartfab (slow), 2013/10/18 12:58:39]

I introduced an enum that combines the two bools into a single state.

+
+  // Background task runner on which all extension caches and cache directory
+  // removal tasks run.
+  const scoped_refptr<base::SequencedTaskRunner> extension_cache_task_runner_;
+
   ObserverList<Observer, true> observers_;
 
-  scoped_ptr<chromeos::CrosSettings::ObserverSubscription>
+  const scoped_ptr<chromeos::CrosSettings::ObserverSubscription>
       local_accounts_subscription_;
 
-  // Weak pointer factory for cros_settings_->PrepareTrustedValues() callbacks.
-  base::WeakPtrFactory<DeviceLocalAccountPolicyService>
-      cros_settings_callback_factory_;
+  base::WeakPtrFactory<DeviceLocalAccountPolicyService> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(DeviceLocalAccountPolicyService);
 };