Clean up threading model of external extension providers

Clean up threading model of external extension providers

Move blocking IO of loading the lists of external extensions from the UI thread to the FILE thread. Remove the ExternalPolicyExtensionProvider-specific parts from ExtensionsService. Side-effects: reduce the number of PostTask operations from 9 to 4. Trigger uninstall of external extensions immediately after they are removed from a policy.

BUG=65107, 63667
TEST=ExtensionManagementTest.ExternalPolicyRefresh for the new functionality, lot of other extension tests are updated

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

[gfeher, 2011-01-01 23:00:17]

Hi, please review my rework of the external extension system. I don't have any
short-term pressure to land such thing, but I think this is good for the longer
term.

It passes the trybots and I've tested the affected functionality manually on
Windows and Linux.

This CL mostly keeps the original interface between the external providers and
the extension system. Changing that interface will be easier after this CL,
because this moves the provider<->extension sys. interaction entirely to the UI
thread.

[jochen (gone - plz use gerrit), 2011-01-04 12:38:01]

only had a quick look at it

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_management_browsertest.cc (right):

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_management_browsertest.cc:375: for (i =
extensions->begin(); i != extensions->end(); ++i) {
no { }

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_management_browsertest.cc:376:
EXPECT_FALSE("ogjcoiohnmldgjemafoockdghcjciccf" == (*i)->id());
EXPECT_NE

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
File chrome/browser/extensions/external_extension_loader.h (right):

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
chrome/browser/extensions/external_extension_loader.h:66: };
disallow copy and assign?

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
File chrome/browser/extensions/external_extension_provider.h (right):

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
chrome/browser/extensions/external_extension_provider.h:19: class
ExternalExtensionProvider {
The coding style wants pure virtual interfaces to be named FooInterface

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
chrome/browser/extensions/external_extension_provider.h:25: class Visitor {
same here

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
File chrome/browser/extensions/external_extension_provider_impl.h (right):

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
chrome/browser/extensions/external_extension_provider_impl.h:94: };
disallow copy and assign?

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
File chrome/browser/extensions/external_policy_extension_loader.cc (right):

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
chrome/browser/extensions/external_policy_extension_loader.cc:38: :
profile_(profile) {
only indent 4 spaces

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
File chrome/browser/extensions/external_policy_extension_loader.h (right):

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
chrome/browser/extensions/external_policy_extension_loader.h:15: #include
"chrome/common/notification_type.h"
why do you need _type and _service?

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
chrome/browser/extensions/external_policy_extension_loader.h:30: //
NotificationObserver
... implementation.

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
chrome/browser/extensions/external_policy_extension_loader.h:47: };
disallow copy and assign?

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
File chrome/browser/extensions/external_pref_extension_loader.h (right):

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
chrome/browser/extensions/external_pref_extension_loader.h:22: explicit
ExternalPrefExtensionLoader();
no explicit

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
chrome/browser/extensions/external_pref_extension_loader.h:33: };
disallow copy and assign?

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
chrome/browser/extensions/external_pref_extension_loader.h:51: };
disallow copy and assign? also no empty line

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
File chrome/browser/extensions/external_registry_extension_loader_win.h (right):

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
chrome/browser/extensions/external_registry_extension_loader_win.h:13: explicit
ExternalRegistryExtensionLoader() {}
no explicit

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
chrome/browser/extensions/external_registry_extension_loader_win.h:24: };
disallow copy and assign?

[Sam Kerner (Chrome), 2011-01-04 14:06:00]

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_management_browsertest.cc (right):

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_management_browsertest.cc:376:
EXPECT_FALSE("ogjcoiohnmldgjemafoockdghcjciccf" == (*i)->id());
Use kExtensionId instead of the string constant.

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:1359: // if all of them are
finished. So first, we  all of them.
Comment missing a word?

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
File chrome/browser/extensions/external_extension_provider_impl.cc (right):

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
chrome/browser/extensions/external_extension_provider_impl.cc:60: if (!service_)
return;
Can this happen?  I see two ways:  User called ServiceShutdown(), or passed
service=NULL in the constructor.  Both seem like the sort of misuse that should
trigger a CHECK.  Is there another way I missed?

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
File chrome/browser/extensions/external_extension_provider_impl.h (right):

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
chrome/browser/extensions/external_extension_provider_impl.h:24: //
ExternalExtensionLoader. Instances leave their entire life on the UI thread.
leave -> live

