Issue 1970693002: Use mojo for Chrome Loading, Part 1

yhirano

Patchset #10 (id:180001) has been deleted

4 years, 7 months ago (2016-05-11 08:51:17 UTC) #1

yhirano

Description was changed from ========== Use mojo for Chrome Loading (1) BUG=603396 ========== to ========== ...

4 years, 7 months ago (2016-05-11 12:21:04 UTC) #13

yhirano

Description was changed from ========== Use mojo for Chrome Loading, Part 1 This CL introduces ...

4 years, 7 months ago (2016-05-11 12:21:21 UTC) #14

yhirano

Description was changed from ========== Use mojo for Chrome Loading, Part 1 This CL introduces ...

4 years, 7 months ago (2016-05-11 12:23:09 UTC) #16

yhirano

Description was changed from ========== Use mojo for Chrome Loading, Part 1 This CL introduces ...

4 years, 7 months ago (2016-05-11 14:21:51 UTC) #19

yhirano

yhirano@chromium.org changed reviewers: + jam@chromium.org, kinuko@chromium.org, yzshen@chromium.org

4 years, 7 months ago (2016-05-11 15:35:46 UTC) #20

dcheng

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

4 years, 7 months ago (2016-05-11 17:05:17 UTC) #22

dcheng

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 ...

4 years, 7 months ago (2016-05-11 17:05:18 UTC) #23

jam

this looks great. just minor comments. regarding testing: we should have unit tests for this ...

4 years, 7 months ago (2016-05-13 00:55:06 UTC) #24

yhirano

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): ...

4 years, 7 months ago (2016-05-16 14:40:06 UTC) #25

jam

it's fine to reuse the existing C++ struct with their IPC traits since this service ...

4 years, 7 months ago (2016-05-16 16:56:02 UTC) #26

yzshen1

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 ...

4 years, 7 months ago (2016-05-16 19:26:25 UTC) #27

yhirano

Oops, I accidentally deleted PS7. Let me respond inline. Sorry for the inconvenience. On 2016/05/16 ...

4 years, 7 months ago (2016-05-17 07:25:17 UTC) #30

yhirano

On 2016/05/16 16:56:02, jam wrote: > it's fine to reuse the existing C++ struct with ...

4 years, 7 months ago (2016-05-17 07:28:49 UTC) #31

yhirano

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, ...

4 years, 7 months ago (2016-05-17 12:38:49 UTC) #32

yhirano

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 ...

4 years, 7 months ago (2016-05-17 14:58:54 UTC) #33

yzshen1

Hi, I haven't seen anything obviously wrong with your change. It would be helpful if ...

4 years, 7 months ago (2016-05-17 17:56:30 UTC) #34

jam

On 2016/05/17 07:28:49, yhirano wrote: > On 2016/05/16 16:56:02, jam wrote: > > it's fine ...

4 years, 7 months ago (2016-05-17 22:00:21 UTC) #35

On 2016/05/17 07:28:49, yhirano wrote:
> On 2016/05/16 16:56:02, jam wrote:
> > 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_loa...
> > File content/common/url_loader.mojom (right):
> > 
> >
>
https://codereview.chromium.org/1970693002/diff/410001/content/common/url_loa...
> > 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?
> 
> 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/r...

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.

yhirano

On 2016/05/17 17:56:30, yzshen1 wrote: > Hi, > > I haven't seen anything obviously wrong ...

4 years, 7 months ago (2016-05-18 06:50:05 UTC) #36

yhirano

On 2016/05/17 22:00:21, jam wrote: > On 2016/05/17 07:28:49, yhirano wrote: > > On 2016/05/16 ...

4 years, 7 months ago (2016-05-18 08:32:04 UTC) #37

