Separate RenderViewHost from RenderWidgetHost, part 5: move calls to the RenderWidgetHostDelegate.

Separate RenderViewHost from RenderWidgetHost, part 5: move calls to the RenderWidgetHostDelegate.

The WebContentsImpl is already a RenderWidgetHostDelegate, so there's no reason to detour through the RenderViewHostImpl.

BUG=542477
TEST=all green

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

[Avi (use Gerrit), 2015-10-29 15:31:56]

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

[commit-bot: I haz the power, 2015-10-29 15:32:59]

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

[commit-bot: I haz the power, 2015-10-29 15:35:18]

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

[commit-bot: I haz the power, 2015-10-29 15:35:18]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Avi (use Gerrit), 2015-10-29 18:38:42]

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

[commit-bot: I haz the power, 2015-10-29 18:39:00]

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

[commit-bot: I haz the power, 2015-10-29 18:50:02]

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

[commit-bot: I haz the power, 2015-10-29 18:50:02]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  cast_shell_android on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...)
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Avi (use Gerrit), 2015-10-29 19:25:39]

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

[commit-bot: I haz the power, 2015-10-29 19:26:19]

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

[commit-bot: I haz the power, 2015-10-29 19:37:21]

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

[commit-bot: I haz the power, 2015-10-29 19:37:21]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Avi (use Gerrit), 2015-10-29 19:41:25]

Description was changed from

==========
Separate RenderViewHost from RenderWidgetHost, part 5: move calls to the
RenderWidgetHostDelegate.

The WebContentsImpl is already a RenderWidgetHostDelegate, so there's no reason
to detour through the RenderViewHostImpl.

BUG=542477
TEST=all green
==========

to

==========
Separate RenderViewHost from RenderWidgetHost, part 5: move calls to the
RenderWidgetHostDelegate.

The WebContentsImpl is already a RenderWidgetHostDelegate, so there's no reason
to detour through the RenderViewHostImpl.

BUG=542477
TEST=all green
==========

[Avi (use Gerrit), 2015-10-29 19:42:04]

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

[commit-bot: I haz the power, 2015-10-29 19:42:17]

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

[commit-bot: I haz the power, 2015-10-29 19:50:49]

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

[commit-bot: I haz the power, 2015-10-29 19:50:50]

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...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Avi (use Gerrit), 2015-10-29 19:56:18]

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

[commit-bot: I haz the power, 2015-10-29 19:57:06]

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

[commit-bot: I haz the power, 2015-10-29 20:07:18]

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

[commit-bot: I haz the power, 2015-10-29 20:07:18]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  cast_shell_android on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...)
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[Avi (use Gerrit), 2015-10-29 22:42:11]

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

[commit-bot: I haz the power, 2015-10-29 22:44:52]

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

[commit-bot: I haz the power, 2015-10-29 23:39:34]

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

[commit-bot: I haz the power, 2015-10-29 23:39:34]

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

[Avi (use Gerrit), 2015-10-29 23:50:06]

avi@chromium.org changed reviewers:
+ nick@chromium.org

[ncarter (slow), 2015-10-30 04:08:11]

nick@chromium.org changed reviewers:
+ dominickn@chromium.org

[ncarter (slow), 2015-10-30 04:08:12]

+dominickn for any thoughts

https://codereview.chromium.org/1410313006/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.cc (left):