[gfeher, 2011-01-04 23:37:09]

Addressed comments.

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_management_browsertest.cc (right):

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_management_browsertest.cc:375: for (i =
extensions->begin(); i != extensions->end(); ++i) {
On 2011/01/04 12:38:01, jochen wrote:
> no { }

Done.

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_management_browsertest.cc:376:
EXPECT_FALSE("ogjcoiohnmldgjemafoockdghcjciccf" == (*i)->id());
On 2011/01/04 12:38:01, jochen wrote:
> EXPECT_NE

Done.

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_management_browsertest.cc:376:
EXPECT_FALSE("ogjcoiohnmldgjemafoockdghcjciccf" == (*i)->id());
On 2011/01/04 14:06:00, Sam Kerner (Chrome) wrote:
> Use kExtensionId instead of the string constant.  

Done.

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:1359: // if all of them are
finished. So first, we  all of them.
On 2011/01/04 14:06:00, Sam Kerner (Chrome) wrote:
> Comment missing a word?

Sorry. Done.

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
File chrome/browser/extensions/external_extension_loader.h (right):

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
chrome/browser/extensions/external_extension_loader.h:66: };
On 2011/01/04 12:38:01, jochen wrote:
> disallow copy and assign?

Done.

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
File chrome/browser/extensions/external_extension_provider.h (right):

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
chrome/browser/extensions/external_extension_provider.h:19: class
ExternalExtensionProvider {
If I read correctly, the style only wants the Interface suffix if the class is
involved in multiple inheritance.
But this is the case now. -> Done.

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
chrome/browser/extensions/external_extension_provider.h:25: class Visitor {
On 2011/01/04 12:38:01, jochen wrote:
> same here

Done.

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
File chrome/browser/extensions/external_extension_provider_impl.cc (right):

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
chrome/browser/extensions/external_extension_provider_impl.cc:60: if (!service_)
return;
It is possible that ExtensionService calls ServiceShutdown while |loader_| is
working on the FILE thread. The loader will call |SetPrefs| after it has
finished with it's stuff.

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
File chrome/browser/extensions/external_extension_provider_impl.h (right):

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
chrome/browser/extensions/external_extension_provider_impl.h:24: //
ExternalExtensionLoader. Instances leave their entire life on the UI thread.
On 2011/01/04 14:06:00, Sam Kerner (Chrome) wrote:
> leave -> live

Done.

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
chrome/browser/extensions/external_extension_provider_impl.h:94: };
On 2011/01/04 12:38:01, jochen wrote:
> disallow copy and assign?

Done.

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
File chrome/browser/extensions/external_policy_extension_loader.cc (right):

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
chrome/browser/extensions/external_policy_extension_loader.cc:38: :
profile_(profile) {
On 2011/01/04 12:38:01, jochen wrote:
> only indent 4 spaces

Done.

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
File chrome/browser/extensions/external_policy_extension_loader.h (right):

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
chrome/browser/extensions/external_policy_extension_loader.h:15: #include
"chrome/common/notification_type.h"
On 2011/01/04 12:38:01, jochen wrote:
> why do you need _type and _service?

Done.

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
chrome/browser/extensions/external_policy_extension_loader.h:30: //
NotificationObserver
On 2011/01/04 12:38:01, jochen wrote:
> ... implementation.

Done.

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
chrome/browser/extensions/external_policy_extension_loader.h:47: };
On 2011/01/04 12:38:01, jochen wrote:
> disallow copy and assign?

Done.

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
File chrome/browser/extensions/external_pref_extension_loader.h (right):

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
chrome/browser/extensions/external_pref_extension_loader.h:22: explicit
ExternalPrefExtensionLoader();
On 2011/01/04 12:38:01, jochen wrote:
> no explicit

Done.

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
chrome/browser/extensions/external_pref_extension_loader.h:33: };
On 2011/01/04 12:38:01, jochen wrote:
> disallow copy and assign?

