Implement a MoveCursorToScreenLocation for just the window manager.

Implement a MoveCursorToScreenLocation for just the window manager.

While we don't want to expose this to any mus app (which could be
hostile), we still need a way for window management tasks to move the
cursor to specific locations.

BUG=693340
Review-Url: https://codereview.chromium.org/2918553003
Cr-Commit-Position: refs/heads/master@{#476768}
Committed: https://chromium.googlesource.com/chromium/src/+/29e1c03f3e5d909b2bd2a784a78bf3580bed4ccd

[Elliot Glaysher, 2017-05-31 21:42:12]

The CQ bit was checked by erg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 21:43:10]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Elliot Glaysher, 2017-05-31 23:16:54]

Description was changed from

==========
Implement a MoveCursorToScreenLocation for just the window manager.

BUG=693340
==========

to

==========
Implement a MoveCursorToScreenLocation for just the window manager.

While we don't want to expose this to any mus app (which could be hostile), we
still need a way for window management tasks to move the cursor to specific
locations.

BUG=693340
==========

[Elliot Glaysher, 2017-05-31 23:18:33]

Description was changed from

==========
Implement a MoveCursorToScreenLocation for just the window manager.

While we don't want to expose this to any mus app (which could be hostile), we
still need a way for window management tasks to move the cursor to specific
locations.

BUG=693340
==========

to

==========
Implement a MoveCursorToScreenLocation for just the window manager.

While we don't want to expose this to any mus app (which could be
hostile), we still need a way for window management tasks to move the
cursor to specific locations.

BUG=693340
==========

[Elliot Glaysher, 2017-05-31 23:47:31]

erg@chromium.org changed reviewers:
+ riajiang@chromium.org

[Elliot Glaysher, 2017-05-31 23:47:32]

erg@chromium.org changed reviewers:
+ riajiang@chromium.org

[Elliot Glaysher, 2017-05-31 23:47:32]

Hi Ria,

I'm trying to add a new method which deals with changing the cursor location,
and it currently looks like this is recorded in pixels on the mus client side,
but that the mus server side is currently transitioning away from screen pixels
to display dips. I was wondering if you knew a quick way of addressing:

https://codereview.chromium.org/2918553003/diff/1/services/ui/ws/window_manag...
File services/ui/ws/window_manager_state.cc (right):

https://codereview.chromium.org/2918553003/diff/1/services/ui/ws/window_manag...
services/ui/ws/window_manager_state.cc:197:
event_dispatcher()->SetMousePointerDisplayLocation(screen_pixel_location, -1);
This here is obviously incorrect in the multi-display case (though it works in
the single display case in tests), but I don't see an easy way to go from this
screen pixels to display location. Is there some helper function I've missed?

[Elliot Glaysher, 2017-05-31 23:47:32]

Hi Ria,

I'm trying to add a new method which deals with changing the cursor location,
and it currently looks like this is recorded in pixels on the mus client side,
but that the mus server side is currently transitioning away from screen pixels
to display dips. I was wondering if you knew a quick way of addressing:

https://codereview.chromium.org/2918553003/diff/1/services/ui/ws/window_manag...
File services/ui/ws/window_manager_state.cc (right):

https://codereview.chromium.org/2918553003/diff/1/services/ui/ws/window_manag...
services/ui/ws/window_manager_state.cc:197:
event_dispatcher()->SetMousePointerDisplayLocation(screen_pixel_location, -1);
This here is obviously incorrect in the multi-display case (though it works in
the single display case in tests), but I don't see an easy way to go from this
screen pixels to display location. Is there some helper function I've missed?

[commit-bot: I haz the power, 2017-06-01 00:14:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-01 00:14:45]

Dry run: This issue passed the CQ dry run.

[riajiang, 2017-06-01 17:06:07]

https://codereview.chromium.org/2918553003/diff/1/services/ui/ws/window_manag...
File services/ui/ws/window_manager_state.cc (right):

https://codereview.chromium.org/2918553003/diff/1/services/ui/ws/window_manag...
services/ui/ws/window_manager_state.cc:197:
event_dispatcher()->SetMousePointerDisplayLocation(screen_pixel_location, -1);
On 2017/05/31 23:47:32, Elliot Glaysher wrote:
> This here is obviously incorrect in the multi-display case (though it works in
> the single display case in tests), but I don't see an easy way to go from this
> screen pixels to display location. Is there some helper function I've missed?

In WindowTreeHost::MoveCursorToInternal, can we convert |host_location|
(screen-location-in-pixels?) to display-location-in-pixels by subtracting
GetLocationOnScreenInPixels() and then also pass display_id from there to
WindowTreeHostMus::MoveCursorToScreenLocationInPixels?

