Install CRX updates one at a time.

Install CRX updates one at a time.

When we raise the size limit on CRXes, it will become possible to have an extension that takes a long time and lots of CPU to unpack.  To avoid starting many resource-intensive unpack jobs at once, update one extension at a time.


BUG=80012
TEST=ExtensionUpdaterTest.TestMultipleExtensionDownloadingUpdates*

[Matt Perry, 2011-05-31 21:34:24]

http://codereview.chromium.org/6965018/diff/1/chrome/browser/extensions/crx_i...
File chrome/browser/extensions/crx_installer.cc (right):

http://codereview.chromium.org/6965018/diff/1/chrome/browser/extensions/crx_i...
chrome/browser/extensions/crx_installer.cc:127: // |frontend_weak| can be NULL
at this point in unit tests.
Can we instead pass in a mock ExtensionService in the test? Allowing NULL for
unit tests is problematic. It can become confusing when required interfaces can
be NULL.

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

http://codereview.chromium.org/6965018/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_service.h:72: Source<CrxInstaller>*
out_install_notification_source) = 0;
Why a Source? Let's just pass a raw pointer out.

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

http://codereview.chromium.org/6965018/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_updater.h:228: struct FetchedCrxFile {
I think the implementation of this can be moved to the .cc file. You can forward
declare here:
  struct FetchedCrxFile;
then in the .cc file:
  struct ExtensionUpdater::FetchedCrxFile { impl };

I prefer to do that when possible, to avoid exposing implementation details in
the header.

http://codereview.chromium.org/6965018/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_updater.h:228: struct FetchedCrxFile {
Put CRX in caps for consistency. Same with other methods here.

http://codereview.chromium.org/6965018/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_updater.h:331: bool MaybeUpdateCrxFile();
I think MaybeInstallCRXFile would mke more sense.

http://codereview.chromium.org/6965018/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_updater.h:351: 
nit: remove extra newline

http://codereview.chromium.org/6965018/diff/1/chrome/browser/extensions/test_...
File chrome/browser/extensions/test_extension_service.h (right):

http://codereview.chromium.org/6965018/diff/1/chrome/browser/extensions/test_...
chrome/browser/extensions/test_extension_service.h:27: // Install an update. 
Return true if the install can be started.
remove the comment for derived classes

[Sam Kerner (Chrome), 2011-05-31 23:16:15]

Ready for another look.

http://codereview.chromium.org/6965018/diff/1/chrome/browser/extensions/crx_i...
File chrome/browser/extensions/crx_installer.cc (right):

http://codereview.chromium.org/6965018/diff/1/chrome/browser/extensions/crx_i...
chrome/browser/extensions/crx_installer.cc:127: // |frontend_weak| can be NULL
at this point in unit tests.
On 2011/05/31 21:34:25, Matt Perry wrote:
> Can we instead pass in a mock ExtensionService in the test? Allowing NULL for
> unit tests is problematic. It can become confusing when required interfaces
can
> be NULL.

There is ExtensionServiceInterface, which allows easy mocking.  Sadly,
CrxInstaller uses many methods not in that interface, and adding them all makes
the interface a lot bigger and harder to reason about.  I started down this
road, and gave up.

I could make the mock service used in the test inherit from the real extension
service.  This brings in a bunch of behavior the existing tests don't want.

I created an extension service on the testing profile, and used it to generate
some CrxInstallers.  Any action the CrxInstallers take is on the testing
profile's extensions service, but I don't want them to take an action.

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

http://codereview.chromium.org/6965018/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_service.h:72: Source<CrxInstaller>*
out_install_notification_source) = 0;
On 2011/05/31 21:34:25, Matt Perry wrote:
> Why a Source? Let's just pass a raw pointer out.

Done.

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

http://codereview.chromium.org/6965018/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_updater.h:228: struct FetchedCrxFile {
On 2011/05/31 21:34:25, Matt Perry wrote:
> Put CRX in caps for consistency. Same with other methods here.

Done.

http://codereview.chromium.org/6965018/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_updater.h:228: struct FetchedCrxFile {
On 2011/05/31 21:34:25, Matt Perry wrote:
> I think the implementation of this can be moved to the .cc file. You can
forward
> declare here:
>   struct FetchedCrxFile;
> then in the .cc file:
>   struct ExtensionUpdater::FetchedCrxFile { impl };
> 
> I prefer to do that when possible, to avoid exposing implementation details in
> the header.

Done.

http://codereview.chromium.org/6965018/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_updater.h:331: bool MaybeUpdateCrxFile();
On 2011/05/31 21:34:25, Matt Perry wrote:
> I think MaybeInstallCRXFile would mke more sense.

Done.

http://codereview.chromium.org/6965018/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_updater.h:351: 
On 2011/05/31 21:34:25, Matt Perry wrote:
> nit: remove extra newline

Done.

http://codereview.chromium.org/6965018/diff/1/chrome/browser/extensions/test_...
File chrome/browser/extensions/test_extension_service.h (right):

http://codereview.chromium.org/6965018/diff/1/chrome/browser/extensions/test_...
chrome/browser/extensions/test_extension_service.h:27: // Install an update. 
Return true if the install can be started.
On 2011/05/31 21:34:25, Matt Perry wrote:
> remove the comment for derived classes

Done.

[Matt Perry, 2011-05-31 23:41:38]

LGTM

http://codereview.chromium.org/6965018/diff/1/chrome/browser/extensions/crx_i...
File chrome/browser/extensions/crx_installer.cc (right):