On 2016/05/17 22:00:21, jam wrote:
> On 2016/05/17 07:28:49, yhirano wrote:
> > On 2016/05/16 16:56:02, jam wrote:
> > > 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_loa...
> > > File content/common/url_loader.mojom (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1970693002/diff/410001/content/common/url_loa...
> > > 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?
> > 
> > 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/r...
> 
> 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.

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.

yzshen1

On Tue, May 17, 2016 at 11:50 PM, <yhirano@chromium.org> wrote: > On 2016/05/17 17:56:30, yzshen1 ...

4 years, 7 months ago (2016-05-18 16:10:08 UTC) #38

yzshen1

On Tue, May 17, 2016 at 11:50 PM, <yhirano@chromium.org> wrote: > On 2016/05/17 17:56:30, yzshen1 ...

4 years, 7 months ago (2016-05-18 16:10:09 UTC) #39

yhirano

> Do you mind testing the browser side using mojo::Watcher, but the renderer > side ...

4 years, 7 months ago (2016-05-18 16:14:38 UTC) #40

Ken Rockot(use gerrit already)

On 2016/05/18 at 16:10:09, yzshen wrote: > On Tue, May 17, 2016 at 11:50 PM, ...

4 years, 7 months ago (2016-05-18 16:19:55 UTC) #41

yzshen1

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

4 years, 7 months ago (2016-05-18 16:24:34 UTC) #42

On 2016/05/18 16:19:55, Ken Rockot wrote:
> On 2016/05/18 at 16:10:09, yzshen wrote:
> > On Tue, May 17, 2016 at 11:50 PM, <mailto:yhirano@chromium.org> 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 mailto:chromium-reviews+unsubscribe@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...

jam

On 2016/05/18 08:32:04, yhirano wrote: > On 2016/05/17 22:00:21, jam wrote: > > On 2016/05/17 ...

4 years, 7 months ago (2016-05-18 17:00:19 UTC) #43

On 2016/05/18 08:32:04, yhirano wrote:
> On 2016/05/17 22:00:21, jam wrote:
> > On 2016/05/17 07:28:49, yhirano wrote:
> > > On 2016/05/16 16:56:02, jam wrote:
> > > > 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_loa...
> > > > File content/common/url_loader.mojom (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1970693002/diff/410001/content/common/url_loa...
> > > > 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?
> > > 
> > > 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/r...
> > 
> > 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.
> 
> 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.

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

kinuko

(Sorry for slow response!) Generally looking good, I haven't looked into the test code & ...

4 years, 7 months ago (2016-05-20 09:38:52 UTC) #44

yhirano

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 ...

4 years, 7 months ago (2016-05-20 11:32:54 UTC) #45

kinuko

Some more comments (will take further look next week) (Gotchas around RDH still bothers me ...

4 years, 7 months ago (2016-05-20 15:56:47 UTC) #46

yhirano

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 ...

4 years, 7 months ago (2016-05-23 11:29:37 UTC) #47

mmenke

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

4 years, 7 months ago (2016-05-23 17:34:50 UTC) #48

mmenke

Some quick comments. I'm concerned about the circular dependencies this introduces, and wonder if ResourceLoader ...

4 years, 7 months ago (2016-05-23 17:34:53 UTC) #49

yzshen1

On 2016/05/18 16:24:34, yzshen1 wrote: > On 2016/05/18 16:19:55, Ken Rockot wrote: > > On ...

4 years, 7 months ago (2016-05-23 17:46:24 UTC) #50

On 2016/05/18 16:24:34, yzshen1 wrote:
> On 2016/05/18 16:19:55, Ken Rockot wrote:
> > On 2016/05/18 at 16:10:09, yzshen wrote:
> > > On Tue, May 17, 2016 at 11:50 PM, <mailto:yhirano@chromium.org> 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 mailto:chromium-reviews+unsubscribe@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...

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!

yhirano

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: ...

4 years, 7 months ago (2016-05-24 06:40:04 UTC) #51

yhirano

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 ...

4 years, 7 months ago (2016-05-24 08:03:21 UTC) #52

mmenke

mmenke@chromium.org changed reviewers: + rdsmith@chromium.org

4 years, 7 months ago (2016-05-24 15:16:31 UTC) #53

mmenke

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 ...

4 years, 7 months ago (2016-05-24 20:01:35 UTC) #55

mmenke

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 ...

4 years, 7 months ago (2016-05-24 20:06:08 UTC) #56

mmenke

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: ...

4 years, 7 months ago (2016-05-25 04:39:03 UTC) #57

kinuko

On 2016/05/25 04:39:03, mmenke wrote: > 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 > ...

4 years, 7 months ago (2016-05-25 05:11:50 UTC) #58

yhirano

On 2016/05/25 05:11:50, kinuko wrote: > On 2016/05/25 04:39:03, mmenke wrote: > > > https://codereview.chromium.org/1970693002/diff/800001/content/browser/loader/resource_dispatcher_host_impl.cc ...

4 years, 7 months ago (2016-05-25 05:23:58 UTC) #59

yhirano

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: ...

4 years, 7 months ago (2016-05-25 12:47:06 UTC) #60

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

https://codereview.chromium.org/1970693002/diff/800001/content/browser/loader...
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...
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...
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...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/1970693002/diff/800001/content/browser/loader...
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...
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.

mmenke

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

4 years, 7 months ago (2016-05-25 16:17:20 UTC) #61

kinuko

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() ? ...

4 years, 7 months ago (2016-05-26 08:43:13 UTC) #62

mmenke

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 ...

4 years, 7 months ago (2016-05-26 15:18:49 UTC) #63

yhirano

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, ...

4 years, 7 months ago (2016-05-26 15:42:45 UTC) #64

mmenke

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

4 years, 7 months ago (2016-05-26 16:28:30 UTC) #65

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...
File content/browser/loader/url_loader_factory_impl_unittest.cc (right):

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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).

mmenke

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 ...

4 years, 7 months ago (2016-05-26 21:46:09 UTC) #66

yhirano

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 ...

4 years, 6 months ago (2016-05-27 11:30:34 UTC) #67

I'll fix test code next week.

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

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader...
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...
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...
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...
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...
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...
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...
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...
File content/browser/loader/mojo_async_resource_handler.h (right):

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader...
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...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader...
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...
File content/browser/loader/url_loader_factory_impl.cc (right):

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader...
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...
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...
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...
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_loa...
File content/common/url_loader_factory.mojom (right):

https://codereview.chromium.org/1970693002/diff/940001/content/common/url_loa...
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.

mmenke

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 ...

4 years, 6 months ago (2016-05-27 15:39:26 UTC) #68

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

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader...
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...
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...
File content/browser/loader/url_loader_factory_impl.cc (right):

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader...
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_loa...
File content/common/url_loader_factory.mojom (right):

https://codereview.chromium.org/1970693002/diff/940001/content/common/url_loa...
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?)

mmenke

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 ...

4 years, 6 months ago (2016-05-27 15:41:04 UTC) #69

yhirano

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 ...

4 years, 6 months ago (2016-05-30 12:41:45 UTC) #70

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

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader...
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?

dcheng

dcheng@chromium.org changed reviewers: - dcheng@chromium.org

4 years, 6 months ago (2016-05-30 19:30:08 UTC) #71

yzshen1

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/30 ...

4 years, 6 months ago (2016-05-31 21:07:39 UTC) #72

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

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader...
content/browser/loader/mojo_async_resource_handler.cc:149: // SHOULD_WAIT should
be handled in OnReadCompleted.
On 2016/05/30 12:41:45, yhirano wrote:
> 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.
> 

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.

> 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?

yhirano

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/31 ...

4 years, 6 months ago (2016-06-01 14:40:52 UTC) #73

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

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader...
content/browser/loader/mojo_async_resource_handler.cc:149: // SHOULD_WAIT should
be handled in OnReadCompleted.
On 2016/05/31 21:07:38, yzshen1 wrote:
> On 2016/05/30 12:41:45, yhirano wrote:
> > 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.
> > 
> 
> 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.
> 
> > 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?

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...
File content/browser/loader/url_loader_factory_impl_unittest.cc (right):

https://codereview.chromium.org/1970693002/diff/940001/content/browser/loader...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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.

yhirano

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

4 years, 6 months ago (2016-06-01 14:45:18 UTC) #74

mmenke

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: ...

4 years, 6 months ago (2016-06-01 15:07:08 UTC) #75

mmenke

On 2016/06/01 15:07:08, mmenke (OOO May 30-31) wrote: > On 2016/06/01 14:45:18, yhirano wrote: > ...

4 years, 6 months ago (2016-06-01 15:08:12 UTC) #76

yhirano

> I'm also concerned about the assumptions this makes about when > mojo::BeginWriteDataRaw may / ...

4 years, 6 months ago (2016-06-02 13:15:35 UTC) #77

yhirano

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 ...

4 years, 6 months ago (2016-06-03 11:27:45 UTC) #78

yhirano

> I'm also concerned about the assumptions this makes about when > mojo::BeginWriteDataRaw may / ...

4 years, 6 months ago (2016-06-06 22:54:03 UTC) #80

yhirano

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 ...

4 years, 6 months ago (2016-06-07 08:16:57 UTC) #81

mmenke

On 2016/06/13 08:34:42, yhirano wrote: > ping Sorry, hadn't realized this was ready for another ...

4 years, 6 months ago (2016-06-13 14:50:29 UTC) #83

mmenke

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. ...

4 years, 6 months ago (2016-06-13 20:56:59 UTC) #84

yhirano

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

4 years, 6 months ago (2016-06-13 22:41:20 UTC) #85

mmenke

On 2016/06/13 22:41:20, yhirano wrote: > I have some questions about test coverage: How much ...

4 years, 6 months ago (2016-06-13 23:21:48 UTC) #86

mmenke

On 2016/06/13 23:21:48, mmenke wrote: > On 2016/06/13 22:41:20, yhirano wrote: > > I have ...

4 years, 6 months ago (2016-06-13 23:27:58 UTC) #87

On 2016/06/13 23:21:48, mmenke wrote:
> On 2016/06/13 22:41:20, yhirano wrote:
> > 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.
> 
> 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.

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.

kinuko

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

4 years, 6 months ago (2016-06-14 08:45:29 UTC) #88

yhirano

Sorry for the long delay. Regarding tests: Thank you, I understand. I think such tests ...

4 years, 5 months ago (2016-07-12 10:49:12 UTC) #105

Sorry for the long delay.

Regarding tests:

Thank you, I understand. I think such tests should be more close to the test
target, so I created mojo_async_handler_unittest.cc and moved some tests to the
file.

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

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.cc:57: class
MojoAsyncResourceHandler::SharedWriter final
On 2016/06/13 20:56:58, mmenke (Away July 2-10) wrote:
> This class needs to be documented.

Done.

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.cc:73: class
MojoAsyncResourceHandler::WriterIOBuffer final
On 2016/06/13 20:56:58, mmenke (Away July 2-10) wrote:
> This class needs to be documented.

Done.

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.cc:167: if (filter) {
On 2016/06/13 20:56:58, mmenke (Away July 2-10) wrote:
> 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.

I tried to mimic AsyncResourceHandler's behavior, but OK, let's remove them.

Regarding OnResponseStarted, I'm removing the parameter at
https://codereview.chromium.org/2136223003/ and so we can remove the branch as
well.

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.cc:326: if
(buffer_bytes_read_ == 0) {
On 2016/06/13 20:56:58, mmenke (Away July 2-10) wrote:
> 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.

Sorry I don't understand. Do you mean "the start of this loop"?

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.cc:333: return false;
On 2016/06/13 20:56:58, mmenke (Away July 2-10) wrote:
> There's no break in the loop.  These two lines shouldn't be needed.

Done.

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.cc:352: if (did_defer_) {
On 2016/06/13 20:56:58, mmenke (Away July 2-10) wrote:
> Can this ever be called if did_defer_ is false?

Currently not.

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

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loade...
content/browser/loader/resource_dispatcher_host_impl.h:339: class MojoHelper;
On 2016/06/14 08:45:29, kinuko wrote:
> no longer needed

Done.

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

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loade...
content/browser/loader/url_loader_factory_holder.cc:32: void Cancel() override
{}
On 2016/06/13 20:56:58, mmenke (Away July 2-10) wrote:
> 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.

Ah, it's a good idea. Thank you.

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loade...
content/browser/loader/url_loader_factory_holder.cc:88: : factory_(new
URLLoaderFactoryImpl(resource_message_filter)) {
On 2016/06/14 08:45:29, kinuko wrote:
> 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)

I'm planning to create a URLLoaderFactory for a RenderFrameHost, so The lifetime
of URLLoaderFactory doesn't equal to RDHI.

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

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loade...
content/browser/loader/url_loader_factory_holder_unittest.cc:59: if
(!quit_closure_.Equals(base::Closure()))
On 2016/06/14 08:45:29, kinuko wrote:
> 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

operator bool() is simpler, I think.

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loade...
content/browser/loader/url_loader_factory_holder_unittest.cc:65: if
(!quit_closure_.Equals(base::Closure()))
On 2016/06/13 20:56:58, mmenke (Away July 2-10) wrote:
> is_null()

Done.

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loade...
content/browser/loader/url_loader_factory_holder_unittest.cc:71: if
(!quit_closure_.Equals(base::Closure()))
On 2016/06/13 20:56:58, mmenke (Away July 2-10) wrote:
> is_null()

Done.

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loade...
content/browser/loader/url_loader_factory_holder_unittest.cc:177: class
URLLoaderFactoryImplTest : public ::testing::TestWithParam<size_t> {
On 2016/06/13 20:56:59, mmenke (Away July 2-10) wrote:
> You should document what the size_t is.

Done.

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loade...
content/browser/loader/url_loader_factory_holder_unittest.cc:240:
BrowserThread::GetBlockingPool());
On 2016/06/13 20:56:59, mmenke (Away July 2-10) wrote:
> Suggest adding all these mock handler setup calls to the test fixture's
> constructor, instead.

I'd prefer writing case-specific preparation in the test case unless it's
reusable.

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loade...
content/browser/loader/url_loader_factory_holder_unittest.cc:406:
::testing::Values(128, 32 * 1024));
On 2016/06/13 20:56:58, mmenke (Away July 2-10) wrote:
> 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?

The parameter is not for such cases. The mime sniffer requires us to allocate at
least 2048 bytes buffer, and 128 is for cases where mojo allocates a smaller
buffer.

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loade...
content/browser/loader/url_loader_factory_holder_unittest.cc:408: }  //
namespace
On 2016/06/13 20:56:59, mmenke (Away July 2-10) wrote:
> 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.

These are tested in mojo_async_resource_handler_unittest.cc.

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

https://codereview.chromium.org/1970693002/diff/1240001/content/child/resourc...
content/child/resource_dispatcher.cc:593:
DCHECK_EQ(blink::WebURLRequest::LoadingIPCType::ChromeIPC, ipc_type);
On 2016/06/14 08:45:29, kinuko wrote:
> Add TODO to use url_loader_factory otherwise?

Done.

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

https://codereview.chromium.org/1970693002/diff/1240001/content/common/url_lo...
content/common/url_loader.mojom:17: // If the request passed to |Start| had
|auto_follow_redirects| set to false,
On 2016/06/14 08:45:29, kinuko wrote:
> nit: this comment looks stale

Done.

mmenke

I expect to get to another full pass this week, but probably won't be today ...

4 years, 5 months ago (2016-07-12 15:10:45 UTC) #106

I expect to get to another full pass this week, but probably won't be today or
tomorrow.

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

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.cc:167: if (filter) {
On 2016/07/12 10:49:11, yhirano wrote:
> On 2016/06/13 20:56:58, mmenke (Away July 2-10) wrote:
> > 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.
> 
> I tried to mimic AsyncResourceHandler's behavior, but OK, let's remove them.
> 
> Regarding OnResponseStarted, I'm removing the parameter at
> https://codereview.chromium.org/2136223003/ and so we can remove the branch as
> well.

My opinion here (And it's certainly something that is reasonable to disagree on)
is that we should try and make the new code as clean as possible, instead of
trying mirroring the old code (Though mirroring the old code was a great way to
start the CL).  Downside is we have more chance of breaking stuff that way. 
Upside is that it makes maintenance easier, and there's a good chance no one is
going to do a cleanup pass here.

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.cc:326: if
(buffer_bytes_read_ == 0) {
On 2016/07/12 10:49:11, yhirano wrote:
> On 2016/06/13 20:56:58, mmenke (Away July 2-10) wrote:
> > 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.
> 
> Sorry I don't understand. Do you mean "the start of this loop"?

Yes

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

https://codereview.chromium.org/1970693002/diff/1240001/content/browser/loade...
content/browser/loader/url_loader_factory_holder_unittest.cc:240:
BrowserThread::GetBlockingPool());
On 2016/07/12 10:49:11, yhirano wrote:
> On 2016/06/13 20:56:59, mmenke (Away July 2-10) wrote:
> > Suggest adding all these mock handler setup calls to the test fixture's
> > constructor, instead.
> 
> I'd prefer writing case-specific preparation in the test case unless it's
> reusable.

You call net::URLRequestFailedJob::AddUrlHandler() on two of them, and I
wouldn't be surprised to see more in the future.  I suspect we'll be using
URLRequestMockHTTPJob::AddUrlHandlers in other tests as well.

yhirano

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#newcode326 content/browser/loader/mojo_async_resource_handler.cc:326: if (buffer_bytes_read_ == 0) { On 2016/07/12 15:10:45, mmenke ...

4 years, 5 months ago (2016-07-13 12:04:57 UTC) #107

mmenke

Not a full pass, but I'm pretty happy with the design now. I'd love it ...

4 years, 5 months ago (2016-07-14 21:23:05 UTC) #108

Not a full pass, but I'm pretty happy with the design now.  I'd love it if we
could simplify the new ResourceHandler a little, but that may not be doable at
the moment.

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

https://codereview.chromium.org/1970693002/diff/1620001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.cc:75: class
MojoAsyncResourceHandler::WriterIOBuffer final
Hrm...Wonder if it would be cleaner to merge this and SharedWriter.  We'd
probably have to make this class implement AllocateBuffer itself, and it would
still have to expose writer_ as well (Or make AllocateBuffer add a pointer to a
method to call back on async completion, and add an EndWriting method).

Trying to figure out how to simplify
MojoAsyncResourceHandler::OnReadCompleted/MojoAsyncResourceHandler::ResumeIfDeferred,
and more generally, clean up the code a bit.

https://codereview.chromium.org/1970693002/diff/1620001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.cc:151: bool* defer) {
note:  This method had been removed from the interface, I believe.

https://codereview.chromium.org/1970693002/diff/1620001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.cc:168:
options.capacity_num_bytes = g_max_allocation_size;
This is the total capacity of the channel, not the number of bytes returned by
each BeginWrite call, right?  Should this be the total shared memory size of the
AsyncResourceHandler (512k) instead?  Happy to see this tuned later, but think
we should try and match AsyncResourceHandler's behavior for now, to try to avoid
a perf regression due to changed behavior.  And I rather suspect using a 32k
total buffer capacity would result in a perf regression.

In that same vein, we'd change buf_size to std::min(buffer_->size(),
g_max_allocation_size);

We would still potentially end up reading sizes less than kMinAllocationSize in
the old code, but the behavior would more closely match that of
AsyncResourceHandler.

https://codereview.chromium.org/1970693002/diff/1620001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.cc:212: bool
MojoAsyncResourceHandler::OnReadCompleted(int bytes_read, bool* defer) {
I think most branches here could use some documentation.

https://codereview.chromium.org/1970693002/diff/1620001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.cc:255: g_max_allocation_size
= size;
Can we just set this via the command line flag?  Each test runs in its own
process, right?  If not...the current unittests don't correctly restore it to
its original size on completion, and they should.

https://codereview.chromium.org/1970693002/diff/1620001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.cc:351: void
MojoAsyncResourceHandler::ResumeIfDeferred() {
I think most branches here could use some documentation.

https://codereview.chromium.org/1970693002/diff/1620001/content/browser/loade...
File content/browser/loader/mojo_async_resource_handler_unittest.cc (right):

https://codereview.chromium.org/1970693002/diff/1620001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:484: 
Any reason to delete url_loader_client_, too?  Guess that would be more checking
for mojo issues than bugs with the new code.

Maybe spin the message loop, make sure the destruction of handler_ has done any
cleanup it's going to do?

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 5 months ago (2016-07-15 11:13:06 UTC) #109

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/1640001

4 years, 5 months ago (2016-07-15 11:13:26 UTC) #110

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 5 months ago (2016-07-15 11:23:29 UTC) #112

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/1660001

4 years, 5 months ago (2016-07-15 11:23:45 UTC) #113

yhirano

https://codereview.chromium.org/1970693002/diff/1620001/content/browser/loader/mojo_async_resource_handler.cc File content/browser/loader/mojo_async_resource_handler.cc (right): https://codereview.chromium.org/1970693002/diff/1620001/content/browser/loader/mojo_async_resource_handler.cc#newcode75 content/browser/loader/mojo_async_resource_handler.cc:75: class MojoAsyncResourceHandler::WriterIOBuffer final On 2016/07/14 21:23:04, mmenke wrote: > ...

4 years, 5 months ago (2016-07-15 11:47:55 UTC) #114

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 5 months ago (2016-07-15 13:17:47 UTC) #115

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/193861)

4 years, 5 months ago (2016-07-15 13:17:50 UTC) #116

mmenke

https://codereview.chromium.org/1970693002/diff/1620001/content/browser/loader/mojo_async_resource_handler.cc File content/browser/loader/mojo_async_resource_handler.cc (right): https://codereview.chromium.org/1970693002/diff/1620001/content/browser/loader/mojo_async_resource_handler.cc#newcode75 content/browser/loader/mojo_async_resource_handler.cc:75: class MojoAsyncResourceHandler::WriterIOBuffer final On 2016/07/15 11:47:55, yhirano wrote: > ...

4 years, 5 months ago (2016-07-15 14:54:07 UTC) #117

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

https://codereview.chromium.org/1970693002/diff/1620001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.cc:75: class
MojoAsyncResourceHandler::WriterIOBuffer final
On 2016/07/15 11:47:55, yhirano wrote:
> On 2016/07/14 21:23:04, mmenke wrote:
> > Hrm...Wonder if it would be cleaner to merge this and SharedWriter.  We'd
> > probably have to make this class implement AllocateBuffer itself, and it
would
> > still have to expose writer_ as well (Or make AllocateBuffer add a pointer
to
> a
> > method to call back on async completion, and add an EndWriting method).
> > 
> > Trying to figure out how to simplify
> >
>
MojoAsyncResourceHandler::OnReadCompleted/MojoAsyncResourceHandler::ResumeIfDeferred,
> > and more generally, clean up the code a bit.
> 
> Are you proposing to return the same IOBuffer pointer from OnWillRead by
placing
> AllocateBuffer in this class? That sounds confusing to me.

Yea, that's what I was thinking.  Currently CopyReadData and ResumeIfDeferred
are quite hard to follow, and just trying to figure out if we can fix that.  May
make things worse, though.  If you prefer it as-is, that's fine.

> As sometimes OnWillRead returns a net::WrappedIOBuffer, I don't think merging
> this class with SharedWriter is a good idea.

https://codereview.chromium.org/1970693002/diff/1620001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.cc:202: *buf = new
net::WrappedIOBuffer(buffer_->data());
Hrm...Why can't we just return buffer_, like we do in the fallthrough case?

https://codereview.chromium.org/1970693002/diff/1620001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.cc:212: bool
MojoAsyncResourceHandler::OnReadCompleted(int bytes_read, bool* defer) {
On 2016/07/15 11:47:55, yhirano wrote:
> On 2016/07/14 21:23:05, mmenke wrote:
> > I think most branches here could use some documentation.
> 
> Personally PS46 version is much easier to explain for me.

I find them both extremely obscure, largely because of the
is_using_io_buffer_not_from_writer_ stuff.

Longer term, I think we should probably make
MojoAsyncResourceHandler::OnWillRead be able to complete asynchronously, and get
rid of is_using_io_buffer_not_from_writer_, but certainly not in this CL. 
Unclear how difficult making OnWillRead async will be, but it will touch a fair
bit of code.

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 5 months ago (2016-07-19 09:30:40 UTC) #118

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/1680001

4 years, 5 months ago (2016-07-19 09:31:25 UTC) #119

yhirano

https://codereview.chromium.org/1970693002/diff/1620001/content/browser/loader/mojo_async_resource_handler.cc File content/browser/loader/mojo_async_resource_handler.cc (right): https://codereview.chromium.org/1970693002/diff/1620001/content/browser/loader/mojo_async_resource_handler.cc#newcode151 content/browser/loader/mojo_async_resource_handler.cc:151: bool* defer) { On 2016/07/14 21:23:04, mmenke wrote: > ...

4 years, 5 months ago (2016-07-19 09:42:47 UTC) #120

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

https://codereview.chromium.org/1970693002/diff/1620001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.cc:151: bool* defer) {
On 2016/07/14 21:23:04, mmenke wrote:
> note:  This method had been removed from the interface, I believe.

OnBeforeNetworkStart was removed from URLRequest::Delegate but not from
ResourceHandler, I think.

https://codereview.chromium.org/1970693002/diff/1620001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.cc:168:
options.capacity_num_bytes = g_max_allocation_size;
On 2016/07/14 21:23:04, mmenke wrote:
> This is the total capacity of the channel, not the number of bytes returned by
> each BeginWrite call, right?  Should this be the total shared memory size of
the
> AsyncResourceHandler (512k) instead?  Happy to see this tuned later, but think
> we should try and match AsyncResourceHandler's behavior for now, to try to
avoid
> a perf regression due to changed behavior.  And I rather suspect using a 32k
> total buffer capacity would result in a perf regression.
> 

I renamed it to g_allocation_size and changed the default value.

> In that same vein, we'd change buf_size to std::min(buffer_->size(),
> g_max_allocation_size);
> 
> We would still potentially end up reading sizes less than kMinAllocationSize
in
> the old code, but the behavior would more closely match that of
> AsyncResourceHandler.

I don't understand the benefit: Can you explain it? if |available| is greater
than |g_max_allocation_size|, exposing smaller size will not save any memory.

https://codereview.chromium.org/1970693002/diff/1620001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.cc:202: *buf = new
net::WrappedIOBuffer(buffer_->data());
On 2016/07/15 14:54:07, mmenke wrote:
> Hrm...Why can't we just return buffer_, like we do in the fallthrough case?

You're right. fixed.

https://codereview.chromium.org/1970693002/diff/1620001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.cc:212: bool
MojoAsyncResourceHandler::OnReadCompleted(int bytes_read, bool* defer) {
On 2016/07/15 14:54:07, mmenke wrote:
> On 2016/07/15 11:47:55, yhirano wrote:
> > On 2016/07/14 21:23:05, mmenke wrote:
> > > I think most branches here could use some documentation.
> > 
> > Personally PS46 version is much easier to explain for me.
> 
> I find them both extremely obscure, largely because of the
> is_using_io_buffer_not_from_writer_ stuff.
> 
> Longer term, I think we should probably make
> MojoAsyncResourceHandler::OnWillRead be able to complete asynchronously, and
get
> rid of is_using_io_buffer_not_from_writer_, but certainly not in this CL. 
> Unclear how difficult making OnWillRead async will be, but it will touch a
fair
> bit of code.

I agree that both are hard to understand and changing OnWillRead may mitigate
the pain (and I think MimeTypeResourceHandler's implicit expectation should be
fixed too).

I reverted the code back to PS46 version and added some comments.

https://codereview.chromium.org/1970693002/diff/1620001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.cc:255: g_max_allocation_size
= size;
On 2016/07/14 21:23:05, mmenke wrote:
> Can we just set this via the command line flag?  Each test runs in its own
> process, right?  If not...the current unittests don't correctly restore it to
> its original size on completion, and they should.

I think some test cases share the same process. I added
SetAllocationSizeForTesting(kDefaultAllocationSize) in TearDown functions.

https://codereview.chromium.org/1970693002/diff/1620001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.cc:351: void
MojoAsyncResourceHandler::ResumeIfDeferred() {
On 2016/07/14 21:23:05, mmenke wrote:
> I think most branches here could use some documentation.

Done.

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 5 months ago (2016-07-19 12:20:52 UTC) #121

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/195241)

4 years, 5 months ago (2016-07-19 12:20:54 UTC) #122

mmenke

https://codereview.chromium.org/1970693002/diff/1620001/content/browser/loader/mojo_async_resource_handler.cc File content/browser/loader/mojo_async_resource_handler.cc (right): https://codereview.chromium.org/1970693002/diff/1620001/content/browser/loader/mojo_async_resource_handler.cc#newcode168 content/browser/loader/mojo_async_resource_handler.cc:168: options.capacity_num_bytes = g_max_allocation_size; On 2016/07/19 09:42:47, yhirano wrote: > ...

4 years, 5 months ago (2016-07-19 15:50:28 UTC) #123

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

https://codereview.chromium.org/1970693002/diff/1620001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.cc:168:
options.capacity_num_bytes = g_max_allocation_size;
On 2016/07/19 09:42:47, yhirano wrote:
> On 2016/07/14 21:23:04, mmenke wrote:
> > This is the total capacity of the channel, not the number of bytes returned
by
> > each BeginWrite call, right?  Should this be the total shared memory size of
> the
> > AsyncResourceHandler (512k) instead?  Happy to see this tuned later, but
think
> > we should try and match AsyncResourceHandler's behavior for now, to try to
> avoid
> > a perf regression due to changed behavior.  And I rather suspect using a 32k
> > total buffer capacity would result in a perf regression.
> > 
> 
> I renamed it to g_allocation_size and changed the default value.
> 
> > In that same vein, we'd change buf_size to std::min(buffer_->size(),
> > g_max_allocation_size);
> > 
> > We would still potentially end up reading sizes less than kMinAllocationSize
> in
> > the old code, but the behavior would more closely match that of
> > AsyncResourceHandler.
> 
> I don't understand the benefit: Can you explain it? if |available| is greater
> than |g_max_allocation_size|, exposing smaller size will not save any memory.

It's not about memory, it's about performance.  Let's suppose it takes 10
milliseconds to process 32k of data and each IPC call introduces 100
milliseconds of latency (These numbers are way too big, just using them to
demonstrate my point).

Suppose we have a 1MB response.

If we send it in 2 512k chunks, then it takes 100+160+100 milliseconds to send
the first chunk, for it to be processed, and for us to learn that the renderer
needs more data.  The same for the second chunk, so that's 360*2 = 720
milliseconds to handle the body and which point we try to read again, learn the
file was complete.  Then we tell the renderer the response is complete, which
has another 100 millisecond delay, so that's about 820 ms of latency from when
we read the first 512k to when the request completes.  All of this is blocking
time, during which we can't do anything else, so add to that any internal work
the network stack does when we read the data.  It can't be done in parallel,
since we don't have a pending read while we're waiting to pass the data around.

Now consider if we read the data in 32 32k chunks.  When we read the first
chunk, we post an IPC, which takes 100 milliseconds to reach the renderer, 10
milliseconds of processing, and 100 milliseconds for the browser process to
learn it was consumed...But we can be doing the next 32k read when that's
happening, so before we hear back, and maybe before we renderer even received
the last 32k, we're sending 32k more of data.  Regardless of how fast the
network is, we learn the renderer processed the first 32k before the time the
renderer starts processing the next 11th 32k chunk.  So basically, we're never
stuck waiting for an IPC to tell us the renderer has finished handling an
earlier read.

So the end result is 100 + 320 + 100 = 520 millseconds (We don't even need the
extra half-trip to learn the response is complete) instead of 720 millisecond. 
Make sense?

mmenke

I still need to look at tests (Which I'll get to tomorrow), and hope I ...

4 years, 5 months ago (2016-07-19 20:50:27 UTC) #124

I still need to look at tests (Which I'll get to tomorrow), and hope I can just
do in one or two passes), but I'm now quite happy with what we have in
browser/loader.

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

https://codereview.chromium.org/1970693002/diff/1620001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.cc:168:
options.capacity_num_bytes = g_max_allocation_size;
On 2016/07/19 15:50:28, mmenke wrote:
> On 2016/07/19 09:42:47, yhirano wrote:
> > On 2016/07/14 21:23:04, mmenke wrote:
> > > This is the total capacity of the channel, not the number of bytes
returned
> by
> > > each BeginWrite call, right?  Should this be the total shared memory size
of
> > the
> > > AsyncResourceHandler (512k) instead?  Happy to see this tuned later, but
> think
> > > we should try and match AsyncResourceHandler's behavior for now, to try to
> > avoid
> > > a perf regression due to changed behavior.  And I rather suspect using a
32k
> > > total buffer capacity would result in a perf regression.
> > > 
> > 
> > I renamed it to g_allocation_size and changed the default value.
> > 
> > > In that same vein, we'd change buf_size to std::min(buffer_->size(),
> > > g_max_allocation_size);
> > > 
> > > We would still potentially end up reading sizes less than
kMinAllocationSize
> > in
> > > the old code, but the behavior would more closely match that of
> > > AsyncResourceHandler.
> > 
> > I don't understand the benefit: Can you explain it? if |available| is
greater
> > than |g_max_allocation_size|, exposing smaller size will not save any
memory.
> 
> It's not about memory, it's about performance.  Let's suppose it takes 10
> milliseconds to process 32k of data and each IPC call introduces 100
> milliseconds of latency (These numbers are way too big, just using them to
> demonstrate my point).
> 
> Suppose we have a 1MB response.
> 
> If we send it in 2 512k chunks, then it takes 100+160+100 milliseconds to send
> the first chunk, for it to be processed, and for us to learn that the renderer
> needs more data.  The same for the second chunk, so that's 360*2 = 720
> milliseconds to handle the body and which point we try to read again, learn
the
> file was complete.  Then we tell the renderer the response is complete, which
> has another 100 millisecond delay, so that's about 820 ms of latency from when
> we read the first 512k to when the request completes.  All of this is blocking
> time, during which we can't do anything else, so add to that any internal work
> the network stack does when we read the data.  It can't be done in parallel,
> since we don't have a pending read while we're waiting to pass the data
around.
> 
> Now consider if we read the data in 32 32k chunks.  When we read the first
> chunk, we post an IPC, which takes 100 milliseconds to reach the renderer, 10
> milliseconds of processing, and 100 milliseconds for the browser process to
> learn it was consumed...But we can be doing the next 32k read when that's
> happening, so before we hear back, and maybe before we renderer even received
> the last 32k, we're sending 32k more of data.  Regardless of how fast the
> network is, we learn the renderer processed the first 32k before the time the
> renderer starts processing the next 11th 32k chunk.  So basically, we're never
> stuck waiting for an IPC to tell us the renderer has finished handling an
> earlier read.
> 
> So the end result is 100 + 320 + 100 = 520 millseconds (We don't even need the
> extra half-trip to learn the response is complete) instead of 720 millisecond.

> Make sense?

It's not necessarily the case that a bunch of 32k writes are faster (There are
more context switches, for instance), but if we see a performance regression
with the Mojo code, I want to minimize potential causes to investigate.

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

https://codereview.chromium.org/1970693002/diff/1680001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.cc:188: }
Can we just use AllocateBuffer here?

https://codereview.chromium.org/1970693002/diff/1680001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.cc:309: MojoResult result =
BeginWrite(&data, &available);
Hrm...This is just like AllocateBuffer, except it doesn't write to buffer_.

Here's my suggestion:  Use AllocateBuffer here, and instead of using the bool
"is_using_io_buffer_not_from_writer_" and storing the GrowableIOBuffer in that
case in buffer_, use a separate IOBuffer for it - non_writer_io_buffer_?  Could
even use a DrainableIOBuffer, and have it take care of tracking how much data is
left in it.

Then when we've copied it all, just release that buffer.  WDYT?

https://codereview.chromium.org/1970693002/diff/1680001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.cc:316: if
(buffer_bytes_read_ == 0) {
The fact that this is after the BeginWrite is a bit unexpected (Though the code
is certainly correct) - maybe a comment here about it, and in CopyReadData's
declaration as well?

https://codereview.chromium.org/1970693002/diff/1680001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.cc:319: }
Maybe add a blank line here?  This methods seems a bit short of them.

https://codereview.chromium.org/1970693002/diff/1680001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.cc:325: if
(EndWrite(copied_size) != MOJO_RESULT_OK)
Fix indent.

https://codereview.chromium.org/1970693002/diff/1680001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.cc:350: DCHECK(did_defer_);
Hrm...did_defer_ is only used between function calls in this one DCHECK.  Should
we get rid of it, as a class variable, and just use locals instead?

https://codereview.chromium.org/1970693002/diff/1680001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.cc:367: }
Suggest a blank line here.

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

https://codereview.chromium.org/1970693002/diff/1680001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.h:91: bool CopyReadData(bool*
defer);
These should all be documented.

https://codereview.chromium.org/1970693002/diff/1680001/content/browser/loade...
File content/browser/loader/mojo_async_resource_handler_unittest.cc (right):

https://codereview.chromium.org/1970693002/diff/1680001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:7: #include
<string.h>
<string> is preferred (And then remove the blank line below this)

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

https://codereview.chromium.org/1970693002/diff/1680001/content/browser/loade...
content/browser/loader/url_loader_factory_holder.cc:7: #include <memory>
nit:  Included in header, so not needed.

https://codereview.chromium.org/1970693002/diff/1680001/content/browser/loade...
content/browser/loader/url_loader_factory_holder.cc:10: #include
"base/memory/ptr_util.h"
I don't think this is used?

https://codereview.chromium.org/1970693002/diff/1680001/content/browser/loade...
content/browser/loader/url_loader_factory_holder.cc:23: class
URLLoaderFactoryHolder::URLLoaderFactoryImpl final
Seems like this class should be in an anonymous namespace, since it's not
declared in the header.

https://codereview.chromium.org/1970693002/diff/1680001/content/browser/loade...
File content/browser/loader/url_loader_factory_holder.h (right):

https://codereview.chromium.org/1970693002/diff/1680001/content/browser/loade...
content/browser/loader/url_loader_factory_holder.h:10: #include
"base/compiler_specific.h"
macros.h is where DISALLOW_COPY_AND_ASSIGN is now declared.

https://codereview.chromium.org/1970693002/diff/1680001/content/browser/loade...
content/browser/loader/url_loader_factory_holder.h:31: class
URLLoaderFactoryImpl;
Suggest a blank line here.

mmenke

https://codereview.chromium.org/1970693002/diff/1680001/content/browser/loader/mojo_async_resource_handler.cc File content/browser/loader/mojo_async_resource_handler.cc (right): https://codereview.chromium.org/1970693002/diff/1680001/content/browser/loader/mojo_async_resource_handler.cc#newcode220 content/browser/loader/mojo_async_resource_handler.cc:220: if (*defer) { BUG (Erm...Or something?): It turns out ...

4 years, 5 months ago (2016-07-19 21:58:52 UTC) #125

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 5 months ago (2016-07-20 10:46:27 UTC) #126

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/1700001

4 years, 5 months ago (2016-07-20 10:46:44 UTC) #127

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 5 months ago (2016-07-20 11:25:46 UTC) #128

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/1720001

4 years, 5 months ago (2016-07-20 11:26:31 UTC) #129

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 5 months ago (2016-07-20 13:27:02 UTC) #130

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 5 months ago (2016-07-20 13:27:05 UTC) #131

yhirano

https://codereview.chromium.org/1970693002/diff/1620001/content/browser/loader/mojo_async_resource_handler.cc File content/browser/loader/mojo_async_resource_handler.cc (right): https://codereview.chromium.org/1970693002/diff/1620001/content/browser/loader/mojo_async_resource_handler.cc#newcode168 content/browser/loader/mojo_async_resource_handler.cc:168: options.capacity_num_bytes = g_max_allocation_size; On 2016/07/19 20:50:26, mmenke wrote: > ...

4 years, 5 months ago (2016-07-20 13:38:36 UTC) #132

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

https://codereview.chromium.org/1970693002/diff/1620001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.cc:168:
options.capacity_num_bytes = g_max_allocation_size;
On 2016/07/19 20:50:26, mmenke wrote:
> On 2016/07/19 15:50:28, mmenke wrote:
> > On 2016/07/19 09:42:47, yhirano wrote:
> > > On 2016/07/14 21:23:04, mmenke wrote:
> > > > This is the total capacity of the channel, not the number of bytes
> returned
> > by
> > > > each BeginWrite call, right?  Should this be the total shared memory
size
> of
> > > the
> > > > AsyncResourceHandler (512k) instead?  Happy to see this tuned later, but
> > think
> > > > we should try and match AsyncResourceHandler's behavior for now, to try
to
> > > avoid
> > > > a perf regression due to changed behavior.  And I rather suspect using a
> 32k
> > > > total buffer capacity would result in a perf regression.
> > > > 
> > > 
> > > I renamed it to g_allocation_size and changed the default value.
> > > 
> > > > In that same vein, we'd change buf_size to std::min(buffer_->size(),
> > > > g_max_allocation_size);
> > > > 
> > > > We would still potentially end up reading sizes less than
> kMinAllocationSize
> > > in
> > > > the old code, but the behavior would more closely match that of
> > > > AsyncResourceHandler.
> > > 
> > > I don't understand the benefit: Can you explain it? if |available| is
> greater
> > > than |g_max_allocation_size|, exposing smaller size will not save any
> memory.
> > 
> > It's not about memory, it's about performance.  Let's suppose it takes 10
> > milliseconds to process 32k of data and each IPC call introduces 100
> > milliseconds of latency (These numbers are way too big, just using them to
> > demonstrate my point).
> > 
> > Suppose we have a 1MB response.
> > 
> > If we send it in 2 512k chunks, then it takes 100+160+100 milliseconds to
send
> > the first chunk, for it to be processed, and for us to learn that the
renderer
> > needs more data.  The same for the second chunk, so that's 360*2 = 720
> > milliseconds to handle the body and which point we try to read again, learn
> the
> > file was complete.  Then we tell the renderer the response is complete,
which
> > has another 100 millisecond delay, so that's about 820 ms of latency from
when
> > we read the first 512k to when the request completes.  All of this is
blocking
> > time, during which we can't do anything else, so add to that any internal
work
> > the network stack does when we read the data.  It can't be done in parallel,
> > since we don't have a pending read while we're waiting to pass the data
> around.
> > 
> > Now consider if we read the data in 32 32k chunks.  When we read the first
> > chunk, we post an IPC, which takes 100 milliseconds to reach the renderer,
10
> > milliseconds of processing, and 100 milliseconds for the browser process to
> > learn it was consumed...But we can be doing the next 32k read when that's
> > happening, so before we hear back, and maybe before we renderer even
received
> > the last 32k, we're sending 32k more of data.  Regardless of how fast the
> > network is, we learn the renderer processed the first 32k before the time
the
> > renderer starts processing the next 11th 32k chunk.  So basically, we're
never
> > stuck waiting for an IPC to tell us the renderer has finished handling an
> > earlier read.
> > 
> > So the end result is 100 + 320 + 100 = 520 millseconds (We don't even need
the
> > extra half-trip to learn the response is complete) instead of 720
millisecond.
> 
> > Make sense?
> 
> It's not necessarily the case that a bunch of 32k writes are faster (There are
> more context switches, for instance), but if we see a performance regression
> with the Mojo code, I want to minimize potential causes to investigate.

Thanks, I see. I added kMaxChunkSize.

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

https://codereview.chromium.org/1970693002/diff/1680001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.cc:188: }
On 2016/07/19 20:50:27, mmenke wrote:
> Can we just use AllocateBuffer here?

Done.

https://codereview.chromium.org/1970693002/diff/1680001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.cc:220: if (*defer) {
On 2016/07/19 21:58:51, mmenke wrote:
> BUG (Erm...Or something?):  It turns out some code cannot handle a defer here,
> on the first read (See MimeTypeResourceHandler::UseAlternateNextHandler - this
> is only the result of the first read).  So...erm...yea...  Also, it's being
> refactored in another CL I'm currently reviewing:
> https://codereview.chromium.org/2005273002/ (Which currently maintains the
same
> limitation, though moves it to another ResourceHandler).

