mus Window Server: implement event capture

mus Window Server: implement event capture
This CL implements an explicit capture API that will be used for popups.

This is a split of the review: https://codereview.chromium.org/1605773004/
Where this review encompasses only the Window Server functionality. The client lib code is just stubs for compilation. Full implementation of the client will follow in a separate review.

TEST=EventDispatcherTest.SetExplicitCapture, EventDispatcherTest.ExplicitCaptureOverridesImplicitCapture, EventDispatcherTest.CaptureUpdatesActivePointerTargets, EventDispatcherTest.UpdatingCaptureStopsObservingPreviousCaputre, EventDispatcherTest.DestroyingCaptureWindowRemovesExplicitCapture
BUG=533161
Committed: https://crrev.com/b72ed077b4cb7309c5e3ee6bb80944a3deab6250
Cr-Commit-Position: refs/heads/master@{#374824}

[jonross, 2016-02-05 15:25:08]

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

[jonross, 2016-02-05 15:25:08]

Hi sky,

I've separated out the window server implementation of capture to this review.
PTAL

[sky, 2016-02-08 16:57:11]

https://codereview.chromium.org/1677513002/diff/1/components/mus/public/inter...
File components/mus/public/interfaces/window_tree.mojom (right):

https://codereview.chromium.org/1677513002/diff/1/components/mus/public/inter...
components/mus/public/interfaces/window_tree.mojom:116: // Releases input event
capture for the given |window_id|.
Document this does nothing if |window_id| does not currently have capture.

https://codereview.chromium.org/1677513002/diff/1/components/mus/ws/window_tr...
File components/mus/ws/window_tree_host_impl.cc (right):

https://codereview.chromium.org/1677513002/diff/1/components/mus/ws/window_tr...
components/mus/ws/window_tree_host_impl.cc:177: display_manager_->SetCapture();
I think this is the wrong place for setting/releasing capture on the
displaymanager. In particular this doesn't cover implicit capture, which should
set/release capture too. I think the delegate should get methods for
setting/releasing capture that call to the displaymanager.

[jonross, 2016-02-10 15:57:57]

https://codereview.chromium.org/1677513002/diff/1/components/mus/public/inter...
File components/mus/public/interfaces/window_tree.mojom (right):

https://codereview.chromium.org/1677513002/diff/1/components/mus/public/inter...
components/mus/public/interfaces/window_tree.mojom:116: // Releases input event
capture for the given |window_id|.
On 2016/02/08 16:57:11, sky wrote:
> Document this does nothing if |window_id| does not currently have capture.

Done.

https://codereview.chromium.org/1677513002/diff/1/components/mus/ws/window_tr...
File components/mus/ws/window_tree_host_impl.cc (right):

https://codereview.chromium.org/1677513002/diff/1/components/mus/ws/window_tr...
components/mus/ws/window_tree_host_impl.cc:177: display_manager_->SetCapture();
On 2016/02/08 16:57:11, sky wrote:
> I think this is the wrong place for setting/releasing capture on the
> displaymanager. In particular this doesn't cover implicit capture, which
should
> set/release capture too. I think the delegate should get methods for
> setting/releasing capture that call to the displaymanager.

Updated EventDispatcherDelegate to have Set/ReleaseCapture.
WTHI notifies the DisplayManager in those.
EventDispatcher uses those for updating on explicit capture.

Currently DefaultDisplayManager is directly signaling the platform window of
implicit capture, without interacting with event dispatcher.

[sky, 2016-02-10 17:38:22]

https://codereview.chromium.org/1677513002/diff/40001/components/mus/ws/event...
File components/mus/ws/event_dispatcher.cc (right):

https://codereview.chromium.org/1677513002/diff/40001/components/mus/ws/event...
components/mus/ws/event_dispatcher.cc:324: if (!IsTrackingPointer(pointer_id) ||
Shouldn't we SetCapture and ReleaseCapture in here too?

https://codereview.chromium.org/1677513002/diff/40001/components/mus/ws/event...
components/mus/ws/event_dispatcher.cc:457: delegate_->OnLostCapture(window);
AFAICT you won't release native capture here. By that I mean this calls to the
delegate which does not call ReleaseCapture on the DisplayManager.

https://codereview.chromium.org/1677513002/diff/40001/components/mus/ws/event...
File components/mus/ws/event_dispatcher_delegate.h (right):

https://codereview.chromium.org/1677513002/diff/40001/components/mus/ws/event...
components/mus/ws/event_dispatcher_delegate.h:25: virtual void
SetCapture(ServerWindow* window) = 0;
Clarify when these are called. In particular it's not clear when ReleaseCapture
vs OnLostCapture is called.
Also, I wouldn't supply a window to SetCapture/ReleaseCapture unless you need
it. It helps clarify this is intended to set/release capture on the native
display. In fact maybe the two should be called
SetNativeCapture/ReleaseNativeCapture.

https://codereview.chromium.org/1677513002/diff/40001/components/mus/ws/windo...
File components/mus/ws/window_tree_host_impl.cc (right):

https://codereview.chromium.org/1677513002/diff/40001/components/mus/ws/windo...
components/mus/ws/window_tree_host_impl.cc:355: if (window)
This should never supply null, right? If I'm right, then DCHECK(window). If I'm
wrong, why would we supply null?

[jonross, 2016-02-10 19:45:49]

https://codereview.chromium.org/1677513002/diff/40001/components/mus/ws/event...
File components/mus/ws/event_dispatcher.cc (right):

https://codereview.chromium.org/1677513002/diff/40001/components/mus/ws/event...
components/mus/ws/event_dispatcher.cc:324: if (!IsTrackingPointer(pointer_id) ||
On 2016/02/10 17:38:22, sky wrote:
> Shouldn't we SetCapture and ReleaseCapture in here too?

Right now DisplayManager is doing its own version of implicit capture directly
in DispatchEvent:

https://code.google.com/p/chromium/codesearch#chromium/src/components/mus/ws/...

This feels wrong, and I do believe that it should go in here.

Shall I tear that out now? or in a followup?

https://codereview.chromium.org/1677513002/diff/40001/components/mus/ws/event...
components/mus/ws/event_dispatcher.cc:457: delegate_->OnLostCapture(window);
On 2016/02/10 17:38:22, sky wrote:
> AFAICT you won't release native capture here. By that I mean this calls to the
> delegate which does not call ReleaseCapture on the DisplayManager.

Correct.

DisplayManagerDelegate::OnLostCapture() leads to updating the EventDispatcher
EventDispatcherDelegate::OnLostCapture(ServerWindow*) leads to updating the
client app.

I'm adding the EventDispatcherDelegate::ReleaseCapture call here.

https://codereview.chromium.org/1677513002/diff/40001/components/mus/ws/event...
File components/mus/ws/event_dispatcher_delegate.h (right):

https://codereview.chromium.org/1677513002/diff/40001/components/mus/ws/event...
components/mus/ws/event_dispatcher_delegate.h:25: virtual void
SetCapture(ServerWindow* window) = 0;
On 2016/02/10 17:38:22, sky wrote:
> Clarify when these are called. In particular it's not clear when
ReleaseCapture
> vs OnLostCapture is called.
> Also, I wouldn't supply a window to SetCapture/ReleaseCapture unless you need
> it. It helps clarify this is intended to set/release capture on the native
> display. In fact maybe the two should be called
> SetNativeCapture/ReleaseNativeCapture.

Done.

As discussed offline the two OnLostCapture methods have been renamed to make
them more clear as to their functionality

https://codereview.chromium.org/1677513002/diff/40001/components/mus/ws/windo...
File components/mus/ws/window_tree_host_impl.cc (right):

https://codereview.chromium.org/1677513002/diff/40001/components/mus/ws/windo...
components/mus/ws/window_tree_host_impl.cc:355: if (window)
On 2016/02/10 17:38:22, sky wrote:
> This should never supply null, right? If I'm right, then DCHECK(window). If
I'm
> wrong, why would we supply null?

Window is no longer passed in. So removed.

[sky, 2016-02-10 20:06:32]

https://codereview.chromium.org/1677513002/diff/40001/components/mus/ws/event...
File components/mus/ws/event_dispatcher.cc (right):

https://codereview.chromium.org/1677513002/diff/40001/components/mus/ws/event...
components/mus/ws/event_dispatcher.cc:324: if (!IsTrackingPointer(pointer_id) ||
On 2016/02/10 19:45:49, jonross wrote:
> On 2016/02/10 17:38:22, sky wrote:
> > Shouldn't we SetCapture and ReleaseCapture in here too?
> 
> Right now DisplayManager is doing its own version of implicit capture directly
> in DispatchEvent:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/mus/ws/...
> 
> This feels wrong, and I do believe that it should go in here.
> 
> Shall I tear that out now? or in a followup?

Yes please as otherwise things are in a bit of weird state.

[jonross, 2016-02-10 22:51:01]

https://codereview.chromium.org/1677513002/diff/40001/components/mus/ws/event...
File components/mus/ws/event_dispatcher.cc (right):

https://codereview.chromium.org/1677513002/diff/40001/components/mus/ws/event...
components/mus/ws/event_dispatcher.cc:324: if (!IsTrackingPointer(pointer_id) ||
On 2016/02/10 20:06:32, sky wrote:
> On 2016/02/10 19:45:49, jonross wrote:
> > On 2016/02/10 17:38:22, sky wrote:
> > > Shouldn't we SetCapture and ReleaseCapture in here too?
> > 
> > Right now DisplayManager is doing its own version of implicit capture
directly
> > in DispatchEvent:
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/mus/ws/...
> > 
> > This feels wrong, and I do believe that it should go in here.
> > 
> > Shall I tear that out now? or in a followup?
> 
> Yes please as otherwise things are in a bit of weird state.

Done.

[sky, 2016-02-10 23:30:02]

LGTM - thanks for seeing this through!

https://codereview.chromium.org/1677513002/diff/80001/components/mus/ws/event...
File components/mus/ws/event_dispatcher.cc (right):

https://codereview.chromium.org/1677513002/diff/80001/components/mus/ws/event...
components/mus/ws/event_dispatcher.cc:464: mouse_button_down_ = false;
It's better to update state before calling out to other classes. Move this
around 458 please.

https://codereview.chromium.org/1677513002/diff/80001/components/mus/ws/event...
File components/mus/ws/event_dispatcher_delegate.h (right):

https://codereview.chromium.org/1677513002/diff/80001/components/mus/ws/event...
components/mus/ws/event_dispatcher_delegate.h:31: // holding capture.
Please add something along the lines of 'delegate should not change native
display capture. ReleaseNativeCapture() is invoked if appropriate.'

[jonross, 2016-02-11 00:26:28]

https://codereview.chromium.org/1677513002/diff/80001/components/mus/ws/event...
File components/mus/ws/event_dispatcher.cc (right):

https://codereview.chromium.org/1677513002/diff/80001/components/mus/ws/event...
components/mus/ws/event_dispatcher.cc:464: mouse_button_down_ = false;
On 2016/02/10 23:30:02, sky wrote:
> It's better to update state before calling out to other classes. Move this
> around 458 please.

Done.

https://codereview.chromium.org/1677513002/diff/80001/components/mus/ws/event...
File components/mus/ws/event_dispatcher_delegate.h (right):

https://codereview.chromium.org/1677513002/diff/80001/components/mus/ws/event...
components/mus/ws/event_dispatcher_delegate.h:31: // holding capture.
On 2016/02/10 23:30:02, sky wrote:
> Please add something along the lines of 'delegate should not change native
> display capture. ReleaseNativeCapture() is invoked if appropriate.'

Done.

[jonross, 2016-02-11 00:26:32]

The CQ bit was checked by jonross@chromium.org

[jonross, 2016-02-11 00:26:32]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1677513002/#ps100001
(title: " ")

[commit-bot: I haz the power, 2016-02-11 00:27:26]

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

[commit-bot: I haz the power, 2016-02-11 01:29:55]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-02-16 22:33:44]

Description was changed from

==========
mus Window Server: implement event capture
This CL implements an explicit capture API that will be used for popups.

This is a split of the review: https://codereview.chromium.org/1605773004/
Where this review encompasses only the Window Server functionality. The client
lib code is just stubs for compilation. Full implementation of the client will
follow in a separate review.

TEST=EventDispatcherTest.SetExplicitCapture,
EventDispatcherTest.ExplicitCaptureOverridesImplicitCapture,
EventDispatcherTest.CaptureUpdatesActivePointerTargets,
EventDispatcherTest.UpdatingCaptureStopsObservingPreviousCaputre,
EventDispatcherTest.DestroyingCaptureWindowRemovesExplicitCapture
BUG=533161
==========

to

==========
mus Window Server: implement event capture
This CL implements an explicit capture API that will be used for popups.

This is a split of the review: https://codereview.chromium.org/1605773004/
Where this review encompasses only the Window Server functionality. The client
lib code is just stubs for compilation. Full implementation of the client will
follow in a separate review.

TEST=EventDispatcherTest.SetExplicitCapture,
EventDispatcherTest.ExplicitCaptureOverridesImplicitCapture,
EventDispatcherTest.CaptureUpdatesActivePointerTargets,
EventDispatcherTest.UpdatingCaptureStopsObservingPreviousCaputre,
EventDispatcherTest.DestroyingCaptureWindowRemovesExplicitCapture
BUG=533161

Committed: https://crrev.com/b72ed077b4cb7309c5e3ee6bb80944a3deab6250
Cr-Commit-Position: refs/heads/master@{#374824}
==========

[commit-bot: I haz the power, 2016-02-16 22:33:45]

Patchset 6 (id:??) landed as
https://crrev.com/b72ed077b4cb7309c5e3ee6bb80944a3deab6250
Cr-Commit-Position: refs/heads/master@{#374824}

[fwang, 2017-03-28 09:31:41]

fwang@igalia.com changed reviewers:
+ fwang@igalia.com