Notify OverscrollController of gesture events in plugins.

Notify OverscrollController of gesture events in plugins.

RenderWidgetHostViewGuest resends unconsumed GestureScrollUpdates to
its embedder (via BrowserPluginGuest::ResendEventToEmbedder) which
are then individually wrapped by RenderWidgetHostImpl with
GestureScrollBegins and GestureScrollEnds. Hence OverscrollController
(a) sees a GestureScrollEnd immediately following every
GestureScrollUpdate and ends the overscroll gesture, and (b) does not
see GestureScrollUpdates consumed by the plugin, so
OverscrollController starts an overscroll gesture even though the
content of the plugin was scrolled. Also, OverscrollController does
not have a chance to filter the plugin's gesture events, so even if
an overscroll gesture has started, the plugin can start consuming
scroll updates again.

We now notify OverscrollController (as well as the Mac history
swiper) of the ACKed gesture events from the plugin, so that it
can update its state correctly and allow it to filter the gesture
events of the plugin. They also ignore the wrapper
GestureScrollBegins and GestureScrollEnds as they do not indicate
the beginning/end of the user's gesture.

Note that OOPIF-based guests do not use this resending logic. They
instead rely on RenderWidgetHostViewChildFrame's scroll bubbling
logic. Ensuring this is correct w.r.t. overscroll is being tracked
in crbug.com/713368 .

BUG=694393
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2815823003
Cr-Commit-Position: refs/heads/master@{#476282}
Committed: https://chromium.googlesource.com/chromium/src/+/19fd249f2bd7f2a7ebf6f26a365119ac71e694cb

[Kevin McNee, 2017-04-12 20:32:22]

Description was changed from

==========
Notify OverscrollController of gesture events consumed in plugins.

BrowserPluginGuest resends unconsumed GestureScrollUpdates to its
embedder which are then individually wrapped by RenderWidgetHostImpl
with GestureScrollBegins and GestureScrollEnds.
Hence OverscrollController (a) sees a GestureScrollEnd immediately
following every GestureScrollUpdate and ends the overscroll gesture,
and (b) does not see GestureScrollUpdates consumed by the plugin, so
OverscrollController starts an overscroll gesture even though the
content of the plugin was scrolled.

We now notify OverscrollController of the consumed gesture events
from the plugin, so that it can update its state correctly.

BUG=694393
==========

to

==========
Notify OverscrollController of gesture events consumed in plugins.

BrowserPluginGuest resends unconsumed GestureScrollUpdates to its
embedder which are then individually wrapped by RenderWidgetHostImpl
with GestureScrollBegins and GestureScrollEnds.
Hence OverscrollController (a) sees a GestureScrollEnd immediately
following every GestureScrollUpdate and ends the overscroll gesture,
and (b) does not see GestureScrollUpdates consumed by the plugin, so
OverscrollController starts an overscroll gesture even though the
content of the plugin was scrolled.

We now notify OverscrollController of the consumed gesture events
from the plugin, so that it can update its state correctly.

BUG=694393
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Kevin McNee, 2017-04-12 20:33:30]

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

[commit-bot: I haz the power, 2017-04-12 20:34:43]

Dry run: 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, 2017-04-12 22:11:10]

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

[commit-bot: I haz the power, 2017-04-12 22:11:11]

Dry run: This issue passed the CQ dry run.

[Kevin McNee, 2017-04-13 15:09:25]

Description was changed from

==========
Notify OverscrollController of gesture events consumed in plugins.

BrowserPluginGuest resends unconsumed GestureScrollUpdates to its
embedder which are then individually wrapped by RenderWidgetHostImpl
with GestureScrollBegins and GestureScrollEnds.
Hence OverscrollController (a) sees a GestureScrollEnd immediately
following every GestureScrollUpdate and ends the overscroll gesture,
and (b) does not see GestureScrollUpdates consumed by the plugin, so
OverscrollController starts an overscroll gesture even though the
content of the plugin was scrolled.

We now notify OverscrollController of the consumed gesture events
from the plugin, so that it can update its state correctly.

BUG=694393
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Notify OverscrollController of gesture events consumed in plugins.

BrowserPluginGuest resends unconsumed GestureScrollUpdates to its
embedder which are then individually wrapped by RenderWidgetHostImpl
with GestureScrollBegins and GestureScrollEnds.
Hence OverscrollController (a) sees a GestureScrollEnd immediately
following every GestureScrollUpdate and ends the overscroll gesture,
and (b) does not see GestureScrollUpdates consumed by the plugin, so
OverscrollController starts an overscroll gesture even though the
content of the plugin was scrolled.

We now notify OverscrollController of the consumed gesture events
from the plugin, so that it can update its state correctly.

