Implement Gesture event hit testing/forwarding for OOPIF.

Implement Gesture event hit testing/forwarding for OOPIF.

This CL implements plumbing to forward gesture events to OOPIF frames.

Scroll bubbling will be handled in a follow-on CL.

BUG=587023
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/4446423513e9f2da52e156bc93b5228e6a3d6e08
Cr-Commit-Position: refs/heads/master@{#379367}

[wjmaclean, 2016-03-01 19:00:10]

Description was changed from

==========
Implement Gesture event hit testing/forwarding for OOPIF.

This CL implements plumbing to forward gesture events to OOPIF frames.

Scroll bubbling will be handled in a follow-on CL.

BUG=587023
==========

to

==========
Implement Gesture event hit testing/forwarding for OOPIF.

This CL implements plumbing to forward gesture events to OOPIF frames.

Scroll bubbling will be handled in a follow-on CL.

BUG=587023
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[wjmaclean, 2016-03-01 19:03:04]

Description was changed from

==========
Implement Gesture event hit testing/forwarding for OOPIF.

This CL implements plumbing to forward gesture events to OOPIF frames.

Scroll bubbling will be handled in a follow-on CL.

BUG=587023
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Implement Gesture event hit testing/forwarding for OOPIF.

This CL implements plumbing to forward gesture events to OOPIF frames.

Scroll bubbling will be handled in a follow-on CL.

BUG=587023
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[wjmaclean, 2016-03-01 19:03:04]

wjmaclean@chromium.org changed reviewers:
+ kenrb@chromium.org

[wjmaclean, 2016-03-01 19:05:46]

kenrb@ - Would you be willing to give this an initial look-over?

*** This CL is dependent on https://codereview.chromium.org/1729373003 landing
first.

[kenrb, 2016-03-02 16:23:13]

I'm not especially familiar with gesture processing, but lgtm for the input
routing aspects of it.

https://codereview.chromium.org/1752833002/diff/1/content/browser/site_per_pr...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1752833002/diff/1/content/browser/site_per_pr...
content/browser/site_per_process_browsertest.cc:4517: // There have been no
GestureTaps send yet.
nit: s/send/sent

[wjmaclean, 2016-03-02 16:31:38]

Patchset #2 (id:20001) has been deleted

[wjmaclean, 2016-03-02 16:36:55]

wjmaclean@chromium.org changed reviewers:
+ nasko@chromium.org, tdresser@chromium.org

[wjmaclean, 2016-03-02 16:38:05]

tdresser@ - please confirm I implemented what we talked about w.r.t. Gesture
event semantics in RenderWidgetHostInputEventRouter.

nasko@ - please review for OWNERS on content/

[tdresser, 2016-03-02 17:54:44]

I think we'll need to add a bunch more tests, there are a lot of cases we should
test here.

We should have at least one test where the size of the queue is > 2.

https://codereview.chromium.org/1752833002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_input_event_router.cc
(right):

https://codereview.chromium.org/1752833002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_input_event_router.cc:43:
active_touches_ = 0;
Let's clear the transforms here as well.

https://codereview.chromium.org/1752833002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_input_event_router.cc:165: //
is not WebGestureEvent equivalent for ET_GESTURE_BEGIN. Note that this
is not -> is no

https://codereview.chromium.org/1752833002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_input_event_router.cc:167: //
GestureTapDown is sent to the previous target, but that is intentional.
We should have a test for this case. We should state why this behavior is
correct.

https://codereview.chromium.org/1752833002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_input_event_router.cc:170:
GestureTargetData& data = gesture_target_queue_.front();
const?

https://codereview.chromium.org/1752833002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_input_event_router.cc:175:
NOTIMPLEMENTED();
We should probably DCHECK that the queue isn't empty.

https://codereview.chromium.org/1752833002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_input_event_router.cc:207: //
for the duration of the sequence.
Computing the transform each event seems more correct and simpler, though I'm a
bit worried about the perf implications. It probably makes sense to take that
approach for now though.

Either way, we should have a test for changing the transform mid-test.

https://codereview.chromium.org/1752833002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_input_event_router.h
(right):

https://codereview.chromium.org/1752833002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_input_event_router.h:99:
RenderWidgetHostViewBase* target;
const?

https://codereview.chromium.org/1752833002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_input_event_router.h:100:
gfx::Vector2d delta;
|delta| needs to be a full transform, doesn't it?
It should probably be called transform (as with touch_delta_).

https://codereview.chromium.org/1752833002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_input_event_router.h:100:
gfx::Vector2d delta;
const?

https://codereview.chromium.org/1752833002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_input_event_router.h:117:
gfx::Vector2d touch_delta_;
touch_sequence_transform_
gesture_sequence_transform_?

We should probably use the current_ prefix for either both target and transform,
or neither.

I think:
touch_target_, gesture_target_ would be fine.

https://codereview.chromium.org/1752833002/diff/40001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1752833002/diff/40001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:4600:
root->child_at(0)->current_frame_host(),
It would likely make sense to cache the current_frame_host().

[wjmaclean, 2016-03-02 18:33:40]

I've addressed your comments as best I can, but need some more input on the
extra tests.

> I think we'll need to add a bunch more tests, there are a 
> lot of cases we should test here.
>
> We should have at least one test where the size of the 
> queue is > 2.

Is there an example of a test for this in the non-oopif case I can use as an
example?

Are there other specific test cases you had in mind, or should I use my best
judgement?

https://codereview.chromium.org/1752833002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_input_event_router.cc
(right):

https://codereview.chromium.org/1752833002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_input_event_router.cc:43:
active_touches_ = 0;
On 2016/03/02 17:54:43, tdresser wrote:
> Let's clear the transforms here as well.

Done.

https://codereview.chromium.org/1752833002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_input_event_router.cc:165: //
is not WebGestureEvent equivalent for ET_GESTURE_BEGIN. Note that this
On 2016/03/02 17:54:43, tdresser wrote:
> is not -> is no

Done.

https://codereview.chromium.org/1752833002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_input_event_router.cc:167: //
GestureTapDown is sent to the previous target, but that is intentional.
On 2016/03/02 17:54:43, tdresser wrote:
> We should have a test for this case. We should state why this behavior is
> correct.

Hmmm, ok. I'm not sure I'm clear on what constitutes a valid test here. The
current test *does* process the initial GestureFlingCancel, but as there have
been no prior touch events, it gets a null target.

At present OOPIF fling-scroll doesn't work. This CL lays the groundwork for
fixing it though.

https://codereview.chromium.org/1752833002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_input_event_router.cc:170:
GestureTargetData& data = gesture_target_queue_.front();
On 2016/03/02 17:54:43, tdresser wrote:
> const?

Done.

https://codereview.chromium.org/1752833002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_input_event_router.cc:175:
NOTIMPLEMENTED();
On 2016/03/02 17:54:43, tdresser wrote:
> We should probably DCHECK that the queue isn't empty.

Done.

https://codereview.chromium.org/1752833002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_input_event_router.cc:207: //
for the duration of the sequence.
On 2016/03/02 17:54:43, tdresser wrote:
> Computing the transform each event seems more correct and simpler, though I'm
a
> bit worried about the perf implications. It probably makes sense to take that
> approach for now though.
> 
> Either way, we should have a test for changing the transform mid-test.

Acknowledged.

As indicated in a previous comment, we need to come up with a way to recover the
transform dynamically, but that's another CL.

https://codereview.chromium.org/1752833002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_input_event_router.h
(right):

https://codereview.chromium.org/1752833002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_input_event_router.h:99:
RenderWidgetHostViewBase* target;
On 2016/03/02 17:54:43, tdresser wrote:
> const?

No ... this pointer is ultimately used to call a non-const function onthe RWHVx.

https://codereview.chromium.org/1752833002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_input_event_router.h:100:
gfx::Vector2d delta;
On 2016/03/02 17:54:43, tdresser wrote:
> |delta| needs to be a full transform, doesn't it?
> It should probably be called transform (as with touch_delta_).

Someday, yes. For this CL, no. We still need to devise a mechanism to compute
the transform for a target RWHVx on-the-fly, at which time we'll move the
transform right out of the queue altogether as its value could change with time.

https://codereview.chromium.org/1752833002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_input_event_router.h:100:
gfx::Vector2d delta;
On 2016/03/02 17:54:43, tdresser wrote:
> const?

Done.

https://codereview.chromium.org/1752833002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_input_event_router.h:117:
gfx::Vector2d touch_delta_;
On 2016/03/02 17:54:43, tdresser wrote:
> touch_sequence_transform_
> gesture_sequence_transform_?
> 
> We should probably use the current_ prefix for either both target and
transform,
> or neither.
> 
> I think:
> touch_target_, gesture_target_ would be fine.

Done.

https://codereview.chromium.org/1752833002/diff/40001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1752833002/diff/40001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:4600:
root->child_at(0)->current_frame_host(),
On 2016/03/02 17:54:43, tdresser wrote:
> It would likely make sense to cache the current_frame_host().

Done.

[tdresser, 2016-03-02 19:25:11]

https://codereview.chromium.org/1752833002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_input_event_router.cc
(right):

https://codereview.chromium.org/1752833002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_input_event_router.cc:167: //
GestureTapDown is sent to the previous target, but that is intentional.
On 2016/03/02 18:33:40, wjmaclean wrote:
> On 2016/03/02 17:54:43, tdresser wrote:
> > We should have a test for this case. We should state why this behavior is
> > correct.
> 
> Hmmm, ok. I'm not sure I'm clear on what constitutes a valid test here. The
> current test *does* process the initial GestureFlingCancel, but as there have
> been no prior touch events, it gets a null target.
> 
> At present OOPIF fling-scroll doesn't work. This CL lays the groundwork for
> fixing it though.

We should have a test where there are prior touch events, so it gets a non-null
target.

https://codereview.chromium.org/1752833002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_input_event_router.cc:207: //
for the duration of the sequence.
On 2016/03/02 18:33:40, wjmaclean wrote:
> On 2016/03/02 17:54:43, tdresser wrote:
> > Computing the transform each event seems more correct and simpler, though
I'm
> a
> > bit worried about the perf implications. It probably makes sense to take
that
> > approach for now though.
> > 
> > Either way, we should have a test for changing the transform mid-test.
> 
> Acknowledged.
> 
> As indicated in a previous comment, we need to come up with a way to recover
the
> transform dynamically, but that's another CL.

Acknowledged.

[nasko, 2016-03-03 19:01:24]

LGTM on browser test, rubberstamp based on Ken's review on the rest. Some nits
included.

https://codereview.chromium.org/1752833002/diff/60001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1752833002/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:4618: //TODO(wjmaclean): Convert
the call to base::Bind() to a lambda someday.
nit: Space between // and TODO.

https://codereview.chromium.org/1752833002/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:4622: runner->Run();
A small comment about why we are running the message loop here will be nice. It
wasn't obvious at first glance.

https://codereview.chromium.org/1752833002/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:4631: EXPECT_EQ("1 clicks
received", result);
fun: "1 clicks" is technically incorrect grammar ;).

