Implement an initial extension settings API.

Implement an initial extension settings API.

Some general notes:
 - It's a lot of code for a single review, about 1.5K lines (plus tests, plus those generated docs, = 3.5K).  Apologies.  But it's close to the minimal amount of useful functionality.  I've left some TODOs in the code to fix up soon.
 - No integration-style tests, but I'll start writing those now.  Works from within a browser though.
 - Sync not hooked up yet, of course.
 - Ditto events.
 - Ditto thinking about incognito mode.
 - Ditto fun bugs like what happens if you set key "foo.bar" to "baz".
 - This is the first significant amount of C++ code I've ever written... so don't hold back.
 - The docs (i.e. my changes to extension_api.json) are a little incomplete, and I'm aware of that.

A summary of the implementation:
 - There is an ExtensionSettings factory-type object which hands out ExtensionSettingsStorage areas for extensions.  You may notice that I basically did the same thing that ExtensionPreferences does (so, lives with the profile, etc).
 - ExtensionSettingsStorage is a pure interface, with three implementations.
   - the main leveldb implementation.
   - a caching decorator, designed to sit on top of the leveldb implementation.
   - a "no-op" implementation which gives a trivial in-memory storage area when wrapped with a cache.  This is used in the cases where the leveldb database fails to initialise (ExtensionSettings handles this).
   -  and note that my plan is, when hooking up sync, that this will be implemented as another decorator.
 - ... the code is pretty well documented so not much else to say.

For testing, turns out gtest is pretty snazzy.  There are a bunch of test fixtrues in extension_settings_storage_unittest.h, so that 3 different configurations are tested: leveldb, leveldb + cache, and no-op + cache.

BUG=47327
TEST=unit tests provided


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96159

[dgrogan, 2011-06-18 01:54:40]

I only looked at the leveldb stuff.  It's pretty solid.

http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings.cc (right):

http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings.cc:107: LOG(WARNING) << "Failed to
create leveldb at " << path.value();
You can also print out status.ToString()

http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings.cc:173: BrowserThread::FILE,
Thread-lifetime issues are fresh in my mind, so: do you know if the FILE thread
will always be around when this is executed?

http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings_leveldb_storage.cc (right):

http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_leveldb_storage.cc:146:
options.snapshot = db_->GetSnapshot();
Why do you use snapshots when you're reading?  Won't all the reads and writes
happen on the same thread?

http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_leveldb_storage.cc:200:
FailOnFileThread("Failed");
Why don't you pass it->status().ToString() back through to the callback instead
of "Failed"?

http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_leveldb_storage.cc:350:
batch.Delete(it->key());
Does batching up a bunch of deletes on an iterator over a snapshot work?  Do the
real non-snapshot values get deleted?

It must work because you have a unit test for it, it just seems weird.

http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings_leveldb_storage.h (right):

http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_leveldb_storage.h:20:
~ExtensionSettingsLeveldbStorage() {}
I was going to comment that this needs to be deleted on the file thread but I
see that you already covered that :)

Is it kosher to add a DCHECK here to ensure the destructor is indeed run on the
right thread?

http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings_storage_unittest.cc (right):

http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_storage_unittest.cc:29:
MessageLoop::current()->Run();
When do you use Run vs RunAllPending?

[not at google - send to devlin, 2011-06-20 05:33:11]

Cheers.

http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings.cc (right):

http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings.cc:107: LOG(WARNING) << "Failed to
create leveldb at " << path.value();
On 2011/06/18 01:54:40, dgrogan wrote:
> You can also print out status.ToString()

Done.

http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings.cc:173: BrowserThread::FILE,
On 2011/06/18 01:54:40, dgrogan wrote:
> Thread-lifetime issues are fresh in my mind, so: do you know if the FILE
thread
> will always be around when this is executed?

Hmm.  I guess not.  Excuse my naivety with respect to thread lifetime in the
following comments.

Do you make this comment in particular for this callback (which seems reasonable
given it's called from a destructor), or for all places when I've done PostTask?

If the former -- I don't really think it matters.  If the FILE thread doesn't
exist then there is a memory leak, but would this only be the case if Chrome is
already shutting down?  Besides, there isn't much that can be done about it.  It
needs to be destructed on the FILE thread.  And also, there isn't anything
waiting on a callback.

If the latter -- i.e. if normal browser operation were to result in the FILE
thread not existing -- I presume a fix would be something along the lines of
changing

BrowserThread::PostTask(...)

to

if (!BrowserThread::PostTask(...)) {
  // Can't post to the FILE thread, so post an asynchronous
  // OnFailure event to the current thread
  MessageLoop::current()->PostTask(FROM_HERE, ...);
}

?

http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings_leveldb_storage.cc (right):

http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_leveldb_storage.cc:146:
options.snapshot = db_->GetSnapshot();
On 2011/06/18 01:54:40, dgrogan wrote:
> Why do you use snapshots when you're reading?  Won't all the reads and writes
> happen on the same thread?

Hm.  Yes.  It just seems... instinctively wrong to read from the db without
using snapshotting, even though it's all from the same thread.

E.g. what if the leveldb instance is somehow modified outside the knowledge of
this class?  Maybe even... two Chrome processes are running somehow, and the
same extension in each is reading while the other is writing?  I don't even know
if that situation can arise, and if it did... there might be other problems too.
 But at least there would be one less :-)

Happy to change this to not use a snapshot, though, if there's a reason not to.

http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_leveldb_storage.cc:200:
FailOnFileThread("Failed");
On 2011/06/18 01:54:40, dgrogan wrote:
> Why don't you pass it->status().ToString() back through to the callback
instead
> of "Failed"?

This is the message sent back to the extension.  I left it out since it seems
like... some kind of attack that an extension might be able to do to gather
information about the browser.  I.e. not sure what kind of messages you get in
here.

Happy to change it if you think it's safe.

http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_leveldb_storage.cc:350:
batch.Delete(it->key());
On 2011/06/18 01:54:40, dgrogan wrote:
> Does batching up a bunch of deletes on an iterator over a snapshot work?  Do
the
> real non-snapshot values get deleted?
> 
> It must work because you have a unit test for it, it just seems weird.

Maybe I'm misunderstanding something.  The snapshotting is to get a consistent
state of all the keys in the db at the time of clearing... so their values don't
come in to it.

And the batch object that the deletes are getting applied to isn't linked with
the database until after the snapshot is released, so that should be fine.

http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings_leveldb_storage.h (right):

http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_leveldb_storage.h:20:
~ExtensionSettingsLeveldbStorage() {}
On 2011/06/18 01:54:40, dgrogan wrote:
> I was going to comment that this needs to be deleted on the file thread but I
> see that you already covered that :)
> 
> Is it kosher to add a DCHECK here to ensure the destructor is indeed run on
the
> right thread?

I don't see why not.  A crash is a crash, whether it's in a destructor or not,
right?

So, Done.

http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings_storage_unittest.cc (right):

http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_storage_unittest.cc:29:
MessageLoop::current()->Run();
On 2011/06/18 01:54:40, dgrogan wrote:
> When do you use Run vs RunAllPending?

I don't think RunAllPending would work, at least with the current flow, which is
 1. do something with storage
 2. wait until quit task
 ..... storage does stuff on FILE thread, responds on UI thread
 3. callback runs quit task
 4. resume
