mus: Implement ScreenMus::GetCursorScreenPoint().

mus: Implement ScreenMus::GetCursorScreenPoint().

ScreenMus (and by extension, much of our views code) assumes that it can
know the position of the cursor on screen. If we broadcast the
position to every client on mouse move, we'll wake up every client on
every mouse move. If we add a sync ipc call from the client to the window
server, what's a fairly common operation now performs a sync ipc.

Instead, have the window server own a piece of shared memory, share a
read only handle to it with anyone who asks, and then write the cursor
position to it on each mouse event. This lets clients read the cursor
position without costly broadcasting or sync ipcs.

BUG=602727
Committed: https://crrev.com/78af65676b7db5aa44335792d6336a6c58847d5d
Cr-Commit-Position: refs/heads/master@{#390539}

[Elliot Glaysher, 2016-04-13 21:50:55]

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

[Elliot Glaysher, 2016-04-13 22:01:56]

(views_mus_unittests is also failing locally on ToT.)

[sky, 2016-04-13 22:58:33]

It seems unfortunate that we would broadcast to every app the location of the
mouse on every move. This effectively wakes up every app and could easily hurt
performance.

Secondarily sending events through the DisplayManagerObserver pipe means the
event ordering between mouse moves and events targeting the client is
non-deterministic.

I'm inclined to go with a sync call. We are already assuming mus is not slow, so
that adding a sync call for the cursor lookup shouldn't be that bad. I'm saying
this assuming we don't call out for the mouse location that often, which I
believe is generally true.

[Elliot Glaysher, 2016-04-13 23:18:44]

On 2016/04/13 22:58:33, sky wrote:
> It seems unfortunate that we would broadcast to every app the location of the
> mouse on every move. This effectively wakes up every app and could easily hurt
> performance.
> 
> Secondarily sending events through the DisplayManagerObserver pipe means the
> event ordering between mouse moves and events targeting the client is
> non-deterministic.
> 
> I'm inclined to go with a sync call. We are already assuming mus is not slow,
so
> that adding a sync call for the cursor lookup shouldn't be that bad. I'm
saying
> this assuming we don't call out for the mouse location that often, which I
> believe is generally true.

In RenderWidgetHostViewAura, we call GetScreenCursorScreenPoint() on every mouse
move event (and on certain renderer messages) through a convoluted path
involving setting the cursor. We get the screen point to ensure that the cursor
is currently over the window so that we don't set the cursor when it's not over
our window (which is apparently a thing that can happen on MS Windows).

[Elliot Glaysher, 2016-04-15 19:11:17]

Moved to using sync for getting the cursor position.

(I did some about:tracing and saw that this call is only half a millisecond, so
you're right that this isn't a big deal and is probably better than broadcasting
to everyone.)

[sky, 2016-04-15 20:38:57]

https://codereview.chromium.org/1881253002/diff/140001/components/mus/public/...
File components/mus/public/interfaces/display.mojom (right):

https://codereview.chromium.org/1881253002/diff/140001/components/mus/public/...
components/mus/public/interfaces/display.mojom:29: GetCursorScreenPoint() =>
(mojo.Point location);
If you keep this on DisplayManager then there is events and cursor screen points
are on separate pipes and can be out of order. I recommend putting this on
WindowTreeHost to address that. Putting it on WindowTreeHost will simplify the
implementation too.

[Elliot Glaysher, 2016-04-15 23:56:52]

https://codereview.chromium.org/1881253002/diff/140001/components/mus/public/...
File components/mus/public/interfaces/display.mojom (right):

https://codereview.chromium.org/1881253002/diff/140001/components/mus/public/...
components/mus/public/interfaces/display.mojom:29: GetCursorScreenPoint() =>
(mojo.Point location);
On 2016/04/15 20:38:57, sky wrote:
> If you keep this on DisplayManager then there is events and cursor screen
points
> are on separate pipes and can be out of order. I recommend putting this on
> WindowTreeHost to address that. Putting it on WindowTreeHost will simplify the
> implementation too.

Done.

The call is now on WindowTree. I left the storage of the point in the
UserDisplayManager after poking at other places for it (notifying all the
WindowTreeHosts, putting it in the WindowServer, etc).

[sky, 2016-04-18 15:09:12]

https://codereview.chromium.org/1881253002/diff/200001/components/mus/public/...
File components/mus/public/cpp/lib/window_tree_client_impl.cc (right):

https://codereview.chromium.org/1881253002/diff/200001/components/mus/public/...
components/mus/public/cpp/lib/window_tree_client_impl.cc:819:
base::ThreadRestrictions::ScopedAllowWait allow_wait;
Add a TODO that this code should use the location from the current event if
we're in the process of dispatching one. We can't do that until James lands
https://codereview.chromium.org/1883263002/ .

https://codereview.chromium.org/1881253002/diff/200001/components/mus/ws/even...
File components/mus/ws/event_dispatcher.cc (right):

https://codereview.chromium.org/1881253002/diff/200001/components/mus/ws/even...
components/mus/ws/event_dispatcher.cc:111:
delegate_->OnMouseCursorLocationChanged(screen_location);
Why do you need to notify the delegate? Can't we query for the location from
EventDispatcher when needed?

[Elliot Glaysher, 2016-04-18 20:21:37]

ptal

I feel a bit iffy about committing this now since I can't test it on ToT; chrome
crashes immediately on startup. :/

https://codereview.chromium.org/1881253002/diff/240001/components/mus/ws/wind...
File components/mus/ws/window_tree.cc (right):

https://codereview.chromium.org/1881253002/diff/240001/components/mus/ws/wind...
components/mus/ws/window_tree.cc:675: if (roots_.empty()) {
Is this empty roots thing a case we reasonably have to care about?

[sky, 2016-04-18 20:53:05]

https://codereview.chromium.org/1881253002/diff/240001/components/mus/ws/wind...
File components/mus/ws/window_tree.cc (right):

https://codereview.chromium.org/1881253002/diff/240001/components/mus/ws/wind...
components/mus/ws/window_tree.cc:675: if (roots_.empty()) {
On 2016/04/18 20:21:37, Elliot Glaysher wrote:
> Is this empty roots thing a case we reasonably have to care about?

Yes. It should be possible to query for the cursor location when a root hasn't
been created. I recommend routing the call to the windowserver, supply the id of
the user id and the WindowServer can then route to the appropriate
WindowManagerState. You'll want the WindowServer function to take a user id. If
the user id isn't active, return 0x0.

[Elliot Glaysher, 2016-04-20 22:29:38]

Rebuilds the patch so that it passes data using a piece of shared memory,
accessed atomically.

This is the first time I've written atomic read/write code, so please review
this carefully.

[sky, 2016-04-20 23:46:36]

I'm not an expert with the atomics either, can you find someone knowledge of
atomics to get them to review this?

https://codereview.chromium.org/1881253002/diff/380001/components/mus/public/...
File components/mus/public/cpp/window_tree_connection.h (right):

https://codereview.chromium.org/1881253002/diff/380001/components/mus/public/...
components/mus/public/cpp/window_tree_connection.h:89: // race the asynchronous
initialization; but in that case we just will return
nit: remove 'just will'

https://codereview.chromium.org/1881253002/diff/380001/components/mus/public/...
File components/mus/public/interfaces/window_tree.mojom (right):

https://codereview.chromium.org/1881253002/diff/380001/components/mus/public/...
components/mus/public/interfaces/window_tree.mojom:255: // Returns a shared
memory segment that contains two 32-bit ints packed into a
nit: document order of coordinates.

https://codereview.chromium.org/1881253002/diff/380001/components/mus/ws/even...
File components/mus/ws/event_dispatcher.cc (right):

https://codereview.chromium.org/1881253002/diff/380001/components/mus/ws/even...
components/mus/ws/event_dispatcher.cc:244:
delegate_->OnMouseCursorLocationChanged(event.root_location());
nit: move this after 245 so the mouse_pointer_last_location_ has been set
appropriately (like you do on 111).

https://codereview.chromium.org/1881253002/diff/380001/components/mus/ws/wind...
File components/mus/ws/window_manager_state.cc (right):

https://codereview.chromium.org/1881253002/diff/380001/components/mus/ws/wind...
components/mus/ws/window_manager_state.cc:192: MojoResult result =
mojo::DuplicateBuffer(cursor_location_handle_.get(),
MOJO_DUPLICATE_BUFFER_HANDLE_OPTIONS_FLAG_READ_ONLY ?

https://codereview.chromium.org/1881253002/diff/380001/components/mus/ws/wind...
File components/mus/ws/window_tree.cc (right):

https://codereview.chromium.org/1881253002/diff/380001/components/mus/ws/wind...
components/mus/ws/window_tree.cc:673: // TODO(sky): need a way for client to
provide context to figure out display.
This TODO isn't applicable here.

https://codereview.chromium.org/1881253002/diff/380001/components/mus/ws/wind...
components/mus/ws/window_tree.cc:677: // TODO(sky): move checks to accesspolicy.
This TODO should be in NewTopLevelWindow, not here.

[Elliot Glaysher, 2016-04-21 17:50:43]

Description was changed from

==========
mus: Implement a ScreenLocation service for ScreenMus.

ScreenMus (and by extension, much of our views code) assumes that it can
know the position of the cursor on screen. This threads mouse movements
from the current user's event dispatcher to all ScreenMus objects.

BUG=602727
==========

to

==========
mus: Implement ScreenMus::GetCursorScreenPoint().

ScreenMus (and by extension, much of our views code) assumes that it can
know the position of the cursor on screen. If we broadcast the
position to every client on mouse move, we'll wake up every client on
every mouse move. If we add a sync ipc call from the client to the window
server, what's a fairly common operation now performs a sync ipc.

Instead, have the window server own a piece of shared memory, share a
read only handle to it with anyone who asks, and then write the cursor
position to it on each mouse event. This lets clients read the cursor
position without costly broadcasting or sync ipcs.

BUG=602727
==========

[Elliot Glaysher, 2016-04-21 17:50:43]

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

[Elliot Glaysher, 2016-04-21 18:08:00]

+thakis

Hi Nico, it looks like you have experience with atomic operations. I've never
written a patch that uses them before and would like someone who knows what
they're doing to look over them.

The file that writes the data atomically is window_manager_state.cc and the file
that reads them is window_tree_client_impl.cc.

[Nico, 2016-04-21 18:21:35]

can't you use a lock instead?

[Elliot Glaysher, 2016-04-21 18:23:45]

On 2016/04/21 18:21:35, Nico wrote:
> can't you use a lock instead?

Do we have a multi-process lock class?

[sky, 2016-04-21 20:19:49]

multi-process lock seems more expensive than using atomics.

  -Scott

On Thu, Apr 21, 2016 at 11:23 AM,  <erg@chromium.org> wrote:
> On 2016/04/21 18:21:35, Nico wrote:
>> can't you use a lock instead?
>
> Do we have a multi-process lock class?
>
> https://codereview.chromium.org/1881253002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2016-04-21 20:25:54]

Sure, but in return you have at least a chance of understanding them. In
general, chrome does locks and message passing instead of shared memory,
for this reason, right?

On Thu, Apr 21, 2016 at 4:19 PM, Scott Violet <sky@chromium.org> wrote:

> multi-process lock seems more expensive than using atomics.
>
>   -Scott
>
> On Thu, Apr 21, 2016 at 11:23 AM,  <erg@chromium.org> wrote:
> > On 2016/04/21 18:21:35, Nico wrote:
> >> can't you use a lock instead?
> >
> > Do we have a multi-process lock class?
> >
> > https://codereview.chromium.org/1881253002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2016-04-21 20:28:41]

Shared memory also has its uses in chrome as well. We're trying to
avoid sending tons of messages on every mouse move when mouse apps
don't care. Using shared memory seems like a better solution for this
use case.

  -Scott

On Thu, Apr 21, 2016 at 1:25 PM, Nico Weber <thakis@chromium.org> wrote:
> Sure, but in return you have at least a chance of understanding them. In
> general, chrome does locks and message passing instead of shared memory, for
> this reason, right?
>
> On Thu, Apr 21, 2016 at 4:19 PM, Scott Violet <sky@chromium.org> wrote:
>>
>> multi-process lock seems more expensive than using atomics.
>>
>>   -Scott
>>
>> On Thu, Apr 21, 2016 at 11:23 AM,  <erg@chromium.org> wrote:
>> > On 2016/04/21 18:21:35, Nico wrote:
>> >> can't you use a lock instead?
>> >
>> > Do we have a multi-process lock class?
>> >
>> > https://codereview.chromium.org/1881253002/
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Elliot Glaysher, 2016-04-27 17:48:48]

Looking at locks, we'd need write access in the reader to acquire the lock and
we probably don't want that. (Also, we don't want to be acquiring locks on the
UI thread.)

Switched to 32-bit integers.

[sky, 2016-04-27 20:10:28]

https://codereview.chromium.org/1881253002/diff/460001/components/mus/public/...
File components/mus/public/cpp/lib/window_tree_client_impl.cc (right):

https://codereview.chromium.org/1881253002/diff/460001/components/mus/public/...
components/mus/public/cpp/lib/window_tree_client_impl.cc:573:
called_initialize_cursor_location_ = true;
DCHECK(!called...?

https://codereview.chromium.org/1881253002/diff/460001/components/mus/public/...
components/mus/public/cpp/lib/window_tree_client_impl.cc:588: return
gfx::Point(location >> 16, location & 0xFFFF);
I believe the way you have this code means we can't have negative coordinates. I
suspect we want them. How about supporting the sign bit?

https://codereview.chromium.org/1881253002/diff/460001/components/mus/public/...
File components/mus/public/cpp/lib/window_tree_client_impl.h (right):

https://codereview.chromium.org/1881253002/diff/460001/components/mus/public/...
components/mus/public/cpp/lib/window_tree_client_impl.h:302: bool
called_initialize_cursor_location_;
Add comment.

https://codereview.chromium.org/1881253002/diff/460001/components/mus/public/...
components/mus/public/cpp/lib/window_tree_client_impl.h:304: // A handle to
shared memory that is one 65 bit integer long. The window
32?

https://codereview.chromium.org/1881253002/diff/460001/components/mus/public/...
File components/mus/public/cpp/window_tree_connection.h (right):

https://codereview.chromium.org/1881253002/diff/460001/components/mus/public/...
components/mus/public/cpp/window_tree_connection.h:86: virtual void
InitializeCursorLocation() = 0;
It seems like what you have implemented is pretty light weight, and we should
just do it all the time. WDYT?

https://codereview.chromium.org/1881253002/diff/460001/components/mus/ws/even...
File components/mus/ws/event_dispatcher.cc (right):

https://codereview.chromium.org/1881253002/diff/460001/components/mus/ws/even...
components/mus/ws/event_dispatcher.cc:111:
delegate_->OnMouseCursorLocationChanged(screen_location);
I worry that the way you have things results in updating the client before
sending events. How about adding a comment to this effect that we're hoping this
won't cause problems.

https://codereview.chromium.org/1881253002/diff/460001/components/mus/ws/wind...
File components/mus/ws/window_manager_state.cc (right):

https://codereview.chromium.org/1881253002/diff/460001/components/mus/ws/wind...
components/mus/ws/window_manager_state.cc:168: mojo::ScopedSharedBufferHandle
WindowManagerState::GetCursorLocationMemory() {
If you put this code here, it means we're only get the coordinates of one
display. I think the right place for this is UserDisplayManager, which you can
get by way of:

window_server()->display_manager()->GetUserDisplayManager(user_id_);

[Elliot Glaysher, 2016-04-27 22:40:45]

ptal

- The memory is now owned by UserDisplayManager.
- We now always initialize on the client side.
- I made the changes necessary to allow negative numbers.

[sky, 2016-04-27 23:55:57]

https://codereview.chromium.org/1881253002/diff/520001/components/mus/public/...
File components/mus/public/cpp/lib/window_tree_client_impl.cc (right):

https://codereview.chromium.org/1881253002/diff/520001/components/mus/public/...
components/mus/public/cpp/lib/window_tree_client_impl.cc:582: return
gfx::Point(location >> 16,
Is the sign bit preserved here? I would expect you need the static_cast as you
have on the next line, but I readily admit I'm not sure.

https://codereview.chromium.org/1881253002/diff/520001/components/mus/ws/user...
File components/mus/ws/user_display_manager.cc (right):

https://codereview.chromium.org/1881253002/diff/520001/components/mus/ws/user...
components/mus/ws/user_display_manager.cc:71: (point.x() & 0xFFFF) << 16 |
(point.y() & 0xFFFF));
Seems like this loses the sign bit, but again, I could be wrong. How about a
test with a negative x/y?

[Elliot Glaysher, 2016-04-28 00:14:01]

https://codereview.chromium.org/1881253002/diff/520001/components/mus/public/...
File components/mus/public/cpp/lib/window_tree_client_impl.cc (right):

https://codereview.chromium.org/1881253002/diff/520001/components/mus/public/...
components/mus/public/cpp/lib/window_tree_client_impl.cc:582: return
gfx::Point(location >> 16,
On 2016/04/27 23:55:57, sky wrote:
> Is the sign bit preserved here? I would expect you need the static_cast as you
> have on the next line, but I readily admit I'm not sure.

I manually tested this and the sign bit is preserved. Added cast anyway for
clarity.

https://codereview.chromium.org/1881253002/diff/520001/components/mus/ws/user...
File components/mus/ws/user_display_manager.cc (right):

https://codereview.chromium.org/1881253002/diff/520001/components/mus/ws/user...
components/mus/ws/user_display_manager.cc:71: (point.x() & 0xFFFF) << 16 |
(point.y() & 0xFFFF));
On 2016/04/27 23:55:57, sky wrote:
> Seems like this loses the sign bit, but again, I could be wrong. How about a
> test with a negative x/y?

Added unit test to user_display_manager_unittest.cc, which exercises this code.
(I'm not sure how to test the WindowTreeClientImpl code.)

[sky, 2016-04-28 03:36:00]

LGTM

[Elliot Glaysher, 2016-04-28 17:18:26]

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

[Elliot Glaysher, 2016-04-28 17:18:27]

Hi Antoine,

After talking about locking with Scott, a hostile process being able to acquire
a read lock and never release it would stop message dispatch, so we probably
don't want to use locks.

I still want someone who knows about atomics to review the atomics use that I
added. components/mus/public/cpp/lib/window_tree_client_impl.cc is where we read
data and components/mus/ws/user_display_manager.cc is where we write it.

[piman, 2016-04-28 20:56:42]

lgtm

[Elliot Glaysher, 2016-04-28 21:00:56]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2016-04-28 21:01:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1881253002/540001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1881253002/540001

[commit-bot: I haz the power, 2016-04-28 21:15:17]

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

[commit-bot: I haz the power, 2016-04-28 21:15:24]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Elliot Glaysher, 2016-04-28 21:55:05]

The CQ bit was checked by erg@chromium.org

[Elliot Glaysher, 2016-04-28 21:55:06]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1881253002/#ps560001
(title: "Merge with tot")

[commit-bot: I haz the power, 2016-04-28 21:55:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1881253002/560001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1881253002/560001

[commit-bot: I haz the power, 2016-04-28 23:56:58]

Description was changed from

==========
mus: Implement ScreenMus::GetCursorScreenPoint().

ScreenMus (and by extension, much of our views code) assumes that it can
know the position of the cursor on screen. If we broadcast the
position to every client on mouse move, we'll wake up every client on
every mouse move. If we add a sync ipc call from the client to the window
server, what's a fairly common operation now performs a sync ipc.

Instead, have the window server own a piece of shared memory, share a
read only handle to it with anyone who asks, and then write the cursor
position to it on each mouse event. This lets clients read the cursor
position without costly broadcasting or sync ipcs.

BUG=602727
==========

to

==========
mus: Implement ScreenMus::GetCursorScreenPoint().

ScreenMus (and by extension, much of our views code) assumes that it can
know the position of the cursor on screen. If we broadcast the
position to every client on mouse move, we'll wake up every client on
every mouse move. If we add a sync ipc call from the client to the window
server, what's a fairly common operation now performs a sync ipc.

Instead, have the window server own a piece of shared memory, share a
read only handle to it with anyone who asks, and then write the cursor
position to it on each mouse event. This lets clients read the cursor
position without costly broadcasting or sync ipcs.

BUG=602727
==========

[commit-bot: I haz the power, 2016-04-28 23:57:00]

Committed patchset #29 (id:560001)

[commit-bot: I haz the power, 2016-04-30 17:22:51]

Patchset 29 (id:??) landed as
https://crrev.com/78af65676b7db5aa44335792d6336a6c58847d5d
Cr-Commit-Position: refs/heads/master@{#390539}

[commit-bot: I haz the power, 2016-04-30 17:58:28]

Description was changed from

==========
mus: Implement ScreenMus::GetCursorScreenPoint().

ScreenMus (and by extension, much of our views code) assumes that it can
know the position of the cursor on screen. If we broadcast the
position to every client on mouse move, we'll wake up every client on
every mouse move. If we add a sync ipc call from the client to the window
server, what's a fairly common operation now performs a sync ipc.

Instead, have the window server own a piece of shared memory, share a
read only handle to it with anyone who asks, and then write the cursor
position to it on each mouse event. This lets clients read the cursor
position without costly broadcasting or sync ipcs.

BUG=602727
==========

to

==========
mus: Implement ScreenMus::GetCursorScreenPoint().

ScreenMus (and by extension, much of our views code) assumes that it can
know the position of the cursor on screen. If we broadcast the
position to every client on mouse move, we'll wake up every client on
every mouse move. If we add a sync ipc call from the client to the window
server, what's a fairly common operation now performs a sync ipc.

Instead, have the window server own a piece of shared memory, share a
read only handle to it with anyone who asks, and then write the cursor
position to it on each mouse event. This lets clients read the cursor
position without costly broadcasting or sync ipcs.

BUG=602727

Committed: https://crrev.com/78af65676b7db5aa44335792d6336a6c58847d5d
Cr-Commit-Position: refs/heads/master@{#390539}
==========