Resend unconsumed scroll update from guest back to embedder (WebView Scroll Bubble).

Resend unconsumed scroll updates from guest back to embedder.

(WebView scroll bubbling)

This CL provides a pathway for a guest process to return
unconsumed scroll updates to its embedder, in order to allow
scroll bubbling. The events are marked with the embedder's
BrowserPlugin instance id so that when the BrowserPlugin sees
the re-sent event it will know to mark it as not-handled so
the normal bubbling process can occur within Blink.

Note that any scroll event that targets a WebView guest is,
by definition, on Blink's main-thread so that it can be
processed by BrowserPlugin and forwarded to the guest.

ScrollUpdates going to the guest outside of a
GestureScrollBegin and GestureScrollEnd/GestureFLingStart
pair are surpressed: these are generated by fling curves, but
will cause problems in the guest's InputHandlerProxy since it
never expects to get them.

This CL depends on

https://codereview.chromium.org/1308313005/

landing first as it provides the necessary modifications to
events to (1) carry the resend source info, and (2) forward
GestureFlingStart to the guest via BrowserPlugin.

Finally: note that when WebView-tag transitions to OOPIF that
much of this code will be redundant and can be removed as
part of the larger effort to cleanup (i.e. remove)
BrowserPlugin.

BUG=491940
Committed: https://crrev.com/73b15a29e89b0150b8ba5ed4b43af298d30bc3da
Cr-Commit-Position: refs/heads/master@{#350838}

[wjmaclean, 2015-09-11 13:22:50]

wjmaclean@chromium.org changed reviewers:
+ creis@chromium.org, rjkroege@chromium.org

[wjmaclean, 2015-09-11 19:14:56]

creis@ and rjkroege@ -

I think this is ready for regular review now ... please take a look?

[Charlie Reis, 2015-09-11 19:42:49]

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

[Charlie Reis, 2015-09-11 19:42:50]

Maybe kenrb@ would be a better reviewer than me?  I'm not as deeply
knowledgeable about RWH{V}.

[wjmaclean, 2015-09-11 19:49:01]

On 2015/09/11 19:42:50, Charlie Reis (slow till 9-14) wrote:
> Maybe kenrb@ would be a better reviewer than me?  I'm not as deeply
> knowledgeable about RWH{V}.

kenrb@ - are you able to take a look?

[kenrb, 2015-09-11 20:34:26]

I can, but I probably can't finish this before Monday morning.

[wjmaclean, 2015-09-11 20:35:01]

On 2015/09/11 20:34:26, kenrb wrote:
> I can, but I probably can't finish this before Monday morning.

Sure, that's fine :-)

[kenrb, 2015-09-14 15:48:51]

https://codereview.chromium.org/1308273003/diff/80001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/1308273003/diff/80001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:481: RenderViewHost* rvh
= embedder_web_contents()->GetRenderViewHost();
I would prefer if you accessed the main RenderFrameHost rather than the
RenderViewHost. i.e.
RenderWidgetHostImpl* host =
embedder_web_contents()->GetMainFrame()->GetRenderWidgetHost();

It helps us move toward removing the relationship between RenderViewHost and
RenderWidgetHost.

https://codereview.chromium.org/1308273003/diff/80001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/1308273003/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.h:462: bool
is_in_gesture_scroll() { return is_in_gesture_scroll_; }
Is there a way to avoid exposing this and keeping the logic entirely in
RenderWidgetHostImpl? It would be cleaner if that was possible, in particular
because then BrowserPluginGuest could call Process*Event() methods on
RenderWidgetHostView instead of Forward*Event() methods on RenderWidgetHostImpl
(which isn't possible now because ProcessGestureEvent doesn't exist yet, but I'm
thinking forward a bit).

[wjmaclean, 2015-09-14 20:47:21]

https://codereview.chromium.org/1308273003/diff/80001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/1308273003/diff/80001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:481: RenderViewHost* rvh
= embedder_web_contents()->GetRenderViewHost();
On 2015/09/14 15:48:51, kenrb wrote:
> I would prefer if you accessed the main RenderFrameHost rather than the
> RenderViewHost. i.e.
> RenderWidgetHostImpl* host =
> embedder_web_contents()->GetMainFrame()->GetRenderWidgetHost();
> 
> It helps us move toward removing the relationship between RenderViewHost and
> RenderWidgetHost.

Done.

https://codereview.chromium.org/1308273003/diff/80001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/1308273003/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.h:462: bool
is_in_gesture_scroll() { return is_in_gesture_scroll_; }
On 2015/09/14 15:48:51, kenrb wrote:
> Is there a way to avoid exposing this and keeping the logic entirely in
> RenderWidgetHostImpl? It would be cleaner if that was possible, in particular
> because then BrowserPluginGuest could call Process*Event() methods on
> RenderWidgetHostView instead of Forward*Event() methods on
RenderWidgetHostImpl
> (which isn't possible now because ProcessGestureEvent doesn't exist yet, but
I'm
> thinking forward a bit).

Yes, but it involves detecting |resendSource| (now renamed |resendingPluginId|)
in the Forward*Event()s, and if necessary creating the synthetic
GestureScrollBegin/End within RenderWidgetHostImpl.

My original idea had been to keep as much of the machinery as possible in
BrowserPluginGuest to make it easier removing this later on (once OOPIF
scrolling works, this code will be unnecessary).

If you're OK with moving the extra baggage into RWHI then I'll go ahead and do
that.

[kenrb, 2015-09-15 18:10:58]

Yes I'm okay with it. lgtm

[wjmaclean, 2015-09-17 20:09:15]

creis@ - I've uploaded the refactoring changes requested by kenrb@ ... can you
please do an OWNER's review?

Note: the required Blink-side changes have now landed:
https://codereview.chromium.org/1308313005/

[wjmaclean, 2015-09-17 20:09:23]

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

[commit-bot: I haz the power, 2015-09-17 20:09:58]

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

[commit-bot: I haz the power, 2015-09-17 20:42:45]

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

[commit-bot: I haz the power, 2015-09-17 20:42:46]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Charlie Reis, 2015-09-18 04:30:36]

