Initialize device scale factor correctly on Android

Initialize device scale factor correctly on Android

The device scale factor was being initialized on Android, but was later
getting overridden back to the default value of 1. This patch fixes the
problem by moving the scale factor initialization from RenderViewImpl to
RenderWidget as is done on other platforms.

TEST=ViewportTest


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=168021

[Sami, 2012-10-30 11:42:14]

Adding jamesr for content/renderer.

johnme, bulach: How do you like the test?

[johnme, 2012-10-30 14:38:23]

Thanks for fixing this!

https://codereview.chromium.org/11348033/diff/1/content/public/android/javate...
File
content/public/android/javatests/src/org/chromium/content/browser/ViewportTest.java
(right):

https://codereview.chromium.org/11348033/diff/1/content/public/android/javate...
content/public/android/javatests/src/org/chromium/content/browser/ViewportTest.java:60:
// window.devicePixelRatio should match the default display. Only check to 4
decimal places
I might only check to 1dp. For example anywhere in the range 1.33 to 1.35 is
reasonable for Nexus 7.

https://codereview.chromium.org/11348033/diff/1/content/public/android/javate...
content/public/android/javatests/src/org/chromium/content/browser/ViewportTest.java:65:
// Screen size should match the default display.
That's not necessarily true. Beverloo and I think this should return logical UI
pixels (i.e. size in physical screen pixels divided by devicePixelRatio). It
seems that would also match what Apple do:
http://stackoverflow.com/questions/4833669/iphone-4-screen-width-reports-320-...

In the meantime, I'm not sure how useful it is to test this.

https://codereview.chromium.org/11348033/diff/1/content/public/android/javate...
content/public/android/javatests/src/org/chromium/content/browser/ViewportTest.java:70:
// now we only ensure they are not the same with a non-identity device pixel
ratio.
It's not clear why you expect them to be different; indeed while they usually
will be, on a 980 screen pixel wide device they should be equal. I think it
would be better to check that:

    eval(window.outerWidth) ~= metrics.widthPixels / metrics.density

and

    eval(window.innerWidth) == eval(document.documentElement.clientWidth) == 980

https://codereview.chromium.org/11348033/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/11348033/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:629: scoped_ptr<DeviceInfo> device_info(new
DeviceInfo());
Shouldn't you remove this too?

https://codereview.chromium.org/11348033/diff/1/content/renderer/render_widge...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/11348033/diff/1/content/renderer/render_widge...
content/renderer/render_widget.cc:134: device_scale_factor_ =
device_info->GetDPIScale();
There's also device scale factor logic in ui/gfx/display.cc, where
device_scale_factor_ is initialized using kForcedDeviceScaleFactor (which will
be wrong on Android).

Would it be better to move both this and the CrOS logic above into display.cc,
and make render_widget depend on that?

[jamesr, 2012-10-30 22:20:44]

On 2012/10/30 14:38:23, johnme wrote:
> content/renderer/render_widget.cc:134: device_scale_factor_ =
> device_info->GetDPIScale();
> There's also device scale factor logic in ui/gfx/display.cc, where
> device_scale_factor_ is initialized using kForcedDeviceScaleFactor (which will
> be wrong on Android).
> 
> Would it be better to move both this and the CrOS logic above into display.cc,
> and make render_widget depend on that?

This is a really good idea.  If you can't do this, please say why.  I don't like
having two copies of this logic that behave differently.

[aelias_OOO_until_Jul13, 2012-10-30 22:26:52]

FYI (not that this is relevant to this patch, but for future work in this area),
we are planning to converge all platforms to use device-scale-independent (DIP)
coordinates everywhere, including in the browser process.  During the transition
period, you can check the value of gfx::Screen::IsDIPEnabled() to see if we're
in that coordinate space.  We need to fix Android to work with both
IsDIPEnabled() == true and false, and then flip the flag to true when everything
is working.

[Sami, 2012-10-31 11:12:57]

https://codereview.chromium.org/11348033/diff/1/content/public/android/javate...
File
content/public/android/javatests/src/org/chromium/content/browser/ViewportTest.java
(right):

https://codereview.chromium.org/11348033/diff/1/content/public/android/javate...
content/public/android/javatests/src/org/chromium/content/browser/ViewportTest.java:60:
// window.devicePixelRatio should match the default display. Only check to 4
decimal places
On 2012/10/30 14:38:23, johnme wrote:
> I might only check to 1dp. For example anywhere in the range 1.33 to 1.35 is
> reasonable for Nexus 7.

