mus: Let clients set the cursor of their window.

mus: Let clients set the cursor of their window.

This adds a cursor property for each window. mus changes the cursor as the pointer passes between windows.

BUG=548428
Committed: https://crrev.com/314e9981a45aa20338d87c1952977603b051c725
Cr-Commit-Position: refs/heads/master@{#363261}

[Elliot Glaysher, 2015-12-01 19:28:32]

Description was changed from

==========
mus: Let clients set the cursor of their window.

[xxx not for commit yet.]

BUG=548428
==========

to

==========
mus: Let clients set the cursor of their window.

This adds a cursor property for each window. mus changes the cursor as the
pointer passes between windows.

BUG=548428
==========

[Elliot Glaysher, 2015-12-01 19:28:32]

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

[Elliot Glaysher, 2015-12-01 19:29:14]

sky: there are a whole bunch of other properties (visibility, image cursors)
that also need to be added, but I wanted to make sure this was done correctly
first.

[sky, 2015-12-01 21:48:01]

https://codereview.chromium.org/1465803003/diff/140001/components/mus/public/...
File components/mus/public/cpp/lib/window.cc (right):

https://codereview.chromium.org/1465803003/diff/140001/components/mus/public/...
components/mus/public/cpp/lib/window.cc:367:
tree_client()->SetStandardCursor(id_, cursor_id);
In general we tend to assume the change is going to take, which means this
should notify observers immediately.

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

https://codereview.chromium.org/1465803003/diff/140001/components/mus/public/...
components/mus/public/interfaces/window_tree.mojom:210: SetStandardCursor(uint32
window_id, Cursor cursor_id);
Make this take a change_id, like SetWindowProperty does. See how
SetWindowProperty deals with this for how to handle it.

I was a bit thrown by the name, did you consider 'predefined', as that is what
it is?

https://codereview.chromium.org/1465803003/diff/140001/components/mus/ws/acce...
File components/mus/ws/access_policy.h (right):

https://codereview.chromium.org/1465803003/diff/140001/components/mus/ws/acce...
components/mus/ws/access_policy.h:53: virtual bool CanSetCursorProperties(const
ServerWindow* window) const = 0;
Document this is used for setting any cursor property.

https://codereview.chromium.org/1465803003/diff/140001/components/mus/ws/conn...
File components/mus/ws/connection_manager.cc (right):

https://codereview.chromium.org/1465803003/diff/140001/components/mus/ws/conn...
components/mus/ws/connection_manager.cc:481: // Pass the cursor change to our
native window.
out->the (as there may be more than one native window).

https://codereview.chromium.org/1465803003/diff/140001/components/mus/ws/even...
File components/mus/ws/event_dispatcher.h (right):

https://codereview.chromium.org/1465803003/diff/140001/components/mus/ws/even...
components/mus/ws/event_dispatcher.h:36: // Retrieves the ServerWindow of the
last mouse move.
Your code is setting for any pointer event, not just mouse. If you only want
mouse then you need to update the code.

https://codereview.chromium.org/1465803003/diff/140001/components/mus/ws/serv...
File components/mus/ws/server_window.h (right):

https://codereview.chromium.org/1465803003/diff/140001/components/mus/ws/serv...
components/mus/ws/server_window.h:116: void SetCursor(mus::mojom::Cursor
cursor_id);
Name this consistently with the mojom, so either PredefinedCursor (if you go
with that) or StandardCursor.

https://codereview.chromium.org/1465803003/diff/140001/components/mus/ws/serv...
File components/mus/ws/server_window_observer.h (right):

https://codereview.chromium.org/1465803003/diff/140001/components/mus/ws/serv...
components/mus/ws/server_window_observer.h:59: virtual void
OnWindowCursorChanged(ServerWindow* window, int32_t cursor_id) {}
Is there a reason you're not using standard here?

https://codereview.chromium.org/1465803003/diff/140001/components/mus/ws/wind...
File components/mus/ws/window_tree_host_impl.cc (right):

https://codereview.chromium.org/1465803003/diff/140001/components/mus/ws/wind...
components/mus/ws/window_tree_host_impl.cc:137: if (window ==
event_dispatcher_.last_targeted_window())
Doesn't this need to be more sophisticated? What if the last event target is no
longer under the mouse?

https://codereview.chromium.org/1465803003/diff/140001/components/mus/ws/wind...
File components/mus/ws/window_tree_impl.cc (right):

https://codereview.chromium.org/1465803003/diff/140001/components/mus/ws/wind...
components/mus/ws/window_tree_impl.cc:875: if (success)
You need to create an Operation here. See 856 for what I mean.

https://codereview.chromium.org/1465803003/diff/140001/ui/views/mus/platform_...
File ui/views/mus/platform_window_mus.cc (right):

https://codereview.chromium.org/1465803003/diff/140001/ui/views/mus/platform_...
ui/views/mus/platform_window_mus.cc:164: if (window == mus_window_)
DCHECK?

[Elliot Glaysher, 2015-12-02 20:01:32]

ptal

Changed to PredefinedCursor, and added a Cursor Operation.

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

https://codereview.chromium.org/1465803003/diff/140001/components/mus/public/...
components/mus/public/interfaces/window_tree.mojom:210: SetStandardCursor(uint32
window_id, Cursor cursor_id);
On 2015/12/01 21:48:00, sky wrote:
> Make this take a change_id, like SetWindowProperty does. See how
> SetWindowProperty deals with this for how to handle it.
> 
> I was a bit thrown by the name, did you consider 'predefined', as that is what
> it is?

Done.

https://codereview.chromium.org/1465803003/diff/140001/components/mus/ws/conn...
File components/mus/ws/connection_manager.cc (right):

https://codereview.chromium.org/1465803003/diff/140001/components/mus/ws/conn...
components/mus/ws/connection_manager.cc:481: // Pass the cursor change to our
native window.
On 2015/12/01 21:48:00, sky wrote:
> out->the (as there may be more than one native window).

Done.

https://codereview.chromium.org/1465803003/diff/140001/components/mus/ws/even...
File components/mus/ws/event_dispatcher.h (right):

https://codereview.chromium.org/1465803003/diff/140001/components/mus/ws/even...
components/mus/ws/event_dispatcher.h:36: // Retrieves the ServerWindow of the
last mouse move.
On 2015/12/01 21:48:00, sky wrote:
> Your code is setting for any pointer event, not just mouse. If you only want
> mouse then you need to update the code.

Filtered on PointerKind here and in the dispatching code.

https://codereview.chromium.org/1465803003/diff/140001/components/mus/ws/wind...
File components/mus/ws/window_tree_host_impl.cc (right):

https://codereview.chromium.org/1465803003/diff/140001/components/mus/ws/wind...
components/mus/ws/window_tree_host_impl.cc:137: if (window ==
event_dispatcher_.last_targeted_window())
On 2015/12/01 21:48:01, sky wrote:
> Doesn't this need to be more sophisticated? What if the last event target is
no
> longer under the mouse?

So I might just be wrong about this, but in event_dispatcher.cc, we appear to
track implicit captures in EventDispatcher::ProcessPointerEvent() before it
passes to DispatchToPointerTarget, which actually sets this targeted window.

Is this insufficient?

[sky, 2015-12-02 21:19:41]

https://codereview.chromium.org/1465803003/diff/140001/components/mus/ws/wind...
File components/mus/ws/window_tree_host_impl.cc (right):

https://codereview.chromium.org/1465803003/diff/140001/components/mus/ws/wind...
components/mus/ws/window_tree_host_impl.cc:137: if (window ==
event_dispatcher_.last_targeted_window())
On 2015/12/02 20:01:32, Elliot Glaysher wrote:
> On 2015/12/01 21:48:01, sky wrote:
> > Doesn't this need to be more sophisticated? What if the last event target is
> no
> > longer under the mouse?
> 
> So I might just be wrong about this, but in event_dispatcher.cc, we appear to
> track implicit captures in EventDispatcher::ProcessPointerEvent() before it
> passes to DispatchToPointerTarget, which actually sets this targeted window.

You are correct. We currently only have implicit capture.

> Is this insufficient?

There are a couple of cases that come to mind:
. press mouse, move mouse over another window. last_targeted_window is now the
wrong window, right?
. press and release mouse, z-order of windows changes, last_targeted_window is
now the wrong window.

[sky, 2015-12-02 21:42:35]

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

https://codereview.chromium.org/1465803003/diff/200001/components/mus/public/...
components/mus/public/cpp/lib/in_flight_change.cc:100: return static_cast<const
InFlightPredefinedCursorChange&>(change)
You shouldn't need to override Matches. The reason Matches is virtual is if the
type is not enough. That isn't the case for cursors as each window only has one
cursor.

https://codereview.chromium.org/1465803003/diff/200001/components/mus/public/...
File components/mus/public/cpp/lib/in_flight_change.h (right):

https://codereview.chromium.org/1465803003/diff/200001/components/mus/public/...
components/mus/public/cpp/lib/in_flight_change.h:31: PREDEFINED_CURSOR
nit: keep sorted.

https://codereview.chromium.org/1465803003/diff/200001/components/mus/public/...
File components/mus/public/cpp/window_observer.h (right):

https://codereview.chromium.org/1465803003/diff/200001/components/mus/public/...
components/mus/public/cpp/window_observer.h:67: virtual void
OnWindowSetPredefinedCursor(Window* window,
OnWindowPredefinedCursorChanged

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

https://codereview.chromium.org/1465803003/diff/200001/components/mus/public/...
components/mus/public/interfaces/window_tree.mojom:327:
OnWindowSetPredefinedCursor(uint32 window_id, Cursor cursor_id);
OnWindowPredefinedCursorChanged.

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

https://codereview.chromium.org/1465803003/diff/200001/components/mus/ws/even...
components/mus/ws/event_dispatcher.h:99: ServerWindow*
last_pointer_target_window_;
As this is specific to a mouse, name it last_mouse_target_window_.

https://codereview.chromium.org/1465803003/diff/200001/components/mus/ws/oper...
File components/mus/ws/operation.h (right):

https://codereview.chromium.org/1465803003/diff/200001/components/mus/ws/oper...
components/mus/ws/operation.h:32: SET_WINDOW_PREDEFINED_CURSOR,
nit: keep these sorted too.

https://codereview.chromium.org/1465803003/diff/200001/components/mus/ws/serv...
File components/mus/ws/server_window_observer.h (right):

https://codereview.chromium.org/1465803003/diff/200001/components/mus/ws/serv...
components/mus/ws/server_window_observer.h:59: virtual void
OnWindowSetPredefinedCursor(ServerWindow* window,
OnWindowPredefinedCursorChanged.

https://codereview.chromium.org/1465803003/diff/200001/components/mus/ws/wind...
File components/mus/ws/window_tree_unittest.cc (right):

https://codereview.chromium.org/1465803003/diff/200001/components/mus/ws/wind...
components/mus/ws/window_tree_unittest.cc:227: TestDisplayManager(int32_t*
cursor_id_storage)
nit: explicit

https://codereview.chromium.org/1465803003/diff/200001/components/mus/ws/wind...
components/mus/ws/window_tree_unittest.cc:264:
TestDisplayManagerFactory(int32_t* cursor_id_storage)
explicit

https://codereview.chromium.org/1465803003/diff/200001/components/mus/ws/wind...
components/mus/ws/window_tree_unittest.cc:363: void
SetupEventTargeting(TestWindowTreeClient** out_client,
Add description.

https://codereview.chromium.org/1465803003/diff/200001/components/mus/ws/wind...
components/mus/ws/window_tree_unittest.cc:398: void
WindowTreeTest::SetupEventTargeting(TestWindowTreeClient** out_client,
Make sure you wrap calls to this in EXPECT_NO_FATAL_FAILURE.

[Elliot Glaysher, 2015-12-03 00:54:57]

Renaming, explicit, other comments addressed.

On 2015/12/02 21:19:41, sky wrote:
> On 2015/12/02 20:01:32, Elliot Glaysher wrote:
> > Is this insufficient?
> 
> There are a couple of cases that come to mind:
> . press mouse, move mouse over another window. last_targeted_window is now the
> wrong window, right?

I have turned this into a unit test and adapted the code in EventDispatcher and
WindowTreeHostImpl so that it passes.

> . press and release mouse, z-order of windows changes, last_targeted_window is
> now the wrong window.

I'm not sure what this case is referring to. This might have incidentally been
fixed by the above, as on mouse up, it looks for the window under the mouse
pointer to select the cursor.

[sky, 2015-12-03 03:23:18]

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

https://codereview.chromium.org/1465803003/diff/280001/components/mus/ws/even...
components/mus/ws/event_dispatcher.cc:227: DCHECK(event->action ==
mojom::EVENT_TYPE_POINTER_DOWN);
This should be the place you add the logic for the down. For a mouse this is
only entered on the first mouse button down, which is what you want.

https://codereview.chromium.org/1465803003/diff/280001/components/mus/ws/even...
components/mus/ws/event_dispatcher.cc:255: ServerWindow* target =
pointer_targets_[pointer_id].window;
And this should be the place you add logic for the up. This block is hit for
both touch and mouse, so you'll need to add a check to make sure the pointer
corresponds to a mouse.

[sky, 2015-12-03 03:24:42]

On 2015/12/03 00:54:57, Elliot Glaysher wrote:
> Renaming, explicit, other comments addressed.
> 
> On 2015/12/02 21:19:41, sky wrote:
> > On 2015/12/02 20:01:32, Elliot Glaysher wrote:
> > > Is this insufficient?
> > 
> > There are a couple of cases that come to mind:
> > . press mouse, move mouse over another window. last_targeted_window is now
the
> > wrong window, right?
> 
> I have turned this into a unit test and adapted the code in EventDispatcher
and
> WindowTreeHostImpl so that it passes.
> 
> > . press and release mouse, z-order of windows changes, last_targeted_window
is
> > now the wrong window.
> 
> I'm not sure what this case is referring to. This might have incidentally been
> fixed by the above, as on mouse up, it looks for the window under the mouse
> pointer to select the cursor.

lets say you have two windows, A and B, with A on top of B. Mouse is not down
and wast last over A. If B is stacked above A, and the mouse didn't mouse, then
you need to reset the cursor from the window that is now stacked on top. Make
sense?

[Elliot Glaysher, 2015-12-03 21:09:30]

- Moved the implementation of tracking the pointer.
- Added comment to handle the programmatic change case later.

[sky, 2015-12-03 22:01:10]

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

https://codereview.chromium.org/1465803003/diff/300001/components/mus/public/...
components/mus/public/cpp/lib/window_tree_client_impl.cc:619:
FOR_EACH_OBSERVER(WindowObserver, *WindowPrivate(window).observers(),
You need two things here:
1. If there is an in flight cursor change, you need to merge with it. See
OnWindowBoundsChanged for an example of what I mean.
2. Potentially set the cursor value locally. Again, follow the modle of
OnWindowBoundsChanged.

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

https://codereview.chromium.org/1465803003/diff/300001/components/mus/ws/even...
components/mus/ws/event_dispatcher.cc:238: mouse_button_down_ = true;
This path is hit for both touch and mouse. Don't you only want to set these if
it's a mouse event?

[Elliot Glaysher, 2015-12-03 22:32:00]

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

https://codereview.chromium.org/1465803003/diff/300001/components/mus/public/...
components/mus/public/cpp/lib/window_tree_client_impl.cc:619:
FOR_EACH_OBSERVER(WindowObserver, *WindowPrivate(window).observers(),
On 2015/12/03 22:01:10, sky wrote:
> You need two things here:
> 1. If there is an in flight cursor change, you need to merge with it. See
> OnWindowBoundsChanged for an example of what I mean.
> 2. Potentially set the cursor value locally. Again, follow the modle of
> OnWindowBoundsChanged.

Done. (I think.)

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

https://codereview.chromium.org/1465803003/diff/300001/components/mus/ws/even...
components/mus/ws/event_dispatcher.cc:238: mouse_button_down_ = true;
On 2015/12/03 22:01:10, sky wrote:
> This path is hit for both touch and mouse. Don't you only want to set these if
> it's a mouse event?

Is it? In a previous comment you said this was only entered on first button
down, and that it was the other block that had to deal with touch.

Whatever. Put all access to mouse_cursor_source_window_ behind a variable that
checks POINTER_KIND_MOUSE.

[sky, 2015-12-03 23:39:56]

LGTM

[Elliot Glaysher, 2015-12-03 23:42:51]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2015-12-03 23:44:16]

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

[commit-bot: I haz the power, 2015-12-04 00:31:47]

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

[Elliot Glaysher, 2015-12-04 18:08:30]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2015-12-04 18:09:07]

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

[commit-bot: I haz the power, 2015-12-04 18:51:57]

Description was changed from

==========
mus: Let clients set the cursor of their window.

This adds a cursor property for each window. mus changes the cursor as the
pointer passes between windows.

BUG=548428
==========

to

==========
mus: Let clients set the cursor of their window.

This adds a cursor property for each window. mus changes the cursor as the
pointer passes between windows.

BUG=548428
==========

[commit-bot: I haz the power, 2015-12-04 18:51:58]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2015-12-04 18:52:51]

Description was changed from

==========
mus: Let clients set the cursor of their window.

This adds a cursor property for each window. mus changes the cursor as the
pointer passes between windows.

BUG=548428
==========

to

==========
mus: Let clients set the cursor of their window.

This adds a cursor property for each window. mus changes the cursor as the
pointer passes between windows.

BUG=548428

Committed: https://crrev.com/314e9981a45aa20338d87c1952977603b051c725
Cr-Commit-Position: refs/heads/master@{#363261}
==========

[commit-bot: I haz the power, 2015-12-04 18:52:52]

Patchset 17 (id:??) landed as
https://crrev.com/314e9981a45aa20338d87c1952977603b051c725
Cr-Commit-Position: refs/heads/master@{#363261}