Looks like there's some relevant test failures.  Otherwise, I'll give a rubber
stamp LGTM with nits, based on Ken's review.

Will this be handled in OOPIFs in a different way?

https://codereview.chromium.org/1308273003/diff/120001/chrome/test/data/exten...
File
chrome/test/data/extensions/platform_apps/web_view/scrollable_embedder_and_guest/guest.js
(right):

https://codereview.chromium.org/1308273003/diff/120001/chrome/test/data/exten...
chrome/test/data/extensions/platform_apps/web_view/scrollable_embedder_and_guest/guest.js:1:
// Copyright 2014 The Chromium Authors. All rights reserved.
nit: 2015

https://codereview.chromium.org/1308273003/diff/120001/chrome/test/data/exten...
File
chrome/test/data/extensions/platform_apps/web_view/scrollable_embedder_and_guest/main.js
(right):

https://codereview.chromium.org/1308273003/diff/120001/chrome/test/data/exten...
chrome/test/data/extensions/platform_apps/web_view/scrollable_embedder_and_guest/main.js:10:
LOG('Hi!!');
Needed?

https://codereview.chromium.org/1308273003/diff/120001/content/browser/browse...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/1308273003/diff/120001/content/browser/browse...
content/browser/browser_plugin/browser_plugin_guest.cc:473:
memcpy(&resent_gesture_event, &event, sizeof(blink::WebGestureEvent));
Sigh.  memcpy is pretty dangerous in general, and I don't like that we're
choosing the size based on the event.type parameter from the renderer process. 
I suppose it seems safe the way it's used here, though, since we won't copy any
more than the size of the destination.

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

https://codereview.chromium.org/1308273003/diff/120001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_guest.cc:706: // We don't
forward innertial GestureScrollUpdates to the guest anymore
nit: inertial

https://codereview.chromium.org/1308273003/diff/120001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/1308273003/diff/120001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.h:809: bool
is_in_gesture_scroll_;
Does this need a TODO for when it can be removed?  The CL description makes it
sound like this code is temporary.

[wjmaclean, 2015-09-18 15:22:59]

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

[commit-bot: I haz the power, 2015-09-18 15:23:06]

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

[wjmaclean, 2015-09-18 15:30:57]

> Looks like there's some relevant test failures.  Otherwise, 
> I'll give a rubber stamp LGTM with nits, based on Ken's
> review.

Thanks. I'm sorting out the tests now. There are a number of tests that send
GestureFlingStart without a preceding GestureScrollBegin, which hits the DCHECKs
I've added to RenderWidgetHostImpl. I'd like to keep the DCHECKs, though merely
adding in the missing GestureScrollStarts may be a challenge ... and some of the
tests live in places outside of content/ so adding a bollean testing flag to
RWHI seems out of the question.

> Will this be handled in OOPIFs in a different way?

Yes. My preferred approach would have been to have the embedder know more about
the scroll-state of the guest, and just return false in
BrowserPluginGuest::handleInputEvent if the guest is not scrollable. But at
present only the root-layer's scroll state is visible via the
CompositorFrameMetadata.

