[AppBanner] Blink implementation of an event when an install banner is shown.

[AppBanner] Blink implementation of an event when an install banner is shown.

If the event is cancelled, the install banner is not being shown. The
event also carries the platform type for which the installation is going
to happen.

This is not implement the userChoice promise.

Intent to implement:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/HSSqpbYd8W8/6I_InR9keNwJ

Explainer:
https://github.com/slightlyoff/AppInstallImprovements/blob/master/explainer.md

BUG=460945
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=191459

[mlamouri (slow - plz ping), 2015-02-20 18:13:19]

This is not handling .userChoice, the WebAppBannerClient (not implemented) will
take care of that.

[mlamouri (slow - plz ping), 2015-02-23 15:59:25]

mlamouri@chromium.org changed reviewers:
+ mkwst@chromium.org

[Mike West, 2015-02-23 21:31:25]

Did this go to blink-dev@ as an intent to implement yet?

It would be nice if you linked to a bug or a spec. I have some questions about
what the event actually will do, when it's supposed to be triggered, etc.

[mlamouri (slow - plz ping), 2015-02-24 12:29:10]

+cc benwells@ fyi

[Mike West, 2015-03-06 14:30:30]

Can you add a link to the intent to implement, the explainer, and the bug?

Otherwise LGTM % comments.

https://codereview.chromium.org/942913002/diff/20001/Source/modules/app_banne...
File Source/modules/app_banner/AppBannerController.h (right):

https://codereview.chromium.org/942913002/diff/20001/Source/modules/app_banne...
Source/modules/app_banner/AppBannerController.h:13: class AppBannerController
final {
Wrapping this in a class doesn't really win you much, does it? We have a number
of other static functions in Blink (mostly in //platform). It wouldn't be
terrible to add another if you'd rather go that route.

https://codereview.chromium.org/942913002/diff/20001/Source/modules/app_banne...
Source/modules/app_banner/AppBannerController.h:15: static void
willShowInstallBannerPrompt(LocalFrame*, const WebString& platform, bool*
cancel);
I have a vague suspicion that replacing that bool with an enum will be clearer
when you start using this.

https://codereview.chromium.org/942913002/diff/20001/Source/modules/app_banne...
File Source/modules/app_banner/BeforeInstallPromptEvent.idl (right):

https://codereview.chromium.org/942913002/diff/20001/Source/modules/app_banne...
Source/modules/app_banner/BeforeInstallPromptEvent.idl:10:
[CallWith=ScriptState] readonly attribute Promise<DOMString> userChoice;
Are we informing the site whether or not the user installed the app?  (Skimming
https://github.com/slightlyoff/AppInstallImprovements/blob/master/explainer.md,
I guess we are). Doesn't that invite bad behavior?

https://codereview.chromium.org/942913002/diff/20001/Source/modules/app_banne...
File Source/modules/app_banner/OWNERS (right):

https://codereview.chromium.org/942913002/diff/20001/Source/modules/app_banne...
Source/modules/app_banner/OWNERS:1: mlamouri@chromium.org
Might want some more OWNERS.

[mlamouri (slow - plz ping), 2015-03-06 18:50:47]

Could you check that you are still okay with me landing this CL?

https://codereview.chromium.org/942913002/diff/20001/Source/modules/app_banne...
File Source/modules/app_banner/AppBannerController.h (right):

https://codereview.chromium.org/942913002/diff/20001/Source/modules/app_banne...
Source/modules/app_banner/AppBannerController.h:13: class AppBannerController
final {
On 2015/03/06 at 14:30:29, Mike West wrote:
> Wrapping this in a class doesn't really win you much, does it? We have a
number of other static functions in Blink (mostly in //platform). It wouldn't be
terrible to add another if you'd rather go that route.

It's in a class because AppBannerController will be used for userChoice. It will
carry the client in order to ask the embedder what the userchoice was (I started
implementing that then cleaned up things, thus the remainings). I would prefer
to keep it as is for now instead or removing the class then add it back. Will
add a FIXME though to make sure the issue is visible in case of userChoice never
happens.

https://codereview.chromium.org/942913002/diff/20001/Source/modules/app_banne...
Source/modules/app_banner/AppBannerController.h:15: static void
willShowInstallBannerPrompt(LocalFrame*, const WebString& platform, bool*
cancel);
On 2015/03/06 at 14:30:29, Mike West wrote:
> I have a vague suspicion that replacing that bool with an enum will be clearer
when you start using this.

Done.

https://codereview.chromium.org/942913002/diff/20001/Source/modules/app_banne...
File Source/modules/app_banner/BeforeInstallPromptEvent.idl (right):

https://codereview.chromium.org/942913002/diff/20001/Source/modules/app_banne...
Source/modules/app_banner/BeforeInstallPromptEvent.idl:10:
[CallWith=ScriptState] readonly attribute Promise<DOMString> userChoice;
On 2015/03/06 at 14:30:29, Mike West wrote:
> Are we informing the site whether or not the user installed the app? 
(Skimming
https://github.com/slightlyoff/AppInstallImprovements/blob/master/explainer.md,
I guess we are). Doesn't that invite bad behavior?

For now, this is not implemented. I believe there are some discussions about
this and I'm pretty sure the implementation will stay in sync.

https://codereview.chromium.org/942913002/diff/20001/Source/modules/app_banne...
File Source/modules/app_banner/OWNERS (right):

https://codereview.chromium.org/942913002/diff/20001/Source/modules/app_banne...
Source/modules/app_banner/OWNERS:1: mlamouri@chromium.org
On 2015/03/06 at 14:30:29, Mike West wrote:
> Might want some more OWNERS.

For now, I have no idea who could be added.

[Mike West, 2015-03-06 20:17:45]

Still LGTM.

https://codereview.chromium.org/942913002/diff/60001/public/platform/modules/...
File public/platform/modules/app_banner/WebAppBannerPromptReply.h (right):

https://codereview.chromium.org/942913002/diff/60001/public/platform/modules/...
public/platform/modules/app_banner/WebAppBannerPromptReply.h:11: None = 0,
Nit: WebAppBannerPromptReplyNone and WebAppBannerPromptReplyCancel.

[mlamouri (slow - plz ping), 2015-03-06 20:42:04]

https://codereview.chromium.org/942913002/diff/60001/public/platform/modules/...
File public/platform/modules/app_banner/WebAppBannerPromptReply.h (right):

https://codereview.chromium.org/942913002/diff/60001/public/platform/modules/...
public/platform/modules/app_banner/WebAppBannerPromptReply.h:11: None = 0,
On 2015/03/06 at 20:17:45, Mike West wrote:
> Nit: WebAppBannerPromptReplyNone and WebAppBannerPromptReplyCancel.

With "enum class", AFAIK, you would have to do
WebAppBannerPromptReply::WebAppBannerPromptReplyNone (see
AppBannerController.cpp).

It seems that other WebKit "enum class" follow the same pattern:
https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&q=%...

[mlamouri (slow - plz ping), 2015-03-06 20:42:08]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2015-03-06 20:43:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/942913002/60001

[commit-bot: I haz the power, 2015-03-06 20:46:26]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=191459