Allow extension overinstall

Allow extensions to be overinstalled with extensions of same
version. This is useful during development, for switching 
themes, and for user scripts (since user scripts have no 
development mode).

Since we can't always immediately delete the version directory
for an extension after unloading it (because some files might
be in use), this required changing the directory layout of the
extensions directory to allow multiple copies of the same
version of the same extension to be present at once. This was
done by adding a counter to the version directory name.

Also get rid of all the old "Current Version" cruft, since we
no longer use that.

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

[Aaron Boodman, 2010-04-17 00:22:11]

Recommended review order:

The meat of this change is in extension_file_util.*. Start at InstallExtension()
and then review the call site in CrxInstaller.

I also had to modify extension_file_util::GarbageCollectExtensions() to fix the
assumptions about the extensions directory layout. Its call sites are also
modified in ExtensionsService::GarbageCollectExtensions.

Other than that, everything else is just cleaning up the fallout. I was able to
remove a lot of special-case code for themes where we wanted to allow
overinstallation, and I had to update a bunch of tests.

[Erik does not do reviews, 2010-04-19 19:59:35]

Nice cleanup.  It's nice to see that nothing else was depending on the version
number in the directory name explicitly.

http://codereview.chromium.org/1521039/diff/1/2
File chrome/browser/automation/automation_provider_observers.cc (left):