Based on discussions with rjkroege@ the new way will be to have the information
passed out via the PropertyTrees, and handled in the browser process, but that's
at least a quarter away it seems.

https://codereview.chromium.org/1308273003/diff/120001/chrome/test/data/exten...
File
chrome/test/data/extensions/platform_apps/web_view/scrollable_embedder_and_guest/guest.js
(right):

https://codereview.chromium.org/1308273003/diff/120001/chrome/test/data/exten...
chrome/test/data/extensions/platform_apps/web_view/scrollable_embedder_and_guest/guest.js:1:
// Copyright 2014 The Chromium Authors. All rights reserved.
On 2015/09/18 04:30:35, Charlie Reis wrote:
> nit: 2015

Done.

https://codereview.chromium.org/1308273003/diff/120001/chrome/test/data/exten...
File
chrome/test/data/extensions/platform_apps/web_view/scrollable_embedder_and_guest/main.js
(right):

https://codereview.chromium.org/1308273003/diff/120001/chrome/test/data/exten...
chrome/test/data/extensions/platform_apps/web_view/scrollable_embedder_and_guest/main.js:10:
LOG('Hi!!');
On 2015/09/18 04:30:36, Charlie Reis wrote:
> Needed?

Ooops, no! Thanks for spotting!!

Done.

https://codereview.chromium.org/1308273003/diff/120001/content/browser/browse...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/1308273003/diff/120001/content/browser/browse...
content/browser/browser_plugin/browser_plugin_guest.cc:473:
memcpy(&resent_gesture_event, &event, sizeof(blink::WebGestureEvent));
On 2015/09/18 04:30:36, Charlie Reis wrote:
> Sigh.  memcpy is pretty dangerous in general, and I don't like that we're
> choosing the size based on the event.type parameter from the renderer process.

> I suppose it seems safe the way it's used here, though, since we won't copy
any
> more than the size of the destination.

Acknowledged.

I used it based on seeing the pattern elsewhere. I guess the main issue is
trusting the event type (and content?) since it's sent from the renderer ...
after that the method of copying (if done properly) doesn't matter, is that
right?

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

https://codereview.chromium.org/1308273003/diff/120001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_guest.cc:706: // We don't
forward innertial GestureScrollUpdates to the guest anymore
On 2015/09/18 04:30:36, Charlie Reis wrote:
> nit: inertial

Done.

https://codereview.chromium.org/1308273003/diff/120001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/1308273003/diff/120001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.h:809: bool
is_in_gesture_scroll_;
On 2015/09/18 04:30:36, Charlie Reis wrote:
> Does this need a TODO for when it can be removed?  The CL description makes it
> sound like this code is temporary.

Done.

[commit-bot: I haz the power, 2015-09-18 15:58:00]

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

[commit-bot: I haz the power, 2015-09-18 15:58:01]

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

[wjmaclean, 2015-09-21 20:33:43]

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

[commit-bot: I haz the power, 2015-09-21 20:34:30]

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

[commit-bot: I haz the power, 2015-09-21 21:11:50]

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

[commit-bot: I haz the power, 2015-09-21 21:11:51]

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

[wjmaclean, 2015-09-22 14:06:51]

Patchset #11 (id:200001) has been deleted

[wjmaclean, 2015-09-24 20:01:24]

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

[commit-bot: I haz the power, 2015-09-24 20:06:19]

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

[commit-bot: I haz the power, 2015-09-24 21:08:19]

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

[commit-bot: I haz the power, 2015-09-24 21:08:20]

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

[wjmaclean, 2015-09-25 13:53:12]

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

[commit-bot: I haz the power, 2015-09-25 13:53:34]

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

[commit-bot: I haz the power, 2015-09-25 15:28:26]

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

[commit-bot: I haz the power, 2015-09-25 15:28:27]

Dry run: This issue passed the CQ dry run.

[wjmaclean, 2015-09-25 15:50:05]

The CQ bit was checked by wjmaclean@chromium.org

[wjmaclean, 2015-09-25 15:50:05]

The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org,
creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1308273003/#ps280001
(title: "Don't require GestureScrollBegin for touch-pad GestureFlingStart.")

[commit-bot: I haz the power, 2015-09-25 15:51:10]

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

[commit-bot: I haz the power, 2015-09-25 15:56:52]

Committed patchset #14 (id:280001)

[commit-bot: I haz the power, 2015-09-25 15:57:43]

Patchset 14 (id:??) landed as
https://crrev.com/73b15a29e89b0150b8ba5ed4b43af298d30bc3da
Cr-Commit-Position: refs/heads/master@{#350838}