Refactored SettingsFrontend and forwarding of calls to the backends.

chrome/browser/extensions/settings/settings_frontend.cc

Index: chrome/browser/extensions/settings/settings_frontend.cc
diff --git a/chrome/browser/extensions/settings/settings_frontend.cc b/chrome/browser/extensions/settings/settings_frontend.cc
index 7eef6bc07fe20229ef9ef8525ba341254c5b05e4..8ce68d50d70541f3beafc64bed60662145f8bffe 100644
--- a/chrome/browser/extensions/settings/settings_frontend.cc
+++ b/chrome/browser/extensions/settings/settings_frontend.cc
@@ -4,21 +4,22 @@
 
 #include "chrome/browser/extensions/settings/settings_frontend.h"
 
+#include <limits>
+
 #include "base/bind.h"
 #include "base/file_path.h"
-#include "base/string_number_conversions.h"
+#include "base/stl_util.h"
 #include "chrome/browser/extensions/extension_event_names.h"
 #include "chrome/browser/extensions/extension_event_router.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/settings/leveldb_settings_storage_factory.h"
 #include "chrome/browser/extensions/settings/settings_backend.h"
+#include "chrome/browser/extensions/settings/settings_managed_frontend.h"
 #include "chrome/browser/extensions/settings/settings_namespace.h"
-#include "chrome/browser/extensions/settings/weak_unlimited_settings_storage.h"
+#include "chrome/browser/extensions/settings/settings_storage_frontend.h"
 #include "chrome/browser/profiles/profile.h"
-#include "chrome/browser/value_store/leveldb_value_store.h"
 #include "chrome/common/extensions/api/storage.h"
 #include "content/public/browser/browser_thread.h"
-#include "content/public/browser/notification_service.h"
 
 using content::BrowserThread;
 
@@ -52,43 +53,6 @@ class DefaultObserver : public SettingsObserver {
   Profile* const profile_;
 };
 