BUG=694393
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Kevin McNee, 2017-04-13 15:09:25]

mcnee@chromium.org changed reviewers:
+ sadrul@chromium.org

[Kevin McNee, 2017-04-13 15:21:13]

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

[Kevin McNee, 2017-04-13 15:35:14]

Adding kenrb for any comments on the notifying of owners about BrowserPlugin's
consumed gesture events.
Adding sadrul for content/browser/renderer_host
Adding creis as an OWNER for render_widget_host_view_guest.cc

[Kevin McNee, 2017-04-13 20:25:41]

On 2017/04/13 15:35:14, Kevin McNee wrote:
> Adding kenrb for any comments on the notifying of owners about BrowserPlugin's
> consumed gesture events.
> Adding sadrul for content/browser/renderer_host
> Adding creis as an OWNER for render_widget_host_view_guest.cc

I just tried out overscroll navigation with out-of-process iframes and it
appears to have a similar issue. OverscrollController doesn't see the consumed
GestureScrollUpdates in the child, so it goes straight into an overscroll
gesture, and it doesn't see GestureFlingStarts, so it doesn't complete the
overscroll gesture. (Although it does see GestureScrollEnds since those are
bubbled) Would it make sense to extend this from notifying when a plugin
consumes a gesture event to when an OOPIF consumes a gesture event as well?

[kenrb, 2017-04-13 21:08:57]

On 2017/04/13 20:25:41, Kevin McNee wrote:
> On 2017/04/13 15:35:14, Kevin McNee wrote:
> > Adding kenrb for any comments on the notifying of owners about
BrowserPlugin's
> > consumed gesture events.
> > Adding sadrul for content/browser/renderer_host
> > Adding creis as an OWNER for render_widget_host_view_guest.cc
> 
> I just tried out overscroll navigation with out-of-process iframes and it
> appears to have a similar issue. OverscrollController doesn't see the consumed
> GestureScrollUpdates in the child, so it goes straight into an overscroll
> gesture, and it doesn't see GestureFlingStarts, so it doesn't complete the
> overscroll gesture. (Although it does see GestureScrollEnds since those are
> bubbled) Would it make sense to extend this from notifying when a plugin
> consumes a gesture event to when an OOPIF consumes a gesture event as well?

For OOPIFs I think it would be better just to proxy the ACKs up to
RenderWidgetHostViewAura::GestureEventAck (via CrossProcessFrameConnector). I
would expect that to work better because GSUs are not individually wrapped by
GSBs and GSEs.

What you are doing here for RWHVG is probably okay, given the GSB/GSE problem.
The bug is flagged as also applying to Mac, though. Are you planning on fixing
it there as well?

Also, if you are adding Guest-specific code to RWHVA and OverscrollController,
it would be a good idea to tag it with comments to be deleted after RWHVG is
removed.

[Kevin McNee, 2017-04-19 20:59:51]

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

[commit-bot: I haz the power, 2017-04-19 21:00:25]

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

[Kevin McNee, 2017-04-19 21:43:32]

On 2017/04/13 21:08:57, kenrb wrote:
> On 2017/04/13 20:25:41, Kevin McNee wrote:
> > On 2017/04/13 15:35:14, Kevin McNee wrote:
> > > Adding kenrb for any comments on the notifying of owners about
> BrowserPlugin's
> > > consumed gesture events.
> > > Adding sadrul for content/browser/renderer_host
> > > Adding creis as an OWNER for render_widget_host_view_guest.cc
> > 
> > I just tried out overscroll navigation with out-of-process iframes and it
> > appears to have a similar issue. OverscrollController doesn't see the
consumed
> > GestureScrollUpdates in the child, so it goes straight into an overscroll
> > gesture, and it doesn't see GestureFlingStarts, so it doesn't complete the
> > overscroll gesture. (Although it does see GestureScrollEnds since those are
> > bubbled) Would it make sense to extend this from notifying when a plugin
> > consumes a gesture event to when an OOPIF consumes a gesture event as well?
> 
> For OOPIFs I think it would be better just to proxy the ACKs up to
> RenderWidgetHostViewAura::GestureEventAck (via CrossProcessFrameConnector). I
> would expect that to work better because GSUs are not individually wrapped by
> GSBs and GSEs.
> 
> What you are doing here for RWHVG is probably okay, given the GSB/GSE problem.
> The bug is flagged as also applying to Mac, though. Are you planning on fixing
> it there as well?
> 
> Also, if you are adding Guest-specific code to RWHVA and OverscrollController,
> it would be a good idea to tag it with comments to be deleted after RWHVG is
> removed.

