Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2645273005:
exo: Fix cursor size for multi-display ARC (Closed)
|
Created:
3 years, 11 months ago by Dominik Laskowski Modified:
3 years, 11 months ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionexo: Fix cursor size for multi-display ARC
When a low-DPI monitor is connected to a high-DPI device, the cursor in
ARC windows appears twice as large if:
1) The primary display is the external display, unless the internal
display uses native resolution and the window is on the internal
display. This includes docked mode.
2) The primary display is the internal display, the internal display
uses native resolution, and the window is on the external display.
BUG=684672
BUG=642894
TEST=samus: Cursor size is correct in the above cases.
Review-Url: https://codereview.chromium.org/2645273005
Cr-Commit-Position: refs/heads/master@{#446192}
Committed: https://chromium.googlesource.com/chromium/src/+/f79ede40658ecab4a6c72b6e51c456d68dab2ad0
Patch Set 1 #
Total comments: 10
Patch Set 2 : Fix tests #Patch Set 3 : Address nits #
Total comments: 2
Patch Set 4 : Remove TODO #
Total comments: 5
Messages
Total messages: 29 (18 generated)
The CQ bit was checked by domlaskowski@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...
domlaskowski@chromium.org changed reviewers: + oshima@chromium.org, reveman@chromium.org
PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by domlaskowski@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: This issue passed the CQ dry run.
lgtm % nits https://codereview.chromium.org/2645273005/diff/1/components/exo/pointer.cc File components/exo/pointer.cc (right): https://codereview.chromium.org/2645273005/diff/1/components/exo/pointer.cc#n... components/exo/pointer.cc:314: auto display = screen->GetDisplayNearestWindow(widget_->GetNativeWindow()); nit: Avoid "auto" here to make it clear what the type of "display" is fyi, I prefer to avoid "auto" unless when we're duplicating type names (e.g. "auto a = new Type" is ok) or it's making the code significantly more easy to read (often the case for iterators) https://codereview.chromium.org/2645273005/diff/1/components/exo/pointer.cc#n... components/exo/pointer.cc:316: nit: is this blank line intentional? remove if not https://codereview.chromium.org/2645273005/diff/1/components/exo/pointer.cc#n... components/exo/pointer.cc:317: float primary_dsf = screen->GetPrimaryDisplay().device_scale_factor(); nit: remove abbreviation. s/primary_dsf/primary_device_scale_factor/ https://codereview.chromium.org/2645273005/diff/1/components/exo/pointer.cc#n... components/exo/pointer.cc:319: // The cursor is scaled down by the DSF of the primary display, and its scaled up? https://codereview.chromium.org/2645273005/diff/1/components/exo/pointer.cc#n... components/exo/pointer.cc:325: auto info = helper->GetDisplayInfo(display::Display::InternalDisplayId()); nit: maybe internal_display_device_scale_factor instead or skip the variable completely?
PTAL at the second patch too. Also awaiting oshima's review. https://codereview.chromium.org/2645273005/diff/1/components/exo/pointer.cc File components/exo/pointer.cc (right): https://codereview.chromium.org/2645273005/diff/1/components/exo/pointer.cc#n... components/exo/pointer.cc:314: auto display = screen->GetDisplayNearestWindow(widget_->GetNativeWindow()); On 2017/01/23 22:22:34, reveman wrote: > nit: Avoid "auto" here to make it clear what the type of "display" is > > fyi, I prefer to avoid "auto" unless when we're duplicating type names (e.g. > "auto a = new Type" is ok) or it's making the code significantly more easy to > read (often the case for iterators) Done, and above too. https://codereview.chromium.org/2645273005/diff/1/components/exo/pointer.cc#n... components/exo/pointer.cc:316: On 2017/01/23 22:22:34, reveman wrote: > nit: is this blank line intentional? remove if not Removed in second patch. https://codereview.chromium.org/2645273005/diff/1/components/exo/pointer.cc#n... components/exo/pointer.cc:317: float primary_dsf = screen->GetPrimaryDisplay().device_scale_factor(); On 2017/01/23 22:22:34, reveman wrote: > nit: remove abbreviation. s/primary_dsf/primary_device_scale_factor/ Done. https://codereview.chromium.org/2645273005/diff/1/components/exo/pointer.cc#n... components/exo/pointer.cc:319: // The cursor is scaled down by the DSF of the primary display, and its On 2017/01/23 22:22:34, reveman wrote: > scaled up? I meant that the surface size is divided by the primary DSF. Changed the wording. https://codereview.chromium.org/2645273005/diff/1/components/exo/pointer.cc#n... components/exo/pointer.cc:325: auto info = helper->GetDisplayInfo(display::Display::InternalDisplayId()); On 2017/01/23 22:22:34, reveman wrote: > nit: maybe internal_display_device_scale_factor instead or skip the variable > completely? Removed variable.
lgtm % nit https://codereview.chromium.org/2645273005/diff/40001/components/exo/pointer.cc File components/exo/pointer.cc (right): https://codereview.chromium.org/2645273005/diff/40001/components/exo/pointer.... components/exo/pointer.cc:319: // but it should be dynamic to support devices without an internal display. nit: I don't think this TODO belongs here as the code that needs to be improved is ARC and not this
https://codereview.chromium.org/2645273005/diff/40001/components/exo/pointer.cc File components/exo/pointer.cc (right): https://codereview.chromium.org/2645273005/diff/40001/components/exo/pointer.... components/exo/pointer.cc:319: // but it should be dynamic to support devices without an internal display. On 2017/01/24 00:16:01, reveman wrote: > nit: I don't think this TODO belongs here as the code that needs to be improved > is ARC and not this Done.
The CQ bit was checked by domlaskowski@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: This issue passed the CQ dry run.
Description was changed from
==========
exo: Fix cursor size for multi-display ARC
When a low-DPI monitor is connected to a high-DPI device, the cursor in
ARC windows appears twice as large if:
1) The primary display is the external display, unless the internal
display uses native resolution and the window is on the internal
display. This includes docked mode.
2) The primary display is the internal display, the internal display
uses native resolution, and the window is on the external display.
BUG=642894
TEST=samus: Cursor size is correct in the above cases.
==========
to
==========
exo: Fix cursor size for multi-display ARC
When a low-DPI monitor is connected to a high-DPI device, the cursor in
ARC windows appears twice as large if:
1) The primary display is the external display, unless the internal
display uses native resolution and the window is on the internal
display. This includes docked mode.
2) The primary display is the internal display, the internal display
uses native resolution, and the window is on the external display.
BUG=684672
BUG=642894
TEST=samus: Cursor size is correct in the above cases.
==========
https://codereview.chromium.org/2645273005/diff/60001/components/exo/pointer.cc File components/exo/pointer.cc (right): https://codereview.chromium.org/2645273005/diff/60001/components/exo/pointer.... components/exo/pointer.cc:327: if (!display.IsInternal() || Isn't this && ? https://codereview.chromium.org/2645273005/diff/60001/components/exo/pointer.... components/exo/pointer.cc:328: display.device_scale_factor() != primary_device_scale_factor) { My understanding was that if the android scale factor (which is determined by the internal display's native scale factor) is not consistent with external display's one we need to scale it back. Am I wrong?
https://codereview.chromium.org/2645273005/diff/60001/components/exo/pointer.cc File components/exo/pointer.cc (right): https://codereview.chromium.org/2645273005/diff/60001/components/exo/pointer.... components/exo/pointer.cc:327: if (!display.IsInternal() || On 2017/01/24 20:48:19, oshima wrote: > Isn't this && ? No, we want to avoid scaling if: 1) The window is on the internal display. 2) The external display is primary. 3) The DSFs are equal. For example, when using native resolution on Samus, the correct cursor size is 50x50 pixels because the DSF is 1 (see comment below). If a low-DPI external display is primary, the cursor should not be scaled to 25x25. If the external display is high-DPI, then the division below cancels out, so the cursor is not scaled either. https://codereview.chromium.org/2645273005/diff/60001/components/exo/pointer.... components/exo/pointer.cc:328: display.device_scale_factor() != primary_device_scale_factor) { On 2017/01/24 20:48:19, oshima wrote: > My understanding was that if the android scale factor (which is determined by > the internal display's native scale factor) is not consistent with external > display's one we need to scale it back. > Am I wrong? It's a bit more complicated because: 1) HWC divides the cursor size (50x50) by the primary DSF, so that (2) is satisfied for the single high-DPI display case. 2) For high-DPI internal displays, the cursor size at native resolution is specified in pixels rather than DIPs. For example, the size is 50x50 pixels for native resolution, and 25x25 DIPs for all other resolutions.
https://codereview.chromium.org/2645273005/diff/60001/components/exo/pointer.cc File components/exo/pointer.cc (right): https://codereview.chromium.org/2645273005/diff/60001/components/exo/pointer.... components/exo/pointer.cc:327: if (!display.IsInternal() || On 2017/01/24 22:14:34, Dominik Laskowski wrote: > On 2017/01/24 20:48:19, oshima wrote: > > Isn't this && ? > > No, we want to avoid scaling if: > > 1) The window is on the internal display. > 2) The external display is primary. > 3) The DSFs are equal. > > For example, when using native resolution on Samus, the correct cursor size is > 50x50 pixels because the DSF is 1 (see comment below). The size of the cursor should be same (50x50 pixels?) regardless of DSF for HighDPI internal display. (That is 50x50 DIP when internal display is 1x, but 25x25 DIP when internal display is 2x) > If a low-DPI external > display is primary, the cursor should not be scaled to 25x25. Hmm, I'm confused. Can you use DIP or pixels always? I thought on low DIP the cursor should be 25x25 pixels. > If the external > display is high-DPI, then the division below cancels out, so the cursor is not > scaled either. Let's have a quick offline chat. I probably don't know exactly how cursor works on Android side, but I want to make sure I fully understand this.
thank you for clarification. I think there is a room to simplify this, but lgtm for the time being.
The CQ bit was checked by domlaskowski@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org Link to the patchset: https://codereview.chromium.org/2645273005/#ps60001 (title: "Remove TODO")
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": 60001, "attempt_start_ts": 1485391600763630,
"parent_rev": "cc5d47b480905335838fab5c6174e06747acb5a7", "commit_rev":
"f79ede40658ecab4a6c72b6e51c456d68dab2ad0"}
Message was sent while issue was closed.
Description was changed from
==========
exo: Fix cursor size for multi-display ARC
When a low-DPI monitor is connected to a high-DPI device, the cursor in
ARC windows appears twice as large if:
1) The primary display is the external display, unless the internal
display uses native resolution and the window is on the internal
display. This includes docked mode.
2) The primary display is the internal display, the internal display
uses native resolution, and the window is on the external display.
BUG=684672
BUG=642894
TEST=samus: Cursor size is correct in the above cases.
==========
to
==========
exo: Fix cursor size for multi-display ARC
When a low-DPI monitor is connected to a high-DPI device, the cursor in
ARC windows appears twice as large if:
1) The primary display is the external display, unless the internal
display uses native resolution and the window is on the internal
display. This includes docked mode.
2) The primary display is the internal display, the internal display
uses native resolution, and the window is on the external display.
BUG=684672
BUG=642894
TEST=samus: Cursor size is correct in the above cases.
Review-Url: https://codereview.chromium.org/2645273005
Cr-Commit-Position: refs/heads/master@{#446192}
Committed:
https://chromium.googlesource.com/chromium/src/+/f79ede40658ecab4a6c72b6e51c4...
==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/f79ede40658ecab4a6c72b6e51c4... |