[Elliot Glaysher, 2017-06-01 19:31:53]

The CQ bit was checked by erg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-01 19:32:35]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Elliot Glaysher, 2017-06-01 21:01:50]

erg@chromium.org changed reviewers:
+ sky@chromium.org

[Elliot Glaysher, 2017-06-01 21:01:50]

ty, ria, that helped.

sky: ptal

[Elliot Glaysher, 2017-06-01 21:02:58]

The CQ bit was checked by erg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-01 21:03:10]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[riajiang, 2017-06-01 21:27:30]

https://codereview.chromium.org/2918553003/diff/40001/ui/aura/mus/window_tree...
File ui/aura/mus/window_tree_host_mus.cc (right):

https://codereview.chromium.org/2918553003/diff/40001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_host_mus.cc:204:
ConvertScreenInPixelsToDIP(&dip_location);
EventDispatcher::SetMousePointerDisplayLocation expects location to be in
display-coordinate-pixels, maybe skip the dip conversion in
ConvertScreenInPixelsToDIP?

[Elliot Glaysher, 2017-06-01 21:52:36]

The CQ bit was checked by erg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-01 21:52:49]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Elliot Glaysher, 2017-06-01 22:05:23]

https://codereview.chromium.org/2918553003/diff/40001/ui/aura/mus/window_tree...
File ui/aura/mus/window_tree_host_mus.cc (right):

https://codereview.chromium.org/2918553003/diff/40001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_host_mus.cc:204:
ConvertScreenInPixelsToDIP(&dip_location);
On 2017/06/01 21:27:30, riajiang wrote:
> EventDispatcher::SetMousePointerDisplayLocation expects location to be in
> display-coordinate-pixels, maybe skip the dip conversion in
> ConvertScreenInPixelsToDIP?

Done, though I remain confused about when and where we are using dips vs pixels.
(For example, CursorLocationManager internally says that its using dips, but the
consumer of CursorLocationManger is Env::last_mouse_location() which doesn't
specify the coordinate space; neither does its consumer in
ScreenMus::GetCursorScreenPoint().)

[sky, 2017-06-01 22:13:20]

https://codereview.chromium.org/2918553003/diff/60001/services/ui/ws/window_m...
File services/ui/ws/window_manager_state.cc (right):

https://codereview.chromium.org/2918553003/diff/60001/services/ui/ws/window_m...
services/ui/ws/window_manager_state.cc:198: UpdateNativeCursorFromDispatcher();
Don't you also need to call through to the platform_window_?
PlatformDisplayDefault has a PlatformWindow which has the function
MoveCursorTo().

https://codereview.chromium.org/2918553003/diff/60001/ui/aura/mus/window_tree...
File ui/aura/mus/window_tree_host_mus.cc (right):

https://codereview.chromium.org/2918553003/diff/60001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_host_mus.cc:204: gfx::Point location =
GetLocationOnScreenInPixels();
I'm confused, all the calculations here are in pixels, yet the rest of the
functions say dips? It seems to me that if you are specifying a location
relative to a display, it can (and should be) in pixels, otherwise you can't
position at pixel locations.

[riajiang, 2017-06-01 22:20:18]

https://codereview.chromium.org/2918553003/diff/40001/ui/aura/mus/window_tree...
File ui/aura/mus/window_tree_host_mus.cc (right):

https://codereview.chromium.org/2918553003/diff/40001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_host_mus.cc:204:
ConvertScreenInPixelsToDIP(&dip_location);
On 2017/06/01 22:05:23, Elliot Glaysher wrote:
> On 2017/06/01 21:27:30, riajiang wrote:
> > EventDispatcher::SetMousePointerDisplayLocation expects location to be in
> > display-coordinate-pixels, maybe skip the dip conversion in
> > ConvertScreenInPixelsToDIP?
> 
> Done, though I remain confused about when and where we are using dips vs
pixels.
> (For example, CursorLocationManager internally says that its using dips, but
the
> consumer of CursorLocationManger is Env::last_mouse_location() which doesn't
> specify the coordinate space; neither does its consumer in
> ScreenMus::GetCursorScreenPoint().)

Event root_location has changed to be in pixels in WS. Since 
EventDispatcher::mouse_pointer_last_location_ gets its value
from event root_location it is now also display-coord-pixels. 
CursorLocationManager is an exception where location would be
converted back to screen-coord-dip in 
WindowManagerState::OnMouseCursorLocationChanged before sending
to CursorLocationManager because I think Env::last_mouse_location()
expects mouse location to be in screen-coord-dip.

