Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2780623002:
exo: Fix multi-display hardware cursor (Closed)
|
Created:
3 years, 9 months ago by Dominik Laskowski Modified:
3 years, 6 months ago CC:
chromium-reviews, kalyank, sadrul, yoshiki Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionexo: Fix multi-display hardware cursor
This CL fixes crashes in extended desktop mode caused by incorrect
parenting of the cursor surface, as well as cases where the cursor
was captured with an incorrect transform or hotspot:
1) The mouse enters a display with a different DSF or UI scale.
2) The internal display is not the primary display.
3) The display is rotated.
It also adds a TODO for an accessibility regression.
BUG=631136
BUG=642894
TEST=caroline: Cursor parenting and scaling is correct for each display.
TEST=caroline: Cursor on rotated displays has correct orientation.
Review-Url: https://codereview.chromium.org/2780623002
Cr-Commit-Position: refs/heads/master@{#476353}
Committed: https://chromium.googlesource.com/chromium/src/+/ba20af04ad131e2fc7b8fd4142cffd7461057d5b
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Split #Patch Set 4 : Rebase and address nit #Patch Set 5 : Rebase properly #
Total comments: 24
Patch Set 6 : Rebase #Patch Set 7 : Address nits #Patch Set 8 : Remove unrelated change #Patch Set 9 : Split #Patch Set 10 : Rebase #Patch Set 11 : Remove capture on mouse enter #
Total comments: 9
Patch Set 12 : Simplify GetContainer #Patch Set 13 : Rebase #Patch Set 14 : Rebase #Patch Set 15 : Rebase #Patch Set 16 : Transform cursor using Skia #Patch Set 17 : Rebase #Patch Set 18 : Fix accessibility test #
Total comments: 20
Patch Set 19 : Rebase #Patch Set 20 : Address comments #
Total comments: 24
Patch Set 21 : Rebase #Patch Set 22 : Refactor #Patch Set 23 : Fix accessibility test #
Total comments: 8
Patch Set 24 : Refactor some more #Patch Set 25 : Fix style error #Dependent Patchsets: Messages
Total messages: 101 (66 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
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. yoshiki: just an FYI about accessibility regression.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Can the this be split into multiple smaller patches? The size of exo changes make it hard to review efficiently.
Description was changed from ========== exo: Fix hardware cursor edge cases The hardware cursor was only captured following a wl_pointer::set_cursor request, a surface commit, or scale change, so the cursor was sometimes stale on entering a surface. In other cases, the cursor was captured with an incorrect transform or hotspot: 1) The mouse enters a display with a different DSF or UI scale. 2) Same as above, but while the mouse is locked. 3) The internal display is not the primary display. 4) The display is rotated. This CL also adds a TODO for an accessibility regression. BUG=631136 BUG=642894 TEST=caroline: Cursor is captured correctly in the above cases. ========== to ========== exo: Fix hardware cursor edge cases The hardware cursor was only captured following a wl_pointer::set_cursor request, a surface commit, or scale change, so the cursor was sometimes stale on entering a surface. In other cases, the cursor was captured with an incorrect transform or hotspot: 1) The mouse enters a display with a different DSF or UI scale. 2) Same as above, but while the mouse is locked. 3) The internal display is not the primary display. This CL also adds a TODO for an accessibility regression. BUG=631136 BUG=642894 TEST=caroline: Cursor is captured correctly in the above cases. ==========
On 2017/03/27 20:14:57, reveman wrote: > Can the this be split into multiple smaller patches? The size of exo changes > make it hard to review efficiently. Done: https://codereview.chromium.org/2778913002/ https://codereview.chromium.org/2777343002/
On 2017/03/27 at 21:54:06, domlaskowski wrote: > On 2017/03/27 20:14:57, reveman wrote: > > Can the this be split into multiple smaller patches? The size of exo changes > > make it hard to review efficiently. > > Done: > > https://codereview.chromium.org/2778913002/ > https://codereview.chromium.org/2777343002/ Please explain the order that patches should be reviewed and is this CL still relevant or replaced by the others?
On 2017/03/28 07:50:46, reveman wrote: > On 2017/03/27 at 21:54:06, domlaskowski wrote: > > On 2017/03/27 20:14:57, reveman wrote: > > > Can the this be split into multiple smaller patches? The size of exo changes > > > make it hard to review efficiently. > > > > Done: > > > > https://codereview.chromium.org/2778913002/ > > https://codereview.chromium.org/2777343002/ > > Please explain the order that patches should be reviewed and is this CL still > relevant or replaced by the others? Yes, it's still relevant. 1) https://codereview.chromium.org/2778913002/ 2) This CL. 3) https://codereview.chromium.org/2777343002/ 4) https://codereview.chromium.org/2779823002/
There's still too much going on in this single CL. I posted some comments but it's really hard to review as I don't know what all the changes in this patch are needed for. Why can't the display change problem not simply be solved by a trivial update to UpdateCursorScale? https://codereview.chromium.org/2780623002/diff/80001/components/exo/pointer.cc File components/exo/pointer.cc (left): https://codereview.chromium.org/2780623002/diff/80001/components/exo/pointer.... components/exo/pointer.cc:134: return cursor_; why does all this need to change? I think the previous logic was easy to understand. https://codereview.chromium.org/2780623002/diff/80001/components/exo/pointer.cc File components/exo/pointer.cc (right): https://codereview.chromium.org/2780623002/diff/80001/components/exo/pointer.... components/exo/pointer.cc:35: // TODO: Some accessibility features, including large cursors, disable hardware nit: TODO(name) and please add a bug number https://codereview.chromium.org/2780623002/diff/80001/components/exo/pointer.... components/exo/pointer.cc:139: return ui::kCursorNull; I think returning null cursor in this case is incorrect. It's critical that the cursor is not changing when entering a new window until the client has had a chance to replace it. https://codereview.chromium.org/2780623002/diff/80001/components/exo/pointer.... components/exo/pointer.cc:174: if (surface_) this is incorrect I think. the client needs to provide a new cursor surface for each enter event. https://codereview.chromium.org/2780623002/diff/80001/components/exo/pointer.... components/exo/pointer.cc:272: // Capture is asynchronous, so avoid rendering old cursor in the meantime. I don't understand why we need to do all this. This type of special case where we force an update makes me worried about race conditions where we incorrectly cancel the capture of a cursor frame. It's important that cursor changes can be pipelined properly or animated cursor will not work correctly. https://codereview.chromium.org/2780623002/diff/80001/components/exo/pointer.... components/exo/pointer.cc:341: // Prevent subsequent requests in the same frame from aborting this capture. why is this needed? https://codereview.chromium.org/2780623002/diff/80001/components/exo/pointer.... components/exo/pointer.cc:389: if (force && cursor_client->IsCursorLocked()) what's the situation where this is currently causing a problem? can this IsCursorLocked part be moved to a separate CL? https://codereview.chromium.org/2780623002/diff/80001/components/exo/pointer.h File components/exo/pointer.h (left): https://codereview.chromium.org/2780623002/diff/80001/components/exo/pointer.... components/exo/pointer.h:80: void OnCursorCaptured(const gfx::Point& hotspot, why not const? https://codereview.chromium.org/2780623002/diff/80001/components/exo/pointer.h File components/exo/pointer.h (right): https://codereview.chromium.org/2780623002/diff/80001/components/exo/pointer.... components/exo/pointer.h:76: void CaptureCursor(bool force); why do we need |force|? can the behavior with force=true be used all the time? https://codereview.chromium.org/2780623002/diff/80001/components/exo/pointer.... components/exo/pointer.h:104: float device_scale_factor_ = 1.0f; Please add a comment here for consistency. https://codereview.chromium.org/2780623002/diff/80001/components/exo/pointer.... components/exo/pointer.h:109: const std::unique_ptr<aura::Window> cursor_; why do we have to change this? We should avoid using aura::Windows except for top level shell surfaces so I'm worried about any changes that introduce more use of them. https://codereview.chromium.org/2780623002/diff/80001/components/exo/shell_su... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2780623002/diff/80001/components/exo/shell_su... components/exo/shell_surface.cc:1155: params.parent = parent_ ? parent_ how is this change related?
UpdateCursorScale is insufficient when dragging across displays, because the display change is only detected once the mouse has crossed and the cursor has been rendered with an incorrect scale for a frame or two. Hooking into AshNativeCursorManager is the only way to solve this. https://codereview.chromium.org/2780623002/diff/80001/components/exo/pointer.cc File components/exo/pointer.cc (left): https://codereview.chromium.org/2780623002/diff/80001/components/exo/pointer.... components/exo/pointer.cc:134: return cursor_; On 2017/03/30 07:47:53, reveman wrote: > why does all this need to change? I think the previous logic was easy to > understand. Keeping track of the latest cursor is unnecessary, i.e. CursorClient::GetCursor does that already. https://codereview.chromium.org/2780623002/diff/80001/components/exo/pointer.cc File components/exo/pointer.cc (right): https://codereview.chromium.org/2780623002/diff/80001/components/exo/pointer.... components/exo/pointer.cc:35: // TODO: Some accessibility features, including large cursors, disable hardware On 2017/03/30 07:47:53, reveman wrote: > nit: TODO(name) and please add a bug number Done. TODO(oshima) to be consistent with: https://chromium.googlesource.com/chromium/src/+/master/ash/wm/ash_native_cur... https://codereview.chromium.org/2780623002/diff/80001/components/exo/pointer.... components/exo/pointer.cc:139: return ui::kCursorNull; On 2017/03/30 07:47:53, reveman wrote: > I think returning null cursor in this case is incorrect. It's critical that the > cursor is not changing when entering a new window until the client has had a > chance to replace it. This is equivalent to the old code. Also, the cursor is captured on mouse enter below, so the null cursor (i.e. the default pointer) would only appear while the capture is pending. https://codereview.chromium.org/2780623002/diff/80001/components/exo/pointer.... components/exo/pointer.cc:174: if (surface_) On 2017/03/30 07:47:53, reveman wrote: > this is incorrect I think. the client needs to provide a new cursor surface for > each enter event. The client does not do that currently. Is that required by the spec? https://codereview.chromium.org/2780623002/diff/80001/components/exo/pointer.... components/exo/pointer.cc:272: // Capture is asynchronous, so avoid rendering old cursor in the meantime. On 2017/03/30 07:47:53, reveman wrote: > I don't understand why we need to do all this. This type of special case where > we force an update makes me worried about race conditions where we incorrectly > cancel the capture of a cursor frame. It's important that cursor changes can be > pipelined properly or animated cursor will not work correctly. This special case is needed when dragging across displays. While dragging, the cursor is locked, so CursorClient::SetCursor calls are collapsed into a single call at the end of the drag. The cursor cannot be animated in that case. When crossing displays, we need to force a capture to update the scale, and hide the old cursor while the capture is pending. https://codereview.chromium.org/2780623002/diff/80001/components/exo/pointer.... components/exo/pointer.cc:341: // Prevent subsequent requests in the same frame from aborting this capture. On 2017/03/30 07:47:53, reveman wrote: > why is this needed? If the client happens to commit or call wl_pointer::set_cursor while the cursor is locked, we don't want those capture requests to cancel a pending request for a forced capture. See above for the special case where |force| is true. https://codereview.chromium.org/2780623002/diff/80001/components/exo/pointer.... components/exo/pointer.cc:389: if (force && cursor_client->IsCursorLocked()) On 2017/03/30 07:47:53, reveman wrote: > what's the situation where this is currently causing a problem? can this > IsCursorLocked part be moved to a separate CL? |force| being true implies that the cursor is locked, but the check is there for completeness. Also, note that CursorClient::SetCursor is called as well to update the state applied when the cursor is unlocked. OK, I will split out that edge case. https://codereview.chromium.org/2780623002/diff/80001/components/exo/pointer.h File components/exo/pointer.h (left): https://codereview.chromium.org/2780623002/diff/80001/components/exo/pointer.... components/exo/pointer.h:80: void OnCursorCaptured(const gfx::Point& hotspot, On 2017/03/30 07:47:53, reveman wrote: > why not const? Belongs to the dependent CL. Moved. https://codereview.chromium.org/2780623002/diff/80001/components/exo/pointer.h File components/exo/pointer.h (right): https://codereview.chromium.org/2780623002/diff/80001/components/exo/pointer.... components/exo/pointer.h:76: void CaptureCursor(bool force); On 2017/03/30 07:47:53, reveman wrote: > why do we need |force|? can the behavior with force=true be used all the time? No, it should only be used to bypass cursor locking. See pointer.cc for details. https://codereview.chromium.org/2780623002/diff/80001/components/exo/pointer.... components/exo/pointer.h:104: float device_scale_factor_ = 1.0f; On 2017/03/30 07:47:53, reveman wrote: > Please add a comment here for consistency. Done. https://codereview.chromium.org/2780623002/diff/80001/components/exo/pointer.... components/exo/pointer.h:109: const std::unique_ptr<aura::Window> cursor_; On 2017/03/30 07:47:53, reveman wrote: > why do we have to change this? We should avoid using aura::Windows except for > top level shell surfaces so I'm worried about any changes that introduce more > use of them. The snapshot is requested on this layer, because the rotation transform in the dependent CL is applied to the surface layer, relative to this layer. Also, it simplifies the code, i.e. the surface window can be unconditionally parented to this window, because this window is reparented between root windows whenever the mouse crosses displays. https://codereview.chromium.org/2780623002/diff/80001/components/exo/shell_su... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2780623002/diff/80001/components/exo/shell_su... components/exo/shell_surface.cc:1155: params.parent = parent_ ? parent_ On 2017/03/30 07:47:53, reveman wrote: > how is this change related? GetContainer gained a parameter.
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...
Description was changed from ========== exo: Fix hardware cursor edge cases The hardware cursor was only captured following a wl_pointer::set_cursor request, a surface commit, or scale change, so the cursor was sometimes stale on entering a surface. In other cases, the cursor was captured with an incorrect transform or hotspot: 1) The mouse enters a display with a different DSF or UI scale. 2) Same as above, but while the mouse is locked. 3) The internal display is not the primary display. This CL also adds a TODO for an accessibility regression. BUG=631136 BUG=642894 TEST=caroline: Cursor is captured correctly in the above cases. ========== to ========== exo: Fix hardware cursor edge cases The hardware cursor was only captured following a wl_pointer::set_cursor request, a surface commit, or scale change, so the cursor was sometimes stale on entering a surface. In other cases, the cursor was captured with an incorrect transform or hotspot: 1) The mouse enters a display with a different DSF or UI scale. 2) The internal display is not the primary display. This CL also adds a TODO for an accessibility regression. BUG=631136 BUG=642894 TEST=caroline: Cursor is captured correctly in the above cases. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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...
PTAL. Split out code related to cursor locking: https://codereview.chromium.org/2812663002/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Just looking at the description of the CL, this seems wrong. The cursor should be stale when entering a new surface. That's a requirement by the spec. I'd like to see: - One patch that only fixes the problem of using the wrong scale on some displays. - One patch for each individual case where we're failing to capture a new snapshot.
Description was changed from ========== exo: Fix hardware cursor edge cases The hardware cursor was only captured following a wl_pointer::set_cursor request, a surface commit, or scale change, so the cursor was sometimes stale on entering a surface. In other cases, the cursor was captured with an incorrect transform or hotspot: 1) The mouse enters a display with a different DSF or UI scale. 2) The internal display is not the primary display. This CL also adds a TODO for an accessibility regression. BUG=631136 BUG=642894 TEST=caroline: Cursor is captured correctly in the above cases. ========== to ========== exo: Fix hardware cursor scaling This CL fixes cases where the hardware cursor was captured with an incorrect transform or hotspot: 1) The mouse enters a display with a different DSF or UI scale. 2) The internal display is not the primary display. It also removes a redundant ui::Cursor member variable, and adds a TODO for an accessibility regression. BUG=631136 BUG=642894 TEST=caroline: Cursor is captured correctly in the above cases. ==========
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...
On 2017/04/10 20:47:13, reveman wrote: > Just looking at the description of the CL, this seems wrong. The cursor should > be stale when entering a new surface. That's a requirement by the spec. > > I'd like to see: > > - One patch that only fixes the problem of using the wrong scale on some > displays. > - One patch for each individual case where we're failing to capture a new > snapshot. Good point. After glancing at the spec, that case is a client bug. Removed. This patch now only deals with incorrect scaling.
why can't we keep the old structure of pointer code and just call UpdateCursorScale() if |focus_| is set when OnCursorDisplayChanged is called? https://codereview.chromium.org/2780623002/diff/200001/components/exo/pointer.cc File components/exo/pointer.cc (right): https://codereview.chromium.org/2780623002/diff/200001/components/exo/pointer... components/exo/pointer.cc:258: // Reparent the cursor to the root window where the mouse is located. is this re-parenting really needed? https://codereview.chromium.org/2780623002/diff/200001/components/exo/wm_help... File components/exo/wm_helper_ash.cc (right): https://codereview.chromium.org/2780623002/diff/200001/components/exo/wm_help... components/exo/wm_helper_ash.cc:76: controller ? controller->GetRootWindow() when is controller null?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/10 21:47:13, reveman wrote: > why can't we keep the old structure of pointer code and just call > UpdateCursorScale() if |focus_| is set when OnCursorDisplayChanged is called? The problem with UpdateCursorScale is the scale comparison. If the layer was reparented, the cursor must be captured even if the cumulative scale is the same, because the DSF may be different, e.g. if (DSF, UI scale) changes from (2, 0.5) to (1, 1). Also, the scale only needs to be updated in OnCursorSetChanged and OnCursorDisplayChanging, i.e. checking for scale changes in OnMouseEvent is overkill. https://codereview.chromium.org/2780623002/diff/200001/components/exo/pointer.cc File components/exo/pointer.cc (right): https://codereview.chromium.org/2780623002/diff/200001/components/exo/pointer... components/exo/pointer.cc:258: // Reparent the cursor to the root window where the mouse is located. On 2017/04/10 21:47:13, reveman wrote: > is this re-parenting really needed? Yes. Layer size is in DIPs, which depends on the DSF of the display mode. The scaling transform is relative to the display where the mouse is located, so the layer must be parented to the corresponding root window. Also, the layer must be reparented when its parent root window is destroyed because the display is removed. https://codereview.chromium.org/2780623002/diff/200001/components/exo/wm_help... File components/exo/wm_helper_ash.cc (right): https://codereview.chromium.org/2780623002/diff/200001/components/exo/wm_help... components/exo/wm_helper_ash.cc:76: controller ? controller->GetRootWindow() On 2017/04/10 21:47:13, reveman wrote: > when is controller null? When a display with |display_id| does not exist.
https://codereview.chromium.org/2780623002/diff/200001/components/exo/pointer.cc File components/exo/pointer.cc (right): https://codereview.chromium.org/2780623002/diff/200001/components/exo/pointer... components/exo/pointer.cc:258: // Reparent the cursor to the root window where the mouse is located. On 2017/04/11 at 04:42:55, Dominik Laskowski wrote: > On 2017/04/10 21:47:13, reveman wrote: > > is this re-parenting really needed? > > Yes. Layer size is in DIPs, which depends on the DSF of the display mode. The scaling transform is relative to the display where the mouse is located, so the layer must be parented to the corresponding root window. Also, the layer must be reparented when its parent root window is destroyed because the display is removed. Ok. Can we parent the cursor surface at the first SetCursor after an enter event and make sure display changes generate leave/enter events. I think that would keep all this multi display logic really simple. https://codereview.chromium.org/2780623002/diff/200001/components/exo/wm_help... File components/exo/wm_helper_ash.cc (right): https://codereview.chromium.org/2780623002/diff/200001/components/exo/wm_help... components/exo/wm_helper_ash.cc:76: controller ? controller->GetRootWindow() On 2017/04/11 at 04:42:55, Dominik Laskowski wrote: > On 2017/04/10 21:47:13, reveman wrote: > > when is controller null? > > When a display with |display_id| does not exist. and why does it need to be valid behavior to call this function with an invalid display id?
https://codereview.chromium.org/2780623002/diff/200001/components/exo/pointer.cc File components/exo/pointer.cc (right): https://codereview.chromium.org/2780623002/diff/200001/components/exo/pointer... components/exo/pointer.cc:258: // Reparent the cursor to the root window where the mouse is located. On 2017/04/11 19:25:13, reveman wrote: > On 2017/04/11 at 04:42:55, Dominik Laskowski wrote: > > On 2017/04/10 21:47:13, reveman wrote: > > > is this re-parenting really needed? > > > > Yes. Layer size is in DIPs, which depends on the DSF of the display mode. The > scaling transform is relative to the display where the mouse is located, so the > layer must be parented to the corresponding root window. Also, the layer must be > reparented when its parent root window is destroyed because the display is > removed. > > Ok. Can we parent the cursor surface at the first SetCursor after an enter event > and make sure display changes generate leave/enter events. I think that would > keep all this multi display logic really simple. That would break client-side dragging, i.e. we would need hacks to prevent the window from being dropped when dragged across a display boundary. https://codereview.chromium.org/2780623002/diff/200001/components/exo/wm_help... File components/exo/wm_helper_ash.cc (right): https://codereview.chromium.org/2780623002/diff/200001/components/exo/wm_help... components/exo/wm_helper_ash.cc:76: controller ? controller->GetRootWindow() On 2017/04/11 19:25:13, reveman wrote: > On 2017/04/11 at 04:42:55, Dominik Laskowski wrote: > > On 2017/04/10 21:47:13, reveman wrote: > > > when is controller null? > > > > When a display with |display_id| does not exist. > > and why does it need to be valid behavior to call this function with an invalid > display id? To retain the old behavior as an option, but YAGNI. Changed to return null if either |display_id| or |container_id| is invalid.
https://codereview.chromium.org/2780623002/diff/200001/components/exo/pointer.cc File components/exo/pointer.cc (right): https://codereview.chromium.org/2780623002/diff/200001/components/exo/pointer... components/exo/pointer.cc:258: // Reparent the cursor to the root window where the mouse is located. On 2017/04/11 at 20:40:37, Dominik Laskowski wrote: > On 2017/04/11 19:25:13, reveman wrote: > > On 2017/04/11 at 04:42:55, Dominik Laskowski wrote: > > > On 2017/04/10 21:47:13, reveman wrote: > > > > is this re-parenting really needed? > > > > > > Yes. Layer size is in DIPs, which depends on the DSF of the display mode. The > > scaling transform is relative to the display where the mouse is located, so the > > layer must be parented to the corresponding root window. Also, the layer must be > > reparented when its parent root window is destroyed because the display is > > removed. > > > > Ok. Can we parent the cursor surface at the first SetCursor after an enter event > > and make sure display changes generate leave/enter events. I think that would > > keep all this multi display logic really simple. > > That would break client-side dragging, i.e. we would need hacks to prevent the window from being dropped when dragged across a display boundary. Why would it break client side dragging? The compositors often need to generate enter/leave events from internal state changes so being able to handle this correctly on the client side is important I think.
On 2017/04/11 22:25:34, reveman wrote: > Why would it break client side dragging? The compositors often need to generate > enter/leave events from internal state changes so being able to handle this > correctly on the client side is important I think. Because the client aborts dragging if the window loses focus. This could be solved by server-driven dragging, but the current approach is a simple stopgap.
On 2017/04/11 22:51:56, Dominik Laskowski wrote: > On 2017/04/11 22:25:34, reveman wrote: > > Why would it break client side dragging? The compositors often need to > generate > > enter/leave events from internal state changes so being able to handle this > > correctly on the client side is important I think. > > Because the client aborts dragging if the window loses focus. This could be > solved by server-driven dragging, but the current approach is a simple stopgap. It would also complicate the dependent CL, because SetCursor would need a special case for the forced capture.
On 2017/04/12 at 00:12:20, domlaskowski wrote: > On 2017/04/11 22:51:56, Dominik Laskowski wrote: > > On 2017/04/11 22:25:34, reveman wrote: > > > Why would it break client side dragging? The compositors often need to > > generate > > > enter/leave events from internal state changes so being able to handle this > > > correctly on the client side is important I think. > > > > Because the client aborts dragging if the window loses focus. This could be > > solved by server-driven dragging, but the current approach is a simple stopgap. Shouldn't the client instead be using the configure event flag we added to determine when the drag ends? > > It would also complicate the dependent CL, because SetCursor would need a special case for the forced capture.
On 2017/04/13 19:49:44, reveman wrote: > Shouldn't the client instead be using the configure event flag we added to > determine when the drag ends? You mean the "moving" window state? Yes, but the client-side plumbing for that is not there yet. Regardless, generating a focus change just to update the cursor scale doesn't seem right.
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...
Description was changed from ========== exo: Fix hardware cursor scaling This CL fixes cases where the hardware cursor was captured with an incorrect transform or hotspot: 1) The mouse enters a display with a different DSF or UI scale. 2) The internal display is not the primary display. It also removes a redundant ui::Cursor member variable, and adds a TODO for an accessibility regression. BUG=631136 BUG=642894 TEST=caroline: Cursor is captured correctly in the above cases. ========== to ========== exo: Fix multi-display hardware cursor This CL fixes crashes in extended desktop mode caused by incorrect parenting of the cursor surface, as well as cases where the cursor was captured with an incorrect transform or hotspot: 1) The mouse enters a display with a different DSF or UI scale. 2) The internal display is not the primary display. It also removes a redundant ui::Cursor member variable, and adds a TODO for an accessibility regression. BUG=631136 BUG=642894 TEST=caroline: Cursor parenting and scaling is correct for each display. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
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...
Ping? Updated CL description. Forgot to mention the crashes fixed with a workaround in M58: https://codereview.chromium.org/2847563003/
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 multi-display hardware cursor This CL fixes crashes in extended desktop mode caused by incorrect parenting of the cursor surface, as well as cases where the cursor was captured with an incorrect transform or hotspot: 1) The mouse enters a display with a different DSF or UI scale. 2) The internal display is not the primary display. It also removes a redundant ui::Cursor member variable, and adds a TODO for an accessibility regression. BUG=631136 BUG=642894 TEST=caroline: Cursor parenting and scaling is correct for each display. ========== to ========== exo: Fix multi-display hardware cursor This CL fixes crashes in extended desktop mode caused by incorrect parenting of the cursor surface, as well as cases where the cursor was captured with an incorrect transform or hotspot: 1) The mouse enters a display with a different DSF or UI scale. 2) The internal display is not the primary display. It also removes a redundant ui::Cursor member variable, and adds a TODO for an accessibility regression. BUG=631136 BUG=642894 BUG=718627 TEST=caroline: Cursor parenting and scaling is correct for each display. ==========
Ping? This is a dependency for the other cursor CLs, and the reparenting is unavoidable for now.
I'd like us to handle this in a way that makes it perfectly clear what root window the cursor surface is parented to at any given time for snapshots. Ideally we create a window and parent it as needed for each snapshot.
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...
Description was changed from ========== exo: Fix multi-display hardware cursor This CL fixes crashes in extended desktop mode caused by incorrect parenting of the cursor surface, as well as cases where the cursor was captured with an incorrect transform or hotspot: 1) The mouse enters a display with a different DSF or UI scale. 2) The internal display is not the primary display. It also removes a redundant ui::Cursor member variable, and adds a TODO for an accessibility regression. BUG=631136 BUG=642894 BUG=718627 TEST=caroline: Cursor parenting and scaling is correct for each display. ========== to ========== exo: Fix multi-display hardware cursor This CL fixes crashes in extended desktop mode caused by incorrect parenting of the cursor surface, as well as cases where the cursor was captured with an incorrect transform or hotspot: 1) The mouse enters a display with a different DSF or UI scale. 2) The internal display is not the primary display. 3) The display is rotated. It also removes a redundant ui::Cursor member variable, and adds a TODO for an accessibility regression. BUG=631136 BUG=642894 TEST=caroline: Cursor parenting and scaling is correct for each display. TEST=caroline: Cursor on rotated displays has correct orientation. ==========
PTAL. As discussed offline, the cursor is now captured on the highest-DPI display, and scaled down using Skia on other displays. I've also squashed the CL for cursor rotation.
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.
https://codereview.chromium.org/2780623002/diff/340001/components/exo/pointer.cc File components/exo/pointer.cc (right): https://codereview.chromium.org/2780623002/diff/340001/components/exo/pointer... components/exo/pointer.cc:38: // TODO(oshima): Some accessibility features, including large cursors, disable please minimize the patch and avoid this change that doesn't seem necessary for it's functionality https://codereview.chromium.org/2780623002/diff/340001/components/exo/pointer... components/exo/pointer.cc:61: display::Display GetCaptureDisplay() { What if no DSF 2.0 display exists but is later added? Can we instead always use the primary display and set the layer scale to always capture at 2x? https://codereview.chromium.org/2780623002/diff/340001/components/exo/pointer... components/exo/pointer.cc:84: why are the following 4 lines needed? https://codereview.chromium.org/2780623002/diff/340001/components/exo/pointer.h File components/exo/pointer.h (left): https://codereview.chromium.org/2780623002/diff/340001/components/exo/pointer... components/exo/pointer.h:75: void UpdateCursorScale(); Can we keep this and instead add a UpdateCursorDeviceScaleFactorAndRotation() so the update logic stays the same as before? https://codereview.chromium.org/2780623002/diff/340001/components/exo/pointer... components/exo/pointer.h:81: void OnCursorCaptured(const gfx::Point& hotspot, I think we need to have hotspot updates flow through this. https://codereview.chromium.org/2780623002/diff/340001/components/exo/pointer.h File components/exo/pointer.h (right): https://codereview.chromium.org/2780623002/diff/340001/components/exo/pointer... components/exo/pointer.h:104: float device_scale_factor_ = 1.0f; nit: s/device_scale_factor_/cursor_device_scale_factor_/ and s/mouse/cursor sprite/ https://codereview.chromium.org/2780623002/diff/340001/components/exo/pointer... components/exo/pointer.h:107: display::Display::Rotation rotation_ = display::Display::ROTATE_0; nit: s/rotation_/cursor_rotation_/ and s/mouse/cursor sprite/ https://codereview.chromium.org/2780623002/diff/340001/components/exo/pointer... components/exo/pointer.h:113: SkBitmap cursor_; Please keep this as before and add another member for the bitmap state. ie. "SkBitmap cursor_bitmap_;". I think that will reduce the diff of the patch significantly and make it much easier to review
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...
https://codereview.chromium.org/2780623002/diff/340001/components/exo/pointer.cc File components/exo/pointer.cc (right): https://codereview.chromium.org/2780623002/diff/340001/components/exo/pointer... components/exo/pointer.cc:38: // TODO(oshima): Some accessibility features, including large cursors, disable On 2017/05/23 17:06:57, reveman wrote: > please minimize the patch and avoid this change that doesn't seem necessary for > it's functionality It's needed due to the new logic for updating scale. https://codereview.chromium.org/2780623002/diff/340001/components/exo/pointer... components/exo/pointer.cc:61: display::Display GetCaptureDisplay() { On 2017/05/23 17:06:57, reveman wrote: > What if no DSF 2.0 display exists but is later added? Can we instead always use > the primary display and set the layer scale to always capture at 2x? Good point. Capturing at constant pixel size is simpler and more predictable. Done. https://codereview.chromium.org/2780623002/diff/340001/components/exo/pointer... components/exo/pointer.cc:84: On 2017/05/23 17:06:57, reveman wrote: > why are the following 4 lines needed? To initialize the state updated by OnCursorSetChanged and OnCursorDisplayChanging. I had the impression that Ash moved the cursor to the primary display on startup, but that's not the case so I've updated the code here. https://codereview.chromium.org/2780623002/diff/340001/components/exo/pointer.h File components/exo/pointer.h (left): https://codereview.chromium.org/2780623002/diff/340001/components/exo/pointer... components/exo/pointer.h:75: void UpdateCursorScale(); On 2017/05/23 17:06:58, reveman wrote: > Can we keep this and instead add a UpdateCursorDeviceScaleFactorAndRotation() so > the update logic stays the same as before? Checking for scale/rotation changes for each mouse event is overkill, i.e. the only events requiring a cursor update are OnCursorSetChanged and OnCursorDisplayChanging. https://codereview.chromium.org/2780623002/diff/340001/components/exo/pointer... components/exo/pointer.h:81: void OnCursorCaptured(const gfx::Point& hotspot, On 2017/05/23 17:06:57, reveman wrote: > I think we need to have hotspot updates flow through this. Good point. I've updated SetCursor to save the hotspot on capture. https://codereview.chromium.org/2780623002/diff/340001/components/exo/pointer.h File components/exo/pointer.h (right): https://codereview.chromium.org/2780623002/diff/340001/components/exo/pointer... components/exo/pointer.h:104: float device_scale_factor_ = 1.0f; On 2017/05/23 17:06:58, reveman wrote: > nit: s/device_scale_factor_/cursor_device_scale_factor_/ and s/mouse/cursor > sprite/ Done. https://codereview.chromium.org/2780623002/diff/340001/components/exo/pointer... components/exo/pointer.h:107: display::Display::Rotation rotation_ = display::Display::ROTATE_0; On 2017/05/23 17:06:58, reveman wrote: > nit: s/rotation_/cursor_rotation_/ and s/mouse/cursor sprite/ Done. https://codereview.chromium.org/2780623002/diff/340001/components/exo/pointer... components/exo/pointer.h:113: SkBitmap cursor_; On 2017/05/23 17:06:58, reveman wrote: > Please keep this as before and add another member for the bitmap state. ie. > "SkBitmap cursor_bitmap_;". I think that will reduce the diff of the patch > significantly and make it much easier to review Renamed, but the ui::Cursor is redundant, i.e. CursorClient::GetCursor is equivalent.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2780623002/diff/340001/components/exo/pointer.cc File components/exo/pointer.cc (right): https://codereview.chromium.org/2780623002/diff/340001/components/exo/pointer... components/exo/pointer.cc:38: // TODO(oshima): Some accessibility features, including large cursors, disable On 2017/05/24 at 00:43:04, Dominik Laskowski wrote: > On 2017/05/23 17:06:57, reveman wrote: > > please minimize the patch and avoid this change that doesn't seem necessary for > > it's functionality > > It's needed due to the new logic for updating scale. I'm failing to see why. Please explain. https://codereview.chromium.org/2780623002/diff/340001/components/exo/pointer.h File components/exo/pointer.h (left): https://codereview.chromium.org/2780623002/diff/340001/components/exo/pointer... components/exo/pointer.h:75: void UpdateCursorScale(); On 2017/05/24 at 00:43:04, Dominik Laskowski wrote: > On 2017/05/23 17:06:58, reveman wrote: > > Can we keep this and instead add a UpdateCursorDeviceScaleFactorAndRotation() so > > the update logic stays the same as before? > > Checking for scale/rotation changes for each mouse event is overkill, i.e. the only events requiring a cursor update are OnCursorSetChanged and OnCursorDisplayChanging. Yes, let's rename this to UpdateCursor() and call that when rotation or scale might have changed. https://codereview.chromium.org/2780623002/diff/380001/components/exo/pointer.cc File components/exo/pointer.cc (left): https://codereview.chromium.org/2780623002/diff/380001/components/exo/pointer... components/exo/pointer.cc:80: // This is used to avoid unnecessary cursor changes. I'd like to keep this logic as before. https://codereview.chromium.org/2780623002/diff/380001/components/exo/pointer.cc File components/exo/pointer.cc (right): https://codereview.chromium.org/2780623002/diff/380001/components/exo/pointer... components/exo/pointer.cc:41: return cursor_set == ui::CURSOR_SET_LARGE ? 2.8f : 1.0f; I still prefer if this was folded into UpdateCursor as before. Let's not change more than necessary in this patch. https://codereview.chromium.org/2780623002/diff/380001/components/exo/pointer... components/exo/pointer.cc:54: inline const display::ManagedDisplayInfo& GetDisplayInfo( this helper doesn't seem useful. please use WMHelper::GetInstance() directly instead. https://codereview.chromium.org/2780623002/diff/380001/components/exo/pointer... components/exo/pointer.cc:78: screen->GetDisplayNearestPoint(screen->GetCursorScreenPoint())); these lines should not be necessary after addressing my other feedback. ping me if not clear why. https://codereview.chromium.org/2780623002/diff/380001/components/exo/pointer... components/exo/pointer.cc:272: cursor_device_scale_factor_ = GetDisplayInfo(display).device_scale_factor(); hm, do we need |cursor_scale_| and |cursor_rotation_| at all. Let's just do: if (focus_) UpdateCursor(); here and in the function above? https://codereview.chromium.org/2780623002/diff/380001/components/exo/pointer.h File components/exo/pointer.h (left): https://codereview.chromium.org/2780623002/diff/380001/components/exo/pointer... components/exo/pointer.h:106: ui::Cursor cursor_; Please keep cursor in addition to bitmap as it's hard to see if GetCursor always return the right thing otherwise and what the lifetime of this is otherwise. https://codereview.chromium.org/2780623002/diff/380001/components/exo/pointer.h File components/exo/pointer.h (right): https://codereview.chromium.org/2780623002/diff/380001/components/exo/pointer... components/exo/pointer.h:78: void CaptureCursor(const gfx::Point& hotspot); why do we need to pass hotspot as an argument to this now? https://codereview.chromium.org/2780623002/diff/380001/components/exo/pointer... components/exo/pointer.h:87: void SetCursor(gfx::NativeCursor cursor); Please a comment for this function and I think we should remove the argument and have it use |cursor_| instead. https://codereview.chromium.org/2780623002/diff/380001/components/exo/pointer... components/exo/pointer.h:102: float cursor_scale_ = 1.0f; Please keep this the scale of |cursor_| as before. https://codereview.chromium.org/2780623002/diff/380001/components/exo/pointer... components/exo/pointer.h:105: float cursor_device_scale_factor_ = 1.0f; Actually, I don't think we need this as all it should do is affect cursor_scale_. cursor_scale_ is set to the scale that is currently used by |cursor_|. https://codereview.chromium.org/2780623002/diff/380001/components/exo/wm_help... File components/exo/wm_helper.h (right): https://codereview.chromium.org/2780623002/diff/380001/components/exo/wm_help... components/exo/wm_helper.h:105: virtual aura::Window* GetContainer(int64_t display_id, int container_id) = 0; I don't think we need this anymore.
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 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...
Description was changed from ========== exo: Fix multi-display hardware cursor This CL fixes crashes in extended desktop mode caused by incorrect parenting of the cursor surface, as well as cases where the cursor was captured with an incorrect transform or hotspot: 1) The mouse enters a display with a different DSF or UI scale. 2) The internal display is not the primary display. 3) The display is rotated. It also removes a redundant ui::Cursor member variable, and adds a TODO for an accessibility regression. BUG=631136 BUG=642894 TEST=caroline: Cursor parenting and scaling is correct for each display. TEST=caroline: Cursor on rotated displays has correct orientation. ========== to ========== exo: Fix multi-display hardware cursor This CL fixes crashes in extended desktop mode caused by incorrect parenting of the cursor surface, as well as cases where the cursor was captured with an incorrect transform or hotspot: 1) The mouse enters a display with a different DSF or UI scale. 2) The internal display is not the primary display. 3) The display is rotated. It also adds a TODO for an accessibility regression. BUG=631136 BUG=642894 TEST=caroline: Cursor parenting and scaling is correct for each display. TEST=caroline: Cursor on rotated displays has correct orientation. ==========
reveman: PTAL. Refactored as discussed offline. oshima: PTAL at ash/. Should I add public members to ash::Shell instead of exposing AshNativeCursorManager? https://codereview.chromium.org/2780623002/diff/340001/components/exo/pointer.cc File components/exo/pointer.cc (right): https://codereview.chromium.org/2780623002/diff/340001/components/exo/pointer... components/exo/pointer.cc:38: // TODO(oshima): Some accessibility features, including large cursors, disable On 2017/05/25 10:52:55, reveman wrote: > I'm failing to see why. Please explain. Folded into UpdateCursor. https://codereview.chromium.org/2780623002/diff/340001/components/exo/pointer.h File components/exo/pointer.h (left): https://codereview.chromium.org/2780623002/diff/340001/components/exo/pointer... components/exo/pointer.h:75: void UpdateCursorScale(); On 2017/05/25 10:52:55, reveman wrote: > On 2017/05/24 at 00:43:04, Dominik Laskowski wrote: > > On 2017/05/23 17:06:58, reveman wrote: > > > Can we keep this and instead add a > UpdateCursorDeviceScaleFactorAndRotation() so > > > the update logic stays the same as before? > > > > Checking for scale/rotation changes for each mouse event is overkill, i.e. the > only events requiring a cursor update are OnCursorSetChanged and > OnCursorDisplayChanging. > > Yes, let's rename this to UpdateCursor() and call that when rotation or scale > might have changed. Done. https://codereview.chromium.org/2780623002/diff/380001/components/exo/pointer.cc File components/exo/pointer.cc (left): https://codereview.chromium.org/2780623002/diff/380001/components/exo/pointer... components/exo/pointer.cc:80: // This is used to avoid unnecessary cursor changes. On 2017/05/25 10:52:55, reveman wrote: > I'd like to keep this logic as before. Done. https://codereview.chromium.org/2780623002/diff/380001/components/exo/pointer.cc File components/exo/pointer.cc (right): https://codereview.chromium.org/2780623002/diff/380001/components/exo/pointer... components/exo/pointer.cc:41: return cursor_set == ui::CURSOR_SET_LARGE ? 2.8f : 1.0f; On 2017/05/25 10:52:55, reveman wrote: > I still prefer if this was folded into UpdateCursor as before. Let's not change > more than necessary in this patch. Done. https://codereview.chromium.org/2780623002/diff/380001/components/exo/pointer... components/exo/pointer.cc:54: inline const display::ManagedDisplayInfo& GetDisplayInfo( On 2017/05/25 10:52:55, reveman wrote: > this helper doesn't seem useful. please use WMHelper::GetInstance() directly > instead. Done. https://codereview.chromium.org/2780623002/diff/380001/components/exo/pointer... components/exo/pointer.cc:78: screen->GetDisplayNearestPoint(screen->GetCursorScreenPoint())); On 2017/05/25 10:52:55, reveman wrote: > these lines should not be necessary after addressing my other feedback. ping me > if not clear why. Done. https://codereview.chromium.org/2780623002/diff/380001/components/exo/pointer... components/exo/pointer.cc:272: cursor_device_scale_factor_ = GetDisplayInfo(display).device_scale_factor(); On 2017/05/25 10:52:55, reveman wrote: > hm, do we need |cursor_scale_| and |cursor_rotation_| at all. Let's just do: > > if (focus_) > UpdateCursor(); > > here and in the function above? Done. https://codereview.chromium.org/2780623002/diff/380001/components/exo/pointer.h File components/exo/pointer.h (left): https://codereview.chromium.org/2780623002/diff/380001/components/exo/pointer... components/exo/pointer.h:106: ui::Cursor cursor_; On 2017/05/25 10:52:55, reveman wrote: > Please keep cursor in addition to bitmap as it's hard to see if GetCursor always > return the right thing otherwise and what the lifetime of this is otherwise. Done. https://codereview.chromium.org/2780623002/diff/380001/components/exo/pointer.h File components/exo/pointer.h (right): https://codereview.chromium.org/2780623002/diff/380001/components/exo/pointer... components/exo/pointer.h:78: void CaptureCursor(const gfx::Point& hotspot); On 2017/05/25 10:52:55, reveman wrote: > why do we need to pass hotspot as an argument to this now? As discussed offline, the bitmap and hotspot are updated together. https://codereview.chromium.org/2780623002/diff/380001/components/exo/pointer... components/exo/pointer.h:87: void SetCursor(gfx::NativeCursor cursor); On 2017/05/25 10:52:55, reveman wrote: > Please a comment for this function and I think we should remove the argument and > have it use |cursor_| instead. Done. https://codereview.chromium.org/2780623002/diff/380001/components/exo/pointer... components/exo/pointer.h:102: float cursor_scale_ = 1.0f; On 2017/05/25 10:52:55, reveman wrote: > Please keep this the scale of |cursor_| as before. Removed per offline chat. https://codereview.chromium.org/2780623002/diff/380001/components/exo/pointer... components/exo/pointer.h:105: float cursor_device_scale_factor_ = 1.0f; On 2017/05/25 10:52:55, reveman wrote: > Actually, I don't think we need this as all it should do is affect > cursor_scale_. cursor_scale_ is set to the scale that is currently used by > |cursor_|. Removed per offline chat. https://codereview.chromium.org/2780623002/diff/380001/components/exo/wm_help... File components/exo/wm_helper.h (right): https://codereview.chromium.org/2780623002/diff/380001/components/exo/wm_help... components/exo/wm_helper.h:105: virtual aura::Window* GetContainer(int64_t display_id, int container_id) = 0; On 2017/05/25 10:52:55, reveman wrote: > I don't think we need this anymore. Still needed to parent the cursor surface to the primary root window.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
exo changes look great! a few comments and maybe still remove the wm_helper change unless I'm missing something? https://codereview.chromium.org/2780623002/diff/380001/components/exo/wm_help... File components/exo/wm_helper.h (right): https://codereview.chromium.org/2780623002/diff/380001/components/exo/wm_help... components/exo/wm_helper.h:105: virtual aura::Window* GetContainer(int64_t display_id, int container_id) = 0; On 2017/05/31 at 02:06:27, Dominik Laskowski wrote: > On 2017/05/25 10:52:55, reveman wrote: > > I don't think we need this anymore. > > Still needed to parent the cursor surface to the primary root window. Isn't this always returning the container for the primary root window? You can rename it to GetPrimaryDisplayContainer if you like but let's not make it less specific unless we have to. https://codereview.chromium.org/2780623002/diff/440001/ash/wm/ash_native_curs... File ash/wm/ash_native_cursor_manager.h (right): https://codereview.chromium.org/2780623002/diff/440001/ash/wm/ash_native_curs... ash/wm/ash_native_cursor_manager.h:39: virtual void OnCursorDisplayChanged(const display::Display& display) = 0; Can we instead add this to CursorClientObserver and the cursor_display() getter to the CursorManager class? https://codereview.chromium.org/2780623002/diff/440001/components/exo/pointer.cc File components/exo/pointer.cc (right): https://codereview.chromium.org/2780623002/diff/440001/components/exo/pointer... components/exo/pointer.cc:44: const float kCursorCaptureScale = 2.0f; nit: blank line below this and a short comment above would be nice. https://codereview.chromium.org/2780623002/diff/440001/components/exo/pointer... components/exo/pointer.cc:364: if (cursor_bitmap_.drawsNothing()) should this set the cursor to None? https://codereview.chromium.org/2780623002/diff/440001/components/exo/pointer.h File components/exo/pointer.h (right): https://codereview.chromium.org/2780623002/diff/440001/components/exo/pointer... components/exo/pointer.h:83: // Synchronously update the cursor to the latest snapshot. nit: not sure synchronously needs to be mentioned but can mentioning that it will also reflect scale and rotation would be nice
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: Try jobs failed on following builders: linux_chromium_headless_rel 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...
oshima: Never mind. Reverted AshNativeCursorManager changes. https://codereview.chromium.org/2780623002/diff/380001/components/exo/wm_help... File components/exo/wm_helper.h (right): https://codereview.chromium.org/2780623002/diff/380001/components/exo/wm_help... components/exo/wm_helper.h:105: virtual aura::Window* GetContainer(int64_t display_id, int container_id) = 0; On 2017/05/31 03:33:10, reveman wrote: > Isn't this always returning the container for the primary root window? You can > rename it to GetPrimaryDisplayContainer if you like but let's not make it less > specific unless we have to. Renamed to GetPrimaryDisplayContainer. https://codereview.chromium.org/2780623002/diff/440001/ash/wm/ash_native_curs... File ash/wm/ash_native_cursor_manager.h (right): https://codereview.chromium.org/2780623002/diff/440001/ash/wm/ash_native_curs... ash/wm/ash_native_cursor_manager.h:39: virtual void OnCursorDisplayChanged(const display::Display& display) = 0; On 2017/05/31 03:33:10, reveman wrote: > Can we instead add this to CursorClientObserver and the cursor_display() getter > to the CursorManager class? Good point. Much better than exposing internals. https://codereview.chromium.org/2780623002/diff/440001/components/exo/pointer.cc File components/exo/pointer.cc (right): https://codereview.chromium.org/2780623002/diff/440001/components/exo/pointer... components/exo/pointer.cc:44: const float kCursorCaptureScale = 2.0f; On 2017/05/31 03:33:10, reveman wrote: > nit: blank line below this and a short comment above would be nice. Done. https://codereview.chromium.org/2780623002/diff/440001/components/exo/pointer... components/exo/pointer.cc:364: if (cursor_bitmap_.drawsNothing()) On 2017/05/31 03:33:10, reveman wrote: > should this set the cursor to None? Done. I've also merged SetCursor into UpdateCursor, since the callers who reset the bitmap can call UpdateCursor instead. https://codereview.chromium.org/2780623002/diff/440001/components/exo/pointer.h File components/exo/pointer.h (right): https://codereview.chromium.org/2780623002/diff/440001/components/exo/pointer... components/exo/pointer.h:83: // Synchronously update the cursor to the latest snapshot. On 2017/05/31 03:33:10, reveman wrote: > nit: not sure synchronously needs to be mentioned but can mentioning that it > will also reflect scale and rotation would be nice Done.
Nice! LGTM
domlaskowski@chromium.org changed reviewers: + dtseng@chromium.org, sky@chromium.org
Thanks! +sky for ui/ +dtseng for arc/accessibility/
LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by domlaskowski@chromium.org
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": 480001, "attempt_start_ts": 1496340334416000,
"parent_rev": "c3cf4c7d10c4796ef841fc181d0207cfdc4ad252", "commit_rev":
"ba20af04ad131e2fc7b8fd4142cffd7461057d5b"}
Message was sent while issue was closed.
Description was changed from ========== exo: Fix multi-display hardware cursor This CL fixes crashes in extended desktop mode caused by incorrect parenting of the cursor surface, as well as cases where the cursor was captured with an incorrect transform or hotspot: 1) The mouse enters a display with a different DSF or UI scale. 2) The internal display is not the primary display. 3) The display is rotated. It also adds a TODO for an accessibility regression. BUG=631136 BUG=642894 TEST=caroline: Cursor parenting and scaling is correct for each display. TEST=caroline: Cursor on rotated displays has correct orientation. ========== to ========== exo: Fix multi-display hardware cursor This CL fixes crashes in extended desktop mode caused by incorrect parenting of the cursor surface, as well as cases where the cursor was captured with an incorrect transform or hotspot: 1) The mouse enters a display with a different DSF or UI scale. 2) The internal display is not the primary display. 3) The display is rotated. It also adds a TODO for an accessibility regression. BUG=631136 BUG=642894 TEST=caroline: Cursor parenting and scaling is correct for each display. TEST=caroline: Cursor on rotated displays has correct orientation. Review-Url: https://codereview.chromium.org/2780623002 Cr-Commit-Position: refs/heads/master@{#476353} Committed: https://chromium.googlesource.com/chromium/src/+/ba20af04ad131e2fc7b8fd4142cf... ==========
Message was sent while issue was closed.
Committed patchset #25 (id:480001) as https://chromium.googlesource.com/chromium/src/+/ba20af04ad131e2fc7b8fd4142cf... |
