Get rid of the extension's "Current Version" file.

Get rid of the extension's "Current Version" file.

The entire manifest.json value is now stored in the prefs file. This will
allow for quick extension checks on startup.

BUG=18293
TEST=Make sure installing/upgrading/uninstalling extensions works as expected.

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

[Aaron Boodman, 2009-08-19 02:49:09]

LGTM

Only thing I think you really should do in this CL is move CompareVersion().
Other things I think we should do, but will leave up to you whether to do so in
this CL.

http://codereview.chromium.org/174036/diff/1011/14
File chrome/browser/extensions/extension_file_util.cc (left):

http://codereview.chromium.org/174036/diff/1011/14#oldcode43
Line 43: FilePath current_version =
dest_dir.AppendASCII(kCurrentVersionFileName);
Hooray.

http://codereview.chromium.org/174036/diff/1011/14
File chrome/browser/extensions/extension_file_util.cc (right):

http://codereview.chromium.org/174036/diff/1011/14#newcode20
Line 20: // TODO(mpcomplete): obsolete. remove after migration period.
Create a bug for beta so we don't forget this?

http://codereview.chromium.org/174036/diff/1011/14#newcode41
Line 41: Extension::InstallType CompareVersions(
This no longer has anything to do with the filesystem. Move to a static member
of Extension?

http://codereview.chromium.org/174036/diff/1011/14#newcode78
Line 78: *install_type = CompareVersions(current_version, extension_version);
We are now passing in current_version just so we can determine install_type,
which is immediately returned.

Can this be pulled out and into the caller? Maybe you were planning that for a
round 2?

http://codereview.chromium.org/174036/diff/1011/17
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/174036/diff/1011/17#newcode52
Line 52: scoped_ptr<InstalledExtensions::Callback> cleanup(callback);
Nit: I usually see these called <thing>_deleter.

http://codereview.chromium.org/174036/diff/1011/19
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/174036/diff/1011/19#newcode599
Line 599: extension_file_util::ValidateExtension(tmp.get(), &error)) {
Thought: we don't really need to validate installed extensions except for the
first time. If we didn't have to validate, we could do this all on the UI thread
now, right?

We only need to validate --load-extension every time.

http://codereview.chromium.org/174036/diff/1011/22
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/174036/diff/1011/22#newcode552
Line 552:
manifest_value_.reset(static_cast<DictionaryValue*>(source.DeepCopy()));
Interesting. With this here, we could start transitioning away from parsing
everything into state inside this class, and instead getting values out of the
manifest directly.

[Matt Perry, 2009-08-19 19:46:02]

http://codereview.chromium.org/174036/diff/1011/14
File chrome/browser/extensions/extension_file_util.cc (right):

http://codereview.chromium.org/174036/diff/1011/14#newcode20
Line 20: // TODO(mpcomplete): obsolete. remove after migration period.
On 2009/08/19 02:49:09, Aaron Boodman wrote:
> Create a bug for beta so we don't forget this?

Done.

http://codereview.chromium.org/174036/diff/1011/14#newcode41
Line 41: Extension::InstallType CompareVersions(
On 2009/08/19 02:49:09, Aaron Boodman wrote:
> This no longer has anything to do with the filesystem. Move to a static member
> of Extension?

See comment below.

http://codereview.chromium.org/174036/diff/1011/14#newcode78
Line 78: *install_type = CompareVersions(current_version, extension_version);
On 2009/08/19 02:49:09, Aaron Boodman wrote:
> We are now passing in current_version just so we can determine install_type,
> which is immediately returned.
> 
> Can this be pulled out and into the caller? Maybe you were planning that for a
> round 2?

OK, I rejiggered CompareVersions so that the caller calls it, which shortens
this method a lot.

http://codereview.chromium.org/174036/diff/1011/19
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/174036/diff/1011/19#newcode599
Line 599: extension_file_util::ValidateExtension(tmp.get(), &error)) {
On 2009/08/19 02:49:09, Aaron Boodman wrote:
> Thought: we don't really need to validate installed extensions except for the
> first time. If we didn't have to validate, we could do this all on the UI
thread
> now, right?
> 
> We only need to validate --load-extension every time.

This function is actually checking the filesystem to make sure all the bits of
the extension exist. I think it still makes sense to check this every time,
because someone could go in and delete some extension files.

[Aaron Boodman, 2009-08-19 20:30:34]

On 2009/08/19 19:46:02, Matt Perry wrote:
> This function is actually checking the filesystem to make sure all the bits of
> the extension exist. I think it still makes sense to check this every time,
> because someone could go in and delete some extension files.

Disagree. I think we shouldn't be worried about the case of users or software
manually deleting extension files after installation. At that point the
extension will be borked and they can manually uninstall it. NBD.

If we skip this validation step for normal startup than we can get rid of a
bunch of file hits on startup, which seems valuable, and it also simplifies the
startup code.

Again, not something I think needs to be done now, but a direction we should go.