https://codereview.chromium.org/1410313006/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:1989: void
RenderWidgetHostImpl::StartUserGesture() {
Nice find.

https://codereview.chromium.org/1410313006/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/1410313006/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:1372: void
RenderWidgetHostImpl::RequestToLockMouse(bool user_gesture,
Maybe inline this function? It's only got one caller; seems it only existed for
the sake of inheritance.

https://codereview.chromium.org/1410313006/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:1859:
delegate_->OnUserGesture();
This is a behavior change for non-view widgets. What are the implications?

For subframe widgets this looks like a bug fix as far as I can tell.

However, for popup widgets (or the fullscreen widget?), are there any
consequences? If there was an original intent to prevent this from happening for
for non-view widgets, do you know why?

https://codereview.chromium.org/1410313006/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:1866:
delegate_->OnUserInteraction(event.type);
With the behavior change above, it seems safe to move the work done by
WebContentsImpl::OnUserGesture into WebContentsImpl::OnUserInteraction, and
eliminate the OnUserGesture delegate method altogether.

It looks like dominickn has been looking at this.

https://codereview.chromium.org/1410313006/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:2074:
delegate_->ResizeDueToAutoResize(new_size);
So in theory, right, this would allow non-view widgets to notify the WebContents
of auto-size events. Which would be bad, except that auto resize happens only to
ever be enabled for view widgets. And that's why this is a safe transformation
to make now.

Do I have that right?

[dominickn, 2015-10-30 04:38:41]

Thanks for the CC, comments regarding OnUserGesture/UserInteraction inline.

https://codereview.chromium.org/1410313006/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/1410313006/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:1866:
delegate_->OnUserInteraction(event.type);
On 2015/10/30 04:08:12, ncarter wrote:
> With the behavior change above, it seems safe to move the work done by
> WebContentsImpl::OnUserGesture into WebContentsImpl::OnUserInteraction, and
> eliminate the OnUserGesture delegate method altogether.
> 
> It looks like dominickn has been looking at this.

I just landed the OnUserInteraction hook earlier this week to propagate all
keypress, click, and gesture taps to WebContentsObservers. We're using this for
the site engagement service to detect user engagement with a site.

I have an in-progress change (not submitted yet) to eliminate *UserGesture in
favour of *UserInteraction, which is more general. The current issue is that the
primary (and original) consumer of *UserGesture  is the download rate limiter,
which explicitly wants the combination of Enter/Space keys, clicks, or taps to
allow an unprompted download. OnUserInteraction deliberately doesn't propagate
the key information so it can be more generalised, while *UserGesture only fires
on that combination of inputs. sky feels quite strongly that "action keys" alone
should be used to allow downloads, rather than arbitrary keys.

One way to ensure the download limiter still responds to only enter and space
may be to have another callback which just propagates keypresses and the
keypress codes. Then the limiter can override that method and OnUserInteraction,
taking enter/space from the former and clicks/taps from the latter. It's uglier
having two methods, but propagating input to the WebContents layer isn't very
clean all round... thoughts welcome on this.

If you're okay with leaving the UserGesture() callbacks in here (or moving them
to WidgetHost), I'm happy to follow up removing them entirely for the
UserInteraction hook.

[Avi (use Gerrit), 2015-10-30 18:36:28]

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

[commit-bot: I haz the power, 2015-10-30 18:38:01]

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

[commit-bot: I haz the power, 2015-10-30 20:01:39]

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

[commit-bot: I haz the power, 2015-10-30 20:01:41]

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_...)

[Avi (use Gerrit), 2015-10-30 20:06:29]

https://codereview.chromium.org/1410313006/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/1410313006/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:1372: void
RenderWidgetHostImpl::RequestToLockMouse(bool user_gesture,
On 2015/10/30 04:08:12, ncarter wrote:
> Maybe inline this function? It's only got one caller; seems it only existed
for
> the sake of inheritance.

Done.

https://codereview.chromium.org/1410313006/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:1866:
delegate_->OnUserInteraction(event.type);
On 2015/10/30 04:38:41, dominickn wrote:
> On 2015/10/30 04:08:12, ncarter wrote:
> > With the behavior change above, it seems safe to move the work done by
> > WebContentsImpl::OnUserGesture into WebContentsImpl::OnUserInteraction, and
> > eliminate the OnUserGesture delegate method altogether.
> > 
> > It looks like dominickn has been looking at this.
> 
> I just landed the OnUserInteraction hook earlier this week to propagate all
> keypress, click, and gesture taps to WebContentsObservers. We're using this
for
> the site engagement service to detect user engagement with a site.
> 
> I have an in-progress change (not submitted yet) to eliminate *UserGesture in
> favour of *UserInteraction, which is more general. The current issue is that
the
> primary (and original) consumer of *UserGesture  is the download rate limiter,
> which explicitly wants the combination of Enter/Space keys, clicks, or taps to
> allow an unprompted download. OnUserInteraction deliberately doesn't propagate
> the key information so it can be more generalised, while *UserGesture only
fires
> on that combination of inputs. sky feels quite strongly that "action keys"
alone
> should be used to allow downloads, rather than arbitrary keys.
> 
> One way to ensure the download limiter still responds to only enter and space
> may be to have another callback which just propagates keypresses and the
> keypress codes. Then the limiter can override that method and
OnUserInteraction,
> taking enter/space from the former and clicks/taps from the latter. It's
uglier
> having two methods, but propagating input to the WebContents layer isn't very
> clean all round... thoughts welcome on this.
> 
> If you're okay with leaving the UserGesture() callbacks in here (or moving
them
> to WidgetHost), I'm happy to follow up removing them entirely for the
> UserInteraction hook.

