mus: Implement Window Server Capture

mus: Implement Window Server Capture

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

This is an uprev of the original review: https://codereview.chromium.org/1352043005/

TEST=WindowTest.SetCapture, EventDispatcherTest.SetExplicitCapture, EventDispatcherTest.ExplicitCaptureOverridesImplicitCapture, EventDispatcherTest.CaptureUpdatesActivePointerTargets, EventDispatcherTest.UpdatingCaptureStopsObservingPreviousCaputre, EventDispatcherTest.DestroyingCaptureWindowRemovesExplicitCapture, WindowTreeAppTest.ExplicitCapturePropagation,
BUG=533161

[jonross, 2016-01-21 20:33:20]

Patchset #5 (id:80001) has been deleted

[jonross, 2016-01-21 20:43:01]

Description was changed from

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

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

This is an uprev of the original review:
https://codereview.chromium.org/1352043005/

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

to

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

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

This is an uprev of the original review:
https://codereview.chromium.org/1352043005/

TEST=WindowTest.SetCapture, EventDispatcherTest.SetExplicitCapture,
EventDispatcherTest.ExplicitCaptureOverridesImplicitCapture,
EventDispatcherTest.CaptureUpdatesActivePointerTargets,
EventDispatcherTest.UpdatingCaptureStopsObservingPreviousCaputre,
EventDispatcherTest.DestroyingCaptureWindowRemovesExplicitCapture,
WindowTreeAppTest.ExplicitCapturePropagation,
BUG=533161
==========

[jonross, 2016-01-21 22:01:06]

jonross@chromium.org changed reviewers:
+ fsamuel@google.com

[jonross, 2016-01-21 22:01:12]

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

https://codereview.chromium.org/1605773004/diff/100001/components/mus/ws/even...
components/mus/ws/event_dispatcher.cc:185: std::set<ServerWindow*>
unobserved_windows;
There was a comment in the previous review:

"The windows should see a cancel when this happens."

I was not sure if that was related to OnLostCapture, which is on line 187 now,
or some other notification.

I didn't see any notification in the other areas where window tracking stops.

[jonross, 2016-01-21 22:02:04]

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

[jonross, 2016-01-21 22:02:04]

Hi Sky,

I've updated a previous review for implementing Window Server Capture. Including
addressing the last set of comments.

Could you provide an owners review?

Thanks.

[sky, 2016-01-22 00:55:00]

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