RunAllPending wouldn't block on (2) because there wouldn't be a pending task
until the FILE thread finishes its stuff.

Can you think of a cleaner way of doing it that wouldn't require the quit task
stuff?

[dgrogan, 2011-06-21 02:36:04]

http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings.cc (right):

http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings.cc:173: BrowserThread::FILE,
On 2011/06/20 05:33:11, kalman1 wrote:
> On 2011/06/18 01:54:40, dgrogan wrote:
> > Thread-lifetime issues are fresh in my mind, so: do you know if the FILE
> thread
> > will always be around when this is executed?
> 
> Hmm.  I guess not.  Excuse my naivety with respect to thread lifetime in the
> following comments.
> 
> Do you make this comment in particular for this callback (which seems
reasonable
> given it's called from a destructor), or for all places when I've done
PostTask?

The former, I saw it was called from the destructor.

> 
> If the former -- I don't really think it matters.  If the FILE thread doesn't
> exist then there is a memory leak, but would this only be the case if Chrome
is
> already shutting down?

I think so but don't know.  I was kind of hoping you would know and could tell
me :)

I erroneously thought that the leveldb object's destructor _needed_ to run.  I
think you're right though: no big deal if it doesn't.

Besides, there isn't much that can be done about it.  It
> needs to be destructed on the FILE thread.  And also, there isn't anything
> waiting on a callback.
> 
> If the latter -- i.e. if normal browser operation were to result in the FILE
> thread not existing -- I presume a fix would be something along the lines of
> changing
> 
> BrowserThread::PostTask(...)
> 
> to
> 
> if (!BrowserThread::PostTask(...)) {
>   // Can't post to the FILE thread, so post an asynchronous
>   // OnFailure event to the current thread
>   MessageLoop::current()->PostTask(FROM_HERE, ...);
> }
> 
> ?

http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings_leveldb_storage.cc (right):

http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_leveldb_storage.cc:146:
options.snapshot = db_->GetSnapshot();
On 2011/06/20 05:33:11, kalman1 wrote:
> On 2011/06/18 01:54:40, dgrogan wrote:
> > Why do you use snapshots when you're reading?  Won't all the reads and
writes
> > happen on the same thread?
> 
> Hm.  Yes.  It just seems... instinctively wrong to read from the db without
> using snapshotting, even though it's all from the same thread.

Ok, they are just defensive.  Using them is fine, just wanted to make sure I
understood was going on.

> 
> E.g. what if the leveldb instance is somehow modified outside the knowledge of
> this class?  Maybe even... two Chrome processes are running somehow, and the
> same extension in each is reading while the other is writing?  I don't even
know
> if that situation can arise, and if it did... there might be other problems
too.
>  But at least there would be one less :-)

That situation shouldn't arise as far as leveldb is concerned, I think it'll
throw an error if opened by more than one process at a time.  But I know what
you mean, "one less" is good.

> 
> Happy to change this to not use a snapshot, though, if there's a reason not
to.

http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_leveldb_storage.cc:200:
FailOnFileThread("Failed");
On 2011/06/20 05:33:11, kalman1 wrote:
> On 2011/06/18 01:54:40, dgrogan wrote:
> > Why don't you pass it->status().ToString() back through to the callback
> instead
> > of "Failed"?
> 
> This is the message sent back to the extension.  I left it out since it seems
> like... some kind of attack that an extension might be able to do to gather
> information about the browser.  I.e. not sure what kind of messages you get in
> here.

Ok, I see where you're coming from.

> Happy to change it if you think it's safe.

No idea if it is or not.  Hopefully Mihai will know.

http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_leveldb_storage.cc:350:
batch.Delete(it->key());
On 2011/06/20 05:33:11, kalman1 wrote:
> On 2011/06/18 01:54:40, dgrogan wrote:
> > Does batching up a bunch of deletes on an iterator over a snapshot work?  Do
> the
> > real non-snapshot values get deleted?
> > 
> > It must work because you have a unit test for it, it just seems weird.
> 
> Maybe I'm misunderstanding something.  The snapshotting is to get a consistent
> state of all the keys in the db at the time of clearing... so their values
don't
> come in to it.

Oh, I didn't realize it gets a consistent set of keys only.

> 
> And the batch object that the deletes are getting applied to isn't linked with
> the database until after the snapshot is released, so that should be fine.

This all seems fine; I hadn't used snapshots before and didn't know how they
worked.

http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings_storage_unittest.cc (right):

http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_storage_unittest.cc:29:
MessageLoop::current()->Run();
On 2011/06/20 05:33:11, kalman1 wrote:
> On 2011/06/18 01:54:40, dgrogan wrote:
> > When do you use Run vs RunAllPending?
> 
> I don't think RunAllPending would work, at least with the current flow, which
is
>  1. do something with storage
>  2. wait until quit task
>  ..... storage does stuff on FILE thread, responds on UI thread
>  3. callback runs quit task
>  4. resume
> RunAllPending wouldn't block on (2) because there wouldn't be a pending task
> until the FILE thread finishes its stuff.
> 

Thanks for the good explanation.

> Can you think of a cleaner way of doing it that wouldn't require the quit task
> stuff?

The way I was just taught was to pass the current message loop to the file
BrowserThread but never start that thread.  Then your Tasks posted to the FILE
thread will queue on the current() message loop.  When you want to wait for them
to be executed you do MessageLoop::current()->RunAllPending();

The test I did that for is indexed_db_quota_client_unittest.cc:
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/content/browser/in_pr...

The long review:
http://codereview.chromium.org/7053030/#msg-ahJjaHJvbWl1bWNvZGVyZXZpZXdyHQsSB...

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

For future reference, it seems like this patch could have been broken up a bit
further:
1. Stubbed out API with no implementation
2. No-op implementation
3. LevelDB implementation

For now I've done a first pass.

