Use exact pixel sizes instead of dip in webapp/WebAPK installability code

Use exact pixel sizes instead of dip in webapp/WebAPK installability code

This CL:
- Makes the members of InstallableManager::InstallableParams use pixel units
  instead of device independent pixel units
- Removes unnecessary conversions between pixels and device independent pixel
  units

Note: android.content.res.Resources#getDimension() multiplies the device
independent pixel measurement in resources by the current device scale factor

BUG=675765

[pkotwicz, 2016-12-17 22:40:39]

Description was changed from

==========
Cleanup make InstallableManager::InstallableParams use px instead of dip members

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

to

==========
Cleanup make InstallableManager::InstallableParams use px instead of dip members

This CL:
- Makes the members of InstallableManager::InstallableParams use pixel units
  instead of device independant pixel units
- Removes unnecessary conversions between pixels and device independant pixel
  units

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

[pkotwicz, 2016-12-18 04:50:22]

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

[pkotwicz, 2016-12-18 04:50:23]

Dominick, can you please take a look?

[dominickn, 2016-12-19 06:23:33]

This is quite a nice clean up, thanks for looking at it. I've been trying to
think of reasons why we might want to keep dip internally, and haven't come up
with anything yet.

Minor nit: "independent" in the description.

Minor nit: please file a bug for this (something like "Clean up the icon size
handling for PWAs"), so it can also track whether we remove the 144px hard
constraint and when I/someone gets around to unifying the various minimum_size_
constants we have (also getLauncherLargeIconSize() is ShortcutHelper.java too)

https://codereview.chromium.org/2589503002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
(right):

https://codereview.chromium.org/2589503002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:619:
private static int getIdealSizeFromResourceInPx(Context context, int resource) {
Nit: rename this to getSizeFromResourceInPx (it's used for a minimum as well as
ideal).

https://codereview.chromium.org/2589503002/diff/1/chrome/browser/android/weba...
File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc (right):

https://codereview.chromium.org/2589503002/diff/1/chrome/browser/android/weba...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:32: #include
"ui/display/display.h"
Nit: remove display.h and screen.h includes.

https://codereview.chromium.org/2589503002/diff/1/chrome/browser/banners/app_...
File chrome/browser/banners/app_banner_manager.cc (right):

https://codereview.chromium.org/2589503002/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_manager.cc:28: #include "ui/display/display.h"
Nit: remove display.h and screen.h includes.

https://codereview.chromium.org/2589503002/diff/1/chrome/browser/installable/...
File chrome/browser/installable/installable_manager_browsertest.cc (right):

https://codereview.chromium.org/2589503002/diff/1/chrome/browser/installable/...
chrome/browser/installable/installable_manager_browsertest.cc:28:
params.ideal_icon_size_in_px = 48;
Change the 48 to 144 in all of these locations.

https://codereview.chromium.org/2589503002/diff/1/chrome/browser/installable/...
chrome/browser/installable/installable_manager_browsertest.cc:351:
params.ideal_icon_size_in_px = 32;
Change 32 to 96.

https://codereview.chromium.org/2589503002/diff/1/chrome/browser/manifest/man...
File chrome/browser/manifest/manifest_icon_downloader.cc (right):

https://codereview.chromium.org/2589503002/diff/1/chrome/browser/manifest/man...
chrome/browser/manifest/manifest_icon_downloader.cc:18: #include
"ui/display/display.h"
Nit: remove display.h and screen.h includes.

https://codereview.chromium.org/2589503002/diff/1/chrome/browser/manifest/man...
File chrome/browser/manifest/manifest_icon_selector.cc (right):

https://codereview.chromium.org/2589503002/diff/1/chrome/browser/manifest/man...
chrome/browser/manifest/manifest_icon_selector.cc:16: #include
"ui/display/display.h"
Nit: remove display.h and screen.h includes.

https://codereview.chromium.org/2589503002/diff/1/chrome/browser/manifest/man...
File chrome/browser/manifest/manifest_icon_selector_unittest.cc (right):

https://codereview.chromium.org/2589503002/diff/1/chrome/browser/manifest/man...
chrome/browser/manifest/manifest_icon_selector_unittest.cc:16: const int
kDefaultIconSize = 48;
Change to 144.

https://codereview.chromium.org/2589503002/diff/1/chrome/browser/manifest/man...
chrome/browser/manifest/manifest_icon_selector_unittest.cc:121: // has sizes
which matches the preferred size for each device scale factor.
Update the comment: "because it contains a preferred size."

https://codereview.chromium.org/2589503002/diff/1/chrome/browser/manifest/man...
chrome/browser/manifest/manifest_icon_selector_unittest.cc:148:
TEST(ManifestIconSelector, UseDeviceDensity) {
Call this "FallbackToSmallestLargerIcon"

https://codereview.chromium.org/2589503002/diff/1/chrome/browser/manifest/man...
chrome/browser/manifest/manifest_icon_selector_unittest.cc:174:
TEST(ManifestIconSelector, CheckDifferentDeviceScaleFactors) {
"FallbackToLargestIconLargerThanMinimum"

[dominickn, 2016-12-19 06:24:50]

Also, you might want to change the title to be "Use exact pixel sizes instead of
dip in installability code", with the note that getDimension() multiplies the
dip value in resources by the current device scale factor.

[pkotwicz, 2016-12-19 23:05:39]

Description was changed from

==========
Cleanup make InstallableManager::InstallableParams use px instead of dip members

This CL:
- Makes the members of InstallableManager::InstallableParams use pixel units
  instead of device independant pixel units
- Removes unnecessary conversions between pixels and device independant pixel
  units

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

to

==========
Use exact pixel sizes instead of dip in webapp/WebAPK installability code

This CL:
- Makes the members of InstallableManager::InstallableParams use pixel units
  instead of device independant pixel units
- Removes unnecessary conversions between pixels and device independant pixel
  units

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

[pkotwicz, 2016-12-19 23:08:20]

Description was changed from

==========
Use exact pixel sizes instead of dip in webapp/WebAPK installability code

This CL:
- Makes the members of InstallableManager::InstallableParams use pixel units
  instead of device independant pixel units
- Removes unnecessary conversions between pixels and device independant pixel
  units

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

to

==========
Use exact pixel sizes instead of dip in webapp/WebAPK installability code

This CL:
- Makes the members of InstallableManager::InstallableParams use pixel units
  instead of device independant pixel units
- Removes unnecessary conversions between pixels and device independant pixel
  units

Note: android.content.res.Resources#getDimension() returns dimensions in pixel
not device independant pixels

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

[pkotwicz, 2016-12-19 23:09:19]

Description was changed from

==========
Use exact pixel sizes instead of dip in webapp/WebAPK installability code

This CL:
- Makes the members of InstallableManager::InstallableParams use pixel units
  instead of device independant pixel units
- Removes unnecessary conversions between pixels and device independant pixel
  units

Note: android.content.res.Resources#getDimension() returns dimensions in pixel
not device independant pixels

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

to

==========
Use exact pixel sizes instead of dip in webapp/WebAPK installability code

This CL:
- Makes the members of InstallableManager::InstallableParams use pixel units
  instead of device independant pixel units
- Removes unnecessary conversions between pixels and device independant pixel
  units

Note: android.content.res.Resources#getDimension() multiplies the device
independent pixel measurement in resources by the current device scale factor

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

[pkotwicz, 2016-12-19 23:09:52]

Description was changed from

==========
Use exact pixel sizes instead of dip in webapp/WebAPK installability code

This CL:
- Makes the members of InstallableManager::InstallableParams use pixel units
  instead of device independant pixel units
- Removes unnecessary conversions between pixels and device independant pixel
  units

Note: android.content.res.Resources#getDimension() multiplies the device
independent pixel measurement in resources by the current device scale factor

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

to

==========
Use exact pixel sizes instead of dip in webapp/WebAPK installability code

This CL:
- Makes the members of InstallableManager::InstallableParams use pixel units
  instead of device independent pixel units
- Removes unnecessary conversions between pixels and device independent pixel
  units

Note: android.content.res.Resources#getDimension() multiplies the device
independent pixel measurement in resources by the current device scale factor

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

[pkotwicz, 2016-12-19 23:16:51]

Description was changed from

==========
Use exact pixel sizes instead of dip in webapp/WebAPK installability code

This CL:
- Makes the members of InstallableManager::InstallableParams use pixel units
  instead of device independent pixel units
- Removes unnecessary conversions between pixels and device independent pixel
  units

Note: android.content.res.Resources#getDimension() multiplies the device
independent pixel measurement in resources by the current device scale factor

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

to

==========
Use exact pixel sizes instead of dip in webapp/WebAPK installability code

This CL:
- Makes the members of InstallableManager::InstallableParams use pixel units
  instead of device independent pixel units
- Removes unnecessary conversions between pixels and device independent pixel
  units

Note: android.content.res.Resources#getDimension() multiplies the device
independent pixel measurement in resources by the current device scale factor

BUG=675765
==========

[pkotwicz, 2016-12-19 23:51:31]

Patchset #2 (id:20001) has been deleted

[pkotwicz, 2016-12-19 23:51:58]

Dominick, can you please take another look?

[dominickn, 2016-12-20 00:00:09]

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

[dominickn, 2016-12-20 00:00:10]

+mlamouri for a sanity check. I couldn't think of any reason to use dip values
internally; it seems very reasonable just to read the dip size once from the
resources file, and then use the px value everywhere. If you're okay with that,
I'll lg.

Also: in the future, can you please upload a rebase separately? It makes
reviewing the version-to-version diff easier.

https://codereview.chromium.org/2589503002/diff/40001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_manager.cc (right):

https://codereview.chromium.org/2589503002/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.cc:30: #include "ui/display/display.h"
Remove display.h and screen.h includes.

[pkotwicz, 2016-12-20 00:28:14]

Thanks, I'll upload rebases as separate patch sets in the future.

https://codereview.chromium.org/2589503002/diff/40001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_manager.cc (right):

https://codereview.chromium.org/2589503002/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.cc:30: #include "ui/display/display.h"
On 2016/12/20 00:00:10, dominickn wrote:
> Remove display.h and screen.h includes.

Done.

[mlamouri (slow - plz ping), 2016-12-23 12:07:05]

lgtm

[dominickn, 2017-01-03 01:44:22]

lgtm

[F, 2017-01-03 20:06:37]

The CQ bit was checked by zpeng@chromium.org

[commit-bot: I haz the power, 2017-01-03 20:07:01]

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-01-03 20:54:53]

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

[commit-bot: I haz the power, 2017-01-03 20:54:54]

Failed to apply patch for
chrome/browser/android/webapk/webapk_update_data_fetcher.cc:
While running git apply --index -p1;
  error: patch failed:
chrome/browser/android/webapk/webapk_update_data_fetcher.cc:108
  error: chrome/browser/android/webapk/webapk_update_data_fetcher.cc: patch does
not apply

Patch:       chrome/browser/android/webapk/webapk_update_data_fetcher.cc
Index: chrome/browser/android/webapk/webapk_update_data_fetcher.cc
diff --git a/chrome/browser/android/webapk/webapk_update_data_fetcher.cc
b/chrome/browser/android/webapk/webapk_update_data_fetcher.cc
index
a5a20efebe050d58bcb9d3074a7fff60b8fd0524..53a944e6afaae8181b5262efa0eb85924cd964e5
100644
--- a/chrome/browser/android/webapk/webapk_update_data_fetcher.cc
+++ b/chrome/browser/android/webapk/webapk_update_data_fetcher.cc
@@ -108,10 +108,10 @@ void WebApkUpdateDataFetcher::FetchInstallableData() {
     return;
 
   InstallableParams params;
-  params.ideal_icon_size_in_dp =
-      ShortcutHelper::GetIdealHomescreenIconSizeInDp();
-  params.minimum_icon_size_in_dp =
-      ShortcutHelper::GetMinimumHomescreenIconSizeInDp();
+  params.ideal_icon_size_in_px =
+      ShortcutHelper::GetIdealHomescreenIconSizeInPx();
+  params.minimum_icon_size_in_px =
+      ShortcutHelper::GetMinimumHomescreenIconSizeInPx();
   params.check_installable = true;
   params.fetch_valid_icon = true;
   InstallableManager::CreateForWebContents(web_contents());

[F, 2017-01-03 22:18:28]

The CQ bit was checked by zpeng@chromium.org

[commit-bot: I haz the power, 2017-01-03 22:19:08]

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-01-03 22:24:24]

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

[commit-bot: I haz the power, 2017-01-03 22:24:25]

Failed to apply patch for
chrome/browser/android/webapk/webapk_update_data_fetcher.cc:
While running git apply --index -p1;
  error: patch failed:
chrome/browser/android/webapk/webapk_update_data_fetcher.cc:108
  error: chrome/browser/android/webapk/webapk_update_data_fetcher.cc: patch does
not apply

Patch:       chrome/browser/android/webapk/webapk_update_data_fetcher.cc
Index: chrome/browser/android/webapk/webapk_update_data_fetcher.cc
diff --git a/chrome/browser/android/webapk/webapk_update_data_fetcher.cc
b/chrome/browser/android/webapk/webapk_update_data_fetcher.cc
index
a5a20efebe050d58bcb9d3074a7fff60b8fd0524..53a944e6afaae8181b5262efa0eb85924cd964e5
100644
--- a/chrome/browser/android/webapk/webapk_update_data_fetcher.cc
+++ b/chrome/browser/android/webapk/webapk_update_data_fetcher.cc
@@ -108,10 +108,10 @@ void WebApkUpdateDataFetcher::FetchInstallableData() {
     return;
 
   InstallableParams params;
-  params.ideal_icon_size_in_dp =
-      ShortcutHelper::GetIdealHomescreenIconSizeInDp();
-  params.minimum_icon_size_in_dp =
-      ShortcutHelper::GetMinimumHomescreenIconSizeInDp();
+  params.ideal_icon_size_in_px =
+      ShortcutHelper::GetIdealHomescreenIconSizeInPx();
+  params.minimum_icon_size_in_px =
+      ShortcutHelper::GetMinimumHomescreenIconSizeInPx();
   params.check_installable = true;
   params.fetch_valid_icon = true;
   InstallableManager::CreateForWebContents(web_contents());

[dominickn, 2017-01-04 05:04:43]

On 2017/01/03 22:24:25, commit-bot: I haz the power wrote:
> Failed to apply patch for
> chrome/browser/android/webapk/webapk_update_data_fetcher.cc:
> While running git apply --index -p1;
>   error: patch failed:
> chrome/browser/android/webapk/webapk_update_data_fetcher.cc:108
>   error: chrome/browser/android/webapk/webapk_update_data_fetcher.cc: patch
does
> not apply
> 
> Patch:       chrome/browser/android/webapk/webapk_update_data_fetcher.cc
> Index: chrome/browser/android/webapk/webapk_update_data_fetcher.cc
> diff --git a/chrome/browser/android/webapk/webapk_update_data_fetcher.cc
> b/chrome/browser/android/webapk/webapk_update_data_fetcher.cc
> index
>
a5a20efebe050d58bcb9d3074a7fff60b8fd0524..53a944e6afaae8181b5262efa0eb85924cd964e5
> 100644
> --- a/chrome/browser/android/webapk/webapk_update_data_fetcher.cc
> +++ b/chrome/browser/android/webapk/webapk_update_data_fetcher.cc
> @@ -108,10 +108,10 @@ void WebApkUpdateDataFetcher::FetchInstallableData() {
>      return;
>  
>    InstallableParams params;
> -  params.ideal_icon_size_in_dp =
> -      ShortcutHelper::GetIdealHomescreenIconSizeInDp();
> -  params.minimum_icon_size_in_dp =
> -      ShortcutHelper::GetMinimumHomescreenIconSizeInDp();
> +  params.ideal_icon_size_in_px =
> +      ShortcutHelper::GetIdealHomescreenIconSizeInPx();
> +  params.minimum_icon_size_in_px =
> +      ShortcutHelper::GetMinimumHomescreenIconSizeInPx();
>    params.check_installable = true;
>    params.fetch_valid_icon = true;
>    InstallableManager::CreateForWebContents(web_contents());

You need to rebase this patch on master - it's failing because there is now a }
at the stop of this patch snippet on HEAD.