Initial PlzNavigate RDH-side logic.

Initial PlzNavigate RDH-side logic.

[Closed in favor of split up versions. See comment #14]

Adds a NavigationURLLoader class to mediate between UI and IO
thread. It owns the request up until the response starts, at
which point ownership is transferred 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.

StreamHandle is split up between a StreamInfo struct and the
handle itself so that consumers may retain ownership of the
stream and release the response information once sent to a
child process.

BUG=376015

[davidben, 2014-09-10 18:38:16]

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

[davidben, 2014-09-10 18:38:18]

This touches quite a lot of things. I'll add OWNERS coverage once you four are
happy with it.

nasko and clamy: overall review
mmenke: c/b/loader changes and API
zork: c/b/e/api/streams_private and c/b/streams changes.

(The streams portion can be split out if needbe.)

[mmenke, 2014-09-11 12:52:55]

On 2014/09/10 18:38:18, David Benjamin wrote:
> This touches quite a lot of things. I'll add OWNERS coverage once you four are
> happy with it.
> 
> nasko and clamy: overall review
> mmenke: c/b/loader changes and API
> zork: c/b/e/api/streams_private and c/b/streams changes.
> 
> (The streams portion can be split out if needbe.)

I haven't looked at the code yet, but this is currently only for
browser-initiated navigations?  (In particular, I notice that
CrossSiteResourceHandler is untouched)

[davidben, 2014-09-11 14:14:23]

On 2014/09/11 12:52:55, mmenke wrote:
> On 2014/09/10 18:38:18, David Benjamin wrote:
> > This touches quite a lot of things. I'll add OWNERS coverage once you four
are
> > happy with it.
> > 
> > nasko and clamy: overall review
> > mmenke: c/b/loader changes and API
> > zork: c/b/e/api/streams_private and c/b/streams changes.
> > 
> > (The streams portion can be split out if needbe.)
> 
> I haven't looked at the code yet, but this is currently only for
> browser-initiated navigations?  (In particular, I notice that
> CrossSiteResourceHandler is untouched)

Yeah, this is for the new navigation flow where the browser owns the navigation
request. (So not really either of browser-initiated or renderer-initiated in the
current flow, but we only handle the browser-initiated ones right now.) Design
doc is here:

https://docs.google.com/a/chromium.org/document/d/1cSW8fpJIUnibQKU8TMwLE5VxYZ...

[clamy, 2014-09-12 20:51:26]

Thanks! After a first pass I have a few question/comments.

https://chromiumcodereview.appspot.com/519533002/diff/200001/chrome/browser/r...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://chromiumcodereview.appspot.com/519533002/diff/200001/chrome/browser/r...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:622:
base::Bind(&SendExecuteMimeTypeHandlerEvent, base::Passed(&stream),
StreamInfo now contains a scoped_refptr of a ResourceResponse. Are we sure that
this ResourceResponse will not be accessed on the IO thread after the call to
ResourceDispatcherHostDelegate::OnStreamCreated?

https://chromiumcodereview.appspot.com/519533002/diff/200001/content/browser/...
File content/browser/frame_host/navigator.h (right):

https://chromiumcodereview.appspot.com/519533002/diff/200001/content/browser/...
content/browser/frame_host/navigator.h:130: scoped_ptr<StreamHandle> body);
Actually, I don't think you need to pass the StreamHandle to the navigator. Its
lifetime will most likely be handled by the RenderFrameHostManager.

https://chromiumcodereview.appspot.com/519533002/diff/200001/content/browser/...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/519533002/diff/200001/content/browser/...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1891:
EXPECT_EQ(cancel_count + 1, TestNavigationURLLoader::cancel_count());
We do not test anymore that a call to OnResponseStarted on the old
NavigationURLLoader will not lead to a commit in RenderFrameHostManager. This is
mainly what we were trying to test here (to avoid the Cancelation-Commit race
between the UI and the IO thread).

https://chromiumcodereview.appspot.com/519533002/diff/200001/content/browser/...
File content/browser/loader/navigation_resource_handler.cc (right):

https://chromiumcodereview.appspot.com/519533002/diff/200001/content/browser/...
content/browser/loader/navigation_resource_handler.cc:45:
controller()->Cancel();
So if we cancel the request on the UI thread, this should end up calling the
destructor of the NavigationResourceHandler, with core_ that is not set to null,
right? Which means that we then call RequestFailedOnIOThread, which then post a
task to the UI thread, just to find that the delegate is null. I am wondering if
we should introduce a function
NavigationResourceHandler::NavigationCanceledOnUIThread, that would do the
following:
core_->set_resource_handler(NULL);
core_ = NULL;
controller()->Cancel();

https://chromiumcodereview.appspot.com/519533002/diff/200001/content/browser/...
content/browser/loader/navigation_resource_handler.cc:110: static const int
kReadBufSize = 32768;
This seem to be a copy paste of StreamResourceHandler::OnWillRead. Do we want to
unify it if we can? Same question for the methods OnReadCompleted and
OnResponseCompleted.

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

https://chromiumcodereview.appspot.com/519533002/diff/200001/content/browser/...
content/browser/loader/navigation_resource_handler.h:68: #endif  //
CONTENT_BROWSER_LOADER_CERTIFICATE_RESOURCE_HANDLER_H_
s/CERTIFICATE/NAVIGATION

https://chromiumcodereview.appspot.com/519533002/diff/200001/content/browser/...
File content/browser/loader/navigation_url_loader_core.cc (right):

https://chromiumcodereview.appspot.com/519533002/diff/200001/content/browser/...
content/browser/loader/navigation_url_loader_core.cc:76:
BrowserThread::PostTask(
Are we sure that after this point ResourceResponse will not be accessed on the
IO thread anymore?

https://chromiumcodereview.appspot.com/519533002/diff/200001/content/browser/...
content/browser/loader/navigation_url_loader_core.cc:94:
DCHECK(!resource_handler_);
When is resource_handler_ set to null? In fact, I think a comment should clarify
a bit the lifetime, destruction order and where the pointer to the various
components are set to null in the main cases (request proceeds normally, request
cancelled on the UI thread, request cancelled on the IO thread), as it is not
immediately explicit.

https://chromiumcodereview.appspot.com/519533002/diff/200001/content/browser/...
content/browser/loader/navigation_url_loader_core.cc:108:
RequestFailedOnIOThread(net::ERR_ABORTED);
Shouldn't we return here?

https://chromiumcodereview.appspot.com/519533002/diff/200001/content/browser/...
File content/browser/loader/navigation_url_loader_core.h (right):

https://chromiumcodereview.appspot.com/519533002/diff/200001/content/browser/...
content/browser/loader/navigation_url_loader_core.h:31: class
NavigationURLLoaderCore
Why do we have a NavigationURLLoaderImpl and a NavigationURLLoaderCore? Would it
be possible to just have one?

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

https://chromiumcodereview.appspot.com/519533002/diff/200001/content/browser/...
content/browser/loader/navigation_url_loader_unittest.cc:160:
FrameHostMsg_BeginNavigation_Params params;
TestRenderFrameHost::SendBeginNavigationWithURL has code that initializes the
FrameHostMsg_BeginNavigation_Params to default value. Maybe it could be reused?

https://chromiumcodereview.appspot.com/519533002/diff/200001/content/browser/...
content/browser/loader/navigation_url_loader_unittest.cc:248:
TEST_F(NavigationURLLoaderTest, CancelOnDestruct) {
Should we also add a test for the case where the loader is destroyed after the
response has been received (the request should not be canceled)?

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

https://chromiumcodereview.appspot.com/519533002/diff/200001/content/browser/...
content/browser/loader/resource_dispatcher_host_impl.cc:1677:
NavigationURLLoaderCore* loader) {
I would add a CHECK that browser side navigation is enabled here.

https://chromiumcodereview.appspot.com/519533002/diff/200001/content/browser/...
File content/browser/loader/upload_data_stream_builder.cc (right):

https://chromiumcodereview.appspot.com/519533002/diff/200001/content/browser/...
content/browser/loader/upload_data_stream_builder.cc:118: if
(file_system_context) {
Is this change there because we may not have supplied a FileSystemContext when
sending a navigation request from the UI thread?

https://chromiumcodereview.appspot.com/519533002/diff/200001/content/browser/...
File content/browser/streams/stream_handle_impl.h (right):

https://chromiumcodereview.appspot.com/519533002/diff/200001/content/browser/...
content/browser/streams/stream_handle_impl.h:21: struct ResourceResponse;
nit: shouldn't the struct be declared after the class?

[Zachary Kuznia, 2014-09-12 21:14:20]

https://codereview.chromium.org/519533002/diff/200001/content/public/browser/...
File content/public/browser/stream_info.cc (right):

https://codereview.chromium.org/519533002/diff/200001/content/public/browser/...
content/public/browser/stream_info.cc:5: #include
"content/public/browser/stream_info.h"
Try to dial down the copy detection on git cl upload, so that it knows this is a
new file.

https://codereview.chromium.org/519533002/diff/200001/content/public/browser/...
File content/public/browser/stream_info.h (right):

https://codereview.chromium.org/519533002/diff/200001/content/public/browser/...
content/public/browser/stream_info.h:34: };
DISALLOW_COPY_AND_ASSIGN(StreamInfo)

[davidben, 2014-09-19 18:30:50]

Lots of changes, largely around streams and threading.

StreamInfo is back to looking like the old StreamHandle in containing just
headers and mime type. There's a threading issue
(https://codereview.chromium.org/588643002/), so it needs to make a copy and I
didn't want to make it copy stuff it doesn't need. (PlzNavigate doesn't look
like it really needs the StreamInfo struct and can just have a couple
parameters, but it also needs to copy) https://crbug.com/416050 should avoid the
copy in both cases, but that requires getting rid of the BRH dispatch.

I've also extracted the common portions of StreamResourceHandle and
NavigationResourceHandler into a common StreamWriter.

I've also moved the logic out of NavigationURLLoaderCore around detatching so
it's all in either the Impl or the ResourceHandler now. The core now only does
thread-shuffling, so hopefully that's a little clearer.

https://codereview.chromium.org/519533002/diff/200001/chrome/browser/renderer...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/519533002/diff/200001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:622:
base::Bind(&SendExecuteMimeTypeHandlerEvent, base::Passed(&stream),
On 2014/09/12 20:51:25, clamy wrote:
> StreamInfo now contains a scoped_refptr of a ResourceResponse. Are we sure
that
> this ResourceResponse will not be accessed on the IO thread after the call to
> ResourceDispatcherHostDelegate::OnStreamCreated?

Apparently the answer is no, it's not thread-safe. In fact, the old code wasn't
either. :-) I've uploaded https://codereview.chromium.org/588643002/ to have it
make a copy of the headers and have reverted StreamInfo to store just what
streams needs and not the full ResourceResponse. We're not using StreamInfo in
PlzNavigate, so it's fine.

https://codereview.chromium.org/519533002/diff/200001/content/browser/frame_h...
File content/browser/frame_host/navigator.h (right):

https://codereview.chromium.org/519533002/diff/200001/content/browser/frame_h...
content/browser/frame_host/navigator.h:130: scoped_ptr<StreamHandle> body);
On 2014/09/12 20:51:25, clamy wrote:
> Actually, I don't think you need to pass the StreamHandle to the navigator.
Its
> lifetime will most likely be handled by the RenderFrameHostManager.

Per out-of-band conversation, leaving as-is so ownership can be managed in
RenderFrameHost on behalf of the renderer-side RenderFrame.

https://codereview.chromium.org/519533002/diff/200001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/519533002/diff/200001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1891:
EXPECT_EQ(cancel_count + 1, TestNavigationURLLoader::cancel_count());
On 2014/09/12 20:51:25, clamy wrote:
> We do not test anymore that a call to OnResponseStarted on the old
> NavigationURLLoader will not lead to a commit in RenderFrameHostManager. This
is
> mainly what we were trying to test here (to avoid the Cancelation-Commit race
> between the UI and the IO thread).

That's true. It got split over two tests. This tests that we cancel the loader
on the UI thread. The loader tests assert that loader cancel stops calls on the
delegate. I've added a test to test the race that may happen.

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

https://codereview.chromium.org/519533002/diff/200001/content/browser/loader/...
content/browser/loader/navigation_resource_handler.cc:45:
controller()->Cancel();
On 2014/09/12 20:51:25, clamy wrote:
> So if we cancel the request on the UI thread, this should end up calling the
> destructor of the NavigationResourceHandler, with core_ that is not set to
null,
> right? Which means that we then call RequestFailedOnIOThread, which then post
a
> task to the UI thread, just to find that the delegate is null. I am wondering
if
> we should introduce a function
> NavigationResourceHandler::NavigationCanceledOnUIThread, that would do the
> following:
> core_->set_resource_handler(NULL);
> core_ = NULL;
> controller()->Cancel();

Done. It'll actually call onResponseCompleted first, so this does require an
extra NULL check on OnResponseCompleted because controller()->Cancel() is
asynchronous. (Which is really awkward and probably worth changing.)

https://codereview.chromium.org/519533002/diff/200001/content/browser/loader/...
content/browser/loader/navigation_resource_handler.cc:110: static const int
kReadBufSize = 32768;
On 2014/09/12 20:51:25, clamy wrote:
> This seem to be a copy paste of StreamResourceHandler::OnWillRead. Do we want
to
> unify it if we can? Same question for the methods OnReadCompleted and
> OnResponseCompleted.

Using StreamResourceHandler directly is a little awkward because OnWillRead and
OnResponseStarted sometimes happen in the wrong order, but I've split it off
into a common StreamWriter that both embed.

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

https://codereview.chromium.org/519533002/diff/200001/content/browser/loader/...
content/browser/loader/navigation_resource_handler.h:68: #endif  //
CONTENT_BROWSER_LOADER_CERTIFICATE_RESOURCE_HANDLER_H_
On 2014/09/12 20:51:25, clamy wrote:
> s/CERTIFICATE/NAVIGATION

Done.

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

https://codereview.chromium.org/519533002/diff/200001/content/browser/loader/...
content/browser/loader/navigation_url_loader_core.cc:76:
BrowserThread::PostTask(
On 2014/09/12 20:51:25, clamy wrote:
> Are we sure that after this point ResourceResponse will not be accessed on the
> IO thread anymore?

I believe it is safe. I tried to ensure this by making ResourceResponse always
passed as a scoped_ptr, but the problem is that BufferedResourceHandler's
dispatch logic is incompatible with this.

I'd like to move the dispatch part of BRH up to NavigationRequest, which would
allow us to scoped_ptr it. But that's further off, so I've made a copy and left
a TODO that we can optimize out the copy once we un-refcount it.

https://codereview.chromium.org/519533002/diff/200001/content/browser/loader/...
content/browser/loader/navigation_url_loader_core.cc:94:
DCHECK(!resource_handler_);
On 2014/09/12 20:51:25, clamy wrote:
> When is resource_handler_ set to null? In fact, I think a comment should
clarify
> a bit the lifetime, destruction order and where the pointer to the various
> components are set to null in the main cases (request proceeds normally,
request
> cancelled on the UI thread, request cancelled on the IO thread), as it is not
> immediately explicit.

I've switched all the NULL setting to explicit calls which should hopefully be
clearer. Also moved the BeginNavigationRequest return code out. Core no longer
has any interesting logic outside of thread shuffling which hopefully is
clearer. Also added a comment on why these should be NULL at this point.

https://codereview.chromium.org/519533002/diff/200001/content/browser/loader/...
content/browser/loader/navigation_url_loader_core.cc:108:
RequestFailedOnIOThread(net::ERR_ABORTED);
On 2014/09/12 20:51:25, clamy wrote:
> Shouldn't we return here?

Oof. Done. Also added a test that exercises this path.

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

https://codereview.chromium.org/519533002/diff/200001/content/browser/loader/...
content/browser/loader/navigation_url_loader_core.h:31: class
NavigationURLLoaderCore
On 2014/09/12 20:51:25, clamy wrote:
> Why do we have a NavigationURLLoaderImpl and a NavigationURLLoaderCore? Would
it
> be possible to just have one?

NavigationURLLoaderImpl is uniquely owned by the UI thread. The Core is some
annoying ref-counted thing shared between the two threads to deal with
synchronization. (Mostly this mirrors URLFetcher{,Impl,Core,Factory})

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

https://codereview.chromium.org/519533002/diff/200001/content/browser/loader/...
content/browser/loader/navigation_url_loader_unittest.cc:160:
FrameHostMsg_BeginNavigation_Params params;
On 2014/09/12 20:51:25, clamy wrote:
> TestRenderFrameHost::SendBeginNavigationWithURL has code that initializes the
> FrameHostMsg_BeginNavigation_Params to default value. Maybe it could be
reused?

Hrm. Maybe, although layering-wise loader/ shouldn't really depend on
frame_host/. Eventually, I think NavigationURLLoader should never even see half
of the fields SendBeginNavigationWithURL sets up:

method through request_body are needed. The remaining are currently used in
ResourceResponseInfo, but some of that isn't needed with
CrossSiteResourceHandler gone. Some of it's needed because navigation
interception happens in the resource stack. When we get that out of there, I
think allow_download will be all that's left. But I also want to later move
BufferedResourceHandler's download/cert/stream dispatch into the navigation
layer too (to avoid the thing where we lie to the old consumer and tell it some
request canceled), so that too can eventually go.

https://codereview.chromium.org/519533002/diff/200001/content/browser/loader/...
content/browser/loader/navigation_url_loader_unittest.cc:248:
TEST_F(NavigationURLLoaderTest, CancelOnDestruct) {
On 2014/09/12 20:51:25, clamy wrote:
> Should we also add a test for the case where the loader is destroyed after the
> response has been received (the request should not be canceled)?

Added a couple of tests in this vein.

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

https://codereview.chromium.org/519533002/diff/200001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1677:
NavigationURLLoaderCore* loader) {
On 2014/09/12 20:51:25, clamy wrote:
> I would add a CHECK that browser side navigation is enabled here.

Good point. Done.

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

https://codereview.chromium.org/519533002/diff/200001/content/browser/loader/...
content/browser/loader/upload_data_stream_builder.cc:118: if
(file_system_context) {
On 2014/09/12 20:51:26, clamy wrote:
> Is this change there because we may not have supplied a FileSystemContext when
> sending a navigation request from the UI thread?

Yeah. For now we don't supply one at all. This is the comment in RDH:

    // TODO(davidben): The FileSystemContext is NULL here. In the case where
    // another renderer requested this navigation, this should be the same
    // FileSystemContext passed into ShouldServiceRequest.

But I think we may continue to have cases where we don't supply one at all. This
FileSystemContext should be that of the renderer which /initiated/ the
navigation because any filesystem URLs (this isn't files on the user's
filesystem, but files in the HTML5 filesystem API) should be resolved relative
to the that renderer's FileSystemContext. If the browser's initiating a
navigation, I doubt it'd make much sense to require it make up a
FileSystemContext each time since the browser's probably never going to start
doing POSTs with a file upload of a filesystem URL. And if we're supporting
NULL, I figured it would be good to check this rather than pass NULLs around.

(On reflection, we want to find out if the browser ever tries to do this.
Switched it to a CHECK.)

https://codereview.chromium.org/519533002/diff/200001/content/browser/streams...
File content/browser/streams/stream_handle_impl.h (right):

https://codereview.chromium.org/519533002/diff/200001/content/browser/streams...
content/browser/streams/stream_handle_impl.h:21: struct ResourceResponse;
On 2014/09/12 20:51:26, clamy wrote:
> nit: shouldn't the struct be declared after the class?

Done.

https://codereview.chromium.org/519533002/diff/200001/content/public/browser/...
File content/public/browser/stream_info.cc (right):

https://codereview.chromium.org/519533002/diff/200001/content/public/browser/...
content/public/browser/stream_info.cc:5: #include
"content/public/browser/stream_info.h"
On 2014/09/12 21:14:19, Zachary Kuznia wrote:
> Try to dial down the copy detection on git cl upload, so that it knows this is
a
> new file.

Done. (Although it's not going to make a difference once it's actually
committed.)

https://codereview.chromium.org/519533002/diff/200001/content/public/browser/...
File content/public/browser/stream_info.h (right):

https://codereview.chromium.org/519533002/diff/200001/content/public/browser/...
content/public/browser/stream_info.h:34: };
On 2014/09/12 21:14:19, Zachary Kuznia wrote:
> DISALLOW_COPY_AND_ASSIGN(StreamInfo)

Done.

[Zachary Kuznia, 2014-09-19 19:50:54]

Streams LGTM

[clamy, 2014-09-22 21:11:01]

Thanks! LGTM

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

https://chromiumcodereview.appspot.com/519533002/diff/200001/content/browser/...
content/browser/loader/navigation_url_loader_unittest.cc:160:
FrameHostMsg_BeginNavigation_Params params;
On 2014/09/19 18:30:50, David Benjamin wrote:
> On 2014/09/12 20:51:25, clamy wrote:
> > TestRenderFrameHost::SendBeginNavigationWithURL has code that initializes
the
> > FrameHostMsg_BeginNavigation_Params to default value. Maybe it could be
> reused?
> 
> Hrm. Maybe, although layering-wise loader/ shouldn't really depend on
> frame_host/. Eventually, I think NavigationURLLoader should never even see
half
> of the fields SendBeginNavigationWithURL sets up:
> 
> method through request_body are needed. The remaining are currently used in
> ResourceResponseInfo, but some of that isn't needed with
> CrossSiteResourceHandler gone. Some of it's needed because navigation
> interception happens in the resource stack. When we get that out of there, I
> think allow_download will be all that's left. But I also want to later move
> BufferedResourceHandler's download/cert/stream dispatch into the navigation
> layer too (to avoid the thing where we lie to the old consumer and tell it
some
> request canceled), so that too can eventually go.

Acknowledged.

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

https://chromiumcodereview.appspot.com/519533002/diff/240001/content/browser/...
content/browser/loader/resource_dispatcher_host_impl.cc:1851:
BeginRequestInternal(new_request.Pass(), handler.Pass());
nit: Do you think that the check that was removed here should be replaced by a
DCHECK(handler.get())?

[davidben, 2014-09-22 21:19:56]

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

https://chromiumcodereview.appspot.com/519533002/diff/240001/content/browser/...
content/browser/loader/resource_dispatcher_host_impl.cc:1851:
BeginRequestInternal(new_request.Pass(), handler.Pass());
On 2014/09/22 21:11:01, clamy wrote:
> nit: Do you think that the check that was removed here should be replaced by a
> DCHECK(handler.get())?

Eh, probably don't need to bother; new ThrottlingResourceHandler can't be NULL.
It came from BeginRequest which wants to check because CreateResourceHandler
might return NULL.

[mmenke, 2014-09-23 17:22:46]

I'm still very slowly making my way through this.  I hope to finally manage a
full pass this week.

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

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
content/browser/loader/navigation_url_loader.h:51: virtual void
OnRequestFailed(int net_error) = 0;
 private:
  DISALLOW_COPY_AND_ASSIGN?

Suppose it's not really needed for abstract classes, but seems simplest to just
always include it.

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
content/browser/loader/navigation_url_loader.h:75: virtual void FollowRedirect()
= 0;
 private:
  DISALLOW_COPY_AND_ASSIGN?

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

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
content/browser/loader/navigation_url_loader_core.cc:34: BrowserThread::IO,
FROM_HERE,
Should include "base/location.h" for FROM_HERE

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
content/browser/loader/navigation_url_loader_core.cc:79: response_copy->head =
response->head;
Copying ResourceResponse is allowed, right?  So we can just use response_copy =
new ResourceResponse(*response);

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

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
content/browser/loader/navigation_url_loader_core.h:31: // thread. It bridges
cross-thread between the two.
nit:  "It bridges cross-thread between the two."  I believe bridges is strictly
a transitive verb, cross-thread is an adverb/adjective.  You're missing a noun /
direct object.

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
content/browser/loader/navigation_url_loader_core.h:80: int64
frame_tree_node_id,
Should include basictypes.h, I believe, for int64

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_factory.h (right):

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
content/browser/loader/navigation_url_loader_factory.h:24: virtual
~NavigationURLLoaderFactory() {}
DISALLOW_COPY_AND_ASSIGN (Yes, not technically needed for abstract classes, but
think it's safer to blindly apply it everywhere than to worry about selectively
applying it, since abstract classes don't always remain that way.

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

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
content/browser/loader/navigation_url_loader_impl.h:29:
scoped_refptr<NavigationURLLoaderCore> core_;
Need ref_counted.h

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
content/browser/loader/navigation_url_loader_impl.h:31:
DISALLOW_COPY_AND_ASSIGN(NavigationURLLoaderImpl);
Need macros.h

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
File content/browser/loader/stream_writer.cc (right):

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
content/browser/loader/stream_writer.cc:32: base::GenerateGUID());
Erm...Wait...So after navigation occurs, we redirect the request to a blob URL? 
I guess we then pass that to the correct renderer process, and have it read from
that, instead of taking ownership of the original loader?

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
content/browser/loader/stream_writer.cc:39: static const int kReadBufSize =
32768;
Can static const int result in something first being initialized on the first
call to a function or not?  I know this has been discussed, just can never
remember what the conclusion was.

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
content/browser/loader/stream_writer.cc:41: DCHECK(buf && buf_size);
nit:  Use two DCHECKs.  The one for buf_size should probably be
DCHECK_GT(buf_size, 0)

[nasko, 2014-09-24 21:15:34]

Initial pass through the CL and a bunch of comments to ensure I understand it
correctly.

https://codereview.chromium.org/519533002/diff/240001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.h (right):

https://codereview.chromium.org/519533002/diff/240001/content/browser/frame_h...
content/browser/frame_host/navigation_request.h:56: FrameTreeNode*
frame_tree_node_;
Do we guarantee that FTN will outlive this object?

https://codereview.chromium.org/519533002/diff/240001/content/browser/frame_h...
File content/browser/frame_host/navigator.cc (right):

https://codereview.chromium.org/519533002/diff/240001/content/browser/frame_h...
content/browser/frame_host/navigator.cc:29: scoped_ptr<StreamHandle> body) {
Why move the empty method here?

https://codereview.chromium.org/519533002/diff/240001/content/browser/frame_h...
File content/browser/frame_host/navigator.h (right):

https://codereview.chromium.org/519533002/diff/240001/content/browser/frame_h...
content/browser/frame_host/navigator.h:129: ResourceResponse* response,
nit: response to me indicates the full response, where it is just the headers.
Not sure if response_headers is better though.

https://codereview.chromium.org/519533002/diff/240001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/519533002/diff/240001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:589:
delegate_->GetControllerForRenderManager();
nit: In general with out-of-process iframes we need to get the controller
through the Navigator object, but other places in this file do it this way.
Let's stick with this for consistency and I can replace that in the future
easily.

https://codereview.chromium.org/519533002/diff/240001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/519533002/diff/240001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1828: // Now
commit the same url.
It is now non-obvious that the following code will commit. I'd add something to
the comment to indicate that CallOnResponseStarted will indeed result in the
commit.

https://codereview.chromium.org/519533002/diff/240001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1907:
EXPECT_EQ(kUrl2_site, main_test_rfh()->GetSiteInstance()->GetSiteURL());
Why not check the cancel count here again? Just to be paranoid ; )

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

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
content/browser/loader/navigation_url_loader.h:37: // Called when the request is
redirected. Call FollowRedirect to continue
What happens if FollowRedirect is not called? What is the Delegate supposed to
do if it wants to cancel the request?

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
content/browser/loader/navigation_url_loader.h:39: // they must not be modified
on the UI thread.
nit: What is "they"?

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
content/browser/loader/navigation_url_loader.h:48: 
Do we need to notify the delegate if the request resulted in a download? Or is
it magically handled behind the scenes somehow?

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

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
content/browser/loader/navigation_url_loader_core.cc:96:
NavigationURLLoaderCore::~NavigationURLLoaderCore() {
Which thread should this object be destroyed on? Can we DCHECK it?

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
content/browser/loader/navigation_url_loader_core.cc:97: // The core may outlive
either the delegate or the resource handler, so each
"may" or "should"? If it is "may", then there is a case where it doesn't, which
will make the DCHECKs below fire, no?

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

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
content/browser/loader/navigation_url_loader_core.h:51: // Called on the UI
thread to cancel a request, if it has not completed. Note:
nit: Note: on new line

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
content/browser/loader/navigation_url_loader_core.h:52: // if the response has
been received, the request is considered completed from
The full response or the response headers? Let's be explicit to avoid confusion.

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

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
content/browser/loader/navigation_url_loader_impl.cc:11:
NavigationURLLoaderImpl::NavigationURLLoaderImpl(
Why do we need the separation of Impl and Core, when the Impl doesn't do much?

https://codereview.chromium.org/519533002/diff/240001/content/public/browser/...
File content/public/browser/stream_info.h (right):

https://codereview.chromium.org/519533002/diff/240001/content/public/browser/...
content/public/browser/stream_info.h:33: // The original redirected to this
Stream.
nit: original what?

https://codereview.chromium.org/519533002/diff/240001/content/public/browser/...
content/public/browser/stream_info.h:39: // The HTTP response headers associated
with this Stream.
nit: lowercase Stream if the concept or StreamHandle if you mean associated with
the handle.

[davidben, 2014-10-03 16:27:53]

Okay, several monster rebases later, that's through.

In the interest of paring down this CL, I'm going to split the streams bit which
zork@ has already reviewed out into a separate CL. And then we'll see what else
can be pulled out. Probably ResourceResponse::DeepCopy can be its own CL. Also
the bits where the signature of CommitNavigation changes can also be split out.

I did have to tweak the navigator tests slightly so that NavigationRequestInfo
is stored on the test loader. clamy@ says it may as well be copied and not a
scoped_ptr in that case. In the interest of not further blowing up the diff
until splitting things out, I've left it as-is for now. :-)

https://codereview.chromium.org/519533002/diff/240001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.h (right):

https://codereview.chromium.org/519533002/diff/240001/content/browser/frame_h...
content/browser/frame_host/navigation_request.h:56: FrameTreeNode*
frame_tree_node_;
On 2014/09/24 21:15:34, nasko wrote:
> Do we guarantee that FTN will outlive this object?

We should. FTN owns RFHM which owns this object. (If the FrameTreeNode is
destroyed for whatever reason, the navigation should get canceled.)

https://codereview.chromium.org/519533002/diff/240001/content/browser/frame_h...
File content/browser/frame_host/navigator.cc (right):

https://codereview.chromium.org/519533002/diff/240001/content/browser/frame_h...
content/browser/frame_host/navigator.cc:29: scoped_ptr<StreamHandle> body) {
On 2014/09/24 21:15:34, nasko wrote:
> Why move the empty method here?

It has a scoped_ptr<StreamHandle> which means it needs access to ~StreamHandle.
I.e. that's not actually quite empty; it calls a destructor. Whee C++.

https://codereview.chromium.org/519533002/diff/240001/content/browser/frame_h...
File content/browser/frame_host/navigator.h (right):

https://codereview.chromium.org/519533002/diff/240001/content/browser/frame_h...
content/browser/frame_host/navigator.h:129: ResourceResponse* response,
On 2014/09/24 21:15:34, nasko wrote:
> nit: response to me indicates the full response, where it is just the headers.
> Not sure if response_headers is better though.

Hrm. So, if it changes here, it should also change everywhere else this object
is passed around, all the way back to the resource loader stack. Which I'd
rather not do in this CL.

(It also has a bit more metadata than just the headers.)

https://codereview.chromium.org/519533002/diff/240001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/519533002/diff/240001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:589:
delegate_->GetControllerForRenderManager();
On 2014/09/24 21:15:34, nasko wrote:
> nit: In general with out-of-process iframes we need to get the controller
> through the Navigator object, but other places in this file do it this way.
> Let's stick with this for consistency and I can replace that in the future
> easily.

Acknowledged.

https://codereview.chromium.org/519533002/diff/240001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/519533002/diff/240001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1828: // Now
commit the same url.
On 2014/09/24 21:15:34, nasko wrote:
> It is now non-obvious that the following code will commit. I'd add something
to
> the comment to indicate that CallOnResponseStarted will indeed result in the
> commit.

Done.

https://codereview.chromium.org/519533002/diff/240001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1907:
EXPECT_EQ(kUrl2_site, main_test_rfh()->GetSiteInstance()->GetSiteURL());
On 2014/09/24 21:15:34, nasko wrote:
> Why not check the cancel count here again? Just to be paranoid ; )

Done.

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

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
content/browser/loader/navigation_url_loader.h:37: // Called when the request is
redirected. Call FollowRedirect to continue
On 2014/09/24 21:15:34, nasko wrote:
> What happens if FollowRedirect is not called? What is the Delegate supposed to
> do if it wants to cancel the request?

If FollowRedirect isn't called, the request just sits there paused. To cancel
the request, destroy the loader. (Can also add an explicit Cancel method if
that's preferable. Seemed we're going to be holding on to a scoped_ptr anyway,
so we can just trash the loader.)

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
content/browser/loader/navigation_url_loader.h:39: // they must not be modified
on the UI thread.
On 2014/09/24 21:15:34, nasko wrote:
> nit: What is "they"?

Removed the comment. It makes a copy of the ResourceResponse now (which we'll be
able to elide with https://crbug.com/416050).

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
content/browser/loader/navigation_url_loader.h:48: 
On 2014/09/24 21:15:34, nasko wrote:
> Do we need to notify the delegate if the request resulted in a download? Or is
> it magically handled behind the scenes somehow?

If it results in a download, we'll get OnRequestFailed(ERR_ABORTED), so we'll
need to treat that error code special as we do now. (Don't show an error page.)

After all this, I'd actually like to move that out of the loader and into the
navigation logic. So something like...

func OnResponseStarted(response, body) {
    if ShouldDownload(response) {
       DoDownloadThing(response, body)
    } else if ShouldInterceptAsStream(response) {
       DoStreamThing(response, body)
    } else if response.mime_type == "application/x-x509-user-cert" {
       DoUserCertThing(response, body)
    } else {
       // Web content
       rfh := PickAppropriateRenderer(response)
       rfh.CommitNavigation(response, body)
    }
}

(I.e. all these download-like mechanisms don't intercept a request but they're
just part of the dispatch logic in deciding what to do with the load. Except,
you know, looking very different and not in a suspiciously Go-ish psuedocode.)

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
content/browser/loader/navigation_url_loader.h:51: virtual void
OnRequestFailed(int net_error) = 0;
On 2014/09/23 17:22:45, mmenke wrote:
>  private:
>   DISALLOW_COPY_AND_ASSIGN?
> 
> Suppose it's not really needed for abstract classes, but seems simplest to
just
> always include it.

Done, but it requires adding a default ctor somewhere.

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
content/browser/loader/navigation_url_loader.h:75: virtual void FollowRedirect()
= 0;
On 2014/09/23 17:22:45, mmenke wrote:
>  private:
>   DISALLOW_COPY_AND_ASSIGN?

Done, but it requires adding a default ctor somewhere.

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

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
content/browser/loader/navigation_url_loader_core.cc:34: BrowserThread::IO,
FROM_HERE,
On 2014/09/23 17:22:45, mmenke wrote:
> Should include "base/location.h" for FROM_HERE

Done.

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
content/browser/loader/navigation_url_loader_core.cc:79: response_copy->head =
response->head;
On 2014/09/23 17:22:45, mmenke wrote:
> Copying ResourceResponse is allowed, right?  So we can just use response_copy
=
> new ResourceResponse(*response);

It doesn't actually have a copy constructor, sadly. I added a DeepCopy method
which a sad TODO about ref-counted modifiable structs.

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
content/browser/loader/navigation_url_loader_core.cc:96:
NavigationURLLoaderCore::~NavigationURLLoaderCore() {
On 2014/09/24 21:15:34, nasko wrote:
> Which thread should this object be destroyed on? Can we DCHECK it?

It actually could be either, which is kinda weird. But this object doesn't
actually have state. I think URLFetcherCore works like this too.

The other option I see would be for the Core to be an IO-thread object, not
ref-counted, and just owned by the NavigationURLLoaderImpl from the UI thread.
(So when the UI thread object decides it's time for the Core to die, it
PostTasks a delete.) The Core needs to call back to the UI thread Impl, so it
gets passed a UI-thread WeakPtr when being constructed.

And then Core and Handler have non-owning pointers to each other and make sure
to disconnect appropriately when either side goes away. That part is kind of
silly and mostly a side effect of RDH owning the ResourceLoader rather than the
Core. Matt and I have been talking about refactoring the RDH, but I didn't want
to block PlzNavigate on that. (Hence hiding all the thread stuff behind this
API.)

Anyway, I can switch it to that version if you prefer. This seemed a slightly
simpler mechanism.

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
content/browser/loader/navigation_url_loader_core.cc:97: // The core may outlive
either the delegate or the resource handler, so each
On 2014/09/24 21:15:34, nasko wrote:
> "may" or "should"? If it is "may", then there is a case where it doesn't,
which
> will make the DCHECKs below fire, no?

Rephrased to be slightly less confusing hopefully.

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

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
content/browser/loader/navigation_url_loader_core.h:31: // thread. It bridges
cross-thread between the two.
On 2014/09/23 17:22:45, mmenke wrote:
> nit:  "It bridges cross-thread between the two."  I believe bridges is
strictly
> a transitive verb, cross-thread is an adverb/adjective.  You're missing a noun
/
> direct object.

Done.

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
content/browser/loader/navigation_url_loader_core.h:51: // Called on the UI
thread to cancel a request, if it has not completed. Note:
On 2014/09/24 21:15:34, nasko wrote:
> nit: Note: on new line

Done.

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
content/browser/loader/navigation_url_loader_core.h:52: // if the response has
been received, the request is considered completed from
On 2014/09/24 21:15:34, nasko wrote:
> The full response or the response headers? Let's be explicit to avoid
confusion.

Done.

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
content/browser/loader/navigation_url_loader_core.h:80: int64
frame_tree_node_id,
On 2014/09/23 17:22:45, mmenke wrote:
> Should include basictypes.h, I believe, for int64

Done.

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_factory.h (right):

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
content/browser/loader/navigation_url_loader_factory.h:24: virtual
~NavigationURLLoaderFactory() {}
On 2014/09/23 17:22:45, mmenke wrote:
> DISALLOW_COPY_AND_ASSIGN (Yes, not technically needed for abstract classes,
but
> think it's safer to blindly apply it everywhere than to worry about
selectively
> applying it, since abstract classes don't always remain that way.

Done. Also required a default ctor.

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

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
content/browser/loader/navigation_url_loader_impl.cc:11:
NavigationURLLoaderImpl::NavigationURLLoaderImpl(
On 2014/09/24 21:15:34, nasko wrote:
> Why do we need the separation of Impl and Core, when the Impl doesn't do much?

Impl is uniquely owned and lives on the UI-thread. Core is ref-counted and lives
on both. Yeah, I'm not very happy with how much machinery is involved here. I do
feel the exposed API should be uniquely owned; the fact that there's craziness
across threads should be hidden from the caller. And then there needs to be some
IO-thread object to plug into the loader stack and plumb signals back to the
UI-thread object, so I think we'll need at least two. The Core/ResourceHandler
separation is slightly weird but probably can't easily be resolved without
ResourceLoader ownership getting tidied up.

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

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
content/browser/loader/navigation_url_loader_impl.h:29:
scoped_refptr<NavigationURLLoaderCore> core_;
On 2014/09/23 17:22:45, mmenke wrote:
> Need ref_counted.h

Done.

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
content/browser/loader/navigation_url_loader_impl.h:31:
DISALLOW_COPY_AND_ASSIGN(NavigationURLLoaderImpl);
On 2014/09/23 17:22:45, mmenke wrote:
> Need macros.h

Done.

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
File content/browser/loader/stream_writer.cc (right):

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
content/browser/loader/stream_writer.cc:32: base::GenerateGUID());
On 2014/09/23 17:22:45, mmenke wrote:
> Erm...Wait...So after navigation occurs, we redirect the request to a blob
URL? 
> I guess we then pass that to the correct renderer process, and have it read
from
> that, instead of taking ownership of the original loader?

Yeah. We're going to get the flow working this way first because it minimizes
the churn Blink-side. This way Blink still makes a normal request and most of
Blink-side navigation logic can continue to go through its resource loader.

Once all that works, we'll try to disentangle the two and Blink can be passed
some kind of handle to the body directly. (Shared memory segment, Mojo data
pipe, etc.) But hopefully browser-side it'll still have the same structure: make
request, get response headers + some handle to the body back. Handle to body can
be passed down to a process.

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
content/browser/loader/stream_writer.cc:39: static const int kReadBufSize =
32768;
On 2014/09/23 17:22:46, mmenke wrote:
> Can static const int result in something first being initialized on the first
> call to a function or not?  I know this has been discussed, just can never
> remember what the conclusion was.

I would expect it to be fine. At least, we do do it everywhere. :-) I just
pulled it from StreamResourceHandler. Can switch it too, *shrug*.

https://codereview.chromium.org/519533002/diff/240001/content/browser/loader/...
content/browser/loader/stream_writer.cc:41: DCHECK(buf && buf_size);
On 2014/09/23 17:22:46, mmenke wrote:
> nit:  Use two DCHECKs.  The one for buf_size should probably be
> DCHECK_GT(buf_size, 0)

Done. (Except buf_size is a pointer, so still just a DCHECK.)

https://codereview.chromium.org/519533002/diff/240001/content/public/browser/...
File content/public/browser/stream_info.h (right):

https://codereview.chromium.org/519533002/diff/240001/content/public/browser/...
content/public/browser/stream_info.h:33: // The original redirected to this
Stream.
On 2014/09/24 21:15:34, nasko wrote:
> nit: original what?

Done.

https://codereview.chromium.org/519533002/diff/240001/content/public/browser/...
content/public/browser/stream_info.h:39: // The HTTP response headers associated
with this Stream.
On 2014/09/24 21:15:34, nasko wrote:
> nit: lowercase Stream if the concept or StreamHandle if you mean associated
with
> the handle.

Done.

[davidben, 2014-10-03 16:54:28]

First piece to be split out:
https://codereview.chromium.org/625993002/