So it turns out the root RWHV needs to be able to filter gesture events so that
when we're in an overscroll gesture, the child can't start consuming scroll
events again. So I did as kenrb suggested and talked to the root RWHV via
CrossProcessFrameConnector so that it can filter gestures and receive ACKs. That
solves the issue for OOPIFs. I then rewrote what I had for plugins to be more
like what we're doing for OOPIFs. So this CL now covers overscroll issues for
Aura for plugins and OOPIFs.

> The bug is flagged as also applying to Mac, though. Are you planning on fixing
> it there as well?
Yeah, once I get access to a Mac, I can fix it there. It'll hopefully just be a
matter of hooking up the platform specific code.

Android is affected by this as well. The overscroll pull-to-refresh doesn't work
properly when you start scrolling from an OOPIF. I've filed a separate bug for
OOPIF overscroll issues: crbug.com/713368

> Also, if you are adding Guest-specific code to RWHVA and OverscrollController,
> it would be a good idea to tag it with comments to be deleted after RWHVG is
> removed.
Done.

[Kevin McNee, 2017-04-19 21:52:04]

Description was changed from

==========
Notify OverscrollController of gesture events consumed in plugins.

BrowserPluginGuest resends unconsumed GestureScrollUpdates to its
embedder which are then individually wrapped by RenderWidgetHostImpl
with GestureScrollBegins and GestureScrollEnds.
Hence OverscrollController (a) sees a GestureScrollEnd immediately
following every GestureScrollUpdate and ends the overscroll gesture,
and (b) does not see GestureScrollUpdates consumed by the plugin, so
OverscrollController starts an overscroll gesture even though the
content of the plugin was scrolled.

We now notify OverscrollController of the consumed gesture events
from the plugin, so that it can update its state correctly.

BUG=694393
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Notify OverscrollController of gesture events in plugins/OOPIFs.

BrowserPluginGuest resends unconsumed GestureScrollUpdates to its
embedder which are then individually wrapped by RenderWidgetHostImpl
with GestureScrollBegins and GestureScrollEnds.
Hence OverscrollController (a) sees a GestureScrollEnd immediately
following every GestureScrollUpdate and ends the overscroll gesture,
and (b) does not see GestureScrollUpdates consumed by the plugin, so
OverscrollController starts an overscroll gesture even though the
content of the plugin was scrolled. Also, OverscrollController does
not have a chance to filter the plugin's gesture events, so even if
an overscroll gesture has started, the plugin can start consuming
scroll updates again.

OOPIFs experience similar issues. OverscrollController does not see
flings or GestureScrollUpdates consumed by the child and does not
filter the child's gesture events.

We now notify OverscrollController of the ACKed gesture events
from the plugin/OOPIF, so that it can update its state correctly and
allow it to filter the gesture events of the plugin/OOPIF.

BUG=694393, 713368
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2017-04-19 22:05:03]

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

[commit-bot: I haz the power, 2017-04-19 22:05:04]

Dry run: This issue passed the CQ dry run.

[Kevin McNee, 2017-04-25 22:19:00]

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

[commit-bot: I haz the power, 2017-04-25 22:20:07]

Dry run: 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, 2017-04-26 00:18:32]

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

[commit-bot: I haz the power, 2017-04-26 00:18:33]

Dry run: This issue passed the CQ dry run.

[Kevin McNee, 2017-05-04 20:16:36]

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

[commit-bot: I haz the power, 2017-05-04 20:17:06]

Dry run: CQ is trying da patch.

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

[Kevin McNee, 2017-05-04 21:05:04]

Description was changed from

==========
Notify OverscrollController of gesture events in plugins/OOPIFs.

BrowserPluginGuest resends unconsumed GestureScrollUpdates to its
embedder which are then individually wrapped by RenderWidgetHostImpl
with GestureScrollBegins and GestureScrollEnds.
Hence OverscrollController (a) sees a GestureScrollEnd immediately
following every GestureScrollUpdate and ends the overscroll gesture,
and (b) does not see GestureScrollUpdates consumed by the plugin, so
OverscrollController starts an overscroll gesture even though the
content of the plugin was scrolled. Also, OverscrollController does
not have a chance to filter the plugin's gesture events, so even if
an overscroll gesture has started, the plugin can start consuming
scroll updates again.

OOPIFs experience similar issues. OverscrollController does not see
flings or GestureScrollUpdates consumed by the child and does not
filter the child's gesture events.

We now notify OverscrollController of the ACKed gesture events
from the plugin/OOPIF, so that it can update its state correctly and
allow it to filter the gesture events of the plugin/OOPIF.

BUG=694393, 713368
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Notify OverscrollController of gesture events in plugins.

