Pull CrxInstaller out of ExtensionsService.

Pull CrxInstaller out of ExtensionsService.

CrxInstaller is a new stateful object that encapsulates a
single installation from unpack through notification.

It currently contains the UI bits, but I suspect in the next
CL (where I will finally implement the install UI) these 
will come out and CrxInstaller will become 
SilentCrxInstaller, and only used for updates and external 
installs.

Also in this change, I removed the concept of install callbacks that ExtensionUpdater was using. This was only used to delete the temp crx file as far as I can tell, and we can easily keep state about that in CrxInstaller.

With this CL, ExtensionsServiceBackend is almost completely
dead, with only a few zombie methods left like
LoadAllExtensions(). These should all become little objects
like CrxInstaller that hold a reference to ExtensionsService
over their lifetime and then kill themselves.

I'll get to that eventually.

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

[Aaron Boodman, 2009-07-29 03:10:08]

asargent: Update related bits
erikkay: Sorry to do this to you, but you know the most about ExtensionsService
other than me. Everything else :(

[asargent_no_longer_on_chrome, 2009-07-29 17:28:19]

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

http://codereview.chromium.org/160311/diff/1/2#newcode49
Line 49: crx_deleter_.Set(crx_path_);
I don't think this will work, because ScopedTempDir::Set() checks if the path
you passed is a directory. If it isn't a directory, then it tries creating a
directory at the path, which will fail, and then it refuses to remember that
directory for cleanup. Maybe we need a ScopedTempFile or perhaps a
ScopedFileDeleter? You could also just call Delete() on crx_path_ in the
CrxInstaller destructor.

http://codereview.chromium.org/160311/diff/1/3
File chrome/browser/extensions/crx_installer.h (right):

http://codereview.chromium.org/160311/diff/1/3#newcode90
Line 90: // Whether we shoud should show prompts. This is sometimes false for
testing
typo: "shoud should"

http://codereview.chromium.org/160311/diff/1/3#newcode108
Line 108: // The frontent we will report results back to.
typo: "frontent"

http://codereview.chromium.org/160311/diff/1/5
File chrome/browser/extensions/extension_updater_unittest.cc (left):

http://codereview.chromium.org/160311/diff/1/5#oldcode467
Line 467: EXPECT_FALSE(file_util::PathExists(tmpfile_path));
We should probably still have a check somewhere that the temp file gets cleaned
up (either here or in ExtensionsService maybe?).

http://codereview.chromium.org/160311/diff/1/8
File chrome/browser/extensions/extensions_service_unittest.cc (right):

http://codereview.chromium.org/160311/diff/1/8#newcode369
Line 369: }
Maybe this is the right place to expect that |path| was deleted? This should
happen regardless of should_succeed, I think.

http://codereview.chromium.org/160311/diff/1/12
File chrome/browser/extensions/sandboxed_extension_unpacker.h (right):

http://codereview.chromium.org/160311/diff/1/12#newcode1
Line 1: // Copyright (c) 2009 The Chromium Authors. All rights reserved.
Rietveld is showing me no changes in this file - a bug in Rietveld? Some weird
git-related problem?

[Erik does not do reviews, 2009-07-29 18:08:36]

I really like the direction this is heading.  Good stuff.

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

http://codereview.chromium.org/160311/diff/1/2#newcode1
Line 1: #include "chrome/browser/extensions/crx_installer.h"
missing copyright

