Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2778943005:
Keep root_location to be in pixels and display coordinates in WS. (Closed)
DescriptionKeep root_location to be in pixels and display coordinates in WS.
We convert event root_location to be in screen coordinates which
mixed pixels and dip in PlatformDisplayDefault::UpdateEventRootLocation().
Then in WindowManagerState::GetRootWindowContaining(), we expect
root_location to be in screen coordinates to find the display that
event is on and mixed pixels and dip again. As root_location should
always be in pixels in window server, changing it to skip the
conversion in PlatformDisplayDefault::UpdateEventRootLocation() so
root_location remains in pixels and display coordinates after that;
and in WindowManagerState::GetRootWindowContaining(), using display
id and display coordinates to get the root_location in screen
coordinates and find the display (not actually updating root_location
to screen coordinates). mouse_pointer_last_location is also in pixels
and display coordinates now as a result as it is the same as event
root_location.
TODO: make sure this works with drag-and-drop once mouse warp is
functional with mushroom.
BUG=701036
TEST= mus_ws_unittests
manual with two displays, one of which is hi-dpi
./out/oxygen/chrome --user-data-dir=/tmp/chrome --mus
--screen-config=1600x900,1600x900^300/i
Review-Url: https://codereview.chromium.org/2778943005
Cr-Commit-Position: refs/heads/master@{#475133}
Committed: https://chromium.googlesource.com/chromium/src/+/0fb00abce4d3e56ef50a1e6fc1eee947d97036f0
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : rebase #Patch Set 4 : . #Patch Set 5 : . #
Total comments: 13
Patch Set 6 : pass display_id #Patch Set 7 : TODO #
Total comments: 18
Patch Set 8 : comments #Patch Set 9 : drop events #
Total comments: 2
Patch Set 10 : comment #Patch Set 11 : const #
Messages
Total messages: 53 (32 generated)
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: CQ has no permission to schedule in bucket master.tryserver.chromium.linux
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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Description was changed from ========== Keep root_location to be in pixels and display coordinates in WS. BUG=701036 ========== to ========== Keep root_location to be in pixels and display coordinates in WS. We convert event root_location to be in screen coordinates which mixed pixels and dip in PlatformDisplayDefault::UpdateEventRootLocation(). Then in WindowManagerState::GetRootWindowContaining(), we expect root_location to be in screen coordinates to find the display that event is on and mixed pixels and dip again. As root_location should always be in pixels in window server, changing it to skip the conversion in PlatformDisplayDefault::UpdateEventRootLocation() so root_location remains in pixels and display coordinates after that; and in WindowManagerState::GetRootWindowContaining(), using display id and display coordinates to get the root_location in screen coordinates and find the display (not actually updating root_location to screen coordinates). mouse_pointer_last_location is also in pixels and display coordinates now as a result as it is the same as event root_location. BUG=701036 ==========
Description was changed from ========== Keep root_location to be in pixels and display coordinates in WS. We convert event root_location to be in screen coordinates which mixed pixels and dip in PlatformDisplayDefault::UpdateEventRootLocation(). Then in WindowManagerState::GetRootWindowContaining(), we expect root_location to be in screen coordinates to find the display that event is on and mixed pixels and dip again. As root_location should always be in pixels in window server, changing it to skip the conversion in PlatformDisplayDefault::UpdateEventRootLocation() so root_location remains in pixels and display coordinates after that; and in WindowManagerState::GetRootWindowContaining(), using display id and display coordinates to get the root_location in screen coordinates and find the display (not actually updating root_location to screen coordinates). mouse_pointer_last_location is also in pixels and display coordinates now as a result as it is the same as event root_location. BUG=701036 ========== to ========== Keep root_location to be in pixels and display coordinates in WS. We convert event root_location to be in screen coordinates which mixed pixels and dip in PlatformDisplayDefault::UpdateEventRootLocation(). Then in WindowManagerState::GetRootWindowContaining(), we expect root_location to be in screen coordinates to find the display that event is on and mixed pixels and dip again. As root_location should always be in pixels in window server, changing it to skip the conversion in PlatformDisplayDefault::UpdateEventRootLocation() so root_location remains in pixels and display coordinates after that; and in WindowManagerState::GetRootWindowContaining(), using display id and display coordinates to get the root_location in screen coordinates and find the display (not actually updating root_location to screen coordinates). mouse_pointer_last_location is also in pixels and display coordinates now as a result as it is the same as event root_location. BUG=701036 ==========
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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #5 (id:80001) has been deleted
Description was changed from
==========
Keep root_location to be in pixels and display coordinates in WS.
We convert event root_location to be in screen coordinates which
mixed pixels and dip in PlatformDisplayDefault::UpdateEventRootLocation().
Then in WindowManagerState::GetRootWindowContaining(), we expect
root_location to be in screen coordinates to find the display that
event is on and mixed pixels and dip again. As root_location should
always be in pixels in window server, changing it to skip the
conversion in PlatformDisplayDefault::UpdateEventRootLocation() so
root_location remains in pixels and display coordinates after that;
and in WindowManagerState::GetRootWindowContaining(), using display
id and display coordinates to get the root_location in screen
coordinates and find the display (not actually updating root_location
to screen coordinates). mouse_pointer_last_location is also in pixels
and display coordinates now as a result as it is the same as event
root_location.
BUG=701036
==========
to
==========
Keep root_location to be in pixels and display coordinates in WS.
We convert event root_location to be in screen coordinates which
mixed pixels and dip in PlatformDisplayDefault::UpdateEventRootLocation().
Then in WindowManagerState::GetRootWindowContaining(), we expect
root_location to be in screen coordinates to find the display that
event is on and mixed pixels and dip again. As root_location should
always be in pixels in window server, changing it to skip the
conversion in PlatformDisplayDefault::UpdateEventRootLocation() so
root_location remains in pixels and display coordinates after that;
and in WindowManagerState::GetRootWindowContaining(), using display
id and display coordinates to get the root_location in screen
coordinates and find the display (not actually updating root_location
to screen coordinates). mouse_pointer_last_location is also in pixels
and display coordinates now as a result as it is the same as event
root_location.
BUG=701036
TEST=manual with two displays, one of which is hi-dpi
./out/oxygen/chrome --user-data-dir=/tmp/chrome --mus
--screen-config=1600x900,1600x900^300/i
==========
Description was changed from
==========
Keep root_location to be in pixels and display coordinates in WS.
We convert event root_location to be in screen coordinates which
mixed pixels and dip in PlatformDisplayDefault::UpdateEventRootLocation().
Then in WindowManagerState::GetRootWindowContaining(), we expect
root_location to be in screen coordinates to find the display that
event is on and mixed pixels and dip again. As root_location should
always be in pixels in window server, changing it to skip the
conversion in PlatformDisplayDefault::UpdateEventRootLocation() so
root_location remains in pixels and display coordinates after that;
and in WindowManagerState::GetRootWindowContaining(), using display
id and display coordinates to get the root_location in screen
coordinates and find the display (not actually updating root_location
to screen coordinates). mouse_pointer_last_location is also in pixels
and display coordinates now as a result as it is the same as event
root_location.
BUG=701036
TEST=manual with two displays, one of which is hi-dpi
./out/oxygen/chrome --user-data-dir=/tmp/chrome --mus
--screen-config=1600x900,1600x900^300/i
==========
to
==========
Keep root_location to be in pixels and display coordinates in WS.
We convert event root_location to be in screen coordinates which
mixed pixels and dip in PlatformDisplayDefault::UpdateEventRootLocation().
Then in WindowManagerState::GetRootWindowContaining(), we expect
root_location to be in screen coordinates to find the display that
event is on and mixed pixels and dip again. As root_location should
always be in pixels in window server, changing it to skip the
conversion in PlatformDisplayDefault::UpdateEventRootLocation() so
root_location remains in pixels and display coordinates after that;
and in WindowManagerState::GetRootWindowContaining(), using display
id and display coordinates to get the root_location in screen
coordinates and find the display (not actually updating root_location
to screen coordinates). mouse_pointer_last_location is also in pixels
and display coordinates now as a result as it is the same as event
root_location.
TODO: make sure this works with drag-and-drop once mouse warp is
functional with mushroom.
BUG=701036
TEST=manual with two displays, one of which is hi-dpi
./out/oxygen/chrome --user-data-dir=/tmp/chrome --mus
--screen-config=1600x900,1600x900^300/i
==========
riajiang@chromium.org changed reviewers: + sadrul@chromium.org, sky@chromium.org
Hi, PTAL, thanks!
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2778943005/diff/100001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.h (right): https://codereview.chromium.org/2778943005/diff/100001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.h:62: void SetMousePointerDisplayLocationAndId(const gfx::Point& display_location, optional: the new name is rather verbose. Leave the old name, and add display_id. https://codereview.chromium.org/2778943005/diff/100001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.h:281: // Id of the display the most recent event is on. Under what circumstances is mouse_pointer_display_id and event_dispaly_id_ different? https://codereview.chromium.org/2778943005/diff/100001/services/ui/ws/window_... File services/ui/ws/window_manager_state.cc (right): https://codereview.chromium.org/2778943005/diff/100001/services/ui/ws/window_... services/ui/ws/window_manager_state.cc:639: ->OnMouseCursorLocationChanged(point_in_screen); Won't this lead to rounding errors? By that I mean should this be in float? Is the client code expecting DIPs right now? Please also update the documentation for CursorLocationManager to say it's in dips. https://codereview.chromium.org/2778943005/diff/100001/services/ui/ws/window_... services/ui/ws/window_manager_state.cc:693: int64_t* display_id) { I'm still confused about this. I thought all events originate with a display_id and this code shouldn't need to figure it out? https://codereview.chromium.org/2778943005/diff/100001/services/ui/ws/window_... File services/ui/ws/window_manager_state.h (right): https://codereview.chromium.org/2778943005/diff/100001/services/ui/ws/window_... services/ui/ws/window_manager_state.h:298: int64_t event_processing_display_id_ = 0; I had added this to avoid sending the display_id from EventDispatcher back to the delegate. Now that EventDispatcher knows the display_id can you remove this? It may require passing the display_id to the delegate in more cases, but you've already gone down that path so it should be ok.
And thanks for taking this on
https://codereview.chromium.org/2778943005/diff/100001/services/ui/ws/window_... File services/ui/ws/window_manager_state.cc (right): https://codereview.chromium.org/2778943005/diff/100001/services/ui/ws/window_... services/ui/ws/window_manager_state.cc:639: ->OnMouseCursorLocationChanged(point_in_screen); On 2017/05/17 16:53:42, sky wrote: > Won't this lead to rounding errors? By that I mean should this be in float? Is > the client code expecting DIPs right now? Please also update the documentation > for CursorLocationManager to say it's in dips. Don't worry about the rounding errors for now. We can deal with that later.
https://codereview.chromium.org/2778943005/diff/100001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.h (right): https://codereview.chromium.org/2778943005/diff/100001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.h:62: void SetMousePointerDisplayLocationAndId(const gfx::Point& display_location, On 2017/05/17 16:53:41, sky wrote: > optional: the new name is rather verbose. Leave the old name, and add > display_id. Done. https://codereview.chromium.org/2778943005/diff/100001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.h:281: // Id of the display the most recent event is on. On 2017/05/17 16:53:41, sky wrote: > Under what circumstances is mouse_pointer_display_id and event_dispaly_id_ > different? We don't update mouse specific info (mouse_pointer_last_location_ and mouse_pointer_display_id_) if the event coming in is not a mouse event, but event_display_id_ would be changed in that case. https://codereview.chromium.org/2778943005/diff/100001/services/ui/ws/window_... File services/ui/ws/window_manager_state.cc (right): https://codereview.chromium.org/2778943005/diff/100001/services/ui/ws/window_... services/ui/ws/window_manager_state.cc:639: ->OnMouseCursorLocationChanged(point_in_screen); On 2017/05/17 19:02:07, sky wrote: > On 2017/05/17 16:53:42, sky wrote: > > Won't this lead to rounding errors? By that I mean should this be in float? Is > > the client code expecting DIPs right now? Please also update the documentation > > for CursorLocationManager to say it's in dips. > > Don't worry about the rounding errors for now. We can deal with that later. This won't introduce new rounding error? We were also converting from display coordinates to screen coordinates before for the event root_location which is where mouse cursor location comes from? CursorLocationManager is expecting screen-coord and DIP (updated comment) because this OnMouseCursorLocationChanged was taking in mouse cursor location in screen-coord and DIP. https://codereview.chromium.org/2778943005/diff/100001/services/ui/ws/window_... services/ui/ws/window_manager_state.cc:693: int64_t* display_id) { On 2017/05/17 16:53:42, sky wrote: > I'm still confused about this. I thought all events originate with a display_id > and this code shouldn't need to figure it out? For most events the display id for the |target_display_root| we found would be the same as the |display_id| param. But if it's a drag-and-drop, the mouse move event would be relative to the first display even when we have moved the mouse to the second display. In that case, after this function call, |location_in_display| and |display_id| would be updated to be relative to the second display. Am I misunderstanding sth? https://codereview.chromium.org/2778943005/diff/100001/services/ui/ws/window_... File services/ui/ws/window_manager_state.h (right): https://codereview.chromium.org/2778943005/diff/100001/services/ui/ws/window_... services/ui/ws/window_manager_state.h:298: int64_t event_processing_display_id_ = 0; On 2017/05/17 16:53:42, sky wrote: > I had added this to avoid sending the display_id from EventDispatcher back to > the delegate. Now that EventDispatcher knows the display_id can you remove this? > It may require passing the display_id to the delegate in more cases, but you've > already gone down that path so it should be ok. Removed event_processing_display_id_ and changed to pass display_id from EventDispatcher to the delegate.
Description was changed from
==========
Keep root_location to be in pixels and display coordinates in WS.
We convert event root_location to be in screen coordinates which
mixed pixels and dip in PlatformDisplayDefault::UpdateEventRootLocation().
Then in WindowManagerState::GetRootWindowContaining(), we expect
root_location to be in screen coordinates to find the display that
event is on and mixed pixels and dip again. As root_location should
always be in pixels in window server, changing it to skip the
conversion in PlatformDisplayDefault::UpdateEventRootLocation() so
root_location remains in pixels and display coordinates after that;
and in WindowManagerState::GetRootWindowContaining(), using display
id and display coordinates to get the root_location in screen
coordinates and find the display (not actually updating root_location
to screen coordinates). mouse_pointer_last_location is also in pixels
and display coordinates now as a result as it is the same as event
root_location.
TODO: make sure this works with drag-and-drop once mouse warp is
functional with mushroom.
BUG=701036
TEST=manual with two displays, one of which is hi-dpi
./out/oxygen/chrome --user-data-dir=/tmp/chrome --mus
--screen-config=1600x900,1600x900^300/i
==========
to
==========
Keep root_location to be in pixels and display coordinates in WS.
We convert event root_location to be in screen coordinates which
mixed pixels and dip in PlatformDisplayDefault::UpdateEventRootLocation().
Then in WindowManagerState::GetRootWindowContaining(), we expect
root_location to be in screen coordinates to find the display that
event is on and mixed pixels and dip again. As root_location should
always be in pixels in window server, changing it to skip the
conversion in PlatformDisplayDefault::UpdateEventRootLocation() so
root_location remains in pixels and display coordinates after that;
and in WindowManagerState::GetRootWindowContaining(), using display
id and display coordinates to get the root_location in screen
coordinates and find the display (not actually updating root_location
to screen coordinates). mouse_pointer_last_location is also in pixels
and display coordinates now as a result as it is the same as event
root_location.
TODO: make sure this works with drag-and-drop once mouse warp is
functional with mushroom.
BUG=701036
TEST= mus_ws_unittests
manual with two displays, one of which is hi-dpi
./out/oxygen/chrome --user-data-dir=/tmp/chrome --mus
--screen-config=1600x900,1600x900^300/i
==========
https://codereview.chromium.org/2778943005/diff/100001/services/ui/ws/window_... File services/ui/ws/window_manager_state.cc (right): https://codereview.chromium.org/2778943005/diff/100001/services/ui/ws/window_... services/ui/ws/window_manager_state.cc:639: ->OnMouseCursorLocationChanged(point_in_screen); On 2017/05/19 20:45:05, riajiang wrote: > On 2017/05/17 19:02:07, sky wrote: > > On 2017/05/17 16:53:42, sky wrote: > > > Won't this lead to rounding errors? By that I mean should this be in float? > Is > > > the client code expecting DIPs right now? Please also update the > documentation > > > for CursorLocationManager to say it's in dips. > > > > Don't worry about the rounding errors for now. We can deal with that later. > > This won't introduce new rounding error? We were also converting from display > coordinates to screen coordinates before for the event root_location which is > where mouse cursor location comes from? > > CursorLocationManager is expecting screen-coord and DIP (updated comment) > because this OnMouseCursorLocationChanged was taking in mouse cursor location in > screen-coord and DIP. Before we were setting the location to pixels, so no rounding errors. Once you convert to DIPs as ints then it is impossible for client code that sees the dip location to convert back to pixels and keep the precision. But this can be dealt with separately. https://codereview.chromium.org/2778943005/diff/100001/services/ui/ws/window_... services/ui/ws/window_manager_state.cc:693: int64_t* display_id) { On 2017/05/19 20:45:05, riajiang wrote: > On 2017/05/17 16:53:42, sky wrote: > > I'm still confused about this. I thought all events originate with a > display_id > > and this code shouldn't need to figure it out? > > For most events the display id for the |target_display_root| we found would be > the same as the |display_id| param. But if it's a drag-and-drop, the mouse move > event would be relative to the first display even when we have moved the mouse > to the second display. In that case, after this function call, > |location_in_display| and |display_id| would be updated to be relative to the > second display. Am I misunderstanding sth? Are you *sure* about this? Again, I believe events always originate from the display the cursor is on. Even during drag and drop. My understanding is that the only way for the cursor onto a different display is via warp. Have you tried this on an actual device? If running on the desktop produces different behavior I would say we should make the desktop give the same behavior so we don't need to special case it. Sadrul knows more than I in this area, so I encourage you to talk with him about how to handle this.
I'm going to wait to review until you resolve the display_id/drag-n-drop issue. Ping once that is resolved and I'll take another look.
On 2017/05/19 22:12:45, sky wrote: > I'm going to wait to review until you resolve the display_id/drag-n-drop issue. > Ping once that is resolved and I'll take another look. As discussed, since for both device and workstation, a drag-n-drop starting from a source display S to a target display T, all events are relative to S (display_id and location) until after the last mouse_release event on T because ozone drm would check for capture window, we are going to keep the conversion here for now. Filed a bug crbug.com/726470 and added TODO.
https://codereview.chromium.org/2778943005/diff/140001/services/ui/ws/cursor_... File services/ui/ws/cursor_location_manager.h (right): https://codereview.chromium.org/2778943005/diff/140001/services/ui/ws/cursor_... services/ui/ws/cursor_location_manager.h:27: void OnMouseCursorLocationChanged(const gfx::Point& point); Please rename to point_in_dips to make that clear. https://codereview.chromium.org/2778943005/diff/140001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.h (right): https://codereview.chromium.org/2778943005/diff/140001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.h:62: void SetMousePointerDisplayLocation(const gfx::Point& display_location, InDips? https://codereview.chromium.org/2778943005/diff/140001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.h:278: int64_t mouse_pointer_display_id_ = 0; kInvalidDisplayId. https://codereview.chromium.org/2778943005/diff/140001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.h:281: int64_t event_display_id_ = 0; kInvalidDisplayId https://codereview.chromium.org/2778943005/diff/140001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher_delegate.h (right): https://codereview.chromium.org/2778943005/diff/140001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_delegate.h:77: // |location_in_display| should be in display coordinates and in pixels. 'should be' -> 'are in' https://codereview.chromium.org/2778943005/diff/140001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_delegate.h:78: // |location_in_display| and |display_id| will be updated if the window we 'will be updated' -> 'are updated' https://codereview.chromium.org/2778943005/diff/140001/services/ui/ws/window_... File services/ui/ws/window_manager_state.cc (right): https://codereview.chromium.org/2778943005/diff/140001/services/ui/ws/window_... services/ui/ws/window_manager_state.cc:553: if (display) { When would there not be a display? https://codereview.chromium.org/2778943005/diff/140001/services/ui/ws/window_... File services/ui/ws/window_manager_state.h (right): https://codereview.chromium.org/2778943005/diff/140001/services/ui/ws/window_... services/ui/ws/window_manager_state.h:248: bool ConvertPointToScreen(gfx::Point* point, const int64_t display_id); Style guide says out params should be last. Also, document return value.
https://codereview.chromium.org/2778943005/diff/140001/services/ui/ws/cursor_... File services/ui/ws/cursor_location_manager.h (right): https://codereview.chromium.org/2778943005/diff/140001/services/ui/ws/cursor_... services/ui/ws/cursor_location_manager.h:27: void OnMouseCursorLocationChanged(const gfx::Point& point); On 2017/05/25 22:10:19, sky wrote: > Please rename to point_in_dips to make that clear. Renamed to point_in_dip? https://codereview.chromium.org/2778943005/diff/140001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.h (right): https://codereview.chromium.org/2778943005/diff/140001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.h:62: void SetMousePointerDisplayLocation(const gfx::Point& display_location, On 2017/05/25 22:10:19, sky wrote: > InDips? mouse_pointer_location is in display-coord and pixels? https://codereview.chromium.org/2778943005/diff/140001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.h:278: int64_t mouse_pointer_display_id_ = 0; On 2017/05/25 22:10:19, sky wrote: > kInvalidDisplayId. Done. https://codereview.chromium.org/2778943005/diff/140001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.h:281: int64_t event_display_id_ = 0; On 2017/05/25 22:10:19, sky wrote: > kInvalidDisplayId Done. https://codereview.chromium.org/2778943005/diff/140001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher_delegate.h (right): https://codereview.chromium.org/2778943005/diff/140001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_delegate.h:77: // |location_in_display| should be in display coordinates and in pixels. On 2017/05/25 22:10:19, sky wrote: > 'should be' -> 'are in' Done. https://codereview.chromium.org/2778943005/diff/140001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_delegate.h:78: // |location_in_display| and |display_id| will be updated if the window we On 2017/05/25 22:10:19, sky wrote: > 'will be updated' -> 'are updated' Done. https://codereview.chromium.org/2778943005/diff/140001/services/ui/ws/window_... File services/ui/ws/window_manager_state.cc (right): https://codereview.chromium.org/2778943005/diff/140001/services/ui/ws/window_... services/ui/ws/window_manager_state.cc:553: if (display) { On 2017/05/25 22:10:19, sky wrote: > When would there not be a display? This is called by WindowManagerState::GetRootWindowContaining which is part of event-targeting. Since we are putting events in a queue if there's an event currently being dispatched, by the time this event is being processed its display might have already gone? In that case it would go to the if (!target_display_root) handling block. https://codereview.chromium.org/2778943005/diff/140001/services/ui/ws/window_... File services/ui/ws/window_manager_state.h (right): https://codereview.chromium.org/2778943005/diff/140001/services/ui/ws/window_... services/ui/ws/window_manager_state.h:248: bool ConvertPointToScreen(gfx::Point* point, const int64_t display_id); On 2017/05/25 22:10:19, sky wrote: > Style guide says out params should be last. Also, document return value. Done.
https://codereview.chromium.org/2778943005/diff/140001/services/ui/ws/window_... File services/ui/ws/window_manager_state.cc (right): https://codereview.chromium.org/2778943005/diff/140001/services/ui/ws/window_... services/ui/ws/window_manager_state.cc:553: if (display) { On 2017/05/25 22:59:23, riajiang wrote: > On 2017/05/25 22:10:19, sky wrote: > > When would there not be a display? > > This is called by WindowManagerState::GetRootWindowContaining which is part of > event-targeting. Since we are putting events in a queue if there's an event > currently being dispatched, by the time this event is being processed its > display might have already gone? In that case it would go to the if > (!target_display_root) handling block. If the display configuration changes in some way, then it's likely the events applied to the new configuration don't make sense. I think if the display configuration changes in anyway we should dump any existing events.
https://codereview.chromium.org/2778943005/diff/140001/services/ui/ws/window_... File services/ui/ws/window_manager_state.cc (right): https://codereview.chromium.org/2778943005/diff/140001/services/ui/ws/window_... services/ui/ws/window_manager_state.cc:553: if (display) { On 2017/05/25 23:55:24, sky wrote: > On 2017/05/25 22:59:23, riajiang wrote: > > On 2017/05/25 22:10:19, sky wrote: > > > When would there not be a display? > > > > This is called by WindowManagerState::GetRootWindowContaining which is part of > > event-targeting. Since we are putting events in a queue if there's an event > > currently being dispatched, by the time this event is being processed its > > display might have already gone? In that case it would go to the if > > (!target_display_root) handling block. > > If the display configuration changes in some way, then it's likely the events > applied to the new configuration don't make sense. I think if the display > configuration changes in anyway we should dump any existing events. I changed to return nullptr in GetRootWindowContaining if display config has changed, then no target window is found and OnEventTargetNotFound (https://cs.chromium.org/chromium/src/services/ui/ws/event_dispatcher_delegate...) would be called to handle this later.
LGTM
lgtm https://codereview.chromium.org/2778943005/diff/180001/services/ui/ws/window_... File services/ui/ws/window_manager_state.h (right): https://codereview.chromium.org/2778943005/diff/180001/services/ui/ws/window_... services/ui/ws/window_manager_state.h:248: // if |point| is successfully updated, false otherwise. Can you add a comment that |point| as the input should be in display-physical-pixel space, and the output is in screen-dip space?
https://codereview.chromium.org/2778943005/diff/180001/services/ui/ws/window_... File services/ui/ws/window_manager_state.h (right): https://codereview.chromium.org/2778943005/diff/180001/services/ui/ws/window_... services/ui/ws/window_manager_state.h:248: // if |point| is successfully updated, false otherwise. On 2017/05/26 18:04:49, sadrul wrote: > Can you add a comment that |point| as the input should be in > display-physical-pixel space, and the output is in screen-dip space? Done.
The CQ bit was checked by riajiang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2778943005/#ps200001 (title: "comment")
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
Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by riajiang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2778943005/#ps220001 (title: "const")
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
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) 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 riajiang@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": 220001, "attempt_start_ts": 1495833234312150,
"parent_rev": "8b73d36ca42ac01b44b8cda903978a0fe79fdc0e", "commit_rev":
"0fb00abce4d3e56ef50a1e6fc1eee947d97036f0"}
Message was sent while issue was closed.
Description was changed from
==========
Keep root_location to be in pixels and display coordinates in WS.
We convert event root_location to be in screen coordinates which
mixed pixels and dip in PlatformDisplayDefault::UpdateEventRootLocation().
Then in WindowManagerState::GetRootWindowContaining(), we expect
root_location to be in screen coordinates to find the display that
event is on and mixed pixels and dip again. As root_location should
always be in pixels in window server, changing it to skip the
conversion in PlatformDisplayDefault::UpdateEventRootLocation() so
root_location remains in pixels and display coordinates after that;
and in WindowManagerState::GetRootWindowContaining(), using display
id and display coordinates to get the root_location in screen
coordinates and find the display (not actually updating root_location
to screen coordinates). mouse_pointer_last_location is also in pixels
and display coordinates now as a result as it is the same as event
root_location.
TODO: make sure this works with drag-and-drop once mouse warp is
functional with mushroom.
BUG=701036
TEST= mus_ws_unittests
manual with two displays, one of which is hi-dpi
./out/oxygen/chrome --user-data-dir=/tmp/chrome --mus
--screen-config=1600x900,1600x900^300/i
==========
to
==========
Keep root_location to be in pixels and display coordinates in WS.
We convert event root_location to be in screen coordinates which
mixed pixels and dip in PlatformDisplayDefault::UpdateEventRootLocation().
Then in WindowManagerState::GetRootWindowContaining(), we expect
root_location to be in screen coordinates to find the display that
event is on and mixed pixels and dip again. As root_location should
always be in pixels in window server, changing it to skip the
conversion in PlatformDisplayDefault::UpdateEventRootLocation() so
root_location remains in pixels and display coordinates after that;
and in WindowManagerState::GetRootWindowContaining(), using display
id and display coordinates to get the root_location in screen
coordinates and find the display (not actually updating root_location
to screen coordinates). mouse_pointer_last_location is also in pixels
and display coordinates now as a result as it is the same as event
root_location.
TODO: make sure this works with drag-and-drop once mouse warp is
functional with mushroom.
BUG=701036
TEST= mus_ws_unittests
manual with two displays, one of which is hi-dpi
./out/oxygen/chrome --user-data-dir=/tmp/chrome --mus
--screen-config=1600x900,1600x900^300/i
Review-Url: https://codereview.chromium.org/2778943005
Cr-Commit-Position: refs/heads/master@{#475133}
Committed:
https://chromium.googlesource.com/chromium/src/+/0fb00abce4d3e56ef50a1e6fc1ee...
==========
Message was sent while issue was closed.
Committed patchset #11 (id:220001) as https://chromium.googlesource.com/chromium/src/+/0fb00abce4d3e56ef50a1e6fc1ee... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