BrowserPluginGuest resends unconsumed GestureScrollUpdates to its
embedder which are then individually wrapped by RenderWidgetHostImpl
with GestureScrollBegins and GestureScrollEnds.
Hence OverscrollController (a) sees a GestureScrollEnd immediately
following every GestureScrollUpdate and ends the overscroll gesture,
and (b) does not see GestureScrollUpdates consumed by the plugin, so
OverscrollController starts an overscroll gesture even though the
content of the plugin was scrolled. Also, OverscrollController does
not have a chance to filter the plugin's gesture events, so even if
an overscroll gesture has started, the plugin can start consuming
scroll updates again.

We now notify OverscrollController (as well as the Mac history
swiper) of the ACKed gesture events from the plugin, so that it
can update its state correctly and allow it to filter the gesture
events of the plugin. They also ignore the wrapper
GestureScrollBegins and GestureScrollEnds as they do not indicate
the beginning/end of the user's gesture.

BUG=694393
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2017-05-04 21:10:11]

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

[commit-bot: I haz the power, 2017-05-04 21:10:12]

Dry run: This issue passed the CQ dry run.

[Kevin McNee, 2017-05-04 21:17:47]

On 2017/04/19 21:43:32, Kevin McNee wrote:
> On 2017/04/13 21:08:57, kenrb wrote:
> > On 2017/04/13 20:25:41, Kevin McNee wrote:
> > > On 2017/04/13 15:35:14, Kevin McNee wrote:
> > > > Adding kenrb for any comments on the notifying of owners about
> > BrowserPlugin's
> > > > consumed gesture events.
> > > > Adding sadrul for content/browser/renderer_host
> > > > Adding creis as an OWNER for render_widget_host_view_guest.cc
> > > 
> > > I just tried out overscroll navigation with out-of-process iframes and it
> > > appears to have a similar issue. OverscrollController doesn't see the
> consumed
> > > GestureScrollUpdates in the child, so it goes straight into an overscroll
> > > gesture, and it doesn't see GestureFlingStarts, so it doesn't complete the
> > > overscroll gesture. (Although it does see GestureScrollEnds since those
are
> > > bubbled) Would it make sense to extend this from notifying when a plugin
> > > consumes a gesture event to when an OOPIF consumes a gesture event as
well?
> > 
> > For OOPIFs I think it would be better just to proxy the ACKs up to
> > RenderWidgetHostViewAura::GestureEventAck (via CrossProcessFrameConnector).
I
> > would expect that to work better because GSUs are not individually wrapped
by
> > GSBs and GSEs.
> > 
> > What you are doing here for RWHVG is probably okay, given the GSB/GSE
problem.
> > The bug is flagged as also applying to Mac, though. Are you planning on
fixing
> > it there as well?
> > 
> > Also, if you are adding Guest-specific code to RWHVA and
OverscrollController,
> > it would be a good idea to tag it with comments to be deleted after RWHVG is
> > removed.
> 
> So it turns out the root RWHV needs to be able to filter gesture events so
that
> when we're in an overscroll gesture, the child can't start consuming scroll
> events again. So I did as kenrb suggested and talked to the root RWHV via
> CrossProcessFrameConnector so that it can filter gestures and receive ACKs.
That
> solves the issue for OOPIFs. I then rewrote what I had for plugins to be more
> like what we're doing for OOPIFs. So this CL now covers overscroll issues for
> Aura for plugins and OOPIFs.
> 
> > The bug is flagged as also applying to Mac, though. Are you planning on
fixing
> > it there as well?
> Yeah, once I get access to a Mac, I can fix it there. It'll hopefully just be
a
> matter of hooking up the platform specific code.
> 
> Android is affected by this as well. The overscroll pull-to-refresh doesn't
work
> properly when you start scrolling from an OOPIF. I've filed a separate bug for
> OOPIF overscroll issues: crbug.com/713368
> 
> > Also, if you are adding Guest-specific code to RWHVA and
OverscrollController,
> > it would be a good idea to tag it with comments to be deleted after RWHVG is
> > removed.
> Done.

Okay, so it turns out that OOPIF overscroll issues are mainly caused by a
difference in scroll bubbling behaviour between OOPIF and non-OOPIF. That'll
still be tracked in https://bugs.chromium.org/p/chromium/issues/detail?id=713368
but I'm switching this CL back to covering the BrowserPlugin case only.

I've also fixed the issue on Mac, but there was another issue with mouse wheel
phase information being dropped that was necessary to fix in order for what I've
done here to work properly. So this CL is now dependent on
https://codereview.chromium.org/2866523002/

kenrb: any comments before I get owners to look at this?

[kenrb, 2017-05-05 15:26:23]

