Set capture to the window being dragged when dragging a window on Ash

Set capture to the window being dragged when dragging a window on Ash

This CL sets capture to the window being dragged when dragging a window on
Ash. This allows state to be reset when a user opens the Ctrl+Alt+Delete dialog
on Windows Ash.

As a side effect, this CL fixes dragging windows from one screen to another on
ChromeOS ozone. Dragging windows from one screen to another
worked on X11 because X11 does an implicit grab when the mouse is pressed. This
implicit grab guarantees that events are sent to WindowTreeHost where the drag
started for the duration of the drag.

BUG=439703, 423383
TEST=None

Committed: https://crrev.com/4882310f75d0a578c068b92dc2eaf163d761c03a
Cr-Commit-Position: refs/heads/master@{#309364}

[pkotwicz, 2014-12-08 20:02:08]

pkotwicz@chromium.org changed reviewers:
+ lionel.g.landwerlin@intel.com

[pkotwicz, 2014-12-08 20:05:18]

lionel@ PTAL

https://codereview.chromium.org/778043004/diff/1/ui/aura/window_tree_host_ozo...
File ui/aura/window_tree_host_ozone.cc (right):

https://codereview.chromium.org/778043004/diff/1/ui/aura/window_tree_host_ozo...
ui/aura/window_tree_host_ozone.cc:89: //platform_window_->SetCapture();
I have commented this out to demonstrate that native capture is unnecessary. I
do not plan on landing the change to this file. I do plan on landing the change
to toplevel_window_event_handler.cc

https://codereview.chromium.org/780273003/ is necessary if native capture is
enabled.

[llandwerlin-old, 2014-12-10 15:40:42]

On 2014/12/08 20:05:18, pkotwicz wrote:
> lionel@ PTAL
> 
>
https://codereview.chromium.org/778043004/diff/1/ui/aura/window_tree_host_ozo...
> File ui/aura/window_tree_host_ozone.cc (right):
> 
>
https://codereview.chromium.org/778043004/diff/1/ui/aura/window_tree_host_ozo...
> ui/aura/window_tree_host_ozone.cc:89: //platform_window_->SetCapture();
> I have commented this out to demonstrate that native capture is unnecessary. I
> do not plan on landing the change to this file. I do plan on landing the
change
> to toplevel_window_event_handler.cc
> 
> https://codereview.chromium.org/780273003/ is necessary if native capture is
> enabled.

Sorry, for the delay, I just tried your 2 CLs and it seems to work with one
little problem, once you start dragging from one screen to the other, if you try
to move the window back to the original screen, the mouse pointer is kind of
blocked on the edge of the screen.

[pkotwicz, 2014-12-11 01:48:42]

llandwerlin@ can you please take another look?

Thanks for noticing the problem. I investigated and found the cause.
https://codereview.chromium.org/695153003/ makes it possible to drag the window
back to the original screen (without doing native capture)

In Patch Set #2, I removed the change to ui/aura/window_tree_host_ozone.cc (So
we do native capture) This allows dragging back to the original screen.

[llandwerlin-old, 2014-12-13 15:44:05]

On 2014/12/11 01:48:42, pkotwicz wrote:
> llandwerlin@ can you please take another look?
> 
> Thanks for noticing the problem. I investigated and found the cause.
> https://codereview.chromium.org/695153003/ makes it possible to drag the
window
> back to the original screen (without doing native capture)
> 
> In Patch Set #2, I removed the change to ui/aura/window_tree_host_ozone.cc (So
> we do native capture) This allows dragging back to the original screen.

Just tested patchset 2 and it works correctly now.
Could you add bug 423383 to this CL? So it can be closed with your changes.
Thanks!

[pkotwicz, 2014-12-16 17:03:18]

pkotwicz@chromium.org changed reviewers:
+ flackr@chromium.org

[pkotwicz, 2014-12-16 17:03:18]

flackr@ can you please take a look?

[flackr, 2014-12-16 21:28:21]

https://codereview.chromium.org/778043004/diff/20001/ash/wm/toplevel_window_e...
File ash/wm/toplevel_window_event_handler.cc (right):