Thank you. Basically I want MimeTypeResourceHandler to accept such valid input.
I will try to modify it tomorrow.

https://codereview.chromium.org/1970693002/diff/1680001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.cc:309: MojoResult result =
BeginWrite(&data, &available);
On 2016/07/19 20:50:26, mmenke wrote:
> Hrm...This is just like AllocateBuffer, except it doesn't write to buffer_.
> 
> Here's my suggestion:  Use AllocateBuffer here, and instead of using the bool
> "is_using_io_buffer_not_from_writer_" and storing the GrowableIOBuffer in that
> case in buffer_, use a separate IOBuffer for it - non_writer_io_buffer_? 
Could
> even use a DrainableIOBuffer, and have it take care of tracking how much data
is
> left in it.
> 
> Then when we've copied it all, just release that buffer.  WDYT?

I added |buffer| parameter to AllocateBuffer.

Regarding GlowableIOBuffer and DrainableIOBuffer, are you suggesting having
three buffers in this class (The GlowableIOBuffer should be held as a member as
we need data() in CopyReadData, and the DrainableIOBuffer should be held as a
member too because CopyReadData need not be done synchronously)?

https://codereview.chromium.org/1970693002/diff/1680001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.cc:316: if
(buffer_bytes_read_ == 0) {
On 2016/07/19 20:50:27, mmenke wrote:
> The fact that this is after the BeginWrite is a bit unexpected (Though the
code
> is certainly correct) - maybe a comment here about it, and in CopyReadData's
> declaration as well?

Done.

https://codereview.chromium.org/1970693002/diff/1680001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.cc:319: }
On 2016/07/19 20:50:26, mmenke wrote:
> Maybe add a blank line here?  This methods seems a bit short of them.

Done.

https://codereview.chromium.org/1970693002/diff/1680001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.cc:325: if
(EndWrite(copied_size) != MOJO_RESULT_OK)
On 2016/07/19 20:50:26, mmenke wrote:
> Fix indent.

Done.

https://codereview.chromium.org/1970693002/diff/1680001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.cc:350: DCHECK(did_defer_);
On 2016/07/19 20:50:26, mmenke wrote:
> Hrm...did_defer_ is only used between function calls in this one DCHECK. 
Should
> we get rid of it, as a class variable, and just use locals instead?

Done.

https://codereview.chromium.org/1970693002/diff/1680001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.cc:367: }
On 2016/07/19 20:50:26, mmenke wrote:
> Suggest a blank line here.

Done.

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

https://codereview.chromium.org/1970693002/diff/1680001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.h:91: bool CopyReadData(bool*
defer);
On 2016/07/19 20:50:27, mmenke wrote:
> These should all be documented.

