Make RWHInputEventRouter manage cross-process scroll bubbling

Make RWHInputEventRouter manage cross-process scroll bubbling

Mousewheel-based scroll bubbling from OOPIFs to parent frames was broken
in a couple of ways, due to it improperly accounting for the mechanics
of synthetic gesture scroll events from the InputRouter class. This
refactors bubbling to do the following:
- remove bubbling of unused scroll delta from WheelEvent acks, which was
  wrong,
- pass acks from GestureScrollUpdate and GestureScrollEnd events to
  the CrossProcessFrameConnector and then have RWHInputEventRouter do
  most of the reasoning about where to target unused scroll,
- track the current scroll bubbling target, which can change due to
  nested OOPIFs, and manage GestureScrollBegin and GestureScrollEnds
  to each renderer (the previous approach of wrapping each
  GestureScrollUpdate led to bubbled scrolls feeling really slow).

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

Committed: https://crrev.com/3bdfea50bdfc6cc2b73c12969ca6207b5949fac1
Cr-Commit-Position: refs/heads/master@{#404500}

[kenrb, 2016-07-05 20:33:18]

Description was changed from

==========
Make RWHInputEventRouter manage cross-process scroll bubbling

Mousewheel-based scroll bubbling from OOPIFs to parent frames was broken
in a couple of ways, due to it improperly accounting for the mechanics
of synthetic gesture scroll events from the InputRouter class. This
refactors bubbling to do the following:
- remove bubbling of unused scroll delta from WheelEvent acks, which was
  wrong,
- pass acks from GestureScrollUpdate and GestureScrollEnd events to
  the CrossProcessFrameConnector and then have RWHInputEventRouter do
  most of the reasoning about where to target unused scroll,
- track the current scroll bubbling target, which can change due to
  nested OOPIFs, and manage GestureScrollBegin and GestureScrollEnds
  to each renderer (the previous approach of wrapping each
  GestureScrollUpdate led to bubbled scrolls feeling really slow).

BUG=621624
==========

to

==========
Make RWHInputEventRouter manage cross-process scroll bubbling

Mousewheel-based scroll bubbling from OOPIFs to parent frames was broken
in a couple of ways, due to it improperly accounting for the mechanics
of synthetic gesture scroll events from the InputRouter class. This
refactors bubbling to do the following:
- remove bubbling of unused scroll delta from WheelEvent acks, which was
  wrong,
- pass acks from GestureScrollUpdate and GestureScrollEnd events to
  the CrossProcessFrameConnector and then have RWHInputEventRouter do
  most of the reasoning about where to target unused scroll,
- track the current scroll bubbling target, which can change due to
  nested OOPIFs, and manage GestureScrollBegin and GestureScrollEnds
  to each renderer (the previous approach of wrapping each
  GestureScrollUpdate led to bubbled scrolls feeling really slow).

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

[kenrb, 2016-07-06 13:48:32]

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

[kenrb, 2016-07-06 13:48:33]

wjmaclean@: PTAL at the overall patch.
dtapuska@: Can you review this to see if it is sane from the perspective of
managing all the gesture events? It is similar-ish to what we were talking about
on a white board last week.

I was having trouble getting a browser test for this working yesterday, but I'd
appreciate a review on the patch itself since it works well manually. Also,
there are a couple of comments that I forgot to remove/update, which I will fix
later on when I upload the test.

[wjmaclean, 2016-07-06 14:30:44]

I've put a few comments on this. I am confused by the first/bubbling target
logic in RWHIER, but for the most part it seems fine otherwise.

https://codereview.chromium.org/2123843002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/cross_process_frame_connector.cc (right):

https://codereview.chromium.org/2123843002/diff/20001/content/browser/frame_h...
content/browser/frame_host/cross_process_frame_connector.cc:65: if (view_) {
Since this block of logic appears identical to the one in the destructor, would
it make sense to move it into a separate function?

Also, especially w.r.t. the call from the destructor, I assume we know the
lifetimes of the various chained objects to be sure that we won't ever hit a
nullptr.

https://codereview.chromium.org/2123843002/diff/20001/content/browser/frame_h...
content/browser/frame_host/cross_process_frame_connector.cc:177:
resent_gesture_event.x += offset_from_parent.x();
At some point we should determine if we ever need to handle more general
transforms here, e.g. a rotated iframe (is that allowed)? It's not just here,
but we should consider starting to enumerate the locations in the event handling
code, perhaps with TODOs and a tracking bug number, to keep track of this.

https://codereview.chromium.org/2123843002/diff/20001/content/browser/frame_h...
content/browser/frame_host/cross_process_frame_connector.cc:180: // With support
from friends, resent_gesture_event might one day overcome
;-)

https://codereview.chromium.org/2123843002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/2123843002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:339: //
forward GestureScrollUpdate.
I was surprised to see forwarding of the GestureScrollEnd above, as I had always
assume this, and that it was the responsibility of the new target (receiving the
bubbled scroll) to make sure it is properly wrapped. But I'm not 100% sure of
how the renderer/input_handler etc. deal with GestureScrollEnd, so this may also
be fine.

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

https://codereview.chromium.org/2123843002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_input_event_router.cc:279:
return;
It seems like this could allow some, but not all, of the bubbling scrolls to
make it through ... is this the desired behaviour? Or should we cancel the
remainder of the bubbling sequence in this case (although that might be
difficult to keep track of)?

https://codereview.chromium.org/2123843002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_input_event_router.cc:291:
bubbling_gesture_scroll_target_.target = target_view;
I'm a little confused by the naming here: fbst and bgst seem to start together,
and bgst if it exists always gets a scroll-end, and then advances (because the
target has changed), but first lingers ... is that correct? Are there never
multiple events that bubble to the same bgst?

Are there better names, or some other way to arrange the logic, to make this
code easier to follow?

[kenrb, 2016-07-06 16:07:52]

Thanks for the review. I had started putting together a drawing last week to get
this all straight in my head, and updated it this morning to reflect what I
actually ended up coding. It might help with some of the complexity:
https://docs.google.com/drawings/d/1FBRfjyAU1Njqb_-Nc6BncYJBPkuR2-OZu_j3Va_pq...

https://codereview.chromium.org/2123843002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/cross_process_frame_connector.cc (right):

https://codereview.chromium.org/2123843002/diff/20001/content/browser/frame_h...
content/browser/frame_host/cross_process_frame_connector.cc:65: if (view_) {
On 2016/07/06 14:30:44, wjmaclean wrote:
> Since this block of logic appears identical to the one in the destructor,
would
> it make sense to move it into a separate function?
> 

Done. Just having the destructor call set_view(nullptr) seems better.

> Also, especially w.r.t. the call from the destructor, I assume we know the
> lifetimes of the various chained objects to be sure that we won't ever hit a
> nullptr.

It's complicated because there are different destruction sequences, e.g. if
things get destroyed based on a renderer crash vs on a frame navigation.

https://codereview.chromium.org/2123843002/diff/20001/content/browser/frame_h...
content/browser/frame_host/cross_process_frame_connector.cc:177:
resent_gesture_event.x += offset_from_parent.x();
On 2016/07/06 14:30:44, wjmaclean wrote:
> At some point we should determine if we ever need to handle more general
> transforms here, e.g. a rotated iframe (is that allowed)? It's not just here,
> but we should consider starting to enumerate the locations in the event
handling
> code, perhaps with TODOs and a tracking bug number, to keep track of this.

Done. https://crbug.com/626020.

https://codereview.chromium.org/2123843002/diff/20001/content/browser/frame_h...
content/browser/frame_host/cross_process_frame_connector.cc:180: // With support
from friends, resent_gesture_event might one day overcome
On 2016/07/06 14:30:44, wjmaclean wrote:
> ;-)

I couldn't find anything in the style guide about bad puns. :)

https://codereview.chromium.org/2123843002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/2123843002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:339: //
forward GestureScrollUpdate.
On 2016/07/06 14:30:44, wjmaclean wrote:
> I was surprised to see forwarding of the GestureScrollEnd above, as I had
always
> assume this, and that it was the responsibility of the new target (receiving
the
> bubbled scroll) to make sure it is properly wrapped. But I'm not 100% sure of
> how the renderer/input_handler etc. deal with GestureScrollEnd, so this may
also
> be fine.

This is part of trying not to individually wrap each GestureScrollUpdate in a
GestureScrollBegin and GestureScrollEnd, which made scrolling feel a lot less
smooth to me. The new code synthesizes a GestureScrollBegin, but forwards the
GestureScrollEnd so that if it has to forward multiple GestureScrollUpdates in
the meantime they can go in sequence without hit testing, etc.
(GestureScrollBegin is what triggers a hit test.)

I have updated the comment, as obviously I had intended to originally.

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

https://codereview.chromium.org/2123843002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_input_event_router.cc:279:
return;
On 2016/07/06 14:30:44, wjmaclean wrote:
> It seems like this could allow some, but not all, of the bubbling scrolls to
> make it through ... is this the desired behaviour? Or should we cancel the
> remainder of the bubbling sequence in this case (although that might be
> difficult to keep track of)?

Good question, I was trying to work this out on Monday, and I think it is okay.
To reach this point in the method, this has to be a GestureScrollUpdate with a
new target. If we drop it at this point, the only consequence is that the scroll
delta is lost and the state is otherwise unchanged. There might be a stream of
GSUs coming, which can all be dropped or possibly at some point they get
accepted and things continue as normal.

https://codereview.chromium.org/2123843002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_input_event_router.cc:291:
bubbling_gesture_scroll_target_.target = target_view;
On 2016/07/06 14:30:44, wjmaclean wrote:
> I'm a little confused by the naming here: fbst and bgst seem to start
together,
> and bgst if it exists always gets a scroll-end, and then advances (because the
> target has changed), but first lingers ... is that correct? Are there never
> multiple events that bubble to the same bgst?
> 
> Are there better names, or some other way to arrange the logic, to make this
> code easier to follow?

The main thing is that incoming scroll updates from the original source will
always be targeted to first_bubbling_scroll_target_, so we need to differentiate
between those and nested "re-bubbles" that change the actual scroll target. Take
a look at my drawing and let me know if you think there are good ways to clarify
this better.

[wjmaclean, 2016-07-06 17:38:10]

https://codereview.chromium.org/2123843002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/cross_process_frame_connector.cc (right):

https://codereview.chromium.org/2123843002/diff/20001/content/browser/frame_h...
content/browser/frame_host/cross_process_frame_connector.cc:177:
resent_gesture_event.x += offset_from_parent.x();
On 2016/07/06 16:07:52, kenrb wrote:
> On 2016/07/06 14:30:44, wjmaclean wrote:
> > At some point we should determine if we ever need to handle more general
> > transforms here, e.g. a rotated iframe (is that allowed)? It's not just
here,
> > but we should consider starting to enumerate the locations in the event
> handling
> > code, perhaps with TODOs and a tracking bug number, to keep track of this.
> 
> Done. https://crbug.com/626020.

Thanks!

https://codereview.chromium.org/2123843002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/2123843002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:339: //
forward GestureScrollUpdate.
On 2016/07/06 16:07:52, kenrb wrote:
> On 2016/07/06 14:30:44, wjmaclean wrote:
> > I was surprised to see forwarding of the GestureScrollEnd above, as I had
> always
> > assume this, and that it was the responsibility of the new target (receiving
> the
> > bubbled scroll) to make sure it is properly wrapped. But I'm not 100% sure
of
> > how the renderer/input_handler etc. deal with GestureScrollEnd, so this may
> also
> > be fine.
> 
> This is part of trying not to individually wrap each GestureScrollUpdate in a
> GestureScrollBegin and GestureScrollEnd, which made scrolling feel a lot less
> smooth to me. The new code synthesizes a GestureScrollBegin, but forwards the
> GestureScrollEnd so that if it has to forward multiple GestureScrollUpdates in
> the meantime they can go in sequence without hit testing, etc.
> (GestureScrollBegin is what triggers a hit test.)
> 
> I have updated the comment, as obviously I had intended to originally.

So when we add the GestureScrollBegin/End in (I think) RWHI, it tracks whether
or not we are already in a GestureScroll, so only one Begin/End pair is ever
added (i.e. we don't wrap every GestureScrollUpdate in a Begin/End ... that
would certainly be bad, and lead to sluggish behaviour :-) ).

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

