Do not show the install prompt for themes.

Do not show the install prompt for themes.

We do not show the install prompt for themes because you can revert from the post-install infobar. This patch updates web store installs to use ExtensionInstallUI.

BUG=122351
TEST=inline, bundle and standard web store installs should still work


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

[jstritar, 2012-04-06 18:58:42]

actually not quite ready

[jstritar, 2012-04-06 20:08:11]

Okay, ready to look at now.

[Aaron Boodman, 2012-04-07 01:05:44]

Is the issue that we aren't using ExtensionInstallUI for installation from the
store?

[jstritar, 2012-04-09 14:24:02]

On 2012/04/07 01:05:44, Aaron Boodman wrote:
> Is the issue that we aren't using ExtensionInstallUI for installation from the
> store?

Yeah, it is, but I figured we were doing that on purpose. I'll see if I can
update this to use ExtensionInstallUI more directly.

[jstritar, 2012-04-16 20:53:37]

Can you take another look?

http://codereview.chromium.org/10010038/diff/9001/chrome/common/chrome_notifi...
File chrome/common/chrome_notification_types.h (left):

http://codereview.chromium.org/10010038/diff/9001/chrome/common/chrome_notifi...
chrome/common/chrome_notification_types.h:153:
NOTIFICATION_EXTENSION_WILL_SHOW_CONFIRM_DIALOG,
This wasn't being used by anyone... I can leave it in if you'd like though.

[jstritar, 2012-04-19 12:21:05]

hey Aaron, can you take a look when you get a chance?

[jstritar, 2012-04-20 13:56:27]

Hi Yoyo, can you take a look?

[Yoyo Zhou, 2012-04-20 15:38:14]

http://codereview.chromium.org/10010038/diff/10014/chrome/browser/extensions/...
File chrome/browser/extensions/extension_install_ui.cc (left):