Done.

https://codereview.chromium.org/1970693002/diff/1680001/content/browser/loade...
File content/browser/loader/mojo_async_resource_handler_unittest.cc (right):

https://codereview.chromium.org/1970693002/diff/1680001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:7: #include
<string.h>
On 2016/07/19 20:50:27, mmenke wrote:
> <string> is preferred (And then remove the blank line below this)

This is not for std::string but for memset (<string> is included in the header).

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

https://codereview.chromium.org/1970693002/diff/1680001/content/browser/loade...
content/browser/loader/url_loader_factory_holder.cc:7: #include <memory>
On 2016/07/19 20:50:27, mmenke wrote:
> nit:  Included in header, so not needed.

Done.

https://codereview.chromium.org/1970693002/diff/1680001/content/browser/loade...
content/browser/loader/url_loader_factory_holder.cc:10: #include
"base/memory/ptr_util.h"
On 2016/07/19 20:50:27, mmenke wrote:
> I don't think this is used?

Done.

https://codereview.chromium.org/1970693002/diff/1680001/content/browser/loade...
content/browser/loader/url_loader_factory_holder.cc:23: class
URLLoaderFactoryHolder::URLLoaderFactoryImpl final
On 2016/07/19 20:50:27, mmenke wrote:
> Seems like this class should be in an anonymous namespace, since it's not
> declared in the header.

URLLoaderFactoryHolder needs to hold factory_ as std::unique_ptr<FactoryImpl> as
it calls URLLoaderFactoryImpl::Bind. That means we need to declare
URLLoaderFactoryImpl in the header, and I think making it an inner class is the
best way.

https://codereview.chromium.org/1970693002/diff/1680001/content/browser/loade...
File content/browser/loader/url_loader_factory_holder.h (right):

https://codereview.chromium.org/1970693002/diff/1680001/content/browser/loade...
content/browser/loader/url_loader_factory_holder.h:10: #include
"base/compiler_specific.h"
On 2016/07/19 20:50:27, mmenke wrote:
> macros.h is where DISALLOW_COPY_AND_ASSIGN is now declared.

Done.

https://codereview.chromium.org/1970693002/diff/1680001/content/browser/loade...
content/browser/loader/url_loader_factory_holder.h:31: class
URLLoaderFactoryImpl;
On 2016/07/19 20:50:27, mmenke wrote:
> Suggest a blank line here.

Done.

mmenke

https://codereview.chromium.org/1970693002/diff/1680001/content/browser/loader/mojo_async_resource_handler.cc File content/browser/loader/mojo_async_resource_handler.cc (right): https://codereview.chromium.org/1970693002/diff/1680001/content/browser/loader/mojo_async_resource_handler.cc#newcode220 content/browser/loader/mojo_async_resource_handler.cc:220: if (*defer) { On 2016/07/20 13:38:36, yhirano wrote: > ...

4 years, 5 months ago (2016-07-20 14:58:37 UTC) #133

mmenke

https://codereview.chromium.org/1970693002/diff/1720001/content/browser/loader/mojo_async_resource_handler_test_util.cc File content/browser/loader/mojo_async_resource_handler_test_util.cc (right): https://codereview.chromium.org/1970693002/diff/1720001/content/browser/loader/mojo_async_resource_handler_test_util.cc#newcode37 content/browser/loader/mojo_async_resource_handler_test_util.cc:37: response_body_ = mojo::ScopedDataPipeConsumerHandle(); I think ".reset()" makes it much ...

4 years, 5 months ago (2016-07-20 19:08:43 UTC) #134

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 5 months ago (2016-07-21 04:09:45 UTC) #135

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/1740001

4 years, 5 months ago (2016-07-21 04:10:05 UTC) #136

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 5 months ago (2016-07-21 04:11:58 UTC) #137

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-generic_chromium_compile_only_ng/builds/170869)

4 years, 5 months ago (2016-07-21 04:12:01 UTC) #138

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 5 months ago (2016-07-21 11:09:38 UTC) #139

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/1760001

4 years, 5 months ago (2016-07-21 11:09:51 UTC) #140

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 5 months ago (2016-07-21 12:59:54 UTC) #141

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 5 months ago (2016-07-21 12:59:56 UTC) #142

yhirano

https://codereview.chromium.org/1970693002/diff/1680001/content/browser/loader/mojo_async_resource_handler.cc File content/browser/loader/mojo_async_resource_handler.cc (right): https://codereview.chromium.org/1970693002/diff/1680001/content/browser/loader/mojo_async_resource_handler.cc#newcode220 content/browser/loader/mojo_async_resource_handler.cc:220: if (*defer) { On 2016/07/20 14:58:37, mmenke wrote: > ...

4 years, 5 months ago (2016-07-21 14:50:22 UTC) #143

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

https://codereview.chromium.org/1970693002/diff/1680001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.cc:220: if (*defer) {
On 2016/07/20 14:58:37, mmenke wrote:
> On 2016/07/20 13:38:36, yhirano wrote:
> > On 2016/07/19 21:58:51, mmenke wrote:
> > > BUG (Erm...Or something?):  It turns out some code cannot handle a defer
> here,
> > > on the first read (See MimeTypeResourceHandler::UseAlternateNextHandler -
> this
> > > is only the result of the first read).  So...erm...yea...  Also, it's
being
> > > refactored in another CL I'm currently reviewing:
> > > https://codereview.chromium.org/2005273002/ (Which currently maintains the
> > same
> > > limitation, though moves it to another ResourceHandler).
> > 
> > Thank you. Basically I want MimeTypeResourceHandler to accept such valid
> input.
> > I will try to modify it tomorrow.
> 
> Here's how I suggest you proceed, to minimize total work done, and chance of
> regression:
> 
> Do it in another CL, to be landed before that (This CL is pretty big, smaller
> CLs are easier for me to review), and build that CL on top of
> https://codereview.chromium.org/2005273002/ (So Camille doesn't have to port
> your changes over to new major refactor of MimeTypeResourceHandler).
> 
> It does mean you can't land these until Camille's CL lands, but I'm happy to
> review other CLs built on top of them in the meantime, if her CL takes longer
> than these to be ready to land.

