Avoids releasing capture in EndMoveLoop if capture was switched to another window while dragging

Avoids releasing capture in EndMoveLoop if capture was switched to another window while dragging.
This variant relies on checking if a drag input window still has capture before releasing the grab.
A better alternative is in CL https://codereview.chromium.org/262893002/.

BUG=363503
TEST=Follow the exact steps in the bug description.
TEST=interactive_ui_tests --gtest_filter=*TabDragging*

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269593

[varkha, 2014-05-02 20:26:49]

sadrul@, can you please take a look? I would prefer the approach in
https://codereview.chromium.org/262893002/ but it needs more work and more
understanding. In the meanwhile for M36 this could allow tab dragging to work
correctly.

[sadrul, 2014-05-05 19:43:32]

What is our testing strategy for changes like this?

https://codereview.chromium.org/265843004/diff/1/ui/views/widget/desktop_aura...
File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc (right):

https://codereview.chromium.org/265843004/diff/1/ui/views/widget/desktop_aura...
ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:139: }
Can we look at the FocusOut event received here to keep track of whether the
capture moved away from the drag-window?

https://codereview.chromium.org/265843004/diff/1/ui/views/widget/desktop_aura...
ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:260:
XUngrabKeyboard(display, CurrentTime);
I assume we wouldn't want to do this either?

[varkha, 2014-05-06 22:22:12]

Do you think this should be abandoned in favour of 
https://codereview.chromium.org/262893002/? I tried to make the behavior right
in M35, if the change is not trivial then this may not have enough merits on its
own. How do you feel about https://codereview.chromium.org/262893002/?

Test coverage should be added to more scenarios that involve tab dragging such
as drag-visit_another_tabstrip-drag_more. I prefer to do that as a part of
https://codereview.chromium.org/262893002/ or maybe separately.

https://codereview.chromium.org/265843004/diff/1/ui/views/widget/desktop_aura...
File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc (right):

https://codereview.chromium.org/265843004/diff/1/ui/views/widget/desktop_aura...
ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:139: }
On 2014/05/05 19:43:33, sadrul wrote:
> Can we look at the FocusOut event received here to keep track of whether the
> capture moved away from the drag-window?

Maybe, I was trying though to make this CL simple enough to merge in M35. If you
are looking for the "right thing" then I think
https://codereview.chromium.org/262893002/ is it.

https://codereview.chromium.org/265843004/diff/1/ui/views/widget/desktop_aura...
ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:260:
XUngrabKeyboard(display, CurrentTime);
On 2014/05/05 19:43:33, sadrul wrote:
> I assume we wouldn't want to do this either?

Since we still grab the keyboard in this class we also need to ungrab it.
Perhaps we should not grab it either but I wanted this CL to be trivial enough
to possibly be merged in M35.

[sadrul, 2014-05-07 14:10:45]

On 2014/05/06 22:22:12, varkha wrote:
> Do you think this should be abandoned in favour of 
> https://codereview.chromium.org/262893002/ I tried to make the behavior right
> in M35, if the change is not trivial then this may not have enough merits on
its
> own. How do you feel about https://codereview.chromium.org/262893002/

I will take a look.

> 
> Test coverage should be added to more scenarios that involve tab dragging such
> as drag-visit_another_tabstrip-drag_more. I prefer to do that as a part of
> https://codereview.chromium.org/262893002/ or maybe separately.

OK. But I think we need to have coverage for non-tab drags too.

https://codereview.chromium.org/265843004/diff/1/ui/views/widget/desktop_aura...
File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc (right):

https://codereview.chromium.org/265843004/diff/1/ui/views/widget/desktop_aura...
ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:139: }
On 2014/05/06 22:22:12, varkha wrote:
> On 2014/05/05 19:43:33, sadrul wrote:
> > Can we look at the FocusOut event received here to keep track of whether the
> > capture moved away from the drag-window?
> 
> Maybe, I was trying though to make this CL simple enough to merge in M35. If
you
> are looking for the "right thing" then I think
> https://codereview.chromium.org/262893002/ is it.

I think keeping track of FocusOut in here would be simpler (the fix would be
entirely contained within this file)

https://codereview.chromium.org/265843004/diff/1/ui/views/widget/desktop_aura...
ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:260:
XUngrabKeyboard(display, CurrentTime);
On 2014/05/06 22:22:12, varkha wrote:
> On 2014/05/05 19:43:33, sadrul wrote:
> > I assume we wouldn't want to do this either?
> 
> Since we still grab the keyboard in this class we also need to ungrab it.
> Perhaps we should not grab it either but I wanted this CL to be trivial enough
> to possibly be merged in M35.

