Use mojo for Chrome Loading, Part 1 (issue 1970693002 by yhirano@chromium.org)

[yhi...@chromium.org]

Reviewers: jam, yzshen1, kinuko
CL: https://codereview.chromium.org/1970693002/

Description:
Use mojo for Chrome Loading, Part 1

This CL introduces content::mojom::URLLoader for Chrome Loading
with mojo.

Currently the loader is used only in LayoutTests/virtual/mojo-loading
tests, and it lacks a lot of functionalities. I will implement more
features in subsequent CLs.

BUG=603396

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+1341, -57 lines):
M content/DEPS
M content/browser/BUILD.gn
M content/browser/loader/DEPS
M content/browser/loader/async_resource_handler.h
M content/browser/loader/async_resource_handler.cc
M content/browser/loader/resource_dispatcher_host_impl.h
M content/browser/loader/resource_dispatcher_host_impl.cc
M content/browser/loader/resource_loader.h
M content/browser/loader/resource_loader.cc
M content/browser/loader/resource_message_filter.h
M content/browser/loader/resource_message_filter.cc
M content/browser/loader/resource_request_info_impl.h
A content/browser/loader/url_loader_factory_impl.h
A content/browser/loader/url_loader_factory_impl.cc
M content/browser/renderer_host/render_process_host_impl.h
M content/browser/renderer_host/render_process_host_impl.cc
M content/child/BUILD.gn
M content/child/blink_platform_impl.h
M content/child/blink_platform_impl.cc
A content/child/body_consumer.h
A content/child/body_consumer.cc
M content/child/resource_dispatcher.h
M content/child/resource_dispatcher.cc
M content/child/resource_dispatcher_unittest.cc
M content/child/web_url_loader_impl.h
M content/child/web_url_loader_impl.cc
M content/child/web_url_loader_impl_unittest.cc
M content/common/BUILD.gn
A content/common/url_loader.mojom
A + content/common/url_loader_factory.mojom
A content/common/url_loader_type_converters.h
A content/common/url_loader_type_converters.cc
M content/content_browser.gypi
M content/content_child.gypi
M content/content_common.gypi
M content/content_common_mojo_bindings.gyp
M content/renderer/renderer_blink_platform_impl.h
M content/renderer/renderer_blink_platform_impl.cc
M content/test/test_blink_web_unit_test_support.cc
M net/http/http_response_headers.h
M net/http/http_response_headers.cc
M third_party/WebKit/LayoutTests/VirtualTestSuites
A third_party/WebKit/LayoutTests/virtual/mojo-loading/webexposed/README.txt
M third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in
M third_party/WebKit/Source/platform/exported/WebURLRequest.cpp
M third_party/WebKit/public/platform/WebURLRequest.h

[dch...@chromium.org]

https://codereview.chromium.org/1970693002/diff/290001/content/common/url_loader.mojom
File content/common/url_loader.mojom (right):

https://codereview.chromium.org/1970693002/diff/290001/content/common/url_loader.mojom#newcode33
content/common/url_loader.mojom:33: string url;
Use url.mojom.Url?

https://codereview.chromium.org/1970693002/diff/290001/content/common/url_loader_type_converters.cc
File content/common/url_loader_type_converters.cc (right):

https://codereview.chromium.org/1970693002/diff/290001/content/common/url_loader_type_converters.cc#newcode53
content/common/url_loader_type_converters.cc:53: URLRequestPtr
TypeConverter<URLRequestPtr, ResourceHostMsg_Request>::Convert(
Let's avoid adding new type converters and use StructTraits instead.

https://codereview.chromium.org/1970693002/

[j...@chromium.org]

this looks great. just minor comments.

regarding testing: we should have unit tests for this new mojo-based loader. see
https://codereview.chromium.org/1873463003/diff/40001/mojo/services/network/url_loader_impl_unittest.cc
for inspiration from the old deleted network service.


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

https://codereview.chromium.org/1970693002/diff/310001/content/browser/loader/resource_dispatcher_host_impl.cc#newcode1799
content/browser/loader/resource_dispatcher_host_impl.cc:1799:
DCHECK(loader);
nit: here and below, and also in next method, these dchecks are
unnecessary. if the pointers are null, the method calls will crash

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

https://codereview.chromium.org/1970693002/diff/310001/content/browser/loader/resource_dispatcher_host_impl.h#newcode324
content/browser/loader/resource_dispatcher_host_impl.h:324: // For mojo:
nit: please document these new methods

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

https://codereview.chromium.org/1970693002/diff/310001/content/browser/loader/resource_message_filter.cc#newcode72
content/browser/loader/resource_message_filter.cc:72: // browser, since
it might allow a corrupt/malicious renderer to hang us.
no need for the second part of this comment, we don't send sync IPCs
from the browser period for the given reasons (there's an exception for
android webview but that's because it has to support existing android
APIs).

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

https://codereview.chromium.org/1970693002/diff/310001/content/browser/loader/url_loader_factory_impl.h#newcode1
content/browser/loader/url_loader_factory_impl.h:1: // Copyright 2016
The Chromium Authors. All rights reserved.
nit: please add entries for this file and its implementation to
content/browser/loader/DEPS. they should be files that will eventually
move to services/network

https://codereview.chromium.org/1970693002/diff/310001/content/browser/loader/url_loader_factory_impl.h#newcode17
content/browser/loader/url_loader_factory_impl.h:17: class
URLLoaderFactoryImpl final : public mojom::URLLoaderFactory {
nit: add class comment

https://codereview.chromium.org/1970693002/diff/310001/content/browser/renderer_host/render_process_host_impl.cc
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1970693002/diff/310001/content/browser/renderer_host/render_process_host_impl.cc#newcode1901
content/browser/renderer_host/render_process_host_impl.cc:1901: // We
need to destruct the filter on the IO thread.
the standard way we deal with this for BrowserMessageFilters is to
override BrowserMessageFilter::OnDestruct and call
BrowserThread::DeleteOnIOThread::Destruct(this);

i.e.
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/renderer_host/render_message_filter.cc&rcl=1463052012&l=259

https://codereview.chromium.org/1970693002/diff/310001/content/child/body_consumer.h
File content/child/body_consumer.h (right):

https://codereview.chromium.org/1970693002/diff/310001/content/child/body_consumer.h#newcode24
content/child/body_consumer.h:24: class BodyConsumer final : public
base::RefCounted<BodyConsumer> {
nit: please document this class and the methods

https://codereview.chromium.org/1970693002/

[yhi...@chromium.org]

Sorry for the late response.
I will address jam@'s comments tomorrow.

https://codereview.chromium.org/1970693002/diff/290001/content/common/url_loader.mojom
File content/common/url_loader.mojom (right):

https://codereview.chromium.org/1970693002/diff/290001/content/common/url_loader.mojom#newcode33
content/common/url_loader.mojom:33: string url;

On 2016/05/11 17:05:18, dcheng wrote:
> Use url.mojom.Url?

Thanks, but I'm now using IPC ParamTraits and we don't need this
property.

https://codereview.chromium.org/1970693002/diff/290001/content/common/url_loader_type_converters.cc
File content/common/url_loader_type_converters.cc (right):

https://codereview.chromium.org/1970693002/diff/290001/content/common/url_loader_type_converters.cc#newcode53
content/common/url_loader_type_converters.cc:53: URLRequestPtr
TypeConverter<URLRequestPtr, ResourceHostMsg_Request>::Convert(

On 2016/05/11 17:05:18, dcheng wrote:
> Let's avoid adding new type converters and use StructTraits instead.

Done (using ipc paramtraits)

https://codereview.chromium.org/1970693002/

[j...@chromium.org]

it's fine to reuse the existing C++ struct with their IPC traits since this
service is only called from C++ for now, and to ease the work in switching over.
Once we're switched completely to Mojo and the old IPC code is deleted, the C++
structs can be removed and we switch over to mojom structs.


https://codereview.chromium.org/1970693002/diff/410001/content/common/url_loader.mojom
File content/common/url_loader.mojom (right):

https://codereview.chromium.org/1970693002/diff/410001/content/common/url_loader.mojom#newcode20
content/common/url_loader.mojom:20: // |request_id| and |routing_id| are
for compatibility with the existing
I don't understand this, can you expand please?

routing IDs shouldn't be used with mojo; they're inherently part of the
old IPC.

I see that the service implementation reuses some RDHImpl methods that
take it. What happens if you pass an invalid routing id there?

https://codereview.chromium.org/1970693002/

[yzs...@chromium.org]

https://codereview.chromium.org/1970693002/diff/410001/content/DEPS
File content/DEPS (right):

https://codereview.chromium.org/1970693002/diff/410001/content/DEPS#newcode35
content/DEPS:35: "+mojo/message_pump",
We would like to remove it entirely and there are few places that
actually need it.

Please use mojo::Watcher in mojo/public/cpp/system instead of
mojo::common::HandleWatcher.

https://codereview.chromium.org/1970693002/diff/410001/content/browser/BUILD.gn
File content/browser/BUILD.gn (right):

https://codereview.chromium.org/1970693002/diff/410001/content/browser/BUILD.gn#newcode72
content/browser/BUILD.gn:72: "//mojo/message_pump",
Please see my comment in the DEPS file.

https://codereview.chromium.org/1970693002/diff/410001/content/browser/loader/async_resource_handler.cc
File content/browser/loader/async_resource_handler.cc (right):

https://codereview.chromium.org/1970693002/diff/410001/content/browser/loader/async_resource_handler.cc#newcode381
content/browser/loader/async_resource_handler.cc:381:
mojo::ScopedDataPipeConsumerHandle handle;
Not used?

https://codereview.chromium.org/1970693002/diff/410001/content/browser/loader/async_resource_handler.cc#newcode464
content/browser/loader/async_resource_handler.cc:464: return true;
Is it possible that this will cause mismatched
mojo::BeignWriteDataRaw/EndWriteDataRaw?

If a Begin is not matched by End, the next Begin will fail with
MOJO_RESULT_BUSY.

https://codereview.chromium.org/1970693002/diff/410001/content/browser/loader/async_resource_handler.h
File content/browser/loader/async_resource_handler.h (right):

https://codereview.chromium.org/1970693002/diff/410001/content/browser/loader/async_resource_handler.h#newcode106
content/browser/loader/async_resource_handler.h:106:
mojo::common::HandleWatcher handle_watcher_;
Please use this instead:

https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/cpp/system/watcher.h&q=watcher.&sq=package:chromium&l=1

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

https://codereview.chromium.org/1970693002/diff/410001/content/browser/loader/resource_dispatcher_host_impl.cc#newcode1640
content/browser/loader/resource_dispatcher_host_impl.cc:1640: child_id,
resource_context, client));
nit: it is not very obvious that "client" here actually means a boolean
value "using_mojo". Maybe make it a little more explicit.

https://codereview.chromium.org/1970693002/diff/410001/content/browser/renderer_host/render_process_host_impl.h
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/1970693002/diff/410001/content/browser/renderer_host/render_process_host_impl.h#newcode313
content/browser/renderer_host/render_process_host_impl.h:313:
mojo::InterfaceRequest<mojom::URLLoaderFactory> request);
nit: in case you don't know, you could use
mojom::URLLoaderFactoryRequest as a shorter name, just like
InterfacePtr<Foo> and FooPtr.

https://codereview.chromium.org/1970693002/diff/410001/content/child/body_consumer.cc
File content/child/body_consumer.cc (right):

https://codereview.chromium.org/1970693002/diff/410001/content/child/body_consumer.cc#newcode60
content/child/body_consumer.cc:60: has_been_cancelled_ = true;
Does it make sense to stop watching in this method?

(And DECHECK in OnReadable() that when it is called, the request must
haven't been canceled.)

https://codereview.chromium.org/1970693002/diff/410001/content/child/body_consumer.h
File content/child/body_consumer.h (right):

https://codereview.chromium.org/1970693002/diff/410001/content/child/body_consumer.h#newcode25
content/child/body_consumer.h:25: class BodyConsumer final : public
base::RefCounted<BodyConsumer> {
The name "BodyConsumer" seems a little bit vague.

https://codereview.chromium.org/1970693002/diff/410001/content/child/body_consumer.h#newcode48
content/child/body_consumer.h:48: mojo::common::HandleWatcher
handle_watcher_;
Please use mojo::Watcher instead.

One difference that should be noted is that once mojo::Watcher is
started, it keeps watching until explicitly told to stop. On the other
hand, mojo::common::HandleWatcher stops after one notification.

https://codereview.chromium.org/1970693002/

[yhi...@chromium.org]

Oops, I accidentally deleted PS7. Let me respond inline. Sorry for the
inconvenience.

On 2016/05/16 19:26:25, yzshen1 wrote:
> https://codereview.chromium.org/1970693002/diff/410001/content/DEPS
> File content/DEPS (right):
>
> https://codereview.chromium.org/1970693002/diff/410001/content/DEPS#newcode35
> content/DEPS:35: "+mojo/message_pump",
> We would like to remove it entirely and there are few places that actually
need
> it.
>
> Please use mojo::Watcher in mojo/public/cpp/system instead of
> mojo::common::HandleWatcher.
>
>
https://codereview.chromium.org/1970693002/diff/410001/content/browser/BUILD.gn
> File content/browser/BUILD.gn (right):
>
>
https://codereview.chromium.org/1970693002/diff/410001/content/browser/BUILD.gn#newcode72
> content/browser/BUILD.gn:72: "//mojo/message_pump",
> Please see my comment in the DEPS file.
>

I tried, but nothing is notified when I use mojo::Watcher. Can you tell me what
is wrong with PS8?

>
https://codereview.chromium.org/1970693002/diff/410001/content/browser/loader/async_resource_handler.cc
> File content/browser/loader/async_resource_handler.cc (right):
>
>
https://codereview.chromium.org/1970693002/diff/410001/content/browser/loader/async_resource_handler.cc#newcode381
> content/browser/loader/async_resource_handler.cc:381:
> mojo::ScopedDataPipeConsumerHandle handle;
> Not used?
>

Deleted.

>
https://codereview.chromium.org/1970693002/diff/410001/content/browser/loader/async_resource_handler.cc#newcode464
> content/browser/loader/async_resource_handler.cc:464: return true;
> Is it possible that this will cause mismatched
> mojo::BeignWriteDataRaw/EndWriteDataRaw?
>
> If a Begin is not matched by End, the next Begin will fail with
> MOJO_RESULT_BUSY.
>

It's not possible.

>
https://codereview.chromium.org/1970693002/diff/410001/content/browser/loader/async_resource_handler.h
> File content/browser/loader/async_resource_handler.h (right):
>
>
https://codereview.chromium.org/1970693002/diff/410001/content/browser/loader/async_resource_handler.h#newcode106
> content/browser/loader/async_resource_handler.h:106:
mojo::common::HandleWatcher
> handle_watcher_;
> Please use this instead:
>

ditto

>
https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/cpp/system/watcher.h&q=watcher.&sq=package:chromium&l=1
>
>
https://codereview.chromium.org/1970693002/diff/410001/content/browser/loader/resource_dispatcher_host_impl.cc
> File content/browser/loader/resource_dispatcher_host_impl.cc (right):
>
>
https://codereview.chromium.org/1970693002/diff/410001/content/browser/loader/resource_dispatcher_host_impl.cc#newcode1640
> content/browser/loader/resource_dispatcher_host_impl.cc:1640: child_id,
> resource_context, client));
> nit: it is not very obvious that "client" here actually means a boolean value
> "using_mojo". Maybe make it a little more explicit.
>

Please see above.

>
https://codereview.chromium.org/1970693002/diff/410001/content/browser/renderer_host/render_process_host_impl.h
> File content/browser/renderer_host/render_process_host_impl.h (right):
>
>
https://codereview.chromium.org/1970693002/diff/410001/content/browser/renderer_host/render_process_host_impl.h#newcode313
> content/browser/renderer_host/render_process_host_impl.h:313:
> mojo::InterfaceRequest<mojom::URLLoaderFactory> request);
> nit: in case you don't know, you could use mojom::URLLoaderFactoryRequest as a
> shorter name, just like InterfacePtr<Foo> and FooPtr.
>

