NOT FOR SUBMIT - Windows Views HiDPI

NOT FOR SUBMIT - Windows Views HiDPI

Work in progress... not intended for submit.

BUG=149881

[kevers, 2013-04-19 20:31:41]

https://codereview.chromium.org/14348033/diff/1016/content/renderer/render_wi...
File content/renderer/render_widget.cc (left):

https://codereview.chromium.org/14348033/diff/1016/content/renderer/render_wi...
content/renderer/render_widget.cc:1193: gfx::ScaleRect(bounds,
device_scale_factor_));
Will need to test in Win-Aura with non-integer scales to ensure that we don't
break the Aura-backing store.  Having said that, I believe that enclosing rect
is a bit dangerous to use as it can be 1 pixel larger than the ceiled size.

https://codereview.chromium.org/14348033/diff/1016/ui/gfx/canvas_paint_win.h
File ui/gfx/canvas_paint_win.h (left):

https://codereview.chromium.org/14348033/diff/1016/ui/gfx/canvas_paint_win.h#...
ui/gfx/canvas_paint_win.h:1: // Copyright (c) 2011 The Chromium Authors. All
rights reserved.
CRLF issue?

https://codereview.chromium.org/14348033/diff/1016/ui/gfx/rect_conversions.cc
File ui/gfx/rect_conversions.cc (right):

https://codereview.chromium.org/14348033/diff/1016/ui/gfx/rect_conversions.cc...
ui/gfx/rect_conversions.cc:14: Rect ToSizedRect(const RectF& rect) {
This logic is identical to ToFlooredRectDeprecated, which is typically not
appropriate for non-integer scale factors.  Non-integer-scaled assets use ceiled
size. If this method is to address issues with off-by-one errors in the
dimensions of the backing store, then consider using ToFlooredPoint combined
with ToCeiledSize.  Rect has a constructor Rect(Point,Size).  Consider using it
to achieve a more compact representation of the scaling operation.

https://codereview.chromium.org/14348033/diff/1016/ui/gfx/skbitmap_operations.cc
File ui/gfx/skbitmap_operations.cc (right):

https://codereview.chromium.org/14348033/diff/1016/ui/gfx/skbitmap_operations...
ui/gfx/skbitmap_operations.cc:136: #if !defined(ENABLE_HIDPI)
These changes should not be necessary.  The rounding rules have been adjusted
for non-integer scale factors.  These are still complaints on some 180P assets,
but should be resolved by using assets of the expected size.  The rgb and alpha
images should be identical in size.  Verified in Win-Aura with non-integer
scales.

https://codereview.chromium.org/14348033/diff/1016/ui/views/controls/native/n...
File ui/views/controls/native/native_view_host.cc (left):

https://codereview.chromium.org/14348033/diff/1016/ui/views/controls/native/n...
ui/views/controls/native/native_view_host.cc:2: // Use of this source code is
governed by a BSD-style license that can be
CRLF issue?

https://codereview.chromium.org/14348033/diff/1016/ui/views/widget/native_wid...
File ui/views/widget/native_widget_win.cc (right):

https://codereview.chromium.org/14348033/diff/1016/ui/views/widget/native_wid...
ui/views/widget/native_widget_win.cc:103: // TODO: Consider scaling to screen
dpi.
AdjustUIFont should already be adjusting the size to account for DIP.

https://codereview.chromium.org/14348033/diff/1016/ui/views/win/hwnd_message_...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/14348033/diff/1016/ui/views/win/hwnd_message_...
ui/views/win/hwnd_message_handler.cc:524: *bounds =
ui::win::ScreenToDIPRect(*bounds);
As it stands, this change will break Win-Aura, which does the transformation in
the caller of this method.

[enne (OOO), 2013-04-19 20:38:07]

https://codereview.chromium.org/14348033/diff/1016/cc/layers/scrollbar_layer.cc
File cc/layers/scrollbar_layer.cc (right):

https://codereview.chromium.org/14348033/diff/1016/cc/layers/scrollbar_layer....
cc/layers/scrollbar_layer.cc:364: return gfx::ToSizedRect(content_rect);
Why do you want to round down the size here?

[kevers, 2013-06-03 17:28:50]

https://codereview.chromium.org/14348033/diff/27025/chrome/browser/thumbnails...
File chrome/browser/thumbnails/simple_thumbnail_crop.cc (right):

https://codereview.chromium.org/14348033/diff/27025/chrome/browser/thumbnails...
chrome/browser/thumbnails/simple_thumbnail_crop.cc:132: copy_size,
ui::GetScaleFactorScale(ui::SCALE_FACTOR_140P)));
ui::GetMaxScaleFactor() in place of the if-else?

