Add a new WebAPK-specific app banner shortcut source.

Add a new WebAPK-specific app banner shortcut source.

Currently, WebAPKs and legacy PWA shortcuts added from app banners are
both recorded using the ShortcutSource::SOURCE_APP_BANNER bucket. This
means it is impossible to determine what shortcut launches from banners
are triggered for legacy PWAs compared to WebAPKs.

This CL adds a new ShortcutSource enum bucket for WebAPKs added to the
homescreen via an app banner. This means that WebAPKs installed from
banners have an explicitly different source, allowing them to be
differentiated in metrics.

The legacy SOURCE_APP_BANNER bucket is still used for non-WebAPK PWA
shortcuts, and will be reported for launches of legacy PWAs added prior
to M59.

BUG=691739
Review-Url: https://codereview.chromium.org/2808263004
Cr-Commit-Position: refs/heads/master@{#469576}
Committed: https://chromium.googlesource.com/chromium/src/+/f197d9b5000564960a0b79babb804fad9c74aa01

[dominickn, 2017-04-12 04:11:15]

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

[commit-bot: I haz the power, 2017-04-12 04:11:24]

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-12 04:11:55]

dominickn@chromium.org changed reviewers:
+ isherman@chromium.org, pkotwicz@chromium.org

[dominickn, 2017-04-12 04:11:55]

PTAL, thanks!

Peter: I realised that we had a problem distinguishing between WebAPKs added
from banners versus legacy shortcuts, so I wrote this CL.

[commit-bot: I haz the power, 2017-04-12 05:16:52]

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

[commit-bot: I haz the power, 2017-04-12 05:16:53]

Dry run: This issue passed the CQ dry run.

[pkotwicz, 2017-04-12 15:45:53]

On 2017/04/12 04:11:55, dominickn wrote:
> PTAL, thanks!
> 
> Peter: I realised that we had a problem distinguishing between WebAPKs added
> from banners versus legacy shortcuts, so I wrote this CL.

[pkotwicz, 2017-04-12 15:46:01]

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

https://codereview.chromium.org/2808263004/diff/1/chrome/browser/android/bann...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:234:
shortcut_info_->UpdateSource(ShortcutInfo::SOURCE_APP_BANNER_WEBAPK);
I would rather if this code was in AppBannerManagerAndroid

That's where we set ShortcutInfo::SOURCE_APP_BANNER too

We would need to plumb |is_webapk| to AppBannerInfoBarDelegateAndroid::Create()

[dominickn, 2017-04-12 21:23:52]

On 2017/04/12 15:46:01, pkotwicz wrote:
>
https://codereview.chromium.org/2808263004/diff/1/chrome/browser/android/bann...
> File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc
> (right):
> 
>
https://codereview.chromium.org/2808263004/diff/1/chrome/browser/android/bann...
> chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:234:
> shortcut_info_->UpdateSource(ShortcutInfo::SOURCE_APP_BANNER_WEBAPK);
> I would rather if this code was in AppBannerManagerAndroid
> 
> That's where we set ShortcutInfo::SOURCE_APP_BANNER too
> 
> We would need to plumb |is_webapk| to
AppBannerInfoBarDelegateAndroid::Create()

I'd prefer to not add that plumbing. Pretty soon, I imagine that we'll remove
app banner support for non-WebAPKs, and so we'll just set the new source
directly in AppBannerManager. Then we'll have added extra complexity that's not
need and will be removed

[pkotwicz, 2017-04-12 22:17:00]

I think that this extra work is warranted. It is pretty hard to figure out what
the source is.

SOURCE_APP_BANNER_WEBAPK overwrites SOURCE_APP_BANNER which overwrites
SOURCE_ADD_TO_HOMESCREEN_STANDALONE which overwrites
SOURCE_ADD_TO_HOMESCREEN_SHORTCUT

[Ilya Sherman, 2017-04-12 22:20:53]

histograms.xml lgtm

[pkotwicz, 2017-04-12 22:31:11]

Actually, a few higher level questions.

For WebAPKs:
- I don't think that ShortcutInfo::source() in
app_banner_infobar_delegate_android.cc makes its way to
WebappLauncherActivity.java and WebApkActivity.java When a WebAPK is launched,
it does not know how it has been created
- We call ShortcutHelper::AddToLauncherWithSkBitmap() from
AppBannerInfoBarDelegateAndroid::OnWebApkInstallFailed(). In this case your CL
will record SOURCE_APP_BANNER_WEBAPK when the fullscreen legacy PWA is launched

