Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
924853002:
linux/x11: Fix setting window bounds in High DPI. (Closed)
Descriptionlinux/x11: Fix window positioning/sizing in High DPI.
Make the appropriate conversions between pixel-space and dip-space
coordinates/sizes depending on the display's device scale factor.
BUG=400837
R=pkotwicz@chromium.org, sky@chromium.org
Committed: https://chromium.googlesource.com/chromium/src/+/b19dc5bf24871e40e986f0f024946b1c281e77c4
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #
Total comments: 4
Patch Set 4 : . #
Total comments: 27
Patch Set 5 : . #
Messages
Total messages: 24 (4 generated)
sadrul@chromium.org changed reviewers: + sky@chromium.org
pkotwicz@chromium.org changed reviewers: + pkotwicz@chromium.org
This seems to be similar to what we do on Desktop Windows. However, I wonder whether we can use WindowTreeHost::ConvertPointToHost() & al. From a quick look, it seems like it should work
On 2015/02/13 19:23:20, pkotwicz wrote: > This seems to be similar to what we do on Desktop Windows. However, I wonder > whether we can use WindowTreeHost::ConvertPointToHost() & al. From a quick look, > it seems like it should work We will need some way to convert a rect, and I don't believe we have those in WindowTreeHost.
sadrul@chromium.org changed reviewers: + erg@chromium.org
+erg@ Peter: Mind reviewing the whole CL?
I would suggest adding methods to do rect conversion to WindowTreeHost
On 2015/02/18 15:24:24, pkotwicz wrote: > I would suggest adding methods to do rect conversion to WindowTreeHost I disagree, since we don't need that for win-aura (or chromeos). We have enough burden in aura that is useful on only one platform and stubbed out in all the rest, let's not add yet more.
I think that the benefit of having less coordinate systems to think about outweighs the difficulty of adding conversion methods for rects from WindowTreeHost coordinates to RootWindow coordinates (You can then convert rects in root window coordinates to rects in screen coordinates.) We do have methods to convert rects between root window coordinates and screen coordinates but those methods are weirdly ash only. (ash::ScreenUtil::ConvertRectToScreen()) You are adding new conversion methods in this CL anyways... These methods could be used on desktop Windows too. I find the names of the methods in ui/gfx/win/dpi.h super confusing (You have to remember that the screen coordinates mentioned in those methods are different from what screen_position_client considers screen coordinates) Please ping me if you disagree. I am not trying to block your CL
On 2015/02/18 16:39:48, pkotwicz wrote: > I think that the benefit of having less coordinate systems to think about > outweighs the difficulty of adding conversion methods for rects from > WindowTreeHost coordinates to RootWindow coordinates (You can then convert rects > in root window coordinates to rects in screen coordinates.) How does adding the relevant methods in WindowTreeHost lets you have less coordinate systems to think about? The WindowTreeHost and DesktopWindowTreeHost interfaces are all in DIP space (except for SetBounds/GetBounds in WTH). We need to do the DIP<-->Pixel conversions at the point where we communicate with the native API (x11, or w32). The Windows implementation does that already, this change does the same for X11. The fact that we have point conversion methods on WindowTreeHost (and that SetBounds/GetBounds expect pixel-space) is perhaps a bit unfortunate. Maybe we should look at those more closely. > We do have methods > to convert rects between root window coordinates and screen coordinates but > those methods are weirdly ash only. (ash::ScreenUtil::ConvertRectToScreen()) From a quick look at that code, it either gives you the incorrect results (because it never transforms the size), and/or it gives you the result in 'DIP screen' coordinate (which unfortunately seems to be a thing we use in some places), which wouldn't be useful here. > > You are adding new conversion methods in this CL anyways... Yes, new methods confined specifically to desktop-X11, and not something that needs to be maintained for all other platforms as well. > These methods could be used on desktop Windows too. I find the names of the > methods in ui/gfx/win/dpi.h super confusing (You have to remember that the > screen coordinates mentioned in those methods are different from what > screen_position_client considers screen coordinates) Indeed. That's why I tried to be more explicit with 'Pixel' instead of 'Screen'.
I have updated the CL to have DesktopWindowTreeHostX11 override GetRootTransform() to return the right thing, and use that to do the rect transforms. PTAL.
All these conversions make me sad. LGTM
On 2015/02/18 22:32:33, sky wrote: > All these conversions make me sad. LGTM Sadness. Sadness everywhere. L"G"TM.
I didn't get through the entire CL. If you are changing DesktopWindowTreeHostX11::bounds_ to DesktopWindowTreeHostX11::bounds_in_pixels to indicate the coordinate system you should add the "in_pixels" suffix to all variables in desktop_window_tree_host_x11.cc which use the pixel coordinate system. (e.g. DesktopWindowTreeHostX11::previous_bounds_ should also have the "in_pixels" suffix). I would advise to add a suffix to all dimensions which ARE NOT in the pixel coordinate space. That seems slightly more sane FYI: I am going to delegate approving the actual name of the coordinate system to Oshima@. I think he is the owner of coordinate system names. https://codereview.chromium.org/924853002/diff/40001/ui/views/widget/desktop_... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): https://codereview.chromium.org/924853002/diff/40001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:854: gfx::Transform DesktopWindowTreeHostX11::GetRootTransform() const { Nit: I would be tempted to query the device scale factor of the primary display. https://codereview.chromium.org/924853002/diff/40001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:859: bounds_in_pixels_.origin()); tldr: If you plan on making ToPixelRect() / ToDIPRect() public in a subsequent CL, you might as well do it in this CL Aside & for another CL: I know that you are passing in |bounds_in_pixels_| to make gfx::Screen::GetDisplayNearestPoint() work but the screen.h specifies that the passed in bounds should be in DIP. We should have a plan for passing in bounds in the correct coordinates. One way of doing this would be to make ToPixelRect() / ToDIPRect() public and to use DesktopWindowTreeHostX11::GetHostForXID()
I think this CL needs more work as I described in comment #15 NLGTM
I think this CL needs more work as I described in comment #15 Not LGTM
On 2015/02/18 22:58:51, pkotwicz wrote: > I didn't get through the entire CL. If you are changing > DesktopWindowTreeHostX11::bounds_ to DesktopWindowTreeHostX11::bounds_in_pixels > to indicate the coordinate system you should add the "in_pixels" suffix to all > variables in desktop_window_tree_host_x11.cc which use the pixel coordinate > system. (e.g. DesktopWindowTreeHostX11::previous_bounds_ should also have the > "in_pixels" suffix). Done. > I would advise to add a suffix to all dimensions which ARE > NOT in the pixel coordinate space. That seems slightly more sane > > FYI: I am going to delegate approving the actual name of the coordinate system > to Oshima@. I think he is the owner of coordinate system names. As mentioned in an earlier comment, we use in_pixels throughout the codebase. This is not a new name that needs new approval. https://codereview.chromium.org/924853002/diff/40001/ui/views/widget/desktop_... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): https://codereview.chromium.org/924853002/diff/40001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:854: gfx::Transform DesktopWindowTreeHostX11::GetRootTransform() const { On 2015/02/18 22:58:50, pkotwicz wrote: > Nit: I would be tempted to query the device scale factor of the primary display. Done. https://codereview.chromium.org/924853002/diff/40001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:859: bounds_in_pixels_.origin()); On 2015/02/18 22:58:50, pkotwicz wrote: > tldr: If you plan on making ToPixelRect() / ToDIPRect() public in a subsequent > CL, you might as well do it in this CL > > Aside & for another CL: I know that you are passing in |bounds_in_pixels_| to > make gfx::Screen::GetDisplayNearestPoint() work but the screen.h specifies that > the passed in bounds should be in DIP. We should have a plan for passing in > bounds in the correct coordinates. > > One way of doing this would be to make ToPixelRect() / ToDIPRect() public > and to use DesktopWindowTreeHostX11::GetHostForXID() Changed to use DisplayNearestWindow() instead. > tldr: If you plan on making ToPixelRect() / ToDIPRect() public in a subsequent > CL, you might as well do it in this CL I don't have plans for that yet. I would like to avoid exposing these if possible, and we don't need them elsewhere yet.
New patchsets have been uploaded after l-g-t-m from sky@chromium.org
On 2015/02/18 22:53:51, Elliot Glaysher wrote: > On 2015/02/18 22:32:33, sky wrote: > > All these conversions make me sad. LGTM > > Sadness. Sadness everywhere. L"G"TM. Indeed. Something like http://crbug.com/417560 may make sense for //ui too. If we end up with something that's easy to use in blink, it might make sense to try to use a similar approach for //ui.
LGTM after you have addressed the comments Please update the .h file with the new parameter names (e.g. SetBounds()) We use "pixel coordinates" on Desktop Windows. We can use them here too. I have yet to understand how "pixel coordinates" differ from "host coordinates" and "native screen coordinates". All of the coordinate systems make sense to you, and I trust your judgement https://codereview.chromium.org/924853002/diff/60001/ui/views/widget/desktop_... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): https://codereview.chromium.org/924853002/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:421: transient_parent_rect.width() >= size_in_pixels.width()) { You meant to compare against |size| not |size_in_pixels|? https://codereview.chromium.org/924853002/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:434: window_bounds.AdjustToFit(parent_bounds_in_pixels); window_bounds -> window_bounds_in_pixels https://codereview.chromium.org/924853002/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:488: void DesktopWindowTreeHostX11::SetShape(gfx::NativeRegion native_region) { |native_region| is in "DIP" coordinates. I quickly tried ultra-violet and the bubble is not big enough. I am ok if you fix this as part of a follow on CL https://codereview.chromium.org/924853002/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:532: AdjustSize(bounds_in_pixels_.size())); adjusted_bounds -> adjusted_bounds_in_pixels https://codereview.chromium.org/924853002/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:859: display = gfx::Screen::GetNativeScreen()->GetDisplayNearestWindow(win); Nit: Why bother? We are going to use the scale factor of the primary display anyways. If the scale factors of the displays was different, perhaps we should do things differently when |window_mapped_| is false (I can see a future version of myself trying to figure out why we have special logic when |window_mapped_| is true) https://codereview.chromium.org/924853002/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:896: AdjustSize(requested_bounds_in_pixel.size())); bounds -> bounds_in_pixels https://codereview.chromium.org/924853002/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:911: gfx::Size size = bounds.size(); size -> size_in_pixels https://codereview.chromium.org/924853002/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1068: AdjustSize(bounds_in_pixels_.size())); Nit: bounds_in_pixels_.set_size(AdjustSize(bounds_in_pixels_.size())); https://codereview.chromium.org/924853002/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1225: gfx::Size size = requested_size_in_pixels; size -> size_in_pixels https://codereview.chromium.org/924853002/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1314: ToPixelRect(gfx::Rect(native_widget_delegate_->GetMaximumSize())).size(); minimum -> minimum_in_pixels maximum -> maximum_in_pixels https://codereview.chromium.org/924853002/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1369: ::Atom state2) { Nit: Can you please fix the alignment here? https://codereview.chromium.org/924853002/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1679: compositor()->ScheduleRedrawRect(damage_rect); damage_rect -> damage_rect_in_pixels https://codereview.chromium.org/924853002/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1746: gfx::Rect bounds(translated_x, translated_y, bounds -> bounds_in_pixels translated_x -> translated_x_in_pixels translated_y -> translated_y_in_pixels https://codereview.chromium.org/924853002/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1930: void DesktopWindowTreeHostX11::DelayedResize(const gfx::Size& size) { size -> size_in_pixels
https://codereview.chromium.org/924853002/diff/60001/ui/views/widget/desktop_... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): https://codereview.chromium.org/924853002/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:421: transient_parent_rect.width() >= size_in_pixels.width()) { On 2015/02/19 03:07:20, pkotwicz wrote: > You meant to compare against |size| not |size_in_pixels|? Good catch. Done. https://codereview.chromium.org/924853002/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:434: window_bounds.AdjustToFit(parent_bounds_in_pixels); On 2015/02/19 03:07:20, pkotwicz wrote: > window_bounds -> window_bounds_in_pixels Done. https://codereview.chromium.org/924853002/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:532: AdjustSize(bounds_in_pixels_.size())); On 2015/02/19 03:07:20, pkotwicz wrote: > adjusted_bounds -> adjusted_bounds_in_pixels Done. https://codereview.chromium.org/924853002/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:859: display = gfx::Screen::GetNativeScreen()->GetDisplayNearestWindow(win); On 2015/02/19 03:07:20, pkotwicz wrote: > Nit: Why bother? We are going to use the scale factor of the primary display > anyways. If the scale factors of the displays was different, perhaps we should > do things differently when |window_mapped_| is false > > (I can see a future version of myself trying to figure out why we have special > logic when |window_mapped_| is true) It makes sense to use the DSF of the display this window belongs to. That's what this block of code does. In the case where we don't still know what display the window belongs to (so when |window_mapped_| is false), we get it from the primary display. I think on linux, we can have only one scale factor on all displays, so always using the DSF from the primary display should be sufficient. But it feels a bit cleaner to try to do the right thing and get the DSF from the relevant display (esp. since it's trivial to do so). https://codereview.chromium.org/924853002/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:896: AdjustSize(requested_bounds_in_pixel.size())); On 2015/02/19 03:07:20, pkotwicz wrote: > bounds -> bounds_in_pixels Done. https://codereview.chromium.org/924853002/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:911: gfx::Size size = bounds.size(); On 2015/02/19 03:07:20, pkotwicz wrote: > size -> size_in_pixels Done. https://codereview.chromium.org/924853002/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1068: AdjustSize(bounds_in_pixels_.size())); On 2015/02/19 03:07:20, pkotwicz wrote: > Nit: bounds_in_pixels_.set_size(AdjustSize(bounds_in_pixels_.size())); Done. https://codereview.chromium.org/924853002/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1225: gfx::Size size = requested_size_in_pixels; On 2015/02/19 03:07:20, pkotwicz wrote: > size -> size_in_pixels Done. https://codereview.chromium.org/924853002/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1314: ToPixelRect(gfx::Rect(native_widget_delegate_->GetMaximumSize())).size(); On 2015/02/19 03:07:20, pkotwicz wrote: > minimum -> minimum_in_pixels > maximum -> maximum_in_pixels Done. https://codereview.chromium.org/924853002/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1369: ::Atom state2) { On 2015/02/19 03:07:20, pkotwicz wrote: > Nit: Can you please fix the alignment here? Done. https://codereview.chromium.org/924853002/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1679: compositor()->ScheduleRedrawRect(damage_rect); On 2015/02/19 03:07:20, pkotwicz wrote: > damage_rect -> damage_rect_in_pixels Done. https://codereview.chromium.org/924853002/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1746: gfx::Rect bounds(translated_x, translated_y, On 2015/02/19 03:07:20, pkotwicz wrote: > bounds -> bounds_in_pixels > translated_x -> translated_x_in_pixels > translated_y -> translated_y_in_pixels Done. https://codereview.chromium.org/924853002/diff/60001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1930: void DesktopWindowTreeHostX11::DelayedResize(const gfx::Size& size) { On 2015/02/19 03:07:20, pkotwicz wrote: > size -> size_in_pixels Done.
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/b19dc5bf24871e40e986f0f024946b1c281e77c4 Cr-Commit-Position: refs/heads/master@{#317733}
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as b19dc5bf24871e40e986f0f024946b1c281e77c4 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