https://codereview.chromium.org/1752833002/diff/60001/content/test/data/page_...
File content/test/data/page_with_click_handler.html (right):

https://codereview.chromium.org/1752833002/diff/60001/content/test/data/page_...
content/test/data/page_with_click_handler.html:10: clickResult.innerHTML =
clicksReceived + " clicks received";
Since you are sticking plaintext in the element, why not use textContent?

https://codereview.chromium.org/1752833002/diff/60001/content/test/data/page_...
content/test/data/page_with_click_handler.html:18:
window.addEventListener('load', function(){ // on page load
nit: The comment is not needed, it is fairly obvious this is a page load
handler.

[wjmaclean, 2016-03-03 21:32:52]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-03 21:33:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1752833002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1752833002/80001

[commit-bot: I haz the power, 2016-03-03 21:52:17]

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

[commit-bot: I haz the power, 2016-03-03 21:52:19]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[wjmaclean, 2016-03-03 22:27:50]

tdresser@ - PTAL, I've expanded the testing of the gesture target queue as we
discussed.

https://codereview.chromium.org/1752833002/diff/60001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1752833002/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:4618: //TODO(wjmaclean): Convert
the call to base::Bind() to a lambda someday.
On 2016/03/03 19:01:24, nasko wrote:
> nit: Space between // and TODO.

Done.

https://codereview.chromium.org/1752833002/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:4622: runner->Run();
On 2016/03/03 19:01:24, nasko wrote:
> A small comment about why we are running the message loop here will be nice.
It
> wasn't obvious at first glance.

Done.

https://codereview.chromium.org/1752833002/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:4631: EXPECT_EQ("1 clicks
received", result);
On 2016/03/03 19:01:24, nasko wrote:
> fun: "1 clicks" is technically incorrect grammar ;).

Ha ha, yes :-)