https://codereview.chromium.org/1605773004/diff/100001/components/mus/ws/even...
components/mus/ws/event_dispatcher.cc:179: void
EventDispatcher::SetCaptureWindow(ServerWindow* window) {
You need to also pass in in_nonclient_area. I think that should be determined
based on who is making the request.

https://codereview.chromium.org/1605773004/diff/100001/components/mus/ws/even...
components/mus/ws/event_dispatcher.cc:197: window_implicit_targets[pair.first] =
pair.second;
Why do you need to maintain pointer_targets_ when there is a capture window?
Seems like we should just empty it out.

https://codereview.chromium.org/1605773004/diff/100001/components/mus/ws/even...
components/mus/ws/event_dispatcher.cc:199: target->RemoveObserver(this);
You should send POINTER_CANCEL here.

https://codereview.chromium.org/1605773004/diff/100001/components/mus/ws/even...
components/mus/ws/event_dispatcher.cc:433: return;
Why the early out here? Don't we still need to reset pointer_targets_?

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

https://codereview.chromium.org/1605773004/diff/100001/components/mus/ws/even...
components/mus/ws/event_dispatcher.h:41: void SetCaptureWindow(ServerWindow*
capture_window);
You should document what capturewindow means and what the results of it are.
Maybe that should be in the mojom and here say look to the mojom for the
details.

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

https://codereview.chromium.org/1605773004/diff/100001/components/mus/ws/wind...
components/mus/ws/window_tree_host_impl.cc:159: ServerWindow* capture_window =
event_dispatcher_.capture_window();
Early out if window == capture_window?

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

https://codereview.chromium.org/1605773004/diff/100001/components/mus/ws/wind...
components/mus/ws/window_tree_impl.cc:555:
client()->OnLostCapture(WindowIdToTransportId(old_capture_window->id()));
You should only send this if it was not initiated by this client. By that I mean
if the client called ReleaseCapture() then don't send OnLostCapture.

https://codereview.chromium.org/1605773004/diff/100001/components/mus/ws/wind...
components/mus/ws/window_tree_impl.cc:922:
access_policy_->CanSetCapture(host->GetCaptureWindow());
I am worried about letting any application capture mouse events at any time.
Seems like an easy way for an app to steal events.

The places I'm aware of requesting capture are in processing a mouse or key
event. Are there other places? If so, can we only allow the capture if the
client is processing an event or already has capture?

https://codereview.chromium.org/1605773004/diff/100001/components/mus/ws/wind...
components/mus/ws/window_tree_impl.cc:937: host->SetCapture(nullptr);
They way you have this now is set capture to null no matter what. I think this
should be set capture to null only if the window already has capture.

[jonross, 2016-01-22 20:31:28]

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

https://codereview.chromium.org/1605773004/diff/100001/components/mus/ws/even...
components/mus/ws/event_dispatcher.cc:179: void
EventDispatcher::SetCaptureWindow(ServerWindow* window) {
On 2016/01/22 00:55:00, sky wrote:
> You need to also pass in in_nonclient_area. I think that should be determined
> based on who is making the request.

I'm not sure if that is needed. There will not always be a direct connection
between an input event and a request to SetCaptureWindow.

Eg. A window launches which desires capture, like a menu.

https://codereview.chromium.org/1605773004/diff/100001/components/mus/ws/even...
components/mus/ws/event_dispatcher.cc:197: window_implicit_targets[pair.first] =
pair.second;
On 2016/01/22 00:55:00, sky wrote:
> Why do you need to maintain pointer_targets_ when there is a capture window?
> Seems like we should just empty it out.

This was based on a comment in the original review querying the removal of all
pointer targets. However I would agree that it isn't necessary while we have a
capture window. I'll switch this to clearing out the pointer_targets_

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

https://codereview.chromium.org/1605773004/diff/100001/components/mus/ws/wind...
components/mus/ws/window_tree_impl.cc:922:
access_policy_->CanSetCapture(host->GetCaptureWindow());
On 2016/01/22 00:55:00, sky wrote:
> I am worried about letting any application capture mouse events at any time.
> Seems like an easy way for an app to steal events.
> 
> The places I'm aware of requesting capture are in processing a mouse or key
> event. Are there other places? If so, can we only allow the capture if the
> client is processing an event or already has capture?

I would also believe that touch events would also want to be captured.

Eg. a window which wishes to dismiss itself when a touch occurs outside of its
bounds.

I would expect valid reasons for wanting capture:
  - Already processing an event, and want to see subsequent events which exit
the region of the client
  - A pseudo-modal dialog which wishes to dismiss itself when an event occurs
outside of its bounds. I'd expect this to request capture at creation time.

[sky, 2016-01-22 22:15:37]

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

https://codereview.chromium.org/1605773004/diff/100001/components/mus/ws/even...
components/mus/ws/event_dispatcher.cc:179: void
EventDispatcher::SetCaptureWindow(ServerWindow* window) {
On 2016/01/22 20:31:28, jonross wrote:
> On 2016/01/22 00:55:00, sky wrote:
> > You need to also pass in in_nonclient_area. I think that should be
determined
> > based on who is making the request.
> 
> I'm not sure if that is needed. There will not always be a direct connection
> between an input event and a request to SetCaptureWindow.
> 
> Eg. A window launches which desires capture, like a menu.

For windows with no one embedded in them there is only one client that gets
event. OTOH windows with another client embedded in them have two clients that
may get the event. The owner and the embedded client. If the owner does the
SetCapture, it should get the events. If the embedded app does the capture, it
should get the events. in_nonclient_frame is used to distinguish these two
cases. in_nonclient_frame means the owner gets the event, otherwise it goes to
the embedded app. It's probably worth renaming the value to make this clearer.
Make sense?

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

https://codereview.chromium.org/1605773004/diff/100001/components/mus/ws/wind...
components/mus/ws/window_tree_impl.cc:922:
access_policy_->CanSetCapture(host->GetCaptureWindow());
On 2016/01/22 20:31:28, jonross wrote:
> On 2016/01/22 00:55:00, sky wrote:
> > I am worried about letting any application capture mouse events at any time.
> > Seems like an easy way for an app to steal events.
> > 
> > The places I'm aware of requesting capture are in processing a mouse or key
> > event. Are there other places? If so, can we only allow the capture if the
> > client is processing an event or already has capture?
> 
> I would also believe that touch events would also want to be captured.

Agreed.

> Eg. a window which wishes to dismiss itself when a touch occurs outside of its
> bounds.
> 
> I would expect valid reasons for wanting capture:
>   - Already processing an event, and want to see subsequent events which exit
> the region of the client

Agreed.

>   - A pseudo-modal dialog which wishes to dismiss itself when an event occurs
> outside of its bounds. I'd expect this to request capture at creation time.

Generally we key off activation to dismiss on blur. Is there a place in chrome
code where we setcapture as you're saying?

[jonross, 2016-01-26 18:38:51]

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

https://codereview.chromium.org/1605773004/diff/100001/components/mus/ws/even...
components/mus/ws/event_dispatcher.cc:179: void
EventDispatcher::SetCaptureWindow(ServerWindow* window) {
On 2016/01/22 22:15:37, sky wrote:
> On 2016/01/22 20:31:28, jonross wrote:
> > On 2016/01/22 00:55:00, sky wrote:
> > > You need to also pass in in_nonclient_area. I think that should be
> determined
> > > based on who is making the request.
> > 
> > I'm not sure if that is needed. There will not always be a direct connection
> > between an input event and a request to SetCaptureWindow.
> > 
> > Eg. A window launches which desires capture, like a menu.
> 
> For windows with no one embedded in them there is only one client that gets
> event. OTOH windows with another client embedded in them have two clients that
> may get the event. The owner and the embedded client. If the owner does the
> SetCapture, it should get the events. If the embedded app does the capture, it
> should get the events. in_nonclient_frame is used to distinguish these two
> cases. in_nonclient_frame means the owner gets the event, otherwise it goes to
> the embedded app. It's probably worth renaming the value to make this clearer.
> Make sense?

That makes sense. I'm passing this in now based on which WindowTreeImpl is
performing the call, the owner, or the embedded app.

A rename may be a good idea. My initial interpretation was skewed by 
IsLocationInNonclientArea which tied the concept to a click location. I had not
realized that this mapped to different WindowTreeImpl in 
WindowTreeHostImpl::DispatchInputEventToWindowImpl.

WindowTreeHostImpl::OnFocusChanged also uses the owning vs embedded
nomenclature, so maybe a term such as "in_owning_client"

https://codereview.chromium.org/1605773004/diff/100001/components/mus/ws/even...
components/mus/ws/event_dispatcher.cc:185: std::set<ServerWindow*>
unobserved_windows;
On 2016/01/21 22:01:12, jonross wrote:
> There was a comment in the previous review:
> 
> "The windows should see a cancel when this happens."
> 
> I was not sure if that was related to OnLostCapture, which is on line 187 now,
> or some other notification.
> 
> I didn't see any notification in the other areas where window tracking stops.

Done.

https://codereview.chromium.org/1605773004/diff/100001/components/mus/ws/even...
components/mus/ws/event_dispatcher.cc:197: window_implicit_targets[pair.first] =
pair.second;
On 2016/01/22 20:31:28, jonross wrote:
> On 2016/01/22 00:55:00, sky wrote:
> > Why do you need to maintain pointer_targets_ when there is a capture window?
> > Seems like we should just empty it out.
> 
> This was based on a comment in the original review querying the removal of all
> pointer targets. However I would agree that it isn't necessary while we have a
> capture window. I'll switch this to clearing out the pointer_targets_

Done.

https://codereview.chromium.org/1605773004/diff/100001/components/mus/ws/even...
components/mus/ws/event_dispatcher.cc:199: target->RemoveObserver(this);
On 2016/01/22 00:55:00, sky wrote:
> You should send POINTER_CANCEL here.

Done.

https://codereview.chromium.org/1605773004/diff/100001/components/mus/ws/even...
components/mus/ws/event_dispatcher.cc:433: return;
On 2016/01/22 00:55:00, sky wrote:
> Why the early out here? Don't we still need to reset pointer_targets_?

With the change to always clear pointer_targets_ in SetCaptureWindow there will
no longer be any to clear here. So the early exit is now correct. However
there's no harm removing it if desired.

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

https://codereview.chromium.org/1605773004/diff/100001/components/mus/ws/even...
components/mus/ws/event_dispatcher.h:41: void SetCaptureWindow(ServerWindow*
capture_window);
On 2016/01/22 00:55:00, sky wrote:
> You should document what capturewindow means and what the results of it are.
> Maybe that should be in the mojom and here say look to the mojom for the
> details.

Done.

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

https://codereview.chromium.org/1605773004/diff/100001/components/mus/ws/wind...
components/mus/ws/window_tree_host_impl.cc:159: ServerWindow* capture_window =
event_dispatcher_.capture_window();
On 2016/01/22 00:55:00, sky wrote:
> Early out if window == capture_window?

Done.

https://codereview.chromium.org/1605773004/diff/100001/components/mus/ws/wind...
components/mus/ws/window_tree_host_impl.cc:164:
connection->ProcessLostCapture(capture_window);
With EventDispatcher now calling EventDispatcherDelegate::OnLostCapture this
would trigger a double notification to the WindowTreeImpl. Removing in favour of
the delegate method.

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

https://codereview.chromium.org/1605773004/diff/100001/components/mus/ws/wind...
components/mus/ws/window_tree_impl.cc:555:
client()->OnLostCapture(WindowIdToTransportId(old_capture_window->id()));
On 2016/01/22 00:55:00, sky wrote:
> You should only send this if it was not initiated by this client. By that I
mean
> if the client called ReleaseCapture() then don't send OnLostCapture.

Done.

https://codereview.chromium.org/1605773004/diff/100001/components/mus/ws/wind...
components/mus/ws/window_tree_impl.cc:922:
access_policy_->CanSetCapture(host->GetCaptureWindow());
> Generally we key off activation to dismiss on blur. Is there a place in chrome
> code where we setcapture as you're saying?

Yes: MenuController::Run eventually triggers
     MenuHost::ShowMenuHost which calls
     native_widget_private()->SetCapture()

The launch of the menu window is triggered off of an input, but the application
of capture is on a window which has not yet shown nor processed any input.

https://codereview.chromium.org/1605773004/diff/100001/components/mus/ws/wind...
components/mus/ws/window_tree_impl.cc:937: host->SetCapture(nullptr);
On 2016/01/22 00:55:00, sky wrote:
> They way you have this now is set capture to null no matter what. I think this
> should be set capture to null only if the window already has capture.

Done.

[sky, 2016-01-26 22:11:14]

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

https://codereview.chromium.org/1605773004/diff/100001/components/mus/ws/wind...
components/mus/ws/window_tree_impl.cc:922:
access_policy_->CanSetCapture(host->GetCaptureWindow());
On 2016/01/26 18:38:50, jonross wrote:
> > Generally we key off activation to dismiss on blur. Is there a place in
chrome
> > code where we setcapture as you're saying?
> 
> Yes: MenuController::Run eventually triggers
>      MenuHost::ShowMenuHost which calls
>      native_widget_private()->SetCapture()
> 
> The launch of the menu window is triggered off of an input, but the
application
> of capture is on a window which has not yet shown nor processed any input.

While the windows are different, it's the same client right? So that if you
restrict this api to only happen when the client is processing an event it
should work. Is that right?

[jonross, 2016-01-26 22:35:15]

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

https://codereview.chromium.org/1605773004/diff/100001/components/mus/ws/wind...
components/mus/ws/window_tree_impl.cc:922:
access_policy_->CanSetCapture(host->GetCaptureWindow());
On 2016/01/26 22:11:14, sky wrote:
> On 2016/01/26 18:38:50, jonross wrote:
> > > Generally we key off activation to dismiss on blur. Is there a place in
> chrome
> > > code where we setcapture as you're saying?
> > 
> > Yes: MenuController::Run eventually triggers
> >      MenuHost::ShowMenuHost which calls
> >      native_widget_private()->SetCapture()
> > 
> > The launch of the menu window is triggered off of an input, but the
> application
> > of capture is on a window which has not yet shown nor processed any input.
> 
> While the windows are different, it's the same client right? So that if you
> restrict this api to only happen when the client is processing an event it
> should work. Is that right?

It should always be the same client which has processed an event.

Though it might be possible for the event to have cleared before a request to
setcapture arrives.

Server
   - mouse press
   - mouse release
          - clears client from implicit capture
   - setcapture

Client
   - mouse press
         - call setcapture
   - mouse release

However the client itself should still have an event queue in that case. I'll
add that restriction to WindowTreeImpl

[jonross, 2016-01-27 21:15:13]

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

https://codereview.chromium.org/1605773004/diff/100001/components/mus/ws/wind...
components/mus/ws/window_tree_impl.cc:922:
access_policy_->CanSetCapture(host->GetCaptureWindow());
On 2016/01/26 22:35:15, jonross wrote:
> On 2016/01/26 22:11:14, sky wrote:
> > On 2016/01/26 18:38:50, jonross wrote:
> > > > Generally we key off activation to dismiss on blur. Is there a place in
> > chrome
> > > > code where we setcapture as you're saying?
> > > 
> > > Yes: MenuController::Run eventually triggers
> > >      MenuHost::ShowMenuHost which calls
> > >      native_widget_private()->SetCapture()
> > > 
> > > The launch of the menu window is triggered off of an input, but the
> > application
> > > of capture is on a window which has not yet shown nor processed any input.
> > 
> > While the windows are different, it's the same client right? So that if you
> > restrict this api to only happen when the client is processing an event it
> > should work. Is that right?
> 
> It should always be the same client which has processed an event.
> 
> Though it might be possible for the event to have cleared before a request to
> setcapture arrives.
> 
> Server
>    - mouse press
>    - mouse release
>           - clears client from implicit capture
>    - setcapture
> 
> Client
>    - mouse press
>          - call setcapture
>    - mouse release
> 
> However the client itself should still have an event queue in that case. I'll
> add that restriction to WindowTreeImpl

sadrul@ did raise a concern that we could have situations like this with context
menus. However he felt that could be updated in the future.

mus -> event -> browser -> renderer
    <- ack   <- 
                        <- renderer
    <-capture<-

Also would be avoided entirely once mus <-> renderer is possible

[jonross, 2016-01-29 17:00:50]

Updated so that an active event is required for requesting capture to succeed.

Also rebased in latest patch.

PTAL

[sky, 2016-01-29 20:47:00]

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

https://codereview.chromium.org/1605773004/diff/160001/components/mus/public/...
components/mus/public/interfaces/window_tree.mojom:112: SetCapture(uint32
change_id, uint32 window_id);
Document capture is only allowed if the window is processing an event.

https://codereview.chromium.org/1605773004/diff/160001/components/mus/ws/disp...
File components/mus/ws/display_manager_delegate.h (right):

https://codereview.chromium.org/1605773004/diff/160001/components/mus/ws/disp...
components/mus/ws/display_manager_delegate.h:39: // Claled whne the Display
loses capture.
called when

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

https://codereview.chromium.org/1605773004/diff/160001/components/mus/ws/even...
components/mus/ws/event_dispatcher.cc:202: } else if
(unobserved_windows.insert(target).second) {
Why do you only send the cancel once per window? If multiple pointer ids are
down shouldn't we send a cancel per id?

I think this code would be less error prone if you called StopTrackingPointer
for each item. That way you don't need unobserved_windows.

https://codereview.chromium.org/1605773004/diff/160001/components/mus/ws/even...
components/mus/ws/event_dispatcher.cc:209: // TODO(jonross): Do we want to track
previous location in PointerTarger
PointerTarger->PointerTarget
And to your TODO, I think we should. I would be inclined to store the last
location in PointerTarget. But you can do that later.

https://codereview.chromium.org/1605773004/diff/160001/components/mus/ws/even...
components/mus/ws/event_dispatcher.cc:291: return;
The problem with an early return here is mouse_pointer_last_location and
mouse_cursor_source_window_ are not updated correctly.

https://codereview.chromium.org/1605773004/diff/160001/components/mus/ws/even...
components/mus/ws/event_dispatcher.cc:443: if (capture_window_ == window) {
When this happens you may need to update the cursor. See 327-337.

https://codereview.chromium.org/1605773004/diff/160001/components/mus/ws/wind...
File components/mus/ws/window_manager_access_policy.cc (right):

https://codereview.chromium.org/1605773004/diff/160001/components/mus/ws/wind...
components/mus/ws/window_manager_access_policy.cc:108: return true;
Actually, you should only allow this if the WM created the window (same as that
on 103). We could allow the wm to grab capture anywhere, but the way the code is
the events would still be routed to the owner.

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

https://codereview.chromium.org/1605773004/diff/160001/components/mus/ws/wind...
components/mus/ws/window_tree_impl.cc:993: ServerWindow* window =
GetWindow(WindowIdFromTransportId(window_id));
Sorry, I've changed things around. You want:

window = GetWindowByClientId(ClientWindowId(window_id));

https://codereview.chromium.org/1605773004/diff/160001/components/mus/ws/wind...
components/mus/ws/window_tree_impl.cc:999:
access_policy_->CanSetCapture(current_capture_window)) &&
Can the condition in () ever hit? Why do we need it?

https://codereview.chromium.org/1605773004/diff/160001/components/mus/ws/wind...
components/mus/ws/window_tree_impl.cc:1009: ServerWindow* window =
GetWindow(WindowIdFromTransportId(window_id));
Same comment about ids.

[jonross, 2016-02-01 18:52:22]

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

https://codereview.chromium.org/1605773004/diff/160001/components/mus/public/...
components/mus/public/interfaces/window_tree.mojom:112: SetCapture(uint32
change_id, uint32 window_id);
On 2016/01/29 20:47:00, sky wrote:
> Document capture is only allowed if the window is processing an event.

Done.

https://codereview.chromium.org/1605773004/diff/160001/components/mus/ws/disp...
File components/mus/ws/display_manager_delegate.h (right):

https://codereview.chromium.org/1605773004/diff/160001/components/mus/ws/disp...
components/mus/ws/display_manager_delegate.h:39: // Claled whne the Display
loses capture.
On 2016/01/29 20:47:00, sky wrote:
> called when

Done.

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

https://codereview.chromium.org/1605773004/diff/160001/components/mus/ws/even...
components/mus/ws/event_dispatcher.cc:202: } else if
(unobserved_windows.insert(target).second) {
On 2016/01/29 20:47:00, sky wrote:
> Why do you only send the cancel once per window? If multiple pointer ids are
> down shouldn't we send a cancel per id?
> 
> I think this code would be less error prone if you called StopTrackingPointer
> for each item. That way you don't need unobserved_windows.

Good catch, I've updated this to send once cancel per window id. Updated the
tests to confirm this.

I am not using StopTrackingPointer here as it directly removes from the map.
I've cleaned this up, so that observing_window is no longer needed.

https://codereview.chromium.org/1605773004/diff/160001/components/mus/ws/even...
components/mus/ws/event_dispatcher.cc:209: // TODO(jonross): Do we want to track
previous location in PointerTarger
On 2016/01/29 20:47:00, sky wrote:
> PointerTarger->PointerTarget
> And to your TODO, I think we should. I would be inclined to store the last
> location in PointerTarget. But you can do that later.

Fixed the comment.

I agree with storing the last location in PointerTarget, I'll do that in a
followup

https://codereview.chromium.org/1605773004/diff/160001/components/mus/ws/even...
components/mus/ws/event_dispatcher.cc:291: return;
On 2016/01/29 20:47:00, sky wrote:
> The problem with an early return here is mouse_pointer_last_location and
> mouse_cursor_source_window_ are not updated correctly.

I've moved this, and added logic for updating mouse_cursor_source_window_ and
mouse_button_down_.

This does ignore the window cursor updating on line 335. We will maintain the
captured window cursor until capture is released. CancelPointerEventsToTarget
will be updated to perform this.

https://codereview.chromium.org/1605773004/diff/160001/components/mus/ws/even...
components/mus/ws/event_dispatcher.cc:443: if (capture_window_ == window) {
On 2016/01/29 20:47:00, sky wrote:
> When this happens you may need to update the cursor. See 327-337.

Done.

https://codereview.chromium.org/1605773004/diff/160001/components/mus/ws/wind...
File components/mus/ws/window_manager_access_policy.cc (right):

https://codereview.chromium.org/1605773004/diff/160001/components/mus/ws/wind...
components/mus/ws/window_manager_access_policy.cc:108: return true;
On 2016/01/29 20:47:00, sky wrote:
> Actually, you should only allow this if the WM created the window (same as
that
> on 103). We could allow the wm to grab capture anywhere, but the way the code
is
> the events would still be routed to the owner.

Done.

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

https://codereview.chromium.org/1605773004/diff/160001/components/mus/ws/wind...
components/mus/ws/window_tree_impl.cc:993: ServerWindow* window =
GetWindow(WindowIdFromTransportId(window_id));
On 2016/01/29 20:47:00, sky wrote:
> Sorry, I've changed things around. You want:
> 
> window = GetWindowByClientId(ClientWindowId(window_id));

Done.

https://codereview.chromium.org/1605773004/diff/160001/components/mus/ws/wind...
components/mus/ws/window_tree_impl.cc:999:
access_policy_->CanSetCapture(current_capture_window)) &&
On 2016/01/29 20:47:00, sky wrote:
> Can the condition in () ever hit? Why do we need it?

Yes this can be hit.

When a window tree has been created, and all capture is currently implicit
within EventDispatcher. This leads to the |current_capture_window| being null.

On subsequent runs the second portion should always be true, unless an access
policy can change at runtime.

https://codereview.chromium.org/1605773004/diff/160001/components/mus/ws/wind...
components/mus/ws/window_tree_impl.cc:1009: ServerWindow* window =
GetWindow(WindowIdFromTransportId(window_id));
On 2016/01/29 20:47:00, sky wrote:
> Same comment about ids.

Done.

[sky, 2016-02-01 20:43:33]

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

https://codereview.chromium.org/1605773004/diff/180001/components/mus/public/...
components/mus/public/cpp/lib/window_tree_client_impl.cc:251: new
CrashInFlightChange(window, ChangeType::SET_CAPTURE)));
We need to handle requests for capture failing and not crash.

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

https://codereview.chromium.org/1605773004/diff/180001/components/mus/ws/even...
components/mus/ws/event_dispatcher.cc:294: mouse_button_down_ = event->action ==
mojom::EventType::POINTER_DOWN;
This isn't right. For setting to true you want to duplicate the logic around
311, and for setting to false duplicate the logic around 321-334.

[jonross, 2016-02-03 18:51:00]

Patchset #10 (id:200001) has been deleted

[jonross, 2016-02-03 18:52:17]

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

https://codereview.chromium.org/1605773004/diff/180001/components/mus/public/...
components/mus/public/cpp/lib/window_tree_client_impl.cc:251: new
CrashInFlightChange(window, ChangeType::SET_CAPTURE)));
On 2016/02/01 20:43:33, sky wrote:
> We need to handle requests for capture failing and not crash.

