Bring back the URLLoader from the old Mandoline network service.

Bring back the URLLoader from the old Mandoline network service.

This was removed in r385900. The major changes are to make it work with the current URLLoader.mojom, and generally bring the code up to latest mojo changes.

BUG=598073
Review-Url: https://codereview.chromium.org/2817453002
Cr-Commit-Position: refs/heads/master@{#463886}
Committed: https://chromium.googlesource.com/chromium/src/+/6f02ddc2ac11955b536b5ee655e1a87d8b5605b5

[jam, 2017-04-11 17:55:03]

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

[commit-bot: I haz the power, 2017-04-11 17:56:03]

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

[jam, 2017-04-11 17:58:09]

jam@chromium.org changed reviewers:
+ yzshen@chromium.org

[commit-bot: I haz the power, 2017-04-11 18:02:30]

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

[commit-bot: I haz the power, 2017-04-11 18:02:31]

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_comp...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[jam, 2017-04-11 18:33:40]

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

[commit-bot: I haz the power, 2017-04-11 18:34:39]

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

[commit-bot: I haz the power, 2017-04-11 18:45:14]

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

[commit-bot: I haz the power, 2017-04-11 18:45:15]

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

[jam, 2017-04-11 20:18:06]

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

[commit-bot: I haz the power, 2017-04-11 20:21:55]

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

[commit-bot: I haz the power, 2017-04-11 20:33:50]

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

[commit-bot: I haz the power, 2017-04-11 20:33:51]

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_linu...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jam, 2017-04-11 20:34:06]

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

[commit-bot: I haz the power, 2017-04-11 20:34:45]

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

[commit-bot: I haz the power, 2017-04-11 21:42:34]

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

[commit-bot: I haz the power, 2017-04-11 21:42:35]

Dry run: This issue passed the CQ dry run.

[yzshen1, 2017-04-11 21:58:04]

https://codereview.chromium.org/2817453002/diff/1/content/network/BUILD.gn
File content/network/BUILD.gn (right):

https://codereview.chromium.org/2817453002/diff/1/content/network/BUILD.gn#ne...
content/network/BUILD.gn:25: "//services/service_manager/public/cpp",
Why this one is needed?

https://codereview.chromium.org/2817453002/diff/1/content/network/net_adapters.h
File content/network/net_adapters.h (right):

https://codereview.chromium.org/2817453002/diff/1/content/network/net_adapter...
content/network/net_adapters.h:21: //   MojoToNetPendingBuffer    --->     
MojoToNetIOBuffer
Please update the comment now that these two are removed.

https://codereview.chromium.org/2817453002/diff/20001/content/network/network...
File content/network/network_context.cc (right):

https://codereview.chromium.org/2817453002/diff/20001/content/network/network...
content/network/network_context.cc:102: : net_log_(new MojoNetLog),
It seems this member is not used?

https://codereview.chromium.org/2817453002/diff/20001/content/network/url_loa...
File content/network/url_loader_impl.cc (right):

https://codereview.chromium.org/2817453002/diff/20001/content/network/url_loa...
content/network/url_loader_impl.cc:211: // TODO
Please consider adding NOTIMPLEMENTED() and/or description for the TODO.

https://codereview.chromium.org/2817453002/diff/20001/content/network/url_loa...
content/network/url_loader_impl.cc:364: response_body_consumer_handle_.reset();
nit: no need to reset, it is already reset by the previous line which moved its
contents.

https://codereview.chromium.org/2817453002/diff/20001/content/network/url_loa...
File content/network/url_loader_unittest.cc (right):