lgtm

[Kevin McNee, 2017-05-05 18:31:21]

mcnee@chromium.org changed reviewers:
+ erikchen@chromium.org

[Kevin McNee, 2017-05-05 18:39:25]

creis, sadrul, erikchen: Now ready for owner reviews. PTAL.

creis for content/browser/frame_host
sadrul for content/browser/renderer_host
erikchen for chrome_render_widget_host_view_mac_history_swiper.mm

Thanks.

[erikchen, 2017-05-05 19:15:37]

https://codereview.chromium.org/2815823003/diff/100001/chrome/browser/rendere...
File
chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper.mm
(right):

https://codereview.chromium.org/2815823003/diff/100001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper.mm:139:
if (event.resending_plugin_id != -1) {
this class is a state machine, and I'm a little bit worried about it ending in
an inconsistent state if there's another layer on top of it consuming and/or
generating events. Can you confirm that we correctly propagate state for the
gesture and touch APIs, e.g. beginGestureWithEvent: and touchesBeganWithEvent:

[Charlie Reis, 2017-05-05 23:45:44]

Similar to https://codereview.chromium.org/2866523002/, is there a test you can
add for this?  Otherwise looks good with nits.

https://codereview.chromium.org/2815823003/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/2815823003/diff/100001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:671: // them
after the root has started an overscroll.
Is there a bug associated with this that you can list here?

https://codereview.chromium.org/2815823003/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_guest.cc (right):

https://codereview.chromium.org/2815823003/diff/100001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_guest.cc:559: else if
(event.GetType() == blink::WebInputEvent::kGestureScrollUpdate ||
Style nit: I think it would improve readability to include braces on both the if
and else, even though each body is one line.

[Kevin McNee, 2017-05-16 16:24:23]

https://codereview.chromium.org/2815823003/diff/100001/chrome/browser/rendere...
File
chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper.mm
(right):

https://codereview.chromium.org/2815823003/diff/100001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper.mm:139:
if (event.resending_plugin_id != -1) {
On 2017/05/05 19:15:36, erikchen wrote:
> this class is a state machine, and I'm a little bit worried about it ending in
> an inconsistent state if there's another layer on top of it consuming and/or
> generating events. Can you confirm that we correctly propagate state for the
> gesture and touch APIs, e.g. beginGestureWithEvent: and touchesBeganWithEvent:

With a guest, the sequence of calls to touches*WithEvent and *GestureWithEvent
is the same as with a normal page. Looking at a stack trace, these events are
not coming up from the guest.

[erikchen, 2017-05-17 04:42:40]

thanks,
	chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper.mm
lgtm

[Kevin McNee, 2017-05-17 22:39:34]

Once I've covered the OOPIF case, it'll make sense to add a
SitePerProcessBrowserTest for overscrolling as it's related to the scroll
bubbling logic being tested there.

https://codereview.chromium.org/2815823003/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/2815823003/diff/100001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:671: // them
after the root has started an overscroll.
On 2017/05/05 23:45:44, Charlie Reis wrote:
> Is there a bug associated with this that you can list here?

Done.

https://codereview.chromium.org/2815823003/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_guest.cc (right):

https://codereview.chromium.org/2815823003/diff/100001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_guest.cc:559: else if
(event.GetType() == blink::WebInputEvent::kGestureScrollUpdate ||
On 2017/05/05 23:45:44, Charlie Reis wrote:
> Style nit: I think it would improve readability to include braces on both the
if
> and else, even though each body is one line.

Done.

[Kevin McNee, 2017-05-17 22:41:41]

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

[commit-bot: I haz the power, 2017-05-17 22:42:51]

Dry run: 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, 2017-05-17 23:18:25]

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

[commit-bot: I haz the power, 2017-05-17 23:18:26]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Kevin McNee, 2017-05-18 14:19:31]

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

[commit-bot: I haz the power, 2017-05-18 14:19:59]

Dry run: 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, 2017-05-18 14:24:09]

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

[commit-bot: I haz the power, 2017-05-18 14:24:10]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Kevin McNee, 2017-05-18 15:02:53]

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

[commit-bot: I haz the power, 2017-05-18 15:03:47]

Dry run: 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, 2017-05-18 15:59:36]

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

[commit-bot: I haz the power, 2017-05-18 15:59:37]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Kevin McNee, 2017-05-18 16:05:12]

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

[commit-bot: I haz the power, 2017-05-18 16:05:45]

Dry run: 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, 2017-05-18 17:09:39]

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

[commit-bot: I haz the power, 2017-05-18 17:09:39]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2017-05-19 17:07:04]

