Change the web store private install API to accept a localized extension name.

Change the web store private install API to accept a localized extension name.

In our initial design of the beginInstallWithManifest function, we forgot that
some extensions have a name which needs token substitution for i18n/l10n. This
change renames beginInstallWithManifest to beginInstallWithManifest2, and 
modifies the signature to take optional localizedName and locale parameters.

I also refactored how we compare the passed in manifest used in the 
pre-download confirmation dialog - we now keep a copy of the source from the
.crx file to compare against, since i18n substututions can happen on the 
manifest we use to construct the Extension object. 

BUG=75821
TEST=Covered by browser tests for now; will require web store server-side 
changes before it can be manually tested.

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

[Matt Perry, 2011-05-25 01:46:05]

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

http://codereview.chromium.org/6992047/diff/1/chrome/browser/extensions/crx_i...
chrome/browser/extensions/crx_installer.cc:355: #if 0
Remove dead code, unless you mean to use it. In the latter case, my personal
preference is the original implementation.

http://codereview.chromium.org/6992047/diff/1/chrome/browser/extensions/crx_i...
chrome/browser/extensions/crx_installer.cc:431: if
(!(original_manifest_->Equals(entry->parsed_manifest.get()))) {
isn't the Extension::manifest_value supposed to be the original_manifest as
contained in the .crx? I don't see why we need to keep this separate copy.

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

http://codereview.chromium.org/6992047/diff/1/chrome/browser/extensions/crx_i...
chrome/browser/extensions/crx_installer.h:68: static void
SetWhitelistEntry(const std::string& id,
nit: can fit on 1 line looks like

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

http://codereview.chromium.org/6992047/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_webstore_private_api.cc:305: // Read the id.
these comments can go - the code is self explanatory.

http://codereview.chromium.org/6992047/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_webstore_private_api.cc:416:
parsed_manifest_->SetString(extension_manifest_keys::kName, original_name);
This restoring feels a little dirty. How about instead we copy the
parsed_manifest to localized_manifest, do the replacement there, and then throw
localized_manifest away.

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

http://codereview.chromium.org/6992047/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_webstore_private_api.h:118:
DECLARE_EXTENSION_FUNCTION_NAME("webstorePrivate.beginInstallWithManifest2");
Why the rename?

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

http://codereview.chromium.org/6992047/diff/1/chrome/browser/extensions/sandb...
chrome/browser/extensions/sandboxed_extension_unpacker.h:61: // Additionally, we
hold a reference to our own client so that it lives at least
oops

[asargent_no_longer_on_chrome, 2011-05-25 04:42:00]

New version up addressing comments.

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

http://codereview.chromium.org/6992047/diff/1/chrome/browser/extensions/crx_i...
chrome/browser/extensions/crx_installer.cc:355: #if 0
On 2011/05/25 01:46:06, Matt Perry wrote:
> Remove dead code, unless you mean to use it. In the latter case, my personal
> preference is the original implementation.

Oops, I had meant to remove this whole block. Done.

http://codereview.chromium.org/6992047/diff/1/chrome/browser/extensions/crx_i...
chrome/browser/extensions/crx_installer.cc:431: if
(!(original_manifest_->Equals(entry->parsed_manifest.get()))) {
On 2011/05/25 01:46:06, Matt Perry wrote:
> isn't the Extension::manifest_value supposed to be the original_manifest as
> contained in the .crx? I don't see why we need to keep this separate copy.
The Extension::manifest_value has already had a couple of transformations
applied to it: localization and insertion of the public key field. (See
SandboxedExtensionUnpacker::OnUnpackExtensionSucceeded).

What I was doing before, just comparing the extension's manifest_value but
ignoring the public key field, worked for non-localized extensions but would
cause comparison failures with any localized fields like name, description, etc.

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

http://codereview.chromium.org/6992047/diff/1/chrome/browser/extensions/crx_i...
chrome/browser/extensions/crx_installer.h:68: static void
SetWhitelistEntry(const std::string& id,
On 2011/05/25 01:46:06, Matt Perry wrote:
> nit: can fit on 1 line looks like

Done.

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

http://codereview.chromium.org/6992047/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_webstore_private_api.cc:305: // Read the id.
On 2011/05/25 01:46:06, Matt Perry wrote:
> these comments can go - the code is self explanatory.

Done.

http://codereview.chromium.org/6992047/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_webstore_private_api.cc:416:
parsed_manifest_->SetString(extension_manifest_keys::kName, original_name);
On 2011/05/25 01:46:06, Matt Perry wrote:
> This restoring feels a little dirty. How about instead we copy the
> parsed_manifest to localized_manifest, do the replacement there, and then
throw
> localized_manifest away.

Done.

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

http://codereview.chromium.org/6992047/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_webstore_private_api.h:118:
DECLARE_EXTENSION_FUNCTION_NAME("webstorePrivate.beginInstallWithManifest2");
On 2011/05/25 01:46:06, Matt Perry wrote:
> Why the rename?
The webstore is still just using the old beginInstall() method that just takes
an id, and no production code is using the newer but i18n-broken
beginInstallWithManifest method. Since I changed the method signature in a
breaking way, to take a details object instead of a list of parameters, I wanted
a way they could reliably detect if the newest version of the function was
available without having to know that my change hit chrome in a specific version
and check the user-agent.

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

http://codereview.chromium.org/6992047/diff/1/chrome/browser/extensions/sandb...
chrome/browser/extensions/sandboxed_extension_unpacker.h:61: // Additionally, we
hold a reference to our own client so that it lives at least
On 2011/05/25 01:46:06, Matt Perry wrote:
> oops

Done.

[Matt Perry, 2011-05-25 19:03:24]

LGTM

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

http://codereview.chromium.org/6992047/diff/1/chrome/browser/extensions/crx_i...
chrome/browser/extensions/crx_installer.cc:431: if
(!(original_manifest_->Equals(entry->parsed_manifest.get()))) {
On 2011/05/25 04:42:00, Antony Sargent wrote:
> On 2011/05/25 01:46:06, Matt Perry wrote:
> > isn't the Extension::manifest_value supposed to be the original_manifest as
> > contained in the .crx? I don't see why we need to keep this separate copy.
> The Extension::manifest_value has already had a couple of transformations
> applied to it: localization and insertion of the public key field. (See
> SandboxedExtensionUnpacker::OnUnpackExtensionSucceeded).
> 
> What I was doing before, just comparing the extension's manifest_value but
> ignoring the public key field, worked for non-localized extensions but would
> cause comparison failures with any localized fields like name, description,
etc.

Should we also check to ensure that the localized names are the same?

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

http://codereview.chromium.org/6992047/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_webstore_private_api.h:118:
DECLARE_EXTENSION_FUNCTION_NAME("webstorePrivate.beginInstallWithManifest2");
On 2011/05/25 04:42:00, Antony Sargent wrote:
> On 2011/05/25 01:46:06, Matt Perry wrote:
> > Why the rename?
> The webstore is still just using the old beginInstall() method that just takes
> an id, and no production code is using the newer but i18n-broken
> beginInstallWithManifest method. Since I changed the method signature in a
> breaking way, to take a details object instead of a list of parameters, I
wanted
> a way they could reliably detect if the newest version of the function was
> available without having to know that my change hit chrome in a specific
version
> and check the user-agent. 

Makes sense. Thanks.