Hookup device dpi for ChromeOS

Hookup device dpi for ChromeOS

Previously, chrome.system.display.getInfo returns a hard
coded dpi value(96). This CL hook up the correct DPI
information on ChromeOS platform.

BUG=522095
Committed: https://crrev.com/93dad6d37dc8c3a68b4950aa37b3a0da25872742
Cr-Commit-Position: refs/heads/master@{#350088}

[bshe, 2015-08-18 15:00:37]

bshe@chromium.org changed reviewers:
+ oshima@chromium.org

[bshe, 2015-08-18 15:10:40]

Hey Oshima-san.
Do you mind to take a look at this CL? I am not familiar with the display code
and how the display information is passed in ash and ChromeOS. But I tested it
on my pixel and this patch seems to work. I might missed some other places that
I am not aware. Any comment is welcome! Thanks

Biao

[oshima, 2015-08-19 02:07:08]

https://codereview.chromium.org/1289043002/diff/40001/ash/display/display_cha...
File ash/display/display_change_observer_chromeos.cc (right):

https://codereview.chromium.org/1289043002/diff/40001/ash/display/display_cha...
ash/display/display_change_observer_chromeos.cc:191:
state->physical_size().width();
This is physical DPI, and it may not represent the UI size that user is using
when DSF and/or UI zooming are applied. Will this really work for your scenario?

[bshe, 2015-08-19 13:37:10]

https://codereview.chromium.org/1289043002/diff/40001/ash/display/display_cha...
File ash/display/display_change_observer_chromeos.cc (right):

https://codereview.chromium.org/1289043002/diff/40001/ash/display/display_cha...
ash/display/display_change_observer_chromeos.cc:191:
state->physical_size().width();
On 2015/08/19 02:07:08, oshima (In Tokyo 17th - 21st) wrote:
> This is physical DPI, and it may not represent the UI size that user is using
> when DSF and/or UI zooming are applied. Will this really work for your
scenario?

right. So I called this one device_dpi instead of dpi. And then in
display_info_provider_chromeos.cc. I divide the device_dpi by
device_scale_factor to calculate the current effective dpi. It looks like
working. On pixel, the size that I calcuated from dpi seems correct(tried
1280x850 and 2560x1700).

[oshima, 2015-08-20 00:20:32]

https://codereview.chromium.org/1289043002/diff/40001/ash/display/display_cha...
File ash/display/display_change_observer_chromeos.cc (right):

https://codereview.chromium.org/1289043002/diff/40001/ash/display/display_cha...
ash/display/display_change_observer_chromeos.cc:191:
state->physical_size().width();
On 2015/08/19 13:37:09, bshe wrote:
> On 2015/08/19 02:07:08, oshima (In Tokyo 17th - 21st) wrote:
> > This is physical DPI, and it may not represent the UI size that user is
using
> > when DSF and/or UI zooming are applied. Will this really work for your
> scenario?
> 
> right. So I called this one device_dpi instead of dpi. And then in
> display_info_provider_chromeos.cc. I divide the device_dpi by
> device_scale_factor to calculate the current effective dpi. It looks like
> working. On pixel, the size that I calcuated from dpi seems correct(tried
> 1280x850 and 2560x1700).

What about 1440x956?

[bshe, 2015-08-20 14:42:05]

Thanks for review. I have updated a new patch and it seems to work on all
resolutions on pixel. Not sure if there are other edge cases that I need to
test. But would you mind to take a look first?

https://codereview.chromium.org/1289043002/diff/40001/ash/display/display_cha...
File ash/display/display_change_observer_chromeos.cc (right):

https://codereview.chromium.org/1289043002/diff/40001/ash/display/display_cha...
ash/display/display_change_observer_chromeos.cc:191:
state->physical_size().width();
On 2015/08/20 00:20:32, oshima (In Tokyo 17th - 21st) wrote:
> On 2015/08/19 13:37:09, bshe wrote:
> > On 2015/08/19 02:07:08, oshima (In Tokyo 17th - 21st) wrote:
> > > This is physical DPI, and it may not represent the UI size that user is
> using
> > > when DSF and/or UI zooming are applied. Will this really work for your
> > scenario?
> > 
> > right. So I called this one device_dpi instead of dpi. And then in
> > display_info_provider_chromeos.cc. I divide the device_dpi by
> > device_scale_factor to calculate the current effective dpi. It looks like
> > working. On pixel, the size that I calcuated from dpi seems correct(tried
> > 1280x850 and 2560x1700).
> 
> What about 1440x956?

I see. 1440x956 will return the wrong dpi as scale factor is tied to device dpi
threshold. I have changed the code so the dpi is calculated from device_dpi,
native screen size and current effective screen size.

[oshima, 2015-09-02 00:23:00]

lgtm

[bshe, 2015-09-08 22:21:17]

The CQ bit was checked by bshe@chromium.org

[commit-bot: I haz the power, 2015-09-08 22:21:44]

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

[commit-bot: I haz the power, 2015-09-08 22:34:11]

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

[commit-bot: I haz the power, 2015-09-08 22:34:12]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[bshe, 2015-09-09 19:31:24]

bshe@chromium.org changed reviewers:
+ rockot@chromium.org

[bshe, 2015-09-09 19:31:25]

+rockot for:
chrome/browser/extensions/display_info_provider_chromeos.cc

Thanks!

[Ken Rockot(use gerrit already), 2015-09-09 20:08:01]

lgtm

[bshe, 2015-09-09 20:28:40]

The CQ bit was checked by bshe@chromium.org

[bshe, 2015-09-09 20:28:41]

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

[commit-bot: I haz the power, 2015-09-09 20:28:57]

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

[commit-bot: I haz the power, 2015-09-09 21:12:34]

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

[commit-bot: I haz the power, 2015-09-09 21:12:34]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bshe, 2015-09-15 13:01:03]

