Implement an initial extension settings API.

chrome/browser/extensions/extension_settings.cc

+// Copyright (c) 2011 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/extension_settings.h"
+
+#include "base/bind.h"
+#include "base/json/json_reader.h"
+#include "base/json/json_writer.h"
+#include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
+#include "content/browser/browser_thread.h"
+#include "chrome/browser/extensions/extension_settings_leveldb_storage.h"
+#include "chrome/browser/extensions/extension_settings_noop_storage.h"
+#include "chrome/browser/extensions/extension_settings_storage_cache.h"
+#include "third_party/leveldb/include/leveldb/iterator.h"
+#include "third_party/leveldb/include/leveldb/write_batch.h"
+
+// TODO(kalman): Policy for dots in the key names.  Make sure the behaviour of

[Mihai Parparita -not on Chrome, 2011/06/21 23:40:11]

Filing these as bugs that block a master extension settings bug seems like a
better way to keep track of this.

[not at google - send to devlin, 2011/06/22 09:40:38]

Ok.  I won't file the bugs yet but will remove the TODOs before submitting
anything and file corresponding bugs.

+//               DictionaryValue and JSON classes is consistent with leveldb.
+// TODO(kalman): Integration tests.
+// TODO(kalman): More unit tests (see extension_settings_storage_unittest.cc).
+// TODO(kalman): Use structured cloning rather than JSON.
+// TODO(kalman): Quotas.
+
+namespace {
+
+// Creates a storage area of a requested type on the FILE thread.
+class CreateStorageClosure {
+ public:
+  // Fallback type may be 0 to not have any fallback type.
+  // Ownership of callback is taken.
+  CreateStorageClosure(const FilePath& base_path,

[Mihai Parparita -not on Chrome, 2011/06/21 23:40:11]

These arguments should be one per line (see on "For function declarations and
definitions, put each argument on a separate line unless..." bit on
http://www.chromium.org/developers/coding-style).

(applies to other places too)

[not at google - send to devlin, 2011/06/22 09:40:38]

Done.

+      std::map<std::string, ExtensionSettingsStorage*>* storage_objs,
+      const std::string& extension_id, int type, int fallback_type,
+      ExtensionSettings::Callback* callback)
+      : base_path_(base_path), storage_objs_(storage_objs),
+        extension_id_(extension_id), type_(type), fallback_type_(fallback_type),
+        callback_(callback), storage_(NULL) {}
+
+  ~CreateStorageClosure() {}
+
+  void Run() {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));

[Mihai Parparita -not on Chrome, 2011/06/21 23:40:11]

We generally have these as CHECKs in extension code.

[not at google - send to devlin, 2011/06/22 09:40:38]

Really?  Seems to be about equal.  grep says 79 occurrences of DCHECK on
BrowserThread and 61 occurrences of CHECK.  Will change if you want.

+    BrowserThread::PostTask(
+        BrowserThread::FILE,
+        FROM_HERE,
+        base::Bind(
+          &CreateStorageClosure::RunOnFileThread, base::Unretained(this)));
+  }
+
+ private:
+  void RunOnFileThread() {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+    storage_ = CreateStorage(type_);
+    if (storage_ == NULL && fallback_type_ > 0) {
+      storage_ = CreateStorage(fallback_type_);
+    }
+    BrowserThread::PostTask(
+        BrowserThread::UI,
+        FROM_HERE,
+        base::Bind(&CreateStorageClosure::Done, base::Unretained(this)));
+  }
+
+  void Done() {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    DCHECK(storage_ != NULL);
+    // Cache the result now.  To avoid a race condition, check again to see
+    // whether a storage has been created already; if so, use that one.
+    std::map<std::string, ExtensionSettingsStorage*>::iterator existing =
+        storage_objs_->find(extension_id_);
+    if (existing == storage_objs_->end()) {
+      (*storage_objs_)[extension_id_] = storage_;
+    } else {
+      delete storage_;
+      storage_ = existing->second;
+      DCHECK(storage_ != NULL);
+    }
+    callback_->Run(storage_);
+    delete this;
+  }
+
+  // Creates a storage object of a given type.  If that fails, returns NULL.
+  ExtensionSettingsStorage* CreateStorage(int type) {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+    ExtensionSettingsStorage* storage = NULL;
+
+    switch (type & ~ExtensionSettingsStorage::CACHED) {
+      case ExtensionSettingsStorage::NOOP:
+        storage = new ExtensionSettingsNoopStorage();
+        break;
+
+      case ExtensionSettingsStorage::LEVELDB: {

[Mihai Parparita -not on Chrome, 2011/06/21 23:40:11]

Ideally the details of the LevelDB implementation vs. the no-op implementation
should be hidden from this class and be encapsulated in the implementation. Can
you move all this into ExtensionSettingsLeveldbStorage?

[not at google - send to devlin, 2011/06/22 09:40:38]

Done.

+        FilePath path = base_path_.Append(extension_id_);
+        // TODO(kalman): why doesn't FilePath/leveldb handle this internally?

[Mihai Parparita -not on Chrome, 2011/06/21 23:40:11]

I see a similar pattern elsewhere in the codebase. Adding a FilePath::AsUTF8
helper seems reasonable to me, but I'm not that familiar with the intricacies of
cross-platform paths.

[not at google - send to devlin, 2011/06/22 09:40:38]

Yes, that's why I have it here -- the other leveldb code seems to do it.  I
haven't actually tried building or running this on Windows yet (probably should
actually) so I was going to check it at that point.

+#if defined(OS_POSIX)
+        std::string os_path(path.value());
+#elif defined(OS_WIN)
+        std::string os_path = base::SysWideToUTF8(path.value());
+#endif
+        leveldb::Options options;
+        options.create_if_missing = true;
+        leveldb::DB* db;
+        leveldb::Status status = leveldb::DB::Open(options, os_path, &db);
+        if (!status.ok()) {
+          LOG(WARNING) << "Failed to create leveldb at " << path.value() <<
+                          ": " << status.ToString();
+          return NULL;
+        }
+        storage = new ExtensionSettingsLeveldbStorage(db);
+        break;
+      }
+
+      default:
+        NOTREACHED();
+    }
+
+    if (type & ExtensionSettingsStorage::CACHED) {
+      storage = new ExtensionSettingsStorageCache(storage);
+    }
+
+    return storage;
+  }
+
+  const FilePath& base_path_;
+  std::map<std::string, ExtensionSettingsStorage*>* storage_objs_;
+  std::string extension_id_;
+  int type_;
+  int fallback_type_;
+  scoped_ptr<ExtensionSettings::Callback> callback_;
+
+  ExtensionSettingsStorage* storage_;
+};
+
+// Returns a specified storage object on the message loop of the current thread.
+// Used to force an asynchronous API even when a storage area already exists.
+class GetExistingStorageClosure {
+ public:
+  GetExistingStorageClosure(ExtensionSettings::Callback* callback,
+      ExtensionSettingsStorage* storage)
+      : callback_(callback), storage_(storage) {}
+
+  ~GetExistingStorageClosure() {
+    delete callback_;
+  }
+
+  void Run() {
+    MessageLoop::current()->PostTask(FROM_HERE,
+        base::Bind(&GetExistingStorageClosure::Run2, base::Unretained(this)));
+  }
+
+ private:
+  void Run2() {
+    callback_->Run(storage_);
+    delete this;
+  }
+
+  ExtensionSettings::Callback* callback_;

[Mihai Parparita -not on Chrome, 2011/06/21 23:40:11]

Any reason why this isn't a scoped_ptr, like it is in CreateStorageClosure?

[not at google - send to devlin, 2011/06/22 09:40:38]

Nope.  Done.

+  ExtensionSettingsStorage* storage_;
+};
+
+// Deletes a storage area on the FILE thread.
+class DeleteStorageClosure {
+ public:
+  explicit DeleteStorageClosure(ExtensionSettingsStorage* storage)
+      : storage_(storage) {}
+
+  ~DeleteStorageClosure() {}
+
+  void Run() {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    BrowserThread::PostTask(
+        BrowserThread::FILE,
+        FROM_HERE,
+        base::Bind(
+          &DeleteStorageClosure::RunOnFileThread, base::Unretained(this)));
+  }
+
+ private:
+  void RunOnFileThread() {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+    delete storage_;
+    delete this;
+  }
+
+  ExtensionSettingsStorage* storage_;

[Mihai Parparita -not on Chrome, 2011/06/21 23:40:11]

Ditto here.

[not at google - send to devlin, 2011/06/22 09:40:38]

Done.

[not at google - send to devlin, 2011/06/22 09:40:38]

Done.

+};
+
+}  // namespace
+
+ExtensionSettings::ExtensionSettings(const FilePath& base_path)
+    : base_path_(base_path) {
+}
+
+ExtensionSettings::~ExtensionSettings() {
+  // Need to delete all the storage objects we created; and any leveldb-based
+  // ones must be deleted on the FILE thread.

[Mihai Parparita -not on Chrome, 2011/06/21 23:40:11]

Same thing here about moving implementation-specific behavior into the
implementation.

[not at google - send to devlin, 2011/06/22 09:40:38]

Done.

[not at google - send to devlin, 2011/06/22 09:40:38]

Done.

+  std::map<std::string, ExtensionSettingsStorage*>::iterator it;
+  for (it = storage_objs_.begin(); it != storage_objs_.end(); ++it) {
+    ExtensionSettingsStorage* storage = it->second;
+    if (storage->type() & ExtensionSettingsStorage::LEVELDB) {
+      (new DeleteStorageClosure(storage))->Run();
+    } else {
+      delete storage;
+    }
+  }
+}
+
+void ExtensionSettings::GetStorage(const std::string& extension_id,
+    ExtensionSettings::Callback* callback) {
+  if (!GetExistingStorage(extension_id, callback)) {
+    (new CreateStorageClosure(
+        base_path_,
+        &storage_objs_,
+        extension_id,
+        ExtensionSettingsStorage::LEVELDB | ExtensionSettingsStorage::CACHED,
+        ExtensionSettingsStorage::NOOP | ExtensionSettingsStorage::CACHED,
+        callback))->Run();
+  }
+}
+
+void ExtensionSettings::GetStorageForTesting(int type, const std::string&
+    extension_id,
+    Callback* callback) {
+  if (!GetExistingStorage(extension_id, callback)) {
+    (new CreateStorageClosure(
+        base_path_,
+        &storage_objs_,
+        extension_id,
+        type,
+        0,
+        callback))->Run();
+  }
+}
+
+bool ExtensionSettings::GetExistingStorage(const std::string& extension_id,
+    ExtensionSettings::Callback* callback) {
+  std::map<std::string, ExtensionSettingsStorage*>::iterator existing =
+      storage_objs_.find(extension_id);
+  if (existing == storage_objs_.end()) {
+    // No existing storage.
+    return false;
+  }
+  // Existing storage.  Reply with that.
+  ExtensionSettingsStorage* storage = existing->second;
+  DCHECK(storage != NULL);
+  (new GetExistingStorageClosure(callback, storage))->Run();

[Mihai Parparita -not on Chrome, 2011/06/21 23:40:11]

Having to have GetExistingStorageClosure to make the response be async seems
hacky. If instead of having your own callback class you just had a typedef
base::Callback<void(ExtensionSettingsStorage*> Callback then:
1) You could just call PostTak with the callback directly
2) Callers wouldn't need a shim class to pass to GetStorage, and could just use
base::Bind with one of their existing methods

[not at google - send to devlin, 2011/06/22 09:40:38]

Hm.  That would be a lot nicer.  The same can probably be said for the callbacks
from the storage functions; just, as is idiomatic in javascript, have the
methods take a DictionaryValue* and a std::string error, and guarantee that only
one will be set (rather than having OnSuccess versus OnFailure).

However, there was actually I reason I didn't do this initially; it seems like
there isn't any way to pass function arguments back on a PostMessage without
defining a closure class.  So you'd need to manually ->Run() the closures anyway
(and .: why not just pass a function pointer?).  I could be missing something
though.

Another way that return values could work, of course, is by having the API as

settings.GetStorage(ExtensionSettingsStorage** storage, base::Closure callback);

i.e. fill in the storage** with a pointer to storage, which works more nicely if
the caller is writing shims for these things anyway.

Hope that makes sense.  I'll have a go tomorrow.

[not at google - send to devlin, 2011/06/22 23:49:42]

Oops.  Because this is C++ not C.

[not at google - send to devlin, 2011/06/23 05:08:33]

Another self correction: the Closure classes support currying.  That's perfect.

[not at google - send to devlin, 2011/06/23 13:44:26]

Ok, done this now.

+  return true;
+}