I'm going to leave them as functionally unchanged as possible, and let you
handle the rest.

[ncarter (slow), 2015-11-02 17:46:56]

lgtm with one fix

https://codereview.chromium.org/1410313006/diff/120001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1410313006/diff/120001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1675:
GotResponseToLockMouseRequest(false);
Should be:

render_widget_host->GotResponseToLockMouseRequest(false);

[Avi (use Gerrit), 2015-11-02 18:42:13]

https://codereview.chromium.org/1410313006/diff/120001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1410313006/diff/120001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1675:
GotResponseToLockMouseRequest(false);
On 2015/11/02 17:46:56, ncarter wrote:
> Should be:
> 
> render_widget_host->GotResponseToLockMouseRequest(false);

Done.

[Avi (use Gerrit), 2015-11-02 18:42:23]

The CQ bit was checked by avi@chromium.org

[Avi (use Gerrit), 2015-11-02 18:42:23]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org
Link to the patchset: https://codereview.chromium.org/1410313006/#ps140001
(title: "fix")

[commit-bot: I haz the power, 2015-11-02 18:42:44]

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

[commit-bot: I haz the power, 2015-11-02 20:00:13]

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

[commit-bot: I haz the power, 2015-11-02 20:00:14]

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_...)

[Avi (use Gerrit), 2015-11-04 20:32:54]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2015-11-04 20:34:21]

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

[commit-bot: I haz the power, 2015-11-04 21:34:58]

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

[commit-bot: I haz the power, 2015-11-04 21:34:59]

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_...)

[Avi (use Gerrit), 2015-11-05 16:41:01]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2015-11-05 16:42:03]

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

[commit-bot: I haz the power, 2015-11-05 17:43:47]

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

[commit-bot: I haz the power, 2015-11-05 17:43:48]

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_...)

[Avi (use Gerrit), 2015-11-05 21:10:25]

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

[commit-bot: I haz the power, 2015-11-05 21:10:44]

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

[commit-bot: I haz the power, 2015-11-05 21:28:37]

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

[commit-bot: I haz the power, 2015-11-05 21:28:37]

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

[Avi (use Gerrit), 2015-11-06 15:56:21]

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

[commit-bot: I haz the power, 2015-11-06 15:56:35]

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

[commit-bot: I haz the power, 2015-11-06 15:58:36]

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

[commit-bot: I haz the power, 2015-11-06 15:58:37]

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

[Avi (use Gerrit), 2015-11-06 21:11:39]

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

[commit-bot: I haz the power, 2015-11-06 21:13:16]

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

[commit-bot: I haz the power, 2015-11-06 22:14:59]

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

[commit-bot: I haz the power, 2015-11-06 22:15:00]

Dry run: This issue passed the CQ dry run.

[ncarter (slow), 2015-11-09 19:45:15]

lgtm

[Avi (use Gerrit), 2015-11-09 19:48:30]

The CQ bit was checked by avi@chromium.org

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

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

[commit-bot: I haz the power, 2015-11-09 20:57:32]

Committed patchset #11 (id:200001)

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

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