[WebAPKs] Display same text for menu & engagement banner

[WebAPKs] Display same text for menu & engagement banner

This CL changes the text of the banner displayed when installing a WebAPK from
the app menu to match the text of the banner displayed via the engagement
checks.

Using the same text in both banners prevents creating a second banner if a user
tries to install a WebAPK from the app menu if the "user engagement A2HS"
banner is already visible.

BUG=728300

[pkotwicz, 2017-05-31 21:27:13]

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

[pkotwicz, 2017-05-31 21:27:14]

Dominick, can you please take a look?

[dominickn, 2017-06-01 01:12:04]

It looks to me like ShortcutInfo::UpdateFromManifest() sets user_title the
reverse of what the app banner code does: defaulting to short_name if it is
present, and only using name if short_name isn't there.

I think it makes more sense to just reverse the logic in UpdateFromManifest() to
match that used in app banners. The other thing that UpdateFromManifest()
doesn't do it check if the string() is empty as opposed to non-null. This means
you could set short_name: "" and end up with a WebAPK banner with an empty title
(we had this issue recently with app banners for m.forbes.com I think).

WDYT?

[pkotwicz, 2017-06-01 05:28:16]

Unfortunately AddToHomescreenManager::OnUserTitleAvailable() uses
ShortcutInfo::user_title so I cannot do what you suggest

I have updated ShortcutInfo::UpdateFromManifest() as you have suggested.

My change to ShortcutInfo::UpdateFromManifest() changes the behavior when the
manifest
- is non empty
- has an empty name
- has an empty short name
However, based on the callers to ShortcutInfo::UpdateFromManifest() I think that
the change is OK

[pkotwicz, 2017-06-01 05:28:18]

Unfortunately AddToHomescreenManager::OnUserTitleAvailable() uses
ShortcutInfo::user_title so I cannot do what you suggest

I have updated ShortcutInfo::UpdateFromManifest() as you have suggested.

My change to ShortcutInfo::UpdateFromManifest() changes the behavior when the
manifest
- is non empty
- has an empty name
- has an empty short name
However, based on the callers to ShortcutInfo::UpdateFromManifest() I think that
the change is OK

[dominickn, 2017-06-02 01:42:25]

lgtm % nit

https://codereview.chromium.org/2915913002/diff/20001/chrome/browser/android/...
File chrome/browser/android/shortcut_info.cc (right):