https://codereview.chromium.org/14348033/diff/27025/chrome/browser/ui/views/t...
File chrome/browser/ui/views/tabs/dragged_tab_view.cc (right):

https://codereview.chromium.org/14348033/diff/27025/chrome/browser/ui/views/t...
chrome/browser/ui/views/tabs/dragged_tab_view.cc:84: #if defined(OS_WIN)
Has this change been tested on Aura?  If for non-Aura Windows, perhaps it should
be moved into the following conditional block.

https://codereview.chromium.org/14348033/diff/27025/chrome/browser/ui/views/t...
File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right):

https://codereview.chromium.org/14348033/diff/27025/chrome/browser/ui/views/t...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:223: #if defined(OS_WIN)
Looks like we're mixing pixels and DIP in arguments to the method.  If this is
so, then maybe we should be adjusting the caller. Looks very error prone to
leave expecting mixed coordinates.  Has this been tested on Win Aura?

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

https://codereview.chromium.org/14348033/diff/27025/content/renderer/render_w...
content/renderer/render_widget.cc:1240: gfx::Rect pixel_bounds = gfx::Rect(
Looks like the necessary backing store changes are missing from this CL. The
bounds should not be updated independently of the backing stores. Suggest a
separate CL which adjusts for Windows and Aura so that the sizing is kept in
sync.

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

https://codereview.chromium.org/14348033/diff/27025/ui/base/win/dpi.cc#newcode97
ui/base/win/dpi.cc:97: // TODO(kevers): Switch to non-deprecated method for
float to int conversions.
Misleading comment.  ToFlooredPoint is not deprecated, only ToFlooredRect. 
Please remove the comment.

https://codereview.chromium.org/14348033/diff/27025/ui/gfx/skbitmap_operation...
File ui/gfx/skbitmap_operations.cc (right):

https://codereview.chromium.org/14348033/diff/27025/ui/gfx/skbitmap_operation...
ui/gfx/skbitmap_operations.cc:147: std::min(rgb.height(), alpha.height()), 0);
Is this change necessary?  Recent changes to the rounding logic at non-integer
scale factors have cleared up many of the problems with size misalignment. UX
should be providing resources in the correct size.

https://codereview.chromium.org/14348033/diff/27025/ui/surface/accelerated_su...
File ui/surface/accelerated_surface_win.cc (right):

https://codereview.chromium.org/14348033/diff/27025/ui/surface/accelerated_su...
ui/surface/accelerated_surface_win.cc:733: #if !defined(ENABLE_HIDPI)
As we are also working on removing the ENABLE_HIDPI flag and replacing with a
runtime flag, it probably makes sense to avoid adding new instances of the
compile time flag.  GetDeviceScaleFactor() returns 1 if high DPI is not enabled.
 Can probably change to early return to check if the scale factor is 1.

https://codereview.chromium.org/14348033/diff/27025/ui/views/win/hwnd_message...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/14348033/diff/27025/ui/views/win/hwnd_message...
ui/views/win/hwnd_message_handler.cc:1162: // We need to clip to the dirty rect
ourselves.
Tested on Aura?

https://codereview.chromium.org/14348033/diff/27025/ui/views/win/hwnd_message...
File ui/views/win/hwnd_message_handler.h (right):

https://codereview.chromium.org/14348033/diff/27025/ui/views/win/hwnd_message...
ui/views/win/hwnd_message_handler.h:78: void SetBoundsInPixel(const gfx::Rect&
bounds);
Tested on Aura?  I suspect this change will break the compile as SetBounds is
called.

[girard, 2013-06-03 22:31:55]