https://codereview.chromium.org/2123843002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_input_event_router.cc:279:
return;
On 2016/07/06 16:07:52, kenrb wrote:
> On 2016/07/06 14:30:44, wjmaclean wrote:
> > It seems like this could allow some, but not all, of the bubbling scrolls to
> > make it through ... is this the desired behaviour? Or should we cancel the
> > remainder of the bubbling sequence in this case (although that might be
> > difficult to keep track of)?
> 
> Good question, I was trying to work this out on Monday, and I think it is
okay.
> To reach this point in the method, this has to be a GestureScrollUpdate with a
> new target. If we drop it at this point, the only consequence is that the
scroll
> delta is lost and the state is otherwise unchanged. There might be a stream of
> GSUs coming, which can all be dropped or possibly at some point they get
> accepted and things continue as normal.

Cool, thanks for figuring that out :-)

https://codereview.chromium.org/2123843002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_input_event_router.cc:291:
bubbling_gesture_scroll_target_.target = target_view;
On 2016/07/06 16:07:52, kenrb wrote:
> On 2016/07/06 14:30:44, wjmaclean wrote:
> > I'm a little confused by the naming here: fbst and bgst seem to start
> together,
> > and bgst if it exists always gets a scroll-end, and then advances (because
the
> > target has changed), but first lingers ... is that correct? Are there never
> > multiple events that bubble to the same bgst?
> > 
> > Are there better names, or some other way to arrange the logic, to make this
> > code easier to follow?
> 
> The main thing is that incoming scroll updates from the original source will
> always be targeted to first_bubbling_scroll_target_, so we need to
differentiate
> between those and nested "re-bubbles" that change the actual scroll target.
Take
> a look at my drawing and let me know if you think there are good ways to
clarify
> this better.

