Adding the --load-component-extension flag.

Removed flag --expose-private-extension-api. Added --load-component-extension.

BUG=None
TEST=None


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

[SeRya, 2011-11-28 18:19:46]

Hi Zel,

~half of year ago you added the --expose-private-extension-api flag but it only
available in the Debug build. It may be used to update the file manager from
sources on unmodified chromium os image. Building Release image is easer so I'd
suggest to enable it in the Release configuration. To avoid graning that
privileges to third-party code I sugget to restrict this flag to extensions
loaded locally (since this mechanism dedicated for debugging extensions).

[Aaron Boodman, 2011-11-28 18:29:59]

It would be nice if instead of these various hacks, there was a way to specify
the path of a component extension on disk. Then all the special cases for
component extensions would naturally apply.

Perhaps something like --override-component-extension-path=<id>,<path>

http://codereview.chromium.org/8659002/diff/1/chrome/common/extensions/extens...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/8659002/diff/1/chrome/common/extensions/extens...
chrome/common/extensions/extension.cc:2919: )
Nit: these parens should be up on line 2918.

[SeRya, 2011-11-28 23:49:53]

Flag  --expose-private-extension-api removed. Instead introduced
--load-component-extension.

http://codereview.chromium.org/8659002/diff/1/chrome/common/extensions/extens...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/8659002/diff/1/chrome/common/extensions/extens...
chrome/common/extensions/extension.cc:2919: )
On 2011/11/28 18:29:59, Aaron Boodman wrote:
> Nit: these parens should be up on line 2918.

Done.

[zel, 2011-11-29 00:19:56]