Then I will wait for a while because when your comment
https://codereview.chromium.org/2005273002/diff/340001/content/browser/loader...
is addressed, my change will be affected largely.

https://codereview.chromium.org/1970693002/diff/1720001/content/browser/loade...
File content/browser/loader/mojo_async_resource_handler_test_util.cc (right):

https://codereview.chromium.org/1970693002/diff/1720001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_test_util.cc:37:
response_body_ = mojo::ScopedDataPipeConsumerHandle();
On 2016/07/20 19:08:41, mmenke wrote:
> I think ".reset()" makes it much clearer that we're deleting the old handle,
> rather than creating some sort of object that does something.

Done.

https://codereview.chromium.org/1970693002/diff/1720001/content/browser/loade...
File content/browser/loader/mojo_async_resource_handler_test_util.h (right):

https://codereview.chromium.org/1970693002/diff/1720001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_test_util.h:18: class
FakeURLLoaderClient final : public mojom::URLLoaderClient {
On 2016/07/20 19:08:42, mmenke wrote:
> Class needs to be documented.

Done.

https://codereview.chromium.org/1970693002/diff/1720001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_test_util.h:18: class
FakeURLLoaderClient final : public mojom::URLLoaderClient {
On 2016/07/20 19:08:42, mmenke wrote:
> Is this really a fake client?  I guess it doesn't drive the reading of the
> body...

I don't feel it so unnatural but maybe my wording is not good. What should we
call this class instead? Stub?

https://codereview.chromium.org/1970693002/diff/1720001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_test_util.h:18: class
FakeURLLoaderClient final : public mojom::URLLoaderClient {
On 2016/07/20 19:08:42, mmenke wrote:
> Can you call this fake_url_loader_client.h?  test_util seems too general.

Will do when we agree on the class name.

https://codereview.chromium.org/1970693002/diff/1720001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_test_util.h:36:
mojom::URLLoaderClientPtr CreateInterfacePtrAndBind() {
On 2016/07/20 19:08:42, mmenke wrote:
> Shouldn't inline a non-trivial method.

Done.

https://codereview.chromium.org/1970693002/diff/1720001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_test_util.h:41: void
set_quit_closure(const base::Closure& quit_closure) {
On 2016/07/20 19:08:42, mmenke wrote:
> Not at all clear when this is called. Also, is there any fundamental property
of
> this that forces it to be a RunLoop::QuitClosure?

Done.

https://codereview.chromium.org/1970693002/diff/1720001/content/browser/loade...
File content/browser/loader/mojo_async_resource_handler_unittest.cc (right):

https://codereview.chromium.org/1970693002/diff/1720001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:177: void
Cancel() override { ADD_FAILURE() << "Cancel should not be called."; }
On 2016/07/20 19:08:42, mmenke wrote:
> Should probably have blank lines around each of these methods.

Done.

https://codereview.chromium.org/1970693002/diff/1720001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:190:
quit_closure_.Run();
On 2016/07/20 19:08:42, mmenke wrote:
> Why is the closure being invoked on resume?  Seems like we need to be clearer
> about what it's intended to be used for.

Mainly for consistency with CancelWithError.

https://codereview.chromium.org/1970693002/diff/1720001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:200: unsigned
num_resume_calls() const { return num_resume_calls_; }
On 2016/07/20 19:08:42, mmenke wrote:
> unsigned -> int, to match num_resume_calls_?

Done.

https://codereview.chromium.org/1970693002/diff/1720001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:289: void
GetContexts(ResourceType resource_type,
On 2016/07/20 19:08:42, mmenke wrote:
> Is this method used?

No.

https://codereview.chromium.org/1970693002/diff/1720001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:305: void
CheckOnWillReadWithLongContents() {
On 2016/07/20 19:08:42, mmenke wrote:
> Need a comment here - don't think the name is sufficient.

Oops, sorry. This should have been deleted when I introduced TEST_P cases.
Removed (and applied the following comments to the corresponding test case).

https://codereview.chromium.org/1970693002/diff/1720001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:314: const size_t
contents_size = 3 * buf_size + 2;
On 2016/07/20 19:08:42, mmenke wrote:
> contents_size = body_size?

Sorry I don't understand the comment.

https://codereview.chromium.org/1970693002/diff/1720001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:316:
ASSERT_FALSE(defer);
On 2016/07/20 19:08:42, mmenke wrote:
> Why are we telling it a 0 byte read completed first?

It's for test code simplicity. I want to start the L325-L331 loop with
OnWillRead, not OnReadCompleted.

https://codereview.chromium.org/1970693002/diff/1720001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:318:
RunUntilNextNotification();
On 2016/07/20 19:08:42, mmenke wrote:
> Think this needs a comment.  What notification are we waiting for?
> 
> It seems problemative that this code results RunLoop being quit in a half
dozen
> places.  Could be make waiting a bit more targeted?

Each RunUntilNextNotification call is followed by EXPECT or ASSERT for what it
waits for. In this case, it's waiting until the response body is available. Do
you think it's not enough?

https://codereview.chromium.org/1970693002/diff/1720001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:328:
memset(io_buffer->data(), 'a', to_be_written);
On 2016/07/20 19:08:42, mmenke wrote:
> Should we make the string more interesting?  Seems like with a bunch of a's,
> things could go badly wrong.  Could a full expected string like
> "012345678901234567890123456789..." up front, and then just copy the
> corresponding substring here.

Done.

https://codereview.chromium.org/1970693002/diff/1720001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:335: MojoResult r
= mojo::ReadDataRaw(url_loader_client_.response_body(), buf,
On 2016/07/20 19:08:42, mmenke wrote:
> nit: r -> result

Done.

https://codereview.chromium.org/1970693002/diff/1720001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:339: actual +=
std::string(buf, read);
On 2016/07/20 19:08:42, mmenke wrote:
> nit:  actual.append(buff, read)?

Done.

https://codereview.chromium.org/1970693002/diff/1720001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:341: unsigned
resume_count = resource_controller_.num_resume_calls();
On 2016/07/20 19:08:42, mmenke wrote:
> Should have a comment on this block of code.

Done.

https://codereview.chromium.org/1970693002/diff/1720001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:341: unsigned
resume_count = resource_controller_.num_resume_calls();
On 2016/07/20 19:08:42, mmenke wrote:
> int?

Done.

https://codereview.chromium.org/1970693002/diff/1720001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:416: // This test
class is parametrized with MojoAsyncResourceHandler's allocation
On 2016/07/20 19:08:42, mmenke wrote:
> parametrized -> parameterized

Done.

https://codereview.chromium.org/1970693002/diff/1720001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:453:
EXPECT_LE(response->head.response_start, now2);
On 2016/07/20 19:08:42, mmenke wrote:
> This seems really weird - the purpose of AsyncResourceHandler modifying
> ResourceResponse is not to modify ResourceResponse, but so that it can pass
the
> modified ResourceResponse down to the other process.

Those are checked after RunUntilNextNotification(). I think checking
side-effects is also beneficial.

https://codereview.chromium.org/1970693002/diff/1720001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:454:
EXPECT_TRUE(rdh_delegate.is_on_response_started_called());
On 2016/07/20 19:08:42, mmenke wrote:
> What do we really get from the RDH delegate in this test?

It merely checks that the function is called, and I think it's enough. Don't you
think so?

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 5 months ago (2016-07-22 04:34:27 UTC) #144

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/1780001

4 years, 5 months ago (2016-07-22 04:35:10 UTC) #145

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 5 months ago (2016-07-22 06:40:40 UTC) #146

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/260359)

4 years, 5 months ago (2016-07-22 06:40:42 UTC) #147

mmenke

Sorry for the delay, gotten bogged down a bit in this and Camille's CLs. I'm ...

4 years, 5 months ago (2016-07-25 20:34:51 UTC) #148

mmenke

https://codereview.chromium.org/1970693002/diff/1720001/content/browser/loader/mojo_async_resource_handler.cc File content/browser/loader/mojo_async_resource_handler.cc (right): https://codereview.chromium.org/1970693002/diff/1720001/content/browser/loader/mojo_async_resource_handler.cc#newcode284 content/browser/loader/mojo_async_resource_handler.cc:284: DCHECK(status.status() != net::URLRequestStatus::IO_PENDING); DCHECK_NE https://codereview.chromium.org/1970693002/diff/1720001/content/browser/loader/mojo_async_resource_handler_unittest.cc File content/browser/loader/mojo_async_resource_handler_unittest.cc (right): https://codereview.chromium.org/1970693002/diff/1720001/content/browser/loader/mojo_async_resource_handler_unittest.cc#newcode314 ...

4 years, 5 months ago (2016-07-25 22:03:54 UTC) #149

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

https://codereview.chromium.org/1970693002/diff/1720001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.cc:284:
DCHECK(status.status() != net::URLRequestStatus::IO_PENDING);
DCHECK_NE

https://codereview.chromium.org/1970693002/diff/1720001/content/browser/loade...
File content/browser/loader/mojo_async_resource_handler_unittest.cc (right):

https://codereview.chromium.org/1970693002/diff/1720001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:314: const size_t
contents_size = 3 * buf_size + 2;
On 2016/07/21 14:50:21, yhirano wrote:
> On 2016/07/20 19:08:42, mmenke wrote:
> > contents_size = body_size?
> 
> Sorry I don't understand the comment.

Sorry, should have been clearer.  Was suggesting a rename ("contents_size" seems
less clear).  Moot now, anyways.

https://codereview.chromium.org/1970693002/diff/1720001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:454:
EXPECT_TRUE(rdh_delegate.is_on_response_started_called());
On 2016/07/21 14:50:21, yhirano wrote:
> On 2016/07/20 19:08:42, mmenke wrote:
> > What do we really get from the RDH delegate in this test?
> 
> It merely checks that the function is called, and I think it's enough. Don't
you
> think so?

Ah, for some reason, I was thinking this was called by the RDH, not the
ResourceHandler.

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

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.h:71:
ResourceRequestInfoImpl* GetRequestInfoForTesting() {
Do we really need this?  Seems like since we have the URLRequest, we can just
use ResourceRequestInfoImpl::ForRequest(request_.get()); in tests, and so not
touch production code.

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
File content/browser/loader/mojo_async_resource_handler_test_util.h (right):

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_test_util.h:22: class
FakeURLLoaderClient final : public mojom::URLLoaderClient {
On 2016/07/21 14:50:20, yhirano wrote:
> On 2016/07/20 19:08:42, mmenke wrote:
> > Is this really a fake client?  I guess it doesn't drive the reading of the
> > body...
> 
> I don't feel it so unnatural but maybe my wording is not good. What should we
> call this class instead? Stub?

The thing is this is a real URLLoaderClient - URLLoaderClient doesn't impose any
behaviors on the client that we're only pretending to do.  So "Fake" seems a
misnomer to me.  More generally, I'd never use the term "Fake" for a class that
implements a pure observer interface, that comes with no contracts in how it
behaves in response to events.

I'd go with "Test", myself (Which is nice and generic and unenlightening,
handily avoiding any such complaints), but I'm perfectly happy with "Stub".

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
File content/browser/loader/mojo_async_resource_handler_unittest.cc (right):

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:167: bool
is_on_response_started_called_ = false;
is -> was?

Or better, make this an integer, and call it on_response_started_called_count_,
so can catch unexpected calls.  Could even check in the destructor that it
hasn't been changed since the last call to the accessor call, if we want to be
really paranoid.

Or could just add an EXPECT_FALSE in OnResponseStarted, to make sure it's not
called twice.  (Destructor check would still potentially be useful in that case)

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:169:
DISALLOW_COPY_AND_ASSIGN(FakeResourceDispatcherHostDelegate);
include base/macros.h

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:287:
rdh_.SetDelegate(nullptr);
Should we just make rdh_delegate_ part of the test fixture, and always set it
up?  May be worth checking it gets called (And, just as importantly, not called
unexpectedly) in a few more tests....

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:344:
base::TimeTicks now1 = base::TimeTicks::Now();
Call OnWillStart?

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:356:
RunUntilNextNotification();
What do you think of more specific methods for these? 
url_loader_client_.RunUntilResponseReceived(), etc?

Not a big ran of just running until some miscellaneous something happens in one
of two classes.

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:365:
TEST_F(MojoAsyncResourceHandlerTest, OnWillStart) {
Seems like this should be first - it's always called before other methods.

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:373:
EXPECT_TRUE(handler_->OnBeforeNetworkStart(request_->url(), &defer));
Now this method really has been removed, honest!  :)

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:381:
EXPECT_TRUE(handler_->OnWillRead(&buffer, &buf_size, -1));
Just calling this method seems weird - normally we call OnWillStart,
OnResponseStarted, and then OnWillRead.

Or with mime sniffing, we call OnWillStart, OnWillRead, OnResponseStarted.

Should test both variants (And preferably run to completion, with at least one
successful read, through both paths).

Testing the actual implementation of the API minimizes this code's dependence on
the MojoHandler's implementation details, instead, we're depending more on
whether it implements the spec.

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:392:
EXPECT_FALSE(handler_->OnWillRead(&buffer, &buf_size, -1));
Not going to comment for all of these, but think they should all go through
OnWillStart+OnResponseStarted.

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:420: std::string
contents;
include <string>?

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:422: char
buffer[16];
There's another local called buffer outside this loop - should avoid identical
variable names.

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:423: uint32_t
read = sizeof(buffer);
include stdint.h?

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:423: uint32_t
read = sizeof(buffer);
sizeof => arraysize?

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:427: if (result
== MOJO_RESULT_SHOULD_WAIT)
Is there any spec on what we have to do if we get this result?  It seems weird
to not even spin the message loop if we get it.

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:430: } while
(contents.size() < 2);
optional:  Suggest switching to a while() loop - they're much more common, and
as a result, easier for the average Chrome dev to follow.

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:459: char
buffer[16];
There's another local called buffer outside this loop - should avoid identical
variable names.

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:468: } while
(contents.size() < data.size());
Again, suggest a while loop

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:615:
TEST_F(MojoAsyncResourceHandlerTest, ResponseErrorDuringBodyTransmission) {
Should we have a failure like this, but where the failure is without the
OnReadCompleted call?  I think that's a more interesting difference than with
the above test.

Could also do a cancellation in the middle of a completion deferal, and make
sure we stop listening to learn the pipe is ready for more data.

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:669:
handler_->set_begin_write_expectation(MOJO_RESULT_SHOULD_WAIT);
Seems like we should simulate both the case where the handler is only ready
after the read completes, and the handler is ready before the read completes,
and make sure the consumer gets the data in both cases.

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-07-26 06:49:50 UTC) #150

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/1800001

4 years, 4 months ago (2016-07-26 06:50:07 UTC) #151

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-07-26 08:32:42 UTC) #152

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 4 months ago (2016-07-26 08:32:44 UTC) #153

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-07-27 06:52:51 UTC) #154

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/1820001

4 years, 4 months ago (2016-07-27 06:53:09 UTC) #155

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds/42070) ios-device-gn on ...

4 years, 4 months ago (2016-07-27 06:54:59 UTC) #157

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-07-27 06:58:14 UTC) #158

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/1840001

4 years, 4 months ago (2016-07-27 06:58:34 UTC) #159

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-07-27 09:07:52 UTC) #160

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/1860001

4 years, 4 months ago (2016-07-27 09:08:06 UTC) #161

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-07-27 10:38:58 UTC) #162

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 4 months ago (2016-07-27 10:39:00 UTC) #163