Another TODO/bug worth filing is cleaning up the storage data when extensions
are uninstalled.

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings.cc (right):

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:19: // TODO(kalman): Policy for
dots in the key names.  Make sure the behaviour of
Filing these as bugs that block a master extension settings bug seems like a
better way to keep track of this.

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:33: CreateStorageClosure(const
FilePath& base_path,
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)

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:44:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
We generally have these as CHECKs in extension code.

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:93: case
ExtensionSettingsStorage::LEVELDB: {
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?

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:95: // TODO(kalman): why doesn't
FilePath/leveldb handle this internally?
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.

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:158:
ExtensionSettings::Callback* callback_;
Any reason why this isn't a scoped_ptr, like it is in CreateStorageClosure?

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:186: ExtensionSettingsStorage*
storage_;
Ditto here.

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:197: // ones must be deleted on
the FILE thread.
Same thing here about moving implementation-specific behavior into the
implementation.

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:247: (new
GetExistingStorageClosure(callback, storage))->Run();
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

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings.h (right):

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.h:10: #include
"chrome/browser/extensions/extension_settings_storage.h"
Can you just forward declare ExtensionSettingsStorage?

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.h:11: #include
"third_party/leveldb/include/leveldb/db.h"
Doesn't seem like you need this include at all.

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.h:34: // a leveldb instance will
fall back to NOOP|CACHED.
I'm not sure that the fallback behavior is a great idea. If we can't create a
persistent DB, then the failure mode of having settings persist until the
browser session ends will confuse users and developers. Better to just fail
upfront.

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.h:46: private:
Should add DISALLOW_COPY_AND_ASSIGN.

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_api.cc (right):

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_api.cc:73: error_ = "Unsupported
argument type";
Errors are generally put in constants.

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_api.h (right):

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_api.h:22: // call
SendResponse(false) -- this is so that the extensions macros work.
Not sure what "macros" refers to here.

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_api.h:62: std::string Thing();
I don't think you need any of the Thing() methods.

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_apitest.cc (right):

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_apitest.cc:15:
host_resolver()->AddRule("*", "127.0.0.1");
I don't think this is necessary.

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_apitest.cc:16:
ASSERT_TRUE(StartTestServer());
We generally put this in the test case, though I don't think you need a test
server at all (we use that when testing the interaction of extensions with page
contents, but your tests lives entirely in the extension process).

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_leveldb_storage.h (right):

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_leveldb_storage.h:24: void
Get(const std::string& key, Callback* callback);
Using virtual and OVERRIDE on all these seems like a good idea (applies to
ExtensionSettingsNoopStorage too).

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_storage.h (right):

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_storage.h:35: // The different
types of extension settings storage, designed to be
It doesn't quite make sense for all these to be in the same enum. Types (no-op,
LevelDB) are separate from decorators/wrappers (cached, eventually synced,
etc.). I think you should have both a Type enum and an Attributes/Properties
bitfield.

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

Thank Mihai, thanks again David.

Btw, I have a list of style issues for myself that I'll run through all my code
with (from your code review Mihai) so don't panic when you see that the latest
code still has (for example) multiple function arguments on the same line.  I'll
fix it tomorrow; ran out of time today.

http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings.cc (right):

http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings.cc:173: BrowserThread::FILE,
> I think so but don't know.  I was kind of hoping you would know and could tell
> me :)

Ah.  Sadly not :)

http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings_leveldb_storage.cc (right):

http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_leveldb_storage.cc:146:
options.snapshot = db_->GetSnapshot();
On 2011/06/21 02:36:05, dgrogan wrote:
> On 2011/06/20 05:33:11, kalman1 wrote:
> > On 2011/06/18 01:54:40, dgrogan wrote:
> > > Why do you use snapshots when you're reading?  Won't all the reads and
> writes
> > > happen on the same thread?
> > 
> > Hm.  Yes.  It just seems... instinctively wrong to read from the db without
> > using snapshotting, even though it's all from the same thread.
> 
> Ok, they are just defensive.  Using them is fine, just wanted to make sure I
> understood was going on.
> 
> > 
> > E.g. what if the leveldb instance is somehow modified outside the knowledge
of
> > this class?  Maybe even... two Chrome processes are running somehow, and the
> > same extension in each is reading while the other is writing?  I don't even
> know
> > if that situation can arise, and if it did... there might be other problems
> too.
> >  But at least there would be one less :-)
> 
> That situation shouldn't arise as far as leveldb is concerned, I think it'll
> throw an error if opened by more than one process at a time.  But I know what
> you mean, "one less" is good.
> 
> > 
> > Happy to change this to not use a snapshot, though, if there's a reason not
> to.
> 

Cool, I added a comment.

http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_leveldb_storage.cc:200:
FailOnFileThread("Failed");
On 2011/06/21 02:36:05, dgrogan wrote:
> On 2011/06/20 05:33:11, kalman1 wrote:
> > On 2011/06/18 01:54:40, dgrogan wrote:
> > > Why don't you pass it->status().ToString() back through to the callback
> > instead
> > > of "Failed"?
> > 
> > This is the message sent back to the extension.  I left it out since it
seems
> > like... some kind of attack that an extension might be able to do to gather
> > information about the browser.  I.e. not sure what kind of messages you get
in
> > here.
> 
> Ok, I see where you're coming from.
> 
> > Happy to change it if you think it's safe.
> 
> No idea if it is or not.  Hopefully Mihai will know.

Btw I had a look through the leveldb code but wasn't able to conclude a whole
lot.

http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_leveldb_storage.cc:350:
batch.Delete(it->key());
On 2011/06/21 02:36:05, dgrogan wrote:
> On 2011/06/20 05:33:11, kalman1 wrote:
> > On 2011/06/18 01:54:40, dgrogan wrote:
> > > Does batching up a bunch of deletes on an iterator over a snapshot work? 
Do
> > the
> > > real non-snapshot values get deleted?
> > > 
> > > It must work because you have a unit test for it, it just seems weird.
> > 
> > Maybe I'm misunderstanding something.  The snapshotting is to get a
consistent
> > state of all the keys in the db at the time of clearing... so their values
> don't
> > come in to it.
> 
> Oh, I didn't realize it gets a consistent set of keys only.
> 
> > 
> > And the batch object that the deletes are getting applied to isn't linked
with
> > the database until after the snapshot is released, so that should be fine.
> 
> This all seems fine; I hadn't used snapshots before and didn't know how they
> worked.

I think it gets a consistent view of both keys and values?  I was just saying
that this code only cares about the keys.

http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings_storage_unittest.cc (right):

http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_storage_unittest.cc:29:
MessageLoop::current()->Run();
On 2011/06/21 02:36:05, dgrogan wrote:
> On 2011/06/20 05:33:11, kalman1 wrote:
> > On 2011/06/18 01:54:40, dgrogan wrote:
> > > When do you use Run vs RunAllPending?
> > 
> > I don't think RunAllPending would work, at least with the current flow,
which
> is
> >  1. do something with storage
> >  2. wait until quit task
> >  ..... storage does stuff on FILE thread, responds on UI thread
> >  3. callback runs quit task
> >  4. resume
> > RunAllPending wouldn't block on (2) because there wouldn't be a pending task
> > until the FILE thread finishes its stuff.
> > 
> 
> Thanks for the good explanation.
> 
> > Can you think of a cleaner way of doing it that wouldn't require the quit
task
> > stuff?
> 
> The way I was just taught was to pass the current message loop to the file
> BrowserThread but never start that thread.  Then your Tasks posted to the FILE
> thread will queue on the current() message loop.  When you want to wait for
them
> to be executed you do MessageLoop::current()->RunAllPending();
> 
> The test I did that for is indexed_db_quota_client_unittest.cc:
>
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/content/browser/in_pr...
> 
> The long review:
>
http://codereview.chromium.org/7053030/#msg-ahJjaHJvbWl1bWNvZGVyZXZpZXdyHQsSB...

Ah, very nice.  Thank you.

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings.cc (right):

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:19: // TODO(kalman): Policy for
dots in the key names.  Make sure the behaviour of
On 2011/06/21 23:40:11, Mihai Parparita wrote:
> Filing these as bugs that block a master extension settings bug seems like a
> better way to keep track of this.

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

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:33: CreateStorageClosure(const
FilePath& base_path,
On 2011/06/21 23:40:11, Mihai Parparita wrote:
> 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)