http://codereview.chromium.org/8659002/diff/4002/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/8659002/diff/4002/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:420: if
(command_line->HasSwitch(switches::kLoadComponentExtension)) {
Can we make this flag load more than one component extension at the time? You
could comma separate component extension paths.

[SeRya, 2011-11-29 01:35:25]

http://codereview.chromium.org/8659002/diff/4002/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/8659002/diff/4002/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:420: if
(command_line->HasSwitch(switches::kLoadComponentExtension)) {
On 2011/11/29 00:19:56, zel wrote:
> Can we make this flag load more than one component extension at the time? You
> could comma separate component extension paths.

Done.

[Aaron Boodman, 2011-11-29 05:32:25]

The implementation is brittle because it relies on the new Add() method being
called after the original component extension is registered, but before
LoadAll() is called. And there are no tests to ensure this doesn't break.

Instead of doing it this way, I think that we should add a new map like
overrides_ to ComponentLoader which maps IDs to manifests. Then in Load(), if
there is a matching override, use it instead of the registered item.

http://codereview.chromium.org/8659002/diff/5010/chrome/browser/extensions/co...
File chrome/browser/extensions/component_loader.cc (right):

http://codereview.chromium.org/8659002/diff/5010/chrome/browser/extensions/co...
chrome/browser/extensions/component_loader.cc:124:
base::ThreadRestrictions::ScopedAllowIO allow_io;
Move this up to the call site in profile_impl.cc. People who call this method
should have to think harder about what they are doing and whether its a good
idea.

http://codereview.chromium.org/8659002/diff/5010/chrome/browser/extensions/co...
chrome/browser/extensions/component_loader.cc:136:
Remove(GenerateId(manifest.get()));
What happens if the extension is already loaded? You should CHECK that it isn't
since we don't need or want to support that for now.

http://codereview.chromium.org/8659002/diff/5010/chrome/browser/extensions/co...
chrome/browser/extensions/component_loader.cc:138: ComponentExtensionInfo
info(manifest.release(), absolute_path);
Can you delegate to the Add() method that takes a manifest and a path here,
rather than duplicating code?

http://codereview.chromium.org/8659002/diff/5010/chrome/browser/extensions/co...
chrome/browser/extensions/component_loader.cc:191: void
ComponentLoader::Remove(const std::string& id) {
Don't duplicate code. One of these methods should delegate to the other.

http://codereview.chromium.org/8659002/diff/5010/chrome/browser/extensions/co...
File chrome/browser/extensions/component_loader.h (right):

http://codereview.chromium.org/8659002/diff/5010/chrome/browser/extensions/co...
chrome/browser/extensions/component_loader.h:45: const Extension*
AddFromCommandLine(const FilePath& path);
The fact that this comes from a command line is a detail outside the scope of
this class. The method should be called something like AddOverride(), and the
comment should explain that it will override any previously registered component
extension with the same ID.

http://codereview.chromium.org/8659002/diff/5010/chrome/browser/extensions/co...
chrome/browser/extensions/component_loader.h:52: // extensions to be loaded.
Nit: it looks like "extensions" fits on the previous line.

http://codereview.chromium.org/8659002/diff/5010/chrome/browser/extensions/co...
chrome/browser/extensions/component_loader.h:106: // Determine the extension id
and unload the extension.
Comment is incorrect, nothing is unloaded here.

http://codereview.chromium.org/8659002/diff/5010/chrome/browser/extensions/co...
chrome/browser/extensions/component_loader.h:107: std::string GenerateId(const
base::DictionaryValue* manifest);
Nit: capitalize "ID".

[SeRya, 2011-11-29 21:31:00]

What to support "override" semantic for? I would't like to manage memory in one
another collection without a reason. We don't load/unload component extensions
dynamically (where it could be useful). With exception for enterprise_web_store.

I renamed the method to 'AddOrReplace' and added DCHECK that there is no
duplicating extension. It should make usage obvious.

That DCHECK helped to realize that 'enterprise_web_store' haven't been actually
unloading its previous instance. Fixed.

http://codereview.chromium.org/8659002/diff/5010/chrome/browser/extensions/co...
File chrome/browser/extensions/component_loader.cc (right):

http://codereview.chromium.org/8659002/diff/5010/chrome/browser/extensions/co...
chrome/browser/extensions/component_loader.cc:124:
base::ThreadRestrictions::ScopedAllowIO allow_io;
On 2011/11/29 05:32:25, Aaron Boodman wrote:
> Move this up to the call site in profile_impl.cc. People who call this method
> should have to think harder about what they are doing and whether its a good
> idea.

Removed completely. It's not necessary for the place it's used.

http://codereview.chromium.org/8659002/diff/5010/chrome/browser/extensions/co...
chrome/browser/extensions/component_loader.cc:136:
Remove(GenerateId(manifest.get()));
On 2011/11/29 05:32:25, Aaron Boodman wrote:
> What happens if the extension is already loaded? You should CHECK that it
isn't
> since we don't need or want to support that for now.

Previous will be removed and unloaded.

http://codereview.chromium.org/8659002/diff/5010/chrome/browser/extensions/co...
chrome/browser/extensions/component_loader.cc:138: ComponentExtensionInfo
info(manifest.release(), absolute_path);
On 2011/11/29 05:32:25, Aaron Boodman wrote:
> Can you delegate to the Add() method that takes a manifest and a path here,
> rather than duplicating code?

Done.

http://codereview.chromium.org/8659002/diff/5010/chrome/browser/extensions/co...
chrome/browser/extensions/component_loader.cc:191: void
ComponentLoader::Remove(const std::string& id) {
On 2011/11/29 05:32:25, Aaron Boodman wrote:
> Don't duplicate code. One of these methods should delegate to the other.

Done.

http://codereview.chromium.org/8659002/diff/5010/chrome/browser/extensions/co...
File chrome/browser/extensions/component_loader.h (right):

http://codereview.chromium.org/8659002/diff/5010/chrome/browser/extensions/co...
chrome/browser/extensions/component_loader.h:45: const Extension*
AddFromCommandLine(const FilePath& path);
On 2011/11/29 05:32:25, Aaron Boodman wrote:
> The fact that this comes from a command line is a detail outside the scope of
> this class. The method should be called something like AddOverride(), and the
> comment should explain that it will override any previously registered
component
> extension with the same ID.

Renamed to AddOrReplace

http://codereview.chromium.org/8659002/diff/5010/chrome/browser/extensions/co...
chrome/browser/extensions/component_loader.h:52: // extensions to be loaded.
On 2011/11/29 05:32:25, Aaron Boodman wrote:
> Nit: it looks like "extensions" fits on the previous line.

Since now comment for the prev method reflects this one as well the comment
removed.

http://codereview.chromium.org/8659002/diff/5010/chrome/browser/extensions/co...
chrome/browser/extensions/component_loader.h:106: // Determine the extension id
and unload the extension.
On 2011/11/29 05:32:25, Aaron Boodman wrote:
> Comment is incorrect, nothing is unloaded here.

Fixed.

http://codereview.chromium.org/8659002/diff/5010/chrome/browser/extensions/co...
chrome/browser/extensions/component_loader.h:107: std::string GenerateId(const
base::DictionaryValue* manifest);
On 2011/11/29 05:32:25, Aaron Boodman wrote:
> Nit: capitalize "ID".

It is 'Id' everywhere except a few unit tests. Including Extension::GenerateId.

[SeRya, 2011-12-05 19:02:12]

Added a unit test.

[Aaron Boodman, 2011-12-05 22:11:25]

http://codereview.chromium.org/8659002/diff/19001/chrome/browser/extensions/c...
File chrome/browser/extensions/component_loader.cc (left):

http://codereview.chromium.org/8659002/diff/19001/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader.cc:104: if
(!absolute_path.IsAbsolute()) {
Did this need to move?

http://codereview.chromium.org/8659002/diff/19001/chrome/browser/extensions/c...
File chrome/browser/extensions/component_loader.cc (right):

http://codereview.chromium.org/8659002/diff/19001/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader.cc:104:
DCHECK(!Exists(GenerateId(parsed_manifest)));
The extension system prefers CHECK to DCHECK

http://codereview.chromium.org/8659002/diff/19001/chrome/browser/extensions/c...
File chrome/browser/extensions/component_loader.h (right):

http://codereview.chromium.org/8659002/diff/19001/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader.h:44: // Loads a component extension
from file system. Replaces previusly added
typo: priviusly

http://codereview.chromium.org/8659002/diff/19001/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader.h:78: return 
component_extensions_.size();
extra space after 'return'

http://codereview.chromium.org/8659002/diff/19001/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader.h:108: // Returns true if an
extension with the specified id has added.
missing word 'been' after 'has'

http://codereview.chromium.org/8659002/diff/19001/chrome/browser/extensions/c...
File chrome/browser/extensions/component_loader_unittest.cc (right):

http://codereview.chromium.org/8659002/diff/19001/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader_unittest.cc:24: size_t unloaded_;
This is not a good name. It should be unloaded_count_ to match the getter.

http://codereview.chromium.org/8659002/diff/19001/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader_unittest.cc:26: bool find(std::string
id) {
Methods are supposed to be ordered before fields.

http://codereview.chromium.org/8659002/diff/19001/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader_unittest.cc:74: size_t
get_unloaded_count() const {
getters in Chrome do not have the 'get_' prefix.

http://codereview.chromium.org/8659002/diff/19001/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader_unittest.cc:131: FilePath
get_extension_path(std::string name) {
Since this does significant work, it should be named like GetExtensionPath().
Only getters are named like foo_bar().

http://codereview.chromium.org/8659002/diff/19001/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader_unittest.cc:135:
test_data_dir.AppendASCII("extensions")
Line 135 looks like it can fit with line 134.

http://codereview.chromium.org/8659002/diff/19001/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader_unittest.cc:282: ASSERT_EQ(0U,
component_loader_.GetRegisteredExtensionsCount());
Chrome uses lower-case 'u'.

http://codereview.chromium.org/8659002/diff/19001/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader_unittest.cc:287:
component_loader_.AddOrReplace(get_extension_path("hhaomjibdihmijegdhdafkllkbggdgoj"));
line length

http://codereview.chromium.org/8659002/diff/19001/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader_unittest.cc:304:
component_loader_.AddOrReplace(get_extension_path("hhaomjibdihmijegdhdafkllkbggdgoj"));
line length

http://codereview.chromium.org/8659002/diff/19001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/8659002/diff/19001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:455:
extension_service_->component_loader()->AddOrReplace(
How does this work? It seems like it should DCHECK because it is doing file IO
on the UI thread?

[SeRya, 2011-12-06 08:19:26]

http://codereview.chromium.org/8659002/diff/19001/chrome/browser/extensions/c...
File chrome/browser/extensions/component_loader.cc (left):

http://codereview.chromium.org/8659002/diff/19001/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader.cc:104: if
(!absolute_path.IsAbsolute()) {
On 2011/12/05 22:11:26, Aaron Boodman wrote:
> Did this need to move?

The code moved for the following reason.

The Remove(root_directory) method should remove the extension previously added
with Add(.., root_directory). At least it seems to be assumed in
AddOrReloadEnterpriseWebStore. However since path used to be immediately
converted to absolute one AddOrReloadEnterpriseWebStore did not remove this
previous copy.

Once I added the CHECK above that bug started failing tests and this move is the
fix.

http://codereview.chromium.org/8659002/diff/19001/chrome/browser/extensions/c...
File chrome/browser/extensions/component_loader.cc (right):

http://codereview.chromium.org/8659002/diff/19001/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader.cc:104:
DCHECK(!Exists(GenerateId(parsed_manifest)));
On 2011/12/05 22:11:26, Aaron Boodman wrote:
> The extension system prefers CHECK to DCHECK

Done.

http://codereview.chromium.org/8659002/diff/19001/chrome/browser/extensions/c...
File chrome/browser/extensions/component_loader.h (right):

http://codereview.chromium.org/8659002/diff/19001/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader.h:44: // Loads a component extension
from file system. Replaces previusly added
On 2011/12/05 22:11:26, Aaron Boodman wrote:
> typo: priviusly

Done.

http://codereview.chromium.org/8659002/diff/19001/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader.h:78: return 
component_extensions_.size();
On 2011/12/05 22:11:26, Aaron Boodman wrote:
> extra space after 'return'

Done.

http://codereview.chromium.org/8659002/diff/19001/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader.h:108: // Returns true if an
extension with the specified id has added.
On 2011/12/05 22:11:26, Aaron Boodman wrote:
> missing word 'been' after 'has'

Done.

http://codereview.chromium.org/8659002/diff/19001/chrome/browser/extensions/c...
File chrome/browser/extensions/component_loader_unittest.cc (right):

http://codereview.chromium.org/8659002/diff/19001/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader_unittest.cc:24: size_t unloaded_;
On 2011/12/05 22:11:26, Aaron Boodman wrote:
> This is not a good name. It should be unloaded_count_ to match the getter.

Done.

http://codereview.chromium.org/8659002/diff/19001/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader_unittest.cc:26: bool find(std::string
id) {
On 2011/12/05 22:11:26, Aaron Boodman wrote:
> Methods are supposed to be ordered before fields.

Done.

http://codereview.chromium.org/8659002/diff/19001/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader_unittest.cc:74: size_t
get_unloaded_count() const {
On 2011/12/05 22:11:26, Aaron Boodman wrote:
> getters in Chrome do not have the 'get_' prefix.

Done.

http://codereview.chromium.org/8659002/diff/19001/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader_unittest.cc:131: FilePath
get_extension_path(std::string name) {
On 2011/12/05 22:11:26, Aaron Boodman wrote:
> Since this does significant work, it should be named like GetExtensionPath().
> Only getters are named like foo_bar().

Done.

http://codereview.chromium.org/8659002/diff/19001/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader_unittest.cc:135:
test_data_dir.AppendASCII("extensions")
On 2011/12/05 22:11:26, Aaron Boodman wrote:
> Line 135 looks like it can fit with line 134.

Done.

http://codereview.chromium.org/8659002/diff/19001/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader_unittest.cc:282: ASSERT_EQ(0U,
component_loader_.GetRegisteredExtensionsCount());
On 2011/12/05 22:11:26, Aaron Boodman wrote:
> Chrome uses lower-case 'u'.

Done.

http://codereview.chromium.org/8659002/diff/19001/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader_unittest.cc:287:
component_loader_.AddOrReplace(get_extension_path("hhaomjibdihmijegdhdafkllkbggdgoj"));
On 2011/12/05 22:11:26, Aaron Boodman wrote:
> line length

Done.

http://codereview.chromium.org/8659002/diff/19001/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader_unittest.cc:304:
component_loader_.AddOrReplace(get_extension_path("hhaomjibdihmijegdhdafkllkbggdgoj"));
On 2011/12/05 22:11:26, Aaron Boodman wrote:
> line length

Done.

http://codereview.chromium.org/8659002/diff/19001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/8659002/diff/19001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:455:
extension_service_->component_loader()->AddOrReplace(
On 2011/12/05 22:11:26, Aaron Boodman wrote:
> How does this work? It seems like it should DCHECK because it is doing file IO
> on the UI thread?

Now sure why it works. Probably caller code allows IO. But it's less fragile to
allow IO explicitly. Added.

[Aaron Boodman, 2011-12-06 09:52:47]

On Tue, Dec 6, 2011 at 12:19 AM,  <serya@chromium.org> wrote:
>
>
http://codereview.chromium.org/8659002/diff/19001/chrome/browser/extensions/c...
> File chrome/browser/extensions/component_loader.cc (left):
>
>
http://codereview.chromium.org/8659002/diff/19001/chrome/browser/extensions/c...
> chrome/browser/extensions/component_loader.cc:104: if
> (!absolute_path.IsAbsolute()) {
> On 2011/12/05 22:11:26, Aaron Boodman wrote:
>>
>> Did this need to move?
>
>
> The code moved for the following reason.
>
> The Remove(root_directory) method should remove the extension previously
> added with Add(.., root_directory). At least it seems to be assumed in
> AddOrReloadEnterpriseWebStore. However since path used to be immediately
> converted to absolute one AddOrReloadEnterpriseWebStore did not remove
> this previous copy.
>
> Once I added the CHECK above that bug started failing tests and this
> move is the fix.

Ok.

>
http://codereview.chromium.org/8659002/diff/19001/chrome/browser/profiles/pro...
> chrome/browser/profiles/profile_impl.cc:455:
> extension_service_->component_loader()->AddOrReplace(
> On 2011/12/05 22:11:26, Aaron Boodman wrote:
>>
>> How does this work? It seems like it should DCHECK because it is doing
>
> file IO
>>
>> on the UI thread?
>
>
> Now sure why it works. Probably caller code allows IO. But it's less
> fragile to allow IO explicitly. Added.
>
> http://codereview.chromium.org/8659002/

Please put the ScopedAllowIO outside this method, wrapping the call
site in profile_impl.cc. We don't want people to be able to call this
method on the UI thread typically.

 -a

[SeRya, 2011-12-06 14:16:28]

On 2011/12/06 09:52:47, Aaron Boodman wrote:
> On Tue, Dec 6, 2011 at 12:19 AM,  <mailto:serya@chromium.org> wrote:
> >
> >
>
http://codereview.chromium.org/8659002/diff/19001/chrome/browser/extensions/c...
> > File chrome/browser/extensions/component_loader.cc (left):
> >
> >
>
http://codereview.chromium.org/8659002/diff/19001/chrome/browser/extensions/c...
> > chrome/browser/extensions/component_loader.cc:104: if
> > (!absolute_path.IsAbsolute()) {
> > On 2011/12/05 22:11:26, Aaron Boodman wrote:
> >>
> >> Did this need to move?
> >
> >
> > The code moved for the following reason.
> >
> > The Remove(root_directory) method should remove the extension previously
> > added with Add(.., root_directory). At least it seems to be assumed in
> > AddOrReloadEnterpriseWebStore. However since path used to be immediately
> > converted to absolute one AddOrReloadEnterpriseWebStore did not remove
> > this previous copy.
> >
> > Once I added the CHECK above that bug started failing tests and this
> > move is the fix.
> 
> Ok.
> 
> >
>
http://codereview.chromium.org/8659002/diff/19001/chrome/browser/profiles/pro...
> > chrome/browser/profiles/profile_impl.cc:455:
> > extension_service_->component_loader()->AddOrReplace(
> > On 2011/12/05 22:11:26, Aaron Boodman wrote:
> >>
> >> How does this work? It seems like it should DCHECK because it is doing
> >
> > file IO
> >>
> >> on the UI thread?
> >
> >
> > Now sure why it works. Probably caller code allows IO. But it's less
> > fragile to allow IO explicitly. Added.
> >
> > http://codereview.chromium.org/8659002/
> 
> Please put the ScopedAllowIO outside this method, wrapping the call
> site in profile_impl.cc. We don't want people to be able to call this
> method on the UI thread typically.
> 
>  -a

Done

[Aaron Boodman, 2011-12-06 19:43:35]

http://codereview.chromium.org/8659002/diff/28017/chrome/browser/extensions/c...
File chrome/browser/extensions/component_loader.cc (right):

http://codereview.chromium.org/8659002/diff/28017/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader.cc:173: for (; it !=
component_extensions_.end(); ++it) {
It seems like this class spends a lot of time finding things by ID. Can you
please change it to use a map internally instead of a list. That would probably
cut down a lot of this code.

http://codereview.chromium.org/8659002/diff/28017/chrome/browser/extensions/c...
File chrome/browser/extensions/component_loader.h (right):

http://codereview.chromium.org/8659002/diff/28017/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader.h:77: size_t
GetRegisteredExtensionsCount() const {
Sorry I didn't notice this earlier: This should be registered_extensions_count()
style because it's just a getter. And it should be moved up to the top of the
class, right below the constructors.

I'm pointing out a lot of style issues. You should probably review:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml

and:

http://www.chromium.org/developers/coding-style

http://codereview.chromium.org/8659002/diff/28017/chrome/browser/extensions/c...
File chrome/browser/extensions/component_loader_unittest.cc (right):

http://codereview.chromium.org/8659002/diff/28017/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader_unittest.cc:22: bool find(std::string
id) {
There is already GetExtensionById in TestExtensionService.

http://codereview.chromium.org/8659002/diff/28017/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader_unittest.cc:233: ASSERT_EQ(0u,
extension_service_.extensions()->size());
thank-you!

http://codereview.chromium.org/8659002/diff/28017/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/8659002/diff/28017/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:402: static void
AddOrReplaceComponentExtension(extensions::ComponentLoader* loader,
Why did you pull this out into a function? It's not repeated. This code can just
go right inside the loop.

Also, even if you were going to do this, it's weird to throw a static function
into the middle of a class's implementation this way.

http://codereview.chromium.org/8659002/diff/28017/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:405:
base::ThreadRestrictions::ScopedAllowIO allow_io;
This is still making me nervous. Since you are already working on this, can you
please add some logging to understand how it was working without this? This
might be a bad bug that we need to fix.

http://codereview.chromium.org/8659002/diff/28017/chrome/test/data/extensions...
File
chrome/test/data/extensions/good/Extensions/mndnfokpggljbaajbnioimlmbfngpief/manifest.json
(right):

http://codereview.chromium.org/8659002/diff/28017/chrome/test/data/extensions...
chrome/test/data/extensions/good/Extensions/mndnfokpggljbaajbnioimlmbfngpief/manifest.json:2:
"key":
"MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQDQcByy+eN9jzazWF/DPn7NW47sW7lgmpk6eKc0BQM18q8hvEM3zNm2n7HkJv/R6fU+X5mtqkDuKvq5skF6qqUF4oEyaleWDFhd1xFwV7JV+/DU7bZ00w2+6gzqsabkerFpoP33ZRIw7OviJenP0c0uWqDWF8EGSyMhB3txqhOtiQIDAQAB",
I think that if you put this file here, it will break other unit tests that
expect a certain number of extensions in this directory.

Even if not, it's a bit weird as this directory is used to test a specific
feature (it's badly named), and this test has nothing to do with that feature.

Please move this to test/data/extensions/extension_with_key/

http://codereview.chromium.org/8659002/diff/28017/chrome/test/data/extensions...
chrome/test/data/extensions/good/Extensions/mndnfokpggljbaajbnioimlmbfngpief/manifest.json:3:
"name": "Bookmark Manager",
Most of these keys are irrelevant to the test. You can remove everything except
the name, version, and key. Please make the name something descriptive, like
"Extension with key"

[SeRya, 2011-12-06 21:00:15]

On 2011/12/06 09:52:47, Aaron Boodman wrote:
> Please put the ScopedAllowIO outside this method, wrapping the call
> site in profile_impl.cc. We don't want people to be able to call this
> method on the UI thread typically.

Hi Aaron,

Looks like I misunderstood your comment. Please, explain where exactly do you
want to move the ScopedAllowIO and how it would help to prevent of using it on
the UI thread?

[SeRya, 2011-12-06 22:19:37]

http://codereview.chromium.org/8659002/diff/28017/chrome/browser/extensions/c...
File chrome/browser/extensions/component_loader.cc (right):

http://codereview.chromium.org/8659002/diff/28017/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader.cc:173: for (; it !=
component_extensions_.end(); ++it) {
On 2011/12/06 19:43:35, Aaron Boodman wrote:
> It seems like this class spends a lot of time finding things by ID. Can you
> please change it to use a map internally instead of a list. That would
probably
> cut down a lot of this code.

It only does it when reloads Enterprise Web Store or component extensions with
command line switch (on startup). I'd prefer refactor this code in a separate
CL.

http://codereview.chromium.org/8659002/diff/28017/chrome/browser/extensions/c...
File chrome/browser/extensions/component_loader.h (right):

http://codereview.chromium.org/8659002/diff/28017/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader.h:77: size_t
GetRegisteredExtensionsCount() const {
On 2011/12/06 19:43:35, Aaron Boodman wrote:
> Sorry I didn't notice this earlier: This should be
registered_extensions_count()
> style because it's just a getter. And it should be moved up to the top of the
> class, right below the constructors.
> 
> I'm pointing out a lot of style issues. You should probably review:
> 
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml
> 
> and:
> 
> http://www.chromium.org/developers/coding-style

Done.

http://codereview.chromium.org/8659002/diff/28017/chrome/browser/extensions/c...
File chrome/browser/extensions/component_loader_unittest.cc (right):

http://codereview.chromium.org/8659002/diff/28017/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader_unittest.cc:22: bool find(std::string
id) {
On 2011/12/06 19:43:35, Aaron Boodman wrote:
> There is already GetExtensionById in TestExtensionService.

Irrelevant after merge.

http://codereview.chromium.org/8659002/diff/28017/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/8659002/diff/28017/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:402: static void
AddOrReplaceComponentExtension(extensions::ComponentLoader* loader,
On 2011/12/06 19:43:35, Aaron Boodman wrote:
> Why did you pull this out into a function? It's not repeated. This code can
just
> go right inside the loop.
> 
> Also, even if you were going to do this, it's weird to throw a static function
> into the middle of a class's implementation this way.

Reverted this change.

http://codereview.chromium.org/8659002/diff/28017/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:405:
base::ThreadRestrictions::ScopedAllowIO allow_io;
On 2011/12/06 19:43:35, Aaron Boodman wrote:
> This is still making me nervous. Since you are already working on this, can
you
> please add some logging to understand how it was working without this? This
> might be a bad bug that we need to fix.

I will try to figure out why it did work, but the logging probably shouldn't be
in the CL.

http://codereview.chromium.org/8659002/diff/28017/chrome/test/data/extensions...
File
chrome/test/data/extensions/good/Extensions/mndnfokpggljbaajbnioimlmbfngpief/manifest.json
(right):

http://codereview.chromium.org/8659002/diff/28017/chrome/test/data/extensions...
chrome/test/data/extensions/good/Extensions/mndnfokpggljbaajbnioimlmbfngpief/manifest.json:2:
"key":
"MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQDQcByy+eN9jzazWF/DPn7NW47sW7lgmpk6eKc0BQM18q8hvEM3zNm2n7HkJv/R6fU+X5mtqkDuKvq5skF6qqUF4oEyaleWDFhd1xFwV7JV+/DU7bZ00w2+6gzqsabkerFpoP33ZRIw7OviJenP0c0uWqDWF8EGSyMhB3txqhOtiQIDAQAB",
On 2011/12/06 19:43:35, Aaron Boodman wrote:
> I think that if you put this file here, it will break other unit tests that
> expect a certain number of extensions in this directory.
> 
> Even if not, it's a bit weird as this directory is used to test a specific
> feature (it's badly named), and this test has nothing to do with that feature.
> 
> Please move this to test/data/extensions/extension_with_key/

Moved to chrome/test/data/extensions/override_component_extension/manifest.json

http://codereview.chromium.org/8659002/diff/28017/chrome/test/data/extensions...
chrome/test/data/extensions/good/Extensions/mndnfokpggljbaajbnioimlmbfngpief/manifest.json:3:
"name": "Bookmark Manager",
On 2011/12/06 19:43:35, Aaron Boodman wrote:
> Most of these keys are irrelevant to the test. You can remove everything
except
> the name, version, and key. Please make the name something descriptive, like
> "Extension with key"

Done.

[Aaron Boodman, 2011-12-07 00:09:02]

lgtm

http://codereview.chromium.org/8659002/diff/28017/chrome/browser/extensions/c...
File chrome/browser/extensions/component_loader.cc (right):

http://codereview.chromium.org/8659002/diff/28017/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader.cc:173: for (; it !=
component_extensions_.end(); ++it) {
On 2011/12/06 22:19:37, SeRya wrote:
> On 2011/12/06 19:43:35, Aaron Boodman wrote:
> > It seems like this class spends a lot of time finding things by ID. Can you
> > please change it to use a map internally instead of a list. That would
> probably
> > cut down a lot of this code.
> 
> It only does it when reloads Enterprise Web Store or component extensions with
> command line switch (on startup). I'd prefer refactor this code in a separate
> CL.

I didn't mean compute time, I meant lines of code. We now repeat the same
pattern to find an extension by ID in several places.

I am OK with you fixing this in a subsequent change, but please do so soon.

http://codereview.chromium.org/8659002/diff/28017/chrome/browser/extensions/c...
File chrome/browser/extensions/component_loader_unittest.cc (right):

http://codereview.chromium.org/8659002/diff/28017/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader_unittest.cc:22: bool find(std::string
id) {
On 2011/12/06 22:19:37, SeRya wrote:
> On 2011/12/06 19:43:35, Aaron Boodman wrote:
> > There is already GetExtensionById in TestExtensionService.
> 
> Irrelevant after merge.

What I mean is that the TestExtensionService base class you are inheriting from
already has a virtual method GetExtensionById() that you could override here.

It's not a huge deal. But if you leave this as is, you should name it Find().
Non-getter methods should be capitalized.

[commit-bot: I haz the power, 2011-12-07 10:42:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/serya@chromium.org/8659002/38010

[commit-bot: I haz the power, 2011-12-07 10:42:10]

Presubmit check for 8659002-38010 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for: chrome/browser/profiles/profile_impl.cc

Presubmit checks took 2.2s to calculate.

[SeRya, 2011-12-07 10:50:42]

Dave | Miranda,

Could you review the change as an owner of
chrome/browser/profiles/profile_impl.cc, please?

[SeRya, 2011-12-07 12:40:18]

>
http://codereview.chromium.org/8659002/diff/28017/chrome/browser/profiles/pro...
> chrome/browser/profiles/profile_impl.cc:405:
> base::ThreadRestrictions::ScopedAllowIO allow_io;
> This is still making me nervous. Since you are already working on this, can
you
> please add some logging to understand how it was working without this? This
> might be a bad bug that we need to fix.

It was working because IO allowed during browser initialization.

Code in content/browser/browser_main_loop.cc:
1. Runs "pre-main" message loops (line 380).
2. Disallow IO (line 385).
3. Runs main message loops (line 389).

Profile initialization happens in the first phase.
	ProfileImpl::InitExtensions() [0x7f5c6040703e]
	ProfileManager::DoFinalInitForServices() [0x7f5c5fbea846]
	ProfileManager::DoFinalInit() [0x7f5c5fbea7ca]
	ProfileManager::AddProfile() [0x7f5c5fbe9fe0]
	ProfileManager::GetProfile() [0x7f5c5fbe997f]
	ProfileManager::GetDefaultProfile() [0x7f5c5fbe9625]
	(anonymous namespace)::CreateProfile() [0x7f5c5ffd15f8]
	ChromeBrowserMainParts::PreMainMessageLoopRunImpl() [0x7f5c5ffd67e5]
	ChromeBrowserMainParts::PreMainMessageLoopRun() [0x7f5c5ffd5dfc]
	ChromeBrowserMainPartsChromeos::PreMainMessageLoopRun() [0x7f5c5f9efbbd]
	content::BrowserMainLoop::RunMainMessageLoopParts() [0x7f5c62c4623f]
	BrowserMain() [0x7f5c62c44747]
	(anonymous namespace)::RunNamedProcessTypeMain() [0x7f5c60a4bbc9]
	content::ContentMain() [0x7f5c60a4c1aa]
	ChromeMain [0x7f5c5f8b0aa5]
	main [0x7f5c5f8b0a6c]
	0x7f5c57e8fc4d
	0x7f5c5f8b0979

[DaveMoore, 2011-12-07 14:22:08]

I understand I'm coming to this late, but haven't you created some opportunity
for the user to see some UI and then get blocked by IO? If not could you please
add a comment explaining why the IO is ok here?
On 2011/12/07 12:40:18, SeRya wrote:
> >
>
http://codereview.chromium.org/8659002/diff/28017/chrome/browser/profiles/pro...
> > chrome/browser/profiles/profile_impl.cc:405:
> > base::ThreadRestrictions::ScopedAllowIO allow_io;
> > This is still making me nervous. Since you are already working on this, can
> you
> > please add some logging to understand how it was working without this? This
> > might be a bad bug that we need to fix.
> 
> It was working because IO allowed during browser initialization.
> 
> Code in content/browser/browser_main_loop.cc:
> 1. Runs "pre-main" message loops (line 380).
> 2. Disallow IO (line 385).
> 3. Runs main message loops (line 389).
> 
> Profile initialization happens in the first phase.
> 	ProfileImpl::InitExtensions() [0x7f5c6040703e]
> 	ProfileManager::DoFinalInitForServices() [0x7f5c5fbea846]
> 	ProfileManager::DoFinalInit() [0x7f5c5fbea7ca]
> 	ProfileManager::AddProfile() [0x7f5c5fbe9fe0]
> 	ProfileManager::GetProfile() [0x7f5c5fbe997f]
> 	ProfileManager::GetDefaultProfile() [0x7f5c5fbe9625]
> 	(anonymous namespace)::CreateProfile() [0x7f5c5ffd15f8]
> 	ChromeBrowserMainParts::PreMainMessageLoopRunImpl() [0x7f5c5ffd67e5]
> 	ChromeBrowserMainParts::PreMainMessageLoopRun() [0x7f5c5ffd5dfc]
> 	ChromeBrowserMainPartsChromeos::PreMainMessageLoopRun() [0x7f5c5f9efbbd]
> 	content::BrowserMainLoop::RunMainMessageLoopParts() [0x7f5c62c4623f]
> 	BrowserMain() [0x7f5c62c44747]
> 	(anonymous namespace)::RunNamedProcessTypeMain() [0x7f5c60a4bbc9]
> 	content::ContentMain() [0x7f5c60a4c1aa]
> 	ChromeMain [0x7f5c5f8b0aa5]
> 	main [0x7f5c5f8b0a6c]
> 	0x7f5c57e8fc4d
> 	0x7f5c5f8b0979

[Miranda Callahan, 2011-12-07 15:35:21]

Yes, I'm a bit worried about the blocking, especially in a multi-profile
environment, where extension initialization will happen while other profiles are
already loaded and running on the same UI thread that may be blocked here.

Can you store this data with the Preferences, which are loaded asynchronously
before InitExtensions is called?

On 2011/12/07 14:22:08, DaveMoore wrote:
> I understand I'm coming to this late, but haven't you created some opportunity
> for the user to see some UI and then get blocked by IO? If not could you
please
> add a comment explaining why the IO is ok here?
> On 2011/12/07 12:40:18, SeRya wrote:
> > >
> >
>
http://codereview.chromium.org/8659002/diff/28017/chrome/browser/profiles/pro...
> > > chrome/browser/profiles/profile_impl.cc:405:
> > > base::ThreadRestrictions::ScopedAllowIO allow_io;
> > > This is still making me nervous. Since you are already working on this,
can
> > you
> > > please add some logging to understand how it was working without this?
This
> > > might be a bad bug that we need to fix.
> > 
> > It was working because IO allowed during browser initialization.
> > 
> > Code in content/browser/browser_main_loop.cc:
> > 1. Runs "pre-main" message loops (line 380).
> > 2. Disallow IO (line 385).
> > 3. Runs main message loops (line 389).
> > 
> > Profile initialization happens in the first phase.
> > 	ProfileImpl::InitExtensions() [0x7f5c6040703e]
> > 	ProfileManager::DoFinalInitForServices() [0x7f5c5fbea846]
> > 	ProfileManager::DoFinalInit() [0x7f5c5fbea7ca]
> > 	ProfileManager::AddProfile() [0x7f5c5fbe9fe0]
> > 	ProfileManager::GetProfile() [0x7f5c5fbe997f]
> > 	ProfileManager::GetDefaultProfile() [0x7f5c5fbe9625]
> > 	(anonymous namespace)::CreateProfile() [0x7f5c5ffd15f8]
> > 	ChromeBrowserMainParts::PreMainMessageLoopRunImpl() [0x7f5c5ffd67e5]
> > 	ChromeBrowserMainParts::PreMainMessageLoopRun() [0x7f5c5ffd5dfc]
> > 	ChromeBrowserMainPartsChromeos::PreMainMessageLoopRun() [0x7f5c5f9efbbd]
> > 	content::BrowserMainLoop::RunMainMessageLoopParts() [0x7f5c62c4623f]
> > 	BrowserMain() [0x7f5c62c44747]
> > 	(anonymous namespace)::RunNamedProcessTypeMain() [0x7f5c60a4bbc9]
> > 	content::ContentMain() [0x7f5c60a4c1aa]
> > 	ChromeMain [0x7f5c5f8b0aa5]
> > 	main [0x7f5c5f8b0a6c]
> > 	0x7f5c57e8fc4d
> > 	0x7f5c5f8b0979

[Aaron Boodman, 2011-12-07 22:21:15]

On 2011/12/07 15:35:21, Miranda Callahan wrote:
> Yes, I'm a bit worried about the blocking, especially in a multi-profile
> environment, where extension initialization will happen while other profiles
are
> already loaded and running on the same UI thread that may be blocked here.

This code only runs when a command line flag is present. The command line flag
is intended for developer use cases, so I think it's fine. No need to make it
more complex.

I didn't ask for a comment because the intention seems obvious (the code checks
for a command line flag right before the ScopedAllowIO).

I would prefer this to more complexity of bouncing to file thread.

[Aaron Boodman, 2011-12-07 22:22:49]

BTW, the latest patchset has gotten substantially more complex in an attempt to
go to the file thread. This is what I was trying to avoid.

[Miranda Callahan, 2011-12-07 22:50:10]

On 2011/12/07 22:22:49, Aaron Boodman wrote:
> BTW, the latest patchset has gotten substantially more complex in an attempt
to
> go to the file thread. This is what I was trying to avoid.

If the flag is only for developer use case, then that really does sound fine --
like there's no need for bouncing to the file thread. A comment would help here,
though -- at least for the reviewers to understand. Not all command-line flags
are for developer use only; had I known that this was the case, I would
certainly have let the simpler patch through.

[SeRya, 2011-12-07 23:42:20]

Comment added.

[Miranda Callahan, 2011-12-08 02:16:21]

On 2011/12/07 23:42:20, SeRya wrote:
> Comment added.

LGTM, and thank you.

[commit-bot: I haz the power, 2011-12-08 07:21:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/serya@chromium.org/8659002/58005

[commit-bot: I haz the power, 2011-12-08 08:52:13]

Change committed as 113578