Done.

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
chrome/browser/extensions/external_pref_extension_loader.h:51: };
On 2011/01/04 12:38:01, jochen wrote:
> disallow copy and assign? also no empty line

Done.

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
File chrome/browser/extensions/external_registry_extension_loader_win.h (right):

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
chrome/browser/extensions/external_registry_extension_loader_win.h:13: explicit
ExternalRegistryExtensionLoader() {}
On 2011/01/04 12:38:01, jochen wrote:
> no explicit

Done.

http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
chrome/browser/extensions/external_registry_extension_loader_win.h:24: };
On 2011/01/04 12:38:01, jochen wrote:
> disallow copy and assign?

Done.

[Sam Kerner (Chrome), 2011-01-05 20:49:25]

LGTM

Given the number of reviewers, you might give the others a chance to take
another look, but I am happy with this.

On 2011/01/04 23:37:09, gfeher wrote:
> Addressed comments.
> 
>
http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
> File chrome/browser/extensions/extension_management_browsertest.cc (right):
> 
>
http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_management_browsertest.cc:375: for (i =
> extensions->begin(); i != extensions->end(); ++i) {
> On 2011/01/04 12:38:01, jochen wrote:
> > no { }
> 
> Done.
> 
>
http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_management_browsertest.cc:376:
> EXPECT_FALSE("ogjcoiohnmldgjemafoockdghcjciccf" == (*i)->id());
> On 2011/01/04 12:38:01, jochen wrote:
> > EXPECT_NE
> 
> Done.
> 
>
http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_management_browsertest.cc:376:
> EXPECT_FALSE("ogjcoiohnmldgjemafoockdghcjciccf" == (*i)->id());
> On 2011/01/04 14:06:00, Sam Kerner (Chrome) wrote:
> > Use kExtensionId instead of the string constant.  
> 
> Done.
> 
>
http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
> File chrome/browser/extensions/extension_service.cc (right):
> 
>
http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_service.cc:1359: // if all of them are
> finished. So first, we  all of them.
> On 2011/01/04 14:06:00, Sam Kerner (Chrome) wrote:
> > Comment missing a word?
> 
> Sorry. Done.
> 
>
http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
> File chrome/browser/extensions/external_extension_loader.h (right):
> 
>
http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
> chrome/browser/extensions/external_extension_loader.h:66: };
> On 2011/01/04 12:38:01, jochen wrote:
> > disallow copy and assign?
> 
> Done.
> 
>
http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
> File chrome/browser/extensions/external_extension_provider.h (right):
> 
>
http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
> chrome/browser/extensions/external_extension_provider.h:19: class
> ExternalExtensionProvider {
> If I read correctly, the style only wants the Interface suffix if the class is
> involved in multiple inheritance.
> But this is the case now. -> Done.
> 
>
http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
> chrome/browser/extensions/external_extension_provider.h:25: class Visitor {
> On 2011/01/04 12:38:01, jochen wrote:
> > same here
> 
> Done.
> 
>
http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
> File chrome/browser/extensions/external_extension_provider_impl.cc (right):
> 
>
http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
> chrome/browser/extensions/external_extension_provider_impl.cc:60: if
(!service_)
> return;
> It is possible that ExtensionService calls ServiceShutdown while |loader_| is
> working on the FILE thread. The loader will call |SetPrefs| after it has
> finished with it's stuff.
> 
>
http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
> File chrome/browser/extensions/external_extension_provider_impl.h (right):
> 
>
http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
> chrome/browser/extensions/external_extension_provider_impl.h:24: //
> ExternalExtensionLoader. Instances leave their entire life on the UI thread.
> On 2011/01/04 14:06:00, Sam Kerner (Chrome) wrote:
> > leave -> live
> 
> Done.
> 
>
http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
> chrome/browser/extensions/external_extension_provider_impl.h:94: };
> On 2011/01/04 12:38:01, jochen wrote:
> > disallow copy and assign?
> 
> Done.
> 
>
http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
> File chrome/browser/extensions/external_policy_extension_loader.cc (right):
> 
>
http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
> chrome/browser/extensions/external_policy_extension_loader.cc:38: :
> profile_(profile) {
> On 2011/01/04 12:38:01, jochen wrote:
> > only indent 4 spaces
> 
> Done.
> 
>
http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
> File chrome/browser/extensions/external_policy_extension_loader.h (right):
> 
>
http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
> chrome/browser/extensions/external_policy_extension_loader.h:15: #include
> "chrome/common/notification_type.h"
> On 2011/01/04 12:38:01, jochen wrote:
> > why do you need _type and _service?
> 
> Done.
> 
>
http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
> chrome/browser/extensions/external_policy_extension_loader.h:30: //
> NotificationObserver
> On 2011/01/04 12:38:01, jochen wrote:
> > ... implementation.
> 
> Done.
> 
>
http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
> chrome/browser/extensions/external_policy_extension_loader.h:47: };
> On 2011/01/04 12:38:01, jochen wrote:
> > disallow copy and assign?
> 
> Done.
> 
>
http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
> File chrome/browser/extensions/external_pref_extension_loader.h (right):
> 
>
http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
> chrome/browser/extensions/external_pref_extension_loader.h:22: explicit
> ExternalPrefExtensionLoader();
> On 2011/01/04 12:38:01, jochen wrote:
> > no explicit
> 
> Done.
> 
>
http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
> chrome/browser/extensions/external_pref_extension_loader.h:33: };
> On 2011/01/04 12:38:01, jochen wrote:
> > disallow copy and assign?
> 
> Done.
> 
>
http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
> chrome/browser/extensions/external_pref_extension_loader.h:51: };
> On 2011/01/04 12:38:01, jochen wrote:
> > disallow copy and assign? also no empty line
> 
> Done.
> 
>
http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
> File chrome/browser/extensions/external_registry_extension_loader_win.h
(right):
> 
>
http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
> chrome/browser/extensions/external_registry_extension_loader_win.h:13:
explicit
> ExternalRegistryExtensionLoader() {}
> On 2011/01/04 12:38:01, jochen wrote:
> > no explicit
> 
> Done.
> 
>
http://codereview.chromium.org/5742008/diff/34001/chrome/browser/extensions/e...
> chrome/browser/extensions/external_registry_extension_loader_win.h:24: };
> On 2011/01/04 12:38:01, jochen wrote:
> > disallow copy and assign?
> 
> Done.