Done.

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:44:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2011/06/21 23:40:11, Mihai Parparita wrote:
> We generally have these as CHECKs in extension code.

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

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:93: case
ExtensionSettingsStorage::LEVELDB: {
On 2011/06/21 23:40:11, Mihai Parparita wrote:
> 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?

Done.

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:95: // TODO(kalman): why doesn't
FilePath/leveldb handle this internally?
On 2011/06/21 23:40:11, Mihai Parparita wrote:
> 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.

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.

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:158:
ExtensionSettings::Callback* callback_;
On 2011/06/21 23:40:11, Mihai Parparita wrote:
> Any reason why this isn't a scoped_ptr, like it is in CreateStorageClosure?

Nope.  Done.

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:186: ExtensionSettingsStorage*
storage_;
On 2011/06/21 23:40:11, Mihai Parparita wrote:
> Ditto here.

Done.

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:186: ExtensionSettingsStorage*
storage_;
On 2011/06/21 23:40:11, Mihai Parparita wrote:
> Ditto here.

Done.

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:197: // ones must be deleted on
the FILE thread.
On 2011/06/21 23:40:11, Mihai Parparita wrote:
> Same thing here about moving implementation-specific behavior into the
> implementation.

Done.

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:197: // ones must be deleted on
the FILE thread.
On 2011/06/21 23:40:11, Mihai Parparita wrote:
> Same thing here about moving implementation-specific behavior into the
> implementation.

Done.

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:247: (new
GetExistingStorageClosure(callback, storage))->Run();
On 2011/06/21 23:40:11, Mihai Parparita wrote:
> 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

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.

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings.h (right):

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.h:10: #include
"chrome/browser/extensions/extension_settings_storage.h"
On 2011/06/21 23:40:11, Mihai Parparita wrote:
> Can you just forward declare ExtensionSettingsStorage?

Done.

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.h:11: #include
"third_party/leveldb/include/leveldb/db.h"
On 2011/06/21 23:40:11, Mihai Parparita wrote:
> Doesn't seem like you need this include at all.

Done.

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.h:34: // a leveldb instance will
fall back to NOOP|CACHED.
On 2011/06/21 23:40:11, Mihai Parparita wrote:
> I'm not sure that the fallback behavior is a great idea. If we can't create a
> persistent DB, then the failure mode of having settings persist until the
> browser session ends will confuse users and developers. Better to just fail
> upfront.

What should the failure behaviour be then?  Just every read/write to settings
call OnFailure?

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.h:46: private:
On 2011/06/21 23:40:11, Mihai Parparita wrote:
> Should add DISALLOW_COPY_AND_ASSIGN.

Done.

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_api.cc (right):

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_api.cc:73: error_ = "Unsupported
argument type";
On 2011/06/21 23:40:11, Mihai Parparita wrote:
> Errors are generally put in constants.

Done.

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_api.h (right):

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_api.h:22: // call
SendResponse(false) -- this is so that the extensions macros work.
On 2011/06/21 23:40:11, Mihai Parparita wrote:
> Not sure what "macros" refers to here.

Oh like EXTENSION_FUNCTION_VALIDATE.

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_api.h:62: std::string Thing();
On 2011/06/21 23:40:11, Mihai Parparita wrote:
> I don't think you need any of the Thing() methods.

Oops.  Debugging cleanup fail.

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_apitest.cc (right):

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_apitest.cc:15:
host_resolver()->AddRule("*", "127.0.0.1");
On 2011/06/21 23:40:11, Mihai Parparita wrote:
> I don't think this is necessary.

Done.

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_apitest.cc:16:
ASSERT_TRUE(StartTestServer());
On 2011/06/21 23:40:11, Mihai Parparita wrote:
> We generally put this in the test case, though I don't think you need a test
> server at all (we use that when testing the interaction of extensions with
page
> contents, but your tests lives entirely in the extension process).

Done.  Yeah this just came from one of the tests I copied, forgot to come back
to it.

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_leveldb_storage.h (right):

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_leveldb_storage.h:24: void
Get(const std::string& key, Callback* callback);
On 2011/06/21 23:40:11, Mihai Parparita wrote:
> Using virtual and OVERRIDE on all these seems like a good idea (applies to
> ExtensionSettingsNoopStorage too).

Done.

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_storage.h (right):

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_storage.h:35: // The different
types of extension settings storage, designed to be
On 2011/06/21 23:40:11, Mihai Parparita wrote:
> It doesn't quite make sense for all these to be in the same enum. Types
(no-op,
> LevelDB) are separate from decorators/wrappers (cached, eventually synced,
> etc.). I think you should have both a Type enum and an Attributes/Properties
> bitfield.

Done.  The type() method wasn't really necessary either, so I removed it.

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

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings.cc (right):

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:247: (new
GetExistingStorageClosure(callback, storage))->Run();
> (and .: why not just pass a function pointer?).

Oops.  Because this is C++ not C.

[Mihai Parparita -not on Chrome, 2011-06-23 03:16:46]

http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings_leveldb_storage.cc (right):

http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_leveldb_storage.cc:200:
FailOnFileThread("Failed");
On 2011/06/21 02:36:05, dgrogan wrote:
> > like... some kind of attack that an extension might be able to do to gather
> > information about the browser.  I.e. not sure what kind of messages you get
in
> > here.
> 
> Ok, I see where you're coming from.
> 
> > Happy to change it if you think it's safe.
> 
> No idea if it is or not.  Hopefully Mihai will know.

Sorry, didn't see this question earlier. Unless the extension author can
actually do something about (even while developing/debugging), I don't think we
need additional information in these error messages (though they should probably
reference "extension settings" instead of just "failed" and use a constant).

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

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings.cc (right):

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:247: (new
GetExistingStorageClosure(callback, storage))->Run();
Another self correction: the Closure classes support currying.  That's perfect.

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

http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings_leveldb_storage.cc (right):

http://codereview.chromium.org/7189029/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_leveldb_storage.cc:200:
FailOnFileThread("Failed");
On 2011/06/23 03:16:46, Mihai Parparita wrote:
> On 2011/06/21 02:36:05, dgrogan wrote:
> > > like... some kind of attack that an extension might be able to do to
gather
> > > information about the browser.  I.e. not sure what kind of messages you
get
> in
> > > here.
> > 
> > Ok, I see where you're coming from.
> > 
> > > Happy to change it if you think it's safe.
> > 
> > No idea if it is or not.  Hopefully Mihai will know.
> 
> Sorry, didn't see this question earlier. Unless the extension author can
> actually do something about (even while developing/debugging), I don't think
we
> need additional information in these error messages (though they should
probably
> reference "extension settings" instead of just "failed" and use a constant).

Done.

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings.cc (right):

http://codereview.chromium.org/7189029/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:247: (new
GetExistingStorageClosure(callback, storage))->Run();
Ok, done this now.

[Matt Perry, 2011-06-23 18:11:45]

http://codereview.chromium.org/7189029/diff/16001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings.cc (right):

http://codereview.chromium.org/7189029/diff/16001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:165: &storage_objs_,
What if ExtensionSettings is deleted before CreateStorageClosure::Done is
called? storage_objs_ will be garbage at that point.

It might be better to handle the thread ping-pong within this class itself, i.e.
  PostTask(this, &GetStorageOnFileThread, args);
and have ExtensionSettings be ref-counted.

