Making ShowExtensionInstallDialog a callback

Making ShowExtensionInstallDialog a callback

BUG=152010
TBR=rdsmith@chromium.org, jrg@chromium.org

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

[Aaron Boodman, 2012-10-11 01:57:57]

https://codereview.chromium.org/11087071/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/11087071/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:332:
ExtensionInstallDialog::CreateDefaultImpl());
Can you make it so you can just pass NULL here and it will default to
CreateDefaultImpl()?

https://codereview.chromium.org/11087071/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_install_dialog.cc (right):

https://codereview.chromium.org/11087071/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_install_dialog.cc:73: AutoConfirmForTest
auto_confirm = CheckAutoConfirmCommandLineSwitch();
Is it possible to move this code into ExtensionInstallPrompt? If so, this would
just be a pure interface, not a base class, which would be nice.

[sail, 2012-10-11 02:32:16]

https://codereview.chromium.org/11087071/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/11087071/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:332:
ExtensionInstallDialog::CreateDefaultImpl());
On 2012/10/11 01:57:57, Aaron Boodman wrote:
> Can you make it so you can just pass NULL here and it will default to
> CreateDefaultImpl()?

Done.

https://codereview.chromium.org/11087071/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_install_dialog.cc (right):

https://codereview.chromium.org/11087071/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_install_dialog.cc:73: AutoConfirmForTest
auto_confirm = CheckAutoConfirmCommandLineSwitch();
On 2012/10/11 01:57:57, Aaron Boodman wrote:
> Is it possible to move this code into ExtensionInstallPrompt? If so, this
would
> just be a pure interface, not a base class, which would be nice.

Done.

[Aaron Boodman, 2012-10-11 02:42:03]

https://codereview.chromium.org/11087071/diff/5001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_install_dialog.h (right):