On 2017/05/17 22:39:34, Kevin McNee wrote:
> Once I've covered the OOPIF case, it'll make sense to add a
> SitePerProcessBrowserTest for overscrolling as it's related to the scroll
> bubbling logic being tested there.

What do you mean by "Once I've covered the OOPIF case?"  Is this CL only fixing
BrowserPlugin?  That doesn't come across in the CL description, since I think
BrowserPluginGuest is used by OOPIF-based webviews as well.  Please update the
description if there's more to do for OOPIFs.

Otherwise LGTM!

[Kevin McNee, 2017-05-19 19:35:00]

Description was changed from

==========
Notify OverscrollController of gesture events in plugins.

BrowserPluginGuest resends unconsumed GestureScrollUpdates to its
embedder which are then individually wrapped by RenderWidgetHostImpl
with GestureScrollBegins and GestureScrollEnds.
Hence OverscrollController (a) sees a GestureScrollEnd immediately
following every GestureScrollUpdate and ends the overscroll gesture,
and (b) does not see GestureScrollUpdates consumed by the plugin, so
OverscrollController starts an overscroll gesture even though the
content of the plugin was scrolled. Also, OverscrollController does
not have a chance to filter the plugin's gesture events, so even if
an overscroll gesture has started, the plugin can start consuming
scroll updates again.

We now notify OverscrollController (as well as the Mac history
swiper) of the ACKed gesture events from the plugin, so that it
can update its state correctly and allow it to filter the gesture
events of the plugin. They also ignore the wrapper
GestureScrollBegins and GestureScrollEnds as they do not indicate
the beginning/end of the user's gesture.

BUG=694393
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Notify OverscrollController of gesture events in plugins.

RenderWidgetHostViewGuest resends unconsumed GestureScrollUpdates to
its embedder (via BrowserPluginGuest::ResendEventToEmbedder) which
are then individually wrapped by RenderWidgetHostImpl with
GestureScrollBegins and GestureScrollEnds. Hence OverscrollController
(a) sees a GestureScrollEnd immediately following every
GestureScrollUpdate and ends the overscroll gesture, and (b) does not
see GestureScrollUpdates consumed by the plugin, so
OverscrollController starts an overscroll gesture even though the
content of the plugin was scrolled. Also, OverscrollController does
not have a chance to filter the plugin's gesture events, so even if
an overscroll gesture has started, the plugin can start consuming
scroll updates again.

We now notify OverscrollController (as well as the Mac history
swiper) of the ACKed gesture events from the plugin, so that it
can update its state correctly and allow it to filter the gesture
events of the plugin. They also ignore the wrapper
GestureScrollBegins and GestureScrollEnds as they do not indicate
the beginning/end of the user's gesture.

Note that OOPIF-based guests do not use this resending logic. They
instead rely on RenderWidgetHostViewChildFrame's scroll bubbling
logic. Ensuring this is correct w.r.t. overscroll is being tracked
in crbug.com/713368 .

BUG=694393
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Kevin McNee, 2017-05-19 19:46:11]

On 2017/05/19 17:07:04, Charlie Reis wrote:
> On 2017/05/17 22:39:34, Kevin McNee wrote:
> > Once I've covered the OOPIF case, it'll make sense to add a
> > SitePerProcessBrowserTest for overscrolling as it's related to the scroll
> > bubbling logic being tested there.
> 
> What do you mean by "Once I've covered the OOPIF case?"  Is this CL only
fixing
> BrowserPlugin?  That doesn't come across in the CL description, since I think
> BrowserPluginGuest is used by OOPIF-based webviews as well.  Please update the
> description if there's more to do for OOPIFs.
> 
> Otherwise LGTM!

Sorry, I should have written "RenderWidgetHostViewGuest via BrowserPluginGuest".
Fixing overscroll for OOPIF-based webviews (and OOPIFs in general) involves
correcting RenderWidgetHostViewChildFrame's scroll bubbling logic (
crbug.com/713368 ) as they don't use RenderWidgetHostViewGuest's resending
logic. I've updated the description accordingly.

[Charlie Reis, 2017-05-19 20:46:19]

On 2017/05/19 19:46:11, Kevin McNee wrote:
> On 2017/05/19 17:07:04, Charlie Reis wrote:
> > On 2017/05/17 22:39:34, Kevin McNee wrote:
> > > Once I've covered the OOPIF case, it'll make sense to add a
> > > SitePerProcessBrowserTest for overscrolling as it's related to the scroll
> > > bubbling logic being tested there.
> > 
> > What do you mean by "Once I've covered the OOPIF case?"  Is this CL only
> fixing
> > BrowserPlugin?  That doesn't come across in the CL description, since I
think
> > BrowserPluginGuest is used by OOPIF-based webviews as well.  Please update
the
> > description if there's more to do for OOPIFs.
> > 
> > Otherwise LGTM!
> 
> Sorry, I should have written "RenderWidgetHostViewGuest via
BrowserPluginGuest".
> Fixing overscroll for OOPIF-based webviews (and OOPIFs in general) involves
> correcting RenderWidgetHostViewChildFrame's scroll bubbling logic (
> crbug.com/713368 ) as they don't use RenderWidgetHostViewGuest's resending
> logic. I've updated the description accordingly.

