Split the private webstore install API into two parts....

Split the private webstore install API into two parts.

The first part, beginInstall, must be called during a user gesture. The second
part should supply a matching id and actually starts the download and install
process.

BUG=61954
TEST=Requires webstore server side changes before it can be fully tested. For 
now, you can test that visiting the webstore and running something like the 
following command in the javascript console generates an error:

chrome.webstorePrivate.beginInstall("mihcahmgecmbnbcchbopgniflfhgnkff")



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

[Erik does not do reviews, 2010-11-09 16:32:21]

http://codereview.chromium.org/4727001/diff/16001/17001
File chrome/browser/extensions/crx_installer.cc (right):

http://codereview.chromium.org/4727001/diff/16001/17001#newcode302
chrome/browser/extensions/crx_installer.cc:302: extension_->plugins().empty() &&
is_gallery_install_;
just curious, why is this extra test necessary?  don't we check this on the way
into the whitelist?

http://codereview.chromium.org/4727001/diff/16001/17003
File chrome/browser/extensions/extension_function_dispatcher.cc (right):

http://codereview.chromium.org/4727001/diff/16001/17003#newcode287
chrome/browser/extensions/extension_function_dispatcher.cc:287:
RegisterFunction<InstallFunction>();
shouldn't we remove this one?

http://codereview.chromium.org/4727001/diff/16001/17005
File chrome/browser/extensions/extension_webstore_private_api.cc (right):

http://codereview.chromium.org/4727001/diff/16001/17005#newcode122
chrome/browser/extensions/extension_webstore_private_api.cc:122:
EXTENSION_FUNCTION_VALIDATE(Extension::IdIsValid(id));
this shouldn't be EXTENSION_FUNCTION_VALIDATE since the schema doesn't validate
it.  you should set error_ and return false.

http://codereview.chromium.org/4727001/diff/16001/17005#newcode129
chrome/browser/extensions/extension_webstore_private_api.cc:129:
CrxInstaller::SetWhitelistedInstallId(id);
add comment saying where this gets cleared, maybe talk about the TODO about
adding time expiration

http://codereview.chromium.org/4727001/diff/16001/17005#newcode145
chrome/browser/extensions/extension_webstore_private_api.cc:145:
EXTENSION_FUNCTION_VALIDATE(Extension::IdIsValid(id));
same here

http://codereview.chromium.org/4727001/diff/16001/17006
File chrome/browser/extensions/extension_webstore_private_api.h (right):

http://codereview.chromium.org/4727001/diff/16001/17006#newcode29
chrome/browser/extensions/extension_webstore_private_api.h:29: // after the
chrome milestone 9 timeframe.
why can't we just remove it?  I don't think anything that's released in the
store actually uses it yet.

http://codereview.chromium.org/4727001/diff/16001/17006#newcode47
chrome/browser/extensions/extension_webstore_private_api.h:47: static void
SetTestingInstallBaseUrl(const char* testing_install_base_url);
add comment

http://codereview.chromium.org/4727001/diff/16001/17008
File chrome/test/data/extensions/api_test/extension_gallery_install/test.html
(right):