[Aaron Boodman, 2011-01-05 20:58:08]

I will take a look at this today.

[Aaron Boodman, 2011-01-06 18:54:39]

I only skimmed this, it seems reasonable though.

Thanks a lot for the cleanup.

http://codereview.chromium.org/5742008/diff/79001/chrome/browser/extensions/e...
File chrome/browser/extensions/external_extension_loader.cc (right):

http://codereview.chromium.org/5742008/diff/79001/chrome/browser/extensions/e...
chrome/browser/extensions/external_extension_loader.cc:14:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
Don't be a wuss: s/DCHECK/CHECK/g

http://codereview.chromium.org/5742008/diff/79001/chrome/browser/extensions/e...
chrome/browser/extensions/external_extension_loader.cc:20: if (running_) return;
How would this ever get called twice?

http://codereview.chromium.org/5742008/diff/79001/chrome/browser/extensions/e...
File chrome/browser/extensions/external_extension_loader.h (right):

http://codereview.chromium.org/5742008/diff/79001/chrome/browser/extensions/e...
chrome/browser/extensions/external_extension_loader.h:51: Lock lock_;
It is rare to use Locks directly in Chromium code and usually indicates
something is wrong with the design.

Do you actually access this concurrently from multiple threads? It looks like
maybe this member is only used as a way to pass something from the file thread
to the ui thread. If so, I don't think the lock is necessary.

http://codereview.chromium.org/5742008/diff/79001/chrome/browser/extensions/e...
File chrome/browser/extensions/external_extension_provider_impl.h (right):

http://codereview.chromium.org/5742008/diff/79001/chrome/browser/extensions/e...
chrome/browser/extensions/external_extension_provider_impl.h:11: #include
"base/lock.h"
Doesn't appear to be used?