yhirano

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loader/mojo_async_resource_handler.h File content/browser/loader/mojo_async_resource_handler.h (right): https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loader/mojo_async_resource_handler.h#newcode71 content/browser/loader/mojo_async_resource_handler.h:71: ResourceRequestInfoImpl* GetRequestInfoForTesting() { On 2016/07/25 22:03:53, mmenke wrote: > ...

4 years, 4 months ago (2016-07-27 16:13:29 UTC) #164

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

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler.h:71:
ResourceRequestInfoImpl* GetRequestInfoForTesting() {
On 2016/07/25 22:03:53, mmenke wrote:
> Do we really need this?  Seems like since we have the URLRequest, we can just
> use ResourceRequestInfoImpl::ForRequest(request_.get()); in tests, and so not
> touch production code.

Thanks, removed.

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
File content/browser/loader/mojo_async_resource_handler_test_util.h (right):

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_test_util.h:22: class
FakeURLLoaderClient final : public mojom::URLLoaderClient {
On 2016/07/25 22:03:53, mmenke wrote:
> On 2016/07/21 14:50:20, yhirano wrote:
> > On 2016/07/20 19:08:42, mmenke wrote:
> > > Is this really a fake client?  I guess it doesn't drive the reading of the
> > > body...
> > 
> > I don't feel it so unnatural but maybe my wording is not good. What should
we
> > call this class instead? Stub?
> 
> The thing is this is a real URLLoaderClient - URLLoaderClient doesn't impose
any
> behaviors on the client that we're only pretending to do.  So "Fake" seems a
> misnomer to me.  More generally, I'd never use the term "Fake" for a class
that
> implements a pure observer interface, that comes with no contracts in how it
> behaves in response to events.
> 
> I'd go with "Test", myself (Which is nice and generic and unenlightening,
> handily avoiding any such complaints), but I'm perfectly happy with "Stub".

Done.

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
File content/browser/loader/mojo_async_resource_handler_unittest.cc (right):

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:167: bool
is_on_response_started_called_ = false;
On 2016/07/25 22:03:54, mmenke wrote:
> is -> was?
> 
> Or better, make this an integer, and call it
on_response_started_called_count_,
> so can catch unexpected calls.  Could even check in the destructor that it
> hasn't been changed since the last call to the accessor call, if we want to be
> really paranoid.
> 
> Or could just add an EXPECT_FALSE in OnResponseStarted, to make sure it's not
> called twice.  (Destructor check would still potentially be useful in that
case)

Done.

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:169:
DISALLOW_COPY_AND_ASSIGN(FakeResourceDispatcherHostDelegate);
On 2016/07/25 22:03:54, mmenke wrote:
> include base/macros.h

My understanding is mojo_async_resource_handler.h is the related header of this
file in terms of [1] (it's included at the top of the file). Include directives
in the related file is not needed to be repeated [2]. As base/macros.h is
included in mojo_async_resource_handler.h, I think we don't need to include it
here.

1: https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:287:
rdh_.SetDelegate(nullptr);
On 2016/07/25 22:03:54, mmenke wrote:
> Should we just make rdh_delegate_ part of the test fixture, and always set it
> up?  May be worth checking it gets called (And, just as importantly, not
called
> unexpectedly) in a few more tests....

Done.

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:356:
RunUntilNextNotification();
On 2016/07/25 22:03:54, mmenke wrote:
> What do you think of more specific methods for these? 
> url_loader_client_.RunUntilResponseReceived(), etc?
> 
> Not a big ran of just running until some miscellaneous something happens in
one
> of two classes.

Done.

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:365:
TEST_F(MojoAsyncResourceHandlerTest, OnWillStart) {
On 2016/07/25 22:03:53, mmenke wrote:
> Seems like this should be first - it's always called before other methods.

Done.

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:373:
EXPECT_TRUE(handler_->OnBeforeNetworkStart(request_->url(), &defer));
On 2016/07/25 22:03:54, mmenke wrote:
> Now this method really has been removed, honest!  :)

Done.

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:381:
EXPECT_TRUE(handler_->OnWillRead(&buffer, &buf_size, -1));
On 2016/07/25 22:03:54, mmenke wrote:
> Just calling this method seems weird - normally we call OnWillStart,
> OnResponseStarted, and then OnWillRead.
> 
> Or with mime sniffing, we call OnWillStart, OnWillRead, OnResponseStarted.
> 
> Should test both variants (And preferably run to completion, with at least one
> successful read, through both paths).
> 
> Testing the actual implementation of the API minimizes this code's dependence
on
> the MojoHandler's implementation details, instead, we're depending more on
> whether it implements the spec.

Having more tests sounds a good idea. Done.

I think it's OK to call OnWillRead without calling OnWillStart in this file,
though. Actually tests in this file already extremely depend on
MojoAsyncResourceHandler's implementation (e.g., a certain BeginWriteData
failure leads to a certain result or side-effect). Additionally, to keep high
code coverage, it is important to keep tests simple, more focused on each code
fragment.

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:420: std::string
contents;
On 2016/07/25 22:03:54, mmenke wrote:
> include <string>?

It's included in mojo_async_resource_handler.h.

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:422: char
buffer[16];
On 2016/07/25 22:03:53, mmenke wrote:
> There's another local called buffer outside this loop - should avoid identical
> variable names.

Done.

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:423: uint32_t
read = sizeof(buffer);
On 2016/07/25 22:03:54, mmenke wrote:
> include stdint.h?

It's included in mojo_async_resource_handler.h.

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:423: uint32_t
read = sizeof(buffer);
On 2016/07/25 22:03:54, mmenke wrote:
> sizeof => arraysize?

sizeof is more preferable here, as the argument represents the size of the
region.

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:427: if (result
== MOJO_RESULT_SHOULD_WAIT)
On 2016/07/25 22:03:54, mmenke wrote:
> Is there any spec on what we have to do if we get this result?  It seems weird
> to not even spin the message loop if we get it.

In production code, we use mojo::Watcher to be notified when the handle becomes
readable, but we cannot use it here because we need to stay in the test case.
spinning the message loop is a good idea.

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:430: } while
(contents.size() < 2);
On 2016/07/25 22:03:53, mmenke wrote:
> optional:  Suggest switching to a while() loop - they're much more common, and
> as a result, easier for the average Chrome dev to follow.

Done.

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:459: char
buffer[16];
On 2016/07/25 22:03:53, mmenke wrote:
> There's another local called buffer outside this loop - should avoid identical
> variable names.

Done.

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:468: } while
(contents.size() < data.size());
On 2016/07/25 22:03:53, mmenke wrote:
> Again, suggest a while loop

Done.

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:615:
TEST_F(MojoAsyncResourceHandlerTest, ResponseErrorDuringBodyTransmission) {
On 2016/07/25 22:03:54, mmenke wrote:
> Should we have a failure like this, but where the failure is without the
> OnReadCompleted call?  I think that's a more interesting difference than with
> the above test.
> 
Done.


> Could also do a cancellation in the middle of a completion deferal, and make
> sure we stop listening to learn the pipe is ready for more data.

BeginWriteFailsOnResume checks it, I think.

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:669:
handler_->set_begin_write_expectation(MOJO_RESULT_SHOULD_WAIT);
On 2016/07/25 22:03:54, mmenke wrote:
> Seems like we should simulate both the case where the handler is only ready
> after the read completes, and the handler is ready before the read completes,
> and make sure the consumer gets the data in both cases.

Sorry, what do you mean by "handler is ready"?

mmenke

Response to comments. Plan to do a pass today - hoping we can finally finish ...

4 years, 4 months ago (2016-07-27 16:49:36 UTC) #165

Response to comments.  Plan to do a pass today - hoping we can finally finish
this up this week (Modulo waiting on clamy's CL, which I also want to finish up
this week)

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
File content/browser/loader/mojo_async_resource_handler_unittest.cc (right):

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:169:
DISALLOW_COPY_AND_ASSIGN(FakeResourceDispatcherHostDelegate);
On 2016/07/27 16:13:29, yhirano wrote:
> On 2016/07/25 22:03:54, mmenke wrote:
> > include base/macros.h
> 
> My understanding is mojo_async_resource_handler.h is the related header of
this
> file in terms of [1] (it's included at the top of the file). Include
directives
> in the related file is not needed to be repeated [2]. As base/macros.h is
> included in mojo_async_resource_handler.h, I think we don't need to include it
> here.
> 
> 1:
https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes

Hrm...I've always considered foo.h the related header for only foo.cc (Well,
include foo.h first in foo_unittest.cc, but otherwise, treat it like a normal
header.)

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:381:
EXPECT_TRUE(handler_->OnWillRead(&buffer, &buf_size, -1));
On 2016/07/27 16:13:28, yhirano wrote:
> On 2016/07/25 22:03:54, mmenke wrote:
> > Just calling this method seems weird - normally we call OnWillStart,
> > OnResponseStarted, and then OnWillRead.
> > 
> > Or with mime sniffing, we call OnWillStart, OnWillRead, OnResponseStarted.
> > 
> > Should test both variants (And preferably run to completion, with at least
one
> > successful read, through both paths).
> > 
> > Testing the actual implementation of the API minimizes this code's
dependence
> on
> > the MojoHandler's implementation details, instead, we're depending more on
> > whether it implements the spec.
> 
> Having more tests sounds a good idea. Done.
> 
> I think it's OK to call OnWillRead without calling OnWillStart in this file,
> though. Actually tests in this file already extremely depend on
> MojoAsyncResourceHandler's implementation (e.g., a certain BeginWriteData
> failure leads to a certain result or side-effect). Additionally, to keep high
> code coverage, it is important to keep tests simple, more focused on each code
> fragment.

I really think we should mimic real world usage as closely as we can - it makes
these tests less fragile when modifying / refactoring AsyncMojoResourceHandler. 
To put it bluntly, as the one most likely to either review refactoring of this
file or make them himself, I'm not going to sign off on this CL unless we follow
the expected order of ResourceHandler calls more closely.

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:669:
handler_->set_begin_write_expectation(MOJO_RESULT_SHOULD_WAIT);
On 2016/07/27 16:13:28, yhirano wrote:
> On 2016/07/25 22:03:54, mmenke wrote:
> > Seems like we should simulate both the case where the handler is only ready
> > after the read completes, and the handler is ready before the read
completes,
> > and make sure the consumer gets the data in both cases.
> 
> Sorry, what do you mean by "handler is ready"?

Erm...  the mojo channel thing is ready for data.

We try to get a buffer from mojo, mojo is tells us to come back later.  We then
read from the network stack into our own memory buffer buffer.  Then that
completes.

Now, we try to get a buffer from mojo again.  Mojo either gives us memory, or
tells us to come back later and we start watching mojo.  We should test both
cases.

mmenke

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loader/mojo_async_resource_handler_unittest.cc File content/browser/loader/mojo_async_resource_handler_unittest.cc (right): https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loader/mojo_async_resource_handler_unittest.cc#newcode615 content/browser/loader/mojo_async_resource_handler_unittest.cc:615: TEST_F(MojoAsyncResourceHandlerTest, ResponseErrorDuringBodyTransmission) { On 2016/07/27 16:13:28, yhirano wrote: > ...

4 years, 4 months ago (2016-07-27 21:38:21 UTC) #166

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-07-28 11:26:19 UTC) #167

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/1880001

4 years, 4 months ago (2016-07-28 11:26:40 UTC) #168

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-07-28 12:42:56 UTC) #169

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 4 months ago (2016-07-28 12:42:59 UTC) #170

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-07-28 13:27:48 UTC) #171

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/1900001

4 years, 4 months ago (2016-07-28 13:28:03 UTC) #172

yhirano

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loader/mojo_async_resource_handler_unittest.cc File content/browser/loader/mojo_async_resource_handler_unittest.cc (right): https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loader/mojo_async_resource_handler_unittest.cc#newcode381 content/browser/loader/mojo_async_resource_handler_unittest.cc:381: EXPECT_TRUE(handler_->OnWillRead(&buffer, &buf_size, -1)); On 2016/07/27 16:49:36, mmenke wrote: > ...