Thanks for the feedback - I've incorporated your suggestions into the code, and
I'm now going to start breaking the code into smaller CL's for commit.  There
are (at least) two outstanding issues at this point:
1) Backing store seems to be incorrectly sized, especially wrt the new tab page.
2) I need to test much of this code with and without aura, and high dpi enabled.

https://codereview.chromium.org/14348033/diff/1016/cc/layers/scrollbar_layer.cc
File cc/layers/scrollbar_layer.cc (right):

https://codereview.chromium.org/14348033/diff/1016/cc/layers/scrollbar_layer....
cc/layers/scrollbar_layer.cc:364: return gfx::ToSizedRect(content_rect);
On 2013/04/19 20:38:07, enne wrote:
> Why do you want to round down the size here?

This line slipped in from another patch I was evaluating.... doesn't belong here
- good catch.

https://codereview.chromium.org/14348033/diff/1016/ui/views/win/hwnd_message_...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/14348033/diff/1016/ui/views/win/hwnd_message_...
ui/views/win/hwnd_message_handler.cc:524: *bounds =
ui::win::ScreenToDIPRect(*bounds);
On 2013/04/19 20:31:42, kevers wrote:
> As it stands, this change will break Win-Aura, which does the transformation
in
> the caller of this method.

I've moved this up to the caller (NativeWidgetWin::GetWindowPlacement) but I
can't find the corresponding conversion in NativeWidgetAura. Where should I be
looking?

https://codereview.chromium.org/14348033/diff/27025/chrome/browser/thumbnails...
File chrome/browser/thumbnails/simple_thumbnail_crop.cc (right):

https://codereview.chromium.org/14348033/diff/27025/chrome/browser/thumbnails...
chrome/browser/thumbnails/simple_thumbnail_crop.cc:132: copy_size,
ui::GetScaleFactorScale(ui::SCALE_FACTOR_140P)));
On 2013/06/03 17:28:50, kevers wrote:
> ui::GetMaxScaleFactor() in place of the if-else?
> 

Good idea. Done.

https://codereview.chromium.org/14348033/diff/27025/chrome/browser/ui/views/t...
File chrome/browser/ui/views/tabs/dragged_tab_view.cc (right):

https://codereview.chromium.org/14348033/diff/27025/chrome/browser/ui/views/t...
chrome/browser/ui/views/tabs/dragged_tab_view.cc:84: #if defined(OS_WIN)
On 2013/06/03 17:28:50, kevers wrote:
> Has this change been tested on Aura?  If for non-Aura Windows, perhaps it
should
> be moved into the following conditional block.

Done.

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

https://codereview.chromium.org/14348033/diff/27025/ui/base/win/dpi.cc#newcode97
ui/base/win/dpi.cc:97: // TODO(kevers): Switch to non-deprecated method for
float to int conversions.
On 2013/06/03 17:28:50, kevers wrote:
> Misleading comment.  ToFlooredPoint is not deprecated, only ToFlooredRect. 
> Please remove the comment.

Done.

https://codereview.chromium.org/14348033/diff/27025/ui/gfx/skbitmap_operation...
File ui/gfx/skbitmap_operations.cc (right):

https://codereview.chromium.org/14348033/diff/27025/ui/gfx/skbitmap_operation...
ui/gfx/skbitmap_operations.cc:147: std::min(rgb.height(), alpha.height()), 0);
On 2013/06/03 17:28:50, kevers wrote:
> Is this change necessary?  Recent changes to the rounding logic at non-integer
> scale factors have cleared up many of the problems with size misalignment. UX
> should be providing resources in the correct size.

This prevents tearing in the new tab page. We've still got an alignment problem
(but I agree that this isn't the correct place to correct it.)

https://codereview.chromium.org/14348033/diff/27025/ui/views/win/hwnd_message...
File ui/views/win/hwnd_message_handler.h (right):

https://codereview.chromium.org/14348033/diff/27025/ui/views/win/hwnd_message...
ui/views/win/hwnd_message_handler.h:78: void SetBoundsInPixel(const gfx::Rect&
bounds);
On 2013/06/03 17:28:50, kevers wrote:
> Tested on Aura?  I suspect this change will break the compile as SetBounds is
> called.

This change isn't necessary. I'm turfing it out (and adding an appropriate
comment.)