Another option is to use WeakPtrs. Pass a WeakPtr to the ExtensionSettings
object, which is only accessed on the UI thread (WeakPtrs are not thread safe),
so that you don't dereference it when it goes away.

http://codereview.chromium.org/7189029/diff/16001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings.h (right):

http://codereview.chromium.org/7189029/diff/16001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.h:49:
DISALLOW_COPY_AND_ASSIGN(ExtensionSettings);
this should be the very last thing in the class

http://codereview.chromium.org/7189029/diff/16001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.h:56: std::map<std::string,
ExtensionSettingsStorage*> storage_objs_;
please document these member vars

http://codereview.chromium.org/7189029/diff/16001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_api.cc (right):

http://codereview.chromium.org/7189029/diff/16001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_api.cc:61: return true;
You need to AddRef here, and Release when you send the response.

http://codereview.chromium.org/7189029/diff/16001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_storage_unittest.cc (right):

http://codereview.chromium.org/7189029/diff/16001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_storage_unittest.cc:20: #define
newCallback(expected) (new AssertEqualsCallback((expected), __LINE__))
style should be NEW_CALLBACK for a macro.

http://codereview.chromium.org/7189029/diff/16001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_storage_unittest.cc:124: static
Value* val = NULL;
valgrind will complain about all these values being leaked. you'll have to
delete them somehow. I would use scoped_ptrs on some object that shares the
test's lifetime instead.

http://codereview.chromium.org/7189029/diff/16001/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/7189029/diff/16001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:712: "type": "any",
you should use the "choices" key rather than an "any" type. See bookmarks.get.

ditto for set and remove

http://codereview.chromium.org/7189029/diff/16001/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/settings/test.html (right):

http://codereview.chromium.org/7189029/diff/16001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/settings/test.html:40:
chrome.experimental.settings.get(['nothing', 'baz', 'hello', 'ignore'], stage3);
line length

http://codereview.chromium.org/7189029/diff/16001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/settings/test.html:42: function
stage1(settings) {
I think you can declare these functions in order.

[Matt Perry, 2011-06-23 19:14:10]

some more comments

http://codereview.chromium.org/7189029/diff/21001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings.cc (right):

http://codereview.chromium.org/7189029/diff/21001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:56:
&CreateStorageClosure::RunOnFileThread, base::Unretained(this)));
nit: indent +2

http://codereview.chromium.org/7189029/diff/21001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_api.cc (right):

http://codereview.chromium.org/7189029/diff/21001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_api.cc:22: void
SettingsFunction::GetStorageCallback::Run(
why not just make this a method of SettingsFunction?

http://codereview.chromium.org/7189029/diff/21001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_api.cc:59:
&GetStorageCallback::Run,
nit: indent +2

http://codereview.chromium.org/7189029/diff/21001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_leveldb_storage.cc (right):

http://codereview.chromium.org/7189029/diff/21001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_leveldb_storage.cc:100:
leveldb::DB* db_;
beware of lifetime here. there's no guarantee that db_ will live longer than
ResultClosure. you need to use some sort of WeakPtr (careful, WeakPtr is not
thread safe), or a refptr.

http://codereview.chromium.org/7189029/diff/21001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_leveldb_storage.cc:109:
callback_->OnSuccess(settings_);
make settings_ a scoped_ptr, and use settings_.release() here instead. it's
better to have ptr ownership explicit.

http://codereview.chromium.org/7189029/diff/21001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_leveldb_storage.cc:257: Value*
value_;
see comment about settings_

http://codereview.chromium.org/7189029/diff/21001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_leveldb_storage.cc:463: (new
DeleteStorageClosure(this))->Run();
we have BrowserThread::DeleteSoon to handle this case

http://codereview.chromium.org/7189029/diff/21001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_noop_storage.cc (right):