Looking ... if I come up with any improvements I'll add them in a different
comment.

[wjmaclean, 2016-07-06 17:49:30]

On 2016/07/06 17:38:10, wjmaclean wrote:
>
https://codereview.chromium.org/2123843002/diff/20001/content/browser/frame_h...
> File content/browser/frame_host/cross_process_frame_connector.cc (right):
> 
>
https://codereview.chromium.org/2123843002/diff/20001/content/browser/frame_h...
> content/browser/frame_host/cross_process_frame_connector.cc:177:
> resent_gesture_event.x += offset_from_parent.x();
> On 2016/07/06 16:07:52, kenrb wrote:
> > On 2016/07/06 14:30:44, wjmaclean wrote:
> > > At some point we should determine if we ever need to handle more general
> > > transforms here, e.g. a rotated iframe (is that allowed)? It's not just
> here,
> > > but we should consider starting to enumerate the locations in the event
> > handling
> > > code, perhaps with TODOs and a tracking bug number, to keep track of this.
> > 
> > Done. https://crbug.com/626020.
> 
> Thanks!
> 
>
https://codereview.chromium.org/2123843002/diff/20001/content/browser/frame_h...
> File content/browser/frame_host/render_widget_host_view_child_frame.cc
(right):
> 
>
https://codereview.chromium.org/2123843002/diff/20001/content/browser/frame_h...
> content/browser/frame_host/render_widget_host_view_child_frame.cc:339: //
> forward GestureScrollUpdate.
> On 2016/07/06 16:07:52, kenrb wrote:
> > On 2016/07/06 14:30:44, wjmaclean wrote:
> > > I was surprised to see forwarding of the GestureScrollEnd above, as I had
> > always
> > > assume this, and that it was the responsibility of the new target
(receiving
> > the
> > > bubbled scroll) to make sure it is properly wrapped. But I'm not 100% sure
> of
> > > how the renderer/input_handler etc. deal with GestureScrollEnd, so this
may
> > also
> > > be fine.
> > 
> > This is part of trying not to individually wrap each GestureScrollUpdate in
a
> > GestureScrollBegin and GestureScrollEnd, which made scrolling feel a lot
less
> > smooth to me. The new code synthesizes a GestureScrollBegin, but forwards
the
> > GestureScrollEnd so that if it has to forward multiple GestureScrollUpdates
in
> > the meantime they can go in sequence without hit testing, etc.
> > (GestureScrollBegin is what triggers a hit test.)
> > 
> > I have updated the comment, as obviously I had intended to originally.
> 
> So when we add the GestureScrollBegin/End in (I think) RWHI, it tracks whether
> or not we are already in a GestureScroll, so only one Begin/End pair is ever
> added (i.e. we don't wrap every GestureScrollUpdate in a Begin/End ... that
> would certainly be bad, and lead to sluggish behaviour :-) ).
> 
>
https://codereview.chromium.org/2123843002/diff/20001/content/browser/rendere...
> File content/browser/renderer_host/render_widget_host_input_event_router.cc
> (right):
> 
>
https://codereview.chromium.org/2123843002/diff/20001/content/browser/rendere...
> content/browser/renderer_host/render_widget_host_input_event_router.cc:279:
> return;
> On 2016/07/06 16:07:52, kenrb wrote:
> > On 2016/07/06 14:30:44, wjmaclean wrote:
> > > It seems like this could allow some, but not all, of the bubbling scrolls
to
> > > make it through ... is this the desired behaviour? Or should we cancel the
> > > remainder of the bubbling sequence in this case (although that might be
> > > difficult to keep track of)?
> > 
> > Good question, I was trying to work this out on Monday, and I think it is
> okay.
> > To reach this point in the method, this has to be a GestureScrollUpdate with
a
> > new target. If we drop it at this point, the only consequence is that the
> scroll
> > delta is lost and the state is otherwise unchanged. There might be a stream
of
> > GSUs coming, which can all be dropped or possibly at some point they get
> > accepted and things continue as normal.
> 
> Cool, thanks for figuring that out :-)
> 
>
https://codereview.chromium.org/2123843002/diff/20001/content/browser/rendere...
> content/browser/renderer_host/render_widget_host_input_event_router.cc:291:
> bubbling_gesture_scroll_target_.target = target_view;
> On 2016/07/06 16:07:52, kenrb wrote:
> > On 2016/07/06 14:30:44, wjmaclean wrote:
> > > I'm a little confused by the naming here: fbst and bgst seem to start
> > together,
> > > and bgst if it exists always gets a scroll-end, and then advances (because
> the
> > > target has changed), but first lingers ... is that correct? Are there
never
> > > multiple events that bubble to the same bgst?
> > > 
> > > Are there better names, or some other way to arrange the logic, to make
this
> > > code easier to follow?
> > 
> > The main thing is that incoming scroll updates from the original source will
> > always be targeted to first_bubbling_scroll_target_, so we need to
> differentiate
> > between those and nested "re-bubbles" that change the actual scroll target.
> Take
> > a look at my drawing and let me know if you think there are good ways to
> clarify
> > this better.
> 
> Looking ... if I come up with any improvements I'll add them in a different
> comment.

