Android: Fire the appinstalled event on WebAPK installation.

Android: Fire the appinstalled event on WebAPK installation.

Previously, it would only fire for a shortcut creation (when the app is
not associated with a WebAPK).

BUG=688178
Review-Url: https://codereview.chromium.org/2670963002
Cr-Commit-Position: refs/heads/master@{#466580}
Committed: https://chromium.googlesource.com/chromium/src/+/00e4fb83516d445e8952416ed29a481b399a0a24

[Matt Giuca, 2017-02-03 04:13:38]

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

[Matt Giuca, 2017-02-03 04:13:38]

Hi Dom,

So IIRC you told me previously that just removing that if statement would not
work for WebAPKs (and hence I wrote that comment). However, I don't know exactly
what you had in mind that would be necessary.

From my visual inspection, rearranging it like this should get it triggered in
all cases (whether coming from a banner or manual A2HS). But I have no way to
test it (manually or automatically). Maybe you can give your thoughts on whether
you think this will work, then next week can help me test it.

[dominickn, 2017-02-03 05:54:46]

https://codereview.chromium.org/2670963002/diff/1/chrome/browser/android/bann...
File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc
(right):

https://codereview.chromium.org/2670963002/diff/1/chrome/browser/android/bann...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:378:
weak_manager_->OnInstall();
This doesn't work because weak_manager_ is nullptr for WebAPKs which are
installed from the add to homescreen menu item (see
https://cs.chromium.org/chromium/src/chrome/browser/android/webapps/add_to_ho...)

A super hacky way to fix this is to grab the AppBannerManagerAndroid off the
WebContents when creating this delegate in the A2HS code path. I have not
thought through the implications of that yet, but it feels a bit ugly.

Longer term, the WebAPK stuff is going to get split out of this class.

[Matt Giuca, 2017-04-07 06:40:40]

Patchset #2 (id:20001) has been deleted

[Matt Giuca, 2017-04-07 06:41:34]

https://codereview.chromium.org/2670963002/diff/1/chrome/browser/android/bann...
File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc
(right):

https://codereview.chromium.org/2670963002/diff/1/chrome/browser/android/bann...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:378:
weak_manager_->OnInstall();
On 2017/02/03 05:54:46, dominickn wrote:
> This doesn't work because weak_manager_ is nullptr for WebAPKs which are
> installed from the add to homescreen menu item (see
>
https://cs.chromium.org/chromium/src/chrome/browser/android/webapps/add_to_ho...)
> 
> A super hacky way to fix this is to grab the AppBannerManagerAndroid off the
> WebContents when creating this delegate in the A2HS code path. I have not
> thought through the implications of that yet, but it feels a bit ugly.
> 
> Longer term, the WebAPK stuff is going to get split out of this class.

Done.

[Matt Giuca, 2017-04-12 07:18:17]

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

[commit-bot: I haz the power, 2017-04-12 07:18:32]

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, 2017-04-12 07:54:44]

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

[commit-bot: I haz the power, 2017-04-12 07:54:45]

Dry run: This issue passed the CQ dry run.

[Matt Giuca, 2017-04-13 06:58:01]

This is too hard to test manually right now. Decided to hold off until after
branch, then land in canary and test it there (then maybe merge to M59).

[Matt Giuca, 2017-04-24 00:13:34]

On 2017/04/13 06:58:01, Matt Giuca wrote:
> This is too hard to test manually right now. Decided to hold off until after
> branch, then land in canary and test it there (then maybe merge to M59).

Hey Dom,

As discussed previously we would like to land this to test on Canary. PTAL.

[dominickn, 2017-04-24 00:56:30]

Just minor comments, thanks!

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

https://codereview.chromium.org/2670963002/diff/80001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:361: //
Send the appinstalled event in all cases (whether triggered from a banner
This comment seems superfluous. What's more important is noting that the
appinstalled event is being fired when the user initiates an install, not when
the install finished. For non-WebAPKs, that's fine because install is trivial.
But WebAPKs can take O(seconds) to install so it's not as accurate as it could
be (maybe TODO to fix that).

https://codereview.chromium.org/2670963002/diff/80001/chrome/browser/android/...
File chrome/browser/android/banners/app_banner_manager_android.h (right):

https://codereview.chromium.org/2670963002/diff/80001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_manager_android.h:74: using
AppBannerManager::GetWeakPtr;
Instead of using, just make the GetWeakPtr method public.

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

https://codereview.chromium.org/2670963002/diff/80001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_manager.cc:95:
app_banner_manager_ =
Having thought about this, I think it's better to not have the instance variable
and just call ::FromWebContents at the two places in this file that need it
(both have a WebContents available). That means you don't need the forward
declaration in the header file and you can isolate the knowledge of the
AppBannerManager in this cc file. The two code paths are disjoint, and it means
we're not carrying around the extra instance pointer in the case where the user
taps add to homescreen but then cancels the process.

WDYT? If you want to keep the instance variable, move this to the start of the
method.

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

https://codereview.chromium.org/2670963002/diff/80001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_manager.h:85:
banners::AppBannerManagerAndroid* app_banner_manager_;
Nit: Add a comment:

// Dispatches the oninstall event to the page when a user adds a shortcut.

[Matt Giuca, 2017-04-24 01:42:14]

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

https://codereview.chromium.org/2670963002/diff/80001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:361: //
Send the appinstalled event in all cases (whether triggered from a banner
On 2017/04/24 00:56:30, dominickn wrote:
> This comment seems superfluous. What's more important is noting that the
> appinstalled event is being fired when the user initiates an install, not when
> the install finished. For non-WebAPKs, that's fine because install is trivial.
> But WebAPKs can take O(seconds) to install so it's not as accurate as it could
> be (maybe TODO to fix that).

Done.

I don't think big-oh works like that...

https://codereview.chromium.org/2670963002/diff/80001/chrome/browser/android/...
File chrome/browser/android/banners/app_banner_manager_android.h (right):

https://codereview.chromium.org/2670963002/diff/80001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_manager_android.h:74: using
AppBannerManager::GetWeakPtr;
On 2017/04/24 00:56:30, dominickn wrote:
> Instead of using, just make the GetWeakPtr method public.

Done.

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

https://codereview.chromium.org/2670963002/diff/80001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_manager.cc:95:
app_banner_manager_ =
On 2017/04/24 00:56:30, dominickn wrote:
> Having thought about this, I think it's better to not have the instance
variable
> and just call ::FromWebContents at the two places in this file that need it
> (both have a WebContents available). That means you don't need the forward
> declaration in the header file and you can isolate the knowledge of the
> AppBannerManager in this cc file. The two code paths are disjoint, and it
means
> we're not carrying around the extra instance pointer in the case where the
user
> taps add to homescreen but then cancels the process.
> 
> WDYT? If you want to keep the instance variable, move this to the start of the
> method.

Done.

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

https://codereview.chromium.org/2670963002/diff/80001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_manager.h:85:
banners::AppBannerManagerAndroid* app_banner_manager_;
On 2017/04/24 00:56:30, dominickn wrote:
> Nit: Add a comment:
> 
> // Dispatches the oninstall event to the page when a user adds a shortcut.

Gone.

[Matt Giuca, 2017-04-24 01:44:35]

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

[commit-bot: I haz the power, 2017-04-24 01:44:49]

Dry run: CQ is trying da patch.

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

[dominickn, 2017-04-24 01:45:14]

lgtm, thanks for everything on this.

[Matt Giuca, 2017-04-24 01:46:42]

The CQ bit was unchecked by mgiuca@chromium.org

[Matt Giuca, 2017-04-24 01:46:48]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2017-04-24 01:46:58]

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, 2017-04-24 02:14:32]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1492998408071830,
"parent_rev": "e40dfa1b230170ab32d3273313c2a606c6da8492", "commit_rev":
"00e4fb83516d445e8952416ed29a481b399a0a24"}

[commit-bot: I haz the power, 2017-04-24 02:14:39]

Description was changed from

==========
Android: Fire the appinstalled event on WebAPK installation.

Previously, it would only fire for a shortcut creation (when the app is
not associated with a WebAPK).

BUG=688178
==========

to

==========
Android: Fire the appinstalled event on WebAPK installation.

Previously, it would only fire for a shortcut creation (when the app is
not associated with a WebAPK).

BUG=688178

Review-Url: https://codereview.chromium.org/2670963002
Cr-Commit-Position: refs/heads/master@{#466580}
Committed:
https://chromium.googlesource.com/chromium/src/+/00e4fb83516d445e8952416ed29a...
==========

[commit-bot: I haz the power, 2017-04-24 02:14:40]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/00e4fb83516d445e8952416ed29a...