http://codereview.chromium.org/10010038/diff/10014/chrome/browser/extensions/...
chrome/browser/extensions/extension_install_ui.cc:354:
service->Notify(chrome::NOTIFICATION_EXTENSION_WILL_SHOW_CONFIRM_DIALOG,
What happened to this? Nobody cared to observe it?

http://codereview.chromium.org/10010038/diff/10014/chrome/browser/extensions/...
File chrome/browser/extensions/extension_install_ui.cc (right):

http://codereview.chromium.org/10010038/diff/10014/chrome/browser/extensions/...
chrome/browser/extensions/extension_install_ui.cc:250:
prompt_(UNSET_PROMPT_TYPE),
Does this really work with the constructor explicit?

http://codereview.chromium.org/10010038/diff/10014/chrome/browser/extensions/...
chrome/browser/extensions/extension_install_ui.cc:275: delegate_ =
static_cast<Delegate*>(bundle);
Is the static_cast needed?

http://codereview.chromium.org/10010038/diff/10014/chrome/browser/extensions/...
File chrome/browser/extensions/extension_install_ui.h (right):

http://codereview.chromium.org/10010038/diff/10014/chrome/browser/extensions/...
chrome/browser/extensions/extension_install_ui.h:140: static
scoped_refptr<Extension> LocalizeExtensionForDisplay(
I think GetLocalizedExtensionForDisplay would be clearer.

http://codereview.chromium.org/10010038/diff/10014/chrome/browser/extensions/...
chrome/browser/extensions/extension_install_ui.h:282: Prompt prompt_;
It seems like if this is sometimes present and sometimes not, it'd be better to
used a pointer type, so you don't need to initialize it in the case that it's
not given.

http://codereview.chromium.org/10010038/diff/10014/chrome/browser/extensions/...
File chrome/browser/extensions/extension_webstore_private_apitest.cc (right):

http://codereview.chromium.org/10010038/diff/10014/chrome/browser/extensions/...
chrome/browser/extensions/extension_webstore_private_apitest.cc:145: //
ExtensionWebstorePrivateApiTest (you can't easily remove switches).
Could you make the inheritance go the other way so only the derived class has to
add the switch?

[jstritar, 2012-04-20 16:18:27]

http://codereview.chromium.org/10010038/diff/10014/chrome/browser/extensions/...
File chrome/browser/extensions/extension_install_ui.cc (left):

http://codereview.chromium.org/10010038/diff/10014/chrome/browser/extensions/...
chrome/browser/extensions/extension_install_ui.cc:354:
service->Notify(chrome::NOTIFICATION_EXTENSION_WILL_SHOW_CONFIRM_DIALOG,
On 2012/04/20 15:38:14, Yoyo Zhou wrote:
> What happened to this? Nobody cared to observe it?

Yeah, no observers.

http://codereview.chromium.org/10010038/diff/10014/chrome/browser/extensions/...
File chrome/browser/extensions/extension_install_ui.cc (right):

http://codereview.chromium.org/10010038/diff/10014/chrome/browser/extensions/...
chrome/browser/extensions/extension_install_ui.cc:250:
prompt_(UNSET_PROMPT_TYPE),
On 2012/04/20 15:38:14, Yoyo Zhou wrote:
> Does this really work with the constructor explicit?

Seems to... I'll make sure none of the try bots complain about it.

http://codereview.chromium.org/10010038/diff/10014/chrome/browser/extensions/...
chrome/browser/extensions/extension_install_ui.cc:275: delegate_ =
static_cast<Delegate*>(bundle);
On 2012/04/20 15:38:14, Yoyo Zhou wrote:
> Is the static_cast needed?

Removed.

http://codereview.chromium.org/10010038/diff/10014/chrome/browser/extensions/...
File chrome/browser/extensions/extension_install_ui.h (right):

http://codereview.chromium.org/10010038/diff/10014/chrome/browser/extensions/...
chrome/browser/extensions/extension_install_ui.h:140: static
scoped_refptr<Extension> LocalizeExtensionForDisplay(
On 2012/04/20 15:38:14, Yoyo Zhou wrote:
> I think GetLocalizedExtensionForDisplay would be clearer.

Done.

http://codereview.chromium.org/10010038/diff/10014/chrome/browser/extensions/...
chrome/browser/extensions/extension_install_ui.h:282: Prompt prompt_;
On 2012/04/20 15:38:14, Yoyo Zhou wrote:
> It seems like if this is sometimes present and sometimes not, it'd be better
to
> used a pointer type, so you don't need to initialize it in the case that it's
> not given.

I'd rather avoid managing the Prompt's life-cycle. I added a set_type method to
the Prompt, so we don't need to recreate it. How does that look?

http://codereview.chromium.org/10010038/diff/10014/chrome/browser/extensions/...
File chrome/browser/extensions/extension_webstore_private_apitest.cc (right):

http://codereview.chromium.org/10010038/diff/10014/chrome/browser/extensions/...
chrome/browser/extensions/extension_webstore_private_apitest.cc:145: //
ExtensionWebstorePrivateApiTest (you can't easily remove switches).
On 2012/04/20 15:38:14, Yoyo Zhou wrote:
> Could you make the inheritance go the other way so only the derived class has
to
> add the switch?

Done.

[Yoyo Zhou, 2012-04-20 17:19:12]

LGTM. A little more cleanup is possible.

http://codereview.chromium.org/10010038/diff/10014/chrome/browser/extensions/...
File chrome/browser/extensions/extension_install_ui.h (right):

http://codereview.chromium.org/10010038/diff/10014/chrome/browser/extensions/...
chrome/browser/extensions/extension_install_ui.h:282: Prompt prompt_;
On 2012/04/20 16:18:27, jstritar wrote:
> On 2012/04/20 15:38:14, Yoyo Zhou wrote:
> > It seems like if this is sometimes present and sometimes not, it'd be better
> to
> > used a pointer type, so you don't need to initialize it in the case that
it's
> > not given.
> 
> I'd rather avoid managing the Prompt's life-cycle. I added a set_type method
to
> the Prompt, so we don't need to recreate it. How does that look?

Seems fine to me -- I mean, we were copying Prompts before, so it's (presumably)
not a big deal. Not having to check for the UNSET_PROMPT_TYPE seems to be an
improvement, though.

http://codereview.chromium.org/10010038/diff/19001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_install_ui.cc (right):

http://codereview.chromium.org/10010038/diff/19001/chrome/browser/extensions/...
chrome/browser/extensions/extension_install_ui.cc:251:
prompt_type_(NUM_PROMPT_TYPES),
nit: Presumably this could be UNSET_PROMPT_TYPE too?

http://codereview.chromium.org/10010038/diff/19001/chrome/browser/extensions/...
chrome/browser/extensions/extension_install_ui.cc:493: void
ExtensionInstallUI::ShowConfirmation(PromptType prompt_type) {
prompt_type doesn't need to be passed in here -- you could set prompt_type_
before calling this, as the other members are set. Then you could skip calling
this function (which would become LoadImageIfNeeded or some such) for bundle and
inline installs and go directly to ShowDialog (which could also be called
ShowConfirmation, same as before, but it doesn't really matter).

[jstritar, 2012-04-20 18:03:29]

http://codereview.chromium.org/10010038/diff/19001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_install_ui.cc (right):

http://codereview.chromium.org/10010038/diff/19001/chrome/browser/extensions/...
chrome/browser/extensions/extension_install_ui.cc:251:
prompt_type_(NUM_PROMPT_TYPES),
On 2012/04/20 17:19:12, Yoyo Zhou wrote:
> nit: Presumably this could be UNSET_PROMPT_TYPE too?

Done.

http://codereview.chromium.org/10010038/diff/19001/chrome/browser/extensions/...
chrome/browser/extensions/extension_install_ui.cc:493: void
ExtensionInstallUI::ShowConfirmation(PromptType prompt_type) {
On 2012/04/20 17:19:12, Yoyo Zhou wrote:
> prompt_type doesn't need to be passed in here -- you could set prompt_type_
> before calling this, as the other members are set. Then you could skip calling
> this function (which would become LoadImageIfNeeded or some such) for bundle
and
> inline installs and go directly to ShowDialog (which could also be called
> ShowConfirmation, same as before, but it doesn't really matter).

Done.

[Yoyo Zhou, 2012-04-20 18:06:11]

LGTM

http://codereview.chromium.org/10010038/diff/30002/chrome/browser/extensions/...
File chrome/browser/extensions/extension_install_ui.cc (right):

http://codereview.chromium.org/10010038/diff/30002/chrome/browser/extensions/...
chrome/browser/extensions/extension_install_ui.cc:500: if (!icon_.empty() ||
prompt_type_ == BUNDLE_INSTALL_PROMPT) {
...and you don't need to check for bundle here

[jstritar, 2012-04-20 18:08:39]

https://chromiumcodereview.appspot.com/10010038/diff/30002/chrome/browser/ext...
File chrome/browser/extensions/extension_install_ui.cc (right):

https://chromiumcodereview.appspot.com/10010038/diff/30002/chrome/browser/ext...
chrome/browser/extensions/extension_install_ui.cc:500: if (!icon_.empty() ||
prompt_type_ == BUNDLE_INSTALL_PROMPT) {
oh yeah, thanks

[commit-bot: I haz the power, 2012-04-20 18:09:13]

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

[commit-bot: I haz the power, 2012-04-20 22:56:38]

Change committed as 133288