Good point. Done.

https://codereview.chromium.org/11348033/diff/1/content/public/android/javate...
content/public/android/javatests/src/org/chromium/content/browser/ViewportTest.java:65:
// Screen size should match the default display.
On 2012/10/30 14:38:23, johnme wrote:
> That's not necessarily true. Beverloo and I think this should return logical
UI
> pixels (i.e. size in physical screen pixels divided by devicePixelRatio). It
> seems that would also match what Apple do:
>
http://stackoverflow.com/questions/4833669/iphone-4-screen-width-reports-320-...
> 
> In the meantime, I'm not sure how useful it is to test this.

That sounds like a different bug. I'll just remove this part of the test.

https://codereview.chromium.org/11348033/diff/1/content/public/android/javate...
content/public/android/javatests/src/org/chromium/content/browser/ViewportTest.java:70:
// now we only ensure they are not the same with a non-identity device pixel
ratio.
On 2012/10/30 14:38:23, johnme wrote:
> It's not clear why you expect them to be different; indeed while they usually
> will be, on a 980 screen pixel wide device they should be equal. I think it
> would be better to check that:
> 
>     eval(window.outerWidth) ~= metrics.widthPixels / metrics.density
> 
> and
> 
>     eval(window.innerWidth) == eval(document.documentElement.clientWidth) ==
980

I've replaced this with the test we designed offline.

https://codereview.chromium.org/11348033/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/11348033/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:629: scoped_ptr<DeviceInfo> device_info(new
DeviceInfo());
On 2012/10/30 14:38:23, johnme wrote:
> Shouldn't you remove this too?

Still being used on line 634 :)

https://codereview.chromium.org/11348033/diff/1/content/renderer/render_widge...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/11348033/diff/1/content/renderer/render_widge...
content/renderer/render_widget.cc:134: device_scale_factor_ =
device_info->GetDPIScale();
On 2012/10/30 14:38:23, johnme wrote:
> There's also device scale factor logic in ui/gfx/display.cc, where
> device_scale_factor_ is initialized using kForcedDeviceScaleFactor (which will
> be wrong on Android).
> 
> Would it be better to move both this and the CrOS logic above into display.cc,
> and make render_widget depend on that?

Good idea. I looked into this a little more and it doesn't seem very
straightforward to move this logic there. The problem is that
Display::device_scale_factor() is either 1 or the value from the command line --
i.e., it's not based on the real DPI on any of the ports as far as I can tell. I
don't want to go and change it to match the real DPI because I don't have a good
way of testing this on platforms other than Android.

On the plus side, it turns out that we can just add OS_ANDROID to the list on
line 124 and things will work.

I've added a TODO to clean this up.

[johnme, 2012-10-31 11:48:37]

https://codereview.chromium.org/11348033/diff/7001/content/renderer/render_wi...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/11348033/diff/7001/content/renderer/render_wi...
content/renderer/render_widget.cc:121: #if defined(OS_CHROMEOS) ||
defined(OS_MACOSX) || defined(OS_ANDROID)
Woah, this code path includes a static_cast<int>; we don't want that on Android
(and it should make your test fail on Nexus 7...)

[Sami, 2012-10-31 11:51:40]

https://codereview.chromium.org/11348033/diff/7001/content/renderer/render_wi...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/11348033/diff/7001/content/renderer/render_wi...
content/renderer/render_widget.cc:121: #if defined(OS_CHROMEOS) ||
defined(OS_MACOSX) || defined(OS_ANDROID)
On 2012/10/31 11:48:37, johnme wrote:
> Woah, this code path includes a static_cast<int>; we don't want that on
Android
> (and it should make your test fail on Nexus 7...)

Ack, serves me right for not re-testing on all devices. Thanks for spotting, let
me fix that.

[Sami, 2012-11-02 15:59:07]

+ben for ui/gfx.

Took another stab at consolidating the scale factor logic to gfx::Display. PTAL.

[Sami, 2012-11-06 11:11:13]

Anyone have a moment to review this?

[bulach, 2012-11-06 13:42:09]

I'm not familiar with this area so will defer to others, sorry! just one
question for the test:

https://codereview.chromium.org/11348033/diff/12004/content/public/android/ja...
File
content/public/android/javatests/src/org/chromium/content/browser/ViewportTest.java
(right):