I meant perhaps we don't want to ungrab the keyboard if we didn't have to ungrab
the pointer.

[varkha, 2014-05-07 20:17:53]

PTAL. My preference is still to land https://codereview.chromium.org/262893002/
instead.

https://codereview.chromium.org/265843004/diff/1/ui/views/widget/desktop_aura...
File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc (right):

https://codereview.chromium.org/265843004/diff/1/ui/views/widget/desktop_aura...
ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:139: }
On 2014/05/07 14:10:46, sadrul wrote:
> On 2014/05/06 22:22:12, varkha wrote:
> > On 2014/05/05 19:43:33, sadrul wrote:
> > > Can we look at the FocusOut event received here to keep track of whether
the
> > > capture moved away from the drag-window?
> > 
> > Maybe, I was trying though to make this CL simple enough to merge in M35. If
> you
> > are looking for the "right thing" then I think
> > https://codereview.chromium.org/262893002/ is it.
> 
> I think keeping track of FocusOut in here would be simpler (the fix would be
> entirely contained within this file)

Yes, I think I now get what you mean.

https://codereview.chromium.org/265843004/diff/1/ui/views/widget/desktop_aura...
ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:260:
XUngrabKeyboard(display, CurrentTime);
On 2014/05/07 14:10:46, sadrul wrote:
> On 2014/05/06 22:22:12, varkha wrote:
> > On 2014/05/05 19:43:33, sadrul wrote:
> > > I assume we wouldn't want to do this either?
> > 
> > Since we still grab the keyboard in this class we also need to ungrab it.
> > Perhaps we should not grab it either but I wanted this CL to be trivial
enough
> > to possibly be merged in M35.
> 
> I meant perhaps we don't want to ungrab the keyboard if we didn't have to
ungrab
> the pointer.

Done.

[varkha, 2014-05-09 15:12:38]

Ping - this is still a backup plan, hope this can land and still get in M35.

[sadrul, 2014-05-09 15:23:10]

lgtm

[varkha, 2014-05-09 15:45:47]

The CQ bit was checked by varkha@chromium.org

[commit-bot: I haz the power, 2014-05-09 15:49:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/varkha@chromium.org/265843004/40001

[commit-bot: I haz the power, 2014-05-10 18:23:51]

Change committed as 269593

[mdempsky, 2014-05-10 20:17:26]

On 2014/05/10 18:23:51, I haz the power (commit-bot) wrote:
> Change committed as 269593

Is it possible this change is responsible for ash_unittests failing on Linux
ChromiumOS Tests(1)?

http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2...

ash_unittests ash_unittests ash_unittests 14 disabled 5 flaky failed 5 ( 1 mins,
31 secs )
stdio
ThreeFingerGestureStopsDrag_0
ThreeFingerGestureStopsDrag_1
TwoFingerDragDelayed_0
TwoFingerDragDelayed_1
MoveWindowWithTransient

[varkha, 2014-05-10 20:28:24]

Not really. This change only affects files that get linked into a Linux /
X11 build (not Chrome OS).
On May 10, 2014 4:17 PM, <mdempsky@chromium.org> wrote:

> On 2014/05/10 18:23:51, I haz the power (commit-bot) wrote:
>
>> Change committed as 269593
>>
>
> Is it possible this change is responsible for ash_unittests failing on
> Linux
> ChromiumOS Tests(1)?
>
> http://build.chromium.org/p/chromium.chromiumos/builders/
> Linux%20ChromiumOS%20Tests%20%281%29/builds/43114
>
> ash_unittests ash_unittests ash_unittests 14 disabled 5 flaky failed 5 ( 1
> mins,
> 31 secs )
> stdio
> ThreeFingerGestureStopsDrag_0
> ThreeFingerGestureStopsDrag_1
> TwoFingerDragDelayed_0
> TwoFingerDragDelayed_1
> MoveWindowWithTransient
>
> https://codereview.chromium.org/265843004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mdempsky, 2014-05-10 20:29:06]

A revert of this CL has been created in
https://codereview.chromium.org/276163002/ by mdempsky@chromium.org.

The reason for reverting is: Appears to be responsible for ash_unittests failing
on Linux ChromiumOS Tests(1):
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2....