Thanks-- that helps!

[Kevin McNee, 2017-05-23 15:21:03]

ping sadrul

I have all the LGTMs needed for this, but did you want to take a look at the
aura changes before I commit?

[sadrul, 2017-05-24 22:22:27]

lgtm

Is it at all possible to write some tests? This all seems fairly subtle. Not
having test coverage means this will break very easily.

[Kevin McNee, 2017-05-26 21:06:01]

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

[commit-bot: I haz the power, 2017-05-26 21:06:34]

Dry run: 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, 2017-05-26 23:24:28]

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

[commit-bot: I haz the power, 2017-05-26 23:24:29]

Dry run: This issue passed the CQ dry run.

[Kevin McNee, 2017-05-29 14:52:22]

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

[Kevin McNee, 2017-05-29 15:09:51]

On 2017/05/24 22:22:27, sadrul wrote:
> lgtm
> 
> Is it at all possible to write some tests? This all seems fairly subtle. Not
> having test coverage means this will break very easily.

sadrul: Okay, so this should at least prevent this from regressing again until
we're off of BrowserPlugin. Look good?

Adding wjmaclean for chrome/browser/apps/guest_view/web_view_browsertest.cc

creis: I made changes to content/public/test/browser_test_utils.h/cc. Look good?

[wjmaclean, 2017-05-29 15:28:03]

lgtm

[sadrul, 2017-05-29 16:53:44]

Thank you for adding the test!

[Charlie Reis, 2017-05-30 22:35:17]

Hmm, it seems like there's a lot of unfortunate changes that the test required. 
If all of those (labeled with TODOs) can go away when BrowserPlugin is removed
(and assuming we still have test coverage for the OOPIF case), then I guess I'm
ok with it.

LGTM with nits.

https://codereview.chromium.org/2815823003/diff/200001/content/public/test/br...
File content/public/test/browser_test_utils.h (right):

https://codereview.chromium.org/2815823003/diff/200001/content/public/test/br...
content/public/test/browser_test_utils.h:857: // (crbug.com/533069).
I don't understand this TODO.  Can you rephrase it to reflect what the action
should be?  (I'm not sure if it should be removed when BrowserPlugin is removed,
or if it's saying that it should be used in OOPIF-based guest tests in the
future as well.)

https://codereview.chromium.org/2815823003/diff/200001/content/public/test/br...
content/public/test/browser_test_utils.h:858: class OverscrollControllerSpy {
Is spy a design pattern?  I don't many instances of it in the code, so I'm
curious about the choice of name.  Maybe it's worth mentioning something about
it in the comment if more common names like Observer aren't the right fit.

https://codereview.chromium.org/2815823003/diff/200001/content/public/test/br...
content/public/test/browser_test_utils.h:866: virtual void
WaitForScrollStateContentScrolling() = 0;
Needs a comment.

[Kevin McNee, 2017-05-31 17:34:07]

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

[commit-bot: I haz the power, 2017-05-31 17:34:35]

Dry run: 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, 2017-05-31 17:42:40]

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

[commit-bot: I haz the power, 2017-05-31 17:42:41]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Kevin McNee, 2017-05-31 17:59:22]

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

[commit-bot: I haz the power, 2017-05-31 17:59:47]

Dry run: CQ is trying da patch.

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

[Kevin McNee, 2017-05-31 18:07:36]

https://codereview.chromium.org/2815823003/diff/200001/content/public/test/br...
File content/public/test/browser_test_utils.h (right):

https://codereview.chromium.org/2815823003/diff/200001/content/public/test/br...
content/public/test/browser_test_utils.h:857: // (crbug.com/533069).
On 2017/05/30 22:35:17, Charlie Reis wrote:
> I don't understand this TODO.  Can you rephrase it to reflect what the action
> should be?  (I'm not sure if it should be removed when BrowserPlugin is
removed,
> or if it's saying that it should be used in OOPIF-based guest tests in the
> future as well.)

Done.