Done.

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

https://codereview.chromium.org/1605773004/diff/180001/components/mus/ws/even...
components/mus/ws/event_dispatcher.cc:294: mouse_button_down_ = event->action ==
mojom::EventType::POINTER_DOWN;
On 2016/02/01 20:43:33, sky wrote:
> This isn't right. For setting to true you want to duplicate the logic around
> 311, and for setting to false duplicate the logic around 321-334.

Yeah I was mistaken here. Done.

[sky, 2016-02-03 21:59:01]

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

https://codereview.chromium.org/1605773004/diff/220001/components/mus/public/...
components/mus/public/cpp/lib/window_tree_client_impl.cc:251:
make_scoped_ptr(new InFlightCaptureChange(window, false)));
false (and true on 260) should come from the capture state of the window.

https://codereview.chromium.org/1605773004/diff/220001/components/mus/public/...
components/mus/public/cpp/lib/window_tree_client_impl.cc:577: if (window) {
This potentially needs to update the capture state of the window. See
OnWindowBoundsChanged for an example of what you need to do.

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

https://codereview.chromium.org/1605773004/diff/220001/components/mus/ws/even...
components/mus/ws/event_dispatcher.cc:338:
UpdateCursorProviderByLastKnownLocation();
You should do this in SetCapture(null) if mouse_button_down_ is false.

[jonross, 2016-02-05 15:24:26]

On 2016/02/03 21:59:01, sky wrote:
>
https://codereview.chromium.org/1605773004/diff/220001/components/mus/public/...
> File components/mus/public/cpp/lib/window_tree_client_impl.cc (right):
> 
>
https://codereview.chromium.org/1605773004/diff/220001/components/mus/public/...
> components/mus/public/cpp/lib/window_tree_client_impl.cc:251:
> make_scoped_ptr(new InFlightCaptureChange(window, false)));
> false (and true on 260) should come from the capture state of the window.
> 
>
https://codereview.chromium.org/1605773004/diff/220001/components/mus/public/...
> components/mus/public/cpp/lib/window_tree_client_impl.cc:577: if (window) {
> This potentially needs to update the capture state of the window. See
> OnWindowBoundsChanged for an example of what you need to do.
> 
>
https://codereview.chromium.org/1605773004/diff/220001/components/mus/ws/even...
> File components/mus/ws/event_dispatcher.cc (right):
> 
>
https://codereview.chromium.org/1605773004/diff/220001/components/mus/ws/even...
> components/mus/ws/event_dispatcher.cc:338:
> UpdateCursorProviderByLastKnownLocation();
> You should do this in SetCapture(null) if mouse_button_down_ is false.

This review is being split into both a window server implementation, as well as
client lib.

The review for the window server can be found here:
https://codereview.chromium.org/1677513002/
The client lib will follow.

I'm closing this review.