LGTM, modulo our in-person discussion.

[wjmaclean, 2016-07-06 17:49:55]

> LGTM, modulo our in-person discussion.

Oh, and a test at some point :-)

[kenrb, 2016-07-06 18:50:46]

Thanks, still working on the test.

https://codereview.chromium.org/2123843002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/2123843002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:339: //
forward GestureScrollUpdate.
On 2016/07/06 17:38:10, wjmaclean wrote:
> On 2016/07/06 16:07:52, kenrb wrote:
> > On 2016/07/06 14:30:44, wjmaclean wrote:
> > > I was surprised to see forwarding of the GestureScrollEnd above, as I had
> > always
> > > assume this, and that it was the responsibility of the new target
(receiving
> > the
> > > bubbled scroll) to make sure it is properly wrapped. But I'm not 100% sure
> of
> > > how the renderer/input_handler etc. deal with GestureScrollEnd, so this
may
> > also
> > > be fine.
> > 
> > This is part of trying not to individually wrap each GestureScrollUpdate in
a
> > GestureScrollBegin and GestureScrollEnd, which made scrolling feel a lot
less
> > smooth to me. The new code synthesizes a GestureScrollBegin, but forwards
the
> > GestureScrollEnd so that if it has to forward multiple GestureScrollUpdates
in
> > the meantime they can go in sequence without hit testing, etc.
> > (GestureScrollBegin is what triggers a hit test.)
> > 
> > I have updated the comment, as obviously I had intended to originally.
> 
> So when we add the GestureScrollBegin/End in (I think) RWHI, it tracks whether
> or not we are already in a GestureScroll, so only one Begin/End pair is ever
> added (i.e. we don't wrap every GestureScrollUpdate in a Begin/End ... that
> would certainly be bad, and lead to sluggish behaviour :-) ).