https://codereview.chromium.org/2815823003/diff/200001/content/public/test/br...
content/public/test/browser_test_utils.h:858: class OverscrollControllerSpy {
On 2017/05/30 22:35:17, Charlie Reis wrote:
> Is spy a design pattern?  I don't many instances of it in the code, so I'm
> curious about the choice of name.  Maybe it's worth mentioning something about
> it in the comment if more common names like Observer aren't the right fit.

It's essentially a partial mock. We're inspecting the calls to a real
implementation rather than an interface. I've changed the name to a mock and
made note of this in a comment.

https://codereview.chromium.org/2815823003/diff/200001/content/public/test/br...
content/public/test/browser_test_utils.h:866: virtual void
WaitForScrollStateContentScrolling() = 0;
On 2017/05/30 22:35:17, Charlie Reis wrote:
> Needs a comment.

Done.

[commit-bot: I haz the power, 2017-05-31 20:18:24]

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

[commit-bot: I haz the power, 2017-05-31 20:18:25]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2017-05-31 21:21:38]

Thanks!  LGTM.

[Kevin McNee, 2017-06-01 14:37:24]

The CQ bit was checked by mcnee@chromium.org

[Kevin McNee, 2017-06-01 14:37:25]

The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org,
erikchen@chromium.org, sadrul@chromium.org, wjmaclean@chromium.org
Link to the patchset: https://codereview.chromium.org/2815823003/#ps220001
(title: "Address comments.")

[commit-bot: I haz the power, 2017-06-01 14:37:47]

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, 2017-06-01 14:42:47]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1496327844513890,
"parent_rev": "1e701e32a7785efb6b77d42e6d2b838b4d4590c3", "commit_rev":
"19fd249f2bd7f2a7ebf6f26a365119ac71e694cb"}

[commit-bot: I haz the power, 2017-06-01 14:42:56]

Description was changed from

==========
Notify OverscrollController of gesture events in plugins.

RenderWidgetHostViewGuest resends unconsumed GestureScrollUpdates to
its embedder (via BrowserPluginGuest::ResendEventToEmbedder) which
are then individually wrapped by RenderWidgetHostImpl with
GestureScrollBegins and GestureScrollEnds. Hence OverscrollController
(a) sees a GestureScrollEnd immediately following every
GestureScrollUpdate and ends the overscroll gesture, and (b) does not
see GestureScrollUpdates consumed by the plugin, so
OverscrollController starts an overscroll gesture even though the
content of the plugin was scrolled. Also, OverscrollController does
not have a chance to filter the plugin's gesture events, so even if
an overscroll gesture has started, the plugin can start consuming
scroll updates again.

We now notify OverscrollController (as well as the Mac history
swiper) of the ACKed gesture events from the plugin, so that it
can update its state correctly and allow it to filter the gesture
events of the plugin. They also ignore the wrapper
GestureScrollBegins and GestureScrollEnds as they do not indicate
the beginning/end of the user's gesture.

Note that OOPIF-based guests do not use this resending logic. They
instead rely on RenderWidgetHostViewChildFrame's scroll bubbling
logic. Ensuring this is correct w.r.t. overscroll is being tracked
in crbug.com/713368 .

BUG=694393
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Notify OverscrollController of gesture events in plugins.

RenderWidgetHostViewGuest resends unconsumed GestureScrollUpdates to
its embedder (via BrowserPluginGuest::ResendEventToEmbedder) which
are then individually wrapped by RenderWidgetHostImpl with
GestureScrollBegins and GestureScrollEnds. Hence OverscrollController
(a) sees a GestureScrollEnd immediately following every
GestureScrollUpdate and ends the overscroll gesture, and (b) does not
see GestureScrollUpdates consumed by the plugin, so
OverscrollController starts an overscroll gesture even though the
content of the plugin was scrolled. Also, OverscrollController does
not have a chance to filter the plugin's gesture events, so even if
an overscroll gesture has started, the plugin can start consuming
scroll updates again.

We now notify OverscrollController (as well as the Mac history
swiper) of the ACKed gesture events from the plugin, so that it
can update its state correctly and allow it to filter the gesture
events of the plugin. They also ignore the wrapper
GestureScrollBegins and GestureScrollEnds as they do not indicate
the beginning/end of the user's gesture.

Note that OOPIF-based guests do not use this resending logic. They
instead rely on RenderWidgetHostViewChildFrame's scroll bubbling
logic. Ensuring this is correct w.r.t. overscroll is being tracked
in crbug.com/713368 .

BUG=694393
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2815823003
Cr-Commit-Position: refs/heads/master@{#476282}
Committed:
https://chromium.googlesource.com/chromium/src/+/19fd249f2bd7f2a7ebf6f26a3651...
==========

[commit-bot: I haz the power, 2017-06-01 14:42:58]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/19fd249f2bd7f2a7ebf6f26a3651...