DevTools: account for the display scale factor while validating manifest under emulation.

DevTools: account for the display scale factor while validating manifest under emulation.

BUG=609580
Committed: https://crrev.com/7a69caa094b972461ad889e1684697b7c171677d
Cr-Commit-Position: refs/heads/master@{#392268}

[pfeldman, 2016-05-05 22:25:10]

pfeldman@chromium.org changed reviewers:
+ dfalcantara@chromium.org, dominickn@chromium.org

[dominickn, 2016-05-05 22:45:11]

This still isn't emulating correctly, since it's picking an arbitrary icon size,
and
not the icon size that the emulated device would actually require (see
AppBannerManagerAndroid::CreateAppBannerDataFetcher for the actual
values passed in). That means there will be an inconsistency between the
emulated banner requirements and the banner requirements when testing on
an actual device using chrome://inspect.

Frustratingly, the actual icon sizes are only available in the Android build
since
they're read from Android resources code. Are only specific devices/scale
factors
allowed to be emulated? If so, maybe we could hard code the required dimensions
so that the required icon size is actually correct....

[pfeldman, 2016-05-06 07:18:26]

Updated with the comment as agreed offline.

[pfeldman, 2016-05-06 07:20:26]

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

[commit-bot: I haz the power, 2016-05-06 07:20:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1950133009/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1950133009/40001

[commit-bot: I haz the power, 2016-05-06 07:54:23]

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

[commit-bot: I haz the power, 2016-05-06 07:54:24]

Dry run: This issue passed the CQ dry run.

[pfeldman, 2016-05-06 17:40:00]

ping

[dominickn, 2016-05-06 18:29:47]

Mostly nits.

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

https://codereview.chromium.org/1950133009/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager_emulation.cc:17: // We make it clear
in the 'add to home screen' emulation UI that the
Nit: It would be nice if it was also stated in the emulation UI that this won't
properly emulate a native app banner, just a web app banner (since the code to
query the Play Store for native data is Android-only)

https://codereview.chromium.org/1950133009/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager_emulation.cc:20: int kMinimumIconSize
= 192;
I think this should be 144px - that has been the advertised minimum size for
some time; for a Nexus 5X, I believe we have 48dp (homescreen icon size) * 3
(scale factor) = 144px.

Minor nit: const int?

https://codereview.chromium.org/1950133009/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager_emulation.cc:31: 
Nit: add a comment along the lines of "Divide by the current screen density as
the data fetcher will multiply it back in when trying to fetch an appropriate
icon".

https://codereview.chromium.org/1950133009/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager_emulation.cc:36: size, size, true);
Nit: pass through is_debug_mode for consistency.

https://codereview.chromium.org/1950133009/diff/40001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/1950133009/diff/40001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1458: bool
Browser::RequestAppBanner(content::WebContents* web_contents) {
Nit: it looks like this method isn't called from anywhere anymore (except from
ChromeServiceWorkerTest). Can you update that test to call
RequestAppBannerFromDevtools() instead?

[pfeldman, 2016-05-06 18:40:02]

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

https://codereview.chromium.org/1950133009/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager_emulation.cc:17: // We make it clear
in the 'add to home screen' emulation UI that the
On 2016/05/06 18:29:47, dominickn wrote:
> Nit: It would be nice if it was also stated in the emulation UI that this
won't
> properly emulate a native app banner, just a web app banner (since the code to
> query the Play Store for native data is Android-only)

We'll get this messaging aligned with Jake.

https://codereview.chromium.org/1950133009/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager_emulation.cc:20: int kMinimumIconSize
= 192;
On 2016/05/06 18:29:47, dominickn wrote:
> I think this should be 144px - that has been the advertised minimum size for
> some time; for a Nexus 5X, I believe we have 48dp (homescreen icon size) * 3
> (scale factor) = 144px.
> 
> Minor nit: const int?

Done.

https://codereview.chromium.org/1950133009/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager_emulation.cc:31: 
On 2016/05/06 18:29:46, dominickn wrote:
> Nit: add a comment along the lines of "Divide by the current screen density as
> the data fetcher will multiply it back in when trying to fetch an appropriate
> icon".

Done.

https://codereview.chromium.org/1950133009/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager_emulation.cc:36: size, size, true);
On 2016/05/06 18:29:46, dominickn wrote:
> Nit: pass through is_debug_mode for consistency.

Done.

[dominickn, 2016-05-06 18:42:39]

lgtm % ping

https://codereview.chromium.org/1950133009/diff/40001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/1950133009/diff/40001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1458: bool
Browser::RequestAppBanner(content::WebContents* web_contents) {
On 2016/05/06 18:29:47, dominickn wrote:
> Nit: it looks like this method isn't called from anywhere anymore (except from
> ChromeServiceWorkerTest). Can you update that test to call
> RequestAppBannerFromDevtools() instead?

Ping?

[pfeldman, 2016-05-06 19:32:05]

https://codereview.chromium.org/1950133009/diff/40001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/1950133009/diff/40001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1458: bool
Browser::RequestAppBanner(content::WebContents* web_contents) {
On 2016/05/06 18:42:38, dominickn wrote:
> On 2016/05/06 18:29:47, dominickn wrote:
> > Nit: it looks like this method isn't called from anywhere anymore (except
from
> > ChromeServiceWorkerTest). Can you update that test to call
> > RequestAppBannerFromDevtools() instead?
> 
> Ping?

I did not realize that. Done.

[pfeldman, 2016-05-06 19:32:32]

Description was changed from

==========
DevTools: account for the display scale factor while validating manifest under
emulation.

BUG=609580
==========

to

==========
DevTools: account for the display scale factor while validating manifest under
emulation.

BUG=609580
==========

[pfeldman, 2016-05-06 19:32:32]

pfeldman@chromium.org changed reviewers:
+ sky@chromium.org

[pfeldman, 2016-05-06 19:32:42]

+sky for OWNERS

[pfeldman, 2016-05-06 19:32:47]

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

[commit-bot: I haz the power, 2016-05-06 19:33:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1950133009/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1950133009/80001

[commit-bot: I haz the power, 2016-05-06 20:18:36]

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

[commit-bot: I haz the power, 2016-05-06 20:18:37]

Dry run: This issue passed the CQ dry run.

[sky, 2016-05-06 21:48:12]

https://codereview.chromium.org/1950133009/diff/80001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/1950133009/diff/80001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1451:
banners::AppBannerManagerEmulation::CreateForWebContents(web_contents);
Why does this code need to be in Browser? AFAICT it doesn't use any state from
browser and could just as well be in the one place that uses it.

If you envision needing it more places, move it into its own file.

[pfeldman, 2016-05-06 22:04:17]

Description was changed from

==========
DevTools: account for the display scale factor while validating manifest under
emulation.

BUG=609580
==========

to

==========
DevTools: account for the display scale factor while validating manifest under
emulation.

BUG=609580
TBR=sky
==========

[pfeldman, 2016-05-06 22:04:23]

The CQ bit was checked by pfeldman@chromium.org

[pfeldman, 2016-05-06 22:04:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org
Link to the patchset: https://codereview.chromium.org/1950133009/#ps80001
(title: "comment addressed")

[commit-bot: I haz the power, 2016-05-06 22:04:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1950133009/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1950133009/80001

[commit-bot: I haz the power, 2016-05-06 22:09:11]

Description was changed from

==========
DevTools: account for the display scale factor while validating manifest under
emulation.

BUG=609580
TBR=sky
==========

to

==========
DevTools: account for the display scale factor while validating manifest under
emulation.

BUG=609580
TBR=sky
==========

[commit-bot: I haz the power, 2016-05-06 22:09:12]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-06 22:10:50]

Description was changed from

==========
DevTools: account for the display scale factor while validating manifest under
emulation.

BUG=609580
TBR=sky
==========

to

==========
DevTools: account for the display scale factor while validating manifest under
emulation.

BUG=609580
TBR=sky

Committed: https://crrev.com/aeb778c817766d5c3b17097556218b0bafdbd1d4
Cr-Commit-Position: refs/heads/master@{#392171}
==========

[commit-bot: I haz the power, 2016-05-06 22:10:51]

Patchset 5 (id:??) landed as
https://crrev.com/aeb778c817766d5c3b17097556218b0bafdbd1d4
Cr-Commit-Position: refs/heads/master@{#392171}

[sky, 2016-05-06 23:38:00]

On 2016/05/06 22:10:51, commit-bot: I haz the power wrote:
> Patchset 5 (id:??) landed as
> https://crrev.com/aeb778c817766d5c3b17097556218b0bafdbd1d4
> Cr-Commit-Position: refs/heads/master@{#392171}

Why did you TBR this patch?

I'm reverting as I had concerns as mentioned above. I'm ok with TBR for trivial
changes, such as moving files, but this wasn't that.

[sky, 2016-05-06 23:38:22]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/1955183002/ by sky@chromium.org.

The reason for reverting is: For reasons outlined in latest comment..

[pfeldman, 2016-05-07 00:02:07]

On 2016/05/06 23:38:22, sky wrote:
> A revert of this CL (patchset #5 id:80001) has been created in
> https://codereview.chromium.org/1955183002/ by mailto:sky@chromium.org.
> 
> The reason for reverting is: For reasons outlined in latest comment..

Sorry about that - somehow I missed your comment and went for tbr.

I'm not sure I can remove that code from the browser since it is how we get from
content/ to the chrome/ here. This method overrides
WebContents::RequestAppBannerFromDevTools.

[pfeldman, 2016-05-07 00:09:44]

Description was changed from

==========
DevTools: account for the display scale factor while validating manifest under
emulation.

BUG=609580
TBR=sky

Committed: https://crrev.com/aeb778c817766d5c3b17097556218b0bafdbd1d4
Cr-Commit-Position: refs/heads/master@{#392171}
==========

to

==========
DevTools: account for the display scale factor while validating manifest under
emulation.

BUG=609580
==========

[pfeldman, 2016-05-07 00:12:30]

>> I'm reverting as I had concerns as mentioned above.
>> I'm ok with TBR for trivial changes, such as moving files, but this wasn't
that.

Sorry about that.

This patch removes one method and makes another existing method trivial. I was
hoping that counted as trivial.

[sky, 2016-05-07 17:00:03]

My mistake. I didn't realize it's an override. LGTM

[pfeldman, 2016-05-07 20:39:00]

The CQ bit was checked by pfeldman@chromium.org

[commit-bot: I haz the power, 2016-05-07 20:39:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1950133009/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1950133009/80001

[commit-bot: I haz the power, 2016-05-07 21:34:04]

Description was changed from

==========
DevTools: account for the display scale factor while validating manifest under
emulation.

BUG=609580
==========

to

==========
DevTools: account for the display scale factor while validating manifest under
emulation.

BUG=609580
==========

[commit-bot: I haz the power, 2016-05-07 21:34:05]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-07 21:35:36]

Description was changed from

==========
DevTools: account for the display scale factor while validating manifest under
emulation.

BUG=609580
==========

to

==========
DevTools: account for the display scale factor while validating manifest under
emulation.

BUG=609580

Committed: https://crrev.com/7a69caa094b972461ad889e1684697b7c171677d
Cr-Commit-Position: refs/heads/master@{#392268}
==========

[commit-bot: I haz the power, 2016-05-07 21:35:37]

Patchset 5 (id:??) landed as
https://crrev.com/7a69caa094b972461ad889e1684697b7c171677d
Cr-Commit-Position: refs/heads/master@{#392268}