Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2504793002:
Scale bounds in DesktopWindowTreeHostMus and in calls to SetBounds. (Closed)
|
Created:
4 years, 1 month ago by riajiang Modified:
4 years, 1 month ago CC:
chromium-reviews, anandc+watch-blimp_chromium.org, sadrul, sriramsr+watch-blimp_chromium.org, bgoldman+watch-blimp_chromium.org, tfarina, maniscalco+watch-blimp_chromium.org, steimel+watch-blimp_chromium.org, gcasto+watch-blimp_chromium.org, shaktisahu+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, perumaal+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, lethalantidote+watch-blimp_chromium.org, kalyank, dtrainor+watch-blimp_chromium.org, scf+watch-blimp_chromium.org, khushalsagar+watch-blimp_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionScale bounds in DesktopWindowTreeHostMus and in calls to SetBounds.
BUG=663524
TEST=views_aura_mus_unittests
manual (with --force-device-scale-factor=2)
Committed: https://crrev.com/7df1b3de855d7a398f12226e1749555f502736c4
Cr-Commit-Position: refs/heads/master@{#434106}
Patch Set 1 #Patch Set 2 : scale factor #
Total comments: 7
Patch Set 3 : blimp #
Total comments: 3
Patch Set 4 : sadrul@'s comment #
Total comments: 2
Patch Set 5 : set bounds in dips #Patch Set 6 : SetBounds + rebase #
Messages
Total messages: 35 (19 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by riajiang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Description was changed from
==========
Scale bounds in DesktopWindowTreeHostMus and in calls to SetBounds.
BUG=663524
TEST=manual (with --force-device-scale-factor=2)
==========
to
==========
Scale bounds in DesktopWindowTreeHostMus and in calls to SetBounds.
BUG=663524
TEST=views_aura_mus_unittests
manual (with --force-device-scale-factor=2)
==========
riajiang@chromium.org changed reviewers: + sadrul@chromium.org, sky@chromium.org
sadrul@ and sky@, PTAL. Thanks!
https://codereview.chromium.org/2504793002/diff/40001/ash/host/ash_window_tre... File ash/host/ash_window_tree_host_unified.cc (right): https://codereview.chromium.org/2504793002/diff/40001/ash/host/ash_window_tre... ash/host/ash_window_tree_host_unified.cc:55: window->SetBounds(gfx::ConvertRectToPixel(compositor()->device_scale_factor(), It may make sense to rename PlatformWindow::SetBounds() to PlatformWindow::SetPixelBounds() to make this clear. (maybe update crbug.com/664212 to include PlatformWindow bounds too) https://codereview.chromium.org/2504793002/diff/40001/blimp/client/app/linux/... File blimp/client/app/linux/blimp_display_manager.cc (right): https://codereview.chromium.org/2504793002/diff/40001/blimp/client/app/linux/... blimp/client/app/linux/blimp_display_manager.cc:45: gfx::Rect(window_size))); Are you sure you want 1.f / device_pixel_ratio_?
https://codereview.chromium.org/2504793002/diff/40001/ash/host/ash_window_tre... File ash/host/ash_window_tree_host_unified.cc (right): https://codereview.chromium.org/2504793002/diff/40001/ash/host/ash_window_tre... ash/host/ash_window_tree_host_unified.cc:55: window->SetBounds(gfx::ConvertRectToPixel(compositor()->device_scale_factor(), On 2016/11/17 20:26:21, sadrul wrote: > It may make sense to rename PlatformWindow::SetBounds() to > PlatformWindow::SetPixelBounds() to make this clear. (maybe update > crbug.com/664212 to include PlatformWindow bounds too) Added to that bug. https://codereview.chromium.org/2504793002/diff/40001/blimp/client/app/linux/... File blimp/client/app/linux/blimp_display_manager.cc (right): https://codereview.chromium.org/2504793002/diff/40001/blimp/client/app/linux/... blimp/client/app/linux/blimp_display_manager.cc:45: gfx::Rect(window_size))); On 2016/11/17 20:26:21, sadrul wrote: > Are you sure you want 1.f / device_pixel_ratio_? When device_pixel_ratio_ is used (e.g. https://cs.chromium.org/chromium/src/blimp/client/app/linux/blimp_display_man...), it is sent to a function with the comment that says this is "in terms of dp to px" (I think the name also suggests this) - which is the opposite of what we are using for device_scale_factor?
https://codereview.chromium.org/2504793002/diff/40001/blimp/client/app/linux/... File blimp/client/app/linux/blimp_display_manager.cc (right): https://codereview.chromium.org/2504793002/diff/40001/blimp/client/app/linux/... blimp/client/app/linux/blimp_display_manager.cc:45: gfx::Rect(window_size))); On 2016/11/17 21:43:28, riajiang wrote: > On 2016/11/17 20:26:21, sadrul wrote: > > Are you sure you want 1.f / device_pixel_ratio_? > > When device_pixel_ratio_ is used (e.g. > https://cs.chromium.org/chromium/src/blimp/client/app/linux/blimp_display_man...), > it is sent to a function with the comment that says this is "in terms of dp to > px" (I think the name also suggests this) - which is the opposite of what we are > using for device_scale_factor? I am not really sure what you mean. But, PlatformWindow::SetBounds() expects bounds in physical-pixels. If |window_size| is in DIP, then you need to convert from DIP to physical pixel (i.e. scale up by device_pixel_ratio_). If |window_size| is already in pixels, then you don't need to do any conversion at all. You should confirm with a blimp owner what pixels |window_size| is in.
https://codereview.chromium.org/2504793002/diff/40001/blimp/client/app/linux/... File blimp/client/app/linux/blimp_display_manager.cc (right): https://codereview.chromium.org/2504793002/diff/40001/blimp/client/app/linux/... blimp/client/app/linux/blimp_display_manager.cc:45: gfx::Rect(window_size))); On 2016/11/18 01:23:55, sadrul wrote: > On 2016/11/17 21:43:28, riajiang wrote: > > On 2016/11/17 20:26:21, sadrul wrote: > > > Are you sure you want 1.f / device_pixel_ratio_? > > > > When device_pixel_ratio_ is used (e.g. > > > https://cs.chromium.org/chromium/src/blimp/client/app/linux/blimp_display_man...), > > it is sent to a function with the comment that says this is "in terms of dp to > > px" (I think the name also suggests this) - which is the opposite of what we > are > > using for device_scale_factor? > > I am not really sure what you mean. But, PlatformWindow::SetBounds() expects > bounds in physical-pixels. If |window_size| is in DIP, then you need to convert > from DIP to physical pixel (i.e. scale up by device_pixel_ratio_). If > |window_size| is already in pixels, then you don't need to do any conversion at > all. You should confirm with a blimp owner what pixels |window_size| is in. Yes gfx::ConvertRectToPixel will convert window_size to pixel - it is expecting device_scale_factor as the first param tho, which is pixel/dip, but device_pixel_ratio_ is dip/pixel, that's why I used 1.f/device_pixel_ratio_. Did I miss something? window_size is constant 800*600 (https://cs.chromium.org/chromium/src/blimp/client/app/linux/blimp_main.cc?sq=...), so I thought it was in dip?
riajiang@chromium.org changed reviewers: + dtrainor@chromium.org
also +dtrainor@ for blimp, thanks! https://codereview.chromium.org/2504793002/diff/40001/blimp/client/app/linux/... File blimp/client/app/linux/blimp_display_manager.cc (right): https://codereview.chromium.org/2504793002/diff/40001/blimp/client/app/linux/... blimp/client/app/linux/blimp_display_manager.cc:45: gfx::Rect(window_size))); On 2016/11/18 01:49:34, riajiang wrote: > On 2016/11/18 01:23:55, sadrul wrote: > > On 2016/11/17 21:43:28, riajiang wrote: > > > On 2016/11/17 20:26:21, sadrul wrote: > > > > Are you sure you want 1.f / device_pixel_ratio_? > > > > > > When device_pixel_ratio_ is used (e.g. > > > > > > https://cs.chromium.org/chromium/src/blimp/client/app/linux/blimp_display_man...), > > > it is sent to a function with the comment that says this is "in terms of dp > to > > > px" (I think the name also suggests this) - which is the opposite of what we > > are > > > using for device_scale_factor? > > > > I am not really sure what you mean. But, PlatformWindow::SetBounds() expects > > bounds in physical-pixels. If |window_size| is in DIP, then you need to > convert > > from DIP to physical pixel (i.e. scale up by device_pixel_ratio_). If > > |window_size| is already in pixels, then you don't need to do any conversion > at > > all. You should confirm with a blimp owner what pixels |window_size| is in. > > Yes gfx::ConvertRectToPixel will convert window_size to pixel - it is expecting > device_scale_factor as the first param tho, which is pixel/dip, but > device_pixel_ratio_ is dip/pixel, that's why I used 1.f/device_pixel_ratio_. Did > I miss something? > window_size is constant 800*600 > (https://cs.chromium.org/chromium/src/blimp/client/app/linux/blimp_main.cc?sq=...), > so I thought it was in dip? As discussed with sadrul@ offline, |device_pixel_ratio_| should actually be px/dip as well. Changed it to just use |device_pixel_ratio_| and updated the comment about |device_pixel_ratio_|.
lgtm https://codereview.chromium.org/2504793002/diff/60001/blimp/client/public/con... File blimp/client/public/contents/blimp_contents_view.h (right): https://codereview.chromium.org/2504793002/diff/60001/blimp/client/public/con... blimp/client/public/contents/blimp_contents_view.h:46: // in pixels and |device_scale_factor| is in terms of px to dp. D'oh thanks! :)
https://codereview.chromium.org/2504793002/diff/60001/ash/host/ash_window_tre... File ash/host/ash_window_tree_host_unified.cc (right): https://codereview.chromium.org/2504793002/diff/60001/ash/host/ash_window_tre... ash/host/ash_window_tree_host_unified.cc:56: initial_bounds)); Hm, I actually think this doesn't need to be scaled. WindowTreeHost receives the bounds in physical pixels.
Patchset #4 (id:80001) has been deleted
https://codereview.chromium.org/2504793002/diff/60001/ash/host/ash_window_tre... File ash/host/ash_window_tree_host_unified.cc (right): https://codereview.chromium.org/2504793002/diff/60001/ash/host/ash_window_tre... ash/host/ash_window_tree_host_unified.cc:56: initial_bounds)); On 2016/11/21 17:39:50, sadrul wrote: > Hm, I actually think this doesn't need to be scaled. WindowTreeHost receives the > bounds in physical pixels. Done. As discussed offline, bounds_in_native is in pixel.
lgtm
I will approve this if you say it's too painful to convert WindowTreeHost now, but it's really confusing right now because the names don't indicate they are pixel based. https://codereview.chromium.org/2504793002/diff/100001/ui/aura/window_tree_ho... File ui/aura/window_tree_host.h (right): https://codereview.chromium.org/2504793002/diff/100001/ui/aura/window_tree_ho... ui/aura/window_tree_host.h:79: ui::Compositor* compositor() const { return compositor_.get(); } const functions should return const objects, otherwise you can easily circumvent const correctness. https://codereview.chromium.org/2504793002/diff/100001/ui/views/mus/desktop_w... File ui/views/mus/desktop_window_tree_host_mus.cc (right): https://codereview.chromium.org/2504793002/diff/100001/ui/views/mus/desktop_w... ui/views/mus/desktop_window_tree_host_mus.cc:158: SetBounds(screen_bounds_in_pixels); This code is hard to read because it isn't clear whether a function is taking dips or pixels. Can you rename functions in WindowTreeHost to make this clear? For example, SetBoundsInScreen()... If you did that, then this class could offer a SetBounds that operates in dips and all functions here could call it rather than lots of conversion.
On 2016/11/21 22:27:25, sky wrote: > I will approve this if you say it's too painful to convert WindowTreeHost now, > but it's really confusing right now because the names don't indicate they are > pixel based. > > https://codereview.chromium.org/2504793002/diff/100001/ui/aura/window_tree_ho... > File ui/aura/window_tree_host.h (right): > > https://codereview.chromium.org/2504793002/diff/100001/ui/aura/window_tree_ho... > ui/aura/window_tree_host.h:79: ui::Compositor* compositor() const { return > compositor_.get(); } > const functions should return const objects, otherwise you can easily circumvent > const correctness. > > https://codereview.chromium.org/2504793002/diff/100001/ui/views/mus/desktop_w... > File ui/views/mus/desktop_window_tree_host_mus.cc (right): > > https://codereview.chromium.org/2504793002/diff/100001/ui/views/mus/desktop_w... > ui/views/mus/desktop_window_tree_host_mus.cc:158: > SetBounds(screen_bounds_in_pixels); > This code is hard to read because it isn't clear whether a function is taking > dips or pixels. Can you rename functions in WindowTreeHost to make this clear? > For example, SetBoundsInScreen()... If you did that, then this class could offer > a SetBounds that operates in dips and all functions here could call it rather > than lots of conversion. How about adding a SetBoundsInDips to DesktopWindowTreeHostMus that does all the conversions and calls WindowTreeHostMus::SetBounds in this CL for now? I'll start converting WindowTreeHost to indicate that it's in pixels and then DesktopWindowTreeHostMus::SetBounds only needs to be renamed.
I'm going to LGTM this as the rename shouldn't hold this patch up. The general rule is unless otherwise stated functions that take rects/sizes/ints... are in dips. So, even if you went with SetBoundsInDips it would still be confusing because you have to call a function named SetBounds from it. Additionally some of the overrides are from WindowTreeHost, which violates this. The best thing is to fix WindowTreeHost.
The CQ bit was checked by riajiang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #6 (id:140001) has been deleted
The CQ bit was checked by riajiang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org, sadrul@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2504793002/#ps160001 (title: "SetBounds + rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1479869243425220,
"parent_rev": "eb4ca6a2011aa3196f40407949aec0ace9fca3cc", "commit_rev":
"aecda90b2c858fb4bfd6a4bedda30a91e8081311"}
Message was sent while issue was closed.
Description was changed from
==========
Scale bounds in DesktopWindowTreeHostMus and in calls to SetBounds.
BUG=663524
TEST=views_aura_mus_unittests
manual (with --force-device-scale-factor=2)
==========
to
==========
Scale bounds in DesktopWindowTreeHostMus and in calls to SetBounds.
BUG=663524
TEST=views_aura_mus_unittests
manual (with --force-device-scale-factor=2)
==========
Message was sent while issue was closed.
Committed patchset #6 (id:160001)
Message was sent while issue was closed.
Description was changed from
==========
Scale bounds in DesktopWindowTreeHostMus and in calls to SetBounds.
BUG=663524
TEST=views_aura_mus_unittests
manual (with --force-device-scale-factor=2)
==========
to
==========
Scale bounds in DesktopWindowTreeHostMus and in calls to SetBounds.
BUG=663524
TEST=views_aura_mus_unittests
manual (with --force-device-scale-factor=2)
Committed: https://crrev.com/7df1b3de855d7a398f12226e1749555f502736c4
Cr-Commit-Position: refs/heads/master@{#434106}
==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/7df1b3de855d7a398f12226e1749555f502736c4 Cr-Commit-Position: refs/heads/master@{#434106} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
