Refactor loading out of ExtensionService.

Refactor loading out of ExtensionService.

- Make ExtensionServiceBackend go away, as  it's only used for installing unpacked extensions. Tear this out into UnpackedInstaller task.
- Pull out LoadAllExtensions / LoadInstalledExtension into InstalledExtensionLoader task.
- Break out component extensions to be handled by ComponentLoader.

Coming soon: a unit test for InstalledExtensionLoader.

BUG=97883
TEST=existing tests + InstalledExtensionLoaderUnittest


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

[Yoyo Zhou, 2011-10-28 00:19:15]

Sending this out for real.

I just realized I hadn't thought about external extensions at all, but I think
they are already not #occupying ExtensionService, so with this loading for all
Extension::Locations should be out of ExtensionService.

[Aaron Boodman, 2011-10-28 22:21:57]

This is sweet.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/automation/autom...
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/8417012/diff/1/chrome/browser/automation/autom...
chrome/browser/automation/automation_provider.cc:869:
installer->set_prompt_for_plugins(with_ui);
fff I think this might be the only caller who wants UI in this path. It doesn't
seem worth supporting just for tests (which I think is all this is used for).

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/compo...
File chrome/browser/extensions/component_loader.h (right):

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/compo...
chrome/browser/extensions/component_loader.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
What about this idea for a simplification:

class ComponentLoader {
 public:
  // No need to expose ComponentExtensionInfo struct.
  Add(const std::string& manifest, const FilePath&);
  Remove(const std::string& manifest);

  // When ES is NULL, calls to Add() and Remove() are queued.
  // When this method is called, all queued additions are loaded. 
  // If Add() or Remove() are called after this is set, they are
  // delegate through to ES immediately.
  SetExtensionService(ExtensionService*);
}

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_service.h (right):

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_service.h:437: // This method is public
because ExtensionLoadTask can post to here.
TODO(aa): DIEDIEDIE

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_service.h:513:
scoped_refptr<UnpackedInstaller> MakeUnpackedInstaller();
It seems a little unfortunate to me to have to have these methods in here.

I'm not super familiar with WeakPtr yet, but I think that if you have ES inherit
SupportsWeakPtr, you can just do:

new InstalledExtensionLoader(extension_service_->AsWeakPtr())

at the callsite. This would be a little more nicely decoupled IMO.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_service_unittest.cc (right):

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_service_unittest.cc:5: #include
"chrome/browser/extensions/extension_service_unittest.h"
It would be nice to pull these tests out in
unpacked_extension_installer_unittest.cc and so-on, but this change is large, so
that could be separate.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/insta...
File chrome/browser/extensions/installed_extension_loader.cc (right):

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/insta...
chrome/browser/extensions/installed_extension_loader.cc:26: // The following
enumeration is used in histograms matching
Nit: reformat comment.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/insta...
chrome/browser/extensions/installed_extension_loader.cc:90:
ReportExtensionLoadError(info.extension_path, error, false);
TODO(aa): Remove ReportExtensionLoadError. It doesn't
do enough to be worth the dependency on ES.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/insta...
chrome/browser/extensions/installed_extension_loader.cc:97:
extension_service()->AddExtension(extension);
We tend to just use the members directly inside the class: extension_service_.
Same in a few places in this file.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/insta...
chrome/browser/extensions/installed_extension_loader.cc:105: // Load the
previously installed extensions.
Seems like this comment can go now, since that is all this class does (yay).

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/insta...
chrome/browser/extensions/installed_extension_loader.cc:147:
Load(*extensions_info->at(i), should_write_prefs);
1. Doesn't this write prefs for all extensions, even if only one needs it? I
think we should only rewrite the prefs of the one that change.

2. If seems like it would be cleaner to do the rewriting in Load() itself. Since
we're also trying to do histograms across the entire LoadAll() transaction, you
could store the reload_reason_counts in a member, and have a separate
ReportReloadHistograms() method that you call at the end of LoadAll() that does
all the histogram stuff.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/insta...
chrome/browser/extensions/installed_extension_loader.cc:194: if (location ==
Extension::COMPONENT)
It seems like this part of the histograms code is not really a part of loading
installed extensions, except that it is supposed to run after it. It counts all
types of extensions loaded, not just the ones loaded by this class.