Okay, I think that works for BrowserPlugin when we are routing inputs through
the parent, but not OOPIF, because the parent renderer is not already in the
midst of a scroll. So every bubbled GestureScrollUpdate will get its own
GestureScrollBegin and End even if there was a long sequence of them sent to the
child renderer.

[wjmaclean, 2016-07-06 18:54:56]

On 2016/07/06 18:50:46, kenrb wrote:
> 
> Okay, I think that works for BrowserPlugin when we are routing inputs through
> the parent, but not OOPIF, because the parent renderer is not already in the
> midst of a scroll. So every bubbled GestureScrollUpdate will get its own
> GestureScrollBegin and End even if there was a long sequence of them sent to
the
> child renderer.

Sure, that makes sense ... I had forgotten that the parent would (likely, modulo
asynchrony) already be in a scroll if the guest started bubbling up GSU's to it
...

[dtapuska, 2016-07-06 19:13:15]

dtapuska@chromium.org changed reviewers:
+ tdresser@chromium.org

[dtapuska, 2016-07-06 19:13:16]

https://codereview.chromium.org/2123843002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/2123843002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:335: //
GestureScrollBegin is consumed by the target frame and not forwarded,
Ideally this would have been simple had if we could use the GestureScrollBegin
as the targeting event. For mouse gestures I imagine that you could but I wonder
if you couldn't on touch gestures. Then the entire gesture sequence could have
been sent there.