http://codereview.chromium.org/7189029/diff/21001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_noop_storage.cc:16: class
SuccessResultClosure {
it seems like a closure class is unnecessary here. you could do the same with a
simple static function:

static CallOnSuccess(ExtensionSettingsStorage::Callback* callback,
DictionaryValue* settings) {
  callback->OnSuccess(settings);
  delete callback;
}

and use it like:
BrowserThread::PostTask(NewRunnableFunction(CallOnSuccess, callback, settings));

(or whatever the equivalent base::Bind() syntax is - that's newer and I'm not
familiar with it yet.)

http://codereview.chromium.org/7189029/diff/21001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_noop_storage.cc:16: class
SuccessResultClosure {
btw, in general, your use of closures is not a common pattern in chrome. more
often you'll just see code calling PostTask directly to a method of the current
class.

sometimes the closures are appropriate and likely result in less+cleaner code
(like your leveldb file), but in this case I think a simpler approach is better.

http://codereview.chromium.org/7189029/diff/21001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_storage.h (right):

http://codereview.chromium.org/7189029/diff/21001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_storage.h:45: virtual void
DestroyEventually() = 0;
nit: name this DeleteSoon to be consistent with other cases like this.

http://codereview.chromium.org/7189029/diff/21001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_storage_cache.cc (right):

http://codereview.chromium.org/7189029/diff/21001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_storage_cache.cc:82:
DictionaryValue* cache_;
who owns this ptr? it seems it won't get deleted in some instances, and in
others, it may get deleted too soon.

http://codereview.chromium.org/7189029/diff/21001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_storage_cache.cc:96: delete
callback_;
make callback_ a scoped_ptr

[not at google - send to devlin, 2011-06-27 08:51:02]

Thanks Matt.

http://codereview.chromium.org/7189029/diff/16001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings.cc (right):

http://codereview.chromium.org/7189029/diff/16001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:165: &storage_objs_,
On 2011/06/23 18:11:45, Matt Perry wrote:
> What if ExtensionSettings is deleted before CreateStorageClosure::Done is
> called? storage_objs_ will be garbage at that point.
> 
> It might be better to handle the thread ping-pong within this class itself,
i.e.
>   PostTask(this, &GetStorageOnFileThread, args);
> and have ExtensionSettings be ref-counted.
> 
> Another option is to use WeakPtrs. Pass a WeakPtr to the ExtensionSettings
> object, which is only accessed on the UI thread (WeakPtrs are not thread
safe),
> so that you don't dereference it when it goes away.

Hm.  So I guess in the case where
 1. GetStorage() caller requests a ExtensionSettingsStorage object when one
doesn't already exist.
 2a. Request goes to file thread, in parallel with
 2b. profile is destroyed (so extension settings is destroyed).
 3. Request goes back to the UI thread, callback called with new
ExtensionSettingsStorage.
the tricky part is in 4; the storage isn't really valid because
ExtensionSettings won't be able to cache it.  In fact, it would need to also
call DestroyEventually, but since the destructor of ExtensionSettings has
already been called, it won't ever be.

So in conclusion: ExtensionSettings should be reference counted so that the
above scenario can't actually occur.  The profile can be destroyed, but the
callback is holding on to a reference so it won't be destroyed until after the
GetStorage callback.

However... this still means that the destructor of ExtensionSettings could be
called after GetStorage(), which calls DestroyEventually (or rather, DeleteSoon)
for all storage objects including the one just created/returned, possibly
destroying the underlying ExtensionSettingsStorage mid-callback.

Which is basically the same scenario here; except that in this case it would be
bad to refcount the ExtensionSettingsStorage objects, since the
ExtensionSettings object must have sole ownership of all active
ExtensionSettingsStorage objects for caching.  Otherwise there could be more
than one active storage object for a single extension (and leveldb instance...
etc).

Actually this is only a problem insofar as refcounting the ExtensionSettings
objects means that there could be more than one of these active if
ProfileImpl::OnPrefsLoaded is called more than once.  And at precisely the wrong
time.

Ah well.

http://codereview.chromium.org/7189029/diff/16001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings.h (right):

http://codereview.chromium.org/7189029/diff/16001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.h:49:
DISALLOW_COPY_AND_ASSIGN(ExtensionSettings);
On 2011/06/23 18:11:45, Matt Perry wrote:
> this should be the very last thing in the class

Done.

http://codereview.chromium.org/7189029/diff/16001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.h:56: std::map<std::string,
ExtensionSettingsStorage*> storage_objs_;
On 2011/06/23 18:11:45, Matt Perry wrote:
> please document these member vars

Done.

http://codereview.chromium.org/7189029/diff/16001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_api.cc (right):

http://codereview.chromium.org/7189029/diff/16001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_api.cc:61: return true;
On 2011/06/23 18:11:45, Matt Perry wrote:
> You need to AddRef here, and Release when you send the response.

Done.

http://codereview.chromium.org/7189029/diff/16001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_storage_unittest.cc (right):

http://codereview.chromium.org/7189029/diff/16001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_storage_unittest.cc:20: #define
newCallback(expected) (new AssertEqualsCallback((expected), __LINE__))
On 2011/06/23 18:11:45, Matt Perry wrote:
> style should be NEW_CALLBACK for a macro.

Done.

http://codereview.chromium.org/7189029/diff/16001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_storage_unittest.cc:124: static
Value* val = NULL;
On 2011/06/23 18:11:45, Matt Perry wrote:
> valgrind will complain about all these values being leaked. you'll have to
> delete them somehow. I would use scoped_ptrs on some object that shares the
> test's lifetime instead.

Done.

http://codereview.chromium.org/7189029/diff/16001/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/7189029/diff/16001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:712: "type": "any",
On 2011/06/23 18:11:45, Matt Perry wrote:
> you should use the "choices" key rather than an "any" type. See bookmarks.get.
> 
> ditto for set and remove

Done.

Though only for remove, not set?

http://codereview.chromium.org/7189029/diff/16001/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/settings/test.html (right):

http://codereview.chromium.org/7189029/diff/16001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/settings/test.html:40:
chrome.experimental.settings.get(['nothing', 'baz', 'hello', 'ignore'], stage3);
On 2011/06/23 18:11:45, Matt Perry wrote:
> line length

Done.

http://codereview.chromium.org/7189029/diff/16001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/settings/test.html:42: function
stage1(settings) {
On 2011/06/23 18:11:45, Matt Perry wrote:
> I think you can declare these functions in order.

Done.

http://codereview.chromium.org/7189029/diff/21001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings.cc (right):

http://codereview.chromium.org/7189029/diff/21001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:56:
&CreateStorageClosure::RunOnFileThread, base::Unretained(this)));
On 2011/06/23 19:14:11, Matt Perry wrote:
> nit: indent +2

Done.

http://codereview.chromium.org/7189029/diff/21001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_api.cc (right):

http://codereview.chromium.org/7189029/diff/21001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_api.cc:22: void
SettingsFunction::GetStorageCallback::Run(
On 2011/06/23 19:14:11, Matt Perry wrote:
> why not just make this a method of SettingsFunction?

Done.

http://codereview.chromium.org/7189029/diff/21001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_api.cc:59:
&GetStorageCallback::Run,
On 2011/06/23 19:14:11, Matt Perry wrote:
> nit: indent +2

Done.

http://codereview.chromium.org/7189029/diff/21001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_api.cc:59:
&GetStorageCallback::Run,
On 2011/06/23 19:14:11, Matt Perry wrote:
> nit: indent +2

Done.

http://codereview.chromium.org/7189029/diff/21001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_leveldb_storage.cc (right):

http://codereview.chromium.org/7189029/diff/21001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_leveldb_storage.cc:100:
leveldb::DB* db_;
On 2011/06/23 19:14:11, Matt Perry wrote:
> beware of lifetime here. there's no guarantee that db_ will live longer than
> ResultClosure. you need to use some sort of WeakPtr (careful, WeakPtr is not
> thread safe), or a refptr.

Hm.  Well this actually should never happen.  The only way db_ could be deleted
is if
 1. DeleteSoon() called
 2. Get/Set/Clear/Remove whatever called
 3. FILE thread deletes this -> deletes db_
 4. FILE thead closure uses db_
However... (2) following (1) should never occur, unless the
ExtensionSettingsLeveldbStorave object is used after its GetStorage() callback
has run, which would be a programmer error.

I.e. it's just as incorrect to use the object after DeleteSoon() is called as it
is to use after delete is called.

So really -- DeleteSoon() should be flipping an about-to-be-deleted flag, and
all methods checking that this is false.

http://codereview.chromium.org/7189029/diff/21001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_leveldb_storage.cc:109:
callback_->OnSuccess(settings_);
On 2011/06/23 19:14:11, Matt Perry wrote:
> make settings_ a scoped_ptr, and use settings_.release() here instead. it's
> better to have ptr ownership explicit.

Done.

http://codereview.chromium.org/7189029/diff/21001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_leveldb_storage.cc:257: Value*
value_;
On 2011/06/23 19:14:11, Matt Perry wrote:
> see comment about settings_

Done.

http://codereview.chromium.org/7189029/diff/21001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_leveldb_storage.cc:463: (new
DeleteStorageClosure(this))->Run();
On 2011/06/23 19:14:11, Matt Perry wrote:
> we have BrowserThread::DeleteSoon to handle this case

Done.

http://codereview.chromium.org/7189029/diff/21001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_noop_storage.cc (right):

http://codereview.chromium.org/7189029/diff/21001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_noop_storage.cc:16: class
SuccessResultClosure {
On 2011/06/23 19:14:11, Matt Perry wrote:
> it seems like a closure class is unnecessary here. you could do the same with
a
> simple static function:
> 
> static CallOnSuccess(ExtensionSettingsStorage::Callback* callback,
> DictionaryValue* settings) {
>   callback->OnSuccess(settings);
>   delete callback;
> }
> 
> and use it like:
> BrowserThread::PostTask(NewRunnableFunction(CallOnSuccess, callback,
settings));
> 
> (or whatever the equivalent base::Bind() syntax is - that's newer and I'm not
> familiar with it yet.)

Done.

Yeah it all came from me not realising how easy it is to curry arguments to
functions with base::Bind.  Thanks.

http://codereview.chromium.org/7189029/diff/21001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_storage.h (right):

http://codereview.chromium.org/7189029/diff/21001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_storage.h:45: virtual void
DestroyEventually() = 0;
On 2011/06/23 19:14:11, Matt Perry wrote:
> nit: name this DeleteSoon to be consistent with other cases like this.

Done.

http://codereview.chromium.org/7189029/diff/21001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_storage_cache.cc (right):

http://codereview.chromium.org/7189029/diff/21001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_storage_cache.cc:82:
DictionaryValue* cache_;
On 2011/06/23 19:14:11, Matt Perry wrote:
> who owns this ptr? it seems it won't get deleted in some instances, and in
> others, it may get deleted too soon.

cache is owned by ExtensionSettingsStorageCache.  If that is deleted then this
will blow up I guess; I'll make it a WeakPtr.

http://codereview.chromium.org/7189029/diff/21001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_storage_cache.cc:96: delete
callback_;
On 2011/06/23 19:14:11, Matt Perry wrote:
> make callback_ a scoped_ptr

Done.

[Matt Perry, 2011-06-29 18:08:11]

http://codereview.chromium.org/7189029/diff/16001/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/7189029/diff/16001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:712: "type": "any",
On 2011/06/27 08:51:02, kalman1 wrote:
> On 2011/06/23 18:11:45, Matt Perry wrote:
> > you should use the "choices" key rather than an "any" type. See
bookmarks.get.
> > 
> > ditto for set and remove
> 
> Done.
> 
> Though only for remove, not set?

Oops, you're right.

http://codereview.chromium.org/7189029/diff/21001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_leveldb_storage.cc (right):

http://codereview.chromium.org/7189029/diff/21001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_leveldb_storage.cc:100:
leveldb::DB* db_;
On 2011/06/27 08:51:02, kalman1 wrote:
> On 2011/06/23 19:14:11, Matt Perry wrote:
> > beware of lifetime here. there's no guarantee that db_ will live longer than
> > ResultClosure. you need to use some sort of WeakPtr (careful, WeakPtr is not
> > thread safe), or a refptr.
> 
> Hm.  Well this actually should never happen.  The only way db_ could be
deleted
> is if
>  1. DeleteSoon() called
>  2. Get/Set/Clear/Remove whatever called
>  3. FILE thread deletes this -> deletes db_
>  4. FILE thead closure uses db_
> However... (2) following (1) should never occur, unless the
> ExtensionSettingsLeveldbStorave object is used after its GetStorage() callback
> has run, which would be a programmer error.
> 
> I.e. it's just as incorrect to use the object after DeleteSoon() is called as
it
> is to use after delete is called.
> 
> So really -- DeleteSoon() should be flipping an about-to-be-deleted flag, and
> all methods checking that this is false.

I see. So you're saying RunOnFileThread is the only thing that touches db_, and
it is guaranteed to finish before the DeleteSoon task is processed. That makes
sense. Could you add a comment explaining that db_ should only be accessed from
within RunOnFileThread?

http://codereview.chromium.org/7189029/diff/31001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings.cc (right):

http://codereview.chromium.org/7189029/diff/31001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:75: base::Unretained(new
Callback(callback)),
you're leaking the new Callback here. I don't believe Callback deletes itself
after running. you need a helper function to dispatch to Run and then delete.

http://codereview.chromium.org/7189029/diff/31001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:88: AddRef();
base::Bind normally handles this for you - except your use of Unretained
prevents it. I guess you're doing this because you can't AddRef/Release on the
FILE thread? if so, please add a quick comment explaining that.

http://codereview.chromium.org/7189029/diff/31001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_api.cc (right):

http://codereview.chromium.org/7189029/diff/31001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_api.cc:41: AddRef();
actually I was wrong before - base::Bind does the AddRef/Release for you if you
get rid of the base::Unretained wrapper.

http://codereview.chromium.org/7189029/diff/31001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_storage_cache.cc (right):

http://codereview.chromium.org/7189029/diff/31001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_storage_cache.cc:155:
CacheModifyingCallback::Create(callback, cache_ptr_factory_));
you can just pass in cache_ptr_factory_.GetWeakPtr(), right?

