mus: Implement Window Server Capture

mus: Implement Window Server Capture

This CL implements an explicit capture API that will be used for popups.

BUG=533161

[Fady Samuel, 2015-09-17 23:55:29]

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

[sadrul, 2015-09-18 01:30:49]

sadrul@chromium.org changed reviewers:
+ sadrul@chromium.org

[sadrul, 2015-09-18 01:30:50]

Both DisplayManager and View should have corresponding ReleaseCapture support as
well.

In the CL description: s/mandoline/mus/

https://codereview.chromium.org/1352043005/diff/40001/components/mus/public/c...
File components/mus/public/cpp/view.h (right):

https://codereview.chromium.org/1352043005/diff/40001/components/mus/public/c...
components/mus/public/cpp/view.h:135: void SetCapture();
This should come with a corresponding ReleaseCapture?

https://codereview.chromium.org/1352043005/diff/40001/components/mus/view_tre...
File components/mus/view_tree_apptest.cc (right):

https://codereview.chromium.org/1352043005/diff/40001/components/mus/view_tre...
components/mus/view_tree_apptest.cc:1686: SetViewCapture(vm1(), view_1_2);
What if the two setcapture calls come from two separate clients?

https://codereview.chromium.org/1352043005/diff/40001/components/mus/view_tre...
components/mus/view_tree_apptest.cc:1694: }
?

[Fady Samuel, 2015-09-18 02:44:43]

PTAL Sadrul. I added a ReleaseCapture. I'm not sure what to do about Capture
across ViewTreeHosts. Thoughts? I would think the operating system would
automagically do the right thing?

https://codereview.chromium.org/1352043005/diff/40001/components/mus/public/c...
File components/mus/public/cpp/view.h (right):

https://codereview.chromium.org/1352043005/diff/40001/components/mus/public/c...
components/mus/public/cpp/view.h:135: void SetCapture();
On 2015/09/18 01:30:49, sadrul wrote:
> This should come with a corresponding ReleaseCapture?

Done.

https://codereview.chromium.org/1352043005/diff/40001/components/mus/view_tre...
File components/mus/view_tree_apptest.cc (right):

https://codereview.chromium.org/1352043005/diff/40001/components/mus/view_tre...
components/mus/view_tree_apptest.cc:1686: SetViewCapture(vm1(), view_1_2);
On 2015/09/18 01:30:50, sadrul wrote:
> What if the two setcapture calls come from two separate clients?

So two separate windows? If one tries to capture, the OS will release capture
for the other, right? I'm not sure how to test that.

https://codereview.chromium.org/1352043005/diff/40001/components/mus/view_tre...
components/mus/view_tree_apptest.cc:1694: }
On 2015/09/18 01:30:50, sadrul wrote:
> ?

Removed.

[sadrul, 2015-09-18 11:47:29]

https://codereview.chromium.org/1352043005/diff/40001/components/mus/view_tre...
File components/mus/view_tree_apptest.cc (right):

https://codereview.chromium.org/1352043005/diff/40001/components/mus/view_tre...
components/mus/view_tree_apptest.cc:1686: SetViewCapture(vm1(), view_1_2);
On 2015/09/18 02:44:43, Fady Samuel wrote:
> On 2015/09/18 01:30:50, sadrul wrote:
> > What if the two setcapture calls come from two separate clients?
> 
> So two separate windows? If one tries to capture, the OS will release capture
> for the other, right?

Yep. (in most cases anyway. The second capture can be declined in some cases)

I meant for the case where an embedder and an oopif both do SetCapture (i.e.
different tree-clients within the same host) one after the other.

> I'm not sure how to test that.

[Fady Samuel, 2015-11-03 15:14:09]

Description was changed from

==========
mus: Implement View Manager Capture

This CL doesn't actually affect input event forwarding. Once the OOPIF
transform/clipping CL lands, I'll write more code built on top of that in
combination with this.

BUG=533161
==========

to

==========
mus: Implement Window Server Capture

This CL doesn't actually affect input event forwarding. Once the OOPIF
transform/clipping CL lands, I'll write more code built on top of that in
combination with this.

BUG=533161
==========

[Fady Samuel, 2015-11-16 23:00:39]

Description was changed from

==========
mus: Implement Window Server Capture

This CL doesn't actually affect input event forwarding. Once the OOPIF
transform/clipping CL lands, I'll write more code built on top of that in
combination with this.

BUG=533161
==========

to

==========
mus: Implement Window Server Capture

This CL implements an explicit capture API that will be used for popups.

BUG=533161
==========

[Fady Samuel, 2015-11-16 23:00:40]

fsamuel@chromium.org changed reviewers:
- sadrul@chromium.org

[Fady Samuel, 2015-11-16 23:02:04]

Hi Scott,

I've reimplemented the explicit capture API and added a few tests. Could you
please take a look? Thanks!

I don't track capture in the client lib. Do you think it makes sense to do so?
It's not possible to track the current capture window in the client lib
currently.

Fady

[sky, 2015-11-17 00:40:54]

On Mon, Nov 16, 2015 at 3:02 PM,  <fsamuel@chromium.org> wrote:
> Hi Scott,
>
> I've reimplemented the explicit capture API and added a few tests. Could you
> please take a look? Thanks!
>
> I don't track capture in the client lib. Do you think it makes sense to do
> so?

Definitely. Otherwise how would the client know if it lost capture for
some reason?

> It's not possible to track the current capture window in the client lib
> currently.

It should be, similar to focus.

  -Scott
>
> Fady
>
> https://codereview.chromium.org/1352043005/

-- 
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, 2015-11-17 01:06:42]

I only made it part way through this. Some things to consider. The common case
is on first mouse button down we grab capture and release on last mouse button
up. Views code explicitly calls to set capture like this. The client lib should
track this, so that calls to setcapture/releasecapture during this time don't do
anything. Bad things could happen if we didn't have the client lib deal with
this. For example, consider a quick press/release. It's entirely possible for
the press/release to be processed before the client has received anything. If
the client does a SetCapture as well, and it makes it to the server, then the
client could end up getting some moves outside the bounds of the window that it
shouldn't. I'm thinking of this sequence:

mouse press
mouse release
move
move
move

It's conceivable this is processed (on the server) as:

mouse press
mouse release

setcapture
move
move
move


I think the client needs to know what the server is going to do. I also think
we'll need some way to opt out of the implicit release capture for certain
things (like menus).

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

https://codereview.chromium.org/1352043005/diff/120001/components/mus/public/...
components/mus/public/cpp/lib/window.cc:381: if (connection_)
We should track capture locally too.

https://codereview.chromium.org/1352043005/diff/120001/components/mus/public/...
File components/mus/public/cpp/window.h (right):

https://codereview.chromium.org/1352043005/diff/120001/components/mus/public/...
components/mus/public/cpp/window.h:170: // Capture.
This comment isn't really useful, nuke it.

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

https://codereview.chromium.org/1352043005/diff/120001/components/mus/ws/even...
components/mus/ws/event_dispatcher.cc:152: CancelAllPointerEvents();
If window is in pointer_targets_ won't this incorrectly remove it?

https://codereview.chromium.org/1352043005/diff/120001/components/mus/ws/even...
components/mus/ws/event_dispatcher.cc:286: if (capture_window_) {
Based on the name I wouldn't expect this to reset capture_window_ too.

https://codereview.chromium.org/1352043005/diff/120001/components/mus/ws/even...
components/mus/ws/event_dispatcher.cc:290: std::set<ServerWindow*>
pointer_targets;
The windows should see a cancel when this happens.