[Elliot Glaysher, 2017-06-01 22:51:43]

The CQ bit was checked by erg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-01 22:52:15]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Elliot Glaysher, 2017-06-01 23:05:42]

https://codereview.chromium.org/2918553003/diff/60001/services/ui/ws/window_m...
File services/ui/ws/window_manager_state.cc (right):

https://codereview.chromium.org/2918553003/diff/60001/services/ui/ws/window_m...
services/ui/ws/window_manager_state.cc:198: UpdateNativeCursorFromDispatcher();
On 2017/06/01 22:13:20, sky wrote:
> Don't you also need to call through to the platform_window_?
> PlatformDisplayDefault has a PlatformWindow which has the function
> MoveCursorTo().

Done.

https://codereview.chromium.org/2918553003/diff/60001/ui/aura/mus/window_tree...
File ui/aura/mus/window_tree_host_mus.cc (right):

https://codereview.chromium.org/2918553003/diff/60001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_host_mus.cc:204: gfx::Point location =
GetLocationOnScreenInPixels();
On 2017/06/01 22:13:20, sky wrote:
> I'm confused, all the calculations here are in pixels, yet the rest of the
> functions say dips? It seems to me that if you are specifying a location
> relative to a display, it can (and should be) in pixels, otherwise you can't
> position at pixel locations.

This was me not changing variable names after Ria's comments above. I've gone
through and changed the variable names across the patch.

[commit-bot: I haz the power, 2017-06-02 01:52:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-02 01:52:14]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[sky, 2017-06-02 13:10:07]

LGTM

https://codereview.chromium.org/2918553003/diff/100001/services/ui/ws/window_...
File services/ui/ws/window_tree_unittest.cc (right):

https://codereview.chromium.org/2918553003/diff/100001/services/ui/ws/window_...
services/ui/ws/window_tree_unittest.cc:1528: // Have the window manager mvoe the
cursor within the embed window.
mvoe -> move

[riajiang, 2017-06-02 15:09:44]

lgtm

[Elliot Glaysher, 2017-06-02 17:18:45]

erg@chromium.org changed reviewers:
+ tsepez@chromium.org

[Elliot Glaysher, 2017-06-02 17:18:45]

+tsepez: mojom review. interface is just from window manager to window server;
this interface shouldn't be exposed to general clients.

[Tom Sepez, 2017-06-02 18:02:32]

Ok, so how do we enforce that this isn't available to the other apps?

[Tom Sepez, 2017-06-02 18:10:49]

LGTM otherwise

[Elliot Glaysher, 2017-06-02 18:15:45]

On 2017/06/02 18:02:32, Tom Sepez (ooo after 6-2) wrote:
> Ok, so how do we enforce that this isn't available to the other apps?

I'm sorry, I should have said I added a method to the window manager interface.
(Which is already checked and we don't vend WindowManagerClient to anyone but
the WindowManagerClient of the WindowTree which hosts the window manager.)

[Elliot Glaysher, 2017-06-02 18:16:17]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2017-06-02 18:16:39]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-02 20:01:01]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1496427377716420,
"parent_rev": "3c6caa287dacd94ee5e64eedaf44ea987f287bf8", "commit_rev":
"9e110cb8b9400a05ebaecc32b10d8d82da6f2832"}

[commit-bot: I haz the power, 2017-06-02 20:01:17]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1496427377716420,
"parent_rev": "bb671c7cb427aadc0e2e946ff426775610c25f98", "commit_rev":
"29e1c03f3e5d909b2bd2a784a78bf3580bed4ccd"}

[commit-bot: I haz the power, 2017-06-02 20:01:26]

Description was changed from

==========
Implement a MoveCursorToScreenLocation for just the window manager.

While we don't want to expose this to any mus app (which could be
hostile), we still need a way for window management tasks to move the
cursor to specific locations.

BUG=693340
==========

to

==========
Implement a MoveCursorToScreenLocation for just the window manager.

While we don't want to expose this to any mus app (which could be
hostile), we still need a way for window management tasks to move the
cursor to specific locations.

BUG=693340

Review-Url: https://codereview.chromium.org/2918553003
Cr-Commit-Position: refs/heads/master@{#476768}
Committed:
https://chromium.googlesource.com/chromium/src/+/29e1c03f3e5d909b2bd2a784a78b...
==========

[commit-bot: I haz the power, 2017-06-02 20:01:28]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/29e1c03f3e5d909b2bd2a784a78b...