[tdresser, 2016-07-06 19:26:08]

I think this might be simplified a fair bit by touchpad latching. We should
revisit this once that has landed.

https://codereview.chromium.org/2123843002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/2123843002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:335: //
GestureScrollBegin is consumed by the target frame and not forwarded,
On 2016/07/06 19:13:16, dtapuska wrote:
> Ideally this would have been simple had if we could use the GestureScrollBegin
> as the targeting event. For mouse gestures I imagine that you could but I
wonder
> if you couldn't on touch gestures. Then the entire gesture sequence could have
> been sent there.

We might be able to only use the GSB once we've got latching for touchpad, but
that's still in progress.

[dtapuska, 2016-07-06 19:31:49]

On 2016/07/06 19:26:08, tdresser wrote:
> I think this might be simplified a fair bit by touchpad latching. We should
> revisit this once that has landed.
> 
>
https://codereview.chromium.org/2123843002/diff/40001/content/browser/frame_h...
> File content/browser/frame_host/render_widget_host_view_child_frame.cc
(right):
> 
>
https://codereview.chromium.org/2123843002/diff/40001/content/browser/frame_h...
> content/browser/frame_host/render_widget_host_view_child_frame.cc:335: //
> GestureScrollBegin is consumed by the target frame and not forwarded,
> On 2016/07/06 19:13:16, dtapuska wrote:
> > Ideally this would have been simple had if we could use the
GestureScrollBegin
> > as the targeting event. For mouse gestures I imagine that you could but I
> wonder
> > if you couldn't on touch gestures. Then the entire gesture sequence could
have
> > been sent there.
> 
> We might be able to only use the GSB once we've got latching for touchpad, but
> that's still in progress.

The only thing that could get messed up in this and I don't know if it is a
grand concern is if you have 3 events.