http://codereview.chromium.org/6965018/diff/1/chrome/browser/extensions/crx_i...
chrome/browser/extensions/crx_installer.cc:127: // |frontend_weak| can be NULL
at this point in unit tests.
On 2011/05/31 23:16:15, Sam Kerner (Chrome) wrote:
> On 2011/05/31 21:34:25, Matt Perry wrote:
> > Can we instead pass in a mock ExtensionService in the test? Allowing NULL
for
> > unit tests is problematic. It can become confusing when required interfaces
> can
> > be NULL.
> 
> There is ExtensionServiceInterface, which allows easy mocking.  Sadly,
> CrxInstaller uses many methods not in that interface, and adding them all
makes
> the interface a lot bigger and harder to reason about.  I started down this
> road, and gave up.
> 
> I could make the mock service used in the test inherit from the real extension
> service.  This brings in a bunch of behavior the existing tests don't want.
> 
> I created an extension service on the testing profile, and used it to generate
> some CrxInstallers.  Any action the CrxInstallers take is on the testing
> profile's extensions service, but I don't want them to take an action.

But I'm confused.. The rest of the code does not NULL-check frontend_weak. That
seems to indicate it's not actually getting used.

Also, we don't set install_directory_ or extensions_enabled_. What is the
behavior of CrxInstaller if those are unset?

[Sam Kerner (Chrome), 2011-06-01 00:15:10]

http://codereview.chromium.org/6965018/diff/1/chrome/browser/extensions/crx_i...
File chrome/browser/extensions/crx_installer.cc (right):

http://codereview.chromium.org/6965018/diff/1/chrome/browser/extensions/crx_i...
chrome/browser/extensions/crx_installer.cc:127: // |frontend_weak| can be NULL
at this point in unit tests.
On 2011/05/31 23:41:38, Matt Perry wrote:
> On 2011/05/31 23:16:15, Sam Kerner (Chrome) wrote:
> > On 2011/05/31 21:34:25, Matt Perry wrote:
> > > Can we instead pass in a mock ExtensionService in the test? Allowing NULL
> for
> > > unit tests is problematic. It can become confusing when required
interfaces
> > can
> > > be NULL.
> > 
> > There is ExtensionServiceInterface, which allows easy mocking.  Sadly,
> > CrxInstaller uses many methods not in that interface, and adding them all
> makes
> > the interface a lot bigger and harder to reason about.  I started down this
> > road, and gave up.
> > 
> > I could make the mock service used in the test inherit from the real
extension
> > service.  This brings in a bunch of behavior the existing tests don't want.
> > 
> > I created an extension service on the testing profile, and used it to
generate
> > some CrxInstallers.  Any action the CrxInstallers take is on the testing
> > profile's extensions service, but I don't want them to take an action.
> 
> But I'm confused.. The rest of the code does not NULL-check frontend_weak.
That
> seems to indicate it's not actually getting used.

Not sure what you mean:  There are checks for frontend_weak before every user
other than the constructor.  They are in the start of
CrxInstaller::ConfirmInstall() and CrxInstaller::ReportSuccessFromUIThread(). 
No check is done in the constructor because construction is done by
ExtensionService::MakeCrxInstaller(), and if you can get there the extension
service is not null.

> Also, we don't set install_directory_ or extensions_enabled_. What is the
> behavior of CrxInstaller if those are unset?

I reverted all changes to this file after the first round of reviews, so there
is no case where these two variables will be unset.  We will dereference a null
pointer while setting them if frontend_weak is null, but that can't happen in
the constructor.

[Matt Perry, 2011-06-01 00:20:20]

http://codereview.chromium.org/6965018/diff/1/chrome/browser/extensions/crx_i...
File chrome/browser/extensions/crx_installer.cc (right):

http://codereview.chromium.org/6965018/diff/1/chrome/browser/extensions/crx_i...
chrome/browser/extensions/crx_installer.cc:127: // |frontend_weak| can be NULL
at this point in unit tests.
On 2011/06/01 00:15:11, Sam Kerner (Chrome) wrote:
> On 2011/05/31 23:41:38, Matt Perry wrote:
> > On 2011/05/31 23:16:15, Sam Kerner (Chrome) wrote:
> > > On 2011/05/31 21:34:25, Matt Perry wrote:
> > > > Can we instead pass in a mock ExtensionService in the test? Allowing
NULL
> > for
> > > > unit tests is problematic. It can become confusing when required
> interfaces
> > > can
> > > > be NULL.
> > > 
> > > There is ExtensionServiceInterface, which allows easy mocking.  Sadly,
> > > CrxInstaller uses many methods not in that interface, and adding them all
> > makes
> > > the interface a lot bigger and harder to reason about.  I started down
this
> > > road, and gave up.
> > > 
> > > I could make the mock service used in the test inherit from the real
> extension
> > > service.  This brings in a bunch of behavior the existing tests don't
want.
> > > 
> > > I created an extension service on the testing profile, and used it to
> generate
> > > some CrxInstallers.  Any action the CrxInstallers take is on the testing
> > > profile's extensions service, but I don't want them to take an action.
> > 
> > But I'm confused.. The rest of the code does not NULL-check frontend_weak.
> That
> > seems to indicate it's not actually getting used.
> 
> Not sure what you mean:  There are checks for frontend_weak before every user
> other than the constructor.  They are in the start of
> CrxInstaller::ConfirmInstall() and CrxInstaller::ReportSuccessFromUIThread(). 
> No check is done in the constructor because construction is done by
> ExtensionService::MakeCrxInstaller(), and if you can get there the extension
> service is not null.

Ah, of course, sorry. That makes sense.

> > Also, we don't set install_directory_ or extensions_enabled_. What is the
> > behavior of CrxInstaller if those are unset?
> 
> I reverted all changes to this file after the first round of reviews, so there
> is no case where these two variables will be unset.  We will dereference a
null
> pointer while setting them if frontend_weak is null, but that can't happen in
> the constructor.

I didn't realize you reverted the changes. Carry on, then!