Fix high-DPI on Windows to make use of DIP scaling in WebKit.

Fix high-DPI on Windows to make use of DIP scaling in WebKit.

BUG=149881
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=179899

[pkotwicz, 2013-01-23 18:13:12]

https://codereview.chromium.org/11953054/diff/1021/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

https://codereview.chromium.org/11953054/diff/1021/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_win.cc:611: gfx::Rect
RenderWidgetHostViewWin::getPixelBounds() const {
 Nit:GetPixelBounds()

https://codereview.chromium.org/11953054/diff/1021/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_win.cc:2746:
gfx::Point(static_cast<short>(LOWORD(lparam)),
Nit: alignment, 4 spaces

https://codereview.chromium.org/11953054/diff/1021/ui/base/resource/resource_...
File ui/base/resource/resource_bundle_win.cc (right):

https://codereview.chromium.org/11953054/diff/1021/ui/base/resource/resource_...
ui/base/resource/resource_bundle_win.cc:47: use_hidpi = ui::GetDPIScale() > 1.2;
For the purpose of this CL, you actually do not need the changes in
ResourceBundle

Nit: You can define scale depending on whether HIDPI is enabled.

Something like this:
#if defined(ENABLE_HIDPI)
float scale = ui::GetDPIScale();
#else
float scale = 1.0f;
#endif

https://codereview.chromium.org/11953054/diff/1021/ui/base/resource/resource_...
ui/base/resource/resource_bundle_win.cc:56: } else {
Nit: Put a comment that we will be using 1.4x resources for 1.25 and 1.5 scale
factors.
This is so that people like me who are unfamiliar with windows code are not
asking "Why are you not checking for (scale == 1.4f) ?"

[pkotwicz, 2013-01-23 19:39:23]

https://codereview.chromium.org/11953054/diff/5001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

https://codereview.chromium.org/11953054/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_win.cc:302: 
Pass in NativeViewId here instead of HWND

Nit: Get rid of the extra line.

https://codereview.chromium.org/11953054/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_win.cc:831:
GetScreenInfoForWindow(results, 0);
This should be GetNativeViewId() instead of 0.

https://codereview.chromium.org/11953054/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_win.cc:837:
GetScreenInfoForWindow(results, NULL);
This should probably be 0 instead of NULL.

https://codereview.chromium.org/11953054/diff/5001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_win.h (right):

https://codereview.chromium.org/11953054/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_win.h:216: virtual
gfx::Rect GetBoundsInRootWindow() OVERRIDE;
Move GetScreenInfo() here and in .cc file right above GetBoundsInRootWindow() to
match order in render_widget_host_view_port.h

https://codereview.chromium.org/11953054/diff/5001/ui/base/win/dpi.cc
File ui/base/win/dpi.cc (right):

https://codereview.chromium.org/11953054/diff/5001/ui/base/win/dpi.cc#newcode76
ui/base/win/dpi.cc:76: return gfx::ToFlooredRectDeprecated(
Nit: Add TODO to use gfx::ToNearestRect().

https://codereview.chromium.org/11953054/diff/5001/ui/base/win/dpi.h
File ui/base/win/dpi.h (right):

https://codereview.chromium.org/11953054/diff/5001/ui/base/win/dpi.h#newcode26
ui/base/win/dpi.h:26: // they are also Windows specific.
Nit.
How about: "TODO(kevers|girard): Move above methods into win namespace."

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

https://codereview.chromium.org/11953054/diff/5001/ui/gfx/display.cc#newcode32
ui/gfx/display.cc:32: #if defined(OS_WIN)
I think that this block of code should be in GetDeviceScaleFactorImpl() in
"dpi.cc"

https://codereview.chromium.org/11953054/diff/5001/ui/gfx/display.cc#newcode52
ui/gfx/display.cc:52: //static
Nit: newline before static

https://codereview.chromium.org/11953054/diff/5001/ui/gfx/display.h
File ui/gfx/display.h (right):

https://codereview.chromium.org/11953054/diff/5001/ui/gfx/display.h#newcode30
ui/gfx/display.h:30: static bool HasForceDeviceScaleFactor();
Add a comment for HasForceDeviceScaleFactor().

https://codereview.chromium.org/11953054/diff/5001/ui/gfx/screen_win.cc
File ui/gfx/screen_win.cc (right):

https://codereview.chromium.org/11953054/diff/5001/ui/gfx/screen_win.cc#newco...
ui/gfx/screen_win.cc:92: #if !defined(ENABLE_HIDPI)
Nit.
This is up to you but you probably want a TODO here with the effect wanting to
have the DCHECK once all of Windows and not just the renderer are DIP aware.

https://codereview.chromium.org/11953054/diff/5001/ui/surface/accelerated_sur...
File ui/surface/accelerated_surface_win.cc (left):

https://codereview.chromium.org/11953054/diff/5001/ui/surface/accelerated_sur...
ui/surface/accelerated_surface_win.cc:691: 
Nit: Keep the extra line

[pkotwicz, 2013-01-23 19:40:08]

LGTM with nits

[kevers, 2013-01-23 20:02:19]

https://codereview.chromium.org/11953054/diff/1021/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

https://codereview.chromium.org/11953054/diff/1021/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_win.cc:611: gfx::Rect
RenderWidgetHostViewWin::getPixelBounds() const {
On 2013/01/23 18:13:12, pkotwicz wrote:
>  Nit:GetPixelBounds()

Done.

https://codereview.chromium.org/11953054/diff/1021/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_win.cc:2746:
gfx::Point(static_cast<short>(LOWORD(lparam)),
On 2013/01/23 18:13:12, pkotwicz wrote:
> Nit: alignment, 4 spaces

Done.

https://codereview.chromium.org/11953054/diff/1021/ui/base/resource/resource_...
File ui/base/resource/resource_bundle_win.cc (right):

https://codereview.chromium.org/11953054/diff/1021/ui/base/resource/resource_...
ui/base/resource/resource_bundle_win.cc:47: use_hidpi = ui::GetDPIScale() > 1.2;
On 2013/01/23 18:13:12, pkotwicz wrote:
> For the purpose of this CL, you actually do not need the changes in
> ResourceBundle
> 
> Nit: You can define scale depending on whether HIDPI is enabled.
> 
> Something like this:
> #if defined(ENABLE_HIDPI)
> float scale = ui::GetDPIScale();
> #else
> float scale = 1.0f;
> #endif

Backing this change out for the time being.  Will keep feedback in mind when
reintroducing resource handling for High DPI.

https://codereview.chromium.org/11953054/diff/1021/ui/base/resource/resource_...
ui/base/resource/resource_bundle_win.cc:56: } else {
On 2013/01/23 18:13:12, pkotwicz wrote:
> Nit: Put a comment that we will be using 1.4x resources for 1.25 and 1.5 scale
> factors.
> This is so that people like me who are unfamiliar with windows code are not
> asking "Why are you not checking for (scale == 1.4f) ?"

Backing out change for now.

[kevers, 2013-01-23 21:37:00]

https://codereview.chromium.org/11953054/diff/5001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

https://codereview.chromium.org/11953054/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_win.cc:302: 
On 2013/01/23 19:39:23, pkotwicz wrote:
> Pass in NativeViewId here instead of HWND
> 
> Nit: Get rid of the extra line.

Done.

https://codereview.chromium.org/11953054/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_win.cc:831:
GetScreenInfoForWindow(results, 0);
On 2013/01/23 19:39:23, pkotwicz wrote:
> This should be GetNativeViewId() instead of 0.

Done.

https://codereview.chromium.org/11953054/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_win.cc:837:
GetScreenInfoForWindow(results, NULL);
On 2013/01/23 19:39:23, pkotwicz wrote:
> This should probably be 0 instead of NULL.

Done.

https://codereview.chromium.org/11953054/diff/5001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_win.h (right):

https://codereview.chromium.org/11953054/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_win.h:216: virtual
gfx::Rect GetBoundsInRootWindow() OVERRIDE;
On 2013/01/23 19:39:23, pkotwicz wrote:
> Move GetScreenInfo() here and in .cc file right above GetBoundsInRootWindow()
to
> match order in render_widget_host_view_port.h

Done.

https://codereview.chromium.org/11953054/diff/5001/ui/base/win/dpi.cc
File ui/base/win/dpi.cc (right):

https://codereview.chromium.org/11953054/diff/5001/ui/base/win/dpi.cc#newcode76
ui/base/win/dpi.cc:76: return gfx::ToFlooredRectDeprecated(
On 2013/01/23 19:39:23, pkotwicz wrote:
> Nit: Add TODO to use gfx::ToNearestRect().

Based on description of method, ToNearestRect might not be the correct choice. 
Added TODO to choose a more appropriate method for float to int conversions.

https://codereview.chromium.org/11953054/diff/5001/ui/base/win/dpi.h
File ui/base/win/dpi.h (right):

https://codereview.chromium.org/11953054/diff/5001/ui/base/win/dpi.h#newcode26
ui/base/win/dpi.h:26: // they are also Windows specific.
On 2013/01/23 19:39:23, pkotwicz wrote:
> Nit.
> How about: "TODO(kevers|girard): Move above methods into win namespace."

Done.

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

https://codereview.chromium.org/11953054/diff/5001/ui/gfx/display.cc#newcode32
ui/gfx/display.cc:32: #if defined(OS_WIN)
On 2013/01/23 19:39:23, pkotwicz wrote:
> I think that this block of code should be in GetDeviceScaleFactorImpl() in
> "dpi.cc"

Done.

https://codereview.chromium.org/11953054/diff/5001/ui/gfx/display.cc#newcode52
ui/gfx/display.cc:52: //static
On 2013/01/23 19:39:23, pkotwicz wrote:
> Nit: newline before static

Done.

https://codereview.chromium.org/11953054/diff/5001/ui/gfx/display.h
File ui/gfx/display.h (right):

https://codereview.chromium.org/11953054/diff/5001/ui/gfx/display.h#newcode30
ui/gfx/display.h:30: static bool HasForceDeviceScaleFactor();
On 2013/01/23 19:39:23, pkotwicz wrote:
> Add a comment for HasForceDeviceScaleFactor().

Done.

https://codereview.chromium.org/11953054/diff/5001/ui/gfx/screen_win.cc
File ui/gfx/screen_win.cc (right):

https://codereview.chromium.org/11953054/diff/5001/ui/gfx/screen_win.cc#newco...
ui/gfx/screen_win.cc:92: #if !defined(ENABLE_HIDPI)
On 2013/01/23 19:39:23, pkotwicz wrote:
> Nit.
> This is up to you but you probably want a TODO here with the effect wanting to
> have the DCHECK once all of Windows and not just the renderer are DIP aware.

Done.

https://codereview.chromium.org/11953054/diff/5001/ui/surface/accelerated_sur...
File ui/surface/accelerated_surface_win.cc (left):

https://codereview.chromium.org/11953054/diff/5001/ui/surface/accelerated_sur...
ui/surface/accelerated_surface_win.cc:691: 
On 2013/01/23 19:39:23, pkotwicz wrote:
> Nit: Keep the extra line

Done.

[kevers, 2013-01-23 21:38:02]

Hi Ben,

Can you please take a look at this CL.

[kevers, 2013-01-28 15:52:31]

+sky for ui OWNERS
+darin for content OWNERS

[sky, 2013-01-28 20:56:35]

https://codereview.chromium.org/11953054/diff/3002/ui/base/win/dpi.cc
File ui/base/win/dpi.cc (right):

https://codereview.chromium.org/11953054/diff/3002/ui/base/win/dpi.cc#newcode22
ui/base/win/dpi.cc:22: #if defined(ENABLE_HIDPI)
Why the ifdef?

https://codereview.chromium.org/11953054/diff/3002/ui/base/win/dpi.cc#newcode73
ui/base/win/dpi.cc:73: static const float device_scale_factor =
GetDeviceScaleFactorImpl();
Why do we need to cache this?

https://codereview.chromium.org/11953054/diff/3002/ui/base/win/dpi.cc#newcode76
ui/base/win/dpi.cc:76: 
nit: remove one newline.

https://codereview.chromium.org/11953054/diff/3002/ui/surface/accelerated_sur...
File ui/surface/accelerated_surface_win.cc (right):

https://codereview.chromium.org/11953054/diff/3002/ui/surface/accelerated_sur...
ui/surface/accelerated_surface_win.cc:483: gfx::Size dip_size =
ui::win::ScreenToDIPSize(window_size);
Does this ifdef mean GetWindowSize has different meanings on aura and windows?

[kevers, 2013-01-28 22:53:40]

https://codereview.chromium.org/11953054/diff/3002/ui/base/win/dpi.cc
File ui/base/win/dpi.cc (right):

https://codereview.chromium.org/11953054/diff/3002/ui/base/win/dpi.cc#newcode22
ui/base/win/dpi.cc:22: #if defined(ENABLE_HIDPI)
On 2013/01/28 20:56:35, sky wrote:
> Why the ifdef?
 
Removing the ifdef would expose the forced device scale factor flag on Windows
regardless of whether high-DPI is enabled.  If we are OK with this, I can
certainly remove the flag, as GetDPIScale() returns 1 unless the process is
DPI-aware, which in turn, is behind the ENABLE_HIDPI flag.

https://codereview.chromium.org/11953054/diff/3002/ui/base/win/dpi.cc#newcode73
ui/base/win/dpi.cc:73: static const float device_scale_factor =
GetDeviceScaleFactorImpl();
On 2013/01/28 20:56:35, sky wrote:
> Why do we need to cache this?

See this caching pattern used in several places where we need to do a
non-trivial amount of work to retrieve a value that doesn't change.  In this
case, without the cache, we need to check the command line on each call, which
seems somewhat wasteful.

https://codereview.chromium.org/11953054/diff/3002/ui/surface/accelerated_sur...
File ui/surface/accelerated_surface_win.cc (right):

https://codereview.chromium.org/11953054/diff/3002/ui/surface/accelerated_sur...
ui/surface/accelerated_surface_win.cc:483: gfx::Size dip_size =
ui::win::ScreenToDIPSize(window_size);
On 2013/01/28 20:56:35, sky wrote:
> Does this ifdef mean GetWindowSize has different meanings on aura and windows?

The size mismatch on Windows with high-DPI enabled results from truncation
errors when converting between pixel and DIP.  Requiring an exact size match
resulted in dropping all frames when the size in pixels is not an integer
multiple of the size in DIP.  A mismatch is far more common with the fractional
scale factors for Windows (1.4 and 1.8 for Win8 and 1.25 and 1.5 for Win7).

Added the high-DPI specific test in order to get some benefit of frame dropping
to make a rapid resize feel smoother.

The method GetWindowSize returns the size in the client's coordinate space. 
Under views, with high-DPI enabled, it maps to screen pixels.

Not clear why we needed to drop the test on Windows-Aura.

[sky, 2013-01-29 15:34:55]

https://codereview.chromium.org/11953054/diff/3002/ui/base/win/dpi.cc
File ui/base/win/dpi.cc (right):

https://codereview.chromium.org/11953054/diff/3002/ui/base/win/dpi.cc#newcode22
ui/base/win/dpi.cc:22: #if defined(ENABLE_HIDPI)
On 2013/01/28 22:53:40, kevers wrote:
> On 2013/01/28 20:56:35, sky wrote:
> > Why the ifdef?
>  
> Removing the ifdef would expose the forced device scale factor flag on Windows
> regardless of whether high-DPI is enabled.  If we are OK with this, I can
> certainly remove the flag, as GetDPIScale() returns 1 unless the process is
> DPI-aware, which in turn, is behind the ENABLE_HIDPI flag.

Why wouldn't we do this? I assume we're going to nuke the ifdef eventually,
right?

[kevers, 2013-01-29 17:08:28]

https://chromiumcodereview.appspot.com/11953054/diff/3002/ui/base/win/dpi.cc
File ui/base/win/dpi.cc (right):

https://chromiumcodereview.appspot.com/11953054/diff/3002/ui/base/win/dpi.cc#...
ui/base/win/dpi.cc:22: #if defined(ENABLE_HIDPI)
On 2013/01/29 15:34:56, sky wrote:
> On 2013/01/28 22:53:40, kevers wrote:
> > On 2013/01/28 20:56:35, sky wrote:
> > > Why the ifdef?
> >  
> > Removing the ifdef would expose the forced device scale factor flag on
Windows
> > regardless of whether high-DPI is enabled.  If we are OK with this, I can
> > certainly remove the flag, as GetDPIScale() returns 1 unless the process is
> > DPI-aware, which in turn, is behind the ENABLE_HIDPI flag.
> 
> Why wouldn't we do this? I assume we're going to nuke the ifdef eventually,
> right?

Yes, ENABLE_HIDPI ifdefs are going away once the feature is stable.  Tested with
this particular ifdef removed and high-DPI disabled. Seeing strange rendering if
a forced scale factor is explicitly set, but behaves as expected if setting an
OS-level scale factor with fonts set to 180% (large blurry text).

https://chromiumcodereview.appspot.com/11953054/diff/3002/ui/base/win/dpi.cc#...
ui/base/win/dpi.cc:76: 
On 2013/01/28 20:56:35, sky wrote:
> nit: remove one newline.

Done.

[sky, 2013-01-29 20:39:56]

If the feature isn't ready, the ifdef is fine by me. I just wanted to
understand why it was there and the long term plan.

  -Scott

On Tue, Jan 29, 2013 at 9:08 AM,  <kevers@chromium.org> wrote:
>
> https://chromiumcodereview.appspot.com/11953054/diff/3002/ui/base/win/dpi.cc
> File ui/base/win/dpi.cc (right):
>
>
https://chromiumcodereview.appspot.com/11953054/diff/3002/ui/base/win/dpi.cc#...
> ui/base/win/dpi.cc:22: #if defined(ENABLE_HIDPI)
>
> On 2013/01/29 15:34:56, sky wrote:
>>
>> On 2013/01/28 22:53:40, kevers wrote:
>> > On 2013/01/28 20:56:35, sky wrote:
>> > > Why the ifdef?
>> >
>> > Removing the ifdef would expose the forced device scale factor flag
>
> on Windows
>>
>> > regardless of whether high-DPI is enabled.  If we are OK with this,
>
> I can
>>
>> > certainly remove the flag, as GetDPIScale() returns 1 unless the
>
> process is
>>
>> > DPI-aware, which in turn, is behind the ENABLE_HIDPI flag.
>
>
>> Why wouldn't we do this? I assume we're going to nuke the ifdef
>
> eventually,
>>
>> right?
>
>
> Yes, ENABLE_HIDPI ifdefs are going away once the feature is stable.
> Tested with this particular ifdef removed and high-DPI disabled. Seeing
> strange rendering if a forced scale factor is explicitly set, but
> behaves as expected if setting an OS-level scale factor with fonts set
> to 180% (large blurry text).
>
>
https://chromiumcodereview.appspot.com/11953054/diff/3002/ui/base/win/dpi.cc#...
> ui/base/win/dpi.cc:76:
>
> On 2013/01/28 20:56:35, sky wrote:
>>
>> nit: remove one newline.
>
>
> Done.
>
> https://chromiumcodereview.appspot.com/11953054/

[kevers, 2013-01-29 21:36:55]

On 2013/01/29 20:39:56, sky wrote:
> If the feature isn't ready, the ifdef is fine by me. I just wanted to
> understand why it was there and the long term plan.
> 
>   -Scott
> 

Reinserted.

Long-term, high-DPI will only be available for Windows 7 and 8.  Can replace the
#ifdef with a check for the process being DPI-aware.  Tough fairly
straightforward, I'd rather tackle this in a separate CL.

[sky, 2013-01-29 22:36:08]

LGTM

[darin (slow to review), 2013-01-30 21:39:12]

https://chromiumcodereview.appspot.com/11953054/diff/31001/content/browser/re...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

https://chromiumcodereview.appspot.com/11953054/diff/31001/content/browser/re...
content/browser/renderer_host/render_widget_host_view_win.cc:310:
results->deviceScaleFactor = ui::win::GetDeviceScaleFactor();
Wouldn't it be better to provide ui::win::GetDeviceScaleFactor() when
ENABLE_HIDPI is not defined?  Just make it return 1.  Then, you can probably
have less #ifdef'ing in the code, right?

https://chromiumcodereview.appspot.com/11953054/diff/31001/content/browser/re...
content/browser/renderer_host/render_widget_host_view_win.cc:2748: #if
defined(ENABLE_HIDPI)
Why do we need ENABLE_HIDPI?  Why not just have all of the math equate to
identify functions when not in HI-DPI mode?

https://chromiumcodereview.appspot.com/11953054/diff/31001/ui/base/win/dpi.cc
File ui/base/win/dpi.cc (right):

https://chromiumcodereview.appspot.com/11953054/diff/31001/ui/base/win/dpi.cc...
ui/base/win/dpi.cc:72: float GetDeviceScaleFactor() {
What happens in the case of multiple monitors, each having different device
scale factors?

[kevers, 2013-01-30 22:27:01]

https://chromiumcodereview.appspot.com/11953054/diff/31001/content/browser/re...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

https://chromiumcodereview.appspot.com/11953054/diff/31001/content/browser/re...
content/browser/renderer_host/render_widget_host_view_win.cc:310:
results->deviceScaleFactor = ui::win::GetDeviceScaleFactor();
On 2013/01/30 21:39:12, darin wrote:
> Wouldn't it be better to provide ui::win::GetDeviceScaleFactor() when
> ENABLE_HIDPI is not defined?  Just make it return 1.  Then, you can probably
> have less #ifdef'ing in the code, right?

Removed the #ifdef, as GetDeviceScaleFactor always returns 1 when high-DPI is
not enabled.

https://chromiumcodereview.appspot.com/11953054/diff/31001/content/browser/re...
content/browser/renderer_host/render_widget_host_view_win.cc:2748: #if
defined(ENABLE_HIDPI)
On 2013/01/30 21:39:12, darin wrote:
> Why do we need ENABLE_HIDPI?  Why not just have all of the math equate to
> identify functions when not in HI-DPI mode?

The ScreenToDPIPoint is an identity function when not in high-DPI mode.  Was
erring on the side of caution for a feature that is currently in development. 
Can remove the #ifdef.

https://chromiumcodereview.appspot.com/11953054/diff/31001/ui/base/win/dpi.cc
File ui/base/win/dpi.cc (right):

https://chromiumcodereview.appspot.com/11953054/diff/31001/ui/base/win/dpi.cc...
ui/base/win/dpi.cc:72: float GetDeviceScaleFactor() {
On 2013/01/30 21:39:12, darin wrote:
> What happens in the case of multiple monitors, each having different device
> scale factors?

Under Windows, all monitors use the same scale factor.  The DPI value (used to
compute the scale factor) is set to a constant value in the OS and will remain
constant for the life of the application (requires logging out for a change to
take effect).

[darin (slow to review), 2013-01-30 22:58:42]

On Wed, Jan 30, 2013 at 2:27 PM, <kevers@chromium.org> wrote:

>
> https://chromiumcodereview.**appspot.com/11953054/diff/**
>
31001/content/browser/**renderer_host/render_widget_**host_view_win.cc<https://chromiumcodereview.appspot.com/11953054/diff/31001/content/browser/renderer_host/render_widget_host_view_win.cc>
> File content/browser/renderer_host/**render_widget_host_view_win.cc
> (right):
>
> https://chromiumcodereview.**appspot.com/11953054/diff/**
> 31001/content/browser/**renderer_host/render_widget_**
>
host_view_win.cc#newcode310<https://chromiumcodereview.appspot.com/11953054/diff/31001/content/browser/renderer_host/render_widget_host_view_win.cc#newcode310>
> content/browser/renderer_host/**render_widget_host_view_win.**cc:310:
> results->deviceScaleFactor = ui::win::GetDeviceScaleFactor(**);
> On 2013/01/30 21:39:12, darin wrote:
>
>> Wouldn't it be better to provide ui::win::GetDeviceScaleFactor(**) when
>> ENABLE_HIDPI is not defined?  Just make it return 1.  Then, you can
>>
> probably
>
>> have less #ifdef'ing in the code, right?
>>
>
> Removed the #ifdef, as GetDeviceScaleFactor always returns 1 when
> high-DPI is not enabled.
>
>
> https://chromiumcodereview.**appspot.com/11953054/diff/**
> 31001/content/browser/**renderer_host/render_widget_**
>
host_view_win.cc#newcode2748<https://chromiumcodereview.appspot.com/11953054/diff/31001/content/browser/renderer_host/render_widget_host_view_win.cc#newcode2748>
> content/browser/renderer_host/**render_widget_host_view_win.**cc:2748: #if
> defined(ENABLE_HIDPI)
> On 2013/01/30 21:39:12, darin wrote:
>
>> Why do we need ENABLE_HIDPI?  Why not just have all of the math equate
>>
> to
>
>> identify functions when not in HI-DPI mode?
>>
>
> The ScreenToDPIPoint is an identity function when not in high-DPI mode.
> Was erring on the side of caution for a feature that is currently in
> development.  Can remove the #ifdef.
>
>
> https://chromiumcodereview.**appspot.com/11953054/diff/**
>
31001/ui/base/win/dpi.cc<https://chromiumcodereview.appspot.com/11953054/diff/31001/ui/base/win/dpi.cc>
> File ui/base/win/dpi.cc (right):
>
> https://chromiumcodereview.**appspot.com/11953054/diff/**
>
31001/ui/base/win/dpi.cc#**newcode72<https://chromiumcodereview.appspot.com/11953054/diff/31001/ui/base/win/dpi.cc#newcode72>
> ui/base/win/dpi.cc:72: float GetDeviceScaleFactor() {
> On 2013/01/30 21:39:12, darin wrote:
>
>> What happens in the case of multiple monitors, each having different
>>
> device
>
>> scale factors?
>>
>
> Under Windows, all monitors use the same scale factor.  The DPI value
> (used to compute the scale factor) is set to a constant value in the OS
> and will remain constant for the life of the application (requires
> logging out for a change to take effect).
>

That's very helpful!

-Darin


>
>
https://chromiumcodereview.**appspot.com/11953054/<https://chromiumcodereview...
>

[darin (slow to review), 2013-01-30 23:24:31]

LGTM for content/

https://chromiumcodereview.appspot.com/11953054/diff/34001/content/browser/re...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

https://chromiumcodereview.appspot.com/11953054/diff/34001/content/browser/re...
content/browser/renderer_host/render_widget_host_view_win.cc:2397: 
nit: the comment seems a bit redundant

https://chromiumcodereview.appspot.com/11953054/diff/34001/content/browser/re...
content/browser/renderer_host/render_widget_host_view_win.cc:2744: // Convert to
DIP
nit: comment seems redundant

[kevers, 2013-01-31 13:50:20]

https://chromiumcodereview.appspot.com/11953054/diff/34001/content/browser/re...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

https://chromiumcodereview.appspot.com/11953054/diff/34001/content/browser/re...
content/browser/renderer_host/render_widget_host_view_win.cc:2397: 
On 2013/01/30 23:24:31, darin wrote:
> nit: the comment seems a bit redundant

Removed comment.

https://chromiumcodereview.appspot.com/11953054/diff/34001/content/browser/re...
content/browser/renderer_host/render_widget_host_view_win.cc:2744: // Convert to
DIP
On 2013/01/30 23:24:31, darin wrote:
> nit: comment seems redundant

Removed comment.

[commit-bot: I haz the power, 2013-01-31 13:50:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kevers@chromium.org/11953054/34002

[commit-bot: I haz the power, 2013-01-31 17:24:44]

Change committed as 179899