Patchset #6 (id:100001) has been deleted

[oshima, 2015-09-15 17:34:24]

just noticed one thing.

https://codereview.chromium.org/1289043002/diff/80001/ash/display/display_cha...
File ash/display/display_change_observer_chromeos.cc (right):

https://codereview.chromium.org/1289043002/diff/80001/ash/display/display_cha...
ash/display/display_change_observer_chromeos.cc:191: kInchInMm *
mode_info->size().width() / state->physical_size().width();
can you check against black listed size and fallback to 96?

[bshe, 2015-09-21 20:14:15]

On 2015/09/15 17:34:24, oshima wrote:
> just noticed one thing.
> 
>
https://codereview.chromium.org/1289043002/diff/80001/ash/display/display_cha...
> File ash/display/display_change_observer_chromeos.cc (right):
> 
>
https://codereview.chromium.org/1289043002/diff/80001/ash/display/display_cha...
> ash/display/display_change_observer_chromeos.cc:191: kInchInMm *
> mode_info->size().width() / state->physical_size().width();
> can you check against black listed size and fallback to 96?

Hi Oshima. Sorry for the delay! I have addressed your comment. Would you mind to
take another look? Thanks!

[oshima, 2015-09-21 23:11:52]

https://codereview.chromium.org/1289043002/diff/140001/ash/display/display_ch...
File ash/display/display_change_observer_chromeos.cc (right):

https://codereview.chromium.org/1289043002/diff/140001/ash/display/display_ch...
ash/display/display_change_observer_chromeos.cc:191: kInchInMm *
mode_info->size().width() / state->physical_size().width();
can you check the blacklist and simply set it to zero (or -1) and check that
condition below?

[bshe, 2015-09-22 00:20:25]

Thanks. PTAL.

https://codereview.chromium.org/1289043002/diff/140001/ash/display/display_ch...
File ash/display/display_change_observer_chromeos.cc (right):

https://codereview.chromium.org/1289043002/diff/140001/ash/display/display_ch...
ash/display/display_change_observer_chromeos.cc:191: kInchInMm *
mode_info->size().width() / state->physical_size().width();
On 2015/09/21 23:11:51, oshima wrote:
> can you check the blacklist and simply set it to zero (or -1) and check that
> condition below?

Done.

[oshima, 2015-09-22 00:45:40]

lgtm

[bshe, 2015-09-22 01:23:29]

The CQ bit was checked by bshe@chromium.org

[bshe, 2015-09-22 01:23:29]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/1289043002/#ps160001
(title: "review")

[commit-bot: I haz the power, 2015-09-22 01:23:40]

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

[commit-bot: I haz the power, 2015-09-22 01:58:20]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2015-09-22 01:59:12]

Patchset 8 (id:??) landed as
https://crrev.com/93dad6d37dc8c3a68b4950aa37b3a0da25872742
Cr-Commit-Position: refs/heads/master@{#350088}