Adds a webstorePrivate method for silently installing extensions.

Adds a webstorePrivate method for silently installing extensions.

BUG=98687
TEST=*WebstorePrivate*


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

[Aaron Boodman, 2011-11-01 17:24:34]

It's also possible to have completely silent UI by just passing NULL to
CrxInstaller for the client* parameter.

That might make this a little simpler? The whitelist would move to
download_extension_util or whatever it is.

http://codereview.chromium.org/8430033/diff/3001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_install_ui.h (right):

http://codereview.chromium.org/8430033/diff/3001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_install_ui.h:119: // Whether or not to show
the default UI after completing the installation.
Consider combining these two booleans flags into an enum (PostInstallAppUI {
LAUNCH, BUBBLE, NONE })

http://codereview.chromium.org/8430033/diff/3001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_webstore_private_api.cc (right):

http://codereview.chromium.org/8430033/diff/3001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_webstore_private_api.cc:81: id ==
std::string("jgoepmocgafhnchmokaimcmlojpnlkhp") ||
Nit: I don't think you need the std::string wrappers -- std::string can compare
to const char* and char[] directly.

http://codereview.chromium.org/8430033/diff/3001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_webstore_private_api.cc:431: if
(WebstoreInstallHelper::Delegate::MANIFEST_ERROR)
I think this will always be true.

http://codereview.chromium.org/8430033/diff/3001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_webstore_private_api.cc:466:
result_.reset(Value::CreateStringValue("unknown_error"));
We don't usually send both a result and an error. In the case of error, we
usually just send a null value for the result.

http://codereview.chromium.org/8430033/diff/3001/chrome/test/data/extensions/...
File chrome/test/data/extensions/api_test/webstore_private/silently_install.html
(right):