4 years, 4 months ago (2016-07-28 14:50:46 UTC) #173

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
File content/browser/loader/mojo_async_resource_handler_unittest.cc (right):

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:381:
EXPECT_TRUE(handler_->OnWillRead(&buffer, &buf_size, -1));
On 2016/07/27 16:49:36, mmenke wrote:
> On 2016/07/27 16:13:28, yhirano wrote:
> > On 2016/07/25 22:03:54, mmenke wrote:
> > > Just calling this method seems weird - normally we call OnWillStart,
> > > OnResponseStarted, and then OnWillRead.
> > > 
> > > Or with mime sniffing, we call OnWillStart, OnWillRead, OnResponseStarted.
> > > 
> > > Should test both variants (And preferably run to completion, with at least
> one
> > > successful read, through both paths).
> > > 
> > > Testing the actual implementation of the API minimizes this code's
> dependence
> > on
> > > the MojoHandler's implementation details, instead, we're depending more on
> > > whether it implements the spec.
> > 
> > Having more tests sounds a good idea. Done.
> > 
> > I think it's OK to call OnWillRead without calling OnWillStart in this file,
> > though. Actually tests in this file already extremely depend on
> > MojoAsyncResourceHandler's implementation (e.g., a certain BeginWriteData
> > failure leads to a certain result or side-effect). Additionally, to keep
high
> > code coverage, it is important to keep tests simple, more focused on each
code
> > fragment.
> 
> I really think we should mimic real world usage as closely as we can - it
makes
> these tests less fragile when modifying / refactoring
AsyncMojoResourceHandler. 
> To put it bluntly, as the one most likely to either review refactoring of this
> file or make them himself, I'm not going to sign off on this CL unless we
follow
> the expected order of ResourceHandler calls more closely.

Done.

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:615:
TEST_F(MojoAsyncResourceHandlerTest, ResponseErrorDuringBodyTransmission) {
On 2016/07/27 21:38:21, mmenke wrote:
> On 2016/07/27 16:13:28, yhirano wrote:
> > On 2016/07/25 22:03:54, mmenke wrote:
> > > Should we have a failure like this, but where the failure is without the
> > > OnReadCompleted call?  I think that's a more interesting difference than
> with
> > > the above test.
> > > 
> > Done.
> > 
> > 
> > > Could also do a cancellation in the middle of a completion deferal, and
make
> > > sure we stop listening to learn the pipe is ready for more data.
> > 
> > BeginWriteFailsOnResume checks it, I think.
> 
> I don't think it does - in that test, we fail when we try to get memory from
> mojo.  What I'm thinking of is we get told to wait for memory to be available.

> Then the request is cancelled (For some external navigation).  We should make
> sure we stop listening for the mojo ready notification.  Otherwise, if we get
> one and start trying to read Bad Things may happen.

Hmm, is CancelWhileWaiting what you want?

I have a question: Is it allowed to call OnResponseCompleted even when the
handler is in the deferred state (i.e., the last call set |*defer| true and
Resume has not been called since then)? It's not clear to me from
ResourceHandler's comments.

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:669:
handler_->set_begin_write_expectation(MOJO_RESULT_SHOULD_WAIT);
On 2016/07/27 16:49:36, mmenke wrote:
> On 2016/07/27 16:13:28, yhirano wrote:
> > On 2016/07/25 22:03:54, mmenke wrote:
> > > Seems like we should simulate both the case where the handler is only
ready
> > > after the read completes, and the handler is ready before the read
> completes,
> > > and make sure the consumer gets the data in both cases.
> > 
> > Sorry, what do you mean by "handler is ready"?
> 
> Erm...  the mojo channel thing is ready for data.
> 
> We try to get a buffer from mojo, mojo is tells us to come back later.  We
then
> read from the network stack into our own memory buffer buffer.  Then that
> completes.
> 
> Now, we try to get a buffer from mojo again.  Mojo either gives us memory, or
> tells us to come back later and we start watching mojo.  We should test both
> cases.

Added BeginWriteReturnsShouldWaitOnWillReadAndThenReturnsOK.

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-07-28 14:54:30 UTC) #174

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 4 months ago (2016-07-28 14:54:33 UTC) #175

mmenke

Sorry for not doing a second pass this week. Between this CL and clamy's, just ...

4 years, 4 months ago (2016-07-29 20:01:15 UTC) #176

Sorry for not doing a second pass this week.  Between this CL and clamy's, just
got burnt out on /loader/ reviews.  Just did my final full pass on clamy's,
though (At long last!), so can focus on getting this one out the door next week.

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
File content/browser/loader/mojo_async_resource_handler_unittest.cc (right):

https://codereview.chromium.org/1970693002/diff/1780001/content/browser/loade...
content/browser/loader/mojo_async_resource_handler_unittest.cc:615:
TEST_F(MojoAsyncResourceHandlerTest, ResponseErrorDuringBodyTransmission) {
On 2016/07/28 14:50:45, yhirano wrote:
> On 2016/07/27 21:38:21, mmenke wrote:
> > On 2016/07/27 16:13:28, yhirano wrote:
> > > On 2016/07/25 22:03:54, mmenke wrote:
> > > > Should we have a failure like this, but where the failure is without the
> > > > OnReadCompleted call?  I think that's a more interesting difference than
> > with
> > > > the above test.
> > > > 
> > > Done.
> > > 
> > > 
> > > > Could also do a cancellation in the middle of a completion deferal, and
> make
> > > > sure we stop listening to learn the pipe is ready for more data.
> > > 
> > > BeginWriteFailsOnResume checks it, I think.
> > 
> > I don't think it does - in that test, we fail when we try to get memory from
> > mojo.  What I'm thinking of is we get told to wait for memory to be
available.
> 
> > Then the request is cancelled (For some external navigation).  We should
make
> > sure we stop listening for the mojo ready notification.  Otherwise, if we
get
> > one and start trying to read Bad Things may happen.
> 
> Hmm, is CancelWhileWaiting what you want?

Looks like it.

> I have a question: Is it allowed to call OnResponseCompleted even when the
> handler is in the deferred state (i.e., the last call set |*defer| true and
> Resume has not been called since then)? It's not clear to me from
> ResourceHandler's comments.

The ResourceHandler API is underspecified, and random ResourceHandlers do random
weird things, unfortunately.  Regardless of whether it's allowed or not, it
happens.  There's no code to hold back a cancellation message while waiting on a
deferred request, and we destroy requests after cancelling them when a renderer
goes away.  Other random things can cancel them at any point, too, so I don't
think it's safe to assume deletion happens synchronously after a cancellation.

mmenke

I think this looks good enough to land, modulo the mime sniffer issue with not ...

4 years, 4 months ago (2016-08-01 18:23:17 UTC) #177

mmenke

On 2016/08/01 18:23:17, mmenke wrote: > I think this looks good enough to land, modulo ...

4 years, 4 months ago (2016-08-01 18:24:46 UTC) #178

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-02 04:14:39 UTC) #179

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/1920001

4 years, 4 months ago (2016-08-02 04:14:57 UTC) #180

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-02 04:28:58 UTC) #181

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_android/builds/105151) linux_chromium_chromeos_ozone_rel_ng on ...

4 years, 4 months ago (2016-08-02 04:29:00 UTC) #182

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-02 08:29:46 UTC) #183

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/1940001

4 years, 4 months ago (2016-08-02 08:29:58 UTC) #184

yhirano

Thank you very much for the thorough review. I would go with the last option.

4 years, 4 months ago (2016-08-02 08:30:19 UTC) #185

yhirano

yhirano@chromium.org changed reviewers: + tsepez@chromium.org

4 years, 4 months ago (2016-08-02 08:31:43 UTC) #186

yhirano

jam@ or kinuko@, are you happy with the change as a content/ owner? Sorry the ...

4 years, 4 months ago (2016-08-02 08:33:14 UTC) #188

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-02 10:11:52 UTC) #189

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 4 months ago (2016-08-02 10:11:55 UTC) #190

kinuko

Sending some nit-pick comments, but overall looks pretty good to me. Let me take another ...

4 years, 4 months ago (2016-08-03 18:20:07 UTC) #192

mmenke

https://codereview.chromium.org/1970693002/diff/1940001/content/browser/loader/mojo_async_resource_handler.cc File content/browser/loader/mojo_async_resource_handler.cc (right): https://codereview.chromium.org/1970693002/diff/1940001/content/browser/loader/mojo_async_resource_handler.cc#newcode190 content/browser/loader/mojo_async_resource_handler.cc:190: is_using_io_buffer_not_from_writer_ = true; On 2016/08/03 18:20:07, kinuko wrote: > ...

4 years, 4 months ago (2016-08-03 18:25:57 UTC) #193

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-04 08:26:21 UTC) #194

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/1960001

4 years, 4 months ago (2016-08-04 08:26:53 UTC) #195

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-04 08:29:59 UTC) #196

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/builds/46305) mac_chromium_compile_dbg_ng on ...

4 years, 4 months ago (2016-08-04 08:30:02 UTC) #197

yhirano

https://codereview.chromium.org/1970693002/diff/1940001/content/browser/loader/mojo_async_resource_handler.cc File content/browser/loader/mojo_async_resource_handler.cc (right): https://codereview.chromium.org/1970693002/diff/1940001/content/browser/loader/mojo_async_resource_handler.cc#newcode32 content/browser/loader/mojo_async_resource_handler.cc:32: int g_allocation_size = MojoAsyncResourceHandler::kDefaultAllocationSize; On 2016/08/03 18:20:07, kinuko wrote: ...

4 years, 4 months ago (2016-08-04 12:50:51 UTC) #198

kinuko

Remaining comments (+ some questions), mostly minor. Thanks for working through this! https://codereview.chromium.org/1970693002/diff/1940001/content/browser/loader/mojo_async_resource_handler.cc File content/browser/loader/mojo_async_resource_handler.cc ...

4 years, 4 months ago (2016-08-04 16:07:03 UTC) #199

mmenke

https://codereview.chromium.org/1970693002/diff/1940001/content/browser/loader/mojo_async_resource_handler_unittest.cc File content/browser/loader/mojo_async_resource_handler_unittest.cc (right): https://codereview.chromium.org/1970693002/diff/1940001/content/browser/loader/mojo_async_resource_handler_unittest.cc#newcode501 content/browser/loader/mojo_async_resource_handler_unittest.cc:501: base::RunLoop().RunUntilIdle(); On 2016/08/04 16:07:02, kinuko wrote: > We call ...

4 years, 4 months ago (2016-08-04 16:12:44 UTC) #200

kinuko

https://codereview.chromium.org/1970693002/diff/1940001/content/browser/loader/mojo_async_resource_handler_unittest.cc File content/browser/loader/mojo_async_resource_handler_unittest.cc (right): https://codereview.chromium.org/1970693002/diff/1940001/content/browser/loader/mojo_async_resource_handler_unittest.cc#newcode501 content/browser/loader/mojo_async_resource_handler_unittest.cc:501: base::RunLoop().RunUntilIdle(); On 2016/08/04 16:12:44, mmenke wrote: > On 2016/08/04 ...

4 years, 4 months ago (2016-08-04 16:57:09 UTC) #201

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-05 08:59:06 UTC) #202

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/1980001

4 years, 4 months ago (2016-08-05 08:59:15 UTC) #203

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-05 09:12:23 UTC) #204

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng/builds/255945) linux_chromium_rel_ng on ...

4 years, 4 months ago (2016-08-05 09:12:25 UTC) #205

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-05 10:54:08 UTC) #206

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/2000001

4 years, 4 months ago (2016-08-05 10:54:24 UTC) #207

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-05 11:35:36 UTC) #208

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng/builds/255992)

4 years, 4 months ago (2016-08-05 11:35:39 UTC) #209

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-05 11:58:23 UTC) #210

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/2020001

4 years, 4 months ago (2016-08-05 11:58:45 UTC) #211

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-05 12:09:14 UTC) #212

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/2040001

4 years, 4 months ago (2016-08-05 12:09:27 UTC) #213

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-05 12:19:07 UTC) #214

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/2060001

4 years, 4 months ago (2016-08-05 12:19:21 UTC) #215

yhirano

https://codereview.chromium.org/1970693002/diff/1940001/content/browser/loader/mojo_async_resource_handler.cc File content/browser/loader/mojo_async_resource_handler.cc (right): https://codereview.chromium.org/1970693002/diff/1940001/content/browser/loader/mojo_async_resource_handler.cc#newcode188 content/browser/loader/mojo_async_resource_handler.cc:188: } On 2016/08/04 16:07:02, kinuko wrote: > nit: I ...

4 years, 4 months ago (2016-08-05 12:21:40 UTC) #216

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-05 12:46:59 UTC) #217

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel_ng/builds/256650)

4 years, 4 months ago (2016-08-05 12:47:02 UTC) #218

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-08 01:59:52 UTC) #219

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/2080001

4 years, 4 months ago (2016-08-08 02:00:04 UTC) #220

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-08 02:32:17 UTC) #221

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_ozone_rel_ng/builds/213902)

4 years, 4 months ago (2016-08-08 02:32:20 UTC) #222

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-08 04:41:44 UTC) #223

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/2100001

4 years, 4 months ago (2016-08-08 04:42:02 UTC) #224

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-08 05:18:02 UTC) #225

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linux/builds/203186)

4 years, 4 months ago (2016-08-08 05:18:05 UTC) #226

kinuko

lgtm! https://codereview.chromium.org/1970693002/diff/1940001/content/child/url_response_body_consumer.h File content/child/url_response_body_consumer.h (right): https://codereview.chromium.org/1970693002/diff/1940001/content/child/url_response_body_consumer.h#newcode37 content/child/url_response_body_consumer.h:37: // 1) The completion status is set by ...

4 years, 4 months ago (2016-08-08 15:47:25 UTC) #227

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-09 06:00:56 UTC) #228

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/2120001

4 years, 4 months ago (2016-08-09 06:01:23 UTC) #229

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-09 06:05:51 UTC) #230

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm64_dbg_recipe/builds/109013) android_compile_dbg on ...

4 years, 4 months ago (2016-08-09 06:05:54 UTC) #231

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-09 06:08:21 UTC) #232

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/2140001

