PlzNavigate: Add first version of NavigationURLLoader.

PlzNavigate: Add first version of NavigationURLLoader.

Split out from https://codereview.chromium.org/519533002/

This adds the first iteration of a NavigationURLLoader to own the URL request
on the UI thread. It owns the request up until the response starts, at which
point ownership is tranferred to a StreamHandle which resolves to the body.

The request itself is missing much of the handlers and context which are
normally attached to the request. That will need to be resolved later, ideally
in a way that allows most consumers to treat navigation and subresource
requests the same.

With this CL, content_shell with --enable-browser-side-navigation can load a
page in fresh window with no existing renderer.

BUG=376015
Committed: https://crrev.com/6b77cd746e176a15d8cd3002732124687c403a53
Cr-Commit-Position: refs/heads/master@{#301929}

[davidben, 2014-10-10 18:42:22]

davidben@chromium.org changed reviewers:
+ clamy@chromium.org, mmenke@chromium.org, nasko@chromium.org

[davidben, 2014-10-10 18:42:23]

This depends on https://codereview.chromium.org/625993002/.

This is the remainder of the original monster CL, rebased on top of the streams
refactor.

clamy, nasko: main review
mmenke: loader portions

I may be able to split this up further if needbe, but there'd be a lot of "add
X, okay now use X" changes. It's reasonably split up by directory anyway.

content/public: ResourceResponse::DeepCopy for silly threading stuff for now.
content/browser/loader: NavigationURLLoader implementation
content/browser/frame_host: hook NavigationURLLoader up to NavigationRequest.

[clamy, 2014-10-13 15:22:43]

Thanks! please find a few comments below.

https://chromiumcodereview.appspot.com/648813002/diff/310001/content/browser/...
File content/browser/loader/navigation_resource_handler.h (right):

https://chromiumcodereview.appspot.com/648813002/diff/310001/content/browser/...
content/browser/loader/navigation_resource_handler.h:17: // The leaf
ResourceHandler used with NavigationURLLoaderCore.
nit: maybe prefix it with PlzNavigate?

https://chromiumcodereview.appspot.com/648813002/diff/310001/content/browser/...
File content/browser/loader/navigation_url_loader_impl.h (right):

https://chromiumcodereview.appspot.com/648813002/diff/310001/content/browser/...
content/browser/loader/navigation_url_loader_impl.h:16: class
NavigationURLLoaderImpl : public NavigationURLLoader {
So NavigationURLLoaderImpl has really been reduced to bare bones, and it doesn't
seem to be doing much. Is there no way we could fold NavigationURLLoaderCore
into it?

https://chromiumcodereview.appspot.com/648813002/diff/310001/content/browser/...
File content/browser/loader/navigation_url_loader_unittest.cc (right):

https://chromiumcodereview.appspot.com/648813002/diff/310001/content/browser/...
content/browser/loader/navigation_url_loader_unittest.cc:215: // Tests that
request failures are propogated correctly.
s/propogated/propagated

https://chromiumcodereview.appspot.com/648813002/diff/310001/content/browser/...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://chromiumcodereview.appspot.com/648813002/diff/310001/content/browser/...
content/browser/loader/resource_dispatcher_host_impl.cc:1305: // not needed.
CreateResourceHandler is not called by BeginNavigationRequest. Hence is this
check here because of the subsequent call that could be made when the renderer
requests the blob URL? If so, could it be precise in comments?

https://chromiumcodereview.appspot.com/648813002/diff/310001/content/browser/...
content/browser/loader/resource_dispatcher_host_impl.cc:1310: // handler either
if so or if a cross-process navigation is already under
nit: We are not pausing for the unload anymore, so this should be removed from
the comment.

https://chromiumcodereview.appspot.com/648813002/diff/310001/content/browser/...
content/browser/loader/resource_dispatcher_host_impl.cc:1925:
scoped_ptr<ResourceHandler> handler(new NavigationResourceHandler(
The code following there is fairly close to CreateResourceHandler - could it be
put in CreateResourceHandler?

[davidben, 2014-10-15 22:53:29]

https://codereview.chromium.org/648813002/diff/310001/content/browser/loader/...
File content/browser/loader/navigation_resource_handler.h (right):

https://codereview.chromium.org/648813002/diff/310001/content/browser/loader/...
content/browser/loader/navigation_resource_handler.h:17: // The leaf
ResourceHandler used with NavigationURLLoaderCore.
On 2014/10/13 15:22:42, clamy wrote:
> nit: maybe prefix it with PlzNavigate?

Done.

https://codereview.chromium.org/648813002/diff/310001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_impl.h (right):

https://codereview.chromium.org/648813002/diff/310001/content/browser/loader/...
content/browser/loader/navigation_url_loader_impl.h:16: class
NavigationURLLoaderImpl : public NavigationURLLoader {
On 2014/10/13 15:22:42, clamy wrote:
> So NavigationURLLoaderImpl has really been reduced to bare bones, and it
doesn't
> seem to be doing much. Is there no way we could fold NavigationURLLoaderCore
> into it?

It's largely so that it can be uniquely owned rather than reference-counted.
net/url_request/url_fetcher has a similar Impl/Core split.

The other option I can think of is to have Core be uniquely owned and live
entirely on the IO thread, though that wouldn't let us fold the two together.
But we could have something like...

NavigationURLLoaderImpl has
  scoped_ptr<NavigationURLLoaderCore, BrowserThread::DeleteOnIOThread> core_;
  base::WeakFactory<NavigationURLLoaderDelegate> weak_delegate_factory_;

It does PostTasks to IO thread with base::Unretained(core_.get()) in ctor and
FollowRedirect.

NavigationURLLoaderCore lives entirely on IO thread and is passed a
base::WeakPtr<Delegate> to be called on the UI thread.

NavigationURLLoaderCore again communicates with NavigationResourceHandler (which
is largely a kludge around the RDH not having a useful external API and owning
all the ResourceLoaders for you; something I want to clean up, but in parallel).
When it needs to call something on the delegate, it PostTasks back to the UI
thread with the WeakPtr.

(Really I'd like Core to be an inner class of Impl, but the ResourceHandler has
to be able to communicate with Core because of the RDH thing.)

https://codereview.chromium.org/648813002/diff/310001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_unittest.cc (right):

https://codereview.chromium.org/648813002/diff/310001/content/browser/loader/...
content/browser/loader/navigation_url_loader_unittest.cc:215: // Tests that
request failures are propogated correctly.
On 2014/10/13 15:22:42, clamy wrote:
> s/propogated/propagated

Done. (Somehow I never spell that word correctly.)

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

https://codereview.chromium.org/648813002/diff/310001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1305: // not needed.
On 2014/10/13 15:22:43, clamy wrote:
> CreateResourceHandler is not called by BeginNavigationRequest. Hence is this
> check here because of the subsequent call that could be made when the renderer
> requests the blob URL? If so, could it be precise in comments?

Done.

https://codereview.chromium.org/648813002/diff/310001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1310: // handler either
if so or if a cross-process navigation is already under
On 2014/10/13 15:22:43, clamy wrote:
> nit: We are not pausing for the unload anymore, so this should be removed from
> the comment.

Done.

https://codereview.chromium.org/648813002/diff/310001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1925:
scoped_ptr<ResourceHandler> handler(new NavigationResourceHandler(
On 2014/10/13 15:22:43, clamy wrote:
> The code following there is fairly close to CreateResourceHandler - could it
be
> put in CreateResourceHandler?

Done.

[clamy, 2014-10-16 13:43:51]

Thanks! Lgtm with a nit.

https://chromiumcodereview.appspot.com/648813002/diff/460001/content/browser/...
File content/browser/loader/resource_dispatcher_host_impl.h (right):

https://chromiumcodereview.appspot.com/648813002/diff/460001/content/browser/...
content/browser/loader/resource_dispatcher_host_impl.h:410: // loading and
navigation requests.
nit: Maybe precise which kind of ResourceHandlers can be added by this function?

[nasko, 2014-10-20 13:52:18]

Mostly a bunch of nits.

https://codereview.chromium.org/648813002/diff/460001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl_unittest.cc (right):

https://codereview.chromium.org/648813002/diff/460001/content/browser/frame_h...
content/browser/frame_host/navigator_impl_unittest.cc:52: if (delegate_)
What would set delegate_ to nullptr?

https://codereview.chromium.org/648813002/diff/460001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_core.h (right):

https://codereview.chromium.org/648813002/diff/460001/content/browser/loader/...
content/browser/loader/navigation_url_loader_core.h:55: // Note: once the
response headers are received, the request is considered
nit: No need for an empty line above.

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

https://codereview.chromium.org/648813002/diff/460001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1365: 
nit: no need for an extra new line.

https://codereview.chromium.org/648813002/diff/460001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1801: //
BeginNavigationRequest currently should only be used for the browser-side
nit: prefix the comment with PlzNavigate

https://codereview.chromium.org/648813002/diff/460001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1822: //
http://crbug.com/90971
nit: the comment should say a bit more than the bug link. Something like "The
following code is here to help catch bug ..."

https://codereview.chromium.org/648813002/diff/460001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1850: // prerender.
There isn't a renderer process yet, so we need to use the
nit: There actually might be a process, right? We want to choose it at a later
point, but the one we pick might already exist.

https://codereview.chromium.org/648813002/diff/460001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1894: // TODO(davidben):
Associate the request with the FrameTreeNode or tab so that
Should be FrameTreeNode, not tab, right? If we associate with tab, then multiple
requests in subframes can all resolve to the same tab, but will be 1:1 with FTN.

https://codereview.chromium.org/648813002/diff/460001/content/public/common/r...
File content/public/common/resource_devtools_info.h (right):

https://codereview.chromium.org/648813002/diff/460001/content/public/common/r...
content/public/common/resource_devtools_info.h:25: int32 http_status_code;
nit: I'd put a comment to remind people to update DeepCopy if they add new
members.

https://codereview.chromium.org/648813002/diff/460001/content/public/common/r...
File content/public/common/resource_response.cc (right):

https://codereview.chromium.org/648813002/diff/460001/content/public/common/r...
content/public/common/resource_response.cc:48: return new_response;
I counted a few more fields exist than copied here. will correlate later to see
which are missing, but this indicates a problem to me - easy to forget to add a
field here when added to the struct.

[mmenke, 2014-10-20 18:52:13]

https://codereview.chromium.org/648813002/diff/460001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_core.h (right):

https://codereview.chromium.org/648813002/diff/460001/content/browser/loader/...
content/browser/loader/navigation_url_loader_core.h:32: // thread. It passes
signals across threads between the two objects.
This currently doesn't pass messages to the NavigationURLLoaderImpl - it only
passes them to the NavigationURLLoader::Delegate.  I think this is a mistake -
it should only talk to the NavigationURLLoader on the UI thread, and that should
talk to the delegate.

https://codereview.chromium.org/648813002/diff/460001/content/browser/loader/...
content/browser/loader/navigation_url_loader_core.h:34: : public
base::RefCountedThreadSafe<NavigationURLLoaderCore> {
Do we really need a refcounted class for this?  Seems like we could just make
this an IO thread class, and have this and the NavigationURLLoader post messages
to each other via weak pointers.

[mmenke, 2014-10-20 18:55:18]

https://codereview.chromium.org/648813002/diff/460001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_core.h (right):

https://codereview.chromium.org/648813002/diff/460001/content/browser/loader/...
content/browser/loader/navigation_url_loader_core.h:34: : public
base::RefCountedThreadSafe<NavigationURLLoaderCore> {
On 2014/10/20 18:52:13, mmenke wrote:
> Do we really need a refcounted class for this?  Seems like we could just make
> this an IO thread class, and have this and the NavigationURLLoader post
messages
> to each other via weak pointers.

Actually, I need to figure out ownership semantics, but do we really not know
which will outlive the other, the *Loader or the *Handler?

[davidben, 2014-10-22 01:32:38]

Okay, reworked the Impl/Core/Handler ownership story. Core is now an IO-thread
object and unambiguously owned by Impl (Impl does a PostTask to IO thread to
destroy it. Core -> Impl is done via a WeakPtr). Core <-> Handler is sillier
because of the ResourceLoader ownership issue.

https://codereview.chromium.org/648813002/diff/460001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl_unittest.cc (right):

https://codereview.chromium.org/648813002/diff/460001/content/browser/frame_h...
content/browser/frame_host/navigator_impl_unittest.cc:52: if (delegate_)
On 2014/10/20 13:52:17, nasko wrote:
> What would set delegate_ to nullptr?

Oh hrm. I think CallOnResponseStarted used to null it. Removed that. The test
code knows whether or not it's calling CallOn* anyway.

The test that used this also got lost. Reworked that test to check a WeakPtr
instead which seems more direct than those counts.

https://codereview.chromium.org/648813002/diff/460001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_core.h (right):

https://codereview.chromium.org/648813002/diff/460001/content/browser/loader/...
content/browser/loader/navigation_url_loader_core.h:32: // thread. It passes
signals across threads between the two objects.
On 2014/10/20 18:52:13, mmenke wrote:
> This currently doesn't pass messages to the NavigationURLLoaderImpl - it only
> passes them to the NavigationURLLoader::Delegate.  I think this is a mistake -
> it should only talk to the NavigationURLLoader on the UI thread, and that
should
> talk to the delegate.

Done.

https://codereview.chromium.org/648813002/diff/460001/content/browser/loader/...
content/browser/loader/navigation_url_loader_core.h:34: : public
base::RefCountedThreadSafe<NavigationURLLoaderCore> {
On 2014/10/20 18:55:18, mmenke wrote:
> On 2014/10/20 18:52:13, mmenke wrote:
> > Do we really need a refcounted class for this?  Seems like we could just
make
> > this an IO thread class, and have this and the NavigationURLLoader post
> messages
> > to each other via weak pointers.
> 
> Actually, I need to figure out ownership semantics, but do we really not know
> which will outlive the other, the *Loader or the *Handler?

Reworked so that Core is owned by Impl, but cross-thread. Handler/Core is still
screwy because the ResourceLoader isn't owned by the Core (yet...).

https://codereview.chromium.org/648813002/diff/460001/content/browser/loader/...
content/browser/loader/navigation_url_loader_core.h:55: // Note: once the
response headers are received, the request is considered
On 2014/10/20 13:52:18, nasko wrote:
> nit: No need for an empty line above.

This class got reworked.

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

https://codereview.chromium.org/648813002/diff/460001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1365: 
On 2014/10/20 13:52:18, nasko wrote:
> nit: no need for an extra new line.

Done.

https://codereview.chromium.org/648813002/diff/460001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1801: //
BeginNavigationRequest currently should only be used for the browser-side
On 2014/10/20 13:52:18, nasko wrote:
> nit: prefix the comment with PlzNavigate

Done.

https://codereview.chromium.org/648813002/diff/460001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1822: //
http://crbug.com/90971
On 2014/10/20 13:52:18, nasko wrote:
> nit: the comment should say a bit more than the bug link. Something like "The
> following code is here to help catch bug ..."

Done. (Just copied that from elsewhere.)

https://codereview.chromium.org/648813002/diff/460001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1850: // prerender.
There isn't a renderer process yet, so we need to use the
On 2014/10/20 13:52:18, nasko wrote:
> nit: There actually might be a process, right? We want to choose it at a later
> point, but the one we pick might already exist.

Done.

https://codereview.chromium.org/648813002/diff/460001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1894: // TODO(davidben):
Associate the request with the FrameTreeNode or tab so that
On 2014/10/20 13:52:18, nasko wrote:
> Should be FrameTreeNode, not tab, right? If we associate with tab, then
multiple
> requests in subframes can all resolve to the same tab, but will be 1:1 with
FTN.

Well, I'm actually not sure if we have anything that actually requires finding
the frame, just the overall tab. It's largely for stapling HTTP auth prompts.
(Pre-OOPIF we don't really track the frame tree browser-side anyway, so there
can't be much code yet that cares, right?) I switched it to and/or.

https://codereview.chromium.org/648813002/diff/460001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.h (right):

https://codereview.chromium.org/648813002/diff/460001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.h:410: // loading and
navigation requests.
On 2014/10/16 13:43:51, clamy (OOO until 11-11) wrote:
> nit: Maybe precise which kind of ResourceHandlers can be added by this
function?

Done.

https://codereview.chromium.org/648813002/diff/460001/content/public/common/r...
File content/public/common/resource_devtools_info.h (right):

https://codereview.chromium.org/648813002/diff/460001/content/public/common/r...
content/public/common/resource_devtools_info.h:25: int32 http_status_code;
On 2014/10/20 13:52:18, nasko wrote:
> nit: I'd put a comment to remind people to update DeepCopy if they add new
> members.

Done.

https://codereview.chromium.org/648813002/diff/460001/content/public/common/r...
File content/public/common/resource_response.cc (right):

https://codereview.chromium.org/648813002/diff/460001/content/public/common/r...
content/public/common/resource_response.cc:48: return new_response;
On 2014/10/20 13:52:18, nasko wrote:
> I counted a few more fields exist than copied here. will correlate later to
see
> which are missing, but this indicates a problem to me - easy to forget to add
a
> field here when added to the struct.

Probably a rebase. Added was_fallback_required_by_service_worker which seems to
have been the new one. But, yeah, I agree. I don't really have a good answer
though. We can almost pass unique ownership down but for the
BufferedResourceHandler dispatch. I'd like that dispatch to move to the
navigation layer, but with or without that, we need to be able to plumb a copy
of the ResourceResponse down to DevTools.

This structure is basically copyable anyway via IPC. It's just not copyable
in-memory, which is kind of bizarre.

If we could instead ensure that the ResourceResponse is not modified past a
certain point, that would be another option. It's currently modified all over
the place though. BufferedResourceHandler makes some modifications.
AsyncResourceHandler staples DevTools info. ChromeResourceDispatcherHostDelegate
strips some headers in some place. I'm sure ServiceWorker makes modifications
too. But I'd be uneasy unless we could make the compiler enforce whatever
immutability we decide on.

[mmenke, 2014-10-22 15:32:59]

Focused on the loader code, haven't yet looked at RDH.

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
File content/browser/loader/navigation_resource_handler.cc (right):

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
content/browser/loader/navigation_resource_handler.cc:32:
core_->NotifyRequestFailed(net::ERR_ABORTED);
Can we end up here without an OnResponseCompleted first, except perhaps during
RDH teardown, at which point, core will have been deleted first, anyways?

This is one case in which the extra check may be worth keeping, but wondering if
it's actually needed.

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
content/browser/loader/navigation_resource_handler.cc:34: core_ = nullptr;
I don't think this gets us anything.

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
content/browser/loader/navigation_resource_handler.cc:61: if (!core_)
Can this happen?  We would have cancelled it by this point, which I think would
prevent us from getting the redirect message.  Same goes for all the other core
checks, except the one on destruction.

If it can't happen, suggest DCHECKs instead.

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
content/browser/loader/navigation_resource_handler.cc:132: // status down. (If
there was an error partway through loading the top-level.)
Do we currently replace committed partially loaded pages with network error
pages?

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
content/browser/loader/navigation_resource_handler.cc:134: writer_.Finalize();
Telling WebKit a blob is happily completed on error doesn't break anything,
right?  For a reload, it will re-request the resource from the browser process,
and get a new blob, rather than using the cached old one (I assume it doesn't
even cache blobs, actually).

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
content/browser/loader/navigation_resource_handler.cc:142: core_ = nullptr;
Are the last two lines needed?  We end up destroying |this| anyways, don't we,
which will end up doing the same thing.

If they are needed, this block is duplicated in the destructor.  Suggest adding
a method with the
shared code.

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
File content/browser/loader/navigation_url_loader.h (right):

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
content/browser/loader/navigation_url_loader.h:34: class CONTENT_EXPORT Delegate
{
I believe that there's a preference to make delegates/observers separate
top-level classes, rather than nesting them like this.

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
content/browser/loader/navigation_url_loader.h:84: protected:
nit:  +1 indent, here and below.

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_core.cc (right):

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
content/browser/loader/navigation_url_loader_core.cc:29:
resource_handler_->Cancel();
Not a real issue, but just for my edification...This does not result in calling
into NavigationURLLoaderCore::NotifyRequestFailed, because URLRequest passes the
cancellation message back to the ResourceLoader asynchronously, by which point
this has been deleted, anyways.  Not that NotifyRequestFailed would be a
problem, anyways, due to the whole WeakPtr thing.

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
content/browser/loader/navigation_url_loader_core.cc:30: resource_handler_ =
nullptr;
NULLing out resource_handler_ doesn't seem to get us anything.

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_core.h (right):

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
content/browser/loader/navigation_url_loader_core.h:34: class
NavigationURLLoaderCore {
Think "NavigationURLLoaderImplCore" is clearer.

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
content/browser/loader/navigation_url_loader_core.h:38:
NavigationURLLoaderCore(const base::WeakPtr<NavigationURLLoaderImpl>& loader);
explicit

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
content/browser/loader/navigation_url_loader_core.h:48: // After a redirect to
follow the redirect.
nit:  Grammar seems a bit weird here.  Maybe just "Follow the currently pending
redirect."?

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_impl.cc (right):

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
content/browser/loader/navigation_url_loader_impl.cc:24: : delegate_(delegate),
nit:  +2 indent

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
content/browser/loader/navigation_url_loader_impl.cc:40: void
NavigationURLLoaderImpl::FollowRedirect() {
Suggest adding thread dchecks here and in NavigationURLLoaderCore

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_impl.h (right):

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
content/browser/loader/navigation_url_loader_impl.h:50: NavigationURLLoaderCore*
core_;
optional:  Suggest making this a scoped_ptr - it's a bug if it's actually
deleted by the destruction of the scoped pointer, of course, but think that
makes ownership a little clearer (That comment certainly helps, too, of course).

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
File content/browser/loader/upload_data_stream_builder.cc (right):

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
content/browser/loader/upload_data_stream_builder.cc:120:
CHECK(file_system_context);
Any reason for this not to just be a DCHECK?

[davidben, 2014-10-22 20:58:04]

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
File content/browser/loader/navigation_resource_handler.cc (right):

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
content/browser/loader/navigation_resource_handler.cc:32:
core_->NotifyRequestFailed(net::ERR_ABORTED);
On 2014/10/22 15:32:58, mmenke wrote:
> Can we end up here without an OnResponseCompleted first, except perhaps during
> RDH teardown, at which point, core will have been deleted first, anyways?
> 
> This is one case in which the extra check may be worth keeping, but wondering
if
> it's actually needed.

If the ResourceLoader is uncleanly torn down, it can happen. I'm not sure if
NavigationURLLoader will always be gone by then. There's
CancelRequestsForContext, but presumably if the ResourceContext is going away,
anyone holder a Loader has already dropped it. There's CancelRequestsForRoute,
but these aren't owned by a route. There's also CancelRequest, though I dunno if
that's called outside of downloads.

I think it's better to be safe here until we finish sorting out
non-renderer-owned ResourceLoader lifetimes.

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
content/browser/loader/navigation_resource_handler.cc:34: core_ = nullptr;
On 2014/10/22 15:32:58, mmenke wrote:
> I don't think this gets us anything.

Moved into helper method.

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
content/browser/loader/navigation_resource_handler.cc:61: if (!core_)
On 2014/10/22 15:32:58, mmenke wrote:
> Can this happen?  We would have cancelled it by this point, which I think
would
> prevent us from getting the redirect message.  Same goes for all the other
core
> checks, except the one on destruction.
> 
> If it can't happen, suggest DCHECKs instead.

I'm never clear on what can happen with all our asynchronous cancels. But it
does call CancelWithError, so I think I agree. Anything happening after cancel
is a bug. (There's probably one of those Cancel/Resume races we still haven't
sorted out...)

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
content/browser/loader/navigation_resource_handler.cc:132: // status down. (If
there was an error partway through loading the top-level.)
On 2014/10/22 15:32:58, mmenke wrote:
> Do we currently replace committed partially loaded pages with network error
> pages?

No, but it apparently isn't a no-op as far as Blink is concerned either.
https://crbug.com/422246. I've filed https://crbug.com/426162 to resolve it
separately and left it as a TODO for now.

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
content/browser/loader/navigation_resource_handler.cc:134: writer_.Finalize();
On 2014/10/22 15:32:58, mmenke wrote:
> Telling WebKit a blob is happily completed on error doesn't break anything,
> right?  For a reload, it will re-request the resource from the browser
process,
> and get a new blob, rather than using the cached old one (I assume it doesn't
> even cache blobs, actually).

Left a TODO and filed https://crbug.com/426162

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
content/browser/loader/navigation_resource_handler.cc:142: core_ = nullptr;
On 2014/10/22 15:32:58, mmenke wrote:
> Are the last two lines needed?  We end up destroying |this| anyways, don't we,
> which will end up doing the same thing.

If we don't detach at this point, then the destructor will NotifyRequestFailed a
second time.

> If they are needed, this block is duplicated in the destructor.  Suggest
adding
> a method with the
> shared code.

Done.

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
File content/browser/loader/navigation_url_loader.h (right):

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
content/browser/loader/navigation_url_loader.h:34: class CONTENT_EXPORT Delegate
{
On 2014/10/22 15:32:58, mmenke wrote:
> I believe that there's a preference to make delegates/observers separate
> top-level classes, rather than nesting them like this.

Done. (Another file? Fine.)

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
content/browser/loader/navigation_url_loader.h:84: protected:
On 2014/10/22 15:32:58, mmenke wrote:
> nit:  +1 indent, here and below.

Done. Huh, I wonder why emacs is failing me here...

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_core.cc (right):

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
content/browser/loader/navigation_url_loader_core.cc:29:
resource_handler_->Cancel();
On 2014/10/22 15:32:58, mmenke wrote:
> Not a real issue, but just for my edification...This does not result in
calling
> into NavigationURLLoaderCore::NotifyRequestFailed, because URLRequest passes
the
> cancellation message back to the ResourceLoader asynchronously, by which point
> this has been deleted, anyways.  Not that NotifyRequestFailed would be a
> problem, anyways, due to the whole WeakPtr thing.

Well, there isn't a WeakPtr on this end. The Cancel hook is how the resource
handler knows the Core has gone away. It NULLs core_ at the end of Cancel.

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
content/browser/loader/navigation_url_loader_core.cc:30: resource_handler_ =
nullptr;
On 2014/10/22 15:32:58, mmenke wrote:
> NULLing out resource_handler_ doesn't seem to get us anything.

Done.

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_core.h (right):

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
content/browser/loader/navigation_url_loader_core.h:34: class
NavigationURLLoaderCore {
On 2014/10/22 15:32:58, mmenke wrote:
> Think "NavigationURLLoaderImplCore" is clearer.

Done. (So long...)

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
content/browser/loader/navigation_url_loader_core.h:38:
NavigationURLLoaderCore(const base::WeakPtr<NavigationURLLoaderImpl>& loader);
On 2014/10/22 15:32:58, mmenke wrote:
> explicit

Done.

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
content/browser/loader/navigation_url_loader_core.h:48: // After a redirect to
follow the redirect.
On 2014/10/22 15:32:58, mmenke wrote:
> nit:  Grammar seems a bit weird here.  Maybe just "Follow the currently
pending
> redirect."?

Done.

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_impl.cc (right):

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
content/browser/loader/navigation_url_loader_impl.cc:24: : delegate_(delegate),
On 2014/10/22 15:32:58, mmenke wrote:
> nit:  +2 indent

Done.

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
content/browser/loader/navigation_url_loader_impl.cc:40: void
NavigationURLLoaderImpl::FollowRedirect() {
On 2014/10/22 15:32:58, mmenke wrote:
> Suggest adding thread dchecks here and in NavigationURLLoaderCore

Done. (I actually ended up removing them now that all the objects live on a
single thread, but they're probably still worthwhile.)

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_impl.h (right):

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
content/browser/loader/navigation_url_loader_impl.h:50: NavigationURLLoaderCore*
core_;
On 2014/10/22 15:32:58, mmenke wrote:
> optional:  Suggest making this a scoped_ptr - it's a bug if it's actually
> deleted by the destruction of the scoped pointer, of course, but think that
> makes ownership a little clearer (That comment certainly helps, too, of
course).

So, I originally made it a scoped_ptr<Blah, BrowserThread::DeleteOnIOThread>.
The problem is that DeleteOnIOThread tries to be clever and breaks the unit
tests. I.e. this pattern is not actually safe:

scoped_ptr<T, DeleteOnIOThread> t(new T);
PostTask(IO, FROM_HERE, base::Bind(&DoSomething, base::Unretained(t.get()));
PostTask(IO, FROM_HERE, base::Bind(&DoSomethingElse, base::Unretained(t.get()));
t.reset();

If the current thread is not the IO thread, this works great because of PostTask
ordering. But if the current thread is the IO thread, DeleteOnIOThread is clever
and deletes the object immediately. In unit tests, UI and IO thread are aliased
to the same thread and badness ensues.

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
File content/browser/loader/upload_data_stream_builder.cc (right):

https://codereview.chromium.org/648813002/diff/480001/content/browser/loader/...
content/browser/loader/upload_data_stream_builder.cc:120:
CHECK(file_system_context);
On 2014/10/22 15:32:59, mmenke wrote:
> Any reason for this not to just be a DCHECK?

Dug up the long comment here:
https://codereview.chromium.org/519533002/diff/200001/content/browser/loader/...

I think mostly so we could find out if it ever happened. A browser-initiated
request would legitimately not have a FileSystemContext because it's the context
of the initiator; and a browser-initiated navigation would also never include a
TYPE_FILE_FILESYSTEM. But a renderer-initiated request should always be relative
to the context of that renderer.

I vaguely suspect that we'll want to actually do this by moving this dance
closer to the renderer-only entry point and the browser-initiated paths only
create a net::UploadDataStream or something.

[mmenke, 2014-10-23 20:43:40]

I'm pretty happy with the loader code

https://codereview.chromium.org/648813002/diff/500001/content/browser/loader/...
File content/browser/loader/navigation_url_loader.h (right):

https://codereview.chromium.org/648813002/diff/500001/content/browser/loader/...
content/browser/loader/navigation_url_loader.h:53: protected:
Doesn't look like you fixed the indent here.

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

https://codereview.chromium.org/648813002/diff/500001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1315: // not require
request transfers.
What about renderer-initiated cross-site navigations, either when we expect them
from the start to be cross-site, or when we only learn they are after redirects?

https://codereview.chromium.org/648813002/diff/500001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1836:
CHECK(ContainsKey(active_resource_contexts_, resource_context));
Is this needed?

https://codereview.chromium.org/648813002/diff/500001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1857:
DCHECK_EQ(load_flags & net::LOAD_IGNORE_LIMITS, 0);
DCHECK(!(load_flags & net::LOAD_IGNORE_LIMITS))?

https://codereview.chromium.org/648813002/diff/500001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1920: // request
transfers and isn't needed for browser-side navigations.
So this will be needed when we handle renderer-initiated "navigations"
(Including reloads)

https://codereview.chromium.org/648813002/diff/500001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1959: -1,  //
TODO(davidben): route_id
Looks like these are only used by the scheduler - may be able to get away
without them, or could use the ones for the old renderer, and it probably
wouldn't be the end of the world.  Just be beginning of the end of the world.

[nasko, 2014-10-23 22:36:17]

Mostly a bunch of nits.

https://codereview.chromium.org/648813002/diff/500001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl_unittest.cc (right):

https://codereview.chromium.org/648813002/diff/500001/content/browser/frame_h...
content/browser/frame_host/navigator_impl_unittest.cc:56: redirect_count_++;
nit: Be consistent, as all other methods that have one liners are all on the
same line (when it fits, as this is the case here).

https://codereview.chromium.org/648813002/diff/500001/content/browser/loader/...
File content/browser/loader/navigation_resource_handler.cc (right):

https://codereview.chromium.org/648813002/diff/500001/content/browser/loader/...
content/browser/loader/navigation_resource_handler.cc:52: uint64 size) {
nit: this should fit on the previous line

https://codereview.chromium.org/648813002/diff/500001/content/browser/loader/...
File content/browser/loader/navigation_resource_handler.h (right):

https://codereview.chromium.org/648813002/diff/500001/content/browser/loader/...
content/browser/loader/navigation_resource_handler.h:31: 
nit: No empty lines between methods from the same interface.

https://codereview.chromium.org/648813002/diff/500001/content/browser/loader/...
content/browser/loader/navigation_resource_handler.h:39: bool* defer) override;
nit: this should fit on previous line.

https://codereview.chromium.org/648813002/diff/500001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_delegate.h (right):

https://codereview.chromium.org/648813002/diff/500001/content/browser/loader/...
content/browser/loader/navigation_url_loader_delegate.h:27: ResourceResponse*
response) = 0;
nit: empty line between the methods

https://codereview.chromium.org/648813002/diff/500001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_impl.h (right):

https://codereview.chromium.org/648813002/diff/500001/content/browser/loader/...
content/browser/loader/navigation_url_loader_impl.h:52:
NavigationURLLoaderDelegate *delegate_;
* goes with the type, not the variable.

https://codereview.chromium.org/648813002/diff/500001/content/browser/loader/...
content/browser/loader/navigation_url_loader_impl.h:55: //
|NavigationURLLoaderImpl goes out of scope.
Drop the | before NavigationURLLoaderImpl, as it is not a parameter name, but
rather a type.

https://codereview.chromium.org/648813002/diff/500001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_impl_core.cc (right):

https://codereview.chromium.org/648813002/diff/500001/content/browser/loader/...
content/browser/loader/navigation_url_loader_impl_core.cc:25: // This object is
created on the UI thread but otherwise lives entirely on the
nit: Let's add that it is also deleted on the IO thread, not that just living
there.

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

https://codereview.chromium.org/648813002/diff/500001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1340: net::URLRequest*
request,
nit: wrong indent, need only 4 spaces, not 6.

https://codereview.chromium.org/648813002/diff/500001/content/public/common/r...
File content/public/common/resource_response.cc (right):

https://codereview.chromium.org/648813002/diff/500001/content/public/common/r...
content/public/common/resource_response.cc:44:
head.original_url_via_service_worker;
I don't see response_type_via_service_worker being copied.

[davidben, 2014-10-27 20:46:05]

https://codereview.chromium.org/648813002/diff/500001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl_unittest.cc (right):

https://codereview.chromium.org/648813002/diff/500001/content/browser/frame_h...
content/browser/frame_host/navigator_impl_unittest.cc:56: redirect_count_++;
On 2014/10/23 22:36:16, nasko wrote:
> nit: Be consistent, as all other methods that have one liners are all on the
> same line (when it fits, as this is the case here).

Done.

https://codereview.chromium.org/648813002/diff/500001/content/browser/loader/...
File content/browser/loader/navigation_resource_handler.cc (right):

https://codereview.chromium.org/648813002/diff/500001/content/browser/loader/...
content/browser/loader/navigation_resource_handler.cc:52: uint64 size) {
On 2014/10/23 22:36:16, nasko wrote:
> nit: this should fit on the previous line

Done.

https://codereview.chromium.org/648813002/diff/500001/content/browser/loader/...
File content/browser/loader/navigation_resource_handler.h (right):

https://codereview.chromium.org/648813002/diff/500001/content/browser/loader/...
content/browser/loader/navigation_resource_handler.h:31: 
On 2014/10/23 22:36:16, nasko wrote:
> nit: No empty lines between methods from the same interface.

Done.

https://codereview.chromium.org/648813002/diff/500001/content/browser/loader/...
content/browser/loader/navigation_resource_handler.h:39: bool* defer) override;
On 2014/10/23 22:36:16, nasko wrote:
> nit: this should fit on previous line.

Done.

https://codereview.chromium.org/648813002/diff/500001/content/browser/loader/...
File content/browser/loader/navigation_url_loader.h (right):

https://codereview.chromium.org/648813002/diff/500001/content/browser/loader/...
content/browser/loader/navigation_url_loader.h:53: protected:
On 2014/10/23 20:43:40, mmenke wrote:
> Doesn't look like you fixed the indent here.

Mrrf. Done.

https://codereview.chromium.org/648813002/diff/500001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_delegate.h (right):

https://codereview.chromium.org/648813002/diff/500001/content/browser/loader/...
content/browser/loader/navigation_url_loader_delegate.h:27: ResourceResponse*
response) = 0;
On 2014/10/23 22:36:16, nasko wrote:
> nit: empty line between the methods

Done.

https://codereview.chromium.org/648813002/diff/500001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_impl.h (right):

https://codereview.chromium.org/648813002/diff/500001/content/browser/loader/...
content/browser/loader/navigation_url_loader_impl.h:52:
NavigationURLLoaderDelegate *delegate_;
On 2014/10/23 22:36:17, nasko wrote:
> * goes with the type, not the variable.

Done.

https://codereview.chromium.org/648813002/diff/500001/content/browser/loader/...
content/browser/loader/navigation_url_loader_impl.h:55: //
|NavigationURLLoaderImpl goes out of scope.
On 2014/10/23 22:36:16, nasko wrote:
> Drop the | before NavigationURLLoaderImpl, as it is not a parameter name, but
> rather a type.

Done. I think emacs M-q tried to be clever...

https://codereview.chromium.org/648813002/diff/500001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_impl_core.cc (right):

https://codereview.chromium.org/648813002/diff/500001/content/browser/loader/...
content/browser/loader/navigation_url_loader_impl_core.cc:25: // This object is
created on the UI thread but otherwise lives entirely on the
On 2014/10/23 22:36:17, nasko wrote:
> nit: Let's add that it is also deleted on the IO thread, not that just living
> there.

Done.

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

https://codereview.chromium.org/648813002/diff/500001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1315: // not require
request transfers.
On 2014/10/23 20:43:40, mmenke wrote:
> What about renderer-initiated cross-site navigations, either when we expect
them
> from the start to be cross-site, or when we only learn they are after
redirects?

So, that's part of the nice thing about inverting the control. We no longer care
whether a navigation becomes cross-site initially or after a redirect. We make a
request for the URL with no reference[1] to the renderer currently in the frame,
if there even is one. At OnResponseStarted, we pop over the UI thread which does
a cross-process vs same-process check once and spin up[2] a renderer from there.

The request transfer craziness is because we're not in a position where we can
do that today and have to undo our early renderer binding.

[1] Except that we ping it so it can trigger beforeunload and also see if this
is a same-document navigation; #fragment change and such.

[2] Well, when the navigation starts, we'd like to predictively spin up a
renderer based on the starting URL and throw it away if it turns out to have
redirected, so they actually can happen in parallel. But from the point of view
of the rest of the system, the renderer is bound late.

https://codereview.chromium.org/648813002/diff/500001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1340: net::URLRequest*
request,
On 2014/10/23 22:36:17, nasko wrote:
> nit: wrong indent, need only 4 spaces, not 6.

Done.

https://codereview.chromium.org/648813002/diff/500001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1836:
CHECK(ContainsKey(active_resource_contexts_, resource_context));
On 2014/10/23 20:43:40, mmenke wrote:
> Is this needed?

I pulled that from the other entry points. I guess it's to catch if you're
passing in an invalid ResourceContext somehow? *shrug* Can also remove it if we
don't have any reason to believe it'll help with anything.

https://codereview.chromium.org/648813002/diff/500001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1857:
DCHECK_EQ(load_flags & net::LOAD_IGNORE_LIMITS, 0);
On 2014/10/23 20:43:40, mmenke wrote:
> DCHECK(!(load_flags & net::LOAD_IGNORE_LIMITS))?

Done.

https://codereview.chromium.org/648813002/diff/500001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1920: // request
transfers and isn't needed for browser-side navigations.
On 2014/10/23 20:43:40, mmenke wrote:
> So this will be needed when we handle renderer-initiated "navigations"
> (Including reloads)

This is needed in the old navigation flow just for request transfers. Outside
request transfers, the RDH stack doesn't care about this value. Once PlzNavigate
is done, it's one of many many fields we can remove from ResourceRequestInfo.
:-)

(The reason for it is that request transfers require the resource loader stack
to be able to recreate any navigation. So all navigation state needs to be
plumbed into the resource layer.)



It's really unfortunate that browser-side vs renderer-side and browser-initiated
vs renderer-initiated are such similar terminology.

browser-side = PlzNavigate = browser drives navigation request.
benderer-side = current logic = renderer drives navigation request.

browser-initiated = navigation came from browser. Omnibox or whatever.
renderer-initiated = navigation came from a renderer. Click a link or whatever.

https://codereview.chromium.org/648813002/diff/500001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1959: -1,  //
TODO(davidben): route_id
On 2014/10/23 20:43:40, mmenke wrote:
> Looks like these are only used by the scheduler - may be able to get away
> without them, or could use the ones for the old renderer, and it probably
> wouldn't be the end of the world.  Just be beginning of the end of the world.

Yeah, it's currently a no-op because it can't find the entry in the ClientMap.
I've revised the TODO.

https://codereview.chromium.org/648813002/diff/500001/content/public/common/r...
File content/public/common/resource_response.cc (right):

https://codereview.chromium.org/648813002/diff/500001/content/public/common/r...
content/public/common/resource_response.cc:44:
head.original_url_via_service_worker;
On 2014/10/23 22:36:17, nasko wrote:
> I don't see response_type_via_service_worker being copied.

Aargh, they keep adding things. Done. (I... very... strongly believe
ServiceWorker is hooking in at the wrong layer but have been unsuccessful in
arguing otherwise. Maybe in the future things will be layered better and I can
try that fight again.)

[nasko, 2014-10-27 23:03:08]

LGTM! Now, commit quickly, so we don't get another change to ResourceResponse
and need to update DeepCopy ;)

[mmenke, 2014-10-28 16:05:39]

https://codereview.chromium.org/648813002/diff/540001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_delegate.h (right):

https://codereview.chromium.org/648813002/diff/540001/content/browser/loader/...
content/browser/loader/navigation_url_loader_delegate.h:27: ResourceResponse*
response) = 0;
Suggest making this a "const scoped_refptr&".  That makes ownership semantics
much clearer.

https://codereview.chromium.org/648813002/diff/540001/content/browser/loader/...
content/browser/loader/navigation_url_loader_delegate.h:33:
scoped_ptr<StreamHandle> body) = 0;
nit:  Maybe body_stream?  Or body_blob?  Or bloby_body_stream or something.

https://codereview.chromium.org/648813002/diff/540001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_unittest.cc (right):

https://codereview.chromium.org/648813002/diff/540001/content/browser/loader/...
content/browser/loader/navigation_url_loader_unittest.cc:54: private:
nit:  Linbreak before private

https://codereview.chromium.org/648813002/diff/540001/content/browser/loader/...
content/browser/loader/navigation_url_loader_unittest.cc:55: StreamRegistry*
registry_;
DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/648813002/diff/540001/content/browser/loader/...
content/browser/loader/navigation_url_loader_unittest.cc:129: ResourceContext*
resource_context) override {
NOTREACHED()?

https://codereview.chromium.org/648813002/diff/540001/content/browser/loader/...
content/browser/loader/navigation_url_loader_unittest.cc:213:
FetchURL(delegate.body()->GetURL()));
Should we make sure only the expected callback is invoked?  Since
BrowserTestThreadBundle one uses one thread, could just only create the run loop
you're currently waiting on.  Since you're reusing response() for both redirects
and successful completion, you're already relying on the previous call to Quit()
immediately exiting the message loop before the next run loop in the chain is
quit, so no concerns there, either.

https://codereview.chromium.org/648813002/diff/540001/content/browser/loader/...
content/browser/loader/navigation_url_loader_unittest.cc:243:
EXPECT_EQ("HTTP/1.1 302 MOVED",
Could we just check the response_code instead?  Relying on the HTTP version and
reason phrase seems a bit much, since URLRequestTestJob does not currently
expose them, or document them in the header.

https://codereview.chromium.org/648813002/diff/540001/content/browser/loader/...
content/browser/loader/navigation_url_loader_unittest.cc:249:
EXPECT_TRUE(net::URLRequestTestJob::ProcessOnePendingMessage());
What does ProcessOnePendingMessage actually do?  It's clearly not needed for
non-redirect requests, reading response bodies, or errors, though the
documentation doesn't seem to say anything about this.

[mmenke, 2014-10-28 18:23:57]

Oh, and loader/ LGTM.

On 2014/10/28 16:05:39, mmenke wrote:
>
https://codereview.chromium.org/648813002/diff/540001/content/browser/loader/...
> File content/browser/loader/navigation_url_loader_delegate.h (right):
> 
>
https://codereview.chromium.org/648813002/diff/540001/content/browser/loader/...
> content/browser/loader/navigation_url_loader_delegate.h:27: ResourceResponse*
> response) = 0;
> Suggest making this a "const scoped_refptr&".  That makes ownership semantics
> much clearer.
> 
>
https://codereview.chromium.org/648813002/diff/540001/content/browser/loader/...
> content/browser/loader/navigation_url_loader_delegate.h:33:
> scoped_ptr<StreamHandle> body) = 0;
> nit:  Maybe body_stream?  Or body_blob?  Or bloby_body_stream or something.
> 
>
https://codereview.chromium.org/648813002/diff/540001/content/browser/loader/...
> File content/browser/loader/navigation_url_loader_unittest.cc (right):
> 
>
https://codereview.chromium.org/648813002/diff/540001/content/browser/loader/...
> content/browser/loader/navigation_url_loader_unittest.cc:54: private:
> nit:  Linbreak before private
> 
>
https://codereview.chromium.org/648813002/diff/540001/content/browser/loader/...
> content/browser/loader/navigation_url_loader_unittest.cc:55: StreamRegistry*
> registry_;
> DISALLOW_COPY_AND_ASSIGN?
> 
>
https://codereview.chromium.org/648813002/diff/540001/content/browser/loader/...
> content/browser/loader/navigation_url_loader_unittest.cc:129: ResourceContext*
> resource_context) override {
> NOTREACHED()?
> 
>
https://codereview.chromium.org/648813002/diff/540001/content/browser/loader/...
> content/browser/loader/navigation_url_loader_unittest.cc:213:
> FetchURL(delegate.body()->GetURL()));
> Should we make sure only the expected callback is invoked?  Since
> BrowserTestThreadBundle one uses one thread, could just only create the run
loop
> you're currently waiting on.  Since you're reusing response() for both
redirects
> and successful completion, you're already relying on the previous call to
Quit()
> immediately exiting the message loop before the next run loop in the chain is
> quit, so no concerns there, either.
> 
>
https://codereview.chromium.org/648813002/diff/540001/content/browser/loader/...
> content/browser/loader/navigation_url_loader_unittest.cc:243:
> EXPECT_EQ("HTTP/1.1 302 MOVED",
> Could we just check the response_code instead?  Relying on the HTTP version
and
> reason phrase seems a bit much, since URLRequestTestJob does not currently
> expose them, or document them in the header.
> 
>
https://codereview.chromium.org/648813002/diff/540001/content/browser/loader/...
> content/browser/loader/navigation_url_loader_unittest.cc:249:
> EXPECT_TRUE(net::URLRequestTestJob::ProcessOnePendingMessage());
> What does ProcessOnePendingMessage actually do?  It's clearly not needed for
> non-redirect requests, reading response bodies, or errors, though the
> documentation doesn't seem to say anything about this.

[davidben, 2014-10-29 19:10:34]

https://codereview.chromium.org/648813002/diff/540001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_delegate.h (right):

https://codereview.chromium.org/648813002/diff/540001/content/browser/loader/...
content/browser/loader/navigation_url_loader_delegate.h:27: ResourceResponse*
response) = 0;
On 2014/10/28 16:05:39, mmenke wrote:
> Suggest making this a "const scoped_refptr&".  That makes ownership semantics
> much clearer.

Done. At least within loader/.

https://codereview.chromium.org/648813002/diff/540001/content/browser/loader/...
content/browser/loader/navigation_url_loader_delegate.h:33:
scoped_ptr<StreamHandle> body) = 0;
On 2014/10/28 16:05:39, mmenke wrote:
> nit:  Maybe body_stream?  Or body_blob?  Or bloby_body_stream or something.

*shrug* I feel like body is pretty descriptive. StreamHandle just happens to be
the mechanism we currently use to convey the body, but it won't be that forever.
Should it then be renamed to body_data_pipe or whatever? frame_host is already
calling it plain |body| and renaming it from |body_stream| to |body| partway
through seems confusing. (And part of the point of landing the frame_host
changes earlier was so this CL doesn't keep growing...)

https://codereview.chromium.org/648813002/diff/540001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_unittest.cc (right):

https://codereview.chromium.org/648813002/diff/540001/content/browser/loader/...
content/browser/loader/navigation_url_loader_unittest.cc:54: private:
On 2014/10/28 16:05:39, mmenke wrote:
> nit:  Linbreak before private

Done.

https://codereview.chromium.org/648813002/diff/540001/content/browser/loader/...
content/browser/loader/navigation_url_loader_unittest.cc:55: StreamRegistry*
registry_;
On 2014/10/28 16:05:39, mmenke wrote:
> DISALLOW_COPY_AND_ASSIGN?

Done.

https://codereview.chromium.org/648813002/diff/540001/content/browser/loader/...
content/browser/loader/navigation_url_loader_unittest.cc:129: ResourceContext*
resource_context) override {
On 2014/10/28 16:05:39, mmenke wrote:
> NOTREACHED()?

That's supposed to be reached. It's part of a test checking that this obeys
ShouldBeginRequest.

https://codereview.chromium.org/648813002/diff/540001/content/browser/loader/...
content/browser/loader/navigation_url_loader_unittest.cc:213:
FetchURL(delegate.body()->GetURL()));
On 2014/10/28 16:05:39, mmenke wrote:
> Should we make sure only the expected callback is invoked?  Since
> BrowserTestThreadBundle one uses one thread, could just only create the run
loop
> you're currently waiting on.  Since you're reusing response() for both
redirects
> and successful completion, you're already relying on the previous call to
Quit()
> immediately exiting the message loop before the next run loop in the chain is
> quit, so no concerns there, either.

Done. Also split up the responses.

https://codereview.chromium.org/648813002/diff/540001/content/browser/loader/...
content/browser/loader/navigation_url_loader_unittest.cc:243:
EXPECT_EQ("HTTP/1.1 302 MOVED",
On 2014/10/28 16:05:39, mmenke wrote:
> Could we just check the response_code instead?  Relying on the HTTP version
and
> reason phrase seems a bit much, since URLRequestTestJob does not currently
> expose them, or document them in the header.

Done.

https://codereview.chromium.org/648813002/diff/540001/content/browser/loader/...
content/browser/loader/navigation_url_loader_unittest.cc:249:
EXPECT_TRUE(net::URLRequestTestJob::ProcessOnePendingMessage());
On 2014/10/28 16:05:39, mmenke wrote:
> What does ProcessOnePendingMessage actually do?  It's clearly not needed for
> non-redirect requests, reading response bodies, or errors, though the
> documentation doesn't seem to say anything about this.

Yeah, URLRequestTestJob's documentation is terrible. Among other things,
test_url_1 behaves differently from all the other test URLs. It's to release the
response body. Except for test_url_1 which release the response body
immediately.

This one was also in the wrong place, so I fixed it.

[mmenke, 2014-10-29 19:44:47]

LGTM

https://codereview.chromium.org/648813002/diff/540001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_delegate.h (right):

https://codereview.chromium.org/648813002/diff/540001/content/browser/loader/...
content/browser/loader/navigation_url_loader_delegate.h:33:
scoped_ptr<StreamHandle> body) = 0;
On 2014/10/29 19:10:33, David Benjamin wrote:
> On 2014/10/28 16:05:39, mmenke wrote:
> > nit:  Maybe body_stream?  Or body_blob?  Or bloby_body_stream or something.
> 
> *shrug* I feel like body is pretty descriptive. StreamHandle just happens to
be
> the mechanism we currently use to convey the body, but it won't be that
forever.
> Should it then be renamed to body_data_pipe or whatever? frame_host is already
> calling it plain |body| and renaming it from |body_stream| to |body| partway
> through seems confusing. (And part of the point of landing the frame_host
> changes earlier was so this CL doesn't keep growing...)

I dunno...  I see a test mucking with something called "body", and I think "Oh,
that's a string", not "Oh, now that we have the body in an in-memory object, we
have to issue a URLRequest to it in order to read from it while spinning the
message loop before we can make sure everything's peachy".

https://codereview.chromium.org/648813002/diff/540001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_unittest.cc (right):

https://codereview.chromium.org/648813002/diff/540001/content/browser/loader/...
content/browser/loader/navigation_url_loader_unittest.cc:129: ResourceContext*
resource_context) override {
On 2014/10/29 19:10:34, David Benjamin wrote:
> On 2014/10/28 16:05:39, mmenke wrote:
> > NOTREACHED()?
> 
> That's supposed to be reached. It's part of a test checking that this obeys
> ShouldBeginRequest.

Ah, right...was forgetting these requests still go through most of the
dispatcher's logic.

[davidben, 2014-10-29 19:46:54]

https://codereview.chromium.org/648813002/diff/540001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_delegate.h (right):

https://codereview.chromium.org/648813002/diff/540001/content/browser/loader/...
content/browser/loader/navigation_url_loader_delegate.h:33:
scoped_ptr<StreamHandle> body) = 0;
On 2014/10/29 19:44:46, mmenke wrote:
> On 2014/10/29 19:10:33, David Benjamin wrote:
> > On 2014/10/28 16:05:39, mmenke wrote:
> > > nit:  Maybe body_stream?  Or body_blob?  Or bloby_body_stream or
something.
> > 
> > *shrug* I feel like body is pretty descriptive. StreamHandle just happens to
> be
> > the mechanism we currently use to convey the body, but it won't be that
> forever.
> > Should it then be renamed to body_data_pipe or whatever? frame_host is
already
> > calling it plain |body| and renaming it from |body_stream| to |body| partway
> > through seems confusing. (And part of the point of landing the frame_host
> > changes earlier was so this CL doesn't keep growing...)
> 
> I dunno...  I see a test mucking with something called "body", and I think
"Oh,
> that's a string", not "Oh, now that we have the body in an in-memory object,
we
> have to issue a URLRequest to it in order to read from it while spinning the
> message loop before we can make sure everything's peachy".

Yeah, that a StreamHandle requires a URLRequest to cash in on it is kind of
unfortunate and why we'll be changing this to something better later. This is
just to let PlzNavigate progress without rewriting all of Blink's navigation
code. :-)

[davidben, 2014-10-29 20:22:18]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-10-29 20:22:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/648813002/580001

[commit-bot: I haz the power, 2014-10-29 21:14:02]

Committed patchset #11 (id:580001)

[commit-bot: I haz the power, 2014-10-29 21:14:41]

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