Make minimum PWA icon size the same accross all device densities

Make minimum PWA icon size the same accross all device densities

This CL:
- Makes the minimum Web Manifest icon size for a web page to be considered a
PWA 144px regardless of the device screen density. This makes Chrome's decision
for whether a page is PWAable the same accross all devices.
- Simplifies the logic in InstallableManager by removing the "minimum size"
parameter of the InstallabeManager::IconParams tuple.

BUG=697205

[pkotwicz, 2017-03-26 22:08:08]

Description was changed from

==========
Make minimum PWA icon size the same accross all device densities

This CL:
- Makes the minimum Web Manifest icon size for a web page to be considered a PWA
144px regardless of the device screen density. This makes Chrome's decision for
whether a page is PWAable the same accross all devices.
- Simplifies the logic in InstallableManager by removing the "minimum size"
parameter of the InstallabeManager::IconParams tuple.

BUG=697205
==========

to

==========
Make minimum PWA icon size the same accross all device densities

This CL:
- Makes the minimum Web Manifest icon size for a web page to be considered a
PWA 144px regardless of the device screen density. This makes Chrome's decision
for whether a page is PWAable the same accross all devices.
- Simplifies the logic in InstallableManager by removing the "minimum size"
parameter of the InstallabeManager::IconParams tuple.

BUG=697205
==========

[pkotwicz, 2017-03-26 22:08:15]

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

[commit-bot: I haz the power, 2017-03-26 22:08:27]

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

[pkotwicz, 2017-03-29 01:18:44]

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

[commit-bot: I haz the power, 2017-03-29 01:19:46]

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-03-29 02:51:22]

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

[commit-bot: I haz the power, 2017-03-29 02:51:22]

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

[pkotwicz, 2017-04-01 01:39:14]

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

[commit-bot: I haz the power, 2017-04-01 01:39:34]

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

[pkotwicz, 2017-04-03 15:36:01]

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

[pkotwicz, 2017-04-03 15:36:01]

Xi, can you please take a look?

[pkotwicz, 2017-04-03 15:36:08]

Patchset #1 (id:1) has been deleted

[pkotwicz, 2017-04-03 15:36:16]

Patchset #1 (id:20001) has been deleted

[pkotwicz, 2017-04-03 15:36:23]

Patchset #1 (id:40001) has been deleted

[pkotwicz, 2017-04-03 15:36:34]

Patchset #1 (id:60001) has been deleted

[Xi Han, 2017-04-03 18:24:58]

This CL looks great!

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

https://codereview.chromium.org/2773353002/diff/80001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:210:
return new AddToHomescreenDataFetcher(web_contents(), 144, 144, 24,
May I know why to change the third parameter? Is the "24" comes from some spec?

https://codereview.chromium.org/2773353002/diff/80001/chrome/browser/installa...
File chrome/browser/installable/installable_manager.cc (right):

https://codereview.chromium.org/2773353002/diff/80001/chrome/browser/installa...
chrome/browser/installable/installable_manager.cc:440: int
minimum_icon_size_in_px = (icon_purpose == IconPurpose::BADGE)
If the |icon_purpose| == IconPurpose::BADGE, I think we should check whether
|ideal_icon_size_in_px| >= |kIconMinimumSizeInPx| and picks up the bigger one.
Is this already checked in any other place?

[pkotwicz, 2017-04-05 04:29:07]

Xi can you please take another look?

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

https://codereview.chromium.org/2773353002/diff/80001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:210:
return new AddToHomescreenDataFetcher(web_contents(), 144, 144, 24,
Reverted 24 to 1

https://codereview.chromium.org/2773353002/diff/80001/chrome/browser/installa...
File chrome/browser/installable/installable_manager.cc (right):

https://codereview.chromium.org/2773353002/diff/80001/chrome/browser/installa...
chrome/browser/installable/installable_manager.cc:440: int
minimum_icon_size_in_px = (icon_purpose == IconPurpose::BADGE)
kIconMinimumSizeInPx is 144px. Badges are smaller than that :)

[pkotwicz, 2017-04-05 14:58:59]

Patchset #3 (id:120001) has been deleted

[pkotwicz, 2017-04-05 14:59:34]

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

https://codereview.chromium.org/2773353002/diff/80001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:210:
return new AddToHomescreenDataFetcher(web_contents(), 144, 144, 24,
Done for real now

[Xi Han, 2017-04-05 15:19:27]

lgtm

https://codereview.chromium.org/2773353002/diff/80001/chrome/browser/installa...
File chrome/browser/installable/installable_manager.cc (right):

https://codereview.chromium.org/2773353002/diff/80001/chrome/browser/installa...
chrome/browser/installable/installable_manager.cc:440: int
minimum_icon_size_in_px = (icon_purpose == IconPurpose::BADGE)
On 2017/04/05 04:29:07, pkotwicz wrote:
> kIconMinimumSizeInPx is 144px. Badges are smaller than that :)

I see, thanks!

[pkotwicz, 2017-04-05 15:23:35]

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

[pkotwicz, 2017-04-05 15:23:35]

Dominick for OWNERS

[dominickn, 2017-04-06 05:30:26]

This cleans up a lot of code, but I think it's against Android best practices to
define icon sizes in pixels rather than dp. There is exactly one image-related
size in dimens.xml that's in px rather than dp (and that's a 16px minimum size
for favicons).

Also, it looks like you've applied the constant to a value which only
splashscreens use.

The hardcoded 144px size is a historical artifact (see crbug.com/457399) that I
would like to remove at some point. So I really don't want to see it codified in
dimens.xml

https://codereview.chromium.org/2773353002/diff/140001/chrome/android/java/re...
File chrome/android/java/res/values/dimens.xml (right):

https://codereview.chromium.org/2773353002/diff/140001/chrome/android/java/re...
chrome/android/java/res/values/dimens.xml:229: <dimen
name="webapp_splash_image_size_minimum">144px</dimen>
This size is only used for splash screens. You haven't changed
webapp_home_screen_icon_size, which is what is actually used for launcher icons.
So the minimum PWA icon size remains device-density dependent.

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

There's a push to make "Is this site WebAPK-able eligible" easier to understand.
Having a "device pixel" minimum size causes the answer to be different on
different devices. For instance, if a Web Manifest has a 144px icon their site
is WebApkable on a Nexus 5 (xxhdpi) but not on a Nexus 6P (xxxhdpi).

[dominickn, 2017-04-07 00:43:54]

I'm aware of this push, and I fully support it. However, it is a reality of
native development that you need to support multiple icon sizes to suit multiple
devices. Having a flat pixel-based cutoff is the wrong move going forward,
especially because launcher icons aren't fixed in size across different
densities.

My preferred solution is something like:

 * remove the 144px cutoff
 * use the minimum size passed into InstallableManager to do the manifest level
check (instead of 144)
 * have Lighthouse be able to check the icons for common devices and their
required densities.

Improving WebAPK eligibility predictability should be done through Lighthouse,
and then ensuring that any site that passes Lighthouse will pass the client
check.