http://codereview.chromium.org/5742008/diff/79001/chrome/browser/extensions/e...
chrome/browser/extensions/external_extension_provider_impl.h:76: // Location for
external extensions that are provided by this provider from
Add a blank line above here.

http://codereview.chromium.org/5742008/diff/79001/chrome/browser/extensions/e...
File chrome/browser/extensions/external_extension_provider_interface.h (right):

http://codereview.chromium.org/5742008/diff/79001/chrome/browser/extensions/e...
chrome/browser/extensions/external_extension_provider_interface.h:19: class
ExternalExtensionProviderInterface {
We don't typically use the 'Interface' suffix in Chromium.

[gfeher, 2011-01-07 00:10:01]

Addressed all comments, except for the 'Interface' suffix. Jochen, please join
that discussion.

I am unable to run tryjobs until Monday. Please do another round of review
before then, because I'd like to avoid resolving merge conflicts.

Thanks,
Gábor

http://codereview.chromium.org/5742008/diff/79001/chrome/browser/extensions/e...
File chrome/browser/extensions/external_extension_loader.cc (right):

http://codereview.chromium.org/5742008/diff/79001/chrome/browser/extensions/e...
chrome/browser/extensions/external_extension_loader.cc:14:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2011/01/06 18:54:40, Aaron Boodman wrote:
> Don't be a wuss: s/DCHECK/CHECK/g

Done.

http://codereview.chromium.org/5742008/diff/79001/chrome/browser/extensions/e...
chrome/browser/extensions/external_extension_loader.cc:20: if (running_) return;
This can get called twice in case of ExternalPolicyExtensionLoader. But that
lives on the UI thread, so the two calls will never overlap.

I was trying to solve the problem when a loader is working on the FILE thread
and it is asked to start loading again. But this is not happening in the current
code. Whoever makes it happen will have more information to fix it the best way.

So I remove this check.

http://codereview.chromium.org/5742008/diff/79001/chrome/browser/extensions/e...
File chrome/browser/extensions/external_extension_loader.h (right):

http://codereview.chromium.org/5742008/diff/79001/chrome/browser/extensions/e...
chrome/browser/extensions/external_extension_loader.h:51: Lock lock_;
Done.

http://codereview.chromium.org/5742008/diff/79001/chrome/browser/extensions/e...
File chrome/browser/extensions/external_extension_provider_impl.h (right):

http://codereview.chromium.org/5742008/diff/79001/chrome/browser/extensions/e...
chrome/browser/extensions/external_extension_provider_impl.h:11: #include
"base/lock.h"
On 2011/01/06 18:54:40, Aaron Boodman wrote:
> Doesn't appear to be used?

Done.

http://codereview.chromium.org/5742008/diff/79001/chrome/browser/extensions/e...
chrome/browser/extensions/external_extension_provider_impl.h:76: // Location for
external extensions that are provided by this provider from
On 2011/01/06 18:54:40, Aaron Boodman wrote:
> Add a blank line above here.

Done.

http://codereview.chromium.org/5742008/diff/79001/chrome/browser/extensions/e...
File chrome/browser/extensions/external_extension_provider_interface.h (right):

http://codereview.chromium.org/5742008/diff/79001/chrome/browser/extensions/e...
chrome/browser/extensions/external_extension_provider_interface.h:19: class
ExternalExtensionProviderInterface {
According to the Chromium style guide, I should either put an Interface or a
Delegate suffix here. Jochen had pointed this out, and I didn't find any
loopholes to avoid this. (I've tried, though.)

Should I change this to Delegate, then?

[gfeher, 2011-01-10 19:56:54]

ping?

[jochen (gone - plz use gerrit), 2011-01-10 20:01:12]

On 2011/01/07 00:10:01, gfeher wrote:
> Addressed all comments, except for the 'Interface' suffix. Jochen, please join
> that discussion.
> 
> I am unable to run tryjobs until Monday. Please do another round of review
> before then, because I'd like to avoid resolving merge conflicts.
> 
> Thanks,
> Gábor
> 
>
http://codereview.chromium.org/5742008/diff/79001/chrome/browser/extensions/e...
> File chrome/browser/extensions/external_extension_loader.cc (right):
> 
>
http://codereview.chromium.org/5742008/diff/79001/chrome/browser/extensions/e...
> chrome/browser/extensions/external_extension_loader.cc:14:
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
> On 2011/01/06 18:54:40, Aaron Boodman wrote:
> > Don't be a wuss: s/DCHECK/CHECK/g
> 
> Done.
> 
>
http://codereview.chromium.org/5742008/diff/79001/chrome/browser/extensions/e...
> chrome/browser/extensions/external_extension_loader.cc:20: if (running_)
return;
> This can get called twice in case of ExternalPolicyExtensionLoader. But that
> lives on the UI thread, so the two calls will never overlap.
> 
> I was trying to solve the problem when a loader is working on the FILE thread
> and it is asked to start loading again. But this is not happening in the
current
> code. Whoever makes it happen will have more information to fix it the best
way.
> 
> So I remove this check.
> 
>
http://codereview.chromium.org/5742008/diff/79001/chrome/browser/extensions/e...
> File chrome/browser/extensions/external_extension_loader.h (right):
> 
>
http://codereview.chromium.org/5742008/diff/79001/chrome/browser/extensions/e...
> chrome/browser/extensions/external_extension_loader.h:51: Lock lock_;
> Done.
> 
>
http://codereview.chromium.org/5742008/diff/79001/chrome/browser/extensions/e...
> File chrome/browser/extensions/external_extension_provider_impl.h (right):
> 
>
http://codereview.chromium.org/5742008/diff/79001/chrome/browser/extensions/e...
> chrome/browser/extensions/external_extension_provider_impl.h:11: #include
> "base/lock.h"
> On 2011/01/06 18:54:40, Aaron Boodman wrote:
> > Doesn't appear to be used?
> 
> Done.
> 
>
http://codereview.chromium.org/5742008/diff/79001/chrome/browser/extensions/e...
> chrome/browser/extensions/external_extension_provider_impl.h:76: // Location
for
> external extensions that are provided by this provider from
> On 2011/01/06 18:54:40, Aaron Boodman wrote:
> > Add a blank line above here.
> 
> Done.
> 
>
http://codereview.chromium.org/5742008/diff/79001/chrome/browser/extensions/e...
> File chrome/browser/extensions/external_extension_provider_interface.h
(right):
> 
>
http://codereview.chromium.org/5742008/diff/79001/chrome/browser/extensions/e...
> chrome/browser/extensions/external_extension_provider_interface.h:19: class
> ExternalExtensionProviderInterface {
> According to the Chromium style guide, I should either put an Interface or a
> Delegate suffix here. Jochen had pointed this out, and I didn't find any
> loopholes to avoid this. (I've tried, though.)
> 
> Should I change this to Delegate, then?

Our coding style clearly states that pure virtual classes should be called
..Interface with the only exception being ...Delegate. We should either stick to
the coding style (or update the coding style, Brett is maintaining it).

[Aaron Boodman, 2011-01-11 20:51:29]

On 2011/01/10 20:01:12, jochen wrote:
> > Should I change this to Delegate, then?
> 
> Our coding style clearly states that pure virtual classes should be called
> ..Interface with the only exception being ...Delegate. We should either stick
to
> the coding style (or update the coding style, Brett is maintaining it).

Sorry, I didn't know about this.

LGTM

[gfeher, 2011-01-12 19:08:33]

Follow up: we had a discussion with Aaron about the following issue: instead of
passing the prefs dictionary in ExternalExtensionLoader via a member variable,
it would be better to pass a copy of it as a function argument. E.g. calling the
following via PostTask:
void ExternalExtensionLoader::LoadFinished(DictionaryValue* prefs)
This approach is simpler and nicer, but I believe I've found a catch. If the
message loop shuts down while the posted task is pending, then the content of
prefs gets memory-leaked. Freeing it safely would need some extra machinery, and
I'd like to defer that from this CL.

Summary: I am planning to land this CL as-is tomorrow, unless somebody objects.