http://codereview.chromium.org/160311/diff/1/2#newcode59
Line 59: void CrxInstaller::OnUnpackSuccess(const FilePath& temp_dir,
I'm a bit nervous about the structure of this function.  We need to require that
Release has been called when we return. We're currently depending on this to
happen through other functions that we call.  It looks good now, but it also
looks like it's easy to introduce ref-counting errors.

What if instead, we change SandboxedExtensionUnpacker to used a scoped_refptr
for client_.  Then at the end of OnUnpackSuccess or OnUnpackFailure, we'd
guarantee that Release was always called, and we wouldn't need to do the
AddRef/Release in all of these exit points.

http://codereview.chromium.org/160311/diff/1/2#newcode66
Line 66: // The unpack dir we don't have to delete explicity since it is a child
of
should there be a DCHECK to enforce the temp_dir/extension_dir relationship?

http://codereview.chromium.org/160311/diff/1/2#newcode91
Line 91: // a special preview infobar UI for them instead.
not "preview" anymore

http://codereview.chromium.org/160311/diff/1/2#newcode100
Line 100: bool CrxInstaller::ConfirmInstall() {
definitely scary to be doing this from the file thread.  it'll be nice to see it
go.

http://codereview.chromium.org/160311/diff/1/3
File chrome/browser/extensions/crx_installer.h (right):

http://codereview.chromium.org/160311/diff/1/3#newcode1
Line 1: #include <string>
missing copyright header
missing #include guard

http://codereview.chromium.org/160311/diff/1/3#newcode19
Line 19: // To ensure that we don't outlive the frontend, we hold a refptr to
him.
And here I always thought the frontend was a she. ;-)

http://codereview.chromium.org/160311/diff/1/3#newcode31
Line 31: CrxInstaller(const FilePath& crx_path, const FilePath&
install_directory,
there are a lot of variables here, so it will be easier to read with each on its
own line

http://codereview.chromium.org/160311/diff/1/3#newcode68
Line 68: ScopedTempDir crx_deleter_;
This is a bit weird given that ScopedTempDir is being assigned to a file and not
a dir.  I'm sure the unit test doesn't verify this behavior.  Maybe the class
should be renamed and unit test updated?  This does seem like useful
functionality.

http://codereview.chromium.org/160311/diff/1/3#newcode117
Line 117: };
add DISALLOW_COPY_AND_ASSIGN macro

http://codereview.chromium.org/160311/diff/1/5
File chrome/browser/extensions/extension_updater_unittest.cc (left):

http://codereview.chromium.org/160311/diff/1/5#oldcode424
Line 424: EXPECT_FALSE(file_util::PathExists(tmpfile_path));
shouldn't we still verify that the temp file gets cleaned up?

http://codereview.chromium.org/160311/diff/1/6
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/160311/diff/1/6#newcode117
Line 117: new CrxInstaller(extension_path, install_directory_,
Extension::INTERNAL,
Is it ever supposed to be the case that a caller would want to hold on to a
reference to this object?  If not, then I'd make the constructor private and
turn this into a static method.  Reading this code makes it seem like a leak,
even though under the covers it's handling its own ref-counting.

[Aaron Boodman, 2009-07-29 20:30:01]

Thanks for the feedback.

Erik, I reworked the lifetime strategy. SandboxedExtensionUnpackerClient is now
ref-counted, as you suggested. But I realized that the RunnableMethod
infrastructure already holds a reference to the callee until after the method is
dispatched, so there's no need for us to do our own AddRef/Release pair, even if
in the future there are more asynchronous steps after unpacking.

Other minor issues addressed.

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

http://codereview.chromium.org/160311/diff/1/2#newcode1
Line 1: #include "chrome/browser/extensions/crx_installer.h"
On 2009/07/29 18:08:36, Erik Kay wrote:
> missing copyright

Done.

http://codereview.chromium.org/160311/diff/1/2#newcode49
Line 49: crx_deleter_.Set(crx_path_);
On 2009/07/29 17:28:19, Antony Sargent wrote:
> I don't think this will work, because ScopedTempDir::Set() checks if the path
> you passed is a directory. If it isn't a directory, then it tries creating a
> directory at the path, which will fail, and then it refuses to remember that
> directory for cleanup. Maybe we need a ScopedTempFile or perhaps a
> ScopedFileDeleter? You could also just call Delete() on crx_path_ in the
> CrxInstaller destructor.

Good point. And you pointing this out made me realize that relying on the
destructor means that we do file IO on the UI thread.

So I moved the delete of both this and the temp dir to be explicit and on the
file thread.

http://codereview.chromium.org/160311/diff/1/2#newcode66
Line 66: // The unpack dir we don't have to delete explicity since it is a child
of
On 2009/07/29 18:08:36, Erik Kay wrote:
> should there be a DCHECK to enforce the temp_dir/extension_dir relationship?

Good idea. Done.

http://codereview.chromium.org/160311/diff/1/2#newcode91
Line 91: // a special preview infobar UI for them instead.
On 2009/07/29 18:08:36, Erik Kay wrote:
> not "preview" anymore

Done.

http://codereview.chromium.org/160311/diff/1/2#newcode100
Line 100: bool CrxInstaller::ConfirmInstall() {
On 2009/07/29 18:08:36, Erik Kay wrote:
> definitely scary to be doing this from the file thread.  it'll be nice to see
it
> go.

Yeah, it is really weird, and it prevents the linux guys from doing the the
prompt at all because Gtk is not threadsafe.

However, this is how it was before, and I'm trying hard to not change what's
actually going on in the code, just changing where it lives.

Next CL moves this all out to the UI.

http://codereview.chromium.org/160311/diff/1/3
File chrome/browser/extensions/crx_installer.h (right):

http://codereview.chromium.org/160311/diff/1/3#newcode1
Line 1: #include <string>
On 2009/07/29 18:08:36, Erik Kay wrote:
> missing copyright header
> missing #include guard

Done.

http://codereview.chromium.org/160311/diff/1/3#newcode19
Line 19: // To ensure that we don't outlive the frontend, we hold a refptr to
him.
On 2009/07/29 18:08:36, Erik Kay wrote:
> And here I always thought the frontend was a she. ;-)

Hm, you may be right. The frontend is the prettier part. Fixed.

http://codereview.chromium.org/160311/diff/1/3#newcode31
Line 31: CrxInstaller(const FilePath& crx_path, const FilePath&
install_directory,
On 2009/07/29 18:08:36, Erik Kay wrote:
> there are a lot of variables here, so it will be easier to read with each on
its
> own line

Done.

http://codereview.chromium.org/160311/diff/1/3#newcode68
Line 68: ScopedTempDir crx_deleter_;
On 2009/07/29 18:08:36, Erik Kay wrote:
> This is a bit weird given that ScopedTempDir is being assigned to a file and
not
> a dir.  I'm sure the unit test doesn't verify this behavior.  Maybe the class
> should be renamed and unit test updated?  This does seem like useful
> functionality.

It turns out that a scoped deleter was not right because that would mean the
delete happens on the ui thread.

Note: it is *really* hard to not accidentally do file IO on the UI thread. We
should have some asserts in file utils or something to warn developers about it.

http://codereview.chromium.org/160311/diff/1/3#newcode90
Line 90: // Whether we shoud should show prompts. This is sometimes false for
testing
On 2009/07/29 17:28:19, Antony Sargent wrote:
> typo: "shoud should"

Done.

http://codereview.chromium.org/160311/diff/1/3#newcode108
Line 108: // The frontent we will report results back to.
On 2009/07/29 17:28:19, Antony Sargent wrote:
> typo: "frontent"

Done.

http://codereview.chromium.org/160311/diff/1/3#newcode117
Line 117: };
On 2009/07/29 18:08:36, Erik Kay wrote:
> add DISALLOW_COPY_AND_ASSIGN macro

Done.

http://codereview.chromium.org/160311/diff/1/5
File chrome/browser/extensions/extension_updater_unittest.cc (left):

http://codereview.chromium.org/160311/diff/1/5#oldcode424
Line 424: EXPECT_FALSE(file_util::PathExists(tmpfile_path));
On 2009/07/29 18:08:36, Erik Kay wrote:
> shouldn't we still verify that the temp file gets cleaned up?

I've added a test to ExtensionsService for this.

http://codereview.chromium.org/160311/diff/1/5#oldcode467
Line 467: EXPECT_FALSE(file_util::PathExists(tmpfile_path));
On 2009/07/29 17:28:19, Antony Sargent wrote:
> We should probably still have a check somewhere that the temp file gets
cleaned
> up (either here or in ExtensionsService maybe?).

I've added a test to ExtensionsService for this.

http://codereview.chromium.org/160311/diff/1/6
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/160311/diff/1/6#newcode117
Line 117: new CrxInstaller(extension_path, install_directory_,
Extension::INTERNAL,
On 2009/07/29 18:08:36, Erik Kay wrote:
> Is it ever supposed to be the case that a caller would want to hold on to a
> reference to this object?  If not, then I'd make the constructor private and
> turn this into a static method.  Reading this code makes it seem like a leak,
> even though under the covers it's handling its own ref-counting.

I am not sure exactly how the next CL is going to be structured. The UI might
own a reference to this object, or this object might keep a client, which the UI
would implement.

If it's OK with you'd I'd like to keep it this way until the next CL.

http://codereview.chromium.org/160311/diff/1/8
File chrome/browser/extensions/extensions_service_unittest.cc (right):

http://codereview.chromium.org/160311/diff/1/8#newcode369
Line 369: }
On 2009/07/29 17:28:19, Antony Sargent wrote:
> Maybe this is the right place to expect that |path| was deleted? This should
> happen regardless of should_succeed, I think.

Yup, done.

[asargent_no_longer_on_chrome, 2009-07-29 21:33:10]

LGTM.

By the way, I like the removal of the install callbacks; that is a nice
simplification.

http://codereview.chromium.org/160311/diff/19/21
File chrome/browser/extensions/crx_installer.h (right):

http://codereview.chromium.org/160311/diff/19/21#newcode111
Line 111: // Whether we're supposed to delete the source crx file when on
destruction.
typo: "when on destruction"

[Erik does not do reviews, 2009-07-29 21:55:54]

LGTM - new lifetime strategy seems good.  I'm OK waiting for next CL for the
CrxInstaller API changes.