Introduce ExtensionsService. Load extensions on startup from a directory in...

Introduce ExtensionsService. Load extensions on startup from a directory in
the profile if a command-line flag is present.

Please carefully scrutinize the threading/ref-counting schenanigans.


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

[Erik does not do reviews, 2008-12-02 22:27:08]

http://codereview.chromium.org/12876/diff/213/221
File chrome/browser/browser_init.cc (right):

http://codereview.chromium.org/12876/diff/213/221#newcode489
Line 489: }
remove extra {}

http://codereview.chromium.org/12876/diff/213/219
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/12876/diff/213/219#newcode28
Line 28: CalledOnValidThread();
DCHECK(CalledOnValidThread());

http://codereview.chromium.org/12876/diff/213/219#newcode44
Line 44: CalledOnValidThread();
DCHECK

http://codereview.chromium.org/12876/diff/213/219#newcode50
Line 50: CalledOnValidThread();
DCHECK

http://codereview.chromium.org/12876/diff/213/219#newcode58
Line 58: CalledOnValidThread();
DCHECK

http://codereview.chromium.org/12876/diff/213/219#newcode59
Line 59: // TODO(aa): Print the error message out somewhere.
Can't you just LOG for now?

http://codereview.chromium.org/12876/diff/213/219#newcode68
Line 68: CalledOnValidThread();
DCHECK - although this will fail right now once you do that

