Refactored SettingsFrontend and forwarding of calls to the backends.

Refactored SettingsFrontend and forwarding of calls to the backends.

Added a SettingsNamespaceFrontend interface for namespace frontends, that allows different namespaces to forward StorageCallbacks in their own way.
The LOCAL and SYNC namespaces use an implementation that does the same as before, but the new MANAGED namespace can now be implemented using a ValueStore that loads settings from policies, in the UI thread.

BUG=108992
TEST=Everything works as before, all tests are green


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

[Joao da Silva, 2012-07-17 13:55:49]

Here's the initial refactoring to enable the 'managed' namespace.

This is basically making a new interface out of
SettingsFrontend::BackendWrappers to abstract the RunWithStorage() call (so that
the 'managed' frontend can do that in a completely different way). The previous
BackendWrapper stuff was moved to the SettingsStorageFrontend implementation of
that interface, and everything should work exactly the same as before.

The DCHECK(CurrentlyOn(FILE)) were removed because the 'managed' namespace will
invoke that from UI. The SettingsFunctions just get a ValueStore and can assume
"now is a good time to use that store" :-)

Please review, thanks!

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

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
File chrome/browser/extensions/settings/settings_frontend.cc (right):

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
chrome/browser/extensions/settings/settings_frontend.cc:28: namespace {
Revisiting this file after going through the others. So yeah, could all the
threading stuff be done in here, make those what-were-backend interfaces
asynchronous?

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

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
chrome/browser/extensions/settings/settings_frontend.cc:108: sync_frontend_ =
Having this as a member variable *and* in the map is a bit of a strange
ownership situation. Can you just put it in the map as normal, then static_cast
to a SettingsStorageFrontend when it's used below?

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
chrome/browser/extensions/settings/settings_frontend.cc:123:
STLDeleteValues(&frontends_);
I prefer making frontend_ contain linked_ptrs rather than raw pointers.

(hot topic right now, apparently)

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
chrome/browser/extensions/settings/settings_frontend.cc:147: DCHECK(frontend);
CHECK not DCHECK

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
chrome/browser/extensions/settings/settings_frontend.cc:151: if (!extension ||
!frontend) {
!frontend check should be unnecessary

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
chrome/browser/extensions/settings/settings_frontend.cc:152: callback.Run(NULL);
The reason for CallbackWithNullStorage is so that this method isn't re-entrant.
I'd prefer to maintain that contract if possible, I think it's a source of
subtle bugs where it's assumed these asynchronous methods are indeed
asynchronous.

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

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

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
File chrome/browser/extensions/settings/settings_frontend.h (right):

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
chrome/browser/extensions/settings/settings_frontend.h:42: // Ownership NOT
taken.
probably an unnecessary comment in the first place. Twas a time before
scoped_ptr had Pass.

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
File chrome/browser/extensions/settings/settings_managed_frontend.cc (right):

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
chrome/browser/extensions/settings/settings_managed_frontend.cc:14:
SettingsManagedFrontend::SettingsManagedFrontend(Profile* profile) {}
so I presume you'll be using the Profile for something later then :)

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
File chrome/browser/extensions/settings/settings_managed_frontend.h (right):

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
chrome/browser/extensions/settings/settings_managed_frontend.h:17: class
SettingsManagedFrontend : public SettingsNamespaceFrontend {
ManagedSettingsFrontend?

(sorry to suggest a naming change, but "SettingsManaged" sounds wrong and
"ManagedSettings" sounds right)

or ManagedValueStoreCache if you go with the ValueStoreCache name.

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
File chrome/browser/extensions/settings/settings_namespace.cc (right):

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
chrome/browser/extensions/settings/settings_namespace.cc:25: default:
NOTREACHED();
nit (to myself! oh well) no default branch, prevents the compiler from picking
up on errors like this. NOTREACHED() before the "return std::string()" at the
end though.

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
chrome/browser/extensions/settings/settings_namespace.cc:35: else if
(namespace_string == kManagedNamespace)
nit, also to myself: remove the "else"s

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
File chrome/browser/extensions/settings/settings_namespace_frontend.h (right):

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
chrome/browser/extensions/settings/settings_namespace_frontend.h:21: class
SettingsNamespaceFrontend {
Name is rather indistinguishable from SettingsFrontend, also, especially vague.

So I guess this class is responsible for returning ValueStore instances on the
correct thread, and likewise deleting them on the right thread. And making sure
that only one is active per extension. How about... ValueStoreCache?

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
chrome/browser/extensions/settings/settings_namespace_frontend.h:32: 
If you add GetNaturalThread or whatever, a contract of this could be that it's
called on the correct thread (SettingsFrontend can easily arrange that).

Then this method could be synchronous i.e. just return the StorageCallback
rather than calling back with it. That would cut out a lot of the boilerplate in
SettingsStorageFrontend.

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
File chrome/browser/extensions/settings/settings_storage_frontend.cc (right):

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
chrome/browser/extensions/settings/settings_storage_frontend.cc:22: 
Yeah, so I think a lot of this file will change/simplify if the interface is
synchronous (and always expected to be on that the FILE thread).

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
chrome/browser/extensions/settings/settings_storage_frontend.cc:36:
WeakUnlimitedSettingsStorage unlimited_storage(
If it's made synchronous then this WeakUnlimited thing needs to go away. I have
no idea why I did that, it's not like the unlimitedStorage permission will
change.

Would need to thread through the unlimitedStorage permission to where it's
created I suppose. Or just thread through the (const) Extension rather than the
extension_id, looks threadsafe to me.

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
File chrome/browser/extensions/settings/settings_storage_frontend.h (right):

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
chrome/browser/extensions/settings/settings_storage_frontend.h:25: class
SettingsStorageFrontend : public SettingsNamespaceFrontend {
If you go with the ValueStoreCache thing, call this SyncOrLocalValueStoreCache?
Or some variation on that.

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

Thanks for the comments, please have another look.

There are some leaks in the unit_tests due to the delayed destruction of the
ValueStoreCaches; the last patch set should have plugged most of them, I'll run
this under valgrind locally later.

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
File chrome/browser/extensions/settings/settings_frontend.cc (right):

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
chrome/browser/extensions/settings/settings_frontend.cc:28: namespace {
On 2012/07/18 07:46:18, kalman wrote:
> Revisiting this file after going through the others. So yeah, could all the
> threading stuff be done in here, make those what-were-backend interfaces
> asynchronous?
> 
> That would make the Managed thing end up posting back to the same thread, at
> least naively, but I think that's a good thing to guarantee that the callbacks
> aren't reentrant.

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

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

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
chrome/browser/extensions/settings/settings_frontend.cc:108: sync_frontend_ =
On 2012/07/18 07:46:18, kalman wrote:
> Having this as a member variable *and* in the map is a bit of a strange
> ownership situation. Can you just put it in the map as normal, then
static_cast
> to a SettingsStorageFrontend when it's used below?

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

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
chrome/browser/extensions/settings/settings_frontend.cc:123:
STLDeleteValues(&frontends_);
On 2012/07/18 07:46:18, kalman wrote:
> I prefer making frontend_ contain linked_ptrs rather than raw pointers.
> 
> (hot topic right now, apparently)

Done.

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
chrome/browser/extensions/settings/settings_frontend.cc:147: DCHECK(frontend);
On 2012/07/18 07:46:18, kalman wrote:
> CHECK not DCHECK

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

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
chrome/browser/extensions/settings/settings_frontend.cc:151: if (!extension ||
!frontend) {
On 2012/07/18 07:46:18, kalman wrote:
> !frontend check should be unnecessary

Done.

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
chrome/browser/extensions/settings/settings_frontend.cc:152: callback.Run(NULL);
On 2012/07/18 07:46:18, kalman wrote:
> The reason for CallbackWithNullStorage is so that this method isn't
re-entrant.
> I'd prefer to maintain that contract if possible, I think it's a source of
> subtle bugs where it's assumed these asynchronous methods are indeed
> asynchronous.

Thanks for clarifying this issue. Done.

> 
> Oh, and the other reason is that callback implementations should always be
> called on the correct thread. So this isn't quite right.
> 
> Could you add a method like GetNaturalThread or something (maybe there's some
> chrome idiom for what that's called) to SettingsNamespaceFrontend and post to
> CallbackWithNullStorage with that?

Done.

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
File chrome/browser/extensions/settings/settings_frontend.h (right):

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
chrome/browser/extensions/settings/settings_frontend.h:42: // Ownership NOT
taken.
On 2012/07/18 07:46:18, kalman wrote:
> probably an unnecessary comment in the first place. Twas a time before
> scoped_ptr had Pass.

Removed these comments.

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
File chrome/browser/extensions/settings/settings_managed_frontend.cc (right):

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
chrome/browser/extensions/settings/settings_managed_frontend.cc:14:
SettingsManagedFrontend::SettingsManagedFrontend(Profile* profile) {}
On 2012/07/18 07:46:18, kalman wrote:
> so I presume you'll be using the Profile for something later then :)

Yep :-) profile->GetPolicyService() is where the stuff comes from later.

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
File chrome/browser/extensions/settings/settings_managed_frontend.h (right):

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
chrome/browser/extensions/settings/settings_managed_frontend.h:17: class
SettingsManagedFrontend : public SettingsNamespaceFrontend {
On 2012/07/18 07:46:18, kalman wrote:
> ManagedSettingsFrontend?
> 
> (sorry to suggest a naming change, but "SettingsManaged" sounds wrong and
> "ManagedSettings" sounds right)
> 
> or ManagedValueStoreCache if you go with the ValueStoreCache name.

Renamed to ManagedValueStoreCache.

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
File chrome/browser/extensions/settings/settings_namespace.cc (right):

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
chrome/browser/extensions/settings/settings_namespace.cc:25: default:
NOTREACHED();
On 2012/07/18 07:46:18, kalman wrote:
> nit (to myself! oh well) no default branch, prevents the compiler from picking
> up on errors like this. NOTREACHED() before the "return std::string()" at the
> end though.

Done.

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
chrome/browser/extensions/settings/settings_namespace.cc:35: else if
(namespace_string == kManagedNamespace)
On 2012/07/18 07:46:18, kalman wrote:
> nit, also to myself: remove the "else"s

Also done.

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
File chrome/browser/extensions/settings/settings_namespace_frontend.h (right):

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
chrome/browser/extensions/settings/settings_namespace_frontend.h:21: class
SettingsNamespaceFrontend {
On 2012/07/18 07:46:18, kalman wrote:
> Name is rather indistinguishable from SettingsFrontend, also, especially
vague.
> 
> So I guess this class is responsible for returning ValueStore instances on the
> correct thread, and likewise deleting them on the right thread. And making
sure
> that only one is active per extension. How about... ValueStoreCache?

It's hard to name this one :-) That's a much better name, done.

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
chrome/browser/extensions/settings/settings_namespace_frontend.h:32: 
On 2012/07/18 07:46:18, kalman wrote:
> If you add GetNaturalThread or whatever, a contract of this could be that it's
> called on the correct thread (SettingsFrontend can easily arrange that).
> 
> Then this method could be synchronous i.e. just return the StorageCallback
> rather than calling back with it. That would cut out a lot of the boilerplate
in
> SettingsStorageFrontend.

Done. The contract for this interface is now that it is created from UI, but
from then on lives on its preferred thread and all methods are invoked there
(except GetTaskRunner). In particular, the dtor is also executed on the
preferred thread.

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
File chrome/browser/extensions/settings/settings_storage_frontend.cc (right):

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
chrome/browser/extensions/settings/settings_storage_frontend.cc:22: 
On 2012/07/18 07:46:18, kalman wrote:
> Yeah, so I think a lot of this file will change/simplify if the interface is
> synchronous (and always expected to be on that the FILE thread).

Yep, managed to do away with the BackendWrapper once every call is guaranteed to
come on FILE.

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
chrome/browser/extensions/settings/settings_storage_frontend.cc:36:
WeakUnlimitedSettingsStorage unlimited_storage(
On 2012/07/18 07:46:18, kalman wrote:
> If it's made synchronous then this WeakUnlimited thing needs to go away. I
have
> no idea why I did that, it's not like the unlimitedStorage permission will
> change.
> 
> Would need to thread through the unlimitedStorage permission to where it's
> created I suppose. Or just thread through the (const) Extension rather than
the
> extension_id, looks threadsafe to me.

I'm not entirely sure of what you're proposing to do about this. I've kept it
mostly as-is; removing the WeakUnlimitedSettingsStorage decorator would require
some other means of pushing the IGNORE_QUOTA flag into the
SyncableSettingsStorage within the SettingsBackend. I'd rather leave that for
another CL, this one is already large-ish :-)

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
File chrome/browser/extensions/settings/settings_storage_frontend.h (right):

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
chrome/browser/extensions/settings/settings_storage_frontend.h:25: class
SettingsStorageFrontend : public SettingsNamespaceFrontend {
On 2012/07/18 07:46:18, kalman wrote:
> If you go with the ValueStoreCache thing, call this
SyncOrLocalValueStoreCache?
> Or some variation on that.

Done.

[not at google - send to devlin, 2012-07-19 06:43:17]

lgtm

You'll need to update the patch description.

Btw, I think that TEST= is for verifying bug fixes, so doesn't really belong in
a feature patch.

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
File chrome/browser/extensions/settings/settings_managed_frontend.cc (right):

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
chrome/browser/extensions/settings/settings_managed_frontend.cc:14:
SettingsManagedFrontend::SettingsManagedFrontend(Profile* profile) {}
On 2012/07/18 21:40:25, Joao da Silva wrote:
> On 2012/07/18 07:46:18, kalman wrote:
> > so I presume you'll be using the Profile for something later then :)
> 
> Yep :-) profile->GetPolicyService() is where the stuff comes from later.

Is it possible to only inject the PolicyService then, not the whole Profile?
Minimal dependencies, and all that.

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
File chrome/browser/extensions/settings/settings_storage_frontend.cc (right):

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
chrome/browser/extensions/settings/settings_storage_frontend.cc:36:
WeakUnlimitedSettingsStorage unlimited_storage(
On 2012/07/18 21:40:25, Joao da Silva wrote:
> On 2012/07/18 07:46:18, kalman wrote:
> > If it's made synchronous then this WeakUnlimited thing needs to go away. I
> have
> > no idea why I did that, it's not like the unlimitedStorage permission will
> > change.
> > 
> > Would need to thread through the unlimitedStorage permission to where it's
> > created I suppose. Or just thread through the (const) Extension rather than
> the
> > extension_id, looks threadsafe to me.
> 
> I'm not entirely sure of what you're proposing to do about this. I've kept it
> mostly as-is; removing the WeakUnlimitedSettingsStorage decorator would
require
> some other means of pushing the IGNORE_QUOTA flag into the
> SyncableSettingsStorage within the SettingsBackend. I'd rather leave that for
> another CL, this one is already large-ish :-)

Oh, I remember now. Yeah, ignore this.

http://codereview.chromium.org/10784035/diff/17002/chrome/browser/extensions/...
File chrome/browser/extensions/settings/managed_value_store_cache.h (right):

http://codereview.chromium.org/10784035/diff/17002/chrome/browser/extensions/...
chrome/browser/extensions/settings/managed_value_store_cache.h:19: explicit
ManagedValueStoreCache(Profile* profile);
Like I said can it just be given the policy-specific stuff it needs?

http://codereview.chromium.org/10784035/diff/17002/chrome/browser/extensions/...
chrome/browser/extensions/settings/managed_value_store_cache.h:28:
scoped_refptr<const Extension> extension) OVERRIDE;
Does forcing this to be a scoped_refptr make sense when passing it into this
method? To me this implies that ownership passing into this method, which isn't
the case. const Extension* should be fine, implementations can hold onto it if
they want.

http://codereview.chromium.org/10784035/diff/17002/chrome/browser/extensions/...
File chrome/browser/extensions/settings/settings_frontend.cc (right):

http://codereview.chromium.org/10784035/diff/17002/chrome/browser/extensions/...
chrome/browser/extensions/settings/settings_frontend.cc:135: static_cast<const
SyncOrLocalValueStoreCache*>(it->second);
(nothing wrong with the occasional cast)

http://codereview.chromium.org/10784035/diff/17002/chrome/browser/extensions/...
File chrome/browser/extensions/settings/value_store_cache.h (right):

http://codereview.chromium.org/10784035/diff/17002/chrome/browser/extensions/...
chrome/browser/extensions/settings/value_store_cache.h:38: scoped_refptr<const
Extension> extension) = 0;
see comment in the other file about making this const Extension*

[Joao da Silva, 2012-07-19 11:26:04]

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
File chrome/browser/extensions/settings/settings_managed_frontend.cc (right):

http://codereview.chromium.org/10784035/diff/1/chrome/browser/extensions/sett...
chrome/browser/extensions/settings/settings_managed_frontend.cc:14:
SettingsManagedFrontend::SettingsManagedFrontend(Profile* profile) {}
On 2012/07/19 06:43:17, kalman wrote:
> On 2012/07/18 21:40:25, Joao da Silva wrote:
> > On 2012/07/18 07:46:18, kalman wrote:
> > > so I presume you'll be using the Profile for something later then :)
> > 
> > Yep :-) profile->GetPolicyService() is where the stuff comes from later.
> 
> Is it possible to only inject the PolicyService then, not the whole Profile?
> Minimal dependencies, and all that.

Sure, done.

http://codereview.chromium.org/10784035/diff/17002/chrome/browser/extensions/...
File chrome/browser/extensions/settings/managed_value_store_cache.h (right):

http://codereview.chromium.org/10784035/diff/17002/chrome/browser/extensions/...
chrome/browser/extensions/settings/managed_value_store_cache.h:19: explicit
ManagedValueStoreCache(Profile* profile);
On 2012/07/19 06:43:17, kalman wrote:
> Like I said can it just be given the policy-specific stuff it needs?

Done.

http://codereview.chromium.org/10784035/diff/17002/chrome/browser/extensions/...
chrome/browser/extensions/settings/managed_value_store_cache.h:28:
scoped_refptr<const Extension> extension) OVERRIDE;
On 2012/07/19 06:43:17, kalman wrote:
> Does forcing this to be a scoped_refptr make sense when passing it into this
> method? To me this implies that ownership passing into this method, which
isn't
> the case. const Extension* should be fine, implementations can hold onto it if
> they want.

This method is invoked as an asynchronous callback, and the Extension might've
been unloaded in the UI thread by the time this executes on FILE, for example.
Not likely, but I think it's correct to make this a scoped_refptr in this case;
wdyt?

[not at google - send to devlin, 2012-07-19 12:29:51]

http://codereview.chromium.org/10784035/diff/17002/chrome/browser/extensions/...
File chrome/browser/extensions/settings/managed_value_store_cache.h (right):

http://codereview.chromium.org/10784035/diff/17002/chrome/browser/extensions/...
chrome/browser/extensions/settings/managed_value_store_cache.h:28:
scoped_refptr<const Extension> extension) OVERRIDE;
On 2012/07/19 11:26:04, Joao da Silva wrote:
> On 2012/07/19 06:43:17, kalman wrote:
> > Does forcing this to be a scoped_refptr make sense when passing it into this
> > method? To me this implies that ownership passing into this method, which
> isn't
> > the case. const Extension* should be fine, implementations can hold onto it
if
> > they want.
> 
> This method is invoked as an asynchronous callback, and the Extension might've
> been unloaded in the UI thread by the time this executes on FILE, for example.
> Not likely, but I think it's correct to make this a scoped_refptr in this
case;
> wdyt?

Ah I see. I just read through the base/bind related files was rather baffled
(it's very easy to read but very hard to understand), but base/callback explains
it; even if this method were to take a "const scoped_refptr&" the bound Callback
only holds onto the reference, not the object. So that wouldn't be safe either.

It's a very subtle implementation detail; could you add a comment in the header
file?

[Joao da Silva, 2012-07-19 13:17:51]

Thanks for the review! All the comments have been addressed by now, and if the
memory bot is happy I'll land this.

http://codereview.chromium.org/10784035/diff/17002/chrome/browser/extensions/...
File chrome/browser/extensions/settings/managed_value_store_cache.h (right):

http://codereview.chromium.org/10784035/diff/17002/chrome/browser/extensions/...
chrome/browser/extensions/settings/managed_value_store_cache.h:28:
scoped_refptr<const Extension> extension) OVERRIDE;
On 2012/07/19 12:29:52, kalman wrote:
> On 2012/07/19 11:26:04, Joao da Silva wrote:
> > On 2012/07/19 06:43:17, kalman wrote:
> > > Does forcing this to be a scoped_refptr make sense when passing it into
this
> > > method? To me this implies that ownership passing into this method, which
> > isn't
> > > the case. const Extension* should be fine, implementations can hold onto
it
> if
> > > they want.
> > 
> > This method is invoked as an asynchronous callback, and the Extension
might've
> > been unloaded in the UI thread by the time this executes on FILE, for
example.
> > Not likely, but I think it's correct to make this a scoped_refptr in this
> case;
> > wdyt?
> 
> Ah I see. I just read through the base/bind related files was rather baffled
> (it's very easy to read but very hard to understand), but base/callback
explains
> it; even if this method were to take a "const scoped_refptr&" the bound
Callback
> only holds onto the reference, not the object. So that wouldn't be safe
either.
> 
> It's a very subtle implementation detail; could you add a comment in the
header
> file?
> 

Yep, done.

[commit-bot: I haz the power, 2012-07-19 15:44:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/10784035/11006

[commit-bot: I haz the power, 2012-07-19 17:22:44]

Change committed as 147464