views: Reset the capture-view at the end of a gesture.

views: Reset the capture-view at the end of a gesture.

If the gesture handler was capturing events, then stop capturing at the end of
the gesture sequence. This fixes selecting tabs with mouse-clicks after taps.

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

[DaveMoore, 2012-09-24 22:16:03]

lgtm

Verified on device.

[sadrul, 2012-09-24 22:21:03]

+sky for OWNERS

[sky, 2012-09-24 22:26:58]

How come Widget::OnGestureEvent doesn't end up releasing capture?

[sadrul, 2012-09-24 22:34:46]

On 2012/09/24 22:26:58, sky wrote:
> How come Widget::OnGestureEvent doesn't end up releasing capture?

Widget::OnGestureEvent does a ReleaseCapture, however ReleaseCapture simply
resets the native-capture, it does not reset the 'capture view'.

[sadrul, 2012-09-24 23:09:50]

[ I have added a browser-test for the tabstrip ]

[sky, 2012-09-24 23:12:50]

How come we explicitly have to do this for capture but not for mouse events?
Shouldn't Widget/RootView being resetting state appropriately?

[sadrul, 2012-09-24 23:15:50]

On 2012/09/24 23:12:50, sky wrote:
> How come we explicitly have to do this for capture but not for mouse events?
> Shouldn't Widget/RootView being resetting state appropriately?

We deliberately do not set capture on gesture events (see comment in lines
tab_strip.cc:1062+).

(I believe we have discussed this issue before; one of the reasons we don't
always set capture on gesture-events is to allow doing simultaneous gestures in
multiple target windows)

[sky, 2012-09-24 23:22:26]

On Mon, Sep 24, 2012 at 4:15 PM,  <sadrul@chromium.org> wrote:
> On 2012/09/24 23:12:50, sky wrote:
>>
>> How come we explicitly have to do this for capture but not for mouse
>> events?
>> Shouldn't Widget/RootView being resetting state appropriately?
>
>
> We deliberately do not set capture on gesture events (see comment in lines
> tab_strip.cc:1062+).

That I get. My questions is shouldn't we clear out state on releasing
capture though?

> (I believe we have discussed this issue before; one of the reasons we don't
> always set capture on gesture-events is to allow doing simultaneous gestures
> in
> multiple target windows)
>
> http://codereview.chromium.org/10983008/

[sadrul, 2012-09-24 23:45:19]

On 2012/09/24 23:22:26, sky wrote:
> On Mon, Sep 24, 2012 at 4:15 PM,  <mailto:sadrul@chromium.org> wrote:
> > On 2012/09/24 23:12:50, sky wrote:
> >>
> >> How come we explicitly have to do this for capture but not for mouse
> >> events?
> >> Shouldn't Widget/RootView being resetting state appropriately?
> >
> >
> > We deliberately do not set capture on gesture events (see comment in lines
> > tab_strip.cc:1062+).
> 
> That I get. My questions is shouldn't we clear out state on releasing
> capture though?

Ah, I misunderstood. You mean calling RootView::SetMouseHandler from
Widget::ReleaseCapture? I think that makes sense (and makes
RootView::SetCapture(NULL) more equivalent to RootView::ReleaseCapture).

[sky, 2012-09-25 00:08:46]

On Mon, Sep 24, 2012 at 4:45 PM,  <sadrul@chromium.org> wrote:
> On 2012/09/24 23:22:26, sky wrote:
>
>> On Mon, Sep 24, 2012 at 4:15 PM,  <mailto:sadrul@chromium.org> wrote:
>> > On 2012/09/24 23:12:50, sky wrote:
>> >>
>> >> How come we explicitly have to do this for capture but not for mouse
>> >> events?
>> >> Shouldn't Widget/RootView being resetting state appropriately?
>> >
>> >
>> > We deliberately do not set capture on gesture events (see comment in
>> > lines
>> > tab_strip.cc:1062+).
>
>
>> That I get. My questions is shouldn't we clear out state on releasing
>> capture though?
>
>
> Ah, I misunderstood. You mean calling RootView::SetMouseHandler from
> Widget::ReleaseCapture? I think that makes sense (and makes
> RootView::SetCapture(NULL) more equivalent to RootView::ReleaseCapture).

Not sure.

I was hoping the sequence would be the same as for the mouse. By that
I mean when the mouse is released RootView::OnMouseReleased resets the
mouse handler (and gesture handler). On a gesture end
RootView::OnGestureEvent resets the gesture_handler_ only. Maybe
RootView::OnGestureEvent needs a boolean indicating that we just
released capture (only set to true if we actually had capture) so that
RootView::OnGestureEvent knows we just released capture and should do
SetMouseHandler(NULL) instead of gesture_handler_ = NULL. Would that
work? If the boolean is false it just resets gesture_handler_.

  -Scott

[sadrul, 2012-09-25 14:51:04]

On 2012/09/25 00:08:46, sky wrote:
> On Mon, Sep 24, 2012 at 4:45 PM,  <mailto:sadrul@chromium.org> wrote:
> > On 2012/09/24 23:22:26, sky wrote:
> >
> >> On Mon, Sep 24, 2012 at 4:15 PM,  <mailto:sadrul@chromium.org> wrote:
> >> > On 2012/09/24 23:12:50, sky wrote:
> >> >>
> >> >> How come we explicitly have to do this for capture but not for mouse
> >> >> events?
> >> >> Shouldn't Widget/RootView being resetting state appropriately?
> >> >
> >> >
> >> > We deliberately do not set capture on gesture events (see comment in
> >> > lines
> >> > tab_strip.cc:1062+).
> >
> >
> >> That I get. My questions is shouldn't we clear out state on releasing
> >> capture though?
> >
> >
> > Ah, I misunderstood. You mean calling RootView::SetMouseHandler from
> > Widget::ReleaseCapture? I think that makes sense (and makes
> > RootView::SetCapture(NULL) more equivalent to RootView::ReleaseCapture).
> 
> Not sure.
> 
> I was hoping the sequence would be the same as for the mouse. By that
> I mean when the mouse is released RootView::OnMouseReleased resets the
> mouse handler (and gesture handler). On a gesture end
> RootView::OnGestureEvent resets the gesture_handler_ only. Maybe
> RootView::OnGestureEvent needs a boolean indicating that we just
> released capture (only set to true if we actually had capture) so that
> RootView::OnGestureEvent knows we just released capture and should do
> SetMouseHandler(NULL) instead of gesture_handler_ = NULL. Would that
> work? If the boolean is false it just resets gesture_handler_.

I have changed the code in RootView to do this (not exactly what you propose,
but essentially the same). PTAL

[sky, 2012-09-25 16:10:24]

This looks fine to me. Since the fix is in rootview can we add a unit test to
views for coverage and nuke the tab one?

[sadrul, 2012-09-25 17:42:24]

On 2012/09/25 16:10:24, sky wrote:
> This looks fine to me. Since the fix is in rootview can we add a unit test to
> views for coverage and nuke the tab one?

Done (I would've kept the test for tabstrip since I was planning to add some
more dragging related tests there, but that can be done separately).

PTAL.

[sky, 2012-09-25 18:26:54]

LGTM

There are a bunch of drag related tests in
tab_drag_controller_interactive_uitest*

[sadrul, 2012-09-25 18:28:04]

On 2012/09/25 18:26:54, sky wrote:
> LGTM
> 
> There are a bunch of drag related tests in
> tab_drag_controller_interactive_uitest*

Oh, ha. Cool! I hadn't noticed those. Thanks!