I'd like to keep this name to avoid header include here.

>
https://codereview.chromium.org/1970693002/diff/410001/content/child/body_consumer.cc
> File content/child/body_consumer.cc (right):
>
>
https://codereview.chromium.org/1970693002/diff/410001/content/child/body_consumer.cc#newcode60
> content/child/body_consumer.cc:60: has_been_cancelled_ = true;
> Does it make sense to stop watching in this method?
>
> (And DECHECK in OnReadable() that when it is called, the request must haven't
> been canceled.)
>

Thanks, done.

>
https://codereview.chromium.org/1970693002/diff/410001/content/child/body_consumer.h
> File content/child/body_consumer.h (right):
>
>
https://codereview.chromium.org/1970693002/diff/410001/content/child/body_consumer.h#newcode25
> content/child/body_consumer.h:25: class BodyConsumer final : public
> base::RefCounted<BodyConsumer> {
> The name "BodyConsumer" seems a little bit vague.
>

Renamed to URLResponseBodyConsumer.

>
https://codereview.chromium.org/1970693002/diff/410001/content/child/body_consumer.h#newcode48
> content/child/body_consumer.h:48: mojo::common::HandleWatcher handle_watcher_;
> Please use mojo::Watcher instead.
>
> One difference that should be noted is that once mojo::Watcher is started, it
> keeps watching until explicitly told to stop. On the other hand,
> mojo::common::HandleWatcher stops after one notification.

Please see above.

https://codereview.chromium.org/1970693002/

[yhi...@chromium.org]

Hmm, I'm trying to keep the existing behavior as much as possible (although I
don't understand how it is used). It's used in CancelRequestsForRoute, and the
ID is set at [1]. Is it safe to use an invalid id?

1:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/render_frame_impl.cc&sq=package:chromium&l=3891&rcl=1463455165

https://codereview.chromium.org/1970693002/

[yhi...@chromium.org]

I added content/browser/loader/url_loader_factory_impl_unittest.cc and
content/child/url_response_body_consumer_unittest.cc.

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

https://codereview.chromium.org/1970693002/diff/310001/content/browser/loader/resource_dispatcher_host_impl.cc#newcode1799
content/browser/loader/resource_dispatcher_host_impl.cc:1799:
DCHECK(loader);

On 2016/05/13 00:55:05, jam wrote:
> nit: here and below, and also in next method, these dchecks are
unnecessary. if
> the pointers are null, the method calls will crash

Done.

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

https://codereview.chromium.org/1970693002/diff/310001/content/browser/loader/resource_dispatcher_host_impl.h#newcode324
content/browser/loader/resource_dispatcher_host_impl.h:324: // For mojo:

On 2016/05/13 00:55:05, jam wrote:
> nit: please document these new methods

Done.

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

https://codereview.chromium.org/1970693002/diff/310001/content/browser/loader/resource_message_filter.cc#newcode72
content/browser/loader/resource_message_filter.cc:72: // browser, since
it might allow a corrupt/malicious renderer to hang us.

On 2016/05/13 00:55:05, jam wrote:
> no need for the second part of this comment, we don't send sync IPCs
from the
> browser period for the given reasons (there's an exception for android
webview
> but that's because it has to support existing android APIs).

Done.

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

https://codereview.chromium.org/1970693002/diff/310001/content/browser/loader/url_loader_factory_impl.h#newcode1
content/browser/loader/url_loader_factory_impl.h:1: // Copyright 2016
The Chromium Authors. All rights reserved.

On 2016/05/13 00:55:05, jam wrote:
> nit: please add entries for this file and its implementation to
> content/browser/loader/DEPS. they should be files that will eventually
move to
> services/network

Done.

https://codereview.chromium.org/1970693002/diff/310001/content/browser/loader/url_loader_factory_impl.h#newcode17
content/browser/loader/url_loader_factory_impl.h:17: class
URLLoaderFactoryImpl final : public mojom::URLLoaderFactory {

On 2016/05/13 00:55:06, jam wrote:
> nit: add class comment

Done.

https://codereview.chromium.org/1970693002/diff/310001/content/browser/renderer_host/render_process_host_impl.cc
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1970693002/diff/310001/content/browser/renderer_host/render_process_host_impl.cc#newcode1901
content/browser/renderer_host/render_process_host_impl.cc:1901: // We
need to destruct the filter on the IO thread.

On 2016/05/13 00:55:06, jam wrote:
> the standard way we deal with this for BrowserMessageFilters is to
override
> BrowserMessageFilter::OnDestruct and call
> BrowserThread::DeleteOnIOThread::Destruct(this);
>
> i.e.
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/renderer_host/render_message_filter.cc&rcl=1463052012&l=259

Thanks, Done.

https://codereview.chromium.org/1970693002/diff/310001/content/child/body_consumer.h
File content/child/body_consumer.h (right):

https://codereview.chromium.org/1970693002/diff/310001/content/child/body_consumer.h#newcode24

content/child/body_consumer.h:24: class BodyConsumer final : public
base::RefCounted<BodyConsumer> {

On 2016/05/13 00:55:06, jam wrote:
> nit: please document this class and the methods

Done.

https://codereview.chromium.org/1970693002/

[yhi...@chromium.org]

https://codereview.chromium.org/1970693002/diff/510001/content/browser/loader/url_loader_factory_impl_unittest.cc
File content/browser/loader/url_loader_factory_impl_unittest.cc (right):

https://codereview.chromium.org/1970693002/diff/510001/content/browser/loader/url_loader_factory_impl_unittest.cc#newcode199
content/browser/loader/url_loader_factory_impl_unittest.cc:199:
&std::remove_reference<decltype(*this)>::type::RecordURLRequest,
I tried to avoid compile errors with this but it turns out it doesn't
work on windows. I will fix it tomorrow.

https://codereview.chromium.org/1970693002/

[yzs...@chromium.org]

Hi,

I haven't seen anything obviously wrong with your change.
It would be helpful if you could help narrow down the issue: which side doesn't
get notified? both? what is the result of mojo::Watcher::Start?

Thanks!

https://codereview.chromium.org/1970693002/

[j...@chromium.org]

I think we can eventually do this using mojo onconnectionerror callbacks. When
the renderer process dies, all mojo pipes to it will run that callback. So we
can clean up that way. Also, for RenderFrame destruction in the renderer it
should kill all pending loaders, and then that will also cause OnConnectionError
in the browser.

If you want to minimize the changes and do this later, that's fine. but please
don't send routing IDs in mojoms. You can use the service registry in
RenderFrameHostImpl, instead of RenderProcessHostImpl. Then the Create call will
know the associated RenderFrameHost and can grab its routing ID there without
having to send it in mojom.

https://codereview.chromium.org/1970693002/

[yhi...@chromium.org]

On 2016/05/17 17:56:30, yzshen1 wrote:
> Hi,
>
> I haven't seen anything obviously wrong with your change.
> It would be helpful if you could help narrow down the issue: which side
doesn't
> get notified? both? what is the result of mojo::Watcher::Start?
>
> Thanks!

I uploaded https://codereview.chromium.org/1990803002/ and
https://codereview.chromium.org/1985423002/.
The browser side Watcher in AsyncResourceHandler looks OK, and the renderer side
Watcher in URLResponseBodyConsumer is not working.
Interestingly, it works on the unittests (URLResponseBodyConsumerTest).

Start returns OK.

https://codereview.chromium.org/1970693002/

[yhi...@chromium.org]

Yes, I would like to provide routing IDs as WebURLLoaderImpl currently does.
Without passing routing ID via Load, we need to have URLLoaderFactory on
RenderProcessHostImpl *and* RenderFrameHostImpl, because I guess there is a case
where there's no associated RenderFrameImpl.
Are you agree with that? If it's the right way to go, I would do that in a
separate CL, so PS13 uses routing_id = 0.


https://codereview.chromium.org/1970693002/

[Yuzhu Shen]

It is interesting that it works in unittest but not in renderer.

Do you mind testing the browser side using mojo::Watcher, but the renderer side using mojo::common::HandleWatcher (I think previously your code worked with mojo::common::HandleWatcher, right)?

If it still works, that may indicate some issue with the mojo::Watcher implementation in multi-process settings.

+CC rockot

[yhi...@chromium.org]

> Do you mind testing the browser side using mojo::Watcher, but the renderer
> side using mojo::common::HandleWatcher (I think previously your code worked
> with mojo::common::HandleWatcher, right)?
> If it still works, that may indicate some issue with the mojo::Watcher
> implementation in multi-process settings.
>

I think https://codereview.chromium.org/1985423002/ does that (windows failures
are compile errors caused by __PRETTY_FUNCTION__).

https://codereview.chromium.org/1970693002/

[roc...@chromium.org]

On 2016/05/18 at 16:10:09, yzshen wrote:

> On Tue, May 17, 2016 at 11:50 PM, <yhi...@chromium.org&gt; wrote:
>
> > On 2016/05/17 17:56:30, yzshen1 wrote:
> > > Hi,
> > >
> > > I haven't seen anything obviously wrong with your change.
> > > It would be helpful if you could help narrow down the issue: which side
> > doesn't
> > > get notified? both? what is the result of mojo::Watcher::Start?
> > >
> > > Thanks!
> >
> > I uploaded https://codereview.chromium.org/1990803002/ and
> > https://codereview.chromium.org/1985423002/.
> > The browser side Watcher in AsyncResourceHandler looks OK, and the
> > renderer side
> > Watcher in URLResponseBodyConsumer is not working.
> > Interestingly, it works on the unittests (URLResponseBodyConsumerTest).
> >
> >
> It is interesting that it works in unittest but not in renderer.
>

> Do you mind testing the browser side using mojo::Watcher, but the renderer
> side using mojo::common::HandleWatcher (I think previously your code worked
> with mojo::common::HandleWatcher, right)?
> If it still works, that may indicate some issue with the mojo::Watcher
> implementation in multi-process settings.
>

> +CC rockot
>
>
> > Start returns OK.
> >
> > https://codereview.chromium.org/1970693002/
> >
>

> --
> You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email to chromium-revie...@chromium.org.
>

Watcher's a pretty simple mechanism. If Start returns OK, signal changes post
tasks to the main task runner of the thread where the Watcher lives. This
continues until the Watcher is destroyed or explicitly Canceled.

Is it somehow possible that the MessageLoop is not running, or has some other
weird behavior which would interfere with these watcher tasks being dispatched?

https://codereview.chromium.org/1970693002/

[yzs...@chromium.org]

On 2016/05/18 16:19:55, Ken Rockot wrote:
> On 2016/05/18 at 16:10:09, yzshen wrote:

> email to mailto:chromium-revie...@chromium.org.

> >
>
> Watcher's a pretty simple mechanism. If Start returns OK, signal changes post
> tasks to the main task runner of the thread where the Watcher lives. This
> continues until the Watcher is destroyed or explicitly Canceled.
>
> Is it somehow possible that the MessageLoop is not running, or has some other
> weird behavior which would interfere with these watcher tasks being
dispatched?

mojo::common::HandleWatcher requires MessageLoop too. If it works, the
MessageLoop on that thread is probably working fine.
Hmm...

https://codereview.chromium.org/1970693002/

[j...@chromium.org]

yep that sounds good, thanks. i have no other outstanding questions, so don't
wait for me. Kinuko can do the final review.

https://codereview.chromium.org/1970693002/

[kin...@chromium.org]

(Sorry for slow response!) Generally looking good, I haven't looked into the
test code & mojo-related details yet but sending out comments.

cc-ing mmenke@ for browser-side loading code (regular loading not going through
the code yet so fyi for now)


https://codereview.chromium.org/1970693002/diff/620001/content/browser/loader/async_resource_handler.cc
File content/browser/loader/async_resource_handler.cc (right):

https://codereview.chromium.org/1970693002/diff/620001/content/browser/loader/async_resource_handler.cc#newcode369
content/browser/loader/async_resource_handler.cc:369: }
nit: having mojo-related code scattered across a class hurts readability
a bit, does it make sense to have an inner class encapsulate
mojo-related members and routines until we start to feel more
comfortable? (Something like what we do for InliningHelper)

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

https://codereview.chromium.org/1970693002/diff/620001/content/browser/loader/resource_dispatcher_host_impl.h#newcode327
content/browser/loader/resource_dispatcher_host_impl.h:327: void
SetMojoLoaderAndClientrForNextLoadRequest(
nit: ...AndClientrFor -> ...AndClientFor ? (extra r)

https://codereview.chromium.org/1970693002/diff/620001/content/browser/loader/resource_dispatcher_host_impl.h#newcode340
content/browser/loader/resource_dispatcher_host_impl.h:340:
mojom::URLLoader* loader);
ditto, these are mojo-only impl details, is it possible to factor out
some of these into another class? Esp. I'm feeling a bit uncomfortable
having for_next_load & uninitiated_loaders bits directly in this class.

https://codereview.chromium.org/1970693002/diff/620001/content/browser/loader/resource_dispatcher_host_impl.h#newcode545
content/browser/loader/resource_dispatcher_host_impl.h:545: // These
function are used to trap messages about to sent to the renderer
nit: function -> functions

https://codereview.chromium.org/1970693002/diff/620001/content/browser/loader/resource_message_filter.cc
File content/browser/loader/resource_message_filter.cc (right):

https://codereview.chromium.org/1970693002/diff/620001/content/browser/loader/resource_message_filter.cc#newcode67
content/browser/loader/resource_message_filter.cc:67: NOTREACHED() <<
"Can't send sync message through ResourceMessageFilter!";
nit: any reason we don't use CHECK or DCHECK here, we're in the browser
process so this shouldn't happen other than by coding so?

https://codereview.chromium.org/1970693002/diff/620001/content/browser/loader/resource_request_info_impl.h
File content/browser/loader/resource_request_info_impl.h (right):

https://codereview.chromium.org/1970693002/diff/620001/content/browser/loader/resource_request_info_impl.h#newcode189
content/browser/loader/resource_request_info_impl.h:189: response_body_
= std::move(response_body);
nit: DCHECK(!response_body_.is_valid()) before setting?

https://codereview.chromium.org/1970693002/diff/620001/content/child/blink_platform_impl.h
File content/child/blink_platform_impl.h (right):

https://codereview.chromium.org/1970693002/diff/620001/content/child/blink_platform_impl.h#newcode166
content/child/blink_platform_impl.h:166: mojom::URLLoaderFactoryPtr
url_loader_factory_;
Do we need this on this class?

https://codereview.chromium.org/1970693002/diff/620001/content/child/resource_dispatcher.cc
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/1970693002/diff/620001/content/child/resource_dispatcher.cc#newcode91
content/child/resource_dispatcher.cc:91: body_consumer_ = new
URLResponseBodyConsumer(
nit: DCHECK(!body_consumer_) to make sure?

https://codereview.chromium.org/1970693002/diff/620001/content/child/resource_dispatcher.cc#newcode584
content/child/resource_dispatcher.cc:584: mojom::URLLoaderPtr
url_loader) {
This one's not used (yet), but you still wanted to pass this too
because-- for symmetricity?

https://codereview.chromium.org/1970693002/diff/620001/content/child/resource_dispatcher.h
File content/child/resource_dispatcher.h (right):

https://codereview.chromium.org/1970693002/diff/620001/content/child/resource_dispatcher.h#newcode162
content/child/resource_dispatcher.h:162:
std::unique_ptr<mojom::URLLoaderClient> url_loader_client;
nit: can we move these at the very end of the struct and after the
comment like "// Used only when MojoIPC is enabled" ?

https://codereview.chromium.org/1970693002/diff/620001/content/child/web_url_loader_impl.cc
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/1970693002/diff/620001/content/child/web_url_loader_impl.cc#newcode546
content/child/web_url_loader_impl.cc:546: }
nit: no {} for one-line body for consistency in this file

https://codereview.chromium.org/1970693002/diff/620001/third_party/WebKit/LayoutTests/VirtualTestSuites
File third_party/WebKit/LayoutTests/VirtualTestSuites (right):

https://codereview.chromium.org/1970693002/diff/620001/third_party/WebKit/LayoutTests/VirtualTestSuites#newcode258
third_party/WebKit/LayoutTests/VirtualTestSuites:258: "base":
"webexposed",
Why webexposed? Does the directory have interesting basic loading
tests??

https://codereview.chromium.org/1970693002/diff/620001/third_party/WebKit/public/platform/WebURLRequest.h
File third_party/WebKit/public/platform/WebURLRequest.h (right):

https://codereview.chromium.org/1970693002/diff/620001/third_party/WebKit/public/platform/WebURLRequest.h#newcode316
third_party/WebKit/public/platform/WebURLRequest.h:316:
BLINK_PLATFORM_EXPORT LoadingIPC loadingIPC() const;
nit: The method name feels a bit vague about what it returns, maybe make
this loadingIPCType() (and enum name to LoadingIPCType too)? Also--
it'll collide with enum name when we start to use capital method name,
so.. getLoadingIPCType()?

https://codereview.chromium.org/1970693002/

[yhi...@chromium.org]

https://codereview.chromium.org/1970693002/diff/620001/content/browser/loader/async_resource_handler.cc
File content/browser/loader/async_resource_handler.cc (right):

https://codereview.chromium.org/1970693002/diff/620001/content/browser/loader/async_resource_handler.cc#newcode369
content/browser/loader/async_resource_handler.cc:369: }

On 2016/05/20 09:38:51, kinuko wrote:
> nit: having mojo-related code scattered across a class hurts
readability a bit,
> does it make sense to have an inner class encapsulate mojo-related
members and
> routines until we start to feel more comfortable? (Something like
what we do
> for InliningHelper)

Done.

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

https://codereview.chromium.org/1970693002/diff/620001/content/browser/loader/resource_dispatcher_host_impl.h#newcode327
content/browser/loader/resource_dispatcher_host_impl.h:327: void
SetMojoLoaderAndClientrForNextLoadRequest(

On 2016/05/20 09:38:51, kinuko wrote:
> nit: ...AndClientrFor -> ...AndClientFor ? (extra r)

Done.

https://codereview.chromium.org/1970693002/diff/620001/content/browser/loader/resource_dispatcher_host_impl.h#newcode340
content/browser/loader/resource_dispatcher_host_impl.h:340:
mojom::URLLoader* loader);

On 2016/05/20 09:38:51, kinuko wrote:
> ditto, these are mojo-only impl details, is it possible to factor out
some of
> these into another class? Esp. I'm feeling a bit uncomfortable having
> for_next_load & uninitiated_loaders bits directly in this class.

Done, but I'm not sure whether the code is getting easier to read.

https://codereview.chromium.org/1970693002/diff/620001/content/browser/loader/resource_dispatcher_host_impl.h#newcode545
content/browser/loader/resource_dispatcher_host_impl.h:545: // These
function are used to trap messages about to sent to the renderer

On 2016/05/20 09:38:51, kinuko wrote:
> nit: function -> functions

Deleted

https://codereview.chromium.org/1970693002/diff/620001/content/browser/loader/resource_message_filter.cc
File content/browser/loader/resource_message_filter.cc (right):

https://codereview.chromium.org/1970693002/diff/620001/content/browser/loader/resource_message_filter.cc#newcode67
content/browser/loader/resource_message_filter.cc:67: NOTREACHED() <<
"Can't send sync message through ResourceMessageFilter!";

On 2016/05/20 09:38:51, kinuko wrote:
> nit: any reason we don't use CHECK or DCHECK here, we're in the
browser process
> so this shouldn't happen other than by coding so?

NOTREACHED is DCHECK.

https://code.google.com/p/chromium/codesearch#chromium/src/base/logging.h&q=notreached&sq=package:chromium&type=cs&l=759

https://codereview.chromium.org/1970693002/diff/620001/content/browser/loader/resource_request_info_impl.h
File content/browser/loader/resource_request_info_impl.h (right):

https://codereview.chromium.org/1970693002/diff/620001/content/browser/loader/resource_request_info_impl.h#newcode189
content/browser/loader/resource_request_info_impl.h:189: response_body_
= std::move(response_body);

On 2016/05/20 09:38:51, kinuko wrote:
> nit: DCHECK(!response_body_.is_valid()) before setting?

Done.

https://codereview.chromium.org/1970693002/diff/620001/content/child/blink_platform_impl.h
File content/child/blink_platform_impl.h (right):

https://codereview.chromium.org/1970693002/diff/620001/content/child/blink_platform_impl.h#newcode166
content/child/blink_platform_impl.h:166: mojom::URLLoaderFactoryPtr
url_loader_factory_;

On 2016/05/20 09:38:51, kinuko wrote:
> Do we need this on this class?

No, deleted

https://codereview.chromium.org/1970693002/diff/620001/content/child/resource_dispatcher.cc
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/1970693002/diff/620001/content/child/resource_dispatcher.cc#newcode91
content/child/resource_dispatcher.cc:91: body_consumer_ = new
URLResponseBodyConsumer(

On 2016/05/20 09:38:51, kinuko wrote:
> nit: DCHECK(!body_consumer_) to make sure?

Done.

https://codereview.chromium.org/1970693002/diff/620001/content/child/resource_dispatcher.cc#newcode584
content/child/resource_dispatcher.cc:584: mojom::URLLoaderPtr
url_loader) {

On 2016/05/20 09:38:51, kinuko wrote:
> This one's not used (yet), but you still wanted to pass this too
because-- for
> symmetricity?

Yes.

https://codereview.chromium.org/1970693002/diff/620001/content/child/resource_dispatcher.h
File content/child/resource_dispatcher.h (right):

https://codereview.chromium.org/1970693002/diff/620001/content/child/resource_dispatcher.h#newcode162
content/child/resource_dispatcher.h:162:
std::unique_ptr<mojom::URLLoaderClient> url_loader_client;

On 2016/05/20 09:38:51, kinuko wrote:
> nit: can we move these at the very end of the struct and after the
comment like
> "// Used only when MojoIPC is enabled" ?

Done.

https://codereview.chromium.org/1970693002/diff/620001/content/child/web_url_loader_impl.cc
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/1970693002/diff/620001/content/child/web_url_loader_impl.cc#newcode546
content/child/web_url_loader_impl.cc:546: }

On 2016/05/20 09:38:51, kinuko wrote:
> nit: no {} for one-line body for consistency in this file

Done.

https://codereview.chromium.org/1970693002/diff/620001/third_party/WebKit/LayoutTests/VirtualTestSuites
File third_party/WebKit/LayoutTests/VirtualTestSuites (right):

https://codereview.chromium.org/1970693002/diff/620001/third_party/WebKit/LayoutTests/VirtualTestSuites#newcode258
third_party/WebKit/LayoutTests/VirtualTestSuites:258: "base":
"webexposed",

On 2016/05/20 09:38:51, kinuko wrote:
> Why webexposed? Does the directory have interesting basic loading
tests??

It's important that they are not interesting. Mojo is used when html
files are fetched, and that's what I want to test for this change.

https://codereview.chromium.org/1970693002/diff/620001/third_party/WebKit/public/platform/WebURLRequest.h
File third_party/WebKit/public/platform/WebURLRequest.h (right):

https://codereview.chromium.org/1970693002/diff/620001/third_party/WebKit/public/platform/WebURLRequest.h#newcode316
third_party/WebKit/public/platform/WebURLRequest.h:316:
BLINK_PLATFORM_EXPORT LoadingIPC loadingIPC() const;

On 2016/05/20 09:38:51, kinuko wrote:
> nit: The method name feels a bit vague about what it returns, maybe
make this
> loadingIPCType() (and enum name to LoadingIPCType too)? Also-- it'll
collide
> with enum name when we start to use capital method name, so..
> getLoadingIPCType()?

Done.

https://codereview.chromium.org/1970693002/

[kin...@chromium.org]

Some more comments (will take further look next week)

(Gotchas around RDH still bothers me a bit, but I'm not fully sure how we can
make it cleaner either)

https://codereview.chromium.org/1970693002/diff/620001/content/browser/loader/resource_message_filter.cc
File content/browser/loader/resource_message_filter.cc (right):

https://codereview.chromium.org/1970693002/diff/620001/content/browser/loader/resource_message_filter.cc#newcode67
content/browser/loader/resource_message_filter.cc:67: NOTREACHED() <<
"Can't send sync message through ResourceMessageFilter!";

On 2016/05/20 11:32:54, yhirano wrote:
> On 2016/05/20 09:38:51, kinuko wrote:
> > nit: any reason we don't use CHECK or DCHECK here, we're in the
browser
> process
> > so this shouldn't happen other than by coding so?
>
> NOTREACHED is DCHECK.
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/logging.h&q=notreached&sq=package:chromium&type=cs&l=759

I'm meaning this:

https://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-

Handling DCHECK failure is discouraged, just use DCHECK instead if it
wouldn't happen other than by coding error.

https://codereview.chromium.org/1970693002/diff/620001/third_party/WebKit/LayoutTests/VirtualTestSuites
File third_party/WebKit/LayoutTests/VirtualTestSuites (right):

https://codereview.chromium.org/1970693002/diff/620001/third_party/WebKit/LayoutTests/VirtualTestSuites#newcode258
third_party/WebKit/LayoutTests/VirtualTestSuites:258: "base":
"webexposed",

On 2016/05/20 11:32:54, yhirano wrote:
> On 2016/05/20 09:38:51, kinuko wrote:
> > Why webexposed? Does the directory have interesting basic loading
tests??
>
> It's important that they are not interesting. Mojo is used when html
files are
> fetched, and that's what I want to test for this change.

Oh I see... (I wish we could have a comment to note that in this file...
but it's json and maybe we can't)

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

https://codereview.chromium.org/1970693002/diff/660001/content/browser/loader/resource_dispatcher_host_impl.cc#newcode1326
content/browser/loader/resource_dispatcher_host_impl.cc:1326:
mojo_helper_->TakeClientForNextLoadRequest();
nit: can these be taken by a single method call by returning these via
out-params?

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

https://codereview.chromium.org/1970693002/diff/660001/content/browser/loader/resource_dispatcher_host_impl.h#newcode327
content/browser/loader/resource_dispatcher_host_impl.h:327: void
SetMojoLoaderAndClientForNextLoadRequest(
Would it possible / make sense to merge this method and
TakeUninitiatedURLLoader?

https://codereview.chromium.org/1970693002/diff/660001/content/browser/loader/url_loader_factory_impl.cc
File content/browser/loader/url_loader_factory_impl.cc (right):

https://codereview.chromium.org/1970693002/diff/660001/content/browser/loader/url_loader_factory_impl.cc#newcode59
content/browser/loader/url_loader_factory_impl.cc:59: filter);
Actually... is there a reason we want to call OnRequestResource via
OnMessageReceived? If we just directly call OnRequestResourceWithMojo
or something that'd allow us to remove
SetMojoLoaderAndClientForNextLoadRequest?

https://codereview.chromium.org/1970693002/

[yhi...@chromium.org]

https://codereview.chromium.org/1970693002/diff/620001/third_party/WebKit/LayoutTests/VirtualTestSuites
File third_party/WebKit/LayoutTests/VirtualTestSuites (right):

https://codereview.chromium.org/1970693002/diff/620001/third_party/WebKit/LayoutTests/VirtualTestSuites#newcode258
third_party/WebKit/LayoutTests/VirtualTestSuites:258: "base":
"webexposed",

On 2016/05/20 15:56:46, kinuko wrote:
> On 2016/05/20 11:32:54, yhirano wrote:
> > On 2016/05/20 09:38:51, kinuko wrote:
> > > Why webexposed? Does the directory have interesting basic loading
tests??
> >
> > It's important that they are not interesting. Mojo is used when html
files are
> > fetched, and that's what I want to test for this change.
>
> Oh I see... (I wish we could have a comment to note that in this
file... but
> it's json and maybe we can't)

Added a comment in README.txt.

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

https://codereview.chromium.org/1970693002/diff/660001/content/browser/loader/resource_dispatcher_host_impl.cc#newcode1326
content/browser/loader/resource_dispatcher_host_impl.cc:1326:
mojo_helper_->TakeClientForNextLoadRequest();

On 2016/05/20 15:56:46, kinuko wrote:
> nit: can these be taken by a single method call by returning these via
> out-params?

Removed

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

https://codereview.chromium.org/1970693002/diff/660001/content/browser/loader/resource_dispatcher_host_impl.h#newcode327
content/browser/loader/resource_dispatcher_host_impl.h:327: void
SetMojoLoaderAndClientForNextLoadRequest(

On 2016/05/20 15:56:46, kinuko wrote:
> Would it possible / make sense to merge this method and
> TakeUninitiatedURLLoader?

Removed.

https://codereview.chromium.org/1970693002/diff/660001/content/browser/loader/url_loader_factory_impl.cc
File content/browser/loader/url_loader_factory_impl.cc (right):

https://codereview.chromium.org/1970693002/diff/660001/content/browser/loader/url_loader_factory_impl.cc#newcode59
content/browser/loader/url_loader_factory_impl.cc:59: filter);

On 2016/05/20 15:56:47, kinuko wrote:
> Actually... is there a reason we want to call OnRequestResource via
> OnMessageReceived? If we just directly call OnRequestResourceWithMojo
or
> something that'd allow us to remove
SetMojoLoaderAndClientForNextLoadRequest?

Done.

https://codereview.chromium.org/1970693002/

[mme...@chromium.org]

Some quick comments. I'm concerned about the circular dependencies this
introduces, and wonder if ResourceLoader really needs to know about mojo, and if
RDH should manage stuff that sits below the AsyncResourceHandler.


https://codereview.chromium.org/1970693002/diff/740001/content/browser/loader/async_resource_handler.cc
File content/browser/loader/async_resource_handler.cc (right):

https://codereview.chromium.org/1970693002/diff/740001/content/browser/loader/async_resource_handler.cc#newcode195
content/browser/loader/async_resource_handler.cc:195: class
AsyncResourceHandler::MojoHelper final {
Think this class needs some docs

https://codereview.chromium.org/1970693002/diff/740001/content/browser/loader/async_resource_handler.cc#newcode220
content/browser/loader/async_resource_handler.cc:220: //
OnReadCompleted.
Avoid "we" in comments.

https://codereview.chromium.org/1970693002/diff/740001/content/browser/loader/async_resource_handler.cc#newcode471
content/browser/loader/async_resource_handler.cc:471: return
mojo_helper_->OnWillRead(buf, buf_size, min_size);
When using the helper, we also send the results over IPC?

https://codereview.chromium.org/1970693002/diff/740001/content/browser/loader/async_resource_handler.h
File content/browser/loader/async_resource_handler.h (right):

https://codereview.chromium.org/1970693002/diff/740001/content/browser/loader/async_resource_handler.h#newcode77
content/browser/loader/async_resource_handler.h:77: void
OnWritable(MojoResult result);
This method doesn't exist.

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

https://codereview.chromium.org/1970693002/diff/740001/content/browser/loader/resource_dispatcher_host_impl.cc#newcode541
content/browser/loader/resource_dispatcher_host_impl.cc:541: class
ResourceDispatcherHostImpl::MojoHelper final {
Add comments.

https://codereview.chromium.org/1970693002/diff/740001/content/browser/loader/resource_dispatcher_host_impl.cc#newcode603
content/browser/loader/resource_dispatcher_host_impl.cc:603:
client->OnReceiveResponse(head,
loader->GetRequestInfo()->TakeBodyReader());
Why do we need to pass the body reader all over the place? The
dependency diagram here seems rather unfortunate.

Can we do this all at the AsyncResourceHandler layer instead? Having
the AsyncResourceHandler both at the bottom of the ResourceHandle chain,
below the ResourceLoader (Which talked directly to the RDH), and also
talk to a private RDH inner class through this sidechannel seems very
unfortunate to me.

https://codereview.chromium.org/1970693002/diff/740001/content/browser/loader/resource_dispatcher_host_impl.cc#newcode1314
content/browser/loader/resource_dispatcher_host_impl.cc:1314:
std::unique_ptr<mojom::URLLoader> loader,
loader is not a good variable name, given that we also have
content::ResourceLoader.

https://codereview.chromium.org/1970693002/diff/740001/content/browser/loader/resource_request_info_impl.h
File content/browser/loader/resource_request_info_impl.h (right):

https://codereview.chromium.org/1970693002/diff/740001/content/browser/loader/resource_request_info_impl.h#newcode237
content/browser/loader/resource_request_info_impl.h:237:
mojo::ScopedDataPipeConsumerHandle response_body_;
I'm not actually sure what this is (Needs comments!), but if I can't get
the response body from it, it's not the response body.

https://codereview.chromium.org/1970693002/

[yzs...@chromium.org]

Hi, Yutaka.
In case you are waiting for my further input on this:
It seems unnecessary to block your work with the issue of using mojo::Watcher in
renderer.
I am fine if you would like to use mojo::common::HandleWatcher there. (If you
can use mojo::Watcher in browser, please do so.)
It would be nice if you could come up with a small repro case and file a bug so
that someone in the mojo team can investigate the problem.

Thanks!

https://codereview.chromium.org/1970693002/

[yhi...@chromium.org]

https://codereview.chromium.org/1970693002/diff/740001/content/browser/loader/async_resource_handler.cc
File content/browser/loader/async_resource_handler.cc (right):

https://codereview.chromium.org/1970693002/diff/740001/content/browser/loader/async_resource_handler.cc#newcode195
content/browser/loader/async_resource_handler.cc:195: class
AsyncResourceHandler::MojoHelper final {

On 2016/05/23 17:34:53, mmenke wrote:
> Think this class needs some docs

Done.

https://codereview.chromium.org/1970693002/diff/740001/content/browser/loader/async_resource_handler.cc#newcode220
content/browser/loader/async_resource_handler.cc:220: //
OnReadCompleted.

On 2016/05/23 17:34:53, mmenke wrote:
> Avoid "we" in comments.

Done.

https://codereview.chromium.org/1970693002/diff/740001/content/browser/loader/async_resource_handler.cc#newcode471
content/browser/loader/async_resource_handler.cc:471: return
mojo_helper_->OnWillRead(buf, buf_size, min_size);

On 2016/05/23 17:34:53, mmenke wrote:
> When using the helper, we also send the results over IPC?

No.

https://codereview.chromium.org/1970693002/diff/740001/content/browser/loader/async_resource_handler.h
File content/browser/loader/async_resource_handler.h (right):

https://codereview.chromium.org/1970693002/diff/740001/content/browser/loader/async_resource_handler.h#newcode77
content/browser/loader/async_resource_handler.h:77: void
OnWritable(MojoResult result);

On 2016/05/23 17:34:53, mmenke wrote:
> This method doesn't exist.

Thanks, done.

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

https://codereview.chromium.org/1970693002/diff/740001/content/browser/loader/resource_dispatcher_host_impl.cc#newcode541
content/browser/loader/resource_dispatcher_host_impl.cc:541: class
ResourceDispatcherHostImpl::MojoHelper final {

On 2016/05/23 17:34:53, mmenke wrote:
> Add comments.

Done.

https://codereview.chromium.org/1970693002/diff/740001/content/browser/loader/resource_dispatcher_host_impl.cc#newcode603
content/browser/loader/resource_dispatcher_host_impl.cc:603:
client->OnReceiveResponse(head,
loader->GetRequestInfo()->TakeBodyReader());

On 2016/05/23 17:34:53, mmenke wrote:
> Why do we need to pass the body reader all over the place? The
dependency
> diagram here seems rather unfortunate.
>

What dependency are you concerned about? Even without this CL,
ResourceDispatcherHostImpl calls loader->GetRequestInfo() (in
CancelRequestsForContext) and AsyncResourceHandler calls
GetRequestInfo() (such as OnRequestRedirected). I didn't think I
introduced a new dependency here. Please correct me.

https://codereview.chromium.org/1970693002/diff/740001/content/browser/loader/resource_dispatcher_host_impl.cc#newcode1314
content/browser/loader/resource_dispatcher_host_impl.cc:1314:
std::unique_ptr<mojom::URLLoader> loader,

On 2016/05/23 17:34:53, mmenke wrote:
> loader is not a good variable name, given that we also have
> content::ResourceLoader.

Done.

https://codereview.chromium.org/1970693002/

[yhi...@chromium.org]

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

https://codereview.chromium.org/1970693002/diff/740001/content/browser/loader/resource_dispatcher_host_impl.cc#newcode603
content/browser/loader/resource_dispatcher_host_impl.cc:603:
client->OnReceiveResponse(head,
loader->GetRequestInfo()->TakeBodyReader());
On 2016/05/23 17:34:53, mmenke wrote:
> Why do we need to pass the body reader all over the place? The
dependency
> diagram here seems rather unfortunate.
>

> Can we do this all at the AsyncResourceHandler layer instead? Having
the
> AsyncResourceHandler both at the bottom of the ResourceHandle chain,
below the
> ResourceLoader (Which talked directly to the RDH), and also talk to a
private
> RDH inner class through this sidechannel seems very unfortunate to me.

PS24 removes the response body from ResourceRequestInfoImpl and uses
ResourceMessageFilter instead.

https://codereview.chromium.org/1970693002/diff/740001/content/browser/loader/resource_request_info_impl.h
File content/browser/loader/resource_request_info_impl.h (right):

https://codereview.chromium.org/1970693002/diff/740001/content/browser/loader/resource_request_info_impl.h#newcode237
content/browser/loader/resource_request_info_impl.h:237:
mojo::ScopedDataPipeConsumerHandle response_body_;

On 2016/05/23 17:34:53, mmenke wrote:

> I'm not actually sure what this is (Needs comments!), but if I can't
get the
> response body from it, it's not the response body.

Removed at PS24.

https://codereview.chromium.org/1970693002/

[mme...@chromium.org]

[+rdsmith]: FYI

Plan to do another pass later today.

https://codereview.chromium.org/1970693002/

[mme...@chromium.org]

https://codereview.chromium.org/1970693002/diff/800001/content/browser/loader/async_resource_handler.cc

File content/browser/loader/async_resource_handler.cc (right):

https://codereview.chromium.org/1970693002/diff/800001/content/browser/loader/async_resource_handler.cc#newcode198

content/browser/loader/async_resource_handler.cc:198: class
AsyncResourceHandler::MojoHelper final {
Long term, is the plan to merge this into AsyncResourceHandler? Once
we're just using mojo, doesn't seem like there'll be much left there.

https://codereview.chromium.org/1970693002/diff/800001/content/browser/loader/async_resource_handler.cc#newcode236
content/browser/loader/async_resource_handler.cc:236: return false;
This case is currently a fatal error?

https://codereview.chromium.org/1970693002/diff/800001/content/browser/loader/async_resource_handler.cc#newcode556
content/browser/loader/async_resource_handler.cc:556: void
AsyncResourceHandler::OnResponseCompleted(
There's no mojo response completed successfully or request failed
message?

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

https://codereview.chromium.org/1970693002/diff/800001/content/browser/loader/resource_dispatcher_host_impl.cc#newcode542
content/browser/loader/resource_dispatcher_host_impl.cc:542: //
ResourceDispatcherHostImpl.
This is confusing...Why is the filter calling into this method to pass
messages from AsyncResourceHandler? Why isn't AsyncResourceHandler
sending those directly, via its mojo-thing, like it does for everything
else?

https://codereview.chromium.org/1970693002/

[mme...@chromium.org]

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

https://codereview.chromium.org/1970693002/diff/800001/content/browser/loader/resource_dispatcher_host_impl.cc#newcode1767
content/browser/loader/resource_dispatcher_host_impl.cc:1767:
handler.reset(new AsyncResourceHandler(request, this, using_mojo));
Why doesn't the AsyncResourceHandler take ownership of the mojo stuff?
It's the only class here that actually talks to mojo, and it should be
the only class sending mojo data.

(Eventually the sync one may need to as well, of course)

https://codereview.chromium.org/1970693002/

[mme...@chromium.org]

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

https://codereview.chromium.org/1970693002/diff/800001/content/browser/loader/resource_dispatcher_host_impl.cc#newcode1767
content/browser/loader/resource_dispatcher_host_impl.cc:1767:
handler.reset(new AsyncResourceHandler(request, this, using_mojo));

On 2016/05/24 20:06:07, mmenke wrote:
> Why doesn't the AsyncResourceHandler take ownership of the mojo stuff?
It's the
> only class here that actually talks to mojo, and it should be the only
class
> sending mojo data.
>
> (Eventually the sync one may need to as well, of course)

Actually, can we just make a MojoResourceHandler, to replace the
AsyncResourceHandler, rather than have the AsyncResourceHandler have two
paths? Then we'll work towards getting rid of the AsyncResourceHandler
entirely.

https://codereview.chromium.org/1970693002/

[kin...@chromium.org]

+1, that sounds sensible to me.

https://codereview.chromium.org/1970693002/

[yhi...@chromium.org]

That sounds great, thank you.
Some code (code in OnResponseCompleted, RecordHistogram, etc) will be
duplicated, is that OK? I prefer staying in AsyncResourceHandler to create a
helper class for that duplicated code.



https://codereview.chromium.org/1970693002/

[yhi...@chromium.org]

https://codereview.chromium.org/1970693002/diff/800001/content/browser/loader/async_resource_handler.cc
File content/browser/loader/async_resource_handler.cc (right):

https://codereview.chromium.org/1970693002/diff/800001/content/browser/loader/async_resource_handler.cc#newcode198
content/browser/loader/async_resource_handler.cc:198: class
AsyncResourceHandler::MojoHelper final {

On 2016/05/24 20:01:35, mmenke wrote:
> Long term, is the plan to merge this into AsyncResourceHandler? Once
we're just
> using mojo, doesn't seem like there'll be much left there.

Yes, our plan is replacing ChromeIPC with mojo.

https://codereview.chromium.org/1970693002/diff/800001/content/browser/loader/async_resource_handler.cc#newcode236
content/browser/loader/async_resource_handler.cc:236: return false;

On 2016/05/24 20:01:35, mmenke wrote:
> This case is currently a fatal error?

Done.

https://codereview.chromium.org/1970693002/diff/800001/content/browser/loader/async_resource_handler.cc#newcode556
content/browser/loader/async_resource_handler.cc:556: void
AsyncResourceHandler::OnResponseCompleted(

On 2016/05/24 20:01:35, mmenke wrote:
> There's no mojo response completed successfully or request failed
message?

There is. In PS25, ResourceMessageFilter traps IPC messages and send
them via mojo if it's appropriate.

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

https://codereview.chromium.org/1970693002/diff/800001/content/browser/loader/resource_dispatcher_host_impl.cc#newcode542
content/browser/loader/resource_dispatcher_host_impl.cc:542: //
ResourceDispatcherHostImpl.

On 2016/05/24 20:01:35, mmenke wrote:
> This is confusing...Why is the filter calling into this method to pass
messages
> from AsyncResourceHandler? Why isn't AsyncResourceHandler sending
those
> directly, via its mojo-thing, like it does for everything else?

Done.

https://codereview.chromium.org/1970693002/diff/800001/content/browser/loader/resource_dispatcher_host_impl.cc#newcode1767
content/browser/loader/resource_dispatcher_host_impl.cc:1767:
handler.reset(new AsyncResourceHandler(request, this, using_mojo));

On 2016/05/25 04:39:03, mmenke wrote:
> On 2016/05/24 20:06:07, mmenke wrote:
> > Why doesn't the AsyncResourceHandler take ownership of the mojo
stuff? It's
> the
> > only class here that actually talks to mojo, and it should be the
only class
> > sending mojo data.
> >
> > (Eventually the sync one may need to as well, of course)
>
> Actually, can we just make a MojoResourceHandler, to replace the
> AsyncResourceHandler, rather than have the AsyncResourceHandler have
two paths?
> Then we'll work towards getting rid of the AsyncResourceHandler
entirely.

Done.

https://codereview.chromium.org/1970693002/

[mme...@chromium.org]

I'm much happier with this. Haven't taken a close look at the new resource
handler, just skimmed over it.


https://codereview.chromium.org/1970693002/diff/880001/content/browser/loader/mojo_async_resource_handler.cc
File content/browser/loader/mojo_async_resource_handler.cc (right):

https://codereview.chromium.org/1970693002/diff/880001/content/browser/loader/mojo_async_resource_handler.cc#newcode31
content/browser/loader/mojo_async_resource_handler.cc:31: int
maxAllocationSize = 1024 * 32;
max_allocation_size, or better, g_max_allocation_size (As suggested, but
not required, by style guide).

https://codereview.chromium.org/1970693002/diff/880001/content/browser/loader/mojo_async_resource_handler.cc#newcode46
content/browser/loader/mojo_async_resource_handler.cc:46:
GetNumericArg("resource-buffer-max-allocation-size",
&maxAllocationSize);
Wow..I was going to complain about this being weird and ugly, but it's
just what AsyncResourceHandler does. :(

https://codereview.chromium.org/1970693002/diff/880001/content/browser/loader/url_loader_factory_impl.cc
File content/browser/loader/url_loader_factory_impl.cc (right):

https://codereview.chromium.org/1970693002/diff/880001/content/browser/loader/url_loader_factory_impl.cc#newcode30
content/browser/loader/url_loader_factory_impl.cc:30:
ResourceDispatcherHostImpl::Get()->AddUninitiatedURLLoader(
"Uninitiated" seems like a more ambiguous term for "NotStarted" /
"Unstarted". Could we rename this?

https://codereview.chromium.org/1970693002/diff/880001/content/browser/loader/url_loader_factory_impl.cc#newcode32
content/browser/loader/url_loader_factory_impl.cc:32:
base::WrapUnique(new URLLoaderImpl(filter.get(), std::move(request))));
This ownership model seems really weird and non-obvious. "Create"
creates a URLLoader, and gives it to the RDH. And then calling Load on
a URLLoader (Not quite sure how someone finds it to call load on in the
first place...) makes the object take ownership of itself back...Just to
pass ownership *back* to the RDH.

Can we do something better here? I guess the mojo code doesn't expect
to own the mojo communications objects itself?

https://codereview.chromium.org/1970693002/diff/880001/content/common/url_loader.mojom
File content/common/url_loader.mojom (right):

https://codereview.chromium.org/1970693002/diff/880001/content/common/url_loader.mojom#newcode17
content/common/url_loader.mojom:17: // Loads the given |request|,
asynchronously producing |response|. Consult
|response|? I don't see anything with that name here.

https://codereview.chromium.org/1970693002/diff/880001/content/common/url_loader.mojom#newcode20
content/common/url_loader.mojom:20: // |request_id| is for compatibility
with the existing ChromeIPC.
I don't think ChromeIPC is one word?

https://codereview.chromium.org/1970693002/diff/880001/content/common/url_loader.mojom#newcode21
content/common/url_loader.mojom:21: Load(int32 request_id,
Is there a reason this is "Load" and not "Start". Think it's best to
use consistent terms across wrapped APIs, unless there's a good reason
not to. Also, "Load" seems to have more of a "Do this <entire> thing
and get back to me" implication.

https://codereview.chromium.org/1970693002/diff/880001/content/common/url_loader.mojom#newcode22
content/common/url_loader.mojom:22: URLRequest request,
Hrm...I'm not sure the world can handle yet another class named
URLRequest. Not that I have a better suggestion.

https://codereview.chromium.org/1970693002/diff/880001/content/common/url_loader.mojom#newcode30
content/common/url_loader.mojom:30: // Cancels the request.
Can we still get data after calling Cancel, or does it instantly cut off
the data stream? I'm assuming the former?

https://codereview.chromium.org/1970693002/

[kin...@chromium.org]

A few lightweight comments

https://codereview.chromium.org/1970693002/diff/880001/content/browser/loader/mojo_async_resource_handler.cc
File content/browser/loader/mojo_async_resource_handler.cc (right):

https://codereview.chromium.org/1970693002/diff/880001/content/browser/loader/mojo_async_resource_handler.cc#newcode197
content/browser/loader/mojo_async_resource_handler.cc:197: // Not
implemented.
NOTIMPLEMENTED() ?

https://codereview.chromium.org/1970693002/diff/880001/content/browser/loader/mojo_async_resource_handler.cc#newcode214
content/browser/loader/mojo_async_resource_handler.cc:214:
sent_received_response_message_);
Not entirely sure if we should mirror these CHECK and TODO comment...

https://codereview.chromium.org/1970693002/diff/880001/content/browser/loader/mojo_async_resource_handler.cc#newcode294
content/browser/loader/mojo_async_resource_handler.cc:294: 100000, 100);
We should probably use different histogram names for them, or just
removing these at all for now (these were added for tracking benefits of
IPC-related optimizations, having these as is here doesn't make much
sense)

https://codereview.chromium.org/1970693002/diff/880001/content/browser/loader/mojo_async_resource_handler.h
File content/browser/loader/mojo_async_resource_handler.h (right):

https://codereview.chromium.org/1970693002/diff/880001/content/browser/loader/mojo_async_resource_handler.h#newcode31
content/browser/loader/mojo_async_resource_handler.h:31: // load events
from the resource dispatcher host.
nit: add a comment to note that this is used only when LoadingWithMojo
is enabled

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

https://codereview.chromium.org/1970693002/diff/880001/content/browser/loader/resource_dispatcher_host_impl.cc#newcode549
content/browser/loader/resource_dispatcher_host_impl.cc:549: class
ResourceDispatcherHostImpl::MojoHelper final {
(At this point it may not make much sense to have this helper class..)

https://codereview.chromium.org/1970693002/

[mme...@chromium.org]

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

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/resource_dispatcher_host_impl.cc#newcode1432
content/browser/loader/resource_dispatcher_host_impl.cc:1432: if (it !=
pending_loaders_.end()) {
This case isn't being handled.

https://codereview.chromium.org/1970693002/diff/940001/content/common/url_loader_factory.mojom
File content/common/url_loader_factory.mojom (right):

https://codereview.chromium.org/1970693002/diff/940001/content/common/url_loader_factory.mojom#newcode10
content/common/url_loader_factory.mojom:10: CreateURLLoader(URLLoader&
loader);
I don't suppose we can combine with with Start? i.e. take a request_id,
URLRequest, and URLLoaderClient? I'm not set on a combined create+start
being the ideal API, but it makes ownership semantecs for the URLLoader
sane. If, once the RDH is using a purely mojo API, you want to change
how things work, and have a reasonably ownership model to go with it,
I'm certainly fine with changing it.

https://codereview.chromium.org/1970693002/

[yhi...@chromium.org]

https://codereview.chromium.org/1970693002/diff/880001/content/browser/loader/mojo_async_resource_handler.cc
File content/browser/loader/mojo_async_resource_handler.cc (right):

https://codereview.chromium.org/1970693002/diff/880001/content/browser/loader/mojo_async_resource_handler.cc#newcode31
content/browser/loader/mojo_async_resource_handler.cc:31: int
maxAllocationSize = 1024 * 32;

On 2016/05/25 16:17:20, mmenke wrote:
> max_allocation_size, or better, g_max_allocation_size (As suggested,
but not
> required, by style guide).

Thanks, done.

https://codereview.chromium.org/1970693002/diff/880001/content/browser/loader/mojo_async_resource_handler.cc#newcode197
content/browser/loader/mojo_async_resource_handler.cc:197: // Not
implemented.

On 2016/05/26 08:43:12, kinuko wrote:
> NOTIMPLEMENTED() ?

I think it's not good to output error messages.

https://codereview.chromium.org/1970693002/diff/880001/content/browser/loader/mojo_async_resource_handler.cc#newcode214
content/browser/loader/mojo_async_resource_handler.cc:214:
sent_received_response_message_);

On 2016/05/26 08:43:12, kinuko wrote:
> Not entirely sure if we should mirror these CHECK and TODO comment...

Deleted.

https://codereview.chromium.org/1970693002/diff/880001/content/browser/loader/mojo_async_resource_handler.cc#newcode294
content/browser/loader/mojo_async_resource_handler.cc:294: 100000, 100);

On 2016/05/26 08:43:12, kinuko wrote:
> We should probably use different histogram names for them, or just
removing
> these at all for now (these were added for tracking benefits of
IPC-related
> optimizations, having these as is here doesn't make much sense)

Done.

https://codereview.chromium.org/1970693002/diff/880001/content/browser/loader/mojo_async_resource_handler.h
File content/browser/loader/mojo_async_resource_handler.h (right):

https://codereview.chromium.org/1970693002/diff/880001/content/browser/loader/mojo_async_resource_handler.h#newcode31
content/browser/loader/mojo_async_resource_handler.h:31: // load events
from the resource dispatcher host.

On 2016/05/26 08:43:12, kinuko wrote:
> nit: add a comment to note that this is used only when LoadingWithMojo
is
> enabled

Done.

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

https://codereview.chromium.org/1970693002/diff/880001/content/browser/loader/resource_dispatcher_host_impl.cc#newcode549
content/browser/loader/resource_dispatcher_host_impl.cc:549: class
ResourceDispatcherHostImpl::MojoHelper final {

On 2016/05/26 08:43:12, kinuko wrote:
> (At this point it may not make much sense to have this helper class..)

Done.

https://codereview.chromium.org/1970693002/diff/880001/content/browser/loader/url_loader_factory_impl.cc
File content/browser/loader/url_loader_factory_impl.cc (right):

https://codereview.chromium.org/1970693002/diff/880001/content/browser/loader/url_loader_factory_impl.cc#newcode30
content/browser/loader/url_loader_factory_impl.cc:30:
ResourceDispatcherHostImpl::Get()->AddUninitiatedURLLoader(

On 2016/05/25 16:17:20, mmenke wrote:
> "Uninitiated" seems like a more ambiguous term for "NotStarted" /
"Unstarted".
> Could we rename this?

Done.

https://codereview.chromium.org/1970693002/diff/880001/content/browser/loader/url_loader_factory_impl.cc#newcode32
content/browser/loader/url_loader_factory_impl.cc:32:
base::WrapUnique(new URLLoaderImpl(filter.get(), std::move(request))));

On 2016/05/25 16:17:20, mmenke wrote:
> This ownership model seems really weird and non-obvious. "Create"
creates a
> URLLoader, and gives it to the RDH. And then calling Load on a
URLLoader (Not
> quite sure how someone finds it to call load on in the first place...)
makes the
> object take ownership of itself back...Just to pass ownership *back*
to the RDH.
>

URLLoaderImpl uses mojo::Binding[1]. mojo::Binding doesn't take target's
ownership, as written in the class comment. mojom::URLLoader functions
are called via mojo::Binding, I think.

> Can we do something better here? I guess the mojo code doesn't expect
to own
> the mojo communications objects itself?

I think you are talking about mojo::StrongBinding[2], but I wanted to
use Binding because I wanted to unbind the loader when the associated
ResourceLoader is destructed and I was not sure I could do that with
StringBinding.

1:
https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/cpp/bindings/binding.h
2:
https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/cpp/bindings/strong_binding.h

https://codereview.chromium.org/1970693002/diff/880001/content/common/url_loader.mojom
File content/common/url_loader.mojom (right):

https://codereview.chromium.org/1970693002/diff/880001/content/common/url_loader.mojom#newcode17
content/common/url_loader.mojom:17: // Loads the given |request|,
asynchronously producing |response|. Consult

On 2016/05/25 16:17:20, mmenke wrote:
> |response|? I don't see anything with that name here.

Ah, sorry, I forgot to update the description when I updated the
signature.

https://codereview.chromium.org/1970693002/diff/880001/content/common/url_loader.mojom#newcode20
content/common/url_loader.mojom:20: // |request_id| is for compatibility
with the existing ChromeIPC.

On 2016/05/25 16:17:20, mmenke wrote:
> I don't think ChromeIPC is one word?

Done.

https://codereview.chromium.org/1970693002/diff/880001/content/common/url_loader.mojom#newcode21
content/common/url_loader.mojom:21: Load(int32 request_id,

On 2016/05/25 16:17:20, mmenke wrote:
> Is there a reason this is "Load" and not "Start". Think it's best to
use
> consistent terms across wrapped APIs, unless there's a good reason not
to.
> Also, "Load" seems to have more of a "Do this <entire> thing and get
back to me"
> implication.

Done.

https://codereview.chromium.org/1970693002/diff/880001/content/common/url_loader.mojom#newcode22
content/common/url_loader.mojom:22: URLRequest request,

On 2016/05/25 16:17:20, mmenke wrote:
> Hrm...I'm not sure the world can handle yet another class named
URLRequest. Not
> that I have a better suggestion.

I'm not particularly fond of names in this file but I don't have better
idea either.

https://codereview.chromium.org/1970693002/diff/880001/content/common/url_loader.mojom#newcode30
content/common/url_loader.mojom:30: // Cancels the request.

On 2016/05/25 16:17:20, mmenke wrote:
> Can we still get data after calling Cancel, or does it instantly cut
off the
> data stream? I'm assuming the former?

I added some comments. Is it clear now?

https://codereview.chromium.org/1970693002/

[mme...@chromium.org]

Here's another round of comments. Want to add some more this afternoon, but
publishing now in case I forget.


https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/url_loader_factory_impl_unittest.cc
File content/browser/loader/url_loader_factory_impl_unittest.cc (right):

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/url_loader_factory_impl_unittest.cc#newcode30
content/browser/loader/url_loader_factory_impl_unittest.cc:30: class
URLLoaderClientImpl final : public mojom::URLLoaderClient {
I'd suggest giving this at least a marginally more distinct name, like
TestURLLoaderClient.

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/url_loader_factory_impl_unittest.cc#newcode64
content/browser/loader/url_loader_factory_impl_unittest.cc:64: bool
has_received_completion_ = false;
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/url_loader_factory_impl_unittest.cc#newcode75
content/browser/loader/url_loader_factory_impl_unittest.cc:75: void
SetCallback(Callback callback) { callback_ = callback; }
nit: const Callback&

Note that I made two suggestions about alternatives to this class
further down, so may want to read through all of my comments before
responding to my ones up here.

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/url_loader_factory_impl_unittest.cc#newcode112
content/browser/loader/url_loader_factory_impl_unittest.cc:112: }
Suggest just checking if the callback_ is_null (is_empty?) in
MaybeCreateJobWithProtocolHandler instead of using this. Think it makes
the method clearer. And it saves a few lines of code!

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/url_loader_factory_impl_unittest.cc#newcode137
content/browser/loader/url_loader_factory_impl_unittest.cc:137:
request_context->set_job_factory(job_factory_.get());
Modifying URLRequestContexts you didn't just create yourself is
generally not a good idea. Other classes internal to the context may
have pointers to the old NetworkDelegate, for instance.

Suggest just using URLRequestFilter.

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/url_loader_factory_impl_unittest.cc#newcode138
content/browser/loader/url_loader_factory_impl_unittest.cc:138:
request_context->set_network_delegate(&network_delegate_);
Why do you need to change the NetworkDelegate?

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/url_loader_factory_impl_unittest.cc#newcode163
content/browser/loader/url_loader_factory_impl_unittest.cc:163:
url_request_job_factory_method_ = request->method();
Any reason to not just record these on every request in the Factory, and
call this method "HangingURLRequest" - it's the hanging that seems to
make the unique.

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/url_loader_factory_impl_unittest.cc#newcode163
content/browser/loader/url_loader_factory_impl_unittest.cc:163:
url_request_job_factory_method_ = request->method();
Also, perhaps count URLRequests, to see if we see any unexpected ones?

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/url_loader_factory_impl_unittest.cc#newcode173
content/browser/loader/url_loader_factory_impl_unittest.cc:173: "hello",
true);
I'd recommend against using URLRequestTestJob - the process one pending
message thing is weird and bad - I'd really like to get rid of the
class, actually. Instead, I'd just make a file on disk (Or use an
already existing one) and use URLRequestMockHTTPJob.

You could even use the EmbeddedTestServer, if you want. You could even
give it callbacks instead of the URLRequestJobFactory, or the
URLRequestFilter (Which I suggested above).

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/url_loader_factory_impl_unittest.cc#newcode238
content/browser/loader/url_loader_factory_impl_unittest.cc:238:
base::RunLoop().RunUntilIdle();
All this spinning of the RunLoop is a bit of an anti-pattern. Better to
use a RunLoop, and call a quit closure when you get the completion
message.

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/url_loader_factory_impl_unittest.cc#newcode248
content/browser/loader/url_loader_factory_impl_unittest.cc:248:
EXPECT_TRUE(client.response_body().is_valid());
Shouldn't we actually read the response body in one of these?

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/url_loader_factory_impl_unittest.cc#newcode252
content/browser/loader/url_loader_factory_impl_unittest.cc:252:
TEST_F(URLLoaderFactoryImplTest, GetFailedResponse) {
Other test suggestions:
* One that goes through AbortRequestBeforeItStarts (We should have tests
for all those paths, but I assume we have tests for them at another
level, that we'll get when switching to mojo completely here).
* One that runs into an error only after reading the headers (Know one
of the mocks in net can do this).
* One that runs into a failure only after reading some of the body (Not
sure if we have a mock that can do this, though I'd guess we do).

https://codereview.chromium.org/1970693002/

[mme...@chromium.org]

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/mojo_async_resource_handler.cc
File content/browser/loader/mojo_async_resource_handler.cc (right):

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/mojo_async_resource_handler.cc#newcode99
content/browser/loader/mojo_async_resource_handler.cc:99: }
Seems like we shouldn't include this code until OnDataDownloaded is
hooked up - half-hooking something up is worth than not hooking
something up. Can just DCHECK it's empty, for now.

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/mojo_async_resource_handler.cc#newcode144
content/browser/loader/mojo_async_resource_handler.cc:144: return false;
Returning false results in a cancel, as does calling CancelWithError.
While URLRequestJob does prevent Bad Things from double cancel, it looks
like the only code that does has a TODO about removing it, and
ResourceLoader doesn't really expect a double cancel message, so let's
return return false, without calling CancelWithError (That will,
unfortunately, cancel the request with ERR_ABORTED instead of
ERR_FAILED, and ERR_ABORTED has magic behavior, but fixing that's beyond
the scope of this CL, and mimics the old class's behavior.

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/mojo_async_resource_handler.cc#newcode149
content/browser/loader/mojo_async_resource_handler.cc:149: //
SHOULD_WAIT should be handled in OnReadCompleted.
I'm not following this comment. Docs seem to indicate this can return
MOJO_RESULT_SHOULD_WAIT, and we should handle it here.

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/mojo_async_resource_handler.cc#newcode155
content/browser/loader/mojo_async_resource_handler.cc:155:
controller()->CancelWithError(net::ERR_FAILED);
remove this.

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/mojo_async_resource_handler.cc#newcode156
content/browser/loader/mojo_async_resource_handler.cc:156: return false;
Suggest the failure case first.

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/mojo_async_resource_handler.cc#newcode174
content/browser/loader/mojo_async_resource_handler.cc:174: (result ==
MOJO_RESULT_OK && available == 0)) {
Can the second condition occur? Should it be able to occur? If it
should be treated just like MOJO_RESULT_SHOULD_WAIT, seems like a mojo
bug.

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/mojo_async_resource_handler.cc#newcode202
content/browser/loader/mojo_async_resource_handler.cc:202:
DCHECK(status.status() != net::URLRequestStatus::IO_PENDING);
Please keep the (status.status() != net::URLRequestStatus::SUCCESS ||
sent_received_response_msg_) check - it's helped us uncover real bugs.
Fine with it being switched to a DCHECK.

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/mojo_async_resource_handler.h
File content/browser/loader/mojo_async_resource_handler.h (right):

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/mojo_async_resource_handler.h#newcode32
content/browser/loader/mojo_async_resource_handler.h:32: // when
LoadingWithMojo runtime enabled flag is enabled.
Could you add TODOs about what this does not yet supports: upload
progress, redirects, download to file, move histograms over, set zoom
level (Which doesn't really seem to belong at this layer), send cached
metadata...I think that's it?

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/url_loader_factory_impl.cc
File content/browser/loader/url_loader_factory_impl.cc (right):

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/url_loader_factory_impl.cc#newcode23
content/browser/loader/url_loader_factory_impl.cc:23: class
URLLoaderImpl final : public mojom::URLLoader {
I don't suppose there's some mojo signal if the other process is killed?
Just wondering if we can work our way towards no longer having to have
the RDH care about all that stuff (Have it not care about RenderView
tear down, too, for that matter.

binding_.set_connection_error_handler?

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/url_loader_factory_impl.cc#newcode77
content/browser/loader/url_loader_factory_impl.cc:77: binding_(this,
std::move(request)) {}
DCHECK_CURRENTLY_ON(BrowserThread::UI);?

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/url_loader_factory_impl.cc#newcode80
content/browser/loader/url_loader_factory_impl.cc:80:
DCHECK_CURRENTLY_ON(BrowserThread::UI);
include browser_thread.h

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/url_loader_factory_impl.cc#newcode87
content/browser/loader/url_loader_factory_impl.cc:87:
base::Passed(std::move(request))));
include base/bind.h

https://codereview.chromium.org/1970693002/

[yhi...@chromium.org]

I'll fix test code next week.

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/mojo_async_resource_handler.cc
File content/browser/loader/mojo_async_resource_handler.cc (right):

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/mojo_async_resource_handler.cc#newcode99
content/browser/loader/mojo_async_resource_handler.cc:99: }

On 2016/05/26 21:46:09, mmenke wrote:
> Seems like we shouldn't include this code until OnDataDownloaded is
hooked up -
> half-hooking something up is worth than not hooking something up. Can
just
> DCHECK it's empty, for now.

Done.

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/mojo_async_resource_handler.cc#newcode144
content/browser/loader/mojo_async_resource_handler.cc:144: return false;

On 2016/05/26 21:46:09, mmenke wrote:
> Returning false results in a cancel, as does calling CancelWithError.
While
> URLRequestJob does prevent Bad Things from double cancel, it looks
like the only
> code that does has a TODO about removing it, and ResourceLoader
doesn't really
> expect a double cancel message, so let's return return false, without
calling
> CancelWithError (That will, unfortunately, cancel the request with
ERR_ABORTED
> instead of ERR_FAILED, and ERR_ABORTED has magic behavior, but fixing
that's
> beyond the scope of this CL, and mimics the old class's behavior.

Done.

Does that mean CancelWithError in checkForSufficientResource (copied
from AsyncResourceHandler) is bad, too?

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/mojo_async_resource_handler.cc#newcode149
content/browser/loader/mojo_async_resource_handler.cc:149: //
SHOULD_WAIT should be handled in OnReadCompleted.

On 2016/05/26 21:46:09, mmenke wrote:
> I'm not following this comment. Docs seem to indicate this can return
> MOJO_RESULT_SHOULD_WAIT, and we should handle it here.

Generally we don't treat MOJO_RESULT_SHOULD_WAIT as a fatal error, but
we do here, because that SHOULD_WAIT should have been handled in
OnReadCompleted(between L173-L181 in this CL) and OnDefer should have
been called (i.e., this function should not be called while we are
waiting the reader for reading).

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/mojo_async_resource_handler.cc#newcode155
content/browser/loader/mojo_async_resource_handler.cc:155:
controller()->CancelWithError(net::ERR_FAILED);

On 2016/05/26 21:46:09, mmenke wrote:
> remove this.

Done.

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/mojo_async_resource_handler.cc#newcode156
content/browser/loader/mojo_async_resource_handler.cc:156: return false;

On 2016/05/26 21:46:09, mmenke wrote:
> Suggest the failure case first.

Done.

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/mojo_async_resource_handler.cc#newcode174
content/browser/loader/mojo_async_resource_handler.cc:174: (result ==
MOJO_RESULT_OK && available == 0)) {

On 2016/05/26 21:46:09, mmenke wrote:
> Can the second condition occur? Should it be able to occur? If it
should be
> treated just like MOJO_RESULT_SHOULD_WAIT, seems like a mojo bug.

The comment was not clear to me, but the actual implementation doesn't
return OK with available = 0.

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/mojo_async_resource_handler.cc#newcode202
content/browser/loader/mojo_async_resource_handler.cc:202:
DCHECK(status.status() != net::URLRequestStatus::IO_PENDING);

On 2016/05/26 21:46:09, mmenke wrote:
> Please keep the (status.status() != net::URLRequestStatus::SUCCESS ||
> sent_received_response_msg_) check - it's helped us uncover real bugs.
Fine
> with it being switched to a DCHECK.

Done.

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/mojo_async_resource_handler.h
File content/browser/loader/mojo_async_resource_handler.h (right):

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/mojo_async_resource_handler.h#newcode32
content/browser/loader/mojo_async_resource_handler.h:32: // when
LoadingWithMojo runtime enabled flag is enabled.

On 2016/05/26 21:46:09, mmenke wrote:
> Could you add TODOs about what this does not yet supports: upload
progress,
> redirects, download to file, move histograms over, set zoom level
(Which doesn't
> really seem to belong at this layer), send cached metadata...I think
that's it?

Done.

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

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/resource_dispatcher_host_impl.cc#newcode1432
content/browser/loader/resource_dispatcher_host_impl.cc:1432: if (it !=
pending_loaders_.end()) {

On 2016/05/26 15:18:49, mmenke wrote:
> This case isn't being handled.

Added a TODO comment.

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/url_loader_factory_impl.cc
File content/browser/loader/url_loader_factory_impl.cc (right):

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/url_loader_factory_impl.cc#newcode23
content/browser/loader/url_loader_factory_impl.cc:23: class
URLLoaderImpl final : public mojom::URLLoader {

On 2016/05/26 21:46:09, mmenke wrote:
> I don't suppose there's some mojo signal if the other process is
killed? Just
> wondering if we can work our way towards no longer having to have the
RDH care
> about all that stuff (Have it not care about RenderView tear down,
too, for that
> matter.
>
> binding_.set_connection_error_handler?

Can you tell me if process death is the only cause of
CancelRequestsForRoute(child_id, MSG_ROUTING_NONE)?

If that's true, we can move unstarted_laoder_ from
ResourceDispatcherHostImpl to URLLoaderImpl('s static member) by using
set_connection_error_handler.

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/url_loader_factory_impl.cc#newcode77
content/browser/loader/url_loader_factory_impl.cc:77: binding_(this,
std::move(request)) {}

On 2016/05/26 21:46:09, mmenke wrote:
> DCHECK_CURRENTLY_ON(BrowserThread::UI);?

Done.

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/url_loader_factory_impl.cc#newcode80
content/browser/loader/url_loader_factory_impl.cc:80:
DCHECK_CURRENTLY_ON(BrowserThread::UI);

On 2016/05/26 21:46:09, mmenke wrote:
> include browser_thread.h

Done.

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/url_loader_factory_impl.cc#newcode87
content/browser/loader/url_loader_factory_impl.cc:87:
base::Passed(std::move(request))));

On 2016/05/26 21:46:09, mmenke wrote:
> include base/bind.h

Done.

https://codereview.chromium.org/1970693002/diff/940001/content/common/url_loader_factory.mojom
File content/common/url_loader_factory.mojom (right):

https://codereview.chromium.org/1970693002/diff/940001/content/common/url_loader_factory.mojom#newcode10
content/common/url_loader_factory.mojom:10: CreateURLLoader(URLLoader&
loader);

On 2016/05/26 15:18:49, mmenke wrote:
> I don't suppose we can combine with with Start? i.e. take a
request_id,
> URLRequest, and URLLoaderClient? I'm not set on a combined
create+start being
> the ideal API, but it makes ownership semantecs for the URLLoader
sane. If,
> once the RDH is using a purely mojo API, you want to change how things
work, and
> have a reasonably ownership model to go with it, I'm certainly fine
with
> changing it.

I think it is at least non-trivial work. content::URLLoaderFactoryImpl
is bound to UI thread, and if we pass a client, it will be bound to that
thread, too.

https://codereview.chromium.org/1970693002/

[mme...@chromium.org]

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/mojo_async_resource_handler.cc
File content/browser/loader/mojo_async_resource_handler.cc (right):

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/mojo_async_resource_handler.cc#newcode144
content/browser/loader/mojo_async_resource_handler.cc:144: return false;

On 2016/05/27 11:30:34, yhirano wrote:
> On 2016/05/26 21:46:09, mmenke wrote:
> > Returning false results in a cancel, as does calling
CancelWithError. While
> > URLRequestJob does prevent Bad Things from double cancel, it looks
like the
> only
> > code that does has a TODO about removing it, and ResourceLoader
doesn't really
> > expect a double cancel message, so let's return return false,
without calling
> > CancelWithError (That will, unfortunately, cancel the request with
ERR_ABORTED
> > instead of ERR_FAILED, and ERR_ABORTED has magic behavior, but
fixing that's
> > beyond the scope of this CL, and mimics the old class's behavior.
> Done.
>
> Does that mean CancelWithError in checkForSufficientResource (copied
from
> AsyncResourceHandler) is bad, too?

Yes. It may make sense to return a network error code, instead of
true/false...but well beyond the scope of this CL.

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/mojo_async_resource_handler.cc#newcode149
content/browser/loader/mojo_async_resource_handler.cc:149: //
SHOULD_WAIT should be handled in OnReadCompleted.

On 2016/05/27 11:30:34, yhirano wrote:
> On 2016/05/26 21:46:09, mmenke wrote:
> > I'm not following this comment. Docs seem to indicate this can
return
> > MOJO_RESULT_SHOULD_WAIT, and we should handle it here.
>
> Generally we don't treat MOJO_RESULT_SHOULD_WAIT as a fatal error, but
we do
> here, because that SHOULD_WAIT should have been handled in
> OnReadCompleted(between L173-L181 in this CL) and OnDefer should have
been
> called (i.e., this function should not be called while we are waiting
the reader
> for reading).

On 2016/05/27 11:30:34, yhirano wrote:
> On 2016/05/26 21:46:09, mmenke wrote:
> > I'm not following this comment. Docs seem to indicate this can
return
> > MOJO_RESULT_SHOULD_WAIT, and we should handle it here.
>
> Generally we don't treat MOJO_RESULT_SHOULD_WAIT as a fatal error, but
we do
> here, because that SHOULD_WAIT should have been handled in
> OnReadCompleted(between L173-L181 in this CL) and OnDefer should have
been
> called (i.e., this function should not be called while we are waiting
the reader
> for reading).

I'm not following. The docs say nothing about when BeginWriteDataRaw
returns MOJO_RESULT_SHOULD_WAIT. If this is the first
mojo::BeginWriteDataRaw, we may not have even called OnReadCompleted
yet.

I also don't see any guarantee that if one call to BeginWriteDataRaw
succeeds, and then we call EndWriteDataRaw(..., 0), that the next call
to BeginWriteDataRaw is guaranteed not to return
MOJO_RESULT_SHOULD_WAIT.

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/url_loader_factory_impl.cc
File content/browser/loader/url_loader_factory_impl.cc (right):

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/url_loader_factory_impl.cc#newcode23
content/browser/loader/url_loader_factory_impl.cc:23: class
URLLoaderImpl final : public mojom::URLLoader {

On 2016/05/27 11:30:34, yhirano wrote:
> On 2016/05/26 21:46:09, mmenke wrote:
> > I don't suppose there's some mojo signal if the other process is
killed? Just
> > wondering if we can work our way towards no longer having to have
the RDH care
> > about all that stuff (Have it not care about RenderView tear down,
too, for
> that
> > matter.
> >
> > binding_.set_connection_error_handler?
>
> Can you tell me if process death is the only cause of
> CancelRequestsForRoute(child_id, MSG_ROUTING_NONE)?
>
> If that's true, we can move unstarted_laoder_ from
ResourceDispatcherHostImpl to
> URLLoaderImpl('s static member) by using set_connection_error_handler.

CancelRequestsForProcess is only called on process deletion.
CancelRequestsForRoute is called whenever a RenderFrame goes away.
Suppose we may get races if we delay these to wait on mojo calls - seems
like this would require more investigation.

https://codereview.chromium.org/1970693002/diff/940001/content/common/url_loader_factory.mojom
File content/common/url_loader_factory.mojom (right):

https://codereview.chromium.org/1970693002/diff/940001/content/common/url_loader_factory.mojom#newcode10
content/common/url_loader_factory.mojom:10: CreateURLLoader(URLLoader&
loader);

On 2016/05/27 11:30:34, yhirano wrote:
> On 2016/05/26 15:18:49, mmenke wrote:
> > I don't suppose we can combine with with Start? i.e. take a
request_id,
> > URLRequest, and URLLoaderClient? I'm not set on a combined
create+start being
> > the ideal API, but it makes ownership semantecs for the URLLoader
sane. If,
> > once the RDH is using a purely mojo API, you want to change how
things work,
> and
> > have a reasonably ownership model to go with it, I'm certainly fine
with
> > changing it.
>
> I think it is at least non-trivial work. content::URLLoaderFactoryImpl
is bound
> to UI thread, and if we pass a client, it will be bound to that
thread, too.

Can we move URLLoaderFactoryImpl over to the UI thread? Make it owned
by the RDH? (Or make the RDH itself the factory?)

https://codereview.chromium.org/1970693002/

[mme...@chromium.org]

https://codereview.chromium.org/1970693002/diff/940001/content/common/url_loader_factory.mojom
File content/common/url_loader_factory.mojom (right):

https://codereview.chromium.org/1970693002/diff/940001/content/common/url_loader_factory.mojom#newcode10
content/common/url_loader_factory.mojom:10: CreateURLLoader(URLLoader&
loader);

On 2016/05/27 15:39:26, mmenke wrote:
> On 2016/05/27 11:30:34, yhirano wrote:
> > On 2016/05/26 15:18:49, mmenke wrote:
> > > I don't suppose we can combine with with Start? i.e. take a
request_id,
> > > URLRequest, and URLLoaderClient? I'm not set on a combined
create+start
> being
> > > the ideal API, but it makes ownership semantecs for the URLLoader
sane. If,
> > > once the RDH is using a purely mojo API, you want to change how
things work,
> > and
> > > have a reasonably ownership model to go with it, I'm certainly
fine with
> > > changing it.
> >
> > I think it is at least non-trivial work.
content::URLLoaderFactoryImpl is
> bound
> > to UI thread, and if we pass a client, it will be bound to that
thread, too.
>
> Can we move URLLoaderFactoryImpl over to the UI thread? Make it owned
by the
> RDH? (Or make the RDH itself the factory?)

OVer to the IO thread, rather.

https://codereview.chromium.org/1970693002/

[yhi...@chromium.org]

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/mojo_async_resource_handler.cc
File content/browser/loader/mojo_async_resource_handler.cc (right):

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/mojo_async_resource_handler.cc#newcode149
content/browser/loader/mojo_async_resource_handler.cc:149: //
SHOULD_WAIT should be handled in OnReadCompleted.

On 2016/05/27 15:39:26, mmenke wrote:
> On 2016/05/27 11:30:34, yhirano wrote:

> > On 2016/05/26 21:46:09, mmenke wrote:
> > > I'm not following this comment. Docs seem to indicate this can
return
> > > MOJO_RESULT_SHOULD_WAIT, and we should handle it here.
> >
> > Generally we don't treat MOJO_RESULT_SHOULD_WAIT as a fatal error,
but we do
> > here, because that SHOULD_WAIT should have been handled in
> > OnReadCompleted(between L173-L181 in this CL) and OnDefer should
have been
> > called (i.e., this function should not be called while we are
waiting the
> reader
> > for reading).
>

> On 2016/05/27 11:30:34, yhirano wrote:

> > On 2016/05/26 21:46:09, mmenke wrote:
> > > I'm not following this comment. Docs seem to indicate this can
return
> > > MOJO_RESULT_SHOULD_WAIT, and we should handle it here.
> >
> > Generally we don't treat MOJO_RESULT_SHOULD_WAIT as a fatal error,
but we do
> > here, because that SHOULD_WAIT should have been handled in
> > OnReadCompleted(between L173-L181 in this CL) and OnDefer should
have been
> > called (i.e., this function should not be called while we are
waiting the
> reader
> > for reading).
>
> I'm not following. The docs say nothing about when BeginWriteDataRaw
returns
> MOJO_RESULT_SHOULD_WAIT. If this is the first
mojo::BeginWriteDataRaw, we may
> not have even called OnReadCompleted yet.
>
> I also don't see any guarantee that if one call to BeginWriteDataRaw
succeeds,
> and then we call EndWriteDataRaw(..., 0), that the next call to
> BeginWriteDataRaw is guaranteed not to return MOJO_RESULT_SHOULD_WAIT.

I expected so:
- After creating a data pipe, the first BeginWriteData will
not return SHOULD_WAIT and |*buffer_num_bytes| will be set
to the capacity if it returns OK.
- When we call EndWriteData with |num_bytes_written| just after
calling successful BeginWriteData (i.e., it returned OK), the
next BeginWriteData will not return SHOULD_WAIT.
unless there is an error. Of course, if there is an error, we can
return with that error as the current code does.

But strictly speaking you're right, the comments say nothing about it.
yzshen1@: can you tell me if it's guaranteed or not?

https://codereview.chromium.org/1970693002/

[yzs...@chromium.org]

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/mojo_async_resource_handler.cc
File content/browser/loader/mojo_async_resource_handler.cc (right):

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/mojo_async_resource_handler.cc#newcode149
content/browser/loader/mojo_async_resource_handler.cc:149: //
SHOULD_WAIT should be handled in OnReadCompleted.

Generally speaking, I think we should stick to what the API
documentation says, instead of the unspecified behavior of the
implementation.

> I expected so:
> - After creating a data pipe, the first BeginWriteData will
> not return SHOULD_WAIT and |*buffer_num_bytes| will be set
> to the capacity if it returns OK.

I think this is true. (But please see my comment above)

> - When we call EndWriteData with |num_bytes_written| just after
> calling successful BeginWriteData (i.e., it returned OK), the
> next BeginWriteData will not return SHOULD_WAIT.

Not necessarily, it is possible that the other side hasn't consumed data
to release the buffer.

[yhi...@chromium.org]

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/mojo_async_resource_handler.cc
File content/browser/loader/mojo_async_resource_handler.cc (right):

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/mojo_async_resource_handler.cc#newcode149
content/browser/loader/mojo_async_resource_handler.cc:149: //
SHOULD_WAIT should be handled in OnReadCompleted.

Thank you. Let me address this issue in a separate CL because I need to
update ResourceHandler's design so that we can set *buf_size to 0.

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/url_loader_factory_impl_unittest.cc
File content/browser/loader/url_loader_factory_impl_unittest.cc (right):

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/url_loader_factory_impl_unittest.cc#newcode30
content/browser/loader/url_loader_factory_impl_unittest.cc:30: class
URLLoaderClientImpl final : public mojom::URLLoaderClient {

On 2016/05/26 16:28:29, mmenke (OOO May 30-31) wrote:
> I'd suggest giving this at least a marginally more distinct name, like
> TestURLLoaderClient.

Done.

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/url_loader_factory_impl_unittest.cc#newcode64
content/browser/loader/url_loader_factory_impl_unittest.cc:64: bool
has_received_completion_ = false;

On 2016/05/26 16:28:29, mmenke (OOO May 30-31) wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/url_loader_factory_impl_unittest.cc#newcode75
content/browser/loader/url_loader_factory_impl_unittest.cc:75: void
SetCallback(Callback callback) { callback_ = callback; }

On 2016/05/26 16:28:29, mmenke (OOO May 30-31) wrote:
> nit: const Callback&
>
> Note that I made two suggestions about alternatives to this class
further down,
> so may want to read through all of my comments before responding to my
ones up
> here.

Deleted

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/url_loader_factory_impl_unittest.cc#newcode112
content/browser/loader/url_loader_factory_impl_unittest.cc:112: }

On 2016/05/26 16:28:29, mmenke (OOO May 30-31) wrote:
> Suggest just checking if the callback_ is_null (is_empty?) in
> MaybeCreateJobWithProtocolHandler instead of using this. Think it
makes the
> method clearer. And it saves a few lines of code!

Deleted

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/url_loader_factory_impl_unittest.cc#newcode137
content/browser/loader/url_loader_factory_impl_unittest.cc:137:
request_context->set_job_factory(job_factory_.get());

On 2016/05/26 16:28:29, mmenke (OOO May 30-31) wrote:
> Modifying URLRequestContexts you didn't just create yourself is
generally not a
> good idea. Other classes internal to the context may have pointers to
the old
> NetworkDelegate, for instance.
>
> Suggest just using URLRequestFilter.

Done.

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/url_loader_factory_impl_unittest.cc#newcode138
content/browser/loader/url_loader_factory_impl_unittest.cc:138:
request_context->set_network_delegate(&network_delegate_);

On 2016/05/26 16:28:29, mmenke (OOO May 30-31) wrote:
> Why do you need to change the NetworkDelegate?

Deleted

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/url_loader_factory_impl_unittest.cc#newcode163
content/browser/loader/url_loader_factory_impl_unittest.cc:163:
url_request_job_factory_method_ = request->method();

On 2016/05/26 16:28:29, mmenke (OOO May 30-31) wrote:
> Also, perhaps count URLRequests, to see if we see any unexpected ones?

Deleted. Updating variables in const functions is complicated and I
don't think it's worth doing any more.

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/url_loader_factory_impl_unittest.cc#newcode173
content/browser/loader/url_loader_factory_impl_unittest.cc:173: "hello",
true);

On 2016/05/26 16:28:29, mmenke (OOO May 30-31) wrote:
> I'd recommend against using URLRequestTestJob - the process one
pending message
> thing is weird and bad - I'd really like to get rid of the class,
actually.
> Instead, I'd just make a file on disk (Or use an already existing one)
and use
> URLRequestMockHTTPJob.
>
> You could even use the EmbeddedTestServer, if you want. You could
even give it
> callbacks instead of the URLRequestJobFactory, or the URLRequestFilter
(Which I
> suggested above).

Done.

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/url_loader_factory_impl_unittest.cc#newcode238
content/browser/loader/url_loader_factory_impl_unittest.cc:238:
base::RunLoop().RunUntilIdle();

On 2016/05/26 16:28:29, mmenke (OOO May 30-31) wrote:
> All this spinning of the RunLoop is a bit of an anti-pattern. Better
to use a
> RunLoop, and call a quit closure when you get the completion message.

Done.

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/url_loader_factory_impl_unittest.cc#newcode248
content/browser/loader/url_loader_factory_impl_unittest.cc:248:
EXPECT_TRUE(client.response_body().is_valid());

On 2016/05/26 16:28:29, mmenke (OOO May 30-31) wrote:
> Shouldn't we actually read the response body in one of these?

Done.

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/url_loader_factory_impl_unittest.cc#newcode252
content/browser/loader/url_loader_factory_impl_unittest.cc:252:
TEST_F(URLLoaderFactoryImplTest, GetFailedResponse) {

On 2016/05/26 16:28:29, mmenke (OOO May 30-31) wrote:
> Other test suggestions:
> * One that goes through AbortRequestBeforeItStarts (We should have
tests for all
> those paths, but I assume we have tests for them at another level,
that we'll
> get when switching to mojo completely here).
> * One that runs into an error only after reading the headers (Know one
of the
> mocks in net can do this).
> * One that runs into a failure only after reading some of the body
(Not sure if
> we have a mock that can do this, though I'd guess we do).

Done.

https://codereview.chromium.org/1970693002/

[yhi...@chromium.org]

memo:
Unresolved 1:
https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/url_loader_factory_impl.cc#newcode23
Unresolved 2:
https://codereview.chromium.org/1970693002/diff/940001/content/common/url_loader_factory.mojom#newcode10

https://codereview.chromium.org/1970693002/

[mme...@chromium.org]

On 2016/06/01 14:45:18, yhirano wrote:
> memo:
> Unresolved 1:
>
https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader/url_loader_factory_impl.cc#newcode23
> Unresolved 2:
>
https://codereview.chromium.org/1970693002/diff/940001/content/common/url_loader_factory.mojom#newcode10

Unresolved 1 I'm certainly fine with punting on this CL (And think we should,
actually).

Unresolved 2 results in really weird ownership semantics, that I'd like to see
resolved before we land this.

I'm also concerned about the assumptions this makes about when
mojo::BeginWriteDataRaw may / may not return "MOJO_RESULT_SHOULD_WAIT", and
think that should also be resolved before we land this.

https://codereview.chromium.org/1970693002/

[mme...@chromium.org]

I'm not sure I'll have time for a full pass today (Took yesterday off, Monday
was a holiday, so I have a lot on my plate today, and given that this has
unresolved issues, I don't feel like I'm blocking it).

https://codereview.chromium.org/1970693002/

[yhi...@chromium.org]

> I'm also concerned about the assumptions this makes about when
> mojo::BeginWriteDataRaw may / may not return "MOJO_RESULT_SHOULD_WAIT", and
> think that should also be resolved before we land this.

I'm writing a ResourceHandler refactoring CL. Hopefully that will help me
address the issue in this CL.

https://codereview.chromium.org/1970693002/

[yhi...@chromium.org]

https://codereview.chromium.org/1970693002/diff/1120001/content/browser/loader/mojo_async_resource_handler.cc
File content/browser/loader/mojo_async_resource_handler.cc (right):

https://codereview.chromium.org/1970693002/diff/1120001/content/browser/loader/mojo_async_resource_handler.cc#newcode138
content/browser/loader/mojo_async_resource_handler.cc:138: MojoResult
result = mojo::BeginWriteDataRaw(
I've just noticed that this breaks OnWillRead's postcondition, "the
output buffer should be able to outlive the handler". I will fix it.

https://codereview.chromium.org/1970693002/

[yhi...@chromium.org]

> I'm also concerned about the assumptions this makes about when
> mojo::BeginWriteDataRaw may / may not return
> "MOJO_RESULT_SHOULD_WAIT", and
> think that should also be resolved before we land this.

Done. I changed my mind and it's done without changing ResourceHandler.

https://codereview.chromium.org/1970693002/diff/1120001/content/browser/loader/mojo_async_resource_handler.cc
File content/browser/loader/mojo_async_resource_handler.cc (right):

https://codereview.chromium.org/1970693002/diff/1120001/content/browser/loader/mojo_async_resource_handler.cc#newcode138
content/browser/loader/mojo_async_resource_handler.cc:138: MojoResult
result = mojo::BeginWriteDataRaw(

On 2016/06/03 11:27:44, yhirano wrote:
> I've just noticed that this breaks OnWillRead's postcondition, "the
output
> buffer should be able to outlive the handler". I will fix it.

Done.

https://codereview.chromium.org/1970693002/

[yhi...@chromium.org]

https://codereview.chromium.org/1970693002/diff/940001/content/common/url_loader_factory.mojom
File content/common/url_loader_factory.mojom (right):

https://codereview.chromium.org/1970693002/diff/940001/content/common/url_loader_factory.mojom#newcode10
content/common/url_loader_factory.mojom:10: CreateURLLoader(URLLoader&
loader);

On 2016/05/27 15:41:04, mmenke wrote:
> On 2016/05/27 15:39:26, mmenke wrote:
> > On 2016/05/27 11:30:34, yhirano wrote:

> > > On 2016/05/26 15:18:49, mmenke wrote:
> > > > I don't suppose we can combine with with Start? i.e. take a
request_id,
> > > > URLRequest, and URLLoaderClient? I'm not set on a combined
create+start
> > being
> > > > the ideal API, but it makes ownership semantecs for the
URLLoader sane.
> If,
> > > > once the RDH is using a purely mojo API, you want to change how
things
> work,
> > > and

> > > > have a reasonably ownership model to go with it, I'm certainly
fine with

> > > > changing it.
> > >
> > > I think it is at least non-trivial work.
content::URLLoaderFactoryImpl is
> > bound
> > > to UI thread, and if we pass a client, it will be bound to that
thread, too.
> >
> > Can we move URLLoaderFactoryImpl over to the UI thread? Make it
owned by the
> > RDH? (Or make the RDH itself the factory?)
>
> OVer to the IO thread, rather.

Done.

https://codereview.chromium.org/1970693002/

[yhi...@chromium.org]

ping

https://codereview.chromium.org/1970693002/

[mme...@chromium.org]

On 2016/06/13 08:34:42, yhirano wrote:
> ping

Sorry, hadn't realized this was ready for another round of review. I'll try to
get to it today.

https://codereview.chromium.org/1970693002/

[mme...@chromium.org]

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loader/mojo_async_resource_handler.cc
File content/browser/loader/mojo_async_resource_handler.cc (right):

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loader/mojo_async_resource_handler.cc#newcode57
content/browser/loader/mojo_async_resource_handler.cc:57: class
MojoAsyncResourceHandler::SharedWriter final
This class needs to be documented.

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loader/mojo_async_resource_handler.cc#newcode73
content/browser/loader/mojo_async_resource_handler.cc:73: class
MojoAsyncResourceHandler::WriterIOBuffer final
This class needs to be documented.

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loader/mojo_async_resource_handler.cc#newcode167
content/browser/loader/mojo_async_resource_handler.cc:167: if (filter) {
All these filter checks, except possibly the one in
MojoAsyncResourceHandler::OnResponseStarted, don't seem to get us
anything, when we're not actually sending anything via the filter.

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loader/mojo_async_resource_handler.cc#newcode326
content/browser/loader/mojo_async_resource_handler.cc:326: if
(buffer_bytes_read_ == 0) {
I think it's clearer (And less code) if you move this to the start of
this method, make it return true, and remove the other
"buffer_bytes_read_ == 0" line. Then you can unconditionally call
AllocateBuffer in MojoAsyncResourceHandler::ResumeIfDeferred.

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loader/mojo_async_resource_handler.cc#newcode333
content/browser/loader/mojo_async_resource_handler.cc:333: return false;
There's no break in the loop. These two lines shouldn't be needed.

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loader/mojo_async_resource_handler.cc#newcode352
content/browser/loader/mojo_async_resource_handler.cc:352: if
(did_defer_) {
Can this ever be called if did_defer_ is false?

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loader/url_loader_factory_holder.cc
File content/browser/loader/url_loader_factory_holder.cc (right):

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loader/url_loader_factory_holder.cc#newcode32
content/browser/loader/url_loader_factory_holder.cc:32: void Cancel()
override {}
NOTREACHED() for both of these, and a TODO about hooking them up? Hrm,
with this architecture, how can they reasonably be hooked up? Should
the MojoAsyncResourceHandler actually be the URLLoader? Not quite sure
what that would look like, in terms of the wiring.

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loader/url_loader_factory_holder.cc#newcode88
content/browser/loader/url_loader_factory_holder.cc:88: : factory_(new
URLLoaderFactoryImpl(resource_message_filter)) {
Why is this cross-thread dongle necessary? Can we just create the
URLLoaderFactory in ResourceDispatcherHostImpl::OnInit, and destroy it
in ResourceDispatcherHostImpl::OnShutdown? That ties its lifecycle
directly to the class it actually depends on.

Alternatively, could even make the RDHI implement URLLoaderFactory, and
set up/tear down the binding itself in those two methods. Think that
makes a lot more sense, in the long term, particularly if we plan to
have nothing call into the RDH directly to create requests, and instead
only provide the mojo interface.

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loader/url_loader_factory_holder_unittest.cc
File content/browser/loader/url_loader_factory_holder_unittest.cc
(right):

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loader/url_loader_factory_holder_unittest.cc#newcode59
content/browser/loader/url_loader_factory_holder_unittest.cc:59: if
(!quit_closure_.Equals(base::Closure()))
There's either an is_empty() or is_null() method that you should be
using instead, not sure which.

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loader/url_loader_factory_holder_unittest.cc#newcode65
content/browser/loader/url_loader_factory_holder_unittest.cc:65: if
(!quit_closure_.Equals(base::Closure()))
is_null()

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loader/url_loader_factory_holder_unittest.cc#newcode71
content/browser/loader/url_loader_factory_holder_unittest.cc:71: if
(!quit_closure_.Equals(base::Closure()))
is_null()

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loader/url_loader_factory_holder_unittest.cc#newcode177
content/browser/loader/url_loader_factory_holder_unittest.cc:177: class
URLLoaderFactoryImplTest : public ::testing::TestWithParam<size_t> {
You should document what the size_t is.

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loader/url_loader_factory_holder_unittest.cc#newcode240
content/browser/loader/url_loader_factory_holder_unittest.cc:240:
BrowserThread::GetBlockingPool());
Suggest adding all these mock handler setup calls to the test fixture's
constructor, instead.

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loader/url_loader_factory_holder_unittest.cc#newcode406
content/browser/loader/url_loader_factory_holder_unittest.cc:406:
::testing::Values(128, 32 * 1024));
None of these tests seem to return anything with a body longer than 128
bytes, so an allocation size of 128 bytes doesn't really help us at all.

We do want to test cases with more data, though. Maybe a test with a
512 byte response?

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loader/url_loader_factory_holder_unittest.cc#newcode408
content/browser/loader/url_loader_factory_holder_unittest.cc:408: } //
namespace
Should have tests that check each of the strings in completion_status()
when they have two different values, to make sure they're passed along
correctly.

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loader/url_loader_factory_holder_unittest.cc#newcode408
content/browser/loader/url_loader_factory_holder_unittest.cc:408: } //
namespace
Can we make BeginWriteDataRaw return sync/async/errors?

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loader/url_loader_factory_holder_unittest.cc#newcode408
content/browser/loader/url_loader_factory_holder_unittest.cc:408: } //
namespace
How about a test that fails the CheckForSufficientResource? May have to
keep on creating and starting hung requests until we get that error.

Can create jobs that hang on the first body read with:
net::URLRequestFailedJob::GetMockHttpUrlWithFailurePhase(net::URLRequestFailedJob::READ_SYNC,
net::ERR_IO_PENDING);

https://codereview.chromium.org/1970693002/

[yhi...@chromium.org]

I have some questions about test coverage: How much coverage do you want for
content/browser/loader code?

- Do you want me to check what happens if each EndWriteDataRaw() fails?
- Do you want me to check what happens if AllocateBuffer() fails in
ResumeIfDeferred()?
- Do you want me to achieve 100% branch coverage?

I can write such tests (maybe), but it will make code unclear, and keeping 100%
branch coverage is even harder when we are changing code.


https://codereview.chromium.org/1970693002/

[mme...@chromium.org]

When it comes to sync vs async returns, I'd like to try and get as much as we
reasonably can. If two cases follow basically the same path, I'm a bit less
concerned (For instance, if we make the non-mojo buffer path and the mojo-buffer
path both use the same AllocateBuffer method, we still want to test both paths,
but I don't think we need to test when allocating the buffer fails in all 3
places we try to do it (Though if we could figure out a way to reduce that to
only one or two places, that would be even better). Testing the case where mojo
returns a 0-byte buffer also seems not too useful, unless it really does that,
in practice).

For cancellation, testing all cases is also probably a bit much, though that's
often where the dragons lie.

https://codereview.chromium.org/1970693002/

[mme...@chromium.org]

Oops, focused on your last question, and ignored your others:

I don't think we need to worry about every possible EndWriteDataRaw failure (The
one in MojoAsyncResourceHandler::OnWillRead, in particular, I'm not too
concerned about... Should we test both of the other two? We should test at
least one, and once we have that in place, is it any harder to test the other?)

Can AllocateBuffer fail in ResumeIfDeferred? If so, testing it may be a good
idea.

100% branch coverage seems a bit much, but I'd like to try and cover most cases.
My experience with similar code in net/ has been if you don't cover them from
the start, the others will break eventually, and then you'll have to cover them
eventually anyways, after wasting time debugging the issue.

https://codereview.chromium.org/1970693002/

[kin...@chromium.org]

(haven't read through the new patch yet, just making a brief comment & some
questions)


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

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loader/resource_dispatcher_host_impl.h#newcode339
content/browser/loader/resource_dispatcher_host_impl.h:339: class
MojoHelper;
no longer needed

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loader/url_loader_factory_holder.cc
File content/browser/loader/url_loader_factory_holder.cc (right):

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loader/url_loader_factory_holder.cc#newcode88
content/browser/loader/url_loader_factory_holder.cc:88: : factory_(new
URLLoaderFactoryImpl(resource_message_filter)) {

On 2016/06/13 20:56:58, mmenke wrote:
> Why is this cross-thread dongle necessary? Can we just create the
> URLLoaderFactory in ResourceDispatcherHostImpl::OnInit, and destroy
it in
> ResourceDispatcherHostImpl::OnShutdown? That ties its lifecycle
directly to the
> class it actually depends on.
>
> Alternatively, could even make the RDHI implement URLLoaderFactory,
and set
> up/tear down the binding itself in those two methods. Think that
makes a lot
> more sense, in the long term, particularly if we plan to have nothing
call into
> the RDH directly to create requests, and instead only provide the mojo
> interface.

While this sounds like a better direction I feel this will need
relatively bigger refactoring that could be possibly done later..?

We'll still need some tie between RPHI (and ResourceMessageFilter) to
call existing RDHI handily with access to storage stuff etc. Creating
this holder with ResourceMessageFilter in RPHI seems to be reasonably
simple to start with to me. (Just wondering, as you must have more
solid design ideas around these bits)

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loader/url_loader_factory_holder_unittest.cc
File content/browser/loader/url_loader_factory_holder_unittest.cc
(right):

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loader/url_loader_factory_holder_unittest.cc#newcode59
content/browser/loader/url_loader_factory_holder_unittest.cc:59: if
(!quit_closure_.Equals(base::Closure()))

On 2016/06/13 20:56:58, mmenke wrote:
> There's either an is_empty() or is_null() method that you should be
using
> instead, not sure which.

is_null() is most common I believe

https://codereview.chromium.org/1970693002/diff/1240001/content/child/resource_dispatcher.cc
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/1970693002/diff/1240001/content/child/resource_dispatcher.cc#newcode593
content/child/resource_dispatcher.cc:593:
DCHECK_EQ(blink::WebURLRequest::LoadingIPCType::ChromeIPC, ipc_type);
Add TODO to use url_loader_factory otherwise?

https://codereview.chromium.org/1970693002/diff/1240001/content/common/url_loader.mojom
File content/common/url_loader.mojom (right):

https://codereview.chromium.org/1970693002/diff/1240001/content/common/url_loader.mojom#newcode17
content/common/url_loader.mojom:17: // If the request passed to |Start|
had |auto_follow_redirects| set to false,
nit: this comment looks stale

https://codereview.chromium.org/1970693002/