https://codereview.chromium.org/11087071/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/extension_install_dialog.h:14: class
ExtensionInstallDialog : public base::RefCounted<ExtensionInstallDialog> {
RefCounted is a design smell. Why does this need to be refcounted?

https://codereview.chromium.org/11087071/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/extension_install_dialog.h:16:
ExtensionInstallDialog();
Make this protected.

[sail, 2012-10-11 02:46:58]

https://codereview.chromium.org/11087071/diff/5001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_install_dialog.h (right):

https://codereview.chromium.org/11087071/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/extension_install_dialog.h:14: class
ExtensionInstallDialog : public base::RefCounted<ExtensionInstallDialog> {
On 2012/10/11 02:42:03, Aaron Boodman wrote:
> RefCounted is a design smell. Why does this need to be refcounted?

So if this isn't refcounted then it's not clear how this object would get
deleted.
If we assume it deletes itself in Show() then case will leak:
   prompt->ConfirmInstall(..., new ExtensionInstallDialog);

Prompt::ConfirmInstall(..., dialog) {
  if (skip) {
     // dialog is leaked here
  } else {
     dialog->Show();
  }
}

We could have have ExtensionInstallPrompt take owner ship then when should the
dialog be deleted?

[sail, 2012-10-11 04:48:56]

The newest version of this CL should be good to go. Let me know what you think.
Thanks

[Aaron Boodman, 2012-10-11 20:28:50]

http://codereview.chromium.org/11087071/diff/4016/chrome/browser/extensions/a...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

http://codereview.chromium.org/11087071/diff/4016/chrome/browser/extensions/a...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:331:
ExtensionInstallPrompt::ShowDialogCallback());
This is super confusing. I thought this was a function call or something, but
it's creating an empty Callback instance.

I guess this is because callbacks must be passed by value, so you can't pass
NULL?

In that case, I think it is better the way you had -- with a method somewhere to
get the default impl of this callback.

Sorry for the flip flop.

http://codereview.chromium.org/11087071/diff/4016/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_install_prompt.cc (right):

http://codereview.chromium.org/11087071/diff/4016/chrome/browser/extensions/e...
chrome/browser/extensions/extension_install_prompt.cc:129: void
AutoConfirmTask(ExtensionInstallPrompt::Delegate* delegate, bool proceed) {
This is a bit weird. Can check proceed in DoAutoConfirm, and just post-task to
either delegate->InstallUIProceed or delegate->InstallUIAbort, right?

http://codereview.chromium.org/11087071/diff/4016/chrome/browser/extensions/e...
chrome/browser/extensions/extension_install_prompt.cc:595: AutoConfirmForTest
auto_confirm = CheckAutoConfirmCommandLineSwitch();
is there any reason for CheckAutoConfirmCommandLineSwitch() to be a separate
function? It seems its result is only passed to DoAutoConfirm. Can it be rolled
into that function, and then here, do:

if (DoAutoConfirm(delegate_))
  return;

http://codereview.chromium.org/11087071/diff/4016/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_install_prompt.h (right):

http://codereview.chromium.org/11087071/diff/4016/chrome/browser/extensions/e...
chrome/browser/extensions/extension_install_prompt.h:165: ShowDialogCallback;
Add documentation

[sail, 2012-10-11 21:18:47]

Addressed all review comments. Please take another look.

http://codereview.chromium.org/11087071/diff/4016/chrome/browser/extensions/a...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

http://codereview.chromium.org/11087071/diff/4016/chrome/browser/extensions/a...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:331:
ExtensionInstallPrompt::ShowDialogCallback());
On 2012/10/11 20:28:50, Aaron Boodman wrote:
> This is super confusing. I thought this was a function call or something, but
> it's creating an empty Callback instance.
> 
> I guess this is because callbacks must be passed by value, so you can't pass
> NULL?
> 
> In that case, I think it is better the way you had -- with a method somewhere
to
> get the default impl of this callback.
> 
> Sorry for the flip flop.

Changed to ExtensionInstallPrompt::GetDefaultShowDialogCallback() instead.

http://codereview.chromium.org/11087071/diff/4016/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_install_prompt.cc (right):

http://codereview.chromium.org/11087071/diff/4016/chrome/browser/extensions/e...
chrome/browser/extensions/extension_install_prompt.cc:129: void
AutoConfirmTask(ExtensionInstallPrompt::Delegate* delegate, bool proceed) {
On 2012/10/11 20:28:50, Aaron Boodman wrote:
> This is a bit weird. Can check proceed in DoAutoConfirm, and just post-task to
> either delegate->InstallUIProceed or delegate->InstallUIAbort, right?

Done.

http://codereview.chromium.org/11087071/diff/4016/chrome/browser/extensions/e...
chrome/browser/extensions/extension_install_prompt.cc:595: AutoConfirmForTest
auto_confirm = CheckAutoConfirmCommandLineSwitch();
On 2012/10/11 20:28:50, Aaron Boodman wrote:
> is there any reason for CheckAutoConfirmCommandLineSwitch() to be a separate
> function? It seems its result is only passed to DoAutoConfirm. Can it be
rolled
> into that function, and then here, do:
> 
> if (DoAutoConfirm(delegate_))
>   return;

Done.

[Aaron Boodman, 2012-10-11 21:44:47]

https://codereview.chromium.org/11087071/diff/3003/chrome/browser/extensions/...
File chrome/browser/extensions/extension_install_prompt.cc (right):

https://codereview.chromium.org/11087071/diff/3003/chrome/browser/extensions/...
chrome/browser/extensions/extension_install_prompt.cc:136: FROM_HERE,
base::Bind(&ExtensionInstallPrompt::Delegate::InstallUIProceed,
Weird indent.

https://codereview.chromium.org/11087071/diff/3003/chrome/browser/extensions/...
chrome/browser/extensions/extension_install_prompt.cc:136: FROM_HERE,
base::Bind(&ExtensionInstallPrompt::Delegate::InstallUIProceed,
80 cols

https://codereview.chromium.org/11087071/diff/3003/chrome/browser/extensions/...
chrome/browser/extensions/extension_install_prompt.cc:142:
MessageLoop::current()->PostTask(
ditto

https://codereview.chromium.org/11087071/diff/3003/chrome/browser/extensions/...
chrome/browser/extensions/extension_install_prompt.cc:586: 
naming: if (AutoConfirmPrompt()) ?

[sail, 2012-10-11 21:49:38]

https://codereview.chromium.org/11087071/diff/3003/chrome/browser/extensions/...
File chrome/browser/extensions/extension_install_prompt.cc (right):

https://codereview.chromium.org/11087071/diff/3003/chrome/browser/extensions/...
chrome/browser/extensions/extension_install_prompt.cc:136: FROM_HERE,
base::Bind(&ExtensionInstallPrompt::Delegate::InstallUIProceed,
On 2012/10/11 21:44:47, Aaron Boodman wrote:
> Weird indent.

Done.

https://codereview.chromium.org/11087071/diff/3003/chrome/browser/extensions/...
chrome/browser/extensions/extension_install_prompt.cc:136: FROM_HERE,
base::Bind(&ExtensionInstallPrompt::Delegate::InstallUIProceed,
On 2012/10/11 21:44:47, Aaron Boodman wrote:
> 80 cols

Done.

https://codereview.chromium.org/11087071/diff/3003/chrome/browser/extensions/...
chrome/browser/extensions/extension_install_prompt.cc:142:
MessageLoop::current()->PostTask(
On 2012/10/11 21:44:47, Aaron Boodman wrote:
> ditto

Done.

[sail, 2012-10-11 21:49:55]

https://codereview.chromium.org/11087071/diff/3003/chrome/browser/extensions/...
File chrome/browser/extensions/extension_install_prompt.cc (right):

https://codereview.chromium.org/11087071/diff/3003/chrome/browser/extensions/...
chrome/browser/extensions/extension_install_prompt.cc:586: 
On 2012/10/11 21:44:47, Aaron Boodman wrote:
> naming: if (AutoConfirmPrompt()) ?

Done.

[Aaron Boodman, 2012-10-11 21:52:07]

lgtm

[sail, 2012-10-11 21:53:40]

On 2012/10/11 21:52:07, Aaron Boodman wrote:
> lgtm

woot! thanks for the review

[commit-bot: I haz the power, 2012-10-11 21:54:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/11087071/3004

[commit-bot: I haz the power, 2012-10-11 21:54:22]

Presubmit check for 11087071-3004 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui/gtk
    chrome
    chrome/browser/ui/views
    chrome/browser/ui/android
    chrome/browser/download

Presubmit checks took 2.2s to calculate.

[sail, 2012-10-11 21:58:21]

rdsmith: download/*
erg: gtk/*
sky: views/*

[sail, 2012-10-11 22:00:01]

android/*
  jrg@chromium.org
  bulach@chromium.org
  satish@chromium.org
  jcivelli@chromium.org
  yfriedman@chromium.org

[Elliot Glaysher, 2012-10-11 22:02:04]

gtk lgtm

[sky, 2012-10-11 22:07:24]

LGTM

[commit-bot: I haz the power, 2012-10-11 22:10:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/11087071/3004

[commit-bot: I haz the power, 2012-10-11 22:49:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/11087071/14001

[commit-bot: I haz the power, 2012-10-11 23:01:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/11087071/16004

[commit-bot: I haz the power, 2012-10-12 02:23:09]

Change committed as 161479

[Randy Smith (Not in Mondays), 2012-10-12 16:32:28]

LGTM :-}.

[Matt Perry, 2012-10-12 19:28:04]

This change causes a crash in some other uses of the ExtensionInstallPrompt
(e.g. when an extension is disabled due to an update requiring more
permissions).

https://chromiumcodereview.appspot.com/11087071/diff/16004/chrome/browser/ext...
File chrome/browser/extensions/extension_install_prompt.cc (right):

https://chromiumcodereview.appspot.com/11087071/diff/16004/chrome/browser/ext...
chrome/browser/extensions/extension_install_prompt.cc:405:
DCHECK(!show_dialog_callback_.is_null());
We need to do this for the 3 other ConfirmFoo() methods as well.

[sail, 2012-10-12 19:29:20]

On 2012/10/12 19:28:04, Matt Perry wrote:
> This change causes a crash in some other uses of the ExtensionInstallPrompt
> (e.g. when an extension is disabled due to an update requiring more
> permissions).
> 
>
https://chromiumcodereview.appspot.com/11087071/diff/16004/chrome/browser/ext...
> File chrome/browser/extensions/extension_install_prompt.cc (right):
> 
>
https://chromiumcodereview.appspot.com/11087071/diff/16004/chrome/browser/ext...
> chrome/browser/extensions/extension_install_prompt.cc:405:
> DCHECK(!show_dialog_callback_.is_null());
> We need to do this for the 3 other ConfirmFoo() methods as well.

I'd like to remove this DCHECK, sending out a CL now.

[Matt Perry, 2012-10-12 19:33:05]

https://chromiumcodereview.appspot.com/11087071/diff/16004/chrome/browser/ext...
File chrome/browser/extensions/extension_install_prompt.cc (right):

https://chromiumcodereview.appspot.com/11087071/diff/16004/chrome/browser/ext...
chrome/browser/extensions/extension_install_prompt.cc:593:
show_dialog_callback_.Run(parent_, navigator_, delegate_, prompt_);
We still need the callback here when we actually want to show the prompt.