[dominickn, 2017-04-12 22:44:46]

On 2017/04/12 22:31:11, pkotwicz wrote:
> Actually, a few higher level questions.
> 
> For WebAPKs:
> - I don't think that ShortcutInfo::source() in
> app_banner_infobar_delegate_android.cc makes its way to
> WebappLauncherActivity.java and WebApkActivity.java When a WebAPK is launched,
> it does not know how it has been created

Doesn't that mean that the Launch.HomescreenSource is mostly useless for
WebAPKs?

> - We call ShortcutHelper::AddToLauncherWithSkBitmap() from
> AppBannerInfoBarDelegateAndroid::OnWebApkInstallFailed(). In this case your CL
> will record SOURCE_APP_BANNER_WEBAPK when the fullscreen legacy PWA is
launched

Then that will have to override the source again (I'll follow up). More
generally, the source is going to be difficult to identify. By its very nature
it is going to be set and overridden in lots of places since there are different
fallback modes and places where async new information is integrated. That's why
I don't really want to change plumbing to make one place where the source is set
marginally clearer (it's going to have confusing overrides elsewhere that we
can't get rid of). There's not a huge benefit to clarity, so I don't think the
extra churn is worth it.

[pkotwicz, 2017-04-13 01:19:18]

Launch.HomescreenSource tells us whether the WebAPK was launched from a
notification or was launched from an external intent. It does not tell us how
the WebAPK was created though

We could change WebApkInstaller to store how the WebAPK was created in the
WebAPK's AndroidManifest.xml However, this a Chrome customization to the WebAPK.
The current (perhaps too optimistic) thinking is that a WebAPK will be created
for a particular site with the exact same AndroidManifest.xml regardless of
which browser was used to generate the WebAPK

[dominickn, 2017-04-13 04:24:21]

On 2017/04/13 01:19:18, pkotwicz wrote:
> Launch.HomescreenSource tells us whether the WebAPK was launched from a
> notification or was launched from an external intent. It does not tell us how
> the WebAPK was created though
> 
> We could change WebApkInstaller to store how the WebAPK was created in the
> WebAPK's AndroidManifest.xml However, this a Chrome customization to the
WebAPK.
> The current (perhaps too optimistic) thinking is that a WebAPK will be created
> for a particular site with the exact same AndroidManifest.xml regardless of
> which browser was used to generate the WebAPK

I think it's pretty important that we preserve the source information. I've
filed crbug.com/711147 to discuss this, and I'll hold off on this CL until we've
worked out what to do.

[dominickn, 2017-05-03 00:26:04]

Sorry for the delay on this one.

pkotwicz: how do you feel about landing this as is? That will unblock hanxi from
working on crbug.com/711147 (which will correctly send these source values for
WebAPKs). I would prefer keeping WebAPK-specific stuff in one class rather than
polluting other classes (it will make upcoming refactoring more difficult when
we either deprecate or split out legacy PWA shortcuts)

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

https://codereview.chromium.org/2808263004/diff/1/chrome/browser/android/bann...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:234:
shortcut_info_->UpdateSource(ShortcutInfo::SOURCE_APP_BANNER_WEBAPK);
On 2017/04/12 15:46:01, pkotwicz wrote:
> I would rather if this code was in AppBannerManagerAndroid
> 
> That's where we set ShortcutInfo::SOURCE_APP_BANNER too
> 
> We would need to plumb |is_webapk| to
AppBannerInfoBarDelegateAndroid::Create()

I'd rather keep WebAPK specific code in here. Soon enough we'll either need to
split apart AppBannerInfoBarDelegateAndroid into a WebAPK version and legacy
version, or we'll entirely remove legacy PWA shortcuts and thus everything in
here will be WebAPKs only. It will be much easier to do that if all of the
WebAPK-specific code is isolated in here.

[pkotwicz, 2017-05-03 17:46:41]

Description was changed from

==========
Add a new WebAPK-specific app banner shortcut source.

Currently, WebAPKs and legacy PWA shortcuts added from app banners are
both recorded using the ShortcutSource::SOURCE_APP_BANNER bucket. This
means it is impossible to determine what shortcut launches from banners
are triggered for legacy PWAs compared to WebAPKs.

This CL adds a new ShortcutSource enum bucket for WebAPKs added to the
homescreen via an app banner. This means that WebAPKs installed from
banners have an explicitly different source, allowing them to be
differentiated in metrics.

The legacy SOURCE_APP_BANNER bucket is still used for non-WebAPK PWA
shortcuts, and will be reported for launches of legacy PWAs added prior
to M59.

BUG=691739
==========

to

==========
Add a new WebAPK-specific app banner shortcut source.

Currently, WebAPKs and legacy PWA shortcuts added from app banners are
both recorded using the ShortcutSource::SOURCE_APP_BANNER bucket. This
means it is impossible to determine what shortcut launches from banners
are triggered for legacy PWAs compared to WebAPKs.

This CL adds a new ShortcutSource enum bucket for WebAPKs added to the
homescreen via an app banner. This means that WebAPKs installed from
banners have an explicitly different source, allowing them to be
differentiated in metrics.

The legacy SOURCE_APP_BANNER bucket is still used for non-WebAPK PWA
shortcuts, and will be reported for launches of legacy PWAs added prior
to M59.

BUG=691739
==========

[pkotwicz, 2017-05-03 17:46:41]

pkotwicz@chromium.org changed reviewers:
+ hanxi@chromium.org

[pkotwicz, 2017-05-03 17:47:30]

+hanxi

Let's do it right right away. I think that hanxi is blocked because she does not
want to duplicate any of your work. Maybe sync with her on who will implement
what?

[Xi Han, 2017-05-03 18:07:50]

It would be nice if this CL can be landed soon:)

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

https://codereview.chromium.org/2808263004/diff/1/chrome/browser/android/bann...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:234:
shortcut_info_->UpdateSource(ShortcutInfo::SOURCE_APP_BANNER_WEBAPK);
On 2017/05/03 00:26:04, dominickn wrote:
> On 2017/04/12 15:46:01, pkotwicz wrote:
> > I would rather if this code was in AppBannerManagerAndroid
> > 
> > That's where we set ShortcutInfo::SOURCE_APP_BANNER too
> > 
> > We would need to plumb |is_webapk| to
> AppBannerInfoBarDelegateAndroid::Create()
> 
> I'd rather keep WebAPK specific code in here. Soon enough we'll either need to
> split apart AppBannerInfoBarDelegateAndroid into a WebAPK version and legacy
> version, or we'll entirely remove legacy PWA shortcuts and thus everything in
> here will be WebAPKs only. It will be much easier to do that if all of the
> WebAPK-specific code is isolated in here.

AppBannerManagerAndroid has the |can_install_webapk_| that already checked
whether it is WebAPK, so we can set the source there without plumbing the
|is_webapk| to AppBannerInfoBarDelegateAndroid::Create(). I don't have a strong
preference, but it is nice to set it ASAP. Dom, it is up to you:)