https://codereview.chromium.org/778043004/diff/20001/ash/wm/toplevel_window_e...
ash/wm/toplevel_window_event_handler.cc:129: if (!target->HasCapture()) {
If this is to accomodate X11 already having capture, can we use #if
!defined(USE_X11) instead?

Alternately we could just capture again on X11 assuming it will be a noop.

https://codereview.chromium.org/778043004/diff/20001/ash/wm/toplevel_window_e...
ash/wm/toplevel_window_event_handler.cc:131: target->SetCapture();
Maybe we should also pay attention to capture loss and stop the drag in such an
event?

[pkotwicz, 2014-12-17 04:48:12]

Rob, can you please take another look?

https://codereview.chromium.org/778043004/diff/20001/ash/wm/toplevel_window_e...
File ash/wm/toplevel_window_event_handler.cc (right):

https://codereview.chromium.org/778043004/diff/20001/ash/wm/toplevel_window_e...
ash/wm/toplevel_window_event_handler.cc:129: if (!target->HasCapture()) {
The reason that I do this is because I want to enforce:
"If ScopedWindowResizer grabbed capture, it should release capture."
I think that things are more straightforward this way.

In particular, I want what happens with capture in this scenario to be clear:
- Click on tab
- Drag tab
- Press escape

https://codereview.chromium.org/778043004/diff/20001/ash/wm/toplevel_window_e...
ash/wm/toplevel_window_event_handler.cc:131: target->SetCapture();
This would be bad. The tab dragging code switches which aura::Window has capture
prior to ending the drag. See TabDragController::DragBrowserToNewTabStrip()

[pkotwicz, 2014-12-17 05:18:36]

https://codereview.chromium.org/778043004/diff/20001/ash/wm/toplevel_window_e...
File ash/wm/toplevel_window_event_handler.cc (right):

https://codereview.chromium.org/778043004/diff/20001/ash/wm/toplevel_window_e...
ash/wm/toplevel_window_event_handler.cc:131: target->SetCapture();
Actually, we already pay attention to loss of capture.
This is why the tab drag is aborted when you use accelerator keys to take a
partial screenshot.

[flackr, 2014-12-17 19:39:15]

lgtm

https://codereview.chromium.org/778043004/diff/20001/ash/wm/toplevel_window_e...
File ash/wm/toplevel_window_event_handler.cc (right):

https://codereview.chromium.org/778043004/diff/20001/ash/wm/toplevel_window_e...
ash/wm/toplevel_window_event_handler.cc:129: if (!target->HasCapture()) {
On 2014/12/17 04:48:12, pkotwicz wrote:
> The reason that I do this is because I want to enforce:
> "If ScopedWindowResizer grabbed capture, it should release capture."
> I think that things are more straightforward this way.
> 
> In particular, I want what happens with capture in this scenario to be clear:
> - Click on tab
> - Drag tab
> - Press escape

Acknowledged.

https://codereview.chromium.org/778043004/diff/20001/ash/wm/toplevel_window_e...
ash/wm/toplevel_window_event_handler.cc:131: target->SetCapture();
On 2014/12/17 05:18:36, pkotwicz wrote:
> Actually, we already pay attention to loss of capture.
> This is why the tab drag is aborted when you use accelerator keys to take a
> partial screenshot.

Acknowledged, my main concern was losing capture to another application in
windows (or as you mentioned the partial screenshot shortcut). As long as it's
handled looks good.

[pkotwicz, 2014-12-17 19:59:13]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-12-17 20:00:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/778043004/20001

[commit-bot: I haz the power, 2014-12-17 20:53:43]

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

[commit-bot: I haz the power, 2014-12-17 20:53:44]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pkotwicz, 2014-12-19 00:31:55]

Patchset #3 (id:40001) has been deleted

[pkotwicz, 2014-12-19 00:38:26]

Sadrul, can you please take a look at the deletion of
SystemGestureEventFilterTest.TwoFingerDragTwoWindows

I deleted the test because adding a third finger during a two finger drag now
always does the same thing regardless of where the third finger is. (It also now
does the same thing as adding a third finger while dragging a tab.)

[pkotwicz, 2014-12-19 00:38:42]

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

[sadrul, 2014-12-19 02:22:38]

On 2014/12/19 00:38:26, pkotwicz wrote:
> Sadrul, can you please take a look at the deletion of
> SystemGestureEventFilterTest.TwoFingerDragTwoWindows
> 
> I deleted the test because adding a third finger during a two finger drag now
> always does the same thing regardless of where the third finger is. (It also
now
> does the same thing as adding a third finger while dragging a tab.)

Can you clarify what the behaviour is? Do we have test coverage for that
behaviour? If we do, LGTM. Otherwise, can you add a test?

[pkotwicz, 2014-12-20 20:35:05]

With this CL, when a third finger is added anywhere on screen while a window is
dragged via touch the drag operation is completed / stopped. Without this CL,
adding a third finger on the window frame completed / stopped the drag. A third
finger elsewhere had different behavior.

With or without this CL, when dragging a tab via tuoch, adding a third finger
anywhere on screen completes / stops the tab drag.

SystemGestureEventFilterTest.ThreeFingerGestureStopsDrag tests that adding a
third finger stops the drag

[pkotwicz, 2014-12-20 20:35:07]

With this CL, when a third finger is added anywhere on screen while a window is
dragged via touch the drag operation is completed / stopped. Without this CL,
adding a third finger on the window frame completed / stopped the drag. A third
finger elsewhere had different behavior.

With or without this CL, when dragging a tab via tuoch, adding a third finger
anywhere on screen completes / stops the tab drag.

SystemGestureEventFilterTest.ThreeFingerGestureStopsDrag tests that adding a
third finger stops the drag

[pkotwicz, 2014-12-20 20:35:10]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-12-20 20:35:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/778043004/60001

[commit-bot: I haz the power, 2014-12-20 22:10:46]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2014-12-20 22:11:39]

Patchset 3 (id:??) landed as
https://crrev.com/4882310f75d0a578c068b92dc2eaf163d761c03a
Cr-Commit-Position: refs/heads/master@{#309364}