http://codereview.chromium.org/8430033/diff/3001/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/webstore_private/silently_install.html:1:
<script src="common.js"></script>
You should test that people can call silentlyInstall() multiple times in quick
succession. I bet that the store team will want to do that to improve UI (so the
download bar doesn't pop up and down repeatedly).

[jstritar, 2011-11-01 17:48:23]

On 2011/11/01 17:24:34, Aaron Boodman wrote:
>
http://codereview.chromium.org/8430033/diff/3001/chrome/test/data/extensions/...
> File
chrome/test/data/extensions/api_test/webstore_private/silently_install.html
> (right):
> 
>
http://codereview.chromium.org/8430033/diff/3001/chrome/test/data/extensions/...
> chrome/test/data/extensions/api_test/webstore_private/silently_install.html:1:
> <script src="common.js"></script>
> You should test that people can call silentlyInstall() multiple times in quick
> succession. I bet that the store team will want to do that to improve UI (so
the
> download bar doesn't pop up and down repeatedly).

Ugh, good point. We download the extensions by redirecting the tab, so this is
going to nuke the existing page's JS context and the callback won't even be
called.

They might be able trigger the downloads one after another with redirects, but
that sounds complicated. I'll see if we can use
DownloadManager::DownloadUrlToFile.

[jstritar, 2011-11-01 19:43:05]

I don't think we can move the whitelist to download_crx_util (to set client* to
NULL) because we don't know the extension ID until we're inside of CrxInstaller.

http://codereview.chromium.org/8430033/diff/3001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_install_ui.h (right):

http://codereview.chromium.org/8430033/diff/3001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_install_ui.h:119: // Whether or not to show
the default UI after completing the installation.
On 2011/11/01 17:24:34, Aaron Boodman wrote:
> Consider combining these two booleans flags into an enum (PostInstallAppUI {
> LAUNCH, BUBBLE, NONE })

The use_app_installed_bubble only applies to apps whereas the flag I added
applies to both, so I don't think it'll get us too much right now.

http://codereview.chromium.org/8430033/diff/3001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_webstore_private_api.cc (right):

http://codereview.chromium.org/8430033/diff/3001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_webstore_private_api.cc:81: id ==
std::string("jgoepmocgafhnchmokaimcmlojpnlkhp") ||
On 2011/11/01 17:24:34, Aaron Boodman wrote:
> Nit: I don't think you need the std::string wrappers -- std::string can
compare
> to const char* and char[] directly.

Done.

http://codereview.chromium.org/8430033/diff/3001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_webstore_private_api.cc:431: if
(WebstoreInstallHelper::Delegate::MANIFEST_ERROR)
On 2011/11/01 17:24:34, Aaron Boodman wrote:
> I think this will always be true.

Oops, I meant to compare that to result_code. Done.

http://codereview.chromium.org/8430033/diff/3001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_webstore_private_api.cc:466:
result_.reset(Value::CreateStringValue("unknown_error"));
On 2011/11/01 17:24:34, Aaron Boodman wrote:
> We don't usually send both a result and an error. In the case of error, we
> usually just send a null value for the result.

I did this to be consistent with BeginInstallWithManifest. I'm fine with
removing the result codes though (then I can get rid of SetResult).

http://codereview.chromium.org/8430033/diff/3001/chrome/test/data/extensions/...
File chrome/test/data/extensions/api_test/webstore_private/silently_install.html
(right):

http://codereview.chromium.org/8430033/diff/3001/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/webstore_private/silently_install.html:1:
<script src="common.js"></script>
On 2011/11/01 17:24:34, Aaron Boodman wrote:
> You should test that people can call silentlyInstall() multiple times in quick
> succession. I bet that the store team will want to do that to improve UI (so
the
> download bar doesn't pop up and down repeatedly).

Done. I needed to add a new flag for this, since the way we were running the
other tests only supported 1 CRX file per test. I also added a couple crx files
which are in a second CL (so we can get this through trybots).

[Aaron Boodman, 2011-11-02 04:16:55]

Also did you see my question about removing the crx files?

http://codereview.chromium.org/8430033/diff/3001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_webstore_private_api.cc (right):

http://codereview.chromium.org/8430033/diff/3001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_webstore_private_api.cc:466:
result_.reset(Value::CreateStringValue("unknown_error"));
On 2011/11/01 19:43:05, jstritar wrote:
> I'm fine with
> removing the result codes though (then I can get rid of SetResult).

Less code sgtm

[jstritar, 2011-11-02 18:28:05]

The test now generates the crx files (sorry for taking a shortcut earlier). I
also got rid of the result from the silentlyInstall method. The
WebstoreInstaller now calls GetDownloadFilePath on the FILE thread (was
triggering a DCHECK).

I'm stuck on another DCHECK being thrown from the downloads system. It doesn't
seem like it should be triggered... rdsmith is helping me out with that.

[jstritar, 2011-11-02 19:10:51]

I removed the DCHECK for now. They will be providing a better fix next week.

[jstritar, 2011-11-02 19:11:31]

+rdsmith for removing the DCHECK from DownloadManager

[jstritar, 2011-11-02 19:12:05]

On 2011/11/02 19:11:31, jstritar wrote:
> +rdsmith for removing the DCHECK from DownloadManager

[Randy Smith (Not in Mondays), 2011-11-02 19:28:46]

downloads_manager.cc LGTM.

[Aaron Boodman, 2011-11-02 19:34:28]

thanks, lgtm

[commit-bot: I haz the power, 2011-11-02 20:55:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jstritar@chromium.org/8430033/5053

[Mihai Parparita -not on Chrome, 2011-11-02 21:05:09]

LGTM

http://codereview.chromium.org/8430033/diff/5053/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_webstore_private_api.cc (right):

http://codereview.chromium.org/8430033/diff/5053/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_webstore_private_api.cc:83: (id ==
"begfmnajjkbjdgmffnjaojchoncnmngg" ||
Nit: Have you considered adding a comment for each ID indicating what it
corresponds to?

http://codereview.chromium.org/8430033/diff/5053/chrome/test/data/extensions/...
File chrome/test/data/extensions/api_test/webstore_private/silently_install.html
(right):

http://codereview.chromium.org/8430033/diff/5053/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/webstore_private/silently_install.html:57:
chrome.webstorePrivate.silentlyInstall(extension2, callbackPass(function() {
Perhaps one of these should be nested in the callback of the other, to make sure
that both parallel and sequential installs are supported.

[jstritar, 2011-11-02 21:20:49]

http://codereview.chromium.org/8430033/diff/5053/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_webstore_private_api.cc (right):

http://codereview.chromium.org/8430033/diff/5053/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_webstore_private_api.cc:83: (id ==
"begfmnajjkbjdgmffnjaojchoncnmngg" ||
On 2011/11/02 21:05:09, Mihai Parparita wrote:
> Nit: Have you considered adding a comment for each ID indicating what it
> corresponds to?

Done.

http://codereview.chromium.org/8430033/diff/5053/chrome/test/data/extensions/...
File chrome/test/data/extensions/api_test/webstore_private/silently_install.html
(right):

http://codereview.chromium.org/8430033/diff/5053/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/webstore_private/silently_install.html:57:
chrome.webstorePrivate.silentlyInstall(extension2, callbackPass(function() {
On 2011/11/02 21:05:09, Mihai Parparita wrote:
> Perhaps one of these should be nested in the callback of the other, to make
sure
> that both parallel and sequential installs are supported.

Done.

[commit-bot: I haz the power, 2011-11-02 21:20:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jstritar@chromium.org/8430033/6033

[commit-bot: I haz the power, 2011-11-02 23:53:49]

Change committed as 108367