https://codereview.chromium.org/11348033/diff/12004/content/public/android/ja...
content/public/android/javatests/src/org/chromium/content/browser/ViewportTest.java:68:
assertTrue(viewportWidth >= 979);
nit: aren't 979 / 981 device and orientation dependent? maybe check against
metrics from above?

[johnme, 2012-11-06 15:45:34]

https://codereview.chromium.org/11348033/diff/12004/content/public/android/ja...
File
content/public/android/javatests/src/org/chromium/content/browser/ViewportTest.java
(right):

https://codereview.chromium.org/11348033/diff/12004/content/public/android/ja...
content/public/android/javatests/src/org/chromium/content/browser/ViewportTest.java:68:
assertTrue(viewportWidth >= 979);
On 2012/11/06 13:42:09, bulach wrote:
> nit: aren't 979 / 981 device and orientation dependent? maybe check against
> metrics from above?

Nope, Clank's minimum viewport width for desktop sites with no viewport tag is
hardcoded to 980 (like most mobile browsers), so this is fine.

https://codereview.chromium.org/11348033/diff/12004/content/renderer/render_w...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/11348033/diff/12004/content/renderer/render_w...
content/renderer/render_widget.cc:127: display->SetScaleAndBounds(
Would it make sense to make a new constructor for Display which accepts a
screen_info and automatically calls SetScaleAndBounds with this scale factor, so
we can move even more logic out of render_widget? jamesr? oshima?

https://codereview.chromium.org/11348033/diff/12004/content/renderer/render_w...
content/renderer/render_widget.cc:128: screen_info.verticalDPI / kStandardDPI,
display->bounds());
Presumably this works on Android because in practice we initialize
screen_info.verticalDPI to device_info->GetDPIScale() * 160? We definitely
shouldn't be relying on DisplayMetrics's xdpi and ydpi, as those are
unreliable/deprecated.

Nit: I'd rather we used horizontalDPI instead of verticalDPI, since one of the
most important uses for the scale is determining the device-width that will be
used in mobile viewports. Alternatively, even better is probably to use
min(horizontalDPI, verticalDPI).

P.S. I notice that Mac and CrOS are using the wrong kStandardDPI; I filed
http://crbug.com/159603 about that (ultimately recommending that we move
chromium away from dealing with dpi to dealing purely in device scale factors).

https://codereview.chromium.org/11348033/diff/12004/ui/gfx/display.cc
File ui/gfx/display.cc (right):

https://codereview.chromium.org/11348033/diff/12004/ui/gfx/display.cc#newcode94
ui/gfx/display.cc:94: #if defined(OS_CHROMEOS) || defined(OS_MACOSX)
We want this on Android (and all other platforms) too, so you can remove this
ifdef. Logical UI pixels should never be smaller than physical screen pixels.

[johnme, 2012-11-06 15:46:12]

+oshima, who's been heavily involved in display.cc

[Sami, 2012-11-06 17:08:46]

https://codereview.chromium.org/11348033/diff/12004/content/public/android/ja...
File
content/public/android/javatests/src/org/chromium/content/browser/ViewportTest.java
(right):

https://codereview.chromium.org/11348033/diff/12004/content/public/android/ja...
content/public/android/javatests/src/org/chromium/content/browser/ViewportTest.java:68:
assertTrue(viewportWidth >= 979);
Thanks for confirming that John. I also checked that the initial page scale
looks sane in portrait mode with this code.

https://codereview.chromium.org/11348033/diff/12004/content/renderer/render_w...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/11348033/diff/12004/content/renderer/render_w...
content/renderer/render_widget.cc:127: display->SetScaleAndBounds(
Good idea, that makes things a little cleaner. Someone correct me if I'm wrong,
but since WebScreenInfo is a public WebKit class we can use it from
gfx::Display.

https://codereview.chromium.org/11348033/diff/12004/content/renderer/render_w...
content/renderer/render_widget.cc:128: screen_info.verticalDPI / kStandardDPI,
display->bounds());
On 2012/11/06 15:45:34, johnme wrote:
> Presumably this works on Android because in practice we initialize
> screen_info.verticalDPI to device_info->GetDPIScale() * 160? We definitely
> shouldn't be relying on DisplayMetrics's xdpi and ydpi, as those are
> unreliable/deprecated.

That's right, and based on the discussion it seems like some of the other ports
are doing it as well.

> Nit: I'd rather we used horizontalDPI instead of verticalDPI, since one of the
> most important uses for the scale is determining the device-width that will be
> used in mobile viewports. Alternatively, even better is probably to use
> min(horizontalDPI, verticalDPI).

