Refactor the guts of ResourceDispatcherHostImpl into a new class named

Refactor the guts of ResourceDispatcherHostImpl into a new class named
ResourceLoader.  This class is responsible for driving the URLRequest (calling
Start, Read, and servicing events).

This change also generalizes ResourceThrottleController so that it can be used
by ResourceHandler implementations to resume or cancel a resource load.  The
ResourceLoader is a ResourceController.  (In a follow-up patch, the
BufferedResourceHandler will also become a ResourceController.  This will
enable us to make the "resume" behavior sane.)

The ResourceDispatcherHostImpl now keeps a set of ResourceLoader objects
instead of URLRequests.  The ResourceLoader owns its URLRequest as well as
ResourceHandler (chain).  Numerous fields from ResourceRequestInfo become
member variables of ResourceLoader because they are only needed by
ResourceLoader.  This helps cleanup some code.  ResourceDispatcherHostImpl
is no longer a friend of ResourceRequestInfoImpl, and indeed RRII no longer
has any private getter/setters intended for use by RDHI!

The ResourceDispatcherHostImpl used to maintain a set of transferred
URLRequests.  This set is eliminated in favor of just storing a boolean
flag on ResourceLoader (is_transferring_).  When transferring a request
from one RenderView to another, we still replace the ResourceHandler
as before, but we keep the ResourceLoader.  This means the logic for
completing the transfer becomes a member function on ResourceLoader.
See ResourceLoader::CompleteTransfer().

NOTE: I left it as future cleanup to improve some of the methods on
ResourceLoader that are essentially just copy/pasted from RDHI.  For
example, I suspect that the new deferred_stage_ member variable will
make called_on_response_started_, has_started_reading_, and is_paused_
obsolete.  I plan to make these simplifications along with making
BufferedResourceHandler implement ResourceController.  For now, I'm
just trying to minimize changes since this CL is already too big.

R=jam@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=142108

[darin (slow to review), 2012-06-13 18:41:43]