Consider moving back into ES. Or even having a separate
extension_load_histograms utils file.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/insta...
chrome/browser/extensions/installed_extension_loader.cc:265: int
InstalledExtensionLoader::GetExtensionCreateFlagsForInstalledExtension(
How about just: GetCreationFlags? The rest of the words seem implied by being in
this class and the ExtensionInfo parameter.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/insta...
File chrome/browser/extensions/installed_extension_loader.h (right):

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/insta...
chrome/browser/extensions/installed_extension_loader.h:12: #include
"base/memory/scoped_ptr.h"
Double check that all these includes and forward declarations are needed.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/insta...
chrome/browser/extensions/installed_extension_loader.h:21: // Loads installed
extensions from the .
Ooohh, suspense. Loads them from where?? I must know!

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/unpac...
File chrome/browser/extensions/unpacked_installer.cc (right):

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/unpac...
chrome/browser/extensions/unpacked_installer.cc:19: class
SimpleExtensionLoadPrompt : public ExtensionInstallUI::Delegate {
I think that we can remove this. I believe the only place it is used in the code
(the only path where we end up with prompt_for_plugins_ = true) is in the
automation file. And in that case, it would only fire for extensions with
plugins. I think that was most likely not intended, or being used.

We definitely don't need this for the current functionality: We always show
prompts when installing packed, and never for installing unpacked.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/unpac...
chrome/browser/extensions/unpacked_installer.cc:84: void
UnpackedInstaller::LoadFromCommandLine(
This only shares OnLoadSingleExtension with the rest of this class. Otherwise it
is totally different. Seems like it should be its own thing.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/unpac...
chrome/browser/extensions/unpacked_installer.cc:129:
base::Bind(&UnpackedInstaller::CheckExtensionFileAccess,
Add a TODO that it would be nice to remove this extra ping-pong, but we need the
flag to know what to pass to extension_file_util::Load().

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/unpac...
chrome/browser/extensions/unpacked_installer.cc:174: // Report this as an
installed extension so that it gets remembered in the
Comment is no longer needed since it is obvious from the name/purpose of this
class. (woo)

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/unpac...
chrome/browser/extensions/unpacked_installer.cc:211: // Not from web store.
Is this comment still legit? I don't see how it relates to above check.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/unpac...
File chrome/browser/extensions/unpacked_installer.h (right):

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/unpac...
chrome/browser/extensions/unpacked_installer.h:12: #include
"base/memory/scoped_ptr.h"
Is this include needed?

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/unpac...
chrome/browser/extensions/unpacked_installer.h:22: class UnpackedInstaller
I think it would make this a little cleaner if there were one of these classes
for each unpacked installation.

Right now, you can create one of these and call Load() on it multiple times, and
the arguments are passed back and forth between threads.

If instead you made it so that you need one of these per installation, some
state could be members of this class.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/unpac...
chrome/browser/extensions/unpacked_installer.h:22: class UnpackedInstaller
Throughout this change: consider putting all these new classes into the
extensions namespace.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/unpac...
chrome/browser/extensions/unpacked_installer.h:35: bool prompt_for_plugins() {
return prompt_for_plugins_; }
I think that this may no longer be needed. For awhile, we did not prompt when
installing extensions from the web store, but we had an exception for plugins.
This may have been introduced to control that.

Now we always prompt again, so maybe this can die.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/8417012/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_impl.cc:448:
extension_service_->component_loader()->RegisterDefaultComponentExtensions();
For now, if you make the change I suggested to ComponentLoader, you can just
call component_extensions::LoadDefault() or whatever from inside
ExtensionService::Init().

Longer term, I think we should probably have an ExtensionInit class or something
that kicks off all the various thingies in the right order.

http://codereview.chromium.org/8417012/diff/1/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/8417012/diff/1/chrome/chrome_browser.gypi#newc...
chrome/chrome_browser.gypi:965: 'browser/extensions/default_apps_trial.cc',
?

[Matt Perry, 2011-10-28 22:28:55]

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/unpac...
File chrome/browser/extensions/unpacked_installer.cc (right):

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/unpac...
chrome/browser/extensions/unpacked_installer.cc:19: class
SimpleExtensionLoadPrompt : public ExtensionInstallUI::Delegate {
On 2011/10/28 22:21:58, Aaron Boodman wrote:
> I think that we can remove this. I believe the only place it is used in the
code
> (the only path where we end up with prompt_for_plugins_ = true) is in the
> automation file. And in that case, it would only fire for extensions with
> plugins. I think that was most likely not intended, or being used.
> 
> We definitely don't need this for the current functionality: We always show
> prompts when installing packed, and never for installing unpacked.

Actually, we show prompts for the first unpacked-load of an extension with
plugins. http://code.google.com/p/chromium/issues/detail?id=83273

[Aaron Boodman, 2011-10-28 22:49:56]

On 2011/10/28 22:28:55, Matt Perry wrote:
>
http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/unpac...
> File chrome/browser/extensions/unpacked_installer.cc (right):
> 
>
http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/unpac...
> chrome/browser/extensions/unpacked_installer.cc:19: class
> SimpleExtensionLoadPrompt : public ExtensionInstallUI::Delegate {
> On 2011/10/28 22:21:58, Aaron Boodman wrote:
> > I think that we can remove this. I believe the only place it is used in the
> code
> > (the only path where we end up with prompt_for_plugins_ = true) is in the
> > automation file. And in that case, it would only fire for extensions with
> > plugins. I think that was most likely not intended, or being used.
> > 
> > We definitely don't need this for the current functionality: We always show
> > prompts when installing packed, and never for installing unpacked.
> 
> Actually, we show prompts for the first unpacked-load of an extension with
> plugins. http://code.google.com/p/chromium/issues/detail?id=83273

Took discussion to bug. Yoyo, I've cc'd you.

[Matt Perry, 2011-10-31 18:49:57]

BTW, I defer to aaron for the review, unless you prefer otherwise.

[Yoyo Zhou, 2011-10-31 21:58:15]

http://codereview.chromium.org/8417012/diff/1/chrome/browser/automation/autom...
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/8417012/diff/1/chrome/browser/automation/autom...
chrome/browser/automation/automation_provider.cc:869:
installer->set_prompt_for_plugins(with_ui);
On 2011/10/28 22:21:58, Aaron Boodman wrote:
> fff I think this might be the only caller who wants UI in this path. It
doesn't
> seem worth supporting just for tests (which I think is all this is used for).

This is actually for maybe disabling the UI. (See the comment there, and/or
Matt's comment.) I haven't really looked yet, but I assume having to deal with a
UI prompt in your test is annoying.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/compo...
File chrome/browser/extensions/component_loader.h (right):

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/compo...
chrome/browser/extensions/component_loader.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2011/10/28 22:21:58, Aaron Boodman wrote:
> What about this idea for a simplification:
> 
> class ComponentLoader {
>  public:
>   // No need to expose ComponentExtensionInfo struct.

Yeah, this is unnecessary. I'll get rid of this right now.

>   Add(const std::string& manifest, const FilePath&);
>   Remove(const std::string& manifest);
> 
>   // When ES is NULL, calls to Add() and Remove() are queued.
>   // When this method is called, all queued additions are loaded. 
>   // If Add() or Remove() are called after this is set, they are
>   // delegate through to ES immediately.
>   SetExtensionService(ExtensionService*);

So we'd associate this with the Profile then. Okay.

> }
> 

As I understand it, the reasons for having this registering are twofold:
1. having component extensions available at startup,
2. not forgetting about them when we ReloadExtensions() (only in tests).

Sounds like this addresses issue 1 but not 2. Maybe for that, the right thing to
do is handle them specially when we do ReloadExtensions/UnloadAllExtensions.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_browsertest.cc (right):

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_browsertest.cc:73:
installer->set_prompt_for_plugins(false);
FYI, here's the one other place where we _disable_ the UI prompting for plugins.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_service.h (right):

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_service.h:513:
scoped_refptr<UnpackedInstaller> MakeUnpackedInstaller();
On 2011/10/28 22:21:58, Aaron Boodman wrote:
> It seems a little unfortunate to me to have to have these methods in here.
> 
> I'm not super familiar with WeakPtr yet, but I think that if you have ES
inherit
> SupportsWeakPtr, you can just do:
> 
> new InstalledExtensionLoader(extension_service_->AsWeakPtr())
> 
> at the callsite. This would be a little more nicely decoupled IMO.

Heh, I went back and forth on this myself. I did look into weak_ptr.h and it
works as you suggest. Originally I thought I'd have to create an object outside
the ES, and I'd have no choice but to use SupportsWeakPtr. But then I didn't.

I then found akalin's change where he added WeakPtrFactory to ES, and he wrote
this:

"Yeah, I thought of making ExtensionService inherit from base::SupportsWeakPtr
(which adds an AsWeakPtr() method) but ultimately decided against it.  The only
things that really need the weakptr are ExtensionBackend and CrxInstaller;
everything else is tied to the profile's lifetime.  It seems like an
implementation detail that the service passes itself as a weak ptr to
CrxInstaller so I thought it better to hide it behind a function."

Of course, now that we have 3, we should perhaps reassess.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_service_unittest.cc (right):

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_service_unittest.cc:5: #include
"chrome/browser/extensions/extension_service_unittest.h"
On 2011/10/28 22:21:58, Aaron Boodman wrote:
> It would be nice to pull these tests out in
> unpacked_extension_installer_unittest.cc and so-on, but this change is large,
so
> that could be separate.

I was considering doing this, though at the time it didn't look obvious which
tests to pull out (but here it does). Maybe I'll do that together later, with
adding tests for InstalledExtensionLoader.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/insta...
File chrome/browser/extensions/installed_extension_loader.cc (right):

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/insta...
chrome/browser/extensions/installed_extension_loader.cc:26: // The following
enumeration is used in histograms matching
On 2011/10/28 22:21:58, Aaron Boodman wrote:
> Nit: reformat comment.

Done.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/insta...
chrome/browser/extensions/installed_extension_loader.cc:90:
ReportExtensionLoadError(info.extension_path, error, false);
On 2011/10/28 22:21:58, Aaron Boodman wrote:
> TODO(aa): Remove ReportExtensionLoadError. It doesn't
> do enough to be worth the dependency on ES.

Added this TODO.

It's really a Profile dependency. Both UnpackedInstaller and
InstalledExtensionLoader use it, so they could theoretically inherit from a
common loading-task-class, but that didn't seem worth the complexity to me.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/insta...
chrome/browser/extensions/installed_extension_loader.cc:147:
Load(*extensions_info->at(i), should_write_prefs);
On 2011/10/28 22:21:58, Aaron Boodman wrote:
> 1. Doesn't this write prefs for all extensions, even if only one needs it? I
> think we should only rewrite the prefs of the one that change.

Yes, this looks silly. I think I'd like to change actual behavior in another CL,
though.

> 2. If seems like it would be cleaner to do the rewriting in Load() itself.
Since
> we're also trying to do histograms across the entire LoadAll() transaction,
you
> could store the reload_reason_counts in a member, and have a separate
> ReportReloadHistograms() method that you call at the end of LoadAll() that
does
> all the histogram stuff.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/insta...
chrome/browser/extensions/installed_extension_loader.cc:194: if (location ==
Extension::COMPONENT)
On 2011/10/28 22:21:58, Aaron Boodman wrote:
> It seems like this part of the histograms code is not really a part of loading
> installed extensions, except that it is supposed to run after it. It counts
all
> types of extensions loaded, not just the ones loaded by this class.

Hmm, it doesn't count component extensions or LOAD (--load-extension), so that
just leaves the ones loaded by this class. Am I missing some?

Implementation-wise, if that is the case, your earlier comment suggests to me
that we should increment the counts in the Load() iteration above.

> Consider moving back into ES. Or even having a separate
> extension_load_histograms utils file.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/insta...
chrome/browser/extensions/installed_extension_loader.cc:265: int
InstalledExtensionLoader::GetExtensionCreateFlagsForInstalledExtension(
On 2011/10/28 22:21:58, Aaron Boodman wrote:
> How about just: GetCreationFlags? The rest of the words seem implied by being
in
> this class and the ExtensionInfo parameter.

Yes.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/insta...
File chrome/browser/extensions/installed_extension_loader.h (right):

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/insta...
chrome/browser/extensions/installed_extension_loader.h:12: #include
"base/memory/scoped_ptr.h"
On 2011/10/28 22:21:58, Aaron Boodman wrote:
> Double check that all these includes and forward declarations are needed.

Done.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/insta...
chrome/browser/extensions/installed_extension_loader.h:21: // Loads installed
extensions from the .
On 2011/10/28 22:21:58, Aaron Boodman wrote:
> Ooohh, suspense. Loads them from where?? I must know!

leave 'em hanging.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/unpac...
File chrome/browser/extensions/unpacked_installer.cc (right):

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/unpac...
chrome/browser/extensions/unpacked_installer.cc:19: class
SimpleExtensionLoadPrompt : public ExtensionInstallUI::Delegate {
On 2011/10/28 22:21:58, Aaron Boodman wrote:
> I think that we can remove this. I believe the only place it is used in the
code
> (the only path where we end up with prompt_for_plugins_ = true) is in the
> automation file. And in that case, it would only fire for extensions with
> plugins. I think that was most likely not intended, or being used.
> 
> We definitely don't need this for the current functionality: We always show
> prompts when installing packed, and never for installing unpacked.

prompt_for_plugins is supposed to be true by default. I must have lost that
somewhere!

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/unpac...
chrome/browser/extensions/unpacked_installer.cc:84: void
UnpackedInstaller::LoadFromCommandLine(
On 2011/10/28 22:21:58, Aaron Boodman wrote:
> This only shares OnLoadSingleExtension with the rest of this class. Otherwise
it
> is totally different. Seems like it should be its own thing.

This actually does exactly the same thing as the rest of the class, but
synchronously. Maybe I should clarify that in the header comments.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/unpac...
chrome/browser/extensions/unpacked_installer.cc:129:
base::Bind(&UnpackedInstaller::CheckExtensionFileAccess,
On 2011/10/28 22:21:58, Aaron Boodman wrote:
> Add a TODO that it would be nice to remove this extra ping-pong, but we need
the
> flag to know what to pass to extension_file_util::Load().

Done.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/unpac...
chrome/browser/extensions/unpacked_installer.cc:174: // Report this as an
installed extension so that it gets remembered in the
On 2011/10/28 22:21:58, Aaron Boodman wrote:
> Comment is no longer needed since it is obvious from the name/purpose of this
> class. (woo)

Done.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/unpac...
chrome/browser/extensions/unpacked_installer.cc:211: // Not from web store.
On 2011/10/28 22:21:58, Aaron Boodman wrote:
> Is this comment still legit? I don't see how it relates to above check.

I believe the comment is about the 'false' parameter.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/unpac...
File chrome/browser/extensions/unpacked_installer.h (right):

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/unpac...
chrome/browser/extensions/unpacked_installer.h:12: #include
"base/memory/scoped_ptr.h"
On 2011/10/28 22:21:58, Aaron Boodman wrote:
> Is this include needed?

No.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/unpac...
chrome/browser/extensions/unpacked_installer.h:22: class UnpackedInstaller
On 2011/10/28 22:21:58, Aaron Boodman wrote:
> I think it would make this a little cleaner if there were one of these classes
> for each unpacked installation.
> 
> Right now, you can create one of these and call Load() on it multiple times,
and
> the arguments are passed back and forth between threads.
> 
> If instead you made it so that you need one of these per installation, some
> state could be members of this class.

It's meant to be used as one per installation, like CrxInstaller. That's how the
uses are (now). It just doesn't have as much state.

How could you enforce such usage?

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/unpac...
chrome/browser/extensions/unpacked_installer.h:22: class UnpackedInstaller
On 2011/10/28 22:21:58, Aaron Boodman wrote:
> Throughout this change: consider putting all these new classes into the
> extensions namespace.

Done.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/unpac...
chrome/browser/extensions/unpacked_installer.h:35: bool prompt_for_plugins() {
return prompt_for_plugins_; }
On 2011/10/28 22:21:58, Aaron Boodman wrote:
> I think that this may no longer be needed. For awhile, we did not prompt when
> installing extensions from the web store, but we had an exception for plugins.
> This may have been introduced to control that.
> 
> Now we always prompt again, so maybe this can die.

This is used in tests. I'm not sure about removing it yet. In any case, it's
much less obtrusive here than it previously was, passed around as an argument at
every step.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/8417012/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_impl.cc:448:
extension_service_->component_loader()->RegisterDefaultComponentExtensions();
On 2011/10/28 22:21:58, Aaron Boodman wrote:
> For now, if you make the change I suggested to ComponentLoader, you can just
> call component_extensions::LoadDefault() or whatever from inside
> ExtensionService::Init().
> 
> Longer term, I think we should probably have an ExtensionInit class or
something
> that kicks off all the various thingies in the right order.

+1

I've seen quite a few subtle init bugs, and looking at startup code always makes
me uneasy, like it will collapse if I look at it funny.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_impl.cc:448:
extension_service_->component_loader()->RegisterDefaultComponentExtensions();
On 2011/10/28 22:21:58, Aaron Boodman wrote:
> For now, if you make the change I suggested to ComponentLoader, you can just
> call component_extensions::LoadDefault() or whatever from inside
> ExtensionService::Init().
> 
> Longer term, I think we should probably have an ExtensionInit class or
something
> that kicks off all the various thingies in the right order.

http://codereview.chromium.org/8417012/diff/1/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/8417012/diff/1/chrome/chrome_browser.gypi#newc...
chrome/chrome_browser.gypi:965: 'browser/extensions/default_apps_trial.cc',
On 2011/10/28 22:21:58, Aaron Boodman wrote:
> ?

Weird.

[Aaron Boodman, 2011-10-31 22:33:25]

http://codereview.chromium.org/8417012/diff/1/chrome/browser/automation/autom...
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/8417012/diff/1/chrome/browser/automation/autom...
chrome/browser/automation/automation_provider.cc:869:
installer->set_prompt_for_plugins(with_ui);
On 2011/10/31 21:58:15, Yoyo Zhou wrote:
> On 2011/10/28 22:21:58, Aaron Boodman wrote:
> > fff I think this might be the only caller who wants UI in this path. It
> doesn't
> > seem worth supporting just for tests (which I think is all this is used
for).
> 
> This is actually for maybe disabling the UI. (See the comment there, and/or
> Matt's comment.) I haven't really looked yet, but I assume having to deal with
a
> UI prompt in your test is annoying.

Yeah, after seeing Matt's comments and the related bug report, I understand. And
I agree that fixing this is out of scope for your CL.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/compo...
File chrome/browser/extensions/component_loader.h (right):

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/compo...
chrome/browser/extensions/component_loader.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2011/10/31 21:58:15, Yoyo Zhou wrote:
> On 2011/10/28 22:21:58, Aaron Boodman wrote:
> > What about this idea for a simplification:
> > 
> > class ComponentLoader {
> >  public:
> >   // No need to expose ComponentExtensionInfo struct.
> 
> Yeah, this is unnecessary. I'll get rid of this right now.
> 
> >   Add(const std::string& manifest, const FilePath&);
> >   Remove(const std::string& manifest);
> > 
> >   // When ES is NULL, calls to Add() and Remove() are queued.
> >   // When this method is called, all queued additions are loaded. 
> >   // If Add() or Remove() are called after this is set, they are
> >   // delegate through to ES immediately.
> >   SetExtensionService(ExtensionService*);
> 
> So we'd associate this with the Profile then. Okay.

I think it needs to be a property of ES, at least for now, because it doesn't
make sense to duplicate in Incognito profiles.

> > }
> > 
> 
> As I understand it, the reasons for having this registering are twofold:
> 1. having component extensions available at startup,
> 2. not forgetting about them when we ReloadExtensions() (only in tests).
> 
> Sounds like this addresses issue 1 but not 2. Maybe for that, the right thing
to
> do is handle them specially when we do ReloadExtensions/UnloadAllExtensions.

In that case, you could keep track of them either way in ComponentLoader. I'm
just trying to find a way to get rid of the weird duplicate Register and Add
methods.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_service.h (right):

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_service.h:513:
scoped_refptr<UnpackedInstaller> MakeUnpackedInstaller();
On 2011/10/31 21:58:15, Yoyo Zhou wrote:
> On 2011/10/28 22:21:58, Aaron Boodman wrote:
> > It seems a little unfortunate to me to have to have these methods in here.
> > 
> > I'm not super familiar with WeakPtr yet, but I think that if you have ES
> inherit
> > SupportsWeakPtr, you can just do:
> > 
> > new InstalledExtensionLoader(extension_service_->AsWeakPtr())
> > 
> > at the callsite. This would be a little more nicely decoupled IMO.
> 
> Heh, I went back and forth on this myself. I did look into weak_ptr.h and it
> works as you suggest. Originally I thought I'd have to create an object
outside
> the ES, and I'd have no choice but to use SupportsWeakPtr. But then I didn't.
> 
> I then found akalin's change where he added WeakPtrFactory to ES, and he wrote
> this:
> 
> "Yeah, I thought of making ExtensionService inherit from base::SupportsWeakPtr
> (which adds an AsWeakPtr() method) but ultimately decided against it.  The
only
> things that really need the weakptr are ExtensionBackend and CrxInstaller;
> everything else is tied to the profile's lifetime.  It seems like an
> implementation detail that the service passes itself as a weak ptr to
> CrxInstaller so I thought it better to hide it behind a function."
> 
> Of course, now that we have 3, we should perhaps reassess.

Can you try it? I'm really fired up about the idea of not having these depend on
ExtensionService long-term.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/insta...
File chrome/browser/extensions/installed_extension_loader.cc (right):

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/insta...
chrome/browser/extensions/installed_extension_loader.cc:194: if (location ==
Extension::COMPONENT)
On 2011/10/31 21:58:15, Yoyo Zhou wrote:
> On 2011/10/28 22:21:58, Aaron Boodman wrote:
> > It seems like this part of the histograms code is not really a part of
loading
> > installed extensions, except that it is supposed to run after it. It counts
> all
> > types of extensions loaded, not just the ones loaded by this class.
> 
> Hmm, it doesn't count component extensions or LOAD (--load-extension), so that
> just leaves the ones loaded by this class. Am I missing some?

Ok, if you want to be correct.

> Implementation-wise, if that is the case, your earlier comment suggests to me
> that we should increment the counts in the Load() iteration above.

Yeah, that makes sense. Sorry, I read the code wrong initially.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/unpac...
File chrome/browser/extensions/unpacked_installer.cc (right):

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/unpac...
chrome/browser/extensions/unpacked_installer.cc:84: void
UnpackedInstaller::LoadFromCommandLine(
On 2011/10/31 21:58:15, Yoyo Zhou wrote:
> On 2011/10/28 22:21:58, Aaron Boodman wrote:
> > This only shares OnLoadSingleExtension with the rest of this class.
Otherwise
> it
> > is totally different. Seems like it should be its own thing.
> 
> This actually does exactly the same thing as the rest of the class, but
> synchronously. Maybe I should clarify that in the header comments.

What I meant is that it doesn't rely on anything else in this class, so it seems
it could be separate if not for OnLoadSingleExtension.

In the glorious future, when the stupid plugin prompt goes away, there wouldn't
be anything in OnExtensionLoaded, so it seems it might be possible to pull this
out.

But since that isn't true now, nevermind me.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/unpac...
chrome/browser/extensions/unpacked_installer.cc:211: // Not from web store.
On 2011/10/31 21:58:15, Yoyo Zhou wrote:
> On 2011/10/28 22:21:58, Aaron Boodman wrote:
> > Is this comment still legit? I don't see how it relates to above check.
> 
> I believe the comment is about the 'false' parameter.

Ah, can you format it like this then:

extension,
false,  // not from web store
-1);  // magic

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/unpac...
File chrome/browser/extensions/unpacked_installer.h (right):

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/unpac...
chrome/browser/extensions/unpacked_installer.h:22: class UnpackedInstaller
On 2011/10/31 21:58:15, Yoyo Zhou wrote:
> On 2011/10/28 22:21:58, Aaron Boodman wrote:
> > I think it would make this a little cleaner if there were one of these
classes
> > for each unpacked installation.
> > 
> > Right now, you can create one of these and call Load() on it multiple times,
> and
> > the arguments are passed back and forth between threads.
> > 
> > If instead you made it so that you need one of these per installation, some
> > state could be members of this class.
> 
> It's meant to be used as one per installation, like CrxInstaller. That's how
the
> uses are (now). It just doesn't have as much state.

Ok, I was reacting to the file paths getting passed from method to method as
arguments. On second look it isn't a big deal. Nevermind.

> How could you enforce such usage?

Dunno.

http://codereview.chromium.org/8417012/diff/11001/chrome/browser/extensions/i...
File chrome/browser/extensions/installed_extension_loader.h (right):

http://codereview.chromium.org/8417012/diff/11001/chrome/browser/extensions/i...
chrome/browser/extensions/installed_extension_loader.h:19: class
InstalledExtensionLoader
You could get rid of the "Extension" from the class name now.

[Yoyo Zhou, 2011-11-01 21:50:56]

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/compo...
File chrome/browser/extensions/component_loader.h (right):

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/compo...
chrome/browser/extensions/component_loader.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2011/10/31 22:33:25, Aaron Boodman wrote:
> On 2011/10/31 21:58:15, Yoyo Zhou wrote:
> > On 2011/10/28 22:21:58, Aaron Boodman wrote:
> > > What about this idea for a simplification:
> > > 
> > > class ComponentLoader {
> > >  public:
> > >   // No need to expose ComponentExtensionInfo struct.
> > 
> > Yeah, this is unnecessary. I'll get rid of this right now.
> > 
> > >   Add(const std::string& manifest, const FilePath&);
> > >   Remove(const std::string& manifest);
> > > 
> > >   // When ES is NULL, calls to Add() and Remove() are queued.
> > >   // When this method is called, all queued additions are loaded. 
> > >   // If Add() or Remove() are called after this is set, they are
> > >   // delegate through to ES immediately.

> > >   SetExtensionService(ExtensionService*);

You can't do this if it's a property of ExtensionService.

> > 
> > So we'd associate this with the Profile then. Okay.
> 
> I think it needs to be a property of ES, at least for now, because it doesn't
> make sense to duplicate in Incognito profiles.

"When ES is null..." Actually, we register when ES has been created but not
initialized. I think I should use ES::is_ready() for this. Here's my plan:

Case 1 (startup):
Add() registers. If ES is initialized, it also loads. Otherwise it doesn't load.
ES::Init calls LoadAll() which loads everything. A slight problem here is the
guarantee that things are loaded eventually isn't built into ComponentLoader.

Remove() unregisters and unloads.

Case 2 (reload all):
ES::ReloadAll calls LoadAll().

> 
> > > }
> > > 
> > 
> > As I understand it, the reasons for having this registering are twofold:
> > 1. having component extensions available at startup,
> > 2. not forgetting about them when we ReloadExtensions() (only in tests).
> > 
> > Sounds like this addresses issue 1 but not 2. Maybe for that, the right
thing
> to
> > do is handle them specially when we do ReloadExtensions/UnloadAllExtensions.
> 
> In that case, you could keep track of them either way in ComponentLoader. I'm
> just trying to find a way to get rid of the weird duplicate Register and Add
> methods.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_service.h (right):

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_service.h:513:
scoped_refptr<UnpackedInstaller> MakeUnpackedInstaller();
On 2011/10/31 22:33:25, Aaron Boodman wrote:
> On 2011/10/31 21:58:15, Yoyo Zhou wrote:
> > On 2011/10/28 22:21:58, Aaron Boodman wrote:
> > > It seems a little unfortunate to me to have to have these methods in here.
> > > 
> > > I'm not super familiar with WeakPtr yet, but I think that if you have ES
> > inherit
> > > SupportsWeakPtr, you can just do:
> > > 
> > > new InstalledExtensionLoader(extension_service_->AsWeakPtr())
> > > 
> > > at the callsite. This would be a little more nicely decoupled IMO.
> > 
> > Heh, I went back and forth on this myself. I did look into weak_ptr.h and it
> > works as you suggest. Originally I thought I'd have to create an object
> outside
> > the ES, and I'd have no choice but to use SupportsWeakPtr. But then I
didn't.
> > 
> > I then found akalin's change where he added WeakPtrFactory to ES, and he
wrote
> > this:
> > 
> > "Yeah, I thought of making ExtensionService inherit from
base::SupportsWeakPtr
> > (which adds an AsWeakPtr() method) but ultimately decided against it.  The
> only
> > things that really need the weakptr are ExtensionBackend and CrxInstaller;
> > everything else is tied to the profile's lifetime.  It seems like an
> > implementation detail that the service passes itself as a weak ptr to
> > CrxInstaller so I thought it better to hide it behind a function."
> > 
> > Of course, now that we have 3, we should perhaps reassess.
> 
> Can you try it? I'm really fired up about the idea of not having these depend
on
> ExtensionService long-term.

Got rid of these two:
1. changed ES::MakeUnpackedInstaller to UnpackedInstaller::Create
2. made InstalledLoader just a regular object (it lives only one one thread); it
looks a little odd though

If this looks ok (see ExtensionService::ReloadExtension for usage example), I'll
do the same for CrxInstaller.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/unpac...
File chrome/browser/extensions/unpacked_installer.cc (right):

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/unpac...
chrome/browser/extensions/unpacked_installer.cc:211: // Not from web store.
On 2011/10/31 22:33:25, Aaron Boodman wrote:
> On 2011/10/31 21:58:15, Yoyo Zhou wrote:
> > On 2011/10/28 22:21:58, Aaron Boodman wrote:
> > > Is this comment still legit? I don't see how it relates to above check.
> > 
> > I believe the comment is about the 'false' parameter.
> 
> Ah, can you format it like this then:
> 
> extension,
> false,  // not from web store
> -1);  // magic

Done.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/unpac...
File chrome/browser/extensions/unpacked_installer.h (right):

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/unpac...
chrome/browser/extensions/unpacked_installer.h:22: class UnpackedInstaller
On 2011/10/31 22:33:25, Aaron Boodman wrote:
> On 2011/10/31 21:58:15, Yoyo Zhou wrote:
> > On 2011/10/28 22:21:58, Aaron Boodman wrote:
> > > I think it would make this a little cleaner if there were one of these
> classes
> > > for each unpacked installation.
> > > 
> > > Right now, you can create one of these and call Load() on it multiple
times,
> > and
> > > the arguments are passed back and forth between threads.
> > > 
> > > If instead you made it so that you need one of these per installation,
some
> > > state could be members of this class.
> > 
> > It's meant to be used as one per installation, like CrxInstaller. That's how
> the
> > uses are (now). It just doesn't have as much state.
> 
> Ok, I was reacting to the file paths getting passed from method to method as
> arguments. On second look it isn't a big deal. Nevermind.

Oh, I suppose I could stop passing them around. It becomes slightly cleaner. I'm
going to rename some of the methods now.

http://codereview.chromium.org/8417012/diff/11001/chrome/browser/extensions/i...
File chrome/browser/extensions/installed_extension_loader.h (right):

http://codereview.chromium.org/8417012/diff/11001/chrome/browser/extensions/i...
chrome/browser/extensions/installed_extension_loader.h:19: class
InstalledExtensionLoader
On 2011/10/31 22:33:25, Aaron Boodman wrote:
> You could get rid of the "Extension" from the class name now.

Done.

[Aaron Boodman, 2011-11-01 22:15:14]

On 2011/11/01 21:50:56, Yoyo Zhou wrote:
>
http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/compo...
> File chrome/browser/extensions/component_loader.h (right):
> 
>
http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/compo...
> chrome/browser/extensions/component_loader.h:1: // Copyright (c) 2011 The
> Chromium Authors. All rights reserved.
> On 2011/10/31 22:33:25, Aaron Boodman wrote:
> > On 2011/10/31 21:58:15, Yoyo Zhou wrote:
> > > On 2011/10/28 22:21:58, Aaron Boodman wrote:
> > > > What about this idea for a simplification:
> > > > 
> > > > class ComponentLoader {
> > > >  public:
> > > >   // No need to expose ComponentExtensionInfo struct.
> > > 
> > > Yeah, this is unnecessary. I'll get rid of this right now.
> > > 
> > > >   Add(const std::string& manifest, const FilePath&);
> > > >   Remove(const std::string& manifest);
> > > > 
> > > >   // When ES is NULL, calls to Add() and Remove() are queued.
> > > >   // When this method is called, all queued additions are loaded. 
> > > >   // If Add() or Remove() are called after this is set, they are
> > > >   // delegate through to ES immediately.
> 
> > > >   SetExtensionService(ExtensionService*);
> 
> You can't do this if it's a property of ExtensionService.
> 
> > > 
> > > So we'd associate this with the Profile then. Okay.
> > 
> > I think it needs to be a property of ES, at least for now, because it
doesn't
> > make sense to duplicate in Incognito profiles.
> 
> "When ES is null..." Actually, we register when ES has been created but not
> initialized. I think I should use ES::is_ready() for this. Here's my plan:
> 
> Case 1 (startup):
> Add() registers. If ES is initialized, it also loads. Otherwise it doesn't
load.
> ES::Init calls LoadAll() which loads everything. A slight problem here is the
> guarantee that things are loaded eventually isn't built into ComponentLoader.
> 
> Remove() unregisters and unloads.
> 
> Case 2 (reload all):
> ES::ReloadAll calls LoadAll().

Ok.

> > > As I understand it, the reasons for having this registering are twofold:
> > > 1. having component extensions available at startup,
> > > 2. not forgetting about them when we ReloadExtensions() (only in tests).
> > > 
> > > Sounds like this addresses issue 1 but not 2. Maybe for that, the right
> thing
> > to
> > > do is handle them specially when we do
ReloadExtensions/UnloadAllExtensions.
> > 
> > In that case, you could keep track of them either way in ComponentLoader.
I'm
> > just trying to find a way to get rid of the weird duplicate Register and Add
> > methods.
> 
>
http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/exten...
> File chrome/browser/extensions/extension_service.h (right):
> 
>
http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/exten...
> chrome/browser/extensions/extension_service.h:513:
> scoped_refptr<UnpackedInstaller> MakeUnpackedInstaller();
> On 2011/10/31 22:33:25, Aaron Boodman wrote:
> > On 2011/10/31 21:58:15, Yoyo Zhou wrote:
> > > On 2011/10/28 22:21:58, Aaron Boodman wrote:
> > > > It seems a little unfortunate to me to have to have these methods in
here.
> > > > 
> > > > I'm not super familiar with WeakPtr yet, but I think that if you have ES
> > > inherit
> > > > SupportsWeakPtr, you can just do:
> > > > 
> > > > new InstalledExtensionLoader(extension_service_->AsWeakPtr())
> > > > 
> > > > at the callsite. This would be a little more nicely decoupled IMO.
> > > 
> > > Heh, I went back and forth on this myself. I did look into weak_ptr.h and
it
> > > works as you suggest. Originally I thought I'd have to create an object
> > outside
> > > the ES, and I'd have no choice but to use SupportsWeakPtr. But then I
> didn't.
> > > 
> > > I then found akalin's change where he added WeakPtrFactory to ES, and he
> wrote
> > > this:
> > > 
> > > "Yeah, I thought of making ExtensionService inherit from
> base::SupportsWeakPtr
> > > (which adds an AsWeakPtr() method) but ultimately decided against it.  The
> > only
> > > things that really need the weakptr are ExtensionBackend and CrxInstaller;
> > > everything else is tied to the profile's lifetime.  It seems like an
> > > implementation detail that the service passes itself as a weak ptr to
> > > CrxInstaller so I thought it better to hide it behind a function."
> > > 
> > > Of course, now that we have 3, we should perhaps reassess.
> > 
> > Can you try it? I'm really fired up about the idea of not having these
depend
> on
> > ExtensionService long-term.
> 
> Got rid of these two:
> 1. changed ES::MakeUnpackedInstaller to UnpackedInstaller::Create
> 2. made InstalledLoader just a regular object (it lives only one one thread);
it
> looks a little odd though
> 
> If this looks ok (see ExtensionService::ReloadExtension for usage example),
I'll
> do the same for CrxInstaller.

LG. You're right that some of it does seem a little odd, but we can keep working
on it in subsequent changes.

http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/unpac...
> File chrome/browser/extensions/unpacked_installer.cc (right):
> 
>
http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/unpac...
> chrome/browser/extensions/unpacked_installer.cc:211: // Not from web store.
> On 2011/10/31 22:33:25, Aaron Boodman wrote:
> > On 2011/10/31 21:58:15, Yoyo Zhou wrote:
> > > On 2011/10/28 22:21:58, Aaron Boodman wrote:
> > > > Is this comment still legit? I don't see how it relates to above check.
> > > 
> > > I believe the comment is about the 'false' parameter.
> > 
> > Ah, can you format it like this then:
> > 
> > extension,
> > false,  // not from web store
> > -1);  // magic
> 
> Done.
> 
>
http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/unpac...
> File chrome/browser/extensions/unpacked_installer.h (right):
> 
>
http://codereview.chromium.org/8417012/diff/1/chrome/browser/extensions/unpac...
> chrome/browser/extensions/unpacked_installer.h:22: class UnpackedInstaller
> On 2011/10/31 22:33:25, Aaron Boodman wrote:
> > On 2011/10/31 21:58:15, Yoyo Zhou wrote:
> > > On 2011/10/28 22:21:58, Aaron Boodman wrote:
> > > > I think it would make this a little cleaner if there were one of these
> > classes
> > > > for each unpacked installation.
> > > > 
> > > > Right now, you can create one of these and call Load() on it multiple
> times,
> > > and
> > > > the arguments are passed back and forth between threads.
> > > > 
> > > > If instead you made it so that you need one of these per installation,
> some
> > > > state could be members of this class.
> > > 
> > > It's meant to be used as one per installation, like CrxInstaller. That's
how
> > the
> > > uses are (now). It just doesn't have as much state.
> > 
> > Ok, I was reacting to the file paths getting passed from method to method as
> > arguments. On second look it isn't a big deal. Nevermind.
> 
> Oh, I suppose I could stop passing them around. It becomes slightly cleaner.
I'm
> going to rename some of the methods now.
> 
>
http://codereview.chromium.org/8417012/diff/11001/chrome/browser/extensions/i...
> File chrome/browser/extensions/installed_extension_loader.h (right):
> 
>
http://codereview.chromium.org/8417012/diff/11001/chrome/browser/extensions/i...
> chrome/browser/extensions/installed_extension_loader.h:19: class
> InstalledExtensionLoader
> On 2011/10/31 22:33:25, Aaron Boodman wrote:
> > You could get rid of the "Extension" from the class name now.
> 
> Done.

[Aaron Boodman, 2011-11-01 22:15:47]

LGTM

You don't need to wait for another lg from me

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

http://codereview.chromium.org/8417012/diff/17001/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader.cc:134: void
ComponentLoader::AddDefaultComponentExtensions() {
Can this just be done implicitly in the constructor? Is there any need to add at
the specific time this method is currently being called in ES? It seems like not
since we just call LoadAll() later.

http://codereview.chromium.org/8417012/diff/17001/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader.cc:217:
RegisterComponentExtension(manifest, path);
Should be Add() now?

[Yoyo Zhou, 2011-11-01 22:45:06]

Ok, I'll kill MakeCrxInstaller too and run more try jobs before I submit.

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

http://codereview.chromium.org/8417012/diff/17001/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader.cc:134: void
ComponentLoader::AddDefaultComponentExtensions() {
On 2011/11/01 22:15:47, Aaron Boodman wrote:
> Can this just be done implicitly in the constructor? Is there any need to add
at
> the specific time this method is currently being called in ES? It seems like
not
> since we just call LoadAll() later.

IIRC, some tests that create an ExtensionService without a Profile depend on
them not being added.

http://codereview.chromium.org/8417012/diff/17001/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader.cc:217:
RegisterComponentExtension(manifest, path);
On 2011/11/01 22:15:47, Aaron Boodman wrote:
> Should be Add() now?

Yes.

[Aaron Boodman, 2011-11-01 22:46:34]

On Tue, Nov 1, 2011 at 3:45 PM,  <yoz@chromium.org> wrote:
> Ok, I'll kill MakeCrxInstaller too and run more try jobs before I submit.
>
>
>
http://codereview.chromium.org/8417012/diff/17001/chrome/browser/extensions/c...
> File chrome/browser/extensions/component_loader.cc (right):
>
>
http://codereview.chromium.org/8417012/diff/17001/chrome/browser/extensions/c...
> chrome/browser/extensions/component_loader.cc:134: void
> ComponentLoader::AddDefaultComponentExtensions() {
> On 2011/11/01 22:15:47, Aaron Boodman wrote:
>>
>> Can this just be done implicitly in the constructor? Is there any need
>
> to add at
>>
>> the specific time this method is currently being called in ES? It
>
> seems like not
>>
>> since we just call LoadAll() later.
>
> IIRC, some tests that create an ExtensionService without a Profile
> depend on them not being added.

I see, makes sense. Thanks for your patience :)

- a

[commit-bot: I haz the power, 2011-11-02 15:52:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/8417012/20001

[commit-bot: I haz the power, 2011-11-02 15:52:27]

Presubmit check for 8417012-20001 failed and returned exit status 1.

Running presubmit commit checks ...

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

Presubmit checks took 3.5s to calculate.

[Yoyo Zhou, 2011-11-02 16:08:52]

Miranda, can you review for profiles/?

[Miranda Callahan, 2011-11-02 16:24:21]

On 2011/11/02 16:08:52, Yoyo Zhou wrote:
> Miranda, can you review for profiles/?

LGTM for profiles/!

[commit-bot: I haz the power, 2011-11-02 16:48:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/8417012/20001

[commit-bot: I haz the power, 2011-11-02 18:18:04]

Change committed as 108319