[dominickn, 2017-05-04 00:33:59]

Thanks for the feedback, PTAL!

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

https://codereview.chromium.org/2808263004/diff/1/chrome/browser/android/bann...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:234:
shortcut_info_->UpdateSource(ShortcutInfo::SOURCE_APP_BANNER_WEBAPK);
On 2017/05/03 18:07:50, Xi Han wrote:
> On 2017/05/03 00:26:04, dominickn wrote:
> > On 2017/04/12 15:46:01, pkotwicz wrote:
> > > I would rather if this code was in AppBannerManagerAndroid
> > > 
> > > That's where we set ShortcutInfo::SOURCE_APP_BANNER too
> > > 
> > > We would need to plumb |is_webapk| to
> > AppBannerInfoBarDelegateAndroid::Create()
> > 
> > I'd rather keep WebAPK specific code in here. Soon enough we'll either need
to
> > split apart AppBannerInfoBarDelegateAndroid into a WebAPK version and legacy
> > version, or we'll entirely remove legacy PWA shortcuts and thus everything
in
> > here will be WebAPKs only. It will be much easier to do that if all of the
> > WebAPK-specific code is isolated in here.
> 
> AppBannerManagerAndroid has the |can_install_webapk_| that already checked
> whether it is WebAPK, so we can set the source there without plumbing the
> |is_webapk| to AppBannerInfoBarDelegateAndroid::Create(). I don't have a
strong
> preference, but it is nice to set it ASAP. Dom, it is up to you:)