https://codereview.chromium.org/2915913002/diff/20001/chrome/browser/android/...
chrome/browser/android/shortcut_info.cc:22: void
ShortcutInfo::UpdateFromManifest(const content::Manifest& manifest) {
Please add:

DCHECK(!short_name.empty() || !name.empty());

[pkotwicz, 2017-06-02 15:53:53]

https://codereview.chromium.org/2915913002/diff/20001/chrome/browser/android/...
File chrome/browser/android/shortcut_info.cc (right):

https://codereview.chromium.org/2915913002/diff/20001/chrome/browser/android/...
chrome/browser/android/shortcut_info.cc:22: void
ShortcutInfo::UpdateFromManifest(const content::Manifest& manifest) {
UpdateManifest() might be called with an empty short_name and an empty name in
AddToHomescreenDataFetcher::OnDidPerformInstallableCheck()

[pkotwicz, 2017-06-05 15:01:49]

Dominick, ping on comment #7

[dominickn, 2017-06-06 00:20:58]

Sorry about the delay here. I've had some hardware issues the last few days so
I've been really slow. Should be back to full speed now.

https://codereview.chromium.org/2915913002/diff/20001/chrome/browser/android/...
File chrome/browser/android/shortcut_info.cc (right):

https://codereview.chromium.org/2915913002/diff/20001/chrome/browser/android/...
chrome/browser/android/shortcut_info.cc:22: void
ShortcutInfo::UpdateFromManifest(const content::Manifest& manifest) {
On 2017/06/02 15:53:53, pkotwicz wrote:
> UpdateManifest() might be called with an empty short_name and an empty name in
> AddToHomescreenDataFetcher::OnDidPerformInstallableCheck()

Good catch. That's not great - it means we'll show the add to homescreen dialog
with an empty title if name = "" and short_name = "" (and I think this might
have done the same before this patch).

Ideally, we'd have one piece of code that works out the best title to use. That
one location should be in InstallableManager because it's already doing that
work. What do you think about returning the title as another member of the
InstallableData struct? Then all of this code could just use that title and we
wouldn't have the title selection logic duplicated everywhere. If you're happy
with that solution I can put up a patch for it.

Otherwise, the simpler stop gap is to guard the call in
AddToHomescreenDataFetcher() with an if check on the short_name and name (which
is a bit yuck and adds more duplication).

[pkotwicz, 2017-06-07 02:22:33]

https://codereview.chromium.org/2915913002/diff/20001/chrome/browser/android/...
File chrome/browser/android/shortcut_info.cc (right):

https://codereview.chromium.org/2915913002/diff/20001/chrome/browser/android/...
chrome/browser/android/shortcut_info.cc:22: void
ShortcutInfo::UpdateFromManifest(const content::Manifest& manifest) {
Ok, I have changed the logic to be a no-op if the manifest's short_name and name
are empty. I have added a unit test
(ShortcutInfoTest::IgnoreEmptyNameAndShortName)

I don't think that changing the logic in InstallableManager makes sense because
AddToHomescreenDataFetcher::OnDidGetWebApplicationInfo() is unique to the
app-menu code path.

Guarding the calling to ShortcutInfo::UpdateFromManifest()  will break creating
a full screen PWA for a page:
- with a Web Manifest with display="standalone" and empty name and short_name
- currently the name and short_name are picked up from the page's <title> tag

[dominickn, 2017-06-07 03:15:39]

On 2017/06/07 02:22:33, pkotwicz wrote:
>
https://codereview.chromium.org/2915913002/diff/20001/chrome/browser/android/...
> File chrome/browser/android/shortcut_info.cc (right):
> 
>
https://codereview.chromium.org/2915913002/diff/20001/chrome/browser/android/...
> chrome/browser/android/shortcut_info.cc:22: void
> ShortcutInfo::UpdateFromManifest(const content::Manifest& manifest) {
> Ok, I have changed the logic to be a no-op if the manifest's short_name and
name
> are empty. I have added a unit test
> (ShortcutInfoTest::IgnoreEmptyNameAndShortName)
> 
> I don't think that changing the logic in InstallableManager makes sense
because
> AddToHomescreenDataFetcher::OnDidGetWebApplicationInfo() is unique to the
> app-menu code path.

This isn't really changing the logic in InstallableManager. It's already
figuring out which one of name / short_name isn't empty and should be used. The
change would be to explicitly send the correct one back in InstallableData.

> 
> Guarding the calling to ShortcutInfo::UpdateFromManifest()  will break
creating
> a full screen PWA for a page:
> - with a Web Manifest with display="standalone" and empty name and short_name
> - currently the name and short_name are picked up from the page's <title> tag

Yes, but I don't really think we need to support sites that have gone to the
trouble of setting a manifest with an explicitly empty name and short name as
well as display standalone. Having either one of them non-empty is an instant
fix.

[pkotwicz, 2017-06-16 01:17:42]

Patchset #5 (id:80001) has been deleted

[pkotwicz, 2017-06-16 01:20:40]

Dominick, can you please take another look? I have deleted the controversial
ShortcutInfo::user_title field. The field was only useful in
AddToHomescreenManager::AddShortcut(). I have added an extra parameter to the
relevant ShortcutHelper methods and have switched all of the other uses of
ShortcutInfo::user_title to use ShortcutInfo::short_name instead

[dominickn, 2017-06-16 01:32:32]

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

https://codereview.chromium.org/2915913002/diff/100001/chrome/browser/android...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:378:
shortcut_info_->short_name,
This is a behaviour change for app banners. Previously, app banners would prefer
|name| over |short_name| if both were present (see the title setting logic in
app_banner_manager.cc). Now it will prefer |short_name| if both are present. In
fact, our documentation explicitly calls out the |name| field as being used in
the banner itself, while |short_name| is used on the homescreen, so we need to
preserve this default behaviour
(https://developers.google.com/web/fundamentals/engage-and-retain/app-install-...)

I'll reiterate the first comment I made: 
I think it makes more sense to just reverse the logic in UpdateFromManifest() to
match that used in app banners. The other thing that UpdateFromManifest()
doesn't do it check if the string() is empty as opposed to non-null. This means
you could set short_name: "" and end up with a WebAPK banner with an empty title
(we had this issue recently with app banners for m.forbes.com I think).

[dominickn, 2017-06-16 01:38:10]

In other words: you should pick |name| over |short_name|, and set |name| =
|short_name| only in the case where |name| is empty string or not provided.

[pkotwicz, 2017-06-17 00:17:23]

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

https://codereview.chromium.org/2915913002/diff/100001/chrome/browser/android...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:378:
shortcut_info_->short_name,
Thank you for your question.

This is actually incorrect. The banner title and the name of the installed web
app used to come from two different places:
The banner title used to come from AppBannerManager::app_title_
The name of the install web app used to come from CreateShortcutInfo() in
app_banner_manager_android.cc

If I changed the logic in ShortcutInfo::UpdateFromManifest() to match the banner
code, this would change home screen shortcuts to use the "name" instead of the
"short_name". This would be a change in behavior for both home screen shortcuts
added from the app menu and home screen shortcuts added from the banner. As far
as I can tell, the "short_name" has been used for home screen shortcuts for at
least a year.

[dominickn, 2017-06-17 01:05:45]

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

https://codereview.chromium.org/2915913002/diff/100001/chrome/browser/android...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:378:
shortcut_info_->short_name,
On 2017/06/17 00:17:23, pkotwicz wrote:
> Thank you for your question.
> 
> This is actually incorrect. The banner title and the name of the installed web
> app used to come from two different places:
> The banner title used to come from AppBannerManager::app_title_
> The name of the install web app used to come from CreateShortcutInfo() in
> app_banner_manager_android.cc
> 
> If I changed the logic in ShortcutInfo::UpdateFromManifest() to match the
banner
> code, this would change home screen shortcuts to use the "name" instead of the
> "short_name". This would be a change in behavior for both home screen
shortcuts
> added from the app menu and home screen shortcuts added from the banner. As
far
> as I can tell, the "short_name" has been used for home screen shortcuts for at
> least a year.

Yes, we should still use the |short_name| for the home screen shortcut, so my
proposed solution there would break that.

The behaviour we want is to use |name| for the banner title if it's present,
rather than |short_name|. This means devs can use a longer, full name in the
banner and have the truncated one be in the actual shortcut if the real name is
too long. In short:

A2HS Dialog:
  - if not WebAPK: prefer short_name (actual text that will go in the shortcut)
  - if WebAPK: prefer name in the banner, but use short_name for shortcut

Banner:
 - prefer name in the banner, but use short_name for shortcut

[pkotwicz, 2017-06-29 19:41:45]

Description was changed from

==========
[WebAPKs] Display same text for menu & engagement banner

This CL changes the text of the banner displayed when installing a WebAPK from
the app menu to match the text of the banner displayed via the engagement
checks.

Using the same text in both banners prevents creating a second banner if a user
tries to install a WebAPK from the app menu if the "user engagement A2HS"
banner is already visible.

BUG=728300
==========

to

==========
[WebAPKs] Display same text for menu & engagement banner

This CL changes the text of the banner displayed when installing a WebAPK from
the app menu to match the text of the banner displayed via the engagement
checks.

Using the same text in both banners prevents creating a second banner if a user
tries to install a WebAPK from the app menu if the "user engagement A2HS"
banner is already visible.

BUG=728300
==========