4 years, 4 months ago (2016-08-09 06:08:46 UTC) #233

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-09 06:12:16 UTC) #234

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm64_dbg_recipe/builds/109018) android_clang_dbg_recipe on ...

4 years, 4 months ago (2016-08-09 06:12:18 UTC) #235

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-09 06:15:32 UTC) #236

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/2160001

4 years, 4 months ago (2016-08-09 06:16:02 UTC) #237

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-09 06:22:17 UTC) #238

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/2180001

4 years, 4 months ago (2016-08-09 06:22:47 UTC) #239

yhirano

https://codereview.chromium.org/1970693002/diff/1940001/content/child/url_response_body_consumer.h File content/child/url_response_body_consumer.h (right): https://codereview.chromium.org/1970693002/diff/1940001/content/child/url_response_body_consumer.h#newcode37 content/child/url_response_body_consumer.h:37: // 1) The completion status is set by this ...

4 years, 4 months ago (2016-08-09 06:35:48 UTC) #240

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-09 06:38:46 UTC) #241

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_compile_dbg/builds/109031)

4 years, 4 months ago (2016-08-09 06:38:48 UTC) #242

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-09 06:49:19 UTC) #243

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/2200001

4 years, 4 months ago (2016-08-09 06:49:46 UTC) #244

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-09 07:31:36 UTC) #245

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linux/builds/204112)

4 years, 4 months ago (2016-08-09 07:31:39 UTC) #246

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-09 07:55:32 UTC) #247

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/2220001

4 years, 4 months ago (2016-08-09 07:56:13 UTC) #248

yhirano

Thank you very much, all reviewers! URLLoaderFactoryHolder is no longer needed as we have InterfaceRegistry ...

4 years, 4 months ago (2016-08-09 09:13:36 UTC) #249

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-09 09:14:18 UTC) #250

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/2240001

4 years, 4 months ago (2016-08-09 09:14:49 UTC) #251

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-09 11:12:36 UTC) #252

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/2260001

4 years, 4 months ago (2016-08-09 11:12:54 UTC) #253

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-09 13:00:19 UTC) #254

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 4 months ago (2016-08-09 13:00:22 UTC) #255

kinuko

still lgtm https://codereview.chromium.org/1970693002/diff/2260001/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/2260001/content/browser/renderer_host/render_process_host_impl.cc#newcode1173 content/browser/renderer_host/render_process_host_impl.cc:1173: base::Bind(&URLLoaderFactoryImpl::Create, resource_message_filter_)); Cool :)

4 years, 4 months ago (2016-08-09 15:44:28 UTC) #256

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-10 01:47:14 UTC) #257

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/2260001

4 years, 4 months ago (2016-08-10 01:48:22 UTC) #258

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-10 01:56:59 UTC) #259

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/234536)

4 years, 4 months ago (2016-08-10 01:57:03 UTC) #260

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-10 02:29:15 UTC) #261

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/2280001

4 years, 4 months ago (2016-08-10 02:29:58 UTC) #262

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-10 02:49:08 UTC) #263

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_compile_dbg/builds/109775) linux_chromium_chromeos_compile_dbg_ng on ...

4 years, 4 months ago (2016-08-10 02:49:11 UTC) #264

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-10 02:55:46 UTC) #265

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/2300001

4 years, 4 months ago (2016-08-10 02:56:10 UTC) #266

yhirano

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org, tsepez@chromium.org, kinuko@chromium.org Link ...

4 years, 4 months ago (2016-08-10 03:20:45 UTC) #269

commit-bot: I haz the power

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

4 years, 4 months ago (2016-08-10 03:21:24 UTC) #270

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-10 03:32:38 UTC) #271

commit-bot: I haz the power

Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/234593)

4 years, 4 months ago (2016-08-10 03:32:44 UTC) #272

yhirano

yzshen@, are you OK with my change at mojo/public/tools/bindings/chromium_bindings_configuration.gni?

4 years, 4 months ago (2016-08-10 03:45:50 UTC) #273

yzshen1

On 2016/08/10 03:45:50, yhirano (slow) wrote: > yzshen@, are you OK with my change at ...

4 years, 4 months ago (2016-08-10 15:39:44 UTC) #274

commit-bot: I haz the power

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

4 years, 4 months ago (2016-08-10 16:10:25 UTC) #276

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-10 18:26:57 UTC) #277

commit-bot: I haz the power

Failed to apply patch for content/child/web_url_loader_impl.cc: While running git apply --index -3 -p1; error: patch ...

4 years, 4 months ago (2016-08-10 18:27:04 UTC) #278

Failed to apply patch for content/child/web_url_loader_impl.cc:
While running git apply --index -3 -p1;
  error: patch failed: content/child/web_url_loader_impl.cc:303
  Falling back to three-way merge...
  Applied patch to 'content/child/web_url_loader_impl.cc' with conflicts.
  U content/child/web_url_loader_impl.cc

Patch:       content/child/web_url_loader_impl.cc
Index: content/child/web_url_loader_impl.cc
diff --git a/content/child/web_url_loader_impl.cc
b/content/child/web_url_loader_impl.cc
index
24e9015628194b6d2e2da9ccca66d1222ed60de1..caf2dc3da1fd900a518b552c80f74609d9d612b8
100644
--- a/content/child/web_url_loader_impl.cc
+++ b/content/child/web_url_loader_impl.cc
@@ -36,6 +36,7 @@
 #include "content/common/resource_request_body_impl.h"
 #include "content/common/service_worker/service_worker_types.h"
 #include "content/common/ssl_status_serialization.h"
+#include "content/common/url_loader.mojom.h"
 #include "content/public/child/fixed_received_data.h"
 #include "content/public/child/request_peer.h"
 #include "content/public/common/browser_side_navigation_policy.h"
@@ -303,7 +304,8 @@ class WebURLLoaderImpl::Context : public
base::RefCounted<Context> {
 
   Context(WebURLLoaderImpl* loader,
           ResourceDispatcher* resource_dispatcher,
-          std::unique_ptr<blink::WebTaskRunner> task_runner);
+          std::unique_ptr<blink::WebTaskRunner> task_runner,
+          mojom::URLLoaderFactory* factory);
 
   WebURLLoaderClient* client() const { return client_; }
   void set_client(WebURLLoaderClient* client) { client_ = client; }
@@ -365,6 +367,8 @@ class WebURLLoaderImpl::Context : public
base::RefCounted<Context> {
   enum DeferState {NOT_DEFERRING, SHOULD_DEFER, DEFERRED_DATA};
   DeferState defers_loading_;
   int request_id_;
+
+  mojom::URLLoaderFactory* url_loader_factory_;
 };
 
 // A thin wrapper class for Context to ensure its lifetime while it is
@@ -399,14 +403,16 @@ class WebURLLoaderImpl::RequestPeerImpl : public
RequestPeer {
 WebURLLoaderImpl::Context::Context(
     WebURLLoaderImpl* loader,
     ResourceDispatcher* resource_dispatcher,
-    std::unique_ptr<blink::WebTaskRunner> web_task_runner)
+    std::unique_ptr<blink::WebTaskRunner> web_task_runner,
+    mojom::URLLoaderFactory* url_loader_factory)
     : loader_(loader),
       client_(NULL),
       resource_dispatcher_(resource_dispatcher),
       web_task_runner_(std::move(web_task_runner)),
       referrer_policy_(blink::WebReferrerPolicyDefault),
       defers_loading_(NOT_DEFERRING),
-      request_id_(-1) {}
+      request_id_(-1),
+      url_loader_factory_(url_loader_factory) {}
 
 void WebURLLoaderImpl::Context::Cancel() {
   TRACE_EVENT_WITH_FLOW0("loading", "WebURLLoaderImpl::Context::Cancel", this,
@@ -565,7 +571,8 @@ void WebURLLoaderImpl::Context::Start(const WebURLRequest&
request,
   if (sync_load_response) {
     DCHECK(defers_loading_ == NOT_DEFERRING);
     resource_dispatcher_->StartSync(
-        request_info, request_body.get(), sync_load_response);
+        request_info, request_body.get(), sync_load_response,
+        request.getLoadingIPCType(), url_loader_factory_);
     return;
   }
 
@@ -573,7 +580,8 @@ void WebURLLoaderImpl::Context::Start(const WebURLRequest&
request,
                          TRACE_EVENT_FLAG_FLOW_OUT);
   request_id_ = resource_dispatcher_->StartAsync(
       request_info, request_body.get(),
-      base::WrapUnique(new WebURLLoaderImpl::RequestPeerImpl(this)));
+      base::WrapUnique(new WebURLLoaderImpl::RequestPeerImpl(this)),
+      request.getLoadingIPCType(), url_loader_factory_);
 
   if (defers_loading_ != NOT_DEFERRING)
     resource_dispatcher_->SetDefersLoading(request_id_, true);
@@ -946,9 +954,12 @@ void WebURLLoaderImpl::RequestPeerImpl::OnCompletedRequest(
 
 WebURLLoaderImpl::WebURLLoaderImpl(
     ResourceDispatcher* resource_dispatcher,
-    std::unique_ptr<blink::WebTaskRunner> web_task_runner)
-    : context_(
-          new Context(this, resource_dispatcher, std::move(web_task_runner)))
{}
+    std::unique_ptr<blink::WebTaskRunner> web_task_runner,
+    mojom::URLLoaderFactory* url_loader_factory)
+    : context_(new Context(this,
+                           resource_dispatcher,
+                           std::move(web_task_runner),
+                           url_loader_factory)) {}
 
 WebURLLoaderImpl::~WebURLLoaderImpl() {
   cancel();

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-11 04:15:30 UTC) #279

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/2320001

4 years, 4 months ago (2016-08-11 04:15:53 UTC) #280

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-11 04:18:51 UTC) #281

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/275879)

4 years, 4 months ago (2016-08-11 04:18:54 UTC) #282

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-11 13:12:23 UTC) #283

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/2340001

4 years, 4 months ago (2016-08-11 13:12:36 UTC) #284

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-11 13:29:49 UTC) #285

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_compile_dbg_ng/builds/245103)

4 years, 4 months ago (2016-08-11 13:29:52 UTC) #286

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-11 13:40:41 UTC) #287

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/2360001

4 years, 4 months ago (2016-08-11 13:40:59 UTC) #288

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-11 14:34:15 UTC) #289

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng/builds/259376)

4 years, 4 months ago (2016-08-11 14:34:18 UTC) #290

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-11 14:52:43 UTC) #291

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/2380001

4 years, 4 months ago (2016-08-11 14:53:17 UTC) #292

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-11 15:07:57 UTC) #293

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/2400001

4 years, 4 months ago (2016-08-11 15:08:11 UTC) #294

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-11 16:44:59 UTC) #295

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/271908)

4 years, 4 months ago (2016-08-11 16:45:02 UTC) #296

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-12 01:09:08 UTC) #297

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/2400001

4 years, 4 months ago (2016-08-12 01:09:49 UTC) #298

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-12 01:15:35 UTC) #299

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/276815)

4 years, 4 months ago (2016-08-12 01:15:38 UTC) #300

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-12 01:35:08 UTC) #301

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/2420001

4 years, 4 months ago (2016-08-12 01:35:35 UTC) #302

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-12 02:33:56 UTC) #303

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/121355)

4 years, 4 months ago (2016-08-12 02:33:59 UTC) #304

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-12 08:54:11 UTC) #305

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/2440001

4 years, 4 months ago (2016-08-12 08:54:23 UTC) #306

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-12 11:12:13 UTC) #307

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 4 months ago (2016-08-12 11:12:15 UTC) #308

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-12 17:52:18 UTC) #309

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/2440001

4 years, 4 months ago (2016-08-12 17:52:53 UTC) #310

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-12 17:56:52 UTC) #311

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 4 months ago (2016-08-12 17:56:56 UTC) #312

yhirano

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, yzshen@chromium.org, tsepez@chromium.org, mmenke@chromium.org ...

4 years, 4 months ago (2016-08-12 23:28:54 UTC) #314

commit-bot: I haz the power

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

4 years, 4 months ago (2016-08-12 23:29:34 UTC) #315

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-12 23:38:36 UTC) #316

commit-bot: I haz the power

Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/237531)

4 years, 4 months ago (2016-08-12 23:38:41 UTC) #317

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-13 10:28:32 UTC) #318

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1970693002/2460001

4 years, 4 months ago (2016-08-13 10:28:47 UTC) #319

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-13 10:36:29 UTC) #320

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds/51683)

4 years, 4 months ago (2016-08-13 10:36:32 UTC) #321

yhirano

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, yzshen@chromium.org, tsepez@chromium.org, mmenke@chromium.org ...

4 years, 4 months ago (2016-08-13 12:09:27 UTC) #323

commit-bot: I haz the power

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

4 years, 4 months ago (2016-08-13 12:09:40 UTC) #324

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-13 12:16:51 UTC) #325

commit-bot: I haz the power

Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds/51689)

4 years, 4 months ago (2016-08-13 12:16:56 UTC) #326

commit-bot: I haz the power

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

4 years, 4 months ago (2016-08-13 12:43:01 UTC) #328

commit-bot: I haz the power

Description was changed from ========== Use mojo for Chrome Loading, Part 1 This CL introduces ...

4 years, 4 months ago (2016-08-13 12:50:28 UTC) #329

commit-bot: I haz the power

Committed patchset #90 (id:2460001)

4 years, 4 months ago (2016-08-13 12:50:34 UTC) #330

commit-bot: I haz the power

Description was changed from ========== Use mojo for Chrome Loading, Part 1 This CL introduces ...

4 years, 4 months ago (2016-08-13 12:52:05 UTC) #331

commit-bot: I haz the power

4 years, 4 months ago (2016-08-13 12:52:10 UTC) #332

Message was sent while issue was closed.

Patchset 90 (id:??) landed as
https://crrev.com/72f62279cd7c4370dd7e068b900d9359a7e9589c
Cr-Commit-Position: refs/heads/master@{#411894}

Issue 1970693002: Use mojo for Chrome Loading, Part 1 (Closed)