Hmm, that's true. I forgot about that member entirely. I'll move it there then.

[dominickn, 2017-05-04 00:34:02]

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

[commit-bot: I haz the power, 2017-05-04 00:34: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, 2017-05-04 01:41:24]

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

[commit-bot: I haz the power, 2017-05-04 01:41:25]

Dry run: This issue passed the CQ dry run.

[Xi Han, 2017-05-04 13:42:18]

lgtm!

[pkotwicz, 2017-05-04 17:38:15]

LGTM with nits:

Can you please pass |can_install_webapk_| to
AppBannerInfoBarDelegateAndroid::Create() It is a bit silly for both
AppBannerManagerAndroid and AppBannerInfoBarDelegateAndroid to call
ChromeWebApkHost::CanInstallWebApk()

https://codereview.chromium.org/2808263004/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2808263004/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:100392: <int value="9" label="External
intent"/>
Might as well comment that this one has to do with WebAPKs

[dominickn, 2017-05-05 00:19:41]

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

[commit-bot: I haz the power, 2017-05-05 00:20:11]

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-05-05 00:24:37]

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

[commit-bot: I haz the power, 2017-05-05 00:24:39]

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_...)

[dominickn, 2017-05-05 00:45:19]

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

[commit-bot: I haz the power, 2017-05-05 00:45:51]

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-05-05 02:14:51]

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

[commit-bot: I haz the power, 2017-05-05 02:14:52]

Dry run: This issue passed the CQ dry run.

[dominickn, 2017-05-05 02:30:40]

Thanks!

https://codereview.chromium.org/2808263004/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2808263004/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:100392: <int value="9" label="External
intent"/>
On 2017/05/04 17:38:15, pkotwicz wrote:
> Might as well comment that this one has to do with WebAPKs

Done.

[dominickn, 2017-05-05 02:32:06]

The CQ bit was checked by dominickn@chromium.org

[dominickn, 2017-05-05 02:32:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, pkotwicz@chromium.org, hanxi@chromium.org
Link to the patchset: https://codereview.chromium.org/2808263004/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2017-05-05 02:32:27]

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-05-05 02:57:40]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493951526246580,
"parent_rev": "ba9abc4c70ae99cf7fc03adbd48258cea2514595", "commit_rev":
"f197d9b5000564960a0b79babb804fad9c74aa01"}

[commit-bot: I haz the power, 2017-05-05 02:58:41]

Description was changed from

==========
Add a new WebAPK-specific app banner shortcut source.

Currently, WebAPKs and legacy PWA shortcuts added from app banners are
both recorded using the ShortcutSource::SOURCE_APP_BANNER bucket. This
means it is impossible to determine what shortcut launches from banners
are triggered for legacy PWAs compared to WebAPKs.

This CL adds a new ShortcutSource enum bucket for WebAPKs added to the
homescreen via an app banner. This means that WebAPKs installed from
banners have an explicitly different source, allowing them to be
differentiated in metrics.

The legacy SOURCE_APP_BANNER bucket is still used for non-WebAPK PWA
shortcuts, and will be reported for launches of legacy PWAs added prior
to M59.

BUG=691739
==========

to

==========
Add a new WebAPK-specific app banner shortcut source.

Currently, WebAPKs and legacy PWA shortcuts added from app banners are
both recorded using the ShortcutSource::SOURCE_APP_BANNER bucket. This
means it is impossible to determine what shortcut launches from banners
are triggered for legacy PWAs compared to WebAPKs.

This CL adds a new ShortcutSource enum bucket for WebAPKs added to the
homescreen via an app banner. This means that WebAPKs installed from
banners have an explicitly different source, allowing them to be
differentiated in metrics.

The legacy SOURCE_APP_BANNER bucket is still used for non-WebAPK PWA
shortcuts, and will be reported for launches of legacy PWAs added prior
to M59.

BUG=691739

Review-Url: https://codereview.chromium.org/2808263004
Cr-Commit-Position: refs/heads/master@{#469576}
Committed:
https://chromium.googlesource.com/chromium/src/+/f197d9b5000564960a0b79babb80...
==========

[commit-bot: I haz the power, 2017-05-05 02:58:43]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/f197d9b5000564960a0b79babb80...