I went for your min(hdpi, vdpi) suggestion. I checked that Android, Mac and CrOS
seem to be initializing both horizontal and vertical DPI to the same value, so
in practice this probably won't matter.
 
> P.S. I notice that Mac and CrOS are using the wrong kStandardDPI; I filed
> http://crbug.com/159603 about that (ultimately recommending that we move
> chromium away from dealing with dpi to dealing purely in device scale
factors).

https://codereview.chromium.org/11348033/diff/12004/ui/gfx/display.cc
File ui/gfx/display.cc (right):

https://codereview.chromium.org/11348033/diff/12004/ui/gfx/display.cc#newcode94
ui/gfx/display.cc:94: #if defined(OS_CHROMEOS) || defined(OS_MACOSX)
On 2012/11/06 15:45:34, johnme wrote:
> We want this on Android (and all other platforms) too, so you can remove this
> ifdef. Logical UI pixels should never be smaller than physical screen pixels.

Ok, I wasn't sure if this was the case so thanks for confirming.

[Sami, 2012-11-06 17:14:35]

On 2012/11/06 17:08:46, Sami wrote:
> Good idea, that makes things a little cleaner. Someone correct me if I'm
wrong,
> but since WebScreenInfo is a public WebKit class we can use it from
> gfx::Display.

checkdeps doesn't seem to agree, so I guess this isn't kosher after all. I'll
revert to the earlier code.

[johnme, 2012-11-06 18:04:00]

On 2012/11/06 17:14:35, Sami wrote:
> On 2012/11/06 17:08:46, Sami wrote:
> > Good idea, that makes things a little cleaner. Someone correct me if I'm
> wrong,
> > but since WebScreenInfo is a public WebKit class we can use it from
> > gfx::Display.
> 
> checkdeps doesn't seem to agree, so I guess this isn't kosher after all. I'll
> revert to the earlier code.

Shame. I guess we'll clean this up as part of http://crbug.com/159603 &
http://crbug.com/155213 anyway.

[jamesr, 2012-11-06 18:36:13]

lgtm for content/renderer

https://codereview.chromium.org/11348033/diff/9005/content/renderer/render_wi...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/11348033/diff/9005/content/renderer/render_wi...
content/renderer/render_widget.cc:126: scoped_ptr<gfx::Display> display(new
gfx::Display());
why heap allocate? can't this go on the stack?

[Sami, 2012-11-06 18:40:32]

On 2012/11/06 18:36:13, jamesr wrote:
> why heap allocate? can't this go on the stack?

Yeah, it's pretty lightweight so let's put it on the stack.

[oshima, 2012-11-06 18:40:35]

can you add scale factor to ScreenInfo instead as mentioned in crbug.com/155213?

https://codereview.chromium.org/11348033/diff/9005/ui/gfx/display.cc
File ui/gfx/display.cc (right):

https://codereview.chromium.org/11348033/diff/9005/ui/gfx/display.cc#newcode94
ui/gfx/display.cc:94: device_scale_factor_ = std::max(1.0f,
device_scale_factor_);
This will break --force-device-scale-factor option when testing on desktop.

[Sami, 2012-11-06 18:52:56]

> can you add scale factor to ScreenInfo instead as mentioned in
crbug.com/155213?

Thanks for the hint. I'll take a look at what that would involve. Problem is
that the only only hi-dpi devices I have for testing are Android devices and I
don't want to accidentally break the other platforms.

https://codereview.chromium.org/11348033/diff/9005/ui/gfx/display.cc
File ui/gfx/display.cc (right):

https://codereview.chromium.org/11348033/diff/9005/ui/gfx/display.cc#newcode94
ui/gfx/display.cc:94: device_scale_factor_ = std::max(1.0f,
device_scale_factor_);
On 2012/11/06 18:40:35, oshima wrote:
> This will break --force-device-scale-factor option when testing on desktop.

Did you mean you want to test with scale factors that are less than 1? Is that a
configuration we want to support?

[oshima, 2012-11-06 19:53:08]

On 2012/11/06 18:52:56, Sami wrote:
> > can you add scale factor to ScreenInfo instead as mentioned in
> crbug.com/155213?
> 
> Thanks for the hint. I'll take a look at what that would involve. Problem is
> that the only only hi-dpi devices I have for testing are Android devices and I
> don't want to accidentally break the other platforms.

