[Android] Make WebApplicationInfo and Web Manifest mutually exclusive

[Android] Make WebApplicationInfo and Web Manifest mutually exclusive

This CL makes add-to-homescreen use either data from WebApplicationInfo or from
the Web Manifest

BUG=733005
TEST=AddToHomescreenDataFetcherTestCommon.ManifestOverwritesWebApplicationInfo

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

Description was changed from

==========
[Android] Make WebApplicationInfo and Web Manifest mutually exclusive

This CL makes add-to-homescreen use either data from WebApplicationInfo or from
the Web Manifest

BUG=None
==========

to

==========
[Android] Make WebApplicationInfo and Web Manifest mutually exclusive

This CL makes add-to-homescreen use either data from WebApplicationInfo or from
the Web Manifest

BUG=733005
==========

[pkotwicz, 2017-06-13 23:00:36]

Description was changed from

==========
[Android] Make WebApplicationInfo and Web Manifest mutually exclusive

This CL makes add-to-homescreen use either data from WebApplicationInfo or from
the Web Manifest

BUG=733005
==========

to

==========
[Android] Make WebApplicationInfo and Web Manifest mutually exclusive

This CL makes add-to-homescreen use either data from WebApplicationInfo or from
the Web Manifest

BUG=733005
TEST=AddToHomescreenDataFetcherTestCommon
==========

[pkotwicz, 2017-06-13 23:01:02]

Description was changed from

==========
[Android] Make WebApplicationInfo and Web Manifest mutually exclusive

This CL makes add-to-homescreen use either data from WebApplicationInfo or from
the Web Manifest

BUG=733005
TEST=AddToHomescreenDataFetcherTestCommon
==========

to

==========
[Android] Make WebApplicationInfo and Web Manifest mutually exclusive

This CL makes add-to-homescreen use either data from WebApplicationInfo or from
the Web Manifest

BUG=733005
TEST=AddToHomescreenDataFetcherTestCommon.ManifestOverwritesWebApplicationInfo
==========

[pkotwicz, 2017-06-13 23:06:47]

Patchset #1 (id:1) has been deleted

[pkotwicz, 2017-06-13 23:08:36]

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

[pkotwicz, 2017-06-13 23:08:37]

Dominick, can you please take a look? This CL addresses the second part of
comment #11 in https://codereview.chromium.org/2915913002/

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-13 23:58:35]

Patchset #2 (id:40001) has been deleted

[dominickn, 2017-06-14 05:16:51]

https://codereview.chromium.org/2937823002/diff/60001/chrome/browser/android/...
File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc (left):

https://codereview.chromium.org/2937823002/diff/60001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:225:
shortcut_info_.UpdateFromManifest(data.manifest);
This isn't quite what I was suggesting on the other CL. What I was suggesting
was adding an if statement right before this line:

if (!data.manifest.name.string().empty() ||
!data.manifest.short_name.string().empty()) {
  shortcut_info_.UpdateFromManifest(data.manifest);
}

The expected behaviour is that a site which specifies a manifest but has no name
or short name shows the <title> in the A2HS dialog. We do this by simply not
updating from the manifest if they haven't bothered to specify a name or
short_name. If they want a site that's standalone they fill in one of those two
fields.

The change you've made means that sites with a manifest with empty name or
short_name end up with an empty title in the A2HS dialog.

https://codereview.chromium.org/2937823002/diff/60001/chrome/browser/android/...
File chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc
(left):

https://codereview.chromium.org/2937823002/diff/60001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:321:
// - WebApplicationInfo::title is used as the "name".
This behaviour should be preserved.

[pkotwicz, 2017-06-15 04:22:18]

Patchset #3 (id:80001) has been deleted

[pkotwicz, 2017-06-15 04:24:31]

Dominick, can you please take another look?

Is this what you had in mind? In my new patch set, the add-to-homescreen logic
uses the Web Manifest only if either the name or the short_name is non-empty. In
particular, any icons in the Web Manifest are ignored unless either the name or
short_name is non-empty. I will update the unit tests once you are happy with
the code change

[dominickn, 2017-06-15 05:16:23]

I think all we need is the if statement that I previously suggested, not these
additional changes....

https://codereview.chromium.org/2937823002/diff/100001/chrome/browser/android...
File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc (right):

https://codereview.chromium.org/2937823002/diff/100001/chrome/browser/android...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:221: if
(data.badge_icon && !data.badge_icon->drawsNothing()) {
We only want to grab the badge icon in the case of a WebAPK.

https://codereview.chromium.org/2937823002/diff/100001/chrome/browser/android...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:248: if
(use_manifest && data.primary_icon) {
I don't think we want to guard this behind use_manifest, particularly since
we're trying to use manifest icons more.

[pkotwicz, 2017-06-21 21:28:51]

Abandoning this CL because https://codereview.chromium.org/2915913002/ is a
better solution