Support targeting for gestures in ViewTargeter

Support targeting for gestures in ViewTargeter

Move the gesture event targeting logic from
RootView::DispatchGestureEvent() into ViewTargeter's
overrides of FindTargetForEvent() and
FindNextBestTarget().

BUG=404224
TEST=ViewTargeterTest.ViewTargeterForGestureEvents

Committed: https://crrev.com/a7b4a883cc904ecda130e0a8d91204f3770f2fae
Cr-Commit-Position: refs/heads/master@{#292321}

[tdanderson, 2014-08-15 22:42:47]

Sadrul, what do you think about landing this in early M39 (once I add a couple
of tests for ViewTargeter)?

[tdanderson, 2014-08-16 20:34:23]

Sadrul, I have added a test and changed this from WIP to ready for formal
review. Please see my inline comment in PS2 and let me know what you think.

https://codereview.chromium.org/481433006/diff/20001/ui/views/widget/root_view.h
File ui/views/widget/root_view.h (right):

https://codereview.chromium.org/481433006/diff/20001/ui/views/widget/root_vie...
ui/views/widget/root_view.h:199: bool dispatch_to_gesture_handler_;
I admit that introducing this member does feel a bit clumsy and it would be
really nice if it could be avoided. But it seems this is necessary because it
keeps track of information which is currently (implicitly) tracked by
RootView::DispatchGestureEvent()'s soon-to-be-removed loops. That is, we know
whether or not |gesture_handler_| was established by a previous gesture event
simply by which loop we're inside in DispatchGestureEvent().

[tdanderson, 2014-08-25 19:53:56]

Sadrul, can you please take a look? Note my inline comment in PS2.

[sadrul, 2014-08-26 16:21:44]

https://codereview.chromium.org/481433006/diff/60001/ui/views/view_targeter.cc
File ui/views/view_targeter.cc (right):

https://codereview.chromium.org/481433006/diff/60001/ui/views/view_targeter.c...
ui/views/view_targeter.cc:111: internal::RootView* root_view =
static_cast<internal::RootView*>(root);
This is pretty ugly.

How about have a RootViewTargeter, that knows about the RootView upon
construction, and this FindTargetForGestureEvent() CHECK_EQ(root_view_, root);?

https://codereview.chromium.org/481433006/diff/60001/ui/views/view_targeter.h
File ui/views/view_targeter.h (right):

https://codereview.chromium.org/481433006/diff/60001/ui/views/view_targeter.h...
ui/views/view_targeter.h:31: //                   has been removed.
Maybe make RootView friend for the short-term instead.

https://codereview.chromium.org/481433006/diff/60001/ui/views/widget/root_view.h
File ui/views/widget/root_view.h (right):

https://codereview.chromium.org/481433006/diff/60001/ui/views/widget/root_vie...
ui/views/widget/root_view.h:199: bool dispatch_to_gesture_handler_;
Is the main purpose here to stop the bubbling of the event? Would
'allow_gesture_bubbling_' be clearer/more appropriate?

[tdanderson, 2014-08-26 20:21:15]

Sadrul, can you please take another look?

https://codereview.chromium.org/481433006/diff/60001/ui/views/view_targeter.cc
File ui/views/view_targeter.cc (right):

https://codereview.chromium.org/481433006/diff/60001/ui/views/view_targeter.c...
ui/views/view_targeter.cc:111: internal::RootView* root_view =
static_cast<internal::RootView*>(root);
On 2014/08/26 16:21:44, sadrul wrote:
> This is pretty ugly.
> 
> How about have a RootViewTargeter, that knows about the RootView upon
> construction, and this FindTargetForGestureEvent() CHECK_EQ(root_view_,
root);?

Done.

https://codereview.chromium.org/481433006/diff/60001/ui/views/view_targeter.h
File ui/views/view_targeter.h (right):

https://codereview.chromium.org/481433006/diff/60001/ui/views/view_targeter.h...
ui/views/view_targeter.h:31: //                   has been removed.
On 2014/08/26 16:21:44, sadrul wrote:
> Maybe make RootView friend for the short-term instead.

Done.

https://codereview.chromium.org/481433006/diff/60001/ui/views/widget/root_view.h
File ui/views/widget/root_view.h (right):

https://codereview.chromium.org/481433006/diff/60001/ui/views/widget/root_vie...
ui/views/widget/root_view.h:199: bool dispatch_to_gesture_handler_;
On 2014/08/26 16:21:44, sadrul wrote:
> Is the main purpose here to stop the bubbling of the event? Would
> 'allow_gesture_bubbling_' be clearer/more appropriate?

Yes, that's pretty much the purpose of it. Changed the name as you suggested. I
was hesitant to name it something similar in the first place because even if the
member is false, we do still allow bubbling for SCROLL_BEGIN. But I do like it
better than the current name.

https://codereview.chromium.org/481433006/diff/80001/ui/views/view_targeter.cc
File ui/views/view_targeter.cc (right):

https://codereview.chromium.org/481433006/diff/80001/ui/views/view_targeter.c...
ui/views/view_targeter.cc:91: NOTREACHED();
There are currently no call-sites that will hit this other than from in
EventProcessor, but that is done on the RootView's targeter. It was turning out
to be a bit of a mess to implement this as we had discussed on IM (using the
effective targeter of the parent) so I'm happy to leave this as NOTREACHED() for
now. We can discuss later about the best way to allow something to request
targeting for gestures at an arbitrary node in a View tree (or whether such a
thing should even be permitted).

[sadrul, 2014-08-27 22:49:54]

LGTM

https://codereview.chromium.org/481433006/diff/80001/ui/views/view_targeter.cc
File ui/views/view_targeter.cc (right):

https://codereview.chromium.org/481433006/diff/80001/ui/views/view_targeter.c...
ui/views/view_targeter.cc:91: NOTREACHED();
On 2014/08/26 20:21:15, tdanderson wrote:
> There are currently no call-sites that will hit this other than from in
> EventProcessor, but that is done on the RootView's targeter. It was turning
out
> to be a bit of a mess to implement this as we had discussed on IM (using the
> effective targeter of the parent) so I'm happy to leave this as NOTREACHED()
for
> now. We can discuss later about the best way to allow something to request
> targeting for gestures at an arbitrary node in a View tree (or whether such a
> thing should even be permitted).

Add a TODO or a comment here as well, so that in case someone hits this, they
know we intend to fix this.

https://codereview.chromium.org/481433006/diff/80001/ui/views/widget/root_vie...
File ui/views/widget/root_view_targeter.cc (right):

https://codereview.chromium.org/481433006/diff/80001/ui/views/widget/root_vie...
ui/views/widget/root_view_targeter.cc:43: rect.set_origin(gesture.location());
You could do:
  rect = gfx::Rect(gesture.location(), gesture.details().bounding_box());

[tdanderson, 2014-08-27 23:25:57]

https://codereview.chromium.org/481433006/diff/80001/ui/views/view_targeter.cc
File ui/views/view_targeter.cc (right):

https://codereview.chromium.org/481433006/diff/80001/ui/views/view_targeter.c...
ui/views/view_targeter.cc:91: NOTREACHED();
On 2014/08/27 22:49:54, sadrul wrote:
> On 2014/08/26 20:21:15, tdanderson wrote:
> > There are currently no call-sites that will hit this other than from in
> > EventProcessor, but that is done on the RootView's targeter. It was turning
> out
> > to be a bit of a mess to implement this as we had discussed on IM (using the
> > effective targeter of the parent) so I'm happy to leave this as NOTREACHED()
> for
> > now. We can discuss later about the best way to allow something to request
> > targeting for gestures at an arbitrary node in a View tree (or whether such
a
> > thing should even be permitted).
> 
> Add a TODO or a comment here as well, so that in case someone hits this, they
> know we intend to fix this.

Done.

https://codereview.chromium.org/481433006/diff/80001/ui/views/widget/root_vie...
File ui/views/widget/root_view_targeter.cc (right):

https://codereview.chromium.org/481433006/diff/80001/ui/views/widget/root_vie...
ui/views/widget/root_view_targeter.cc:43: rect.set_origin(gesture.location());
On 2014/08/27 22:49:54, sadrul wrote:
> You could do:
>   rect = gfx::Rect(gesture.location(), gesture.details().bounding_box());

Did you mean .bounding_box().size()? I eliminated one line of code here by using
set_size() instead.

[tdanderson, 2014-08-27 23:26:01]

The CQ bit was checked by tdanderson@chromium.org

[commit-bot: I haz the power, 2014-08-27 23:27:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tdanderson@chromium.org/481433006/100001

[commit-bot: I haz the power, 2014-08-28 02:45:39]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-08-28 02:50:53]

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

[commit-bot: I haz the power, 2014-08-28 02:50:54]

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

[tdanderson, 2014-08-28 03:54:15]

The CQ bit was checked by tdanderson@chromium.org

[commit-bot: I haz the power, 2014-08-28 03:55:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tdanderson@chromium.org/481433006/100001

[commit-bot: I haz the power, 2014-08-28 04:33:30]

Committed patchset #6 (id:100001) as f7290bcce1b59595b2585d93322e2663ed6254af

[commit-bot: I haz the power, 2014-09-10 02:57:33]

Patchset 6 (id:??) landed as
https://crrev.com/a7b4a883cc904ecda130e0a8d91204f3770f2fae
Cr-Commit-Position: refs/heads/master@{#292321}