Mac Web Intents Part 15: Inline extension install prompt (model)

Mac Web Intents Part 15: Inline extension install prompt (model)

This is the model specific part of the work to show extension install prompts inside the web intent picker dialog.

This CL adds a custom_install_dialog_callback field to WebstoreInstaller::Approval. If this field is set then the install prompt code will run this callback to show the install prompt.

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

[Nico, 2012-10-02 05:11:00]

Looks harmless enough, but I think a chrome/browser/extensions/OWNER should take
a look at this.

https://codereview.chromium.org/11035017/diff/1/chrome/browser/extensions/crx...
File chrome/browser/extensions/crx_installer.h (right):

https://codereview.chromium.org/11035017/diff/1/chrome/browser/extensions/crx...
chrome/browser/extensions/crx_installer.h:362: // Shows a custom extension
install dialog.
Is the plan to eventually migrate everything to always pass this?

[Randy Smith (Not in Mondays), 2012-10-02 13:33:50]

OWNERS chrome/browser/download/download_browsertest.cc LGTM (in case you need
it).

[sail, 2012-10-03 05:28:08]

+miket and +mihaip for extensions/* changes

[Greg Billock, 2012-10-03 17:20:35]

http://codereview.chromium.org/11035017/diff/5001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_install_dialog.cc (right):

http://codereview.chromium.org/11035017/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_install_dialog.cc:77:
ShowExtensionInstallDialogImpl(parent, navigator, delegate, prompt);
Can this just use the same mechanism, so there's only one code path and it
always invokes the callback?

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

http://codereview.chromium.org/11035017/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_install_prompt.cc:358:
ConfirmInstall(delegate, extension, ShowDialogCallback());
This is where the impl method would go to invoke the standard one.

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

http://codereview.chromium.org/11035017/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_install_prompt.h:216: // We *MUST*
eventually call either Proceed() or Abort() on |delegate|.
Can you comment this as well where the web intents picker gets the prompt, so
the responsibility is documented there?

http://codereview.chromium.org/11035017/diff/5001/chrome/browser/extensions/u...
File chrome/browser/extensions/unpacked_installer.cc (right):

http://codereview.chromium.org/11035017/diff/5001/chrome/browser/extensions/u...
chrome/browser/extensions/unpacked_installer.cc:65: this, extension_,
ExtensionInstallPrompt::ShowDialogCallback());
This is (also) where the impl method callback would go to show the standard
dialog. (Anywhere else?)

http://codereview.chromium.org/11035017/diff/5001/chrome/browser/ui/intents/w...
File chrome/browser/ui/intents/web_intent_picker_controller.h (right):

http://codereview.chromium.org/11035017/diff/5001/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller.h:232: // Called to show
a cusotm extension install dialog.
custom

http://codereview.chromium.org/11035017/diff/5001/chrome/browser/ui/intents/w...
File chrome/browser/ui/intents/web_intent_picker_model.h (right):

http://codereview.chromium.org/11035017/diff/5001/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_model.h:185: void
SetPendingExtensionInstallDelegate(
Should these go on the picker dialog itself? It doesn't seem like a model
member.

[sail, 2012-10-03 18:39:05]

spamming more extensions/* OWNERS

[Mihai Parparita -not on Chrome, 2012-10-03 19:57:50]

Generally, the number of places that the custom callback had to be passed
through seems like a code smell (but as of right now, I don't have any concrete
suggestions for how to make things better).

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

https://codereview.chromium.org/11035017/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/extension_install_dialog.cc:77:
ShowExtensionInstallDialogImpl(parent, navigator, delegate, prompt);
On 2012/10/03 17:20:35, Greg Billock wrote:
> Can this just use the same mechanism, so there's only one code path and it
> always invokes the callback?

+1

We already have two ways in which different UIs can be triggered (by platform,
and by prompt type). Adding a third makes things even more confusing.

[sail, 2012-10-03 20:24:28]

Addressed review comments, please take another look. Thanks.

http://codereview.chromium.org/11035017/diff/5001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_install_dialog.cc (right):

http://codereview.chromium.org/11035017/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_install_dialog.cc:77:
ShowExtensionInstallDialogImpl(parent, navigator, delegate, prompt);
On 2012/10/03 17:20:35, Greg Billock wrote:
> Can this just use the same mechanism, so there's only one code path and it
> always invokes the callback?

Done.

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

http://codereview.chromium.org/11035017/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_install_prompt.cc:358:
ConfirmInstall(delegate, extension, ShowDialogCallback());
On 2012/10/03 17:20:35, Greg Billock wrote:
> This is where the impl method would go to invoke the standard one.

Done.

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

http://codereview.chromium.org/11035017/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_install_prompt.h:216: // We *MUST*
eventually call either Proceed() or Abort() on |delegate|.
On 2012/10/03 17:20:35, Greg Billock wrote:
> Can you comment this as well where the web intents picker gets the prompt, so
> the responsibility is documented there?

Done.
Added comment to WebIntentPicker::OnShowExtensionInstallDialog().

http://codereview.chromium.org/11035017/diff/5001/chrome/browser/extensions/u...
File chrome/browser/extensions/unpacked_installer.cc (right):

http://codereview.chromium.org/11035017/diff/5001/chrome/browser/extensions/u...
chrome/browser/extensions/unpacked_installer.cc:65: this, extension_,
ExtensionInstallPrompt::ShowDialogCallback());
On 2012/10/03 17:20:35, Greg Billock wrote:
> This is (also) where the impl method callback would go to show the standard
> dialog. (Anywhere else?)

Done.
Those two were it. Both of them updated.

http://codereview.chromium.org/11035017/diff/5001/chrome/browser/ui/intents/w...
File chrome/browser/ui/intents/web_intent_picker_controller.h (right):

http://codereview.chromium.org/11035017/diff/5001/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller.h:232: // Called to show
a cusotm extension install dialog.
On 2012/10/03 17:20:35, Greg Billock wrote:
> custom

Done.

http://codereview.chromium.org/11035017/diff/5001/chrome/browser/ui/intents/w...
File chrome/browser/ui/intents/web_intent_picker_model.h (right):

http://codereview.chromium.org/11035017/diff/5001/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_model.h:185: void
SetPendingExtensionInstallDelegate(
On 2012/10/03 17:20:35, Greg Billock wrote:
> Should these go on the picker dialog itself? It doesn't seem like a model
> member.

Caching these values is really convenient for the picker UI. This way the picker
can see that the model has changed, run an animation, then pick up the new
values.

In the mean time it doesn't have to keep track of this state itself.

[Greg Billock, 2012-10-03 21:15:38]

lgtm intents

http://codereview.chromium.org/11035017/diff/5001/chrome/browser/ui/intents/w...
File chrome/browser/ui/intents/web_intent_picker_model.h (right):

http://codereview.chromium.org/11035017/diff/5001/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_model.h:185: void
SetPendingExtensionInstallDelegate(
OK. Like I was saying tearing out the intent data part may end up leaving a
picker dialog model with more dialog-related state, so this would be part of
that, right?

On 2012/10/03 20:24:29, sail wrote:
> On 2012/10/03 17:20:35, Greg Billock wrote:
> > Should these go on the picker dialog itself? It doesn't seem like a model
> > member.
> 
> Caching these values is really convenient for the picker UI. This way the
picker
> can see that the model has changed, run an animation, then pick up the new
> values.
> 
> In the mean time it doesn't have to keep track of this state itself.

http://codereview.chromium.org/11035017/diff/17001/chrome/browser/extensions/...
File chrome/browser/extensions/unpacked_installer.cc (right):

http://codereview.chromium.org/11035017/diff/17001/chrome/browser/extensions/...
chrome/browser/extensions/unpacked_installer.cc:66: this, extension_,
base::Bind(ShowExtensionInstallDialogImpl));
I think needs & here and the other place, right?

[Aaron Boodman, 2012-10-03 21:17:40]

Yeah, ExtensionInstallDialog is getting gnarly.

What does this dialog actually look like? Does it (need to) include all the
functionality in ExtensionInstallDialog? If not, it seems like one natural
solution could be to abstract ExtensionInstallDialog into an interface.

[sail, 2012-10-03 21:21:43]

On 2012/10/03 21:17:40, Aaron Boodman wrote:
> Yeah, ExtensionInstallDialog is getting gnarly.
> 
> What does this dialog actually look like? Does it (need to) include all the
> functionality in ExtensionInstallDialog? If not, it seems like one natural
> solution could be to abstract ExtensionInstallDialog into an interface.

It looks exactly the same. The callback allows us to inline the UI when it's
invoked from our specialized UI. For example, in the web intents picker user can
choose to install a suggested service. When installing the service we want to
embed the install prompt inside the web intents picker.

We do this by piping our custom callback when making the install request. When
the callback fires we show the UI inside the picker dialog.

[sail, 2012-10-03 21:24:07]

On 2012/10/03 21:21:43, sail wrote:
> On 2012/10/03 21:17:40, Aaron Boodman wrote:
> > Yeah, ExtensionInstallDialog is getting gnarly.
> > 
> > What does this dialog actually look like? Does it (need to) include all the
> > functionality in ExtensionInstallDialog? If not, it seems like one natural
> > solution could be to abstract ExtensionInstallDialog into an interface.
> 
> It looks exactly the same. The callback allows us to inline the UI when it's
> invoked from our specialized UI. For example, in the web intents picker user
can
> choose to install a suggested service. When installing the service we want to
> embed the install prompt inside the web intents picker.
> 
> We do this by piping our custom callback when making the install request. When
> the callback fires we show the UI inside the picker dialog.

If my explanation wasn't clear then here's a screenshot of what it looks like
when we don't inline:
http://i.imgur.com/MuM0W.png
We get two dialogs instead of one.

[Aaron Boodman, 2012-10-03 21:28:02]

I see. OK, let me think about this a bit more.

[Aaron Boodman, 2012-10-04 06:33:35]

http://codereview.chromium.org/11035017/diff/17001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_install_dialog.cc (right):

http://codereview.chromium.org/11035017/diff/17001/chrome/browser/extensions/...
chrome/browser/extensions/extension_install_dialog.cc:74:
show_dialog_callback.Run(parent, navigator, delegate, prompt);
Too much inversion-of-control. I can't remember which way is up anymore.

We have ExtensionInstallDialog which is badly named and not actually a dialog,
but is the business logic of getting ready to display the dialog.

Then we have ExtensionInstallPrompt which is the actual impl of showing the
prompt. Currently EIP varies at compile-time by platform. This change
essentially wants to make it vary at runtime too, but instead of changing it to
be a real runtime interface, you are passing an interface through it, so we have
this weird nested ioc thing going on.

One really simple thing that Mihai suggested in person today would be to pass a
NativeWidget or NativeWindow or something as a param to
ShowExtensionInstallDialog. If non-NULL, this is the native view that the UI
should be appended to. Otherwise, we create a dialog and put it in there.

However, if it turns out you need to do more custom stuff, then you might
actually need to vary the impl of the prompt at runtime. In that case, I think
the right answer is to change extension_install_dialog.* into an actual
interface, and then have implementations for the current different behaviors,
then pass instances of this class through the various layers of the cake.

Sorry this isn't easier, but this code definitely needs to be cleaned up, and
you're lucky modifier #436.

[Aaron Boodman, 2012-10-04 06:36:51]

On 2012/10/04 06:33:35, Aaron Boodman wrote:
> However, if it turns out you need to do more custom stuff, then you might
> actually need to vary the impl of the prompt at runtime. In that case, I think
> the right answer is to change extension_install_dialog.* into an actual
> interface, and then have implementations for the current different behaviors,
> then pass instances of this class through the various layers of the cake.

Note, when I say 'interface', it could be a callback like you're using now if
that works. Just something that's polymorphic at runtime.

[miket_OOO, 2012-10-04 17:07:47]

-miket as reviewer (mihaip >= miket)

[sail, 2012-10-11 04:47:06]

I've moved out the extensions changes into a separate CL:
http://codereview.chromium.org/11087071/

[commit-bot: I haz the power, 2012-10-12 02:28:26]

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

[commit-bot: I haz the power, 2012-10-12 05:47:08]

Change committed as 161507