Fixing ScreenInfo is cleaner and will have less chance to break other platforms.
Are you sure this won't break Windows and Linux? Looks like they'll use
scale factors other than 1.0 with this change, which probably will break.

I think Chrome shouldn't do any decision making about scale factor. Each
platform has
its own way to provide scale factor and we should simply use it (other than
testing environment).
Android has DisplayMetrics class that has scale factor info (or something
equivalent if it's chanced
since I wrote it :), and there should be no need to compute it from DPI.

The current code is a bit of hack due to historical reason (I guess this was
done as short
term solution, but not sure), and since this caused the issue before I prefer
this
to be fixed in right way.

> 
> https://codereview.chromium.org/11348033/diff/9005/ui/gfx/display.cc
> File ui/gfx/display.cc (right):
> 
> https://codereview.chromium.org/11348033/diff/9005/ui/gfx/display.cc#newcode94
> ui/gfx/display.cc:94: device_scale_factor_ = std::max(1.0f,
> device_scale_factor_);
> On 2012/11/06 18:40:35, oshima wrote:
> > This will break --force-device-scale-factor option when testing on desktop.
> 
> Did you mean you want to test with scale factors that are less than 1? Is that
a
> configuration we want to support?

Sorry, I read it wrong. It doesn't really break per se, but I still like to be
able to
set any scale factor using --force option because you know, force means force.
I have no objection to do this for normal case. 
IIRC, android used to support scale factor <1.0, but I guess it's no longer
supported?

[johnme, 2012-11-07 10:44:59]

On 2012/11/06 19:53:08, oshima wrote:
> IIRC, android used to support scale factor <1.0, but I guess it's no longer
> supported?

To the best of my knowledge Android has never supported device scale factor < 1.
It simply doesn't make sense - CSS has a reasonable expectation that every CSS
px will be visible to the user, and if you set dSF < 1 you end up with CSS
pixels that are smaller than physical screen pixels.

Android does however also have a page scale factor (as do the Chrome OS and
Windows 8 variants of Chrome) which is used by pinch zoom, and that is allowed
to go < 1 (for example when a desktop page first loads, it will be displayed in
overview zoom level, which is < 1).

[oshima, 2012-11-07 16:55:42]

On 2012/11/07 10:44:59, johnme wrote:
> On 2012/11/06 19:53:08, oshima wrote:
> > IIRC, android used to support scale factor <1.0, but I guess it's no longer
> > supported?
> 
> To the best of my knowledge Android has never supported device scale factor <
1.
> It simply doesn't make sense - CSS has a reasonable expectation that every CSS
> px will be visible to the user, and if you set dSF < 1 you end up with CSS
> pixels that are smaller than physical screen pixels.

