Separate RenderViewHost from RenderWidgetHost, part 4: delegate

Separate RenderViewHost from RenderWidgetHost, part 4: delegate

This makes a delegate interface between the RenderViewHost and the RenderWidgetHost, which will eventually be used for their communication.

This also allows the removal of the horrible terrible no-good RenderWidgetHost::IsRenderView().

BUG=542477
TEST=all green

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

[Avi (use Gerrit), 2015-10-25 02:37:50]

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

[commit-bot: I haz the power, 2015-10-25 02:38:04]

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

[commit-bot: I haz the power, 2015-10-25 02:46:06]

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

[commit-bot: I haz the power, 2015-10-25 02:46:07]

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

[Avi (use Gerrit), 2015-10-25 03:24:14]

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

[commit-bot: I haz the power, 2015-10-25 03:24:32]

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

[Avi (use Gerrit), 2015-10-25 03:38:05]

Description was changed from

==========
Separate RenderViewHost from RenderWidgetHost, part 4: delegate

This makes a delegate interface between the RenderViewHost and the
RenderWidgetHost, which will eventually be used for their communication.

This also allows the removal of the horrible terrible no-good
RenderWidgetHost::IsRenderView().

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

to

==========
Separate RenderViewHost from RenderWidgetHost, part 4: delegate

This makes a delegate interface between the RenderViewHost and the
RenderWidgetHost, which will eventually be used for their communication.

This also allows the removal of the horrible terrible no-good
RenderWidgetHost::IsRenderView().

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

[commit-bot: I haz the power, 2015-10-25 03:42:14]

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

[commit-bot: I haz the power, 2015-10-25 03:42:14]

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-25 04:02:27]

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

[commit-bot: I haz the power, 2015-10-25 04:02:35]

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

[Avi (use Gerrit), 2015-10-25 04:03:01]

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

[Avi (use Gerrit), 2015-10-25 04:03:02]

Nick and Daniel, for your amusement and consideration.

[commit-bot: I haz the power, 2015-10-25 05:29:21]

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

[commit-bot: I haz the power, 2015-10-25 05:29:21]

Dry run: This issue passed the CQ dry run.

[ncarter (slow), 2015-10-26 18:35:03]

Good idea, to make this step its own CL.

https://codereview.chromium.org/1411203010/diff/40001/chrome/browser/renderer...
File chrome/browser/renderer_host/chrome_render_widget_host_view_mac_delegate.mm
(right):

https://codereview.chromium.org/1411203010/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_render_widget_host_view_mac_delegate.mm:144:
RenderViewHost::From(renderWidgetHost_));
In the case where renderWidgetHost_ is not a for a view, seems like you've
changed the return value here from 'NO' to 'YES'. Is that intentional?