http://codereview.chromium.org/7189029/diff/31001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_storage_unittest.h (right):

http://codereview.chromium.org/7189029/diff/31001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_storage_unittest.h:41: Value*
val1_;
use scoped_ptr so you don't have to manually delete

http://codereview.chromium.org/7189029/diff/31001/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/7189029/diff/31001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:780: "description": "Callback
with an empty settings object, or on failure (in which case lastError will be
set).",
I didn't notice this before, but why are we passing an empty object? We can pass
no arguments, and just have lastError set on an error.

http://codereview.chromium.org/7189029/diff/31001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:802: "description": "Callback
with an empty settings object, or on failure (in which case lastError will be
set).",
ditto

[not at google - send to devlin, 2011-08-03 06:36:51]

I'm going to merge now, so the diff will presumably be quite different (it's
been a month)?  I'm not sure how rietveld handles this.

http://codereview.chromium.org/7189029/diff/21001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_leveldb_storage.cc (right):

http://codereview.chromium.org/7189029/diff/21001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_leveldb_storage.cc:100:
leveldb::DB* db_;
On 2011/06/29 18:08:11, Matt Perry wrote:
> On 2011/06/27 08:51:02, kalman1 wrote:
> > On 2011/06/23 19:14:11, Matt Perry wrote:
> > > beware of lifetime here. there's no guarantee that db_ will live longer
than
> > > ResultClosure. you need to use some sort of WeakPtr (careful, WeakPtr is
not
> > > thread safe), or a refptr.
> > 
> > Hm.  Well this actually should never happen.  The only way db_ could be
> deleted
> > is if
> >  1. DeleteSoon() called
> >  2. Get/Set/Clear/Remove whatever called
> >  3. FILE thread deletes this -> deletes db_
> >  4. FILE thead closure uses db_
> > However... (2) following (1) should never occur, unless the
> > ExtensionSettingsLeveldbStorave object is used after its GetStorage()
callback
> > has run, which would be a programmer error.
> > 
> > I.e. it's just as incorrect to use the object after DeleteSoon() is called
as
> it
> > is to use after delete is called.
> > 
> > So really -- DeleteSoon() should be flipping an about-to-be-deleted flag,
and
> > all methods checking that this is false.
> 
> I see. So you're saying RunOnFileThread is the only thing that touches db_,
and
> it is guaranteed to finish before the DeleteSoon task is processed. That makes
> sense. Could you add a comment explaining that db_ should only be accessed
from
> within RunOnFileThread?

Done.

http://codereview.chromium.org/7189029/diff/31001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings.cc (right):

http://codereview.chromium.org/7189029/diff/31001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:75: base::Unretained(new
Callback(callback)),
On 2011/06/29 18:08:11, Matt Perry wrote:
> you're leaking the new Callback here. I don't believe Callback deletes itself
> after running. you need a helper function to dispatch to Run and then delete.

Done.

http://codereview.chromium.org/7189029/diff/31001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:88: AddRef();
On 2011/06/29 18:08:11, Matt Perry wrote:
> base::Bind normally handles this for you - except your use of Unretained
> prevents it. I guess you're doing this because you can't AddRef/Release on the
> FILE thread? if so, please add a quick comment explaining that.

Actually it should be fine to just let Bind() refcount it.

http://codereview.chromium.org/7189029/diff/31001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_api.cc (right):

http://codereview.chromium.org/7189029/diff/31001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_api.cc:41: AddRef();
On 2011/06/29 18:08:11, Matt Perry wrote:
> actually I was wrong before - base::Bind does the AddRef/Release for you if
you
> get rid of the base::Unretained wrapper.

Done.