I'm not sure how "1" gets special treatment, since "0 clicks" makes sense, and
"2 clicks", etc.

Fixed.

https://codereview.chromium.org/1752833002/diff/60001/content/test/data/page_...
File content/test/data/page_with_click_handler.html (right):

https://codereview.chromium.org/1752833002/diff/60001/content/test/data/page_...
content/test/data/page_with_click_handler.html:10: clickResult.innerHTML =
clicksReceived + " clicks received";
On 2016/03/03 19:01:24, nasko wrote:
> Since you are sticking plaintext in the element, why not use textContent?

Done.

https://codereview.chromium.org/1752833002/diff/60001/content/test/data/page_...
content/test/data/page_with_click_handler.html:18:
window.addEventListener('load', function(){ // on page load
On 2016/03/03 19:01:24, nasko wrote:
> nit: The comment is not needed, it is fairly obvious this is a page load
> handler.

Done.

[tdresser, 2016-03-04 19:51:01]

LGTM!

https://codereview.chromium.org/1752833002/diff/120001/content/browser/render...
File content/browser/renderer_host/render_widget_host_input_event_router.cc
(right):

https://codereview.chromium.org/1752833002/diff/120001/content/browser/render...
content/browser/renderer_host/render_widget_host_input_event_router.cc:170: //
GestureTapDown is sent to the previous target, but that is intentional.
Describe why it's correct for the GFC to target the previous frame.

https://codereview.chromium.org/1752833002/diff/120001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1752833002/diff/120001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:4693: // GestureFlingCancel that
will be inserted before GestureTapDown.
We don't get a GFC in this test, correct? That makes this comment somewhat
confusing. Perhaps we should synthesize a GFC?

[kenrb, 2016-03-04 19:58:46]

still lgtm

[wjmaclean, 2016-03-04 20:02:37]

https://codereview.chromium.org/1752833002/diff/120001/content/browser/render...
File content/browser/renderer_host/render_widget_host_input_event_router.cc
(right):

https://codereview.chromium.org/1752833002/diff/120001/content/browser/render...
content/browser/renderer_host/render_widget_host_input_event_router.cc:170: //
GestureTapDown is sent to the previous target, but that is intentional.
On 2016/03/04 19:51:01, tdresser wrote:
> Describe why it's correct for the GFC to target the previous frame.

Done.

https://codereview.chromium.org/1752833002/diff/120001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1752833002/diff/120001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:4693: // GestureFlingCancel that
will be inserted before GestureTapDown.
On 2016/03/04 19:51:01, tdresser wrote:
> We don't get a GFC in this test, correct? That makes this comment somewhat
> confusing. Perhaps we should synthesize a GFC?

Actually, there is a GestureFlingCancel in the test, but it's added by
RenderWidgetHostViewAura::OnGestureEvent() when it sees the
ui::ET_GESTURE_TAP_DOWN, so we don't have to add it explicitly. I've modified
the comment to make that clear.

[wjmaclean, 2016-03-04 20:03:48]

The CQ bit was checked by wjmaclean@chromium.org

[wjmaclean, 2016-03-04 20:03:48]

The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org,
kenrb@chromium.org, tdresser@chromium.org
Link to the patchset: https://codereview.chromium.org/1752833002/#ps140001
(title: "Update comments as per suggestions.")

[commit-bot: I haz the power, 2016-03-04 20:04:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1752833002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1752833002/140001

[commit-bot: I haz the power, 2016-03-04 21:49:27]

Description was changed from

==========
Implement Gesture event hit testing/forwarding for OOPIF.

This CL implements plumbing to forward gesture events to OOPIF frames.

Scroll bubbling will be handled in a follow-on CL.

BUG=587023
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Implement Gesture event hit testing/forwarding for OOPIF.

This CL implements plumbing to forward gesture events to OOPIF frames.

Scroll bubbling will be handled in a follow-on CL.

BUG=587023
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-03-04 21:49:28]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-03-04 21:50:36]

Description was changed from

==========
Implement Gesture event hit testing/forwarding for OOPIF.

This CL implements plumbing to forward gesture events to OOPIF frames.

Scroll bubbling will be handled in a follow-on CL.

BUG=587023
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Implement Gesture event hit testing/forwarding for OOPIF.

This CL implements plumbing to forward gesture events to OOPIF frames.

Scroll bubbling will be handled in a follow-on CL.

BUG=587023
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/4446423513e9f2da52e156bc93b5228e6a3d6e08
Cr-Commit-Position: refs/heads/master@{#379367}
==========

[commit-bot: I haz the power, 2016-03-04 21:50:38]

Patchset 7 (id:??) landed as
https://crrev.com/4446423513e9f2da52e156bc93b5228e6a3d6e08
Cr-Commit-Position: refs/heads/master@{#379367}