https://codereview.chromium.org/1411203010/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_render_widget_host_view_mac_delegate.mm:165:
renderWidgetHost_ && RenderViewHost::From(renderWidgetHost_) != nullptr;
This seems to be the only place in this class where we handle a null rWH_ (and
the old code deref'd it unconditionally). Unless this fixes a bug, I'd drop it.

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

https://codereview.chromium.org/1411203010/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:201: if (!owner_delegate)
Seems a little dirty for this code to assume it's the only owner delegate. I'm
okay with leaving it that way if we're pretty sure that
RenderWidgetHostOwnerDelegate will have a truly short life. But if we think in
the interim we might want to port other behaviors to this delegate (e.g.
RWHI::owned_by_render_frame_host_) then we should make it safer maybe?

Two options I'd be okay with:
 1. Add one of those lame-o methods
RenderWidgetHostOwnerDelegate::GetAsRenderViewHost()
 2. Or, alternatively just add here a DCHECK(result->GetWidget() == rwh) so that
we'll at least fail here if things do go south.

https://codereview.chromium.org/1411203010/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (left):

https://codereview.chromium.org/1411203010/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:438: bool
RenderWidgetHostImpl::IsRenderView() const {
In a class that I once knew
lived the call IsRenderView()
But what a load of ballyhoo
It was to leak the layers through
So farewell, farewell, farewell, you:
I'll not forget how much you blew.

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

https://codereview.chromium.org/1411203010/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:229: if
(!base::CommandLine::ForCurrentProcess()->HasSwitch(
You fixed the glitch :)

https://codereview.chromium.org/1411203010/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1411203010/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:557:
contentses.insert(static_cast<WebContentsImpl*>(web_contents));
Random observation: We could build the vector directly, no std::set needed, by
adding only if "web_contents->GetRenderViewHost() == rvh" is true.

[Avi (use Gerrit), 2015-10-26 19:08:45]

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

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

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

[Avi (use Gerrit), 2015-10-26 19:41:28]

https://codereview.chromium.org/1411203010/diff/40001/chrome/browser/renderer...
File chrome/browser/renderer_host/chrome_render_widget_host_view_mac_delegate.mm
(right):

https://codereview.chromium.org/1411203010/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_render_widget_host_view_mac_delegate.mm:144:
RenderViewHost::From(renderWidgetHost_));
On 2015/10/26 18:35:02, ncarter wrote:
> In the case where renderWidgetHost_ is not a for a view, seems like you've
> changed the return value here from 'NO' to 'YES'. Is that intentional?

No. :\

https://codereview.chromium.org/1411203010/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_render_widget_host_view_mac_delegate.mm:165:
renderWidgetHost_ && RenderViewHost::From(renderWidgetHost_) != nullptr;
On 2015/10/26 18:35:02, ncarter wrote:
> This seems to be the only place in this class where we handle a null rWH_ (and
> the old code deref'd it unconditionally). Unless this fixes a bug, I'd drop
it. 

Well, there are a few other places (note the method -shouldAllowHistorySwiping
in which you commented). But point taken.

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

https://codereview.chromium.org/1411203010/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:201: if (!owner_delegate)
On 2015/10/26 18:35:02, ncarter wrote:
> Seems a little dirty for this code to assume it's the only owner delegate.

Yes, this is the weakest part of the scheme.

> But if we think in
> the interim we might want to port other behaviors to this delegate (e.g.
> RWHI::owned_by_render_frame_host_)

BTW I want to rename that to a more general "is owned / should not delete self"
name.

> Two options I'd be okay with:
>  1. Add one of those lame-o methods
> RenderWidgetHostOwnerDelegate::GetAsRenderViewHost()

That just adds the layering violation back in :(

>  2. Or, alternatively just add here a DCHECK(result->GetWidget() == rwh) so
that
> we'll at least fail here if things do go south.

That's fair. I do want to make that weird delegate go away quickly, FWIW.

https://codereview.chromium.org/1411203010/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (left):

https://codereview.chromium.org/1411203010/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:438: bool
RenderWidgetHostImpl::IsRenderView() const {
On 2015/10/26 18:35:02, ncarter wrote:
> In a class that I once knew
> lived the call IsRenderView()
> But what a load of ballyhoo
> It was to leak the layers through
> So farewell, farewell, farewell, you:
> I'll not forget how much you blew.

Acknowledged.

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

https://codereview.chromium.org/1411203010/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:229: if
(!base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2015/10/26 18:35:02, ncarter wrote:
> You fixed the glitch :)

Yes.

https://codereview.chromium.org/1411203010/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1411203010/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:557:
contentses.insert(static_cast<WebContentsImpl*>(web_contents));
On 2015/10/26 18:35:02, ncarter wrote:
> Random observation: We could build the vector directly, no std::set needed, by
> adding only if "web_contents->GetRenderViewHost() == rvh" is true.

Oh, that's clever/evil.

[commit-bot: I haz the power, 2015-10-26 20:15:57]

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

[commit-bot: I haz the power, 2015-10-26 20:15:57]

Dry run: This issue passed the CQ dry run.

[ncarter (slow), 2015-10-26 20:27:35]

lgtm

https://codereview.chromium.org/1411203010/diff/60001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1411203010/diff/60001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:550:
std::set<WebContentsImpl*> web_contents_set;
Delete this line.

[Avi (use Gerrit), 2015-10-26 20:36:57]

https://codereview.chromium.org/1411203010/diff/60001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1411203010/diff/60001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:550:
std::set<WebContentsImpl*> web_contents_set;
On 2015/10/26 20:27:35, ncarter wrote:
> Delete this line.

Done.

[Avi (use Gerrit), 2015-10-26 20:37:06]

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

[commit-bot: I haz the power, 2015-10-26 20:37:27]

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

[Avi (use Gerrit), 2015-10-26 20:39:45]

avi@chromium.org changed reviewers:
+ benwells@chromium.org, michaelbai@chromium.org, sky@chromium.org

[Avi (use Gerrit), 2015-10-26 20:39:46]

For review.

Michael: aw_
Scott: chrome/
Ben: extensions

Thanks!

[sky, 2015-10-26 21:07:38]

https://codereview.chromium.org/1411203010/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/processes/processes_api.cc (left):

https://codereview.chromium.org/1411203010/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/processes/processes_api.cc:122: if
(!widget->IsRenderView())
How come this is safe? If widget is not a RenderViewHost would 123 be null, and
127 then crash?

https://codereview.chromium.org/1411203010/diff/80001/chrome/browser/task_man...
File chrome/browser/task_manager/guest_information.cc (right):

https://codereview.chromium.org/1411203010/diff/80001/chrome/browser/task_man...
chrome/browser/task_manager/guest_information.cc:112: WebContents* web_contents
= WebContents::FromRenderViewHost(rvh);
Same comment here about chrome/browser/extensions/api/processes/processes_api.cc
.

[commit-bot: I haz the power, 2015-10-26 21:36:39]

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

[commit-bot: I haz the power, 2015-10-26 21:36:40]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2015-10-26 23:20:14]

https://codereview.chromium.org/1411203010/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/processes/processes_api.cc (left):

https://codereview.chromium.org/1411203010/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/processes/processes_api.cc:122: if
(!widget->IsRenderView())
On 2015/10/26 21:07:38, sky wrote:
> How come this is safe? If widget is not a RenderViewHost would 123 be null,
and
> 127 then crash?

If widget isn't a RVH then the RenderViewHost* |host| on line 123 will be null.
Passing in a null RVH to WebContents::FromRenderViewHost now is safe and _also_
returns null (new in this CL!) and that null result will be tested on line 126.

https://codereview.chromium.org/1411203010/diff/80001/chrome/browser/task_man...
File chrome/browser/task_manager/guest_information.cc (right):

https://codereview.chromium.org/1411203010/diff/80001/chrome/browser/task_man...
chrome/browser/task_manager/guest_information.cc:112: WebContents* web_contents
= WebContents::FromRenderViewHost(rvh);
On 2015/10/26 21:07:38, sky wrote:
> Same comment here about
chrome/browser/extensions/api/processes/processes_api.cc
> .

Same answer here. As of this CL, it is safe to pass in null to
WebContents::FromRenderViewHost and you will get a null WebContents back.

[michaelbai, 2015-10-26 23:55:02]

android_webview lgtm

[benwells, 2015-10-27 00:34:21]

apps and extensions lgtm

[sky, 2015-10-27 13:12:33]

LGTM

https://codereview.chromium.org/1411203010/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/processes/processes_api.cc (left):

https://codereview.chromium.org/1411203010/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/processes/processes_api.cc:122: if
(!widget->IsRenderView())
On 2015/10/26 23:20:14, Avi wrote:
> On 2015/10/26 21:07:38, sky wrote:
> > How come this is safe? If widget is not a RenderViewHost would 123 be null,
> and
> > 127 then crash?
> 
> If widget isn't a RVH then the RenderViewHost* |host| on line 123 will be
null.
> Passing in a null RVH to WebContents::FromRenderViewHost now is safe and
_also_
> returns null (new in this CL!) and that null result will be tested on line
126.

Ok, I missed that. I looked at the existing code which doesn't handle null.

[Avi (use Gerrit), 2015-10-27 13:40:52]

The CQ bit was checked by avi@chromium.org

[Avi (use Gerrit), 2015-10-27 13:40:53]

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/1411203010/#ps80001
(title: "oops")

[commit-bot: I haz the power, 2015-10-27 13:41:02]

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

[commit-bot: I haz the power, 2015-10-27 13:45:20]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-10-27 13:46:14]

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