http://codereview.chromium.org/7189029/diff/31001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_storage_cache.cc (right):

http://codereview.chromium.org/7189029/diff/31001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_storage_cache.cc:155:
CacheModifyingCallback::Create(callback, cache_ptr_factory_));
On 2011/06/29 18:08:11, Matt Perry wrote:
> you can just pass in cache_ptr_factory_.GetWeakPtr(), right?

Done.

http://codereview.chromium.org/7189029/diff/31001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_storage_unittest.h (right):

http://codereview.chromium.org/7189029/diff/31001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_storage_unittest.h:41: Value*
val1_;
On 2011/06/29 18:08:11, Matt Perry wrote:
> use scoped_ptr so you don't have to manually delete

Done.

http://codereview.chromium.org/7189029/diff/31001/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/7189029/diff/31001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:780: "description": "Callback
with an empty settings object, or on failure (in which case lastError will be
set).",
On 2011/06/29 18:08:11, Matt Perry wrote:
> I didn't notice this before, but why are we passing an empty object? We can
pass
> no arguments, and just have lastError set on an error.

Initially so that, C++ implementation wise, all methods can use the same
callback types; just a DictionaryValue.

From an API standpoint this arguably doesn't make much sense, but I can see it
being useful even for a developer to have all these functions have the same
type, if function pointers are passed around... or something.

[Matt Perry, 2011-08-03 19:33:52]

http://codereview.chromium.org/7189029/diff/31001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings.cc (right):

http://codereview.chromium.org/7189029/diff/31001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:88: AddRef();
On 2011/08/03 06:36:51, kalman1 wrote:
> On 2011/06/29 18:08:11, Matt Perry wrote:
> > base::Bind normally handles this for you - except your use of Unretained
> > prevents it. I guess you're doing this because you can't AddRef/Release on
the
> > FILE thread? if so, please add a quick comment explaining that.
> 
> Actually it should be fine to just let Bind() refcount it.

if Bind holds the last reference, then the Release on the FILE thread will cause
the object to be deleted on that thread.

http://codereview.chromium.org/7189029/diff/31001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:88: AddRef();
On 2011/08/03 06:36:51, kalman1 wrote:
> On 2011/06/29 18:08:11, Matt Perry wrote:
> > base::Bind normally handles this for you - except your use of Unretained
> > prevents it. I guess you're doing this because you can't AddRef/Release on
the
> > FILE thread? if so, please add a quick comment explaining that.
> 
> Actually it should be fine to just let Bind() refcount it.

Bind will release its reference when the task is run, on the FILE thread. Since
this class doesn't use the thread-safe RefCountedThreadSafe, that won't work.
All refcounting must be on the same thread.

[not at google - send to devlin, 2011-08-03 22:06:55]

http://codereview.chromium.org/7189029/diff/31001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings.cc (right):

http://codereview.chromium.org/7189029/diff/31001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:88: AddRef();
On 2011/08/03 19:33:52, Matt Perry wrote:
> On 2011/08/03 06:36:51, kalman1 wrote:
> > On 2011/06/29 18:08:11, Matt Perry wrote:
> > > base::Bind normally handles this for you - except your use of Unretained
> > > prevents it. I guess you're doing this because you can't AddRef/Release on
> the
> > > FILE thread? if so, please add a quick comment explaining that.
> > 
> > Actually it should be fine to just let Bind() refcount it.
> 
> Bind will release its reference when the task is run, on the FILE thread.
Since
> this class doesn't use the thread-safe RefCountedThreadSafe, that won't work.
> All refcounting must be on the same thread.

Changed to RefCountedThreadSafe.

The last reference should never be able to be let go from this method (and .:
not from the FILE thread), because at the end there is a Bind to this again, and
the reference is only Released after the method has run.

[Matt Perry, 2011-08-03 22:57:47]

LGTM, though I didn't review that closely post-merge.

http://codereview.chromium.org/7189029/diff/31001/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/7189029/diff/31001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:780: "description": "Callback
with an empty settings object, or on failure (in which case lastError will be
set).",
On 2011/08/03 06:36:51, kalman1 wrote:
> On 2011/06/29 18:08:11, Matt Perry wrote:
> > I didn't notice this before, but why are we passing an empty object? We can
> pass
> > no arguments, and just have lastError set on an error.
> 
> Initially so that, C++ implementation wise, all methods can use the same
> callback types; just a DictionaryValue.

If you pass NULL to that callback (which then sets the
ExtensionFunction::result_ to NULL), it will pass no arguments to the callback.

http://codereview.chromium.org/7189029/diff/36051/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_storage_cache.cc (right):

http://codereview.chromium.org/7189029/diff/36051/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_storage_cache.cc:46: //
TODO(kalman): shouldn't expose the factory here.  just the weakptr.
TODO can go away

[not at google - send to devlin, 2011-08-04 00:59:06]

merge was very clean.

http://codereview.chromium.org/7189029/diff/31001/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/7189029/diff/31001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:780: "description": "Callback
with an empty settings object, or on failure (in which case lastError will be
set).",
On 2011/08/03 22:57:47, Matt Perry wrote:
> On 2011/08/03 06:36:51, kalman1 wrote:
> > On 2011/06/29 18:08:11, Matt Perry wrote:
> > > I didn't notice this before, but why are we passing an empty object? We
can
> > pass
> > > no arguments, and just have lastError set on an error.
> > 
> > Initially so that, C++ implementation wise, all methods can use the same
> > callback types; just a DictionaryValue.
> 
> If you pass NULL to that callback (which then sets the
> ExtensionFunction::result_ to NULL), it will pass no arguments to the
callback.

Done.

http://codereview.chromium.org/7189029/diff/31001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:802: "description": "Callback
with an empty settings object, or on failure (in which case lastError will be
set).",
On 2011/06/29 18:08:11, Matt Perry wrote:
> ditto

Done.

http://codereview.chromium.org/7189029/diff/36051/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_storage_cache.cc (right):

http://codereview.chromium.org/7189029/diff/36051/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_storage_cache.cc:46: //
TODO(kalman): shouldn't expose the factory here.  just the weakptr.
On 2011/08/03 22:57:48, Matt Perry wrote:
> TODO can go away

Done.

http://codereview.chromium.org/7189029/diff/36051/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_storage_cache.cc:107: //
TODO(kalman): don't make this a whole class.  unnecessary.
(also deleted this)

[not at google - send to devlin, 2011-08-04 04:23:22]

Adding reviewers from OWNERS files for things outside extensions.

Not sure if those without set noparent are strictly necessary.

[Miranda Callahan, 2011-08-08 08:58:23]

On 2011/08/04 04:23:22, kalman1 wrote:
> Adding reviewers from OWNERS files for things outside extensions.
> 
> Not sure if those without set noparent are strictly necessary.

Profiles LGTM, although this service will need to be moved out of profile and
into a factory singleton as part of the general profiles refactoring. I think
that this particular refactoring will have to happen as part of the Extension
service refactoring, so I'll file a separate bug for this.

[not at google - send to devlin, 2011-08-10 07:42:42]

I had to fiddle with a few destructors to make clang happy and compile.  Fairly
trivial changes though, so I'm going to try to submit.

[commit-bot: I haz the power, 2011-08-10 09:24:24]

Change committed as 96159