A, B get sent to a renderer. A indicates it isn't consumed. The target gets
updated to the parent and then C comes in.

The parent will see A, C, B ordering.

[kenrb, 2016-07-06 19:58:44]

Thanks for noticing the ordering problem. As we were saying, it seems like a
very minor issue if a small number of GestureScrollUpdates get out of order from
each other in a long chain, but then I realized that it could also mean the
GestureScrollEnd getting out of order with a GestureScrollUpdate, which is a lot
more problematic.

https://codereview.chromium.org/2123843002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/2123843002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:335: //
GestureScrollBegin is consumed by the target frame and not forwarded,
On 2016/07/06 19:26:07, tdresser wrote:
> On 2016/07/06 19:13:16, dtapuska wrote:
> > Ideally this would have been simple had if we could use the
GestureScrollBegin
> > as the targeting event. For mouse gestures I imagine that you could but I
> wonder
> > if you couldn't on touch gestures. Then the entire gesture sequence could
have
> > been sent there.
> 
> We might be able to only use the GSB once we've got latching for touchpad, but
> that's still in progress.
> 

I talked to Dave about this offline, and I don't think it really simplifies
things because gesture events after a mousewheel are generated by the OOPIF
RenderWidgetHostImpl's InputRouter and sent directly to the renderer process. We
could deal with that but it isn't very clean.

Also, am I correct in thinking that there is currently no way to know if a
scroll is going to be consumed just from seeing the GestureScrollBegin ACK?

[wjmaclean, 2016-07-06 20:07:50]

On 2016/07/06 19:58:44, kenrb wrote:
> Thanks for noticing the ordering problem. As we were saying, it seems like a
> very minor issue if a small number of GestureScrollUpdates get out of order
from
> each other in a long chain, but then I realized that it could also mean the
> GestureScrollEnd getting out of order with a GestureScrollUpdate, which is a
lot
> more problematic.

This is why having the target manage its GSB/GSE state is helpful ... this can
be stored in RWHIER on a per-target basis easily enough.

[tdresser, 2016-07-07 13:45:26]

On 2016/07/06 20:07:50, wjmaclean wrote:
> On 2016/07/06 19:58:44, kenrb wrote:
> > Thanks for noticing the ordering problem. As we were saying, it seems like a
> > very minor issue if a small number of GestureScrollUpdates get out of order
> from
> > each other in a long chain, but then I realized that it could also mean the
> > GestureScrollEnd getting out of order with a GestureScrollUpdate, which is a
> lot
> > more problematic.
> 
> This is why having the target manage its GSB/GSE state is helpful ... this can
> be stored in RWHIER on a per-target basis easily enough.

Once we have scroll latching, we can know whether a scroller will be the target
of the scroll just by looking at the GSB, and scroll will never chain up from
that scroller.

[kenrb, 2016-07-07 15:21:28]

On 2016/07/07 13:45:26, tdresser wrote:
> Once we have scroll latching, we can know whether a scroller will be the
target
> of the scroll just by looking at the GSB, and scroll will never chain up from
> that scroller.

That would be really nice here, looks like. I don't see a clean way to resolve
the race condition Dave noticed, but this design can be fixed quite easily if
GSB triggers the bubbling (including making it consistent with scroll latching
behavior).

How far away is that from landing?

I think the trickiest part about adapting this is that when the
MouseWheelEventQueue for an OOPIF process generates gesture scrolls and they are
not going to be consumed, it needs to make sure the scroll finds its way back up
to be re-routed.

[tdresser, 2016-07-07 15:31:32]

On 2016/07/07 15:21:28, kenrb wrote:
> On 2016/07/07 13:45:26, tdresser wrote:
> > Once we have scroll latching, we can know whether a scroller will be the
> target
> > of the scroll just by looking at the GSB, and scroll will never chain up
from
> > that scroller.
> 
> That would be really nice here, looks like. I don't see a clean way to resolve
> the race condition Dave noticed, but this design can be fixed quite easily if
> GSB triggers the bubbling (including making it consistent with scroll latching
> behavior).
> 
> How far away is that from landing?
> 
> I think the trickiest part about adapting this is that when the
> MouseWheelEventQueue for an OOPIF process generates gesture scrolls and they
are
> not going to be consumed, it needs to make sure the scroll finds its way back
up
> to be re-routed.

