Implement "appinstalled" event on Android.

Implement "appinstalled" event on Android.

This event is fired when a web app is "installed" (i.e., a shortcut is
added to home screen).

This implements from the working draft of Web App Manifest:
https://www.w3.org/TR/appmanifest/#onappinstalled-attribute

BUG=621393
Committed: https://crrev.com/b02c2580cd1427502c41072ebe834993b1d139d0
Cr-Commit-Position: refs/heads/master@{#439416}

[Matt Giuca, 2016-10-05 07:10:17]

Patchset #5 (id:80001) has been deleted

[Matt Giuca, 2016-10-05 07:25:14]

Patchset #4 (id:60001) has been deleted

[Matt Giuca, 2016-10-06 06:00:55]

Patchset #6 (id:140001) has been deleted

[Matt Giuca, 2016-10-10 03:18:07]

Description was changed from

==========
Implement "install" event when an app is installed on Android.

This implements from the working draft of Web App Manifest:
https://www.w3.org/TR/appmanifest/#oninstall-attribute

BUG=621393
==========

to

==========
Implement "appinstalled" event on Android.

This implements from the working draft of Web App Manifest:
https://www.w3.org/TR/appmanifest/#onappinstalled-attribute

BUG=621393
==========

[Matt Giuca, 2016-10-11 05:39:36]

Description was changed from

==========
Implement "appinstalled" event on Android.

This implements from the working draft of Web App Manifest:
https://www.w3.org/TR/appmanifest/#onappinstalled-attribute

BUG=621393
==========

to

==========
Implement "appinstalled" event on Android.

This implements from the working draft of Web App Manifest:
https://www.w3.org/TR/appmanifest/#onappinstalled-attribute

BUG=621393
==========

[Matt Giuca, 2016-10-12 06:37:28]

Description was changed from

==========
Implement "appinstalled" event on Android.

This implements from the working draft of Web App Manifest:
https://www.w3.org/TR/appmanifest/#onappinstalled-attribute

BUG=621393
==========

to

==========
Implement "appinstalled" event on Android.

This event is fired when a web app is "installed" (i.e., a shortcut is
added to home screen).

This implements from the working draft of Web App Manifest:
https://www.w3.org/TR/appmanifest/#onappinstalled-attribute

BUG=621393
==========

[Matt Giuca, 2016-10-12 07:06:46]

mgiuca@chromium.org changed reviewers:
+ dominickn@chromium.org

[Matt Giuca, 2016-10-12 07:06:47]

dominickn for initial review. (OWNERS of chrome/android/*, chrome/browser/*)

Will follow up with reviewers:
esprehn: third_party/WebKit/*
tsepez:
third_party/WebKit/public/platform/modules/installation/installation.mojom
(SECURITY_OWNERS)
rkaplow: tools/metrics/histograms/histograms.xml,
third_party/WebKit/Source/core/frame/UseCounter.h (metrics OWNERS)
jochen: chrome/renderer/*, chrome/browser/DEPS

But for now leaving them out in case there is something majorly wrong with this
CL.

[dominickn, 2016-10-12 22:35:09]

First pass.

https://codereview.chromium.org/2379923002/diff/400001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenManagerTest.java
(right):

https://codereview.chromium.org/2379923002/diff/400001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenManagerTest.java:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
Can you also add a test to AppBannerManagerTest?

https://codereview.chromium.org/2379923002/diff/400001/chrome/browser/android...
File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc
(right):

https://codereview.chromium.org/2379923002/diff/400001/chrome/browser/android...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:274: void
AppBannerInfoBarDelegateAndroid::SendBannerAccepted(
What about AppBannerInfoBarDelegateDesktop? Shouldn't that also call OnInstall?

https://codereview.chromium.org/2379923002/diff/400001/chrome/browser/android...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:283:
weak_manager_->OnInstall();
We're already sending a message to the renderer here. It would be nice if we
could combine them. My patch makes the first message go straight to the
BeforeInstallPromptEvent; perhaps that can then ask the Frame to call OnInstall?

Also, my CL moves the IPC call from here to AppBannerManager so you probably
won't need to have this call in here at all - it'll be in AppBannerManager.

https://codereview.chromium.org/2379923002/diff/400001/chrome/browser/android...
File chrome/browser/android/webapps/add_to_homescreen_manager.cc (right):

https://codereview.chromium.org/2379923002/diff/400001/chrome/browser/android...
chrome/browser/android/webapps/add_to_homescreen_manager.cc:121: 
This doesn't account for WebAPKs, which call ShortcutHelper::InstallWebApk
through AppBannerInfoBarDelegateAndroid. Of course, they also call
AppBannerManager::SendBannerAccepted if they're initiated from a banner, so you
have to be a bit careful about it.

https://codereview.chromium.org/2379923002/diff/400001/chrome/renderer/instal...
File chrome/renderer/installation_service_impl.h (right):

https://codereview.chromium.org/2379923002/diff/400001/chrome/renderer/instal...
chrome/renderer/installation_service_impl.h:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
Adding new code in chrome/renderer is frowned upon. Can this get routed straight
to Blink in some way? Possibly after my CL?

https://codereview.chromium.org/2379923002/diff/400001/chrome/test/data/banne...
File chrome/test/data/banners/appinstalled_test_page.html (right):

https://codereview.chromium.org/2379923002/diff/400001/chrome/test/data/banne...
chrome/test/data/banners/appinstalled_test_page.html:1: <!DOCTYPE html>
Possibly dumb question: could you put the JS in here in an existing test file if
nothing else uses the title update to verify?

[Matt Giuca, 2016-11-01 06:45:27]

Patchset #21 (id:460001) has been deleted

[Matt Giuca, 2016-11-01 06:47:31]

Patchset #20 (id:440001) has been deleted

[Matt Giuca, 2016-11-01 07:15:57]

Patchset #22 (id:510001) has been deleted

[Matt Giuca, 2016-12-07 05:03:20]

The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 05:03:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-07 05:06:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-07 05:06:28]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Matt Giuca, 2016-12-07 06:37:35]

The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 06:37:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-07 09:21:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-07 09:21:07]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Matt Giuca, 2016-12-08 01:15:48]

The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-08 01:16:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 01:24:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-08 01:24:21]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Matt Giuca, 2016-12-08 02:29:34]

The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-08 02:30:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 02:33:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-08 02:33:59]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Matt Giuca, 2016-12-08 02:53:02]

Patchset #28 (id:650001) has been deleted

[Matt Giuca, 2016-12-08 02:53:40]

The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-08 02:54:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 02:57:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-08 02:57:32]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Matt Giuca, 2016-12-08 03:29:48]

The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-08 03:30:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 05:04:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-08 05:04:36]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Matt Giuca, 2016-12-09 04:35:06]

The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 04:35:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Matt Giuca, 2016-12-09 04:36:48]

Finally addressed your comments!

https://codereview.chromium.org/2379923002/diff/400001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenManagerTest.java
(right):

https://codereview.chromium.org/2379923002/diff/400001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenManagerTest.java:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
On 2016/10/12 22:35:09, dominickn wrote:
> Can you also add a test to AppBannerManagerTest?

Done.

https://codereview.chromium.org/2379923002/diff/400001/chrome/browser/android...
File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc
(right):

https://codereview.chromium.org/2379923002/diff/400001/chrome/browser/android...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:274: void
AppBannerInfoBarDelegateAndroid::SendBannerAccepted(
On 2016/10/12 22:35:09, dominickn wrote:
> What about AppBannerInfoBarDelegateDesktop? Shouldn't that also call
OnInstall?

I think desktop support for this can go in another CL (even if trivial). Testing
on desktop, at least, will be non-trivial.

https://codereview.chromium.org/2379923002/diff/400001/chrome/browser/android...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:283:
weak_manager_->OnInstall();
On 2016/10/12 22:35:09, dominickn wrote:
> We're already sending a message to the renderer here. It would be nice if we
> could combine them. My patch makes the first message go straight to the
> BeforeInstallPromptEvent; perhaps that can then ask the Frame to call
OnInstall?
> 
> Also, my CL moves the IPC call from here to AppBannerManager so you probably
> won't need to have this call in here at all - it'll be in AppBannerManager.

Hmmm yeah your CL did make this easier but it's still a bit messy to do that
because OnInstall can get called from not the banner. I prefer not to go through
my own mojo message for some cases but hitch-hike in your mojo message for this
one. Hard to understand.

Not worth it given how rare this message is sent.

https://codereview.chromium.org/2379923002/diff/400001/chrome/browser/android...
File chrome/browser/android/webapps/add_to_homescreen_manager.cc (right):

https://codereview.chromium.org/2379923002/diff/400001/chrome/browser/android...
chrome/browser/android/webapps/add_to_homescreen_manager.cc:121: 
On 2016/10/12 22:35:09, dominickn wrote:
> This doesn't account for WebAPKs, which call ShortcutHelper::InstallWebApk
> through AppBannerInfoBarDelegateAndroid. Of course, they also call
> AppBannerManager::SendBannerAccepted if they're initiated from a banner, so
you
> have to be a bit careful about it.

As with Desktop, let's leave WebAPKs for another CL?

https://codereview.chromium.org/2379923002/diff/400001/chrome/renderer/instal...
File chrome/renderer/installation_service_impl.h (right):

https://codereview.chromium.org/2379923002/diff/400001/chrome/renderer/instal...
chrome/renderer/installation_service_impl.h:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
On 2016/10/12 22:35:09, dominickn wrote:
> Adding new code in chrome/renderer is frowned upon. Can this get routed
straight
> to Blink in some way? Possibly after my CL?

Done.

https://codereview.chromium.org/2379923002/diff/400001/chrome/test/data/banne...
File chrome/test/data/banners/appinstalled_test_page.html (right):

https://codereview.chromium.org/2379923002/diff/400001/chrome/test/data/banne...
chrome/test/data/banners/appinstalled_test_page.html:1: <!DOCTYPE html>
On 2016/10/12 22:35:09, dominickn wrote:
> Possibly dumb question: could you put the JS in here in an existing test file
if
> nothing else uses the title update to verify?

Test pages used in this test: the other pages for the basic tests here aren't in
files at all. They are just hard-coded here, so that doesn't work.

Other .html files in this directory: I could but that would unnecessarily
complicate them. Best to have a dedicated page to test this thing, I think.

[commit-bot: I haz the power, 2016-12-09 05:01:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-09 05:01:23]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dominickn, 2016-12-09 06:08:18]

https://codereview.chromium.org/2379923002/diff/750001/chrome/browser/android...
File chrome/browser/android/webapps/add_to_homescreen_manager.h (right):

https://codereview.chromium.org/2379923002/diff/750001/chrome/browser/android...
chrome/browser/android/webapps/add_to_homescreen_manager.h:82:
blink::mojom::InstallationServicePtr installation_service_;
Doesn't need to be a member, use a local variable in AddShortcut().

https://codereview.chromium.org/2379923002/diff/750001/chrome/browser/banners...
File chrome/browser/banners/app_banner_manager.cc (right):

https://codereview.chromium.org/2379923002/diff/750001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager.cc:26: #include
"mojo/public/cpp/bindings/interface_request.h"
Is this include necessary? We're already calling GetInterface in here without
it.

https://codereview.chromium.org/2379923002/diff/750001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager.cc:137: if (!installation_service_) {
As noted in the header file, just made this a local variable in here.

https://codereview.chromium.org/2379923002/diff/750001/chrome/browser/banners...
File chrome/browser/banners/app_banner_manager.h (right):

https://codereview.chromium.org/2379923002/diff/750001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager.h:282:
blink::mojom::InstallationServicePtr installation_service_;
This is a one-way message (no callback). No need for the pointer to be a member;
just make it a local variable in OnInstall.

https://codereview.chromium.org/2379923002/diff/750001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/installation/InstallationServiceImpl.cpp
(right):

https://codereview.chromium.org/2379923002/diff/750001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/installation/InstallationServiceImpl.cpp:21:
mojo::MakeStrongBinding(makeUnique<InstallationServiceImpl>(frame),
You have to qualify this as WTF::makeUnique now (to disambiguate from
base::MakeUnique after the blink rename).

https://codereview.chromium.org/2379923002/diff/750001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/installation/InstallationServiceImpl.h
(right):

https://codereview.chromium.org/2379923002/diff/750001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/installation/InstallationServiceImpl.h:18:
explicit InstallationServiceImpl(LocalFrame*);
This should be a reference, not a pointer.

[Matt Giuca, 2016-12-12 04:28:11]

The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run

[Matt Giuca, 2016-12-12 04:28:20]

https://codereview.chromium.org/2379923002/diff/750001/chrome/browser/android...
File chrome/browser/android/webapps/add_to_homescreen_manager.h (right):

https://codereview.chromium.org/2379923002/diff/750001/chrome/browser/android...
chrome/browser/android/webapps/add_to_homescreen_manager.h:82:
blink::mojom::InstallationServicePtr installation_service_;
On 2016/12/09 06:08:17, dominickn wrote:
> Doesn't need to be a member, use a local variable in AddShortcut().

Acknowledged.

https://codereview.chromium.org/2379923002/diff/750001/chrome/browser/banners...
File chrome/browser/banners/app_banner_manager.cc (right):

https://codereview.chromium.org/2379923002/diff/750001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager.cc:26: #include
"mojo/public/cpp/bindings/interface_request.h"
On 2016/12/09 06:08:17, dominickn wrote:
> Is this include necessary? We're already calling GetInterface in here without
> it.

Done.

https://codereview.chromium.org/2379923002/diff/750001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager.cc:137: if (!installation_service_) {
On 2016/12/09 06:08:17, dominickn wrote:
> As noted in the header file, just made this a local variable in here.

Are there performance issues about calling GetMainFrame, GetRemoteInterfaces,
GetInterface and GetProxy every time?

(Can you link to a precedent for not caching this?)

https://codereview.chromium.org/2379923002/diff/750001/chrome/browser/banners...
File chrome/browser/banners/app_banner_manager.h (right):

https://codereview.chromium.org/2379923002/diff/750001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager.h:282:
blink::mojom::InstallationServicePtr installation_service_;
On 2016/12/09 06:08:17, dominickn wrote:
> This is a one-way message (no callback). No need for the pointer to be a
member;
> just make it a local variable in OnInstall.

Same question as on the other one.

https://codereview.chromium.org/2379923002/diff/750001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/installation/InstallationServiceImpl.cpp
(right):

https://codereview.chromium.org/2379923002/diff/750001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/installation/InstallationServiceImpl.cpp:21:
mojo::MakeStrongBinding(makeUnique<InstallationServiceImpl>(frame),
On 2016/12/09 06:08:18, dominickn wrote:
> You have to qualify this as WTF::makeUnique now (to disambiguate from
> base::MakeUnique after the blink rename).

Done.

https://codereview.chromium.org/2379923002/diff/750001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/installation/InstallationServiceImpl.h
(right):

https://codereview.chromium.org/2379923002/diff/750001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/installation/InstallationServiceImpl.h:18:
explicit InstallationServiceImpl(LocalFrame*);
On 2016/12/09 06:08:18, dominickn wrote:
> This should be a reference, not a pointer.

This goes against every fibre of my being. I know Blink people love to accept
them as arguments, but is it really OK to *store* a non-const reference (which I
would have to do here if taking a reference)?

I tried this and I couldn't get the callsite to work properly (some interaction
with wrapWeakPersistent I don't understand). Is this really necessary? Your
AppBannerController just takes a regular pointer....

[commit-bot: I haz the power, 2016-12-12 04:28:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-12 04:46:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-12 04:46:58]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[dominickn, 2016-12-12 05:03:31]

https://codereview.chromium.org/2379923002/diff/750001/chrome/browser/banners...
File chrome/browser/banners/app_banner_manager.cc (right):

https://codereview.chromium.org/2379923002/diff/750001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager.cc:137: if (!installation_service_) {
On 2016/12/12 04:28:20, Matt Giuca wrote:
> On 2016/12/09 06:08:17, dominickn wrote:
> > As noted in the header file, just made this a local variable in here.
> 
> Are there performance issues about calling GetMainFrame, GetRemoteInterfaces,
> GetInterface and GetProxy every time?
> 
> (Can you link to a precedent for not caching this?)

1) this isn't a frequently called method, but adding a member variable bloats
the size of this object, one of which lives on every WebContents. I don't think
performance overhead is really an issue here - I'd be surprised if users often
added to homescreen from the exact same tab multiple times.

2) some example of not caching include:

chrome/browser/android/contextualsearch/contextual_search_manager.cc
chrome/browser/task_manager/providers/child_process_task.cc
chrome/renderer/chrome_content_renderer_client.cc
chrome/renderer/chrome_render_thread_observer.cc

And app_banner_manager.cc too.

3) make it a local variable here and in AddToHomescreenManager. :)

https://codereview.chromium.org/2379923002/diff/750001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/installation/InstallationServiceImpl.h
(right):

https://codereview.chromium.org/2379923002/diff/750001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/installation/InstallationServiceImpl.h:18:
explicit InstallationServiceImpl(LocalFrame*);
On 2016/12/12 04:28:20, Matt Giuca wrote:
> On 2016/12/09 06:08:18, dominickn wrote:
> > This should be a reference, not a pointer.
> 
> This goes against every fibre of my being. I know Blink people love to accept
> them as arguments, but is it really OK to *store* a non-const reference (which
I
> would have to do here if taking a reference)?
> 
> I tried this and I couldn't get the callsite to work properly (some
interaction
> with wrapWeakPersistent I don't understand). Is this really necessary? Your
> AppBannerController just takes a regular pointer....

AppBannerController::bindMojoRequest (aka your create() method) takes a pointer,
because it needs to have wrapWeakPersistent. The constructor takes a reference.
See dcheng's comment at
https://codereview.chromium.org/2393513004/diff/340001/third_party/WebKit/Sou...

https://codereview.chromium.org/2379923002/diff/790001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java
(right):

https://codereview.chromium.org/2379923002/diff/790001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java:512:
@RetryOnFailure
Nit: remove RetryOnFailure (I have a CL up that removes it from this test
entirely). I believe adding this on new tests is now discouraged.

https://codereview.chromium.org/2379923002/diff/790001/chrome/browser/android...
File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc
(right):

https://codereview.chromium.org/2379923002/diff/790001/chrome/browser/android...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:362:
weak_manager_->SendBannerAccepted(event_request_id_);
You need if (!is_webapk_) to stop this triggering on WebAPKs.

https://codereview.chromium.org/2379923002/diff/790001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/installation/InstallationServiceImpl.cpp
(right):

https://codereview.chromium.org/2379923002/diff/790001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/installation/InstallationServiceImpl.cpp:26:
m_frame->onAppInstalled();
Instead of adding to LocalFrame's API, can you do

m_frame->localDOMWindow()->dispatchEvent(Event::create(EventTypeNames::appinstalled));

That may help this pass blink owners. Plenty of other code in Source/modules
calls Event::create :)

https://codereview.chromium.org/2379923002/diff/790001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/installation/installation.mojom
(right):

https://codereview.chromium.org/2379923002/diff/790001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/installation/installation.mojom:8: //
Indicates that a web app has been successfully installed.
The event is currently being fired for native apps as well (SendBannerAccepted()
from AppBannerInfoBarDelegateAndroid is called for WebAPKs, native apps, and web
apps). This comment isn't accurate.

[Matt Giuca, 2016-12-14 03:39:41]

https://codereview.chromium.org/2379923002/diff/750001/chrome/browser/banners...
File chrome/browser/banners/app_banner_manager.cc (right):

https://codereview.chromium.org/2379923002/diff/750001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager.cc:137: if (!installation_service_) {
On 2016/12/12 05:03:30, dominickn wrote:
> On 2016/12/12 04:28:20, Matt Giuca wrote:
> > On 2016/12/09 06:08:17, dominickn wrote:
> > > As noted in the header file, just made this a local variable in here.
> > 
> > Are there performance issues about calling GetMainFrame,
GetRemoteInterfaces,
> > GetInterface and GetProxy every time?
> > 
> > (Can you link to a precedent for not caching this?)
> 
> 1) this isn't a frequently called method, but adding a member variable bloats
> the size of this object, one of which lives on every WebContents. I don't
think
> performance overhead is really an issue here - I'd be surprised if users often
> added to homescreen from the exact same tab multiple times.
> 
> 2) some example of not caching include:
> 
> chrome/browser/android/contextualsearch/contextual_search_manager.cc
> chrome/browser/task_manager/providers/child_process_task.cc
> chrome/renderer/chrome_content_renderer_client.cc
> chrome/renderer/chrome_render_thread_observer.cc
> 
> And app_banner_manager.cc too.
> 
> 3) make it a local variable here and in AddToHomescreenManager. :)

Done.

https://codereview.chromium.org/2379923002/diff/750001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/installation/InstallationServiceImpl.h
(right):

https://codereview.chromium.org/2379923002/diff/750001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/installation/InstallationServiceImpl.h:18:
explicit InstallationServiceImpl(LocalFrame*);
On 2016/12/12 05:03:30, dominickn wrote:
> On 2016/12/12 04:28:20, Matt Giuca wrote:
> > On 2016/12/09 06:08:18, dominickn wrote:
> > > This should be a reference, not a pointer.
> > 
> > This goes against every fibre of my being. I know Blink people love to
accept
> > them as arguments, but is it really OK to *store* a non-const reference
(which
> I
> > would have to do here if taking a reference)?
> > 
> > I tried this and I couldn't get the callsite to work properly (some
> interaction
> > with wrapWeakPersistent I don't understand). Is this really necessary? Your
> > AppBannerController just takes a regular pointer....
> 
> AppBannerController::bindMojoRequest (aka your create() method) takes a
pointer,
> because it needs to have wrapWeakPersistent. The constructor takes a
reference.
> See dcheng's comment at
>
https://codereview.chromium.org/2393513004/diff/340001/third_party/WebKit/Sou...

Done.

This is super weird then. create takes a pointer, uses * to turn it into a
reference for the ctor, then the ctor immediately passes it to the
WeakPersistent ctor which uses & to turn it back into a pointer.

https://codereview.chromium.org/2379923002/diff/790001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java
(right):

https://codereview.chromium.org/2379923002/diff/790001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java:512:
@RetryOnFailure
On 2016/12/12 05:03:30, dominickn wrote:
> Nit: remove RetryOnFailure (I have a CL up that removes it from this test
> entirely). I believe adding this on new tests is now discouraged.

Done.

https://codereview.chromium.org/2379923002/diff/790001/chrome/browser/android...
File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc
(right):

https://codereview.chromium.org/2379923002/diff/790001/chrome/browser/android...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:362:
weak_manager_->SendBannerAccepted(event_request_id_);
On 2016/12/12 05:03:30, dominickn wrote:
> You need if (!is_webapk_) to stop this triggering on WebAPKs.

Done.

https://codereview.chromium.org/2379923002/diff/790001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/installation/InstallationServiceImpl.cpp
(right):

https://codereview.chromium.org/2379923002/diff/790001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/installation/InstallationServiceImpl.cpp:26:
m_frame->onAppInstalled();
On 2016/12/12 05:03:30, dominickn wrote:
> Instead of adding to LocalFrame's API, can you do
> 
>
m_frame->localDOMWindow()->dispatchEvent(Event::create(EventTypeNames::appinstalled));
> 
> That may help this pass blink owners. Plenty of other code in Source/modules
> calls Event::create :)

Done. I think I did that because InstallationServiceImpl used to be in Chrome,
but now it makes sense.

https://codereview.chromium.org/2379923002/diff/790001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/installation/installation.mojom
(right):

https://codereview.chromium.org/2379923002/diff/790001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/installation/installation.mojom:8: //
Indicates that a web app has been successfully installed.
On 2016/12/12 05:03:30, dominickn wrote:
> The event is currently being fired for native apps as well
(SendBannerAccepted()
> from AppBannerInfoBarDelegateAndroid is called for WebAPKs, native apps, and
web
> apps). This comment isn't accurate.

Done.

[Matt Giuca, 2016-12-14 03:39:53]

The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 03:40:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-14 03:44:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-14 03:44:51]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[Matt Giuca, 2016-12-15 02:38:02]

The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-15 02:38:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 02:56:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-15 02:56:37]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Matt Giuca, 2016-12-15 03:33:15]

Patchset #36 (id:830001) has been deleted

[Matt Giuca, 2016-12-15 03:33:57]

The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-15 03:34:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominickn, 2016-12-15 03:45:59]

lgtm % nits

https://codereview.chromium.org/2379923002/diff/850001/chrome/browser/android...
File chrome/browser/android/webapps/add_to_homescreen_manager.cc (right):

https://codereview.chromium.org/2379923002/diff/850001/chrome/browser/android...
chrome/browser/android/webapps/add_to_homescreen_manager.cc:23: #include
"jni/AddToHomescreenManager_jni.h"
#include "mojo/public/cpp/bindings/interface_request.h" for GetProxy (you added
this include in app_banner_manager.h)

https://codereview.chromium.org/2379923002/diff/850001/chrome/browser/android...
File chrome/browser/android/webapps/add_to_homescreen_manager.h (right):

https://codereview.chromium.org/2379923002/diff/850001/chrome/browser/android...
chrome/browser/android/webapps/add_to_homescreen_manager.h:12: #include
"third_party/WebKit/public/platform/modules/installation/installation.mojom.h"
Nit: move this include to the cc file.

https://codereview.chromium.org/2379923002/diff/850001/chrome/browser/banners...
File chrome/browser/banners/app_banner_manager.h (right):

https://codereview.chromium.org/2379923002/diff/850001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager.h:20: #include
"third_party/WebKit/public/platform/modules/installation/installation.mojom.h"
Nit: move this include to the cc file.

https://codereview.chromium.org/2379923002/diff/850001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/installation/InstallationServiceImpl.h
(right):

https://codereview.chromium.org/2379923002/diff/850001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/installation/InstallationServiceImpl.h:16:
class MODULES_EXPORT InstallationServiceImpl final
I imagine someone is going to ask about rolling this and the banner blink impl
together at some point. I don't think it's necessary for this CL, but if/when
that happens, the lifetime semantics of this object might need to change. In
fact, perhaps we should make merging all of the installability blink stuff
dependent on changes that make the blink code execution context scoped rather
than frame scoped.

[commit-bot: I haz the power, 2016-12-15 05:17:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-15 05:17:17]

Dry run: This issue passed the CQ dry run.

[Matt Giuca, 2016-12-15 06:17:23]

https://codereview.chromium.org/2379923002/diff/850001/chrome/browser/android...
File chrome/browser/android/webapps/add_to_homescreen_manager.cc (right):

https://codereview.chromium.org/2379923002/diff/850001/chrome/browser/android...
chrome/browser/android/webapps/add_to_homescreen_manager.cc:23: #include
"jni/AddToHomescreenManager_jni.h"
On 2016/12/15 03:45:59, dominickn wrote:
> #include "mojo/public/cpp/bindings/interface_request.h" for GetProxy (you
added
> this include in app_banner_manager.h)

Done.

https://codereview.chromium.org/2379923002/diff/850001/chrome/browser/android...
File chrome/browser/android/webapps/add_to_homescreen_manager.h (right):

https://codereview.chromium.org/2379923002/diff/850001/chrome/browser/android...
chrome/browser/android/webapps/add_to_homescreen_manager.h:12: #include
"third_party/WebKit/public/platform/modules/installation/installation.mojom.h"
On 2016/12/15 03:45:59, dominickn wrote:
> Nit: move this include to the cc file.

Done.

https://codereview.chromium.org/2379923002/diff/850001/chrome/browser/banners...
File chrome/browser/banners/app_banner_manager.h (right):

https://codereview.chromium.org/2379923002/diff/850001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager.h:20: #include
"third_party/WebKit/public/platform/modules/installation/installation.mojom.h"
On 2016/12/15 03:45:59, dominickn wrote:
> Nit: move this include to the cc file.

Done.

https://codereview.chromium.org/2379923002/diff/850001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/installation/InstallationServiceImpl.h
(right):

https://codereview.chromium.org/2379923002/diff/850001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/installation/InstallationServiceImpl.h:16:
class MODULES_EXPORT InstallationServiceImpl final
On 2016/12/15 03:45:59, dominickn wrote:
> I imagine someone is going to ask about rolling this and the banner blink impl
> together at some point. I don't think it's necessary for this CL, but if/when
> that happens, the lifetime semantics of this object might need to change. In
> fact, perhaps we should make merging all of the installability blink stuff
> dependent on changes that make the blink code execution context scoped rather
> than frame scoped.

Acknowledged.

[Matt Giuca, 2016-12-15 06:17:34]

The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-15 06:17:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Matt Giuca, 2016-12-15 06:25:15]

mgiuca@chromium.org changed reviewers:
+ haraken@chromium.org, jochen@chromium.org, rkaplow@chromium.org,
tsepez@chromium.org

[Matt Giuca, 2016-12-15 06:25:16]

Hi reviewers, please review the following files:

haraken:
  third_party/WebKit/*
jochen:
  chrome/browser/DEPS
rkaplow:
  tools/metrics/histograms/histograms.xml,
  third_party/WebKit/Source/core/frame/UseCounter.h
  (metrics OWNERS)
tsepez:
  content/public/app/mojo/content_renderer_manifest.json,
  third_party/WebKit/public/platform/modules/installation/installation.mojom
  (SECURITY_OWNERS)

Intent to implement and ship thread (with 3 LGs):
https://groups.google.com/a/chromium.org/d/topic/blink-dev/hWT6i_qXcjo/discus...

Thanks!

[Matt Giuca, 2016-12-15 06:29:36]

mgiuca@chromium.org changed reviewers:
+ chrishtr@chromium.org

[Matt Giuca, 2016-12-15 06:29:38]

+chrishtr: I got this presubmit error.

You need LGTM from owners of depends-on paths in DEPS that were modified in this
CL:
   
'+third_party/WebKit/public/platform/modules/installation/installation.mojom.h',

Suggested missing target path OWNERS:
    chrishtr@chromium.org

OK... I don't really understand this (I thought I just needed jochen@ or
somebody to approve the change to DEPS) but maybe you can approve this too?

[haraken, 2016-12-15 07:27:50]

WebKit LGTM

https://codereview.chromium.org/2379923002/diff/870001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/installation/InstallationServiceImpl.cpp
(right):

https://codereview.chromium.org/2379923002/diff/870001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/installation/InstallationServiceImpl.cpp:8:
#include "core/frame/LocalDOMWindow.h"

unused

https://codereview.chromium.org/2379923002/diff/870001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/installation/InstallationServiceImpl.cpp:12:
#include <utility>

unused ?

https://codereview.chromium.org/2379923002/diff/870001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/installation/InstallationServiceImpl.cpp:28:
m_frame->domWindow()->dispatchEvent(

You need to check if (m_frame && m_frame->domWindow()).

[commit-bot: I haz the power, 2016-12-15 07:40:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-15 07:40:30]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2016-12-15 14:18:27]

lgtm

[Tom Sepez, 2016-12-15 17:42:11]

mojom LGTM

[Matt Giuca, 2016-12-16 03:53:47]

https://codereview.chromium.org/2379923002/diff/870001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/installation/InstallationServiceImpl.cpp
(right):

https://codereview.chromium.org/2379923002/diff/870001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/installation/InstallationServiceImpl.cpp:8:
#include "core/frame/LocalDOMWindow.h"
On 2016/12/15 07:27:50, haraken wrote:
> 
> unused

m_frame->domWindow() is a LocalDOMWindow, so I need this.

(It just the other day got renamed from localDOMWindow to domWindow; not sure
why since its type is still LocalDOMWindow.)

https://codereview.chromium.org/2379923002/diff/870001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/installation/InstallationServiceImpl.cpp:12:
#include <utility>
On 2016/12/15 07:27:50, haraken wrote:
> 
> unused ?

std::move is in <utility>

https://codereview.chromium.org/2379923002/diff/870001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/installation/InstallationServiceImpl.cpp:28:
m_frame->domWindow()->dispatchEvent(
On 2016/12/15 07:27:50, haraken wrote:
> 
> You need to check if (m_frame && m_frame->domWindow()).

Done.

[Matt Giuca, 2016-12-16 03:55:07]

The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 03:55:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-16 03:58:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-16 03:58:03]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Matt Giuca, 2016-12-19 02:08:37]

mgiuca@chromium.org changed reviewers:
+ isherman@chromium.org

[Matt Giuca, 2016-12-19 02:08:38]

+isherman: I haven't heard from rkaplow in many days so maybe you can review
histograms?

[Ilya Sherman, 2016-12-19 02:50:12]

histograms.xml lgtm

[Matt Giuca, 2016-12-19 02:51:39]

On 2016/12/19 02:50:12, Ilya Sherman wrote:
> histograms.xml lgtm

Thanks!

[Matt Giuca, 2016-12-19 02:53:34]

The CQ bit was checked by mgiuca@chromium.org

[Matt Giuca, 2016-12-19 02:53:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org, jochen@chromium.org, haraken@chromium.org,
tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2379923002/#ps910001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-12-19 02:53:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-19 05:44:11]

CQ is committing da patch.

Bot data: {"patchset_id": 910001, "attempt_start_ts": 1482116014953130,
"parent_rev": "cff979756e9da4cb813f5c9c44630c4e3b663e54", "commit_rev":
"4aac82cf6e8014e939dd66b207b0445d9204c84c"}

[commit-bot: I haz the power, 2016-12-19 05:44:50]

Description was changed from

==========
Implement "appinstalled" event on Android.

This event is fired when a web app is "installed" (i.e., a shortcut is
added to home screen).

This implements from the working draft of Web App Manifest:
https://www.w3.org/TR/appmanifest/#onappinstalled-attribute

BUG=621393
==========

to

==========
Implement "appinstalled" event on Android.

This event is fired when a web app is "installed" (i.e., a shortcut is
added to home screen).

This implements from the working draft of Web App Manifest:
https://www.w3.org/TR/appmanifest/#onappinstalled-attribute

BUG=621393

Review-Url: https://codereview.chromium.org/2379923002
==========

[commit-bot: I haz the power, 2016-12-19 05:44:52]

Committed patchset #39 (id:910001)

[commit-bot: I haz the power, 2016-12-19 05:47:22]

Description was changed from

==========
Implement "appinstalled" event on Android.

This event is fired when a web app is "installed" (i.e., a shortcut is
added to home screen).

This implements from the working draft of Web App Manifest:
https://www.w3.org/TR/appmanifest/#onappinstalled-attribute

BUG=621393

Review-Url: https://codereview.chromium.org/2379923002
==========

to

==========
Implement "appinstalled" event on Android.

This event is fired when a web app is "installed" (i.e., a shortcut is
added to home screen).

This implements from the working draft of Web App Manifest:
https://www.w3.org/TR/appmanifest/#onappinstalled-attribute

BUG=621393

Committed: https://crrev.com/b02c2580cd1427502c41072ebe834993b1d139d0
Cr-Commit-Position: refs/heads/master@{#439416}
==========

[commit-bot: I haz the power, 2016-12-19 05:47:23]

Patchset 39 (id:??) landed as
https://crrev.com/b02c2580cd1427502c41072ebe834993b1d139d0
Cr-Commit-Position: refs/heads/master@{#439416}