http://codereview.chromium.org/12876/diff/213/219#newcode77
Line 77: while (!(child_path = enumerator.Next()).empty()) {
yuck.  Assignments inside of condition tests are evil.  Looking at the code, I
see why you have it this way, but I'd rather see a while(true) { child_path =
enumerator.Next(); if (child_path.empty()) break;
I know it's more lines of code, but it's less error prone, easier to debug, etc.

http://codereview.chromium.org/12876/diff/213/219#newcode81
Line 81: frontend->GetMessageLoop()->PostTask(FROM_HERE, NewRunnableMethod(
This is a large enough wad of code that's repeated, that I think you should pull
it out into a helper function.

http://codereview.chromium.org/12876/diff/213/220
File chrome/browser/extensions/extensions_service.h (right):

http://codereview.chromium.org/12876/diff/213/220#newcode83
Line 83: public base::RefCountedThreadSafe<ExtensionsServiceBackend> {
Did you intend for this to be both NonThreadSafe and RefCountedThreadSafe? 
Right now it looks like it should fail since it's created in the
ExtensionServiceFrontend's thread (UI?) and then has a method called on it in
the file thread.

http://codereview.chromium.org/12876/diff/213/218
File chrome/browser/extensions/extensions_service_unittest.cc (right):

http://codereview.chromium.org/12876/diff/213/218#newcode18
Line 18: // Populate a directory with extensions.
my guess is that it will be easier to maintain this and add tests by just
creating a directory in chrome/test/data/extensions.  Eventually you're going to
want to verify the contents of the manifest (content scripts, etc.), and perhaps
even load and execute these extensions in unit tests.

http://codereview.chromium.org/12876/diff/213/218#newcode127
Line 127: EXPECT_EQ(ASCIIToWide("com.google.myextension3"),
frontend->extensions()->at(0)->id());
80 cols

http://codereview.chromium.org/12876/diff/213/215
File chrome/browser/profile.h (right):

http://codereview.chromium.org/12876/diff/213/215#newcode28
Line 28: class ExtensionsService;
sort order

[brettw, 2008-12-02 22:32:13]

Drive by style nits

http://codereview.chromium.org/12876/diff/213/219
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/12876/diff/213/219#newcode1
Line 1: #include "chrome/browser/extensions/extensions_service.h"
Copyright?

http://codereview.chromium.org/12876/diff/213/219#newcode12
Line 12: const std::wstring
ExtensionsService::kInstallDirectoryName(L"Extensions");
Don't use global objects including strings. Use a const wchar_t[] here.

http://codereview.chromium.org/12876/diff/213/220
File chrome/browser/extensions/extensions_service.h (right):

http://codereview.chromium.org/12876/diff/213/220#newcode5
Line 5: #ifndef CHROME_BROWSER_EXTENSIONS_EXTENSIONS_SERVICE_H__
Only use one underscore at the end nowadays (does this apply to other files?)

http://codereview.chromium.org/12876/diff/213/218
File chrome/browser/extensions/extensions_service_unittest.cc (right):

http://codereview.chromium.org/12876/diff/213/218#newcode127
Line 127: EXPECT_EQ(ASCIIToWide("com.google.myextension3"),
frontend->extensions()->at(0)->id());
80 cols

[Aaron Boodman, 2008-12-03 01:40:21]

Thanks for the comments. New patch ready.

http://codereview.chromium.org/12876/diff/213/221
File chrome/browser/browser_init.cc (right):

http://codereview.chromium.org/12876/diff/213/221#newcode489
Line 489: }
On 2008/12/02 22:27:08, Erik Kay wrote:
> remove extra {}

Done.

http://codereview.chromium.org/12876/diff/213/219
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/12876/diff/213/219#newcode1
Line 1: #include "chrome/browser/extensions/extensions_service.h"
On 2008/12/02 22:32:13, brettw wrote:
> Copyright?

Done.

http://codereview.chromium.org/12876/diff/213/219#newcode12
Line 12: const std::wstring
ExtensionsService::kInstallDirectoryName(L"Extensions");
On 2008/12/02 22:32:13, brettw wrote:
> Don't use global objects including strings. Use a const wchar_t[] here.

Done.

http://codereview.chromium.org/12876/diff/213/219#newcode28
Line 28: CalledOnValidThread();
On 2008/12/02 22:27:08, Erik Kay wrote:
> DCHECK(CalledOnValidThread());

Yikes :/ No longer valid per your other comment, so removed all these.

http://codereview.chromium.org/12876/diff/213/219#newcode59
Line 59: // TODO(aa): Print the error message out somewhere.
On 2008/12/02 22:27:08, Erik Kay wrote:
> Can't you just LOG for now?

Yes, did that for now, but see comment.

http://codereview.chromium.org/12876/diff/213/219#newcode68
Line 68: CalledOnValidThread();
On 2008/12/02 22:27:08, Erik Kay wrote:
> DCHECK - although this will fail right now once you do that

Done.

http://codereview.chromium.org/12876/diff/213/219#newcode77
Line 77: while (!(child_path = enumerator.Next()).empty()) {
On 2008/12/02 22:27:08, Erik Kay wrote:
> yuck.  Assignments inside of condition tests are evil.  Looking at the code, I
> see why you have it this way, but I'd rather see a while(true) { child_path =
> enumerator.Next(); if (child_path.empty()) break;
> I know it's more lines of code, but it's less error prone, easier to debug,
etc.

Good point. I changed this to a for loop instead.

http://codereview.chromium.org/12876/diff/213/219#newcode81
Line 81: frontend->GetMessageLoop()->PostTask(FROM_HERE, NewRunnableMethod(
On 2008/12/02 22:27:08, Erik Kay wrote:
> This is a large enough wad of code that's repeated, that I think you should
pull
> it out into a helper function.

Done.

http://codereview.chromium.org/12876/diff/213/220
File chrome/browser/extensions/extensions_service.h (right):

http://codereview.chromium.org/12876/diff/213/220#newcode5
Line 5: #ifndef CHROME_BROWSER_EXTENSIONS_EXTENSIONS_SERVICE_H__
On 2008/12/02 22:32:13, brettw wrote:
> Only use one underscore at the end nowadays (does this apply to other files?)

Done, fixed other files.

http://codereview.chromium.org/12876/diff/213/220#newcode83
Line 83: public base::RefCountedThreadSafe<ExtensionsServiceBackend> {
On 2008/12/02 22:27:08, Erik Kay wrote:
> Did you intend for this to be both NonThreadSafe and RefCountedThreadSafe? 
> Right now it looks like it should fail since it's created in the
> ExtensionServiceFrontend's thread (UI?) and then has a method called on it in
> the file thread.

Yeah, I did intend it, because I thought that these guys methods' (other than
AddRef/Release) were only called from one thread. But I forgot about the
constructor. Removed NonThreadSafe base class.

http://codereview.chromium.org/12876/diff/213/218
File chrome/browser/extensions/extensions_service_unittest.cc (right):

http://codereview.chromium.org/12876/diff/213/218#newcode18
Line 18: // Populate a directory with extensions.
On 2008/12/02 22:27:08, Erik Kay wrote:
> my guess is that it will be easier to maintain this and add tests by just
> creating a directory in chrome/test/data/extensions.  Eventually you're going
to
> want to verify the contents of the manifest (content scripts, etc.), and
perhaps
> even load and execute these extensions in unit tests.

Ah, I didn't know about the test data directory. I moved all this there.

http://codereview.chromium.org/12876/diff/213/218#newcode127
Line 127: EXPECT_EQ(ASCIIToWide("com.google.myextension3"),
frontend->extensions()->at(0)->id());
On 2008/12/02 22:27:08, Erik Kay wrote:
> 80 cols

Done.

http://codereview.chromium.org/12876/diff/213/218#newcode127
Line 127: EXPECT_EQ(ASCIIToWide("com.google.myextension3"),
frontend->extensions()->at(0)->id());
On 2008/12/02 22:32:13, brettw wrote:
> 80 cols

Done.

http://codereview.chromium.org/12876/diff/213/215
File chrome/browser/profile.h (right):

http://codereview.chromium.org/12876/diff/213/215#newcode28
Line 28: class ExtensionsService;
On 2008/12/02 22:27:08, Erik Kay wrote:
> sort order

Done.

[Erik does not do reviews, 2008-12-03 02:28:17]

Didn't you say that you needed to patch file_util* for this CL?

otherwise, LGTM

[Aaron Boodman, 2008-12-04 21:37:07]

On 2008/12/03 02:28:17, Erik Kay wrote:
> Didn't you say that you needed to patch file_util* for this CL?
> 
> otherwise, LGTM

Ok, got this building and running on linux. But the new test does not pass
without more linux porting skill than I can muster, so I #ifdef'd that out.

Also, I tried updating FileEnumerator to accept FilePath, as well as
FilePath::StringType, but both of those ended up cascading out all over the
place. So I used the existing FilePath::ToWStringHack() and
FilePath::FromWStringHack() instead.

[Erik does not do reviews, 2008-12-04 22:03:05]

LGTM

http://codereview.chromium.org/12876/diff/279/285
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/12876/diff/279/285#newcode79
Line 79: FilePath manifest_path = FilePath::FromWStringHack(child_path).Append(
indent is off

http://codereview.chromium.org/12876/diff/279/285#newcode130
Line 130: }
is this file missing a trailing newline?

http://codereview.chromium.org/12876/diff/279/284
File chrome/browser/extensions/extensions_service_unittest.cc (right):

http://codereview.chromium.org/12876/diff/279/284#newcode98
Line 98: };
missing newline at end of file