http://codereview.chromium.org/4727001/diff/16001/17008#newcode61
chrome/test/data/extensions/api_test/extension_gallery_install/test.html:61:
chrome.webstorePrivate.completeInstall(id, function() {
seems like you need a couple of tests that call completeInstall without
beginInstall or with a failed beginInstall.

[asargent_no_longer_on_chrome, 2010-11-10 00:23:48]

new version addressing comments

http://codereview.chromium.org/4727001/diff/16001/chrome/browser/extensions/c...
File chrome/browser/extensions/crx_installer.cc (right):

http://codereview.chromium.org/4727001/diff/16001/chrome/browser/extensions/c...
chrome/browser/extensions/crx_installer.cc:302: extension_->plugins().empty() &&
is_gallery_install_;
On 2010/11/09 16:32:21, Erik Kay wrote:
> just curious, why is this extra test necessary?  don't we check this on the
way
> into the whitelist?

I don't think it's strictly necessary, but since our download code already sets
this flag it seemed like a useful "defense in depth" check to throw in.

http://codereview.chromium.org/4727001/diff/16001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_function_dispatcher.cc (right):

http://codereview.chromium.org/4727001/diff/16001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_function_dispatcher.cc:287:
RegisterFunction<InstallFunction>();
On 2010/11/09 16:32:21, Erik Kay wrote:
> shouldn't we remove this one?

Done.

http://codereview.chromium.org/4727001/diff/16001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_webstore_private_api.cc (right):

http://codereview.chromium.org/4727001/diff/16001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webstore_private_api.cc:122:
EXTENSION_FUNCTION_VALIDATE(Extension::IdIsValid(id));
On 2010/11/09 16:32:21, Erik Kay wrote:
> this shouldn't be EXTENSION_FUNCTION_VALIDATE since the schema doesn't
validate
> it.  you should set error_ and return false.

Done.

http://codereview.chromium.org/4727001/diff/16001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webstore_private_api.cc:129:
CrxInstaller::SetWhitelistedInstallId(id);
On 2010/11/09 16:32:21, Erik Kay wrote:
> add comment saying where this gets cleared, maybe talk about the TODO about
> adding time expiration

Done.

http://codereview.chromium.org/4727001/diff/16001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webstore_private_api.cc:145:
EXTENSION_FUNCTION_VALIDATE(Extension::IdIsValid(id));
On 2010/11/09 16:32:21, Erik Kay wrote:
> same here

Done.

http://codereview.chromium.org/4727001/diff/16001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_webstore_private_api.h (right):

http://codereview.chromium.org/4727001/diff/16001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webstore_private_api.h:29: // after the
chrome milestone 9 timeframe.
Ok, removed

http://codereview.chromium.org/4727001/diff/16001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webstore_private_api.h:47: static void
SetTestingInstallBaseUrl(const char* testing_install_base_url);
On 2010/11/09 16:32:21, Erik Kay wrote:
> add comment

Done.

http://codereview.chromium.org/4727001/diff/16001/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/extension_gallery_install/test.html
(right):

http://codereview.chromium.org/4727001/diff/16001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/extension_gallery_install/test.html:61:
chrome.webstorePrivate.completeInstall(id, function() {
On 2010/11/09 16:32:21, Erik Kay wrote:
> seems like you need a couple of tests that call completeInstall without
> beginInstall or with a failed beginInstall.
I've simplified this test and added others to cover the cases you suggested.

http://codereview.chromium.org/4727001/diff/26001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_gallery_install_apitest.cc (right):

http://codereview.chromium.org/4727001/diff/26001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_gallery_install_apitest.cc:39: #define
MAYBE_InstallAndUninstall FLAKY_InstallAndUninstall
FYI, I'm changing this to FLAKY so I can see if it still causes any problems,
now that I've simplified some of the test code.

[Erik does not do reviews, 2010-11-10 16:47:44]

LGTM

http://codereview.chromium.org/4727001/diff/26001/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/extension_gallery_install/common.js
(right):

http://codereview.chromium.org/4727001/diff/26001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/extension_gallery_install/common.js:14:
extensions.forEach(function(extension) {
you can use one of the more recent built-in array functions for this:
callback(extensions.some(function(ext) { return extension_id == ext.id); });

http://codereview.chromium.org/4727001/diff/26001/chrome/test/data/extensions...
File
chrome/test/data/extensions/api_test/extension_gallery_install/invalid_begin.html
(right):

http://codereview.chromium.org/4727001/diff/26001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/extension_gallery_install/invalid_begin.html:8:
chrome.webstorePrivate.completeInstall(extension_id, function() {
this seems redundant since you already have the separate complete without begin
test.  if you want to test that complete doesn't trigger just because someone
called begin before, then it seems like a better test would be to try a
different valid id for begin.  I'm still not sure that's very interesting
though.

http://codereview.chromium.org/4727001/diff/26001/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/extension_gallery_install/test.html
(right):

http://codereview.chromium.org/4727001/diff/26001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/extension_gallery_install/test.html:3: //
This tests that the management beginInstall and completeInstall functions.
"that the"?

[asargent_no_longer_on_chrome, 2010-11-10 22:58:23]

[final version for the record]

http://codereview.chromium.org/4727001/diff/26001/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/extension_gallery_install/common.js
(right):

http://codereview.chromium.org/4727001/diff/26001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/extension_gallery_install/common.js:14:
extensions.forEach(function(extension) {
On 2010/11/10 16:47:45, Erik Kay wrote:
> you can use one of the more recent built-in array functions for this:
> callback(extensions.some(function(ext) { return extension_id == ext.id); });

nice - I hadn't heard of that one before. done.

http://codereview.chromium.org/4727001/diff/26001/chrome/test/data/extensions...
File
chrome/test/data/extensions/api_test/extension_gallery_install/invalid_begin.html
(right):

http://codereview.chromium.org/4727001/diff/26001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/extension_gallery_install/invalid_begin.html:8:
chrome.webstorePrivate.completeInstall(extension_id, function() {
On 2010/11/10 16:47:45, Erik Kay wrote:
> this seems redundant since you already have the separate complete without
begin
> test.  if you want to test that complete doesn't trigger just because someone
> called begin before, then it seems like a better test would be to try a
> different valid id for begin.  I'm still not sure that's very interesting
> though.
Agreed - I've removed this part.

http://codereview.chromium.org/4727001/diff/26001/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/extension_gallery_install/test.html
(right):

http://codereview.chromium.org/4727001/diff/26001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/extension_gallery_install/test.html:3: //
This tests that the management beginInstall and completeInstall functions.
On 2010/11/10 16:47:45, Erik Kay wrote:
> "that the"?
fixed