https://codereview.chromium.org/2817453002/diff/20001/content/network/url_loa...
content/network/url_loader_unittest.cc:53: CHECK_EQ(MojoReadData(consumer,
buffer.data(), &num_bytes,
FYI, although this probably always works, there is actually no guarantee that
the data will be available on the first call. The readable signal may be
triggered for partial data, so a ALL_OR_NONE read will result in SHOULD_WAIT in
that case.

Therefore, we should use a while loop to wait and read data until we
successfully read the data out.

[jam, 2017-04-11 22:58:19]

https://codereview.chromium.org/2817453002/diff/1/content/network/BUILD.gn
File content/network/BUILD.gn (right):

https://codereview.chromium.org/2817453002/diff/1/content/network/BUILD.gn#ne...
content/network/BUILD.gn:25: "//services/service_manager/public/cpp",
On 2017/04/11 21:58:04, yzshen1 wrote:
> Why this one is needed?

It's not, removed

https://codereview.chromium.org/2817453002/diff/1/content/network/net_adapters.h
File content/network/net_adapters.h (right):

https://codereview.chromium.org/2817453002/diff/1/content/network/net_adapter...
content/network/net_adapters.h:21: //   MojoToNetPendingBuffer    --->     
MojoToNetIOBuffer
On 2017/04/11 21:58:04, yzshen1 wrote:
> Please update the comment now that these two are removed.

Done.

https://codereview.chromium.org/2817453002/diff/20001/content/network/network...
File content/network/network_context.cc (right):

https://codereview.chromium.org/2817453002/diff/20001/content/network/network...
content/network/network_context.cc:102: : net_log_(new MojoNetLog),
On 2017/04/11 21:58:04, yzshen1 wrote:
> It seems this member is not used?

The destructor does work

https://codereview.chromium.org/2817453002/diff/20001/content/network/url_loa...
File content/network/url_loader_impl.cc (right):

https://codereview.chromium.org/2817453002/diff/20001/content/network/url_loa...
content/network/url_loader_impl.cc:211: // TODO
On 2017/04/11 21:58:04, yzshen1 wrote:
> Please consider adding NOTIMPLEMENTED() and/or description for the TODO.

Done.

https://codereview.chromium.org/2817453002/diff/20001/content/network/url_loa...
content/network/url_loader_impl.cc:364: response_body_consumer_handle_.reset();
On 2017/04/11 21:58:04, yzshen1 wrote:
> nit: no need to reset, it is already reset by the previous line which moved
its
> contents.

Done.

https://codereview.chromium.org/2817453002/diff/20001/content/network/url_loa...
File content/network/url_loader_unittest.cc (right):

https://codereview.chromium.org/2817453002/diff/20001/content/network/url_loa...
content/network/url_loader_unittest.cc:53: CHECK_EQ(MojoReadData(consumer,
buffer.data(), &num_bytes,
On 2017/04/11 21:58:04, yzshen1 wrote:
> FYI, although this probably always works, there is actually no guarantee that
> the data will be available on the first call. The readable signal may be
> triggered for partial data, so a ALL_OR_NONE read will result in SHOULD_WAIT
in
> that case.
> 
> Therefore, we should use a while loop to wait and read data until we
> successfully read the data out.

Are you sure how it's used here can fail? Given that everything is running on
the same thread, and it's been flushed before a post task, I figured that it
would be ready by the time this is called. Other unit tests do this as well.

If it's not guaranteed, we will see flakiness and that can be the signal that
it's not working in practice :)

[jam, 2017-04-11 22:59:22]

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

[commit-bot: I haz the power, 2017-04-11 23:00:24]

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

[yzshen1, 2017-04-11 23:22:43]

lgtm

https://codereview.chromium.org/2817453002/diff/20001/content/network/url_loa...
File content/network/url_loader_unittest.cc (right):

https://codereview.chromium.org/2817453002/diff/20001/content/network/url_loa...
content/network/url_loader_unittest.cc:53: CHECK_EQ(MojoReadData(consumer,
buffer.data(), &num_bytes,
On 2017/04/11 22:58:19, jam wrote:
> On 2017/04/11 21:58:04, yzshen1 wrote:
> > FYI, although this probably always works, there is actually no guarantee
that
> > the data will be available on the first call. The readable signal may be
> > triggered for partial data, so a ALL_OR_NONE read will result in SHOULD_WAIT
> in
> > that case.
> > 
> > Therefore, we should use a while loop to wait and read data until we
> > successfully read the data out.
> 
> Are you sure how it's used here can fail? Given that everything is running on
> the same thread, and it's been flushed before a post task, I figured that it
> would be ready by the time this is called. Other unit tests do this as well.
> 
> If it's not guaranteed, we will see flakiness and that can be the signal that
> it's not working in practice :)

I am pretty sure that this relies on some undocumented behavior of the Mojo
system API.
Because people find example and copy/paste code, they probably will assume that
this is the right way to do things.

I don't think this test itself matters much though, I will let you decide
whether to leave it as is.

I heard Ken mentioned at some point that he would like to develop some data pipe
utility. Hopefully it will help people to write more robust code.

[jam, 2017-04-11 23:26:05]

The CQ bit was unchecked by jam@chromium.org

[jam, 2017-04-11 23:26:34]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2017-04-11 23:27:07]

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

[jam, 2017-04-12 00:33:14]

The CQ bit was checked by jam@chromium.org

[jam, 2017-04-12 00:33:15]

The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org
Link to the patchset: https://codereview.chromium.org/2817453002/#ps100001
(title: "fix checkdeps")

[commit-bot: I haz the power, 2017-04-12 00:34:04]

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

[commit-bot: I haz the power, 2017-04-12 01:43:54]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491957194986680,
"parent_rev": "81fd5881b8449937c1287452222df4dfeb407dd0", "commit_rev":
"6f02ddc2ac11955b536b5ee655e1a87d8b5605b5"}

[commit-bot: I haz the power, 2017-04-12 01:44:44]

Description was changed from

==========
Bring back the URLLoader from the old Mandoline network service.

This was removed in r385900. The major changes are to make it work with the
current URLLoader.mojom, and generally bring the code up to latest mojo changes.

BUG=598073
==========

to

==========
Bring back the URLLoader from the old Mandoline network service.

This was removed in r385900. The major changes are to make it work with the
current URLLoader.mojom, and generally bring the code up to latest mojo changes.

BUG=598073

Review-Url: https://codereview.chromium.org/2817453002
Cr-Commit-Position: refs/heads/master@{#463886}
Committed:
https://chromium.googlesource.com/chromium/src/+/6f02ddc2ac11955b536b5ee655e1...
==========

[commit-bot: I haz the power, 2017-04-12 01:44:45]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/6f02ddc2ac11955b536b5ee655e1...