Refactored SettingsFrontend and forwarding of calls to the backends.

chrome/browser/extensions/settings/settings_frontend.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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;
 
 namespace extensions {
 
 namespace {

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

Revisiting this file after going through the others. So yeah, could all the
threading stuff be done in here, make those what-were-backend interfaces
asynchronous?

That would make the Managed thing end up posting back to the same thread, at
least naively, but I think that's a good thing to guarantee that the callbacks
aren't reentrant.

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

So the ValueStoreCache interface has been extended with a new method to get the
preferred TaskRunner, and the other tasks are always posted to that runner. This
means they're always executed asynchronously, and the ValueStoreCache
implementations can be a lot simpler.

That pattern introduces a lifetime issue with the caches though, since the
SettingsFrontend lives on UI and can be deleted while a task that was posted to
FILE is still pending (this used to be prevented by the BackendWrapper). This
issue is easy to avoid by posting the cache deletion to its TaskRunner; any
previously pending task will execute before the dtors. This also means the
CacheMap can't have linked_ptrs (which I dislike :-) ). wdyt?

 
 // Settings change Observer which forwards changes on to the extension
 // processes for |profile| and its incognito partner if it exists.
 class DefaultObserver : public SettingsObserver {
  public:
   explicit DefaultObserver(Profile* profile) : profile_(profile) {}
 
   // SettingsObserver implementation.
   virtual void OnSettingsChanged(
       const std::string& extension_id,
       settings_namespace::Namespace settings_namespace,
       const std::string& change_json) OVERRIDE {
     profile_->GetExtensionEventRouter()->DispatchEventToExtension(
         extension_id,
         extension_event_names::kOnSettingsChanged,
         // This is the list of function arguments to pass to the onChanged
         // handler of extensions, an array of [changes, settings_namespace].
         std::string("[") + change_json + ",\"" +
             settings_namespace::ToString(settings_namespace) + "\"]",
         NULL,
         GURL());
   }
 
  private:
   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),
     std::numeric_limits<size_t>::max(),
     std::numeric_limits<size_t>::max()
   };
   return limits;
 }
 
 SettingsStorageQuotaEnforcer::Limits GetSyncLimits() {
   SettingsStorageQuotaEnforcer::Limits limits = {
     static_cast<size_t>(api::storage::sync::QUOTA_BYTES),
     static_cast<size_t>(api::storage::sync::QUOTA_BYTES_PER_ITEM),
     static_cast<size_t>(api::storage::sync::MAX_ITEMS)
   };
   return limits;
 }
 
 }  // 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);
 }
 
 // static
 SettingsFrontend* SettingsFrontend::Create(
     const scoped_refptr<SettingsStorageFactory>& storage_factory,
     Profile* profile) {
   return new SettingsFrontend(storage_factory, profile);
 }
 
 SettingsFrontend::SettingsFrontend(
     const scoped_refptr<SettingsStorageFactory>& factory, Profile* profile)
     : local_quota_limit_(GetLocalLimits()),
       sync_quota_limit_(GetSyncLimits()),
       profile_(profile),
       observers_(new SettingsObserverList()),
       profile_observer_(new DefaultObserver(profile)) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(!profile->IsOffTheRecord());
 
   observers_->AddObserver(profile_observer_.get());
 
   const FilePath& profile_path = profile->GetPath();
-  backends_[settings_namespace::LOCAL].app =
-      BackendWrapper::CreateAndInit(
+  frontends_[settings_namespace::LOCAL] =
+      new SettingsStorageFrontend(
+          settings_namespace::LOCAL,
           factory,
           local_quota_limit_,
           observers_,
-          profile_path.AppendASCII(
-              ExtensionService::kLocalAppSettingsDirectoryName));
-  backends_[settings_namespace::LOCAL].extension =
-      BackendWrapper::CreateAndInit(
-          factory,
-          local_quota_limit_,
-          observers_,
-          profile_path.AppendASCII(
-              ExtensionService::kLocalExtensionSettingsDirectoryName));
-  backends_[settings_namespace::SYNC].app =
-      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::kSyncAppSettingsDirectoryName));
-  backends_[settings_namespace::SYNC].extension =
-      BackendWrapper::CreateAndInit(
-          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;
   }
 }
 
 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);
   }
 }
 
 scoped_refptr<SettingsObserverList> SettingsFrontend::GetObservers() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   return observers_;
 }
 
-// BackendWrappers
-
-SettingsFrontend::BackendWrappers::BackendWrappers() {}
-SettingsFrontend::BackendWrappers::~BackendWrappers() {}
-
 }  // namespace extensions