Remove dependency of ResourceDispatcherHostImpl on RenderViewHostImpl

Remove dependency of ResourceDispatcherHostImpl on RenderViewHostImpl

Create an async interface LoaderDelegate to delegate from c/b/loader to
the rest of content. This allows decoupling of loader so that it can
eventually be moved to a separate service.

Start by using it to remove the dependency on RenderViewHostImpl.

R=jam
BUG=622050
Committed: https://crrev.com/5cbd1cf54e80149c704295563e9466712f86db56
Cr-Commit-Position: refs/heads/master@{#402680}

[scottmg, 2016-06-24 22:41:32]

Description was changed from

==========
Remove dependency of ResourceDispatcherHostImpl on RenderViewHostImpl

Pass a ViewHost mojo interface to ResourceDispatcherHostImpl to allow it
to communicate with RenderViewHostImpl. Use that to pass
LoadStateChanged() from loaders through to RenderViewHostImpl and back
to its delegate.

R=jam
BUG=622050
==========

to

==========
Remove dependency of ResourceDispatcherHostImpl on RenderViewHostImpl

Pass a ViewHost mojo interface to ResourceDispatcherHostImpl to allow it
to communicate with RenderViewHostImpl. Use that to pass
LoadStateChanged() from loaders back to RenderViewHostImpl and then back
to its delegate.

R=jam
BUG=622050
==========

[mmenke, 2016-06-24 23:08:47]

Drive by comments:

* This seems like a really weird mojo interface.  Is there any reason RDH needs
to know it's the RVH that wants theese updates?  Also note that we're working
towards getting rid of The ResourceScheduler, so even if the
"scheduler_->OnClientCreated" call still seems weird, that will (hopefully) go
away at some point.
* If we're using a mojo interface for this, is there any reason
ResourceDispatcherHostImpl::UpdateLoadInfoOnUIThread has to be called on the UI
thread?  I assume mojo has no magic to make same-thread calls more efficient
that a cross-thread call, or does it?
* Are there performance considerations for this kind of thing?  My understanding
it mojo always serializes/deserializes all of its arguments.
* Are there any tests for this?  If not, it's a pre-existing issue, just wonder
if we have protection if this, say, breaks updating the displayed page load
status.

[scottmg, 2016-06-24 23:33:47]

On 2016/06/24 23:08:47, mmenke wrote:
> Drive by comments:

Thanks for looking! I have no idea what I'm doing. :)

> 
> * This seems like a really weird mojo interface.  Is there any reason RDH
needs
> to know it's the RVH that wants theese updates?  Also note that we're working
> towards getting rid of The ResourceScheduler, so even if the
> "scheduler_->OnClientCreated" call still seems weird, that will (hopefully) go
> away at some point.

I'm not sure what you mean by RDH needing to know. You mean the hookup could be
less explicit? Or that the interface would be better named something like
ResourceDispatchHostNotifier or something like that? It'd be nice if
WebContentsImpl could be the one to get notified directly rather than through
layers of delegates, but I wasn't sure how to set that up, so this was my
attempt at making some progress in that direction.

> * If we're using a mojo interface for this, is there any reason
> ResourceDispatcherHostImpl::UpdateLoadInfoOnUIThread has to be called on the
UI
> thread?  I assume mojo has no magic to make same-thread calls more efficient
> that a cross-thread call, or does it?

Good point, I think probably not. I'll look into that.

> * Are there performance considerations for this kind of thing?  My
understanding
> it mojo always serializes/deserializes all of its arguments.

Maybe I underestimated how often this happens, it seemed fairly infrequent based
on my limited testing (per URL, every 250ms). But yes, I would assume it's
slightly more expensive when mojo is talking in-process to serialize the 4 ints
+ string per LoadStateChanged(), than it will be to call a delegate function.

> * Are there any tests for this?  If not, it's a pre-existing issue, just
wonder
> if we have protection if this, say, breaks updating the displayed page load
> status.

I was hoping I might break something along the way, but I don't see any. I'll
see if I can add something to web_contents_impl_(unit|browser)test.cc.

[jam, 2016-06-27 16:38:43]

Do we really care about adding a mojo interface between code in the browser at
this point, as opposed to when we're closer to running the network code in a
separate process? i.e. don't we get most of the benefit by just having an async
interface?

As a concrete example: for protocol handlers and url intercepters, it's a lot of
work to create mojo interfaces of the net/ C++ interfaces which we don't need
until we have multi-process going.

[scottmg, 2016-06-27 22:30:41]

On 2016/06/27 16:38:43, jam wrote:
> Do we really care about adding a mojo interface between code in the browser at
> this point, as opposed to when we're closer to running the network code in a
> separate process? i.e. don't we get most of the benefit by just having an
async
> interface?
> 
> As a concrete example: for protocol handlers and url intercepters, it's a lot
of
> work to create mojo interfaces of the net/ C++ interfaces which we don't need
> until we have multi-process going.

OK, after off-line conversation, switched to a plain C++ delegate interface as
this is all staying inside content for now.

This sidesteps the weirdness of figuring out what the actual interface should be
interfacing (frames, views, etc.), doesn't cause the potential performance
regression of using mojo in-process, and doesn't add more to
OnRenderViewHostCreated, et al.

PTAL.

[scottmg, 2016-06-27 22:30:43]

On 2016/06/27 16:38:43, jam wrote:
> Do we really care about adding a mojo interface between code in the browser at
> this point, as opposed to when we're closer to running the network code in a
> separate process? i.e. don't we get most of the benefit by just having an
async
> interface?
> 
> As a concrete example: for protocol handlers and url intercepters, it's a lot
of
> work to create mojo interfaces of the net/ C++ interfaces which we don't need
> until we have multi-process going.

OK, after off-line conversation, switched to a plain C++ delegate interface as
this is all staying inside content for now.

This sidesteps the weirdness of figuring out what the actual interface should be
interfacing (frames, views, etc.), doesn't cause the potential performance
regression of using mojo in-process, and doesn't add more to
OnRenderViewHostCreated, et al.

PTAL.

[Charlie Harrison, 2016-06-27 23:08:51]

csharrison@chromium.org changed reviewers:
+ csharrison@chromium.org

[Charlie Harrison, 2016-06-27 23:08:52]

drive-by: Just noticed this and it seems like we're stepping on each other's
feet here.

https://codereview.chromium.org/1564963002/

Feel free to continue as the reviews petered out on that CL.

[Ben Goodger (Google), 2016-06-27 23:14:43]

On 2016/06/24 22:42:11, scottmg wrote:

P.S. I love that you're working on this. This stuff is going to be so awesome
once it's done!

[scottmg, 2016-06-27 23:25:13]

On 2016/06/27 23:08:52, csharrison wrote:
> drive-by: Just noticed this and it seems like we're stepping on each other's
> feet here.
> 
> https://codereview.chromium.org/1564963002/
> 
> Feel free to continue as the reviews petered out on that CL.

Thanks for the pointer. If this one lands, hopefully we could still remove a
layer as you've done, there doesn't seem to be any reason to hop through RVH.

[jam, 2016-06-28 16:23:49]

lgtm

https://codereview.chromium.org/2098923002/diff/120001/content/browser/loader...
File content/browser/loader/loader_delegate.h (right):

https://codereview.chromium.org/2098923002/diff/120001/content/browser/loader...
content/browser/loader/loader_delegate.h:10: #include
"content/browser/loader/global_routing_id.h"
nit: how about just sending child_id and route_id instead of using
GlobalRoutingID? that avoids introducing a dependency on this header from
outside the loader code.

https://codereview.chromium.org/2098923002/diff/120001/content/browser/loader...
content/browser/loader/loader_delegate.h:17: // Delegate from loader to the rest
of content. Lives on the IO thread.
nit: to help others understand how this differs from other delegates in the
loader code, please add something to the effect of:

This is used for breaking dependency from files in content/browser/loader
that'll be moved to a separate networking service. All methods here should be
asynchronous since eventually this will be a Mojo interface once the networking
code runs in a separate process. http://crbug.com/622050

https://codereview.chromium.org/2098923002/diff/120001/content/browser/loader...
content/browser/loader/loader_delegate.h:22: virtual void LoadStateChanged(const
GlobalRoutingID& routing_id,
nit: please document

https://codereview.chromium.org/2098923002/diff/120001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2098923002/diff/120001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:2462: if
(!loader_delegate_)
do we really need this? i.e. we should just assume it's always there to keep the
code simpler

[scottmg, 2016-06-28 18:22:11]

Description was changed from

==========
Remove dependency of ResourceDispatcherHostImpl on RenderViewHostImpl

Pass a ViewHost mojo interface to ResourceDispatcherHostImpl to allow it
to communicate with RenderViewHostImpl. Use that to pass
LoadStateChanged() from loaders back to RenderViewHostImpl and then back
to its delegate.

R=jam
BUG=622050
==========

to

==========
Remove dependency of ResourceDispatcherHostImpl on RenderViewHostImpl

Create an async interface LoaderDelegate to delegate from c/b/loader to
the rest of content. This allows decoupling of loader so that it can
eventually be moved to a separate service.

Start by using it to remove the dependency on RenderViewHostImpl.

R=jam
BUG=622050
==========

[scottmg, 2016-06-28 18:33:39]

Thanks! mmenke, do you want to take another look?

https://codereview.chromium.org/2098923002/diff/120001/content/browser/loader...
File content/browser/loader/loader_delegate.h (right):

https://codereview.chromium.org/2098923002/diff/120001/content/browser/loader...
content/browser/loader/loader_delegate.h:10: #include
"content/browser/loader/global_routing_id.h"
On 2016/06/28 16:23:49, jam wrote:
> nit: how about just sending child_id and route_id instead of using
> GlobalRoutingID? that avoids introducing a dependency on this header from
> outside the loader code.

Done.

https://codereview.chromium.org/2098923002/diff/120001/content/browser/loader...
content/browser/loader/loader_delegate.h:17: // Delegate from loader to the rest
of content. Lives on the IO thread.
On 2016/06/28 16:23:49, jam wrote:
> nit: to help others understand how this differs from other delegates in the
> loader code, please add something to the effect of:
> 
> This is used for breaking dependency from files in content/browser/loader
> that'll be moved to a separate networking service. All methods here should be
> asynchronous since eventually this will be a Mojo interface once the
networking
> code runs in a separate process. http://crbug.com/622050

Done.

https://codereview.chromium.org/2098923002/diff/120001/content/browser/loader...
content/browser/loader/loader_delegate.h:22: virtual void LoadStateChanged(const
GlobalRoutingID& routing_id,
On 2016/06/28 16:23:49, jam wrote:
> nit: please document

Done.

https://codereview.chromium.org/2098923002/diff/120001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2098923002/diff/120001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:2462: if
(!loader_delegate_)
On 2016/06/28 16:23:49, jam wrote:
> do we really need this? i.e. we should just assume it's always there to keep
the
> code simpler

Yeah, probably a good idea, done.

[scottmg, 2016-06-28 21:54:56]

The CQ bit was checked by scottmg@chromium.org

[scottmg, 2016-06-28 21:54:57]

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

[commit-bot: I haz the power, 2016-06-28 21:56:06]

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

[commit-bot: I haz the power, 2016-06-29 02:35:10]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-06-29 02:36:27]

Description was changed from

==========
Remove dependency of ResourceDispatcherHostImpl on RenderViewHostImpl

Create an async interface LoaderDelegate to delegate from c/b/loader to
the rest of content. This allows decoupling of loader so that it can
eventually be moved to a separate service.

Start by using it to remove the dependency on RenderViewHostImpl.

R=jam
BUG=622050
==========

to

==========
Remove dependency of ResourceDispatcherHostImpl on RenderViewHostImpl

Create an async interface LoaderDelegate to delegate from c/b/loader to
the rest of content. This allows decoupling of loader so that it can
eventually be moved to a separate service.

Start by using it to remove the dependency on RenderViewHostImpl.

R=jam
BUG=622050

Committed: https://crrev.com/5cbd1cf54e80149c704295563e9466712f86db56
Cr-Commit-Position: refs/heads/master@{#402680}
==========

[commit-bot: I haz the power, 2016-06-29 02:36:29]

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

[mmenke, 2016-06-29 15:22:32]

On 2016/06/28 18:33:39, scottmg wrote:
> Thanks! mmenke, do you want to take another look?

Oops, sorry I didn't follow up on this!  I had thought I was CCed on this issue,
due to making a comment, but turns out I wasn't.

[mmenke, 2016-06-29 15:24:19]

On 2016/06/24 23:33:47, scottmg wrote:
> On 2016/06/24 23:08:47, mmenke wrote:
> > Drive by comments:
> 
> Thanks for looking! I have no idea what I'm doing. :)
> 
> > 
> > * This seems like a really weird mojo interface.  Is there any reason RDH
> needs
> > to know it's the RVH that wants theese updates?  Also note that we're
working
> > towards getting rid of The ResourceScheduler, so even if the
> > "scheduler_->OnClientCreated" call still seems weird, that will (hopefully)
go
> > away at some point.
> 
> I'm not sure what you mean by RDH needing to know. You mean the hookup could
be
> less explicit? Or that the interface would be better named something like
> ResourceDispatchHostNotifier or something like that? It'd be nice if
> WebContentsImpl could be the one to get notified directly rather than through
> layers of delegates, but I wasn't sure how to set that up, so this was my
> attempt at making some progress in that direction.

What I meant was the naming of those methods - which it looks like you
addressed.

> > * If we're using a mojo interface for this, is there any reason
> > ResourceDispatcherHostImpl::UpdateLoadInfoOnUIThread has to be called on the
> UI
> > thread?  I assume mojo has no magic to make same-thread calls more efficient
> > that a cross-thread call, or does it?
> 
> Good point, I think probably not. I'll look into that.
> 
> > * Are there performance considerations for this kind of thing?  My
> understanding
> > it mojo always serializes/deserializes all of its arguments.
> 
> Maybe I underestimated how often this happens, it seemed fairly infrequent
based
> on my limited testing (per URL, every 250ms). But yes, I would assume it's
> slightly more expensive when mojo is talking in-process to serialize the 4
ints
> + string per LoadStateChanged(), than it will be to call a delegate function.
> 
> > * Are there any tests for this?  If not, it's a pre-existing issue, just
> wonder
> > if we have protection if this, say, breaks updating the displayed page load
> > status.
> 
> I was hoping I might break something along the way, but I don't see any. I'll
> see if I can add something to web_contents_impl_(unit|browser)test.cc.