There used to be a low-end device that has DPI~120dpi and I believe these device
were using 0.75 scale factor. (see ldpi in
http://developer.android.com/guide/practices/screens_support.html)
I doubt that such devices are on market nowadays, nor can run android OS that
chrome
supports, and I won't be surprised even if it's dropped.
> 
> Android does however also have a page scale factor (as do the Chrome OS and
> Windows 8 variants of Chrome) which is used by pinch zoom, and that is allowed
> to go < 1 (for example when a desktop page first loads, it will be displayed
in
> overview zoom level, which is < 1).

[Sami, 2012-11-07 19:09:17]

I looked into fixing this in WebScreenInfo and have formulated this cunning 5
step plan:

1. [WebKit] Introduce WebScreenInfo::deviceScaleFactor and initialize it on Mac.
2. [chromium] Initialize WebScreenInfo::deviceScaleFactor on Android and Aura.
3. [WebKit] Remove WebCore::screen{Horizontal,Vertical}DPI() and use
WebScreenInfo::deviceScaleFactor in
ChromeClientImpl::dispatchViewportPropertiesDidChange.
4. [chromium] Use WebScreenInfo::deviceScaleFactor instead of
WebScreenInfo::{horizontal,vertical}DPI everywhere (i.e., this patch).
5. [WebKit] Remove WebScreenInfo::{horizontal,vertical}DPI.

Sound good? This still leaves the problem of RenderWidget having two sources for
device_scale_factor_ but let's fix that later.

On 2012/11/07 16:55:42, oshima wrote:
> There used to be a low-end device that has DPI~120dpi and I believe these
device
> were using 0.75 scale factor. (see ldpi in
> http://developer.android.com/guide/practices/screens_support.html)
> I doubt that such devices are on market nowadays, nor can run android OS that
> chrome
> supports, and I won't be surprised even if it's dropped.

Sounds like we can assume that device scale factor >= 1.

[oshima, 2012-11-07 21:06:15]

On 2012/11/07 19:09:17, Sami wrote:
> I looked into fixing this in WebScreenInfo and have formulated this cunning 5
> step plan:
> 
> 1. [WebKit] Introduce WebScreenInfo::deviceScaleFactor and initialize it on
Mac.
> 2. [chromium] Initialize WebScreenInfo::deviceScaleFactor on Android and Aura.
> 3. [WebKit] Remove WebCore::screen{Horizontal,Vertical}DPI() and use
> WebScreenInfo::deviceScaleFactor in
> ChromeClientImpl::dispatchViewportPropertiesDidChange.
> 4. [chromium] Use WebScreenInfo::deviceScaleFactor instead of
> WebScreenInfo::{horizontal,vertical}DPI everywhere (i.e., this patch).
> 5. [WebKit] Remove WebScreenInfo::{horizontal,vertical}DPI.
> 
> Sound good? This still leaves the problem of RenderWidget having two sources
for
> device_scale_factor_ but let's fix that later.


Sounds good. We may want to set scale factor in one place for all platforms. 
For 2), we may want to do this in the same way as mac on aura as well,
but that'd be separate change.


> 
> On 2012/11/07 16:55:42, oshima wrote:
> > There used to be a low-end device that has DPI~120dpi and I believe these
> device
> > were using 0.75 scale factor. (see ldpi in
> > http://developer.android.com/guide/practices/screens_support.html)
> > I doubt that such devices are on market nowadays, nor can run android OS
that
> > chrome
> > supports, and I won't be surprised even if it's dropped.
> 
> Sounds like we can assume that device scale factor >= 1.

[johnme, 2012-11-08 12:49:46]

On 2012/11/07 19:09:17, Sami wrote:
> I looked into fixing this in WebScreenInfo and have formulated this cunning 5
> step plan:
> 
> 1. [WebKit] Introduce WebScreenInfo::deviceScaleFactor and initialize it on
Mac.
> 2. [chromium] Initialize WebScreenInfo::deviceScaleFactor on Android and Aura.
> 3. [WebKit] Remove WebCore::screen{Horizontal,Vertical}DPI() and use
> WebScreenInfo::deviceScaleFactor in
> ChromeClientImpl::dispatchViewportPropertiesDidChange.
> 4. [chromium] Use WebScreenInfo::deviceScaleFactor instead of
> WebScreenInfo::{horizontal,vertical}DPI everywhere (i.e., this patch).
> 5. [WebKit] Remove WebScreenInfo::{horizontal,vertical}DPI.
> 
> Sound good? This still leaves the problem of RenderWidget having two sources
for
> device_scale_factor_ but let's fix that later.

I like the general approach.
- Agree with oshima that it would be nice to initialize deviceScaleFactor in the
same place across Mac and other platforms if possible.
- After step 5, how are you going to implement
WebCore/platform/PlatformScreen.h's screen{Horizontal,Vertical}DPI methods, and
hence WebCore/page/Screen.h's {horizontal,vertical}DPI methods? Are you going to
refactor WebCore to remove those? It seems they are only used by
WebCore/css/MediaQueryEvaluator.cpp, which uses them incorrectly anyway (it
should just multiply deviceScaleFactor by 96 instead), so it might actually be
reasonably easy to remove these.

[Sami, 2012-11-08 16:23:35]

On 2012/11/08 12:49:46, johnme wrote:
> - Agree with oshima that it would be nice to initialize deviceScaleFactor in
the
> same place across Mac and other platforms if possible.

Agreed, but I'd prefer someone more familiar with that code would make that
change.

> - After step 5, how are you going to implement
> WebCore/platform/PlatformScreen.h's screen{Horizontal,Vertical}DPI methods,
and
> hence WebCore/page/Screen.h's {horizontal,vertical}DPI methods? Are you going
to
> refactor WebCore to remove those? It seems they are only used by
> WebCore/css/MediaQueryEvaluator.cpp, which uses them incorrectly anyway (it
> should just multiply deviceScaleFactor by 96 instead), so it might actually be
> reasonably easy to remove these.

Right, my plan was to drop both and make media queries do the right thing.

[Sami, 2012-11-09 14:04:37]

This is a combination of steps 2 and 4 since I realized they can go in in one
go.

[Sami, 2012-11-12 21:49:46]

oshima@, do you think this patch looks okay now? If so, I'll try to get the
different directories reviewed.

[Sami, 2012-11-14 16:05:05]

I checked offline with oshima@ that he's happy with this, so:

sky@: could you please check the changes in content/browser and ui/gfx?

cdn@: ok to add the new field in view_messages.h?

bulach@: I can haz lgtm for the test?

[oshima, 2012-11-14 16:52:35]

lgtm with a few questions.

https://codereview.chromium.org/11348033/diff/13008/content/renderer/render_w...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/11348033/diff/13008/content/renderer/render_w...
content/renderer/render_widget.cc:1594: screen_info_ = screen_info;
can you add 

// TODO(oshima): Update scale factor here and remove OnSetDeviceScaleFactor

?

https://codereview.chromium.org/11348033/diff/13008/ui/gfx/display.cc
File ui/gfx/display.cc (right):

https://codereview.chromium.org/11348033/diff/13008/ui/gfx/display.cc#newcode94
ui/gfx/display.cc:94: device_scale_factor_ = std::max(1.0f,
device_scale_factor_);
Is this necessary for android?
I most likely eliminate these safe guard in near future, and want to know if
I should do it only for chromeos, or can just eliminate for all platforms.

[johnme, 2012-11-14 16:56:45]

https://codereview.chromium.org/11348033/diff/13008/ui/gfx/display.cc
File ui/gfx/display.cc (right):

https://codereview.chromium.org/11348033/diff/13008/ui/gfx/display.cc#newcode94
ui/gfx/display.cc:94: device_scale_factor_ = std::max(1.0f,
device_scale_factor_);
On 2012/11/14 16:52:35, oshima wrote:
> Is this necessary for android?
> I most likely eliminate these safe guard in near future, and want to know if
> I should do it only for chromeos, or can just eliminate for all platforms.

There should never be any harm in using this safeguard (as device_scale_factor_
should never be less than 1). So I'd recommend just keeping it for all
platforms.

[oshima, 2012-11-14 17:07:36]

On 2012/11/14 16:56:45, johnme wrote:
> https://codereview.chromium.org/11348033/diff/13008/ui/gfx/display.cc
> File ui/gfx/display.cc (right):
> 
>
https://codereview.chromium.org/11348033/diff/13008/ui/gfx/display.cc#newcode94
> ui/gfx/display.cc:94: device_scale_factor_ = std::max(1.0f,
> device_scale_factor_);
> On 2012/11/14 16:52:35, oshima wrote:
> > Is this necessary for android?
> > I most likely eliminate these safe guard in near future, and want to know if
> > I should do it only for chromeos, or can just eliminate for all platforms.
> 
> There should never be any harm in using this safeguard (as
device_scale_factor_
> should never be less than 1). So I'd recommend just keeping it for all
> platforms.
We'd like to experiment < 1.0 scale factor for  (physically) larger screen such
as TV monitor.

[oshima, 2012-11-14 17:18:45]

On 2012/11/14 17:07:36, oshima wrote:
> On 2012/11/14 16:56:45, johnme wrote:
> > https://codereview.chromium.org/11348033/diff/13008/ui/gfx/display.cc
> > File ui/gfx/display.cc (right):
> > 
> >
>
https://codereview.chromium.org/11348033/diff/13008/ui/gfx/display.cc#newcode94
> > ui/gfx/display.cc:94: device_scale_factor_ = std::max(1.0f,
> > device_scale_factor_);
> > On 2012/11/14 16:52:35, oshima wrote:
> > > Is this necessary for android?
> > > I most likely eliminate these safe guard in near future, and want to know
if
> > > I should do it only for chromeos, or can just eliminate for all platforms.
> > 
> > There should never be any harm in using this safeguard (as
> device_scale_factor_
> > should never be less than 1). So I'd recommend just keeping it for all
> > platforms.
> We'd like to experiment < 1.0 scale factor for  (physically) larger screen
such
> as TV monitor.

Please disregard.  It's opposite and I probably have lost my mind. 
I still will remove integral part as we'd like to experiment non integral scale
factor,
not via --force-device-scale-factor  (this is true).

[johnme, 2012-11-14 17:47:57]

On 2012/11/14 17:18:45, oshima wrote:
> On 2012/11/14 17:07:36, oshima wrote:
> > We'd like to experiment < 1.0 scale factor for  (physically) larger screen
> such
> > as TV monitor.
> 
> Please disregard.  It's opposite and I probably have lost my mind. 
> I still will remove integral part as we'd like to experiment non integral
scale
> factor,
> not via --force-device-scale-factor  (this is true).

Yup, as you obviously realized you want a > 1.0 deviceScaleFactor for high-res
televisions that are viewed from a distance.

I'm very happy for you to remove the clamping to integers. Android definitely
doesn't want that (we have devices with 1.333 and 1.5 deviceScaleFactors, and we
don't want those to be arbitrarily clamped).

[Sami, 2012-11-14 18:37:26]

On 2012/11/14 16:52:35, oshima wrote:
> lgtm with a few questions.
> 
>
https://codereview.chromium.org/11348033/diff/13008/content/renderer/render_w...
> File content/renderer/render_widget.cc (right):
> 
>
https://codereview.chromium.org/11348033/diff/13008/content/renderer/render_w...
> content/renderer/render_widget.cc:1594: screen_info_ = screen_info;
> can you add 
> 
> // TODO(oshima): Update scale factor here and remove OnSetDeviceScaleFactor
> 
> ?

Sure, done.

> https://codereview.chromium.org/11348033/diff/13008/ui/gfx/display.cc
> File ui/gfx/display.cc (right):
> 
>
https://codereview.chromium.org/11348033/diff/13008/ui/gfx/display.cc#newcode94
> ui/gfx/display.cc:94: device_scale_factor_ = std::max(1.0f,
> device_scale_factor_);
> Is this necessary for android?
> I most likely eliminate these safe guard in near future, and want to know if
> I should do it only for chromeos, or can just eliminate for all platforms.

It sounds like we want to have this safeguard in place so I'll leave it as is.

[oshima, 2012-11-14 20:25:50]

still lgtm

On 2012/11/14 17:47:57, johnme wrote:
> On 2012/11/14 17:18:45, oshima wrote:
> > On 2012/11/14 17:07:36, oshima wrote:
> > > We'd like to experiment < 1.0 scale factor for  (physically) larger screen
> > such
> > > as TV monitor.
> > 
> > Please disregard.  It's opposite and I probably have lost my mind. 
> > I still will remove integral part as we'd like to experiment non integral
> scale
> > factor,
> > not via --force-device-scale-factor  (this is true).
> 
> Yup, as you obviously realized you want a > 1.0 deviceScaleFactor for high-res
> televisions that are viewed from a distance.
> 
> I'm very happy for you to remove the clamping to integers. Android definitely
> doesn't want that (we have devices with 1.333 and 1.5 deviceScaleFactors, and
we
> don't want those to be arbitrarily clamped).

Thanks, can you just remove defined(OS_CHROMEOS) then?

[Sami, 2012-11-14 20:30:44]

On 2012/11/14 20:25:50, oshima wrote:
> Thanks, can you just remove defined(OS_CHROMEOS) then?

Done.

[Cris Neckar, 2012-11-14 20:36:09]

lgtm for view_messages

[bulach, 2012-11-15 00:39:22]

lgtm for android/, thanks!

[sky, 2012-11-15 17:23:51]

https://codereview.chromium.org/11348033/diff/12016/ui/gfx/display.cc
File ui/gfx/display.cc (right):

https://codereview.chromium.org/11348033/diff/12016/ui/gfx/display.cc#newcode87
ui/gfx/display.cc:87: #if defined(OS_MACOSX)
Shouldn't this be done at the call site and not here? And are you sure you want
a floor?

[Sami, 2012-11-15 17:31:38]

On 2012/11/15 17:23:51, sky wrote:
> https://codereview.chromium.org/11348033/diff/12016/ui/gfx/display.cc
> File ui/gfx/display.cc (right):
> 
>
https://codereview.chromium.org/11348033/diff/12016/ui/gfx/display.cc#newcode87
> ui/gfx/display.cc:87: #if defined(OS_MACOSX)
> Shouldn't this be done at the call site and not here? And are you sure you
want
> a floor?

We wanted to consolidate the device scale factor calculation in a single place
(see comment #2), so this is just moving the code from render_widget.cc to here.
On Mac the factor was already truncated to an integer before and I didn't have a
reason to change that. I did remove the truncation for CrOS as discussed above.

[sky, 2012-11-15 18:26:23]

LGTM

[commit-bot: I haz the power, 2012-11-15 18:49:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/11348033/12016

[commit-bot: I haz the power, 2012-11-15 20:49:55]

Change committed as 168021