We've got an OKR this quarter to make scroll latching work. To actually make the
decision on GSB will be some additional effort.

It's a fair bit of work to get scroll latching in place, as there's a bunch of
platform specific logic to get start/begin signals from touchpads.

[kenrb, 2016-07-07 21:45:29]

Dave and Tim, PTAL?

I added one small change to catch a potentially common race, and I think the
remaining race problems, worst case, will cause a small loss of scroll delta
while bubbling scroll across nested OOPIFs. Also, it has a test now to check
that the earlier 'double-scroll' problem is resolved. There are existing tests
for bubbling and nested bubbling.

There is a comment calling out the need to refactor it after scroll latching
lands.

[tdresser, 2016-07-08 14:35:15]

LGTM (but make sure Dave takes a look at it too).

[dtapuska, 2016-07-08 14:38:21]

On 2016/07/08 14:35:15, tdresser wrote:
> LGTM (but make sure Dave takes a look at it too).

lgtm

[kenrb, 2016-07-08 15:31:42]

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

[kenrb, 2016-07-08 15:31:43]

creis: PTAL? (Or else punt to another content owner if you are too busy)

[Charlie Reis, 2016-07-08 18:31:40]

Thanks for the thorough comments.  Mostly deferring to the previous reviewers,
but otherwise content/ LGTM.

[Charlie Reis, 2016-07-08 20:42:00]

CQ'ing, since this looks ready.

[Charlie Reis, 2016-07-08 20:42:06]

The CQ bit was checked by creis@chromium.org

[Charlie Reis, 2016-07-08 20:42:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
wjmaclean@chromium.org, dtapuska@chromium.org, tdresser@chromium.org
Link to the patchset: https://codereview.chromium.org/2123843002/#ps140001
(title: "Test fix, take 2")

[commit-bot: I haz the power, 2016-07-08 20:42:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-08 21:50:48]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-07-08 21:51:19]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-08 21:51:54]

Description was changed from

==========
Make RWHInputEventRouter manage cross-process scroll bubbling

Mousewheel-based scroll bubbling from OOPIFs to parent frames was broken
in a couple of ways, due to it improperly accounting for the mechanics
of synthetic gesture scroll events from the InputRouter class. This
refactors bubbling to do the following:
- remove bubbling of unused scroll delta from WheelEvent acks, which was
  wrong,
- pass acks from GestureScrollUpdate and GestureScrollEnd events to
  the CrossProcessFrameConnector and then have RWHInputEventRouter do
  most of the reasoning about where to target unused scroll,
- track the current scroll bubbling target, which can change due to
  nested OOPIFs, and manage GestureScrollBegin and GestureScrollEnds
  to each renderer (the previous approach of wrapping each
  GestureScrollUpdate led to bubbled scrolls feeling really slow).

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

to

==========
Make RWHInputEventRouter manage cross-process scroll bubbling

Mousewheel-based scroll bubbling from OOPIFs to parent frames was broken
in a couple of ways, due to it improperly accounting for the mechanics
of synthetic gesture scroll events from the InputRouter class. This
refactors bubbling to do the following:
- remove bubbling of unused scroll delta from WheelEvent acks, which was
  wrong,
- pass acks from GestureScrollUpdate and GestureScrollEnd events to
  the CrossProcessFrameConnector and then have RWHInputEventRouter do
  most of the reasoning about where to target unused scroll,
- track the current scroll bubbling target, which can change due to
  nested OOPIFs, and manage GestureScrollBegin and GestureScrollEnds
  to each renderer (the previous approach of wrapping each
  GestureScrollUpdate led to bubbled scrolls feeling really slow).

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

Committed: https://crrev.com/3bdfea50bdfc6cc2b73c12969ca6207b5949fac1
Cr-Commit-Position: refs/heads/master@{#404500}
==========

[commit-bot: I haz the power, 2016-07-08 21:51:55]

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