http://codereview.chromium.org/1521039/diff/1/2#oldcode376
chrome/browser/automation/automation_provider_observers.cc:376:
registrar_.Add(this, NotificationType::EXTENSION_OVERINSTALL_ERROR,
are there any ui automation tests that use this?

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

http://codereview.chromium.org/1521039/diff/1/8#newcode86
chrome/browser/extensions/extension_management_browsertest.cc:86:
EXPECT_FALSE(IsExtensionAtVersion(service->extensions()->at(size_before),
We should probably check that the install directory differs to ensure that the
install actually worked as expected.

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

http://codereview.chromium.org/1521039/diff/1/9#newcode788
chrome/browser/extensions/extensions_service.cc:788:
DCHECK(extension->version()->CompareTo(*(old->version())) >= 0);
We should probably keep this as an if or maybe change it to a CHECK.  It doesn't
lead to an immediate crash below, but if a downgrade got through it could lead
to really odd behavior.

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

http://codereview.chromium.org/1521039/diff/1/11#newcode1625
chrome/browser/extensions/extensions_service_unittest.cc:1625: ASSERT_EQ(0u,
GetErrors().size());
what causes this change?

http://codereview.chromium.org/1521039/diff/1/14
File chrome/common/extensions/extension_file_util_unittest.cc (right):

http://codereview.chromium.org/1521039/diff/1/14#newcode66
chrome/common/extensions/extension_file_util_unittest.cc:66:
ASSERT_FALSE(file_util::DirectoryExists(version_2));
should test that up one level is removed instead

[Aaron Boodman, 2010-05-19 01:07:40]

PTAL

http://codereview.chromium.org/1521039/diff/1/2
File chrome/browser/automation/automation_provider_observers.cc (left):

http://codereview.chromium.org/1521039/diff/1/2#oldcode376
chrome/browser/automation/automation_provider_observers.cc:376:
registrar_.Add(this, NotificationType::EXTENSION_OVERINSTALL_ERROR,
On 2010/04/19 19:59:36, Erik Kay wrote:
> are there any ui automation tests that use this?

Doesn't look like it.

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

http://codereview.chromium.org/1521039/diff/1/8#newcode86
chrome/browser/extensions/extension_management_browsertest.cc:86:
EXPECT_FALSE(IsExtensionAtVersion(service->extensions()->at(size_before),
On 2010/04/19 19:59:36, Erik Kay wrote:
> We should probably check that the install directory differs to ensure that the
> install actually worked as expected.

Done.

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

http://codereview.chromium.org/1521039/diff/1/9#newcode788
chrome/browser/extensions/extensions_service.cc:788:
DCHECK(extension->version()->CompareTo(*(old->version())) >= 0);
On 2010/04/19 19:59:36, Erik Kay wrote:
> We should probably keep this as an if or maybe change it to a CHECK.  It
doesn't
> lead to an immediate crash below, but if a downgrade got through it could lead
> to really odd behavior.

Done.

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

http://codereview.chromium.org/1521039/diff/1/11#newcode1625
chrome/browser/extensions/extensions_service_unittest.cc:1625: ASSERT_EQ(0u,
GetErrors().size());
On 2010/04/19 19:59:36, Erik Kay wrote:
> what causes this change?

I tracked this down. The test was broken before. I've fixed it in a separate CL:
http://codereview.chromium.org/2121009/show

[rafaelw, 2010-05-19 20:15:47]

lgtm w/ nits.

http://codereview.chromium.org/1521039/diff/10001/11002
File chrome/browser/extensions/crx_installer.cc (right):

http://codereview.chromium.org/1521039/diff/10001/11002#newcode226
chrome/browser/extensions/crx_installer.cc:226: unpacked_extension_root_,
extension_->id(), extension_->VersionString(),
these should all go on separate lines (darin beat this one into me):

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

http://codereview.chromium.org/1521039/diff/10001/11002#newcode230
chrome/browser/extensions/crx_installer.cc:230: "Could not copy unpacked
extension to install location.");
Did you intentionally ditch the
i10n_util::GetStringUTF8(IDS_EXTENSION_MOVE_DIRECTORY_TO_PROFILE_FAILED) ?

if so, does the resource string need to be garbage collected?

http://codereview.chromium.org/1521039/diff/10001/11007
File chrome/browser/extensions/extension_management_browsertest.cc (right):

http://codereview.chromium.org/1521039/diff/10001/11007#newcode21
chrome/browser/extensions/extension_management_browsertest.cc:21: // as well as
asking the extension itself.
Can you add a note here explaining the difference between the manifest version
and then "version()" as reported by the background page?

http://codereview.chromium.org/1521039/diff/10001/11008
File chrome/browser/extensions/extensions_service.cc (left):

http://codereview.chromium.org/1521039/diff/10001/11008#oldcode60
chrome/browser/extensions/extensions_service.cc:60: // Helper class to collect
the IDs of every extension listed in the prefs.
strong work killing this thing.

http://codereview.chromium.org/1521039/diff/10001/11011
File chrome/common/extensions/extension_file_util.cc (right):

http://codereview.chromium.org/1521039/diff/10001/11011#newcode55
chrome/common/extensions/extension_file_util.cc:55: } else if (i ==
(kMaxAttempts - 1)) {
consider declaring |i| outside the for loop and doing this test when the loop
completes, to avoid having to do this test for each iteration?

http://codereview.chromium.org/1521039/diff/10001/11012
File chrome/common/extensions/extension_file_util.h (right):

http://codereview.chromium.org/1521039/diff/10001/11012#newcode31
chrome/common/extensions/extension_file_util.h:31: // Removes all versions from
the extension |id| from |extensions_dir|.
"Removes all versions of the extension with |id| from |extensions_dir|" ?

http://codereview.chromium.org/1521039/diff/10001/11012#newcode45
chrome/common/extensions/extension_file_util.h:45: // Clean up directories that
aren't valid extensions from the install directory.
Can you add commentary about the input params?

http://codereview.chromium.org/1521039/diff/10001/11013
File chrome/common/extensions/extension_file_util_unittest.cc (right):

http://codereview.chromium.org/1521039/diff/10001/11013#newcode60
chrome/common/extensions/extension_file_util_unittest.cc:60:
extension_file_util::GarbageCollectExtensions(all_extensions,extension_paths);
nit: space after first arg.

[Aaron Boodman, 2010-05-19 21:44:21]

fyi

http://codereview.chromium.org/1521039/diff/10001/11002
File chrome/browser/extensions/crx_installer.cc (right):

http://codereview.chromium.org/1521039/diff/10001/11002#newcode226
chrome/browser/extensions/crx_installer.cc:226: unpacked_extension_root_,
extension_->id(), extension_->VersionString(),
On 2010/05/19 20:15:47, rafaelw wrote:
> these should all go on separate lines (darin beat this one into me):
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

Done.

http://codereview.chromium.org/1521039/diff/10001/11002#newcode230
chrome/browser/extensions/crx_installer.cc:230: "Could not copy unpacked
extension to install location.");
On 2010/05/19 20:15:47, rafaelw wrote:
> Did you intentionally ditch the
> i10n_util::GetStringUTF8(IDS_EXTENSION_MOVE_DIRECTORY_TO_PROFILE_FAILED) ?
> 
> if so, does the resource string need to be garbage collected?

Done.

http://codereview.chromium.org/1521039/diff/10001/11007
File chrome/browser/extensions/extension_management_browsertest.cc (right):

http://codereview.chromium.org/1521039/diff/10001/11007#newcode21
chrome/browser/extensions/extension_management_browsertest.cc:21: // as well as
asking the extension itself.
On 2010/05/19 20:15:47, rafaelw wrote:
> Can you add a note here explaining the difference between the manifest version
> and then "version()" as reported by the background page?

Done.

http://codereview.chromium.org/1521039/diff/10001/11011
File chrome/common/extensions/extension_file_util.cc (right):

http://codereview.chromium.org/1521039/diff/10001/11011#newcode55
chrome/common/extensions/extension_file_util.cc:55: } else if (i ==
(kMaxAttempts - 1)) {
On 2010/05/19 20:15:47, rafaelw wrote:
> consider declaring |i| outside the for loop and doing this test when the loop
> completes, to avoid having to do this test for each iteration?

Good idea. Makes it easier to read too.

http://codereview.chromium.org/1521039/diff/10001/11012
File chrome/common/extensions/extension_file_util.h (right):

http://codereview.chromium.org/1521039/diff/10001/11012#newcode31
chrome/common/extensions/extension_file_util.h:31: // Removes all versions from
the extension |id| from |extensions_dir|.
On 2010/05/19 20:15:47, rafaelw wrote:
> "Removes all versions of the extension with |id| from |extensions_dir|" ?

Done.

http://codereview.chromium.org/1521039/diff/10001/11012#newcode45
chrome/common/extensions/extension_file_util.h:45: // Clean up directories that
aren't valid extensions from the install directory.
On 2010/05/19 20:15:47, rafaelw wrote:
> Can you add commentary about the input params?

Done.

http://codereview.chromium.org/1521039/diff/10001/11013
File chrome/common/extensions/extension_file_util_unittest.cc (right):

http://codereview.chromium.org/1521039/diff/10001/11013#newcode60
chrome/common/extensions/extension_file_util_unittest.cc:60:
extension_file_util::GarbageCollectExtensions(all_extensions,extension_paths);
On 2010/05/19 20:15:47, rafaelw wrote:
> nit: space after first arg.

Done.