Sorry for the huge CL! :-(

[jam, 2012-06-14 01:10:22]

lgtm

I assume that the moved parts in resource_loader.cc didn't change, or changed to
preserve functionality, as that part was hard to follow. But given green try
runs, I think if there was a problem it would probably be apparent very quickly.

nice cleanup!

http://codereview.chromium.org/10501004/diff/39081/content/browser/renderer_h...
File content/browser/renderer_host/resource_dispatcher_host_impl.cc (right):

http://codereview.chromium.org/10501004/diff/39081/content/browser/renderer_h...
content/browser/renderer_host/resource_dispatcher_host_impl.cc:1315:
CHECK(loader);
nit: this isn't needed, it'll crash below which will be just as useful of a
signal?

http://codereview.chromium.org/10501004/diff/39081/content/browser/renderer_h...
File content/browser/renderer_host/resource_dispatcher_host_impl.h (right):

http://codereview.chromium.org/10501004/diff/39081/content/browser/renderer_h...
content/browser/renderer_host/resource_dispatcher_host_impl.h:90: //
ResourceLoaderDelegate implementation:
nit: does this need to be public?

http://codereview.chromium.org/10501004/diff/39081/content/browser/renderer_h...
File content/browser/renderer_host/resource_loader.h (right):

http://codereview.chromium.org/10501004/diff/39081/content/browser/renderer_h...
content/browser/renderer_host/resource_loader.h:22: class ResourceLoader :
public net::URLRequest::Delegate,
nit: add a comment about what this class is used for

http://codereview.chromium.org/10501004/diff/39081/content/browser/renderer_h...
content/browser/renderer_host/resource_loader.h:47: // IPC message handlers:
nit: these message handlers can be private right? same for the other overridden
functions?

[darin (slow to review), 2012-06-14 06:51:26]

On Wed, Jun 13, 2012 at 6:10 PM, <jam@chromium.org> wrote:

> lgtm
>
> I assume that the moved parts in resource_loader.cc didn't change, or
> changed to
> preserve functionality, as that part was hard to follow. But given green
> try
> runs, I think if there was a problem it would probably be apparent very
> quickly.
>

Right.  I tried doing a "svn cp" of resource_dispatcher_host_impl.cc to
resource_loader.cc, but that doesn't turn out to really help make the patch
easier to read.  Because many variables are no longer under
ResourceRequestInfoImpl, almost all of the lines end up being changed.



>
> nice cleanup!
>
>
> http://codereview.chromium.**org/10501004/diff/39081/**
>
content/browser/renderer_host/**resource_dispatcher_host_impl.**cc<http://codereview.chromium.org/10501004/diff/39081/content/browser/renderer_host/resource_dispatcher_host_impl.cc>
> File content/browser/renderer_host/**resource_dispatcher_host_impl.**cc
> (right):
>
> http://codereview.chromium.**org/10501004/diff/39081/**
> content/browser/renderer_host/**resource_dispatcher_host_impl.**
>
cc#newcode1315<http://codereview.chromium.org/10501004/diff/39081/content/browser/renderer_host/resource_dispatcher_host_impl.cc#newcode1315>
> content/browser/renderer_host/**resource_dispatcher_host_impl.**cc:1315:
> CHECK(loader);
> nit: this isn't needed, it'll crash below which will be just as useful
> of a signal?
>

Done.  Changed to just be GetLoader(id)->Function()


>
> http://codereview.chromium.**org/10501004/diff/39081/**
>
content/browser/renderer_host/**resource_dispatcher_host_impl.**h<http://codereview.chromium.org/10501004/diff/39081/content/browser/renderer_host/resource_dispatcher_host_impl.h>
> File content/browser/renderer_host/**resource_dispatcher_host_impl.**h
> (right):
>
> http://codereview.chromium.**org/10501004/diff/39081/**
> content/browser/renderer_host/**resource_dispatcher_host_impl.**
>
h#newcode90<http://codereview.chromium.org/10501004/diff/39081/content/browser/renderer_host/resource_dispatcher_host_impl.h#newcode90>
> content/browser/renderer_host/**resource_dispatcher_host_impl.**h:90: //
> ResourceLoaderDelegate implementation:
> nit: does this need to be public?
>

Nope, made private.


>
> http://codereview.chromium.**org/10501004/diff/39081/**
>
content/browser/renderer_host/**resource_loader.h<http://codereview.chromium.org/10501004/diff/39081/content/browser/renderer_host/resource_loader.h>
> File content/browser/renderer_host/**resource_loader.h (right):
>
> http://codereview.chromium.**org/10501004/diff/39081/**
>
content/browser/renderer_host/**resource_loader.h#newcode22<http://codereview.chromium.org/10501004/diff/39081/content/browser/renderer_host/resource_loader.h#newcode22>
> content/browser/renderer_host/**resource_loader.h:22: class ResourceLoader
> : public net::URLRequest::Delegate,
> nit: add a comment about what this class is used for
>

Done


>
> http://codereview.chromium.**org/10501004/diff/39081/**
>
content/browser/renderer_host/**resource_loader.h#newcode47<http://codereview.chromium.org/10501004/diff/39081/content/browser/renderer_host/resource_loader.h#newcode47>
> content/browser/renderer_host/**resource_loader.h:47: // IPC message
> handlers:
> nit: these message handlers can be private right? same for the other
> overridden functions?
>

Actually, the message handlers can't.  They are called by RDHI.  I intend
to eliminate them / make them private by using some kind of message
interception system.  The other overridden functions can be private with
one exception.  RDHI needs to call Resume() on the ResourceLoader from
its DataReceivedACK() method.  My plan is to move that code to
AsyncResourceHandler, which would mean that we'd just be calling methods
on controller().  So, I decided to go ahead and make all of these overidden
methods private, and then in that one special place in RDHI, I can just
static_cast to ResourceController in order to call Resume().  I'll add a
TODO
to explain this.



>
>
http://codereview.chromium.**org/10501004/<http://codereview.chromium.org/105...
>