-void CallbackWithSyncableService(
-    const SettingsFrontend::SyncableServiceCallback& callback,
-    SettingsBackend* backend) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  callback.Run(backend);
-}
-
-void CallbackWithStorage(
-    const std::string& extension_id,
-    const SettingsFrontend::StorageCallback& callback,
-    SettingsBackend* backend) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  callback.Run(backend->GetStorage(extension_id));
-}
-
-void CallbackWithNullStorage(
-    const SettingsFrontend::StorageCallback& callback) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  callback.Run(NULL);
-}
-
-void DeleteStorageOnFileThread(
-    const std::string& extension_id, SettingsBackend* backend) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  backend->DeleteStorage(extension_id);
-}
-
-void CallbackWithUnlimitedStorage(
-    const std::string& extension_id,
-    const SettingsFrontend::StorageCallback& callback,
-    SettingsBackend* backend) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  WeakUnlimitedSettingsStorage unlimited_storage(
-      backend->GetStorage(extension_id));
-  callback.Run(&unlimited_storage);
-}
-
 SettingsStorageQuotaEnforcer::Limits GetLocalLimits() {
   SettingsStorageQuotaEnforcer::Limits limits = {
     static_cast<size_t>(api::storage::local::QUOTA_BYTES),
@@ -109,100 +73,6 @@ SettingsStorageQuotaEnforcer::Limits GetSyncLimits() {
 
 }  // namespace
 
-// Ref-counted container for a SettingsBackend object.
-class SettingsFrontend::BackendWrapper
-    : public base::RefCountedThreadSafe<BackendWrapper> {
- public:
-  // Creates a new BackendWrapper and initializes it on the FILE thread.
-  static scoped_refptr<BackendWrapper> CreateAndInit(
-      const scoped_refptr<SettingsStorageFactory>& factory,
-      const SettingsStorageQuotaEnforcer::Limits& quota,
-      const scoped_refptr<SettingsObserverList>& observers,
-      const FilePath& path) {
-    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-    scoped_refptr<BackendWrapper> backend_wrapper =
-        new BackendWrapper(factory, quota, observers);
-    BrowserThread::PostTask(
-        BrowserThread::FILE,
-        FROM_HERE,
-        base::Bind(
-            &SettingsFrontend::BackendWrapper::InitOnFileThread,
-            backend_wrapper,
-            path));
-    return backend_wrapper;
-  }
-
-  typedef base::Callback<void(SettingsBackend*)> BackendCallback;
-
-  // Runs |callback| with the wrapped Backend on the FILE thread.
-  void RunWithBackend(const BackendCallback& callback) {
-    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-    BrowserThread::PostTask(
-        BrowserThread::FILE,
-        FROM_HERE,
-        base::Bind(
-            &SettingsFrontend::BackendWrapper::RunWithBackendOnFileThread,
-            this,
-            callback));
-  }
-
-  SettingsBackend* GetBackend() const {
-    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-    return backend_;
-  }
-
- private:
-  friend class base::RefCountedThreadSafe<BackendWrapper>;
-
-  BackendWrapper(
-      const scoped_refptr<SettingsStorageFactory>& storage_factory,
-      const SettingsStorageQuotaEnforcer::Limits& quota,
-      const scoped_refptr<SettingsObserverList>& observers)
-      : storage_factory_(storage_factory),
-        quota_(quota),
-        observers_(observers),
-        backend_(NULL) {
-    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  }
-
-  virtual ~BackendWrapper() {
-    if (BrowserThread::CurrentlyOn(BrowserThread::FILE)) {
-      delete backend_;
-    } else if (BrowserThread::CurrentlyOn(BrowserThread::UI)) {
-      BrowserThread::DeleteSoon(BrowserThread::FILE, FROM_HERE, backend_);
-    } else {
-      NOTREACHED();
-    }
-  }
-
-  void InitOnFileThread(const FilePath& path) {
-    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-    DCHECK(!backend_);
-    backend_ = new SettingsBackend(storage_factory_, path, quota_, observers_);
-    storage_factory_ = NULL;
-    observers_ = NULL;
-  }
-
-  void RunWithBackendOnFileThread(const BackendCallback& callback) {
-    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-    DCHECK(backend_);
-    callback.Run(backend_);
-  }
-
-  // Only need these until |backend_| exists.
-  scoped_refptr<SettingsStorageFactory> storage_factory_;
-  const SettingsStorageQuotaEnforcer::Limits quota_;
-  scoped_refptr<SettingsObserverList> observers_;
-
-  // Wrapped Backend.  Used exclusively on the FILE thread, and is created on
-  // the FILE thread in InitOnFileThread.
-  SettingsBackend* backend_;
-
-  DISALLOW_COPY_AND_ASSIGN(BackendWrapper);
-};
-
-// SettingsFrontend
-
 // static
 SettingsFrontend* SettingsFrontend::Create(Profile* profile) {
   return new SettingsFrontend(new LeveldbSettingsStorageFactory(), profile);
@@ -228,52 +98,39 @@ SettingsFrontend::SettingsFrontend(
   observers_->AddObserver(profile_observer_.get());
 
   const FilePath& profile_path = profile->GetPath();
-  backends_[settings_namespace::LOCAL].app =
-      BackendWrapper::CreateAndInit(
-          factory,
-          local_quota_limit_,
-          observers_,
-          profile_path.AppendASCII(
-              ExtensionService::kLocalAppSettingsDirectoryName));
-  backends_[settings_namespace::LOCAL].extension =
-      BackendWrapper::CreateAndInit(
+  frontends_[settings_namespace::LOCAL] =
+      new SettingsStorageFrontend(
+          settings_namespace::LOCAL,
           factory,
           local_quota_limit_,
           observers_,
-          profile_path.AppendASCII(
-              ExtensionService::kLocalExtensionSettingsDirectoryName));
-  backends_[settings_namespace::SYNC].app =
-      BackendWrapper::CreateAndInit(
-          factory,
-          sync_quota_limit_,
-          observers_,
-          profile_path.AppendASCII(
-              ExtensionService::kSyncAppSettingsDirectoryName));
-  backends_[settings_namespace::SYNC].extension =
-      BackendWrapper::CreateAndInit(
+          profile_path);
+  sync_frontend_ =

[not at google - send to devlin, 2012/07/18 07:46:18]

Having this as a member variable *and* in the map is a bit of a strange
ownership situation. Can you just put it in the map as normal, then static_cast
to a SettingsStorageFrontend when it's used below?

[Joao da Silva, 2012/07/18 21:40:25]

The purpose of this pointer was precisely to avoid that cast. Done anyway :-)

+      new SettingsStorageFrontend(
+          settings_namespace::SYNC,
           factory,
           sync_quota_limit_,
           observers_,
-          profile_path.AppendASCII(
-              ExtensionService::kSyncExtensionSettingsDirectoryName));
+          profile_path);
+  frontends_[settings_namespace::SYNC] = sync_frontend_;
+  frontends_[settings_namespace::MANAGED] =
+      new SettingsManagedFrontend(profile);
 }
 
 SettingsFrontend::~SettingsFrontend() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   observers_->RemoveObserver(profile_observer_.get());
+  STLDeleteValues(&frontends_);

[not at google - send to devlin, 2012/07/18 07:46:18]

I prefer making frontend_ contain linked_ptrs rather than raw pointers.

(hot topic right now, apparently)

[Joao da Silva, 2012/07/18 21:40:25]

Done.

 }
 
 syncer::SyncableService* SettingsFrontend::GetBackendForSync(
     syncer::ModelType type) const {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  std::map<settings_namespace::Namespace, BackendWrappers>::const_iterator
-      sync_backends = backends_.find(settings_namespace::SYNC);
-  DCHECK(sync_backends != backends_.end());
   switch (type) {
     case syncer::APP_SETTINGS:
-      return sync_backends->second.app->GetBackend();
+      return sync_frontend_->GetAppBackend();
     case syncer::EXTENSION_SETTINGS:
-      return sync_backends->second.extension->GetBackend();
+      return sync_frontend_->GetExtensionBackend();
     default:
       NOTREACHED();
       return NULL;
@@ -283,52 +140,28 @@ syncer::SyncableService* SettingsFrontend::GetBackendForSync(
 void SettingsFrontend::RunWithStorage(
     const std::string& extension_id,
     settings_namespace::Namespace settings_namespace,
-    const StorageCallback& callback) {
+    const SettingsNamespaceFrontend::StorageCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
+  SettingsNamespaceFrontend* frontend = frontends_[settings_namespace];
+  DCHECK(frontend);

[not at google - send to devlin, 2012/07/18 07:46:18]

CHECK not DCHECK

[Joao da Silva, 2012/07/18 21:40:25]

The idea was to make this fail silently in release builds, but I guess CHECKing
is better. Done.

+
   const Extension* extension =
       profile_->GetExtensionService()->GetExtensionById(extension_id, true);
-  if (!extension) {
-    BrowserThread::PostTask(
-        BrowserThread::FILE,
-        FROM_HERE,
-        base::Bind(&CallbackWithNullStorage, callback));
+  if (!extension || !frontend) {

[not at google - send to devlin, 2012/07/18 07:46:18]

!frontend check should be unnecessary

[Joao da Silva, 2012/07/18 21:40:25]

Done.

+    callback.Run(NULL);

[not at google - send to devlin, 2012/07/18 07:46:18]

The reason for CallbackWithNullStorage is so that this method isn't re-entrant.
I'd prefer to maintain that contract if possible, I think it's a source of
subtle bugs where it's assumed these asynchronous methods are indeed
asynchronous.

Oh, and the other reason is that callback implementations should always be
called on the correct thread. So this isn't quite right.

Could you add a method like GetNaturalThread or something (maybe there's some
chrome idiom for what that's called) to SettingsNamespaceFrontend and post to
CallbackWithNullStorage with that?

[Joao da Silva, 2012/07/18 21:40:25]

Thanks for clarifying this issue. Done.
Done.

     return;
   }
 
-  // A neat way to implement unlimited storage; if the extension has the
-  // unlimited storage permission, force through all calls to Set() (in the
-  // same way that writes from sync ignore quota).
-  // But only if it's local storage (bad stuff would happen if sync'ed
-  // storage is allowed to be unlimited).
-  bool is_unlimited =
-      settings_namespace == settings_namespace::LOCAL &&
-      extension->HasAPIPermission(APIPermission::kUnlimitedStorage);
-
-  scoped_refptr<BackendWrapper> backend;
-  if (extension->is_app()) {
-    backend = backends_[settings_namespace].app;
-  } else {
-    backend = backends_[settings_namespace].extension;
-  }
-
-  backend->RunWithBackend(
-      base::Bind(
-          is_unlimited ?
-              &CallbackWithUnlimitedStorage : &CallbackWithStorage,
-          extension_id,
-          callback));
+  frontend->RunWithStorage(extension, callback);
 }
 
 void SettingsFrontend::DeleteStorageSoon(
     const std::string& extension_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  SettingsFrontend::BackendWrapper::BackendCallback callback =
-      base::Bind(&DeleteStorageOnFileThread, extension_id);
-  for (std::map<settings_namespace::Namespace, BackendWrappers>::iterator it =
-      backends_.begin(); it != backends_.end(); ++it) {
-    it->second.app->RunWithBackend(callback);
-    it->second.extension->RunWithBackend(callback);
+  for (FrontendMap::iterator it = frontends_.begin();
+       it != frontends_.end(); ++it) {
+    it->second->DeleteStorageSoon(extension_id);
   }
 }
 
@@ -337,9 +170,4 @@ scoped_refptr<SettingsObserverList> SettingsFrontend::GetObservers() {
   return observers_;
 }
 
-// BackendWrappers
-
-SettingsFrontend::BackendWrappers::BackendWrappers() {}
-SettingsFrontend::BackendWrappers::~BackendWrappers() {}
-
 }  // namespace extensions