network service: pass PlzNavigate resulting data via mojo data pipe

network service: pass PlzNavigate resulting data via mojo data pipe

Mostly Kinuko's work from https://codereview.chromium.org/2797443005/ !

For now, maintains the old/current path of passing the data from PlzNav
via a stream and adds the mojo data pipe as another option. This is to
avoid changing anything that might affect PlzNav performance, etc. while
it is in the middle of being launched.

BUG=598073, 705744
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2813243002
Cr-Commit-Position: refs/heads/master@{#464180}
Committed: https://chromium.googlesource.com/chromium/src/+/efb697309fb4c64a99e8c38b0448a4b4bb97886a

[scottmg, 2017-04-12 17:31:30]

Description was changed from

==========
WIP Not ready: nav hookup via mojo data pipe

Mostly kinuko's work from https://codereview.chromium.org/2797443005/!

BUG=598073,705744
==========

to

==========
WIP Not ready: nav hookup via mojo data pipe

Mostly kinuko's work from https://codereview.chromium.org/2797443005/!

BUG=598073,705744
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[scottmg, 2017-04-12 17:31:53]

Description was changed from

==========
WIP Not ready: nav hookup via mojo data pipe

Mostly kinuko's work from https://codereview.chromium.org/2797443005/!

BUG=598073,705744
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
WIP Not ready: nav hookup via mojo data pipe

Mostly kinuko's work from https://codereview.chromium.org/2797443005/ !

BUG=598073,705744
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[jam, 2017-04-12 18:13:45]

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

[jam, 2017-04-12 18:13:45]

Thanks Kinuko for writing this!

This looks good to me, but the only comment is I don't understand the changes in
web_url_loader_impl?

https://codereview.chromium.org/2813243002/diff/1/content/child/web_url_loade...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/2813243002/diff/1/content/child/web_url_loade...
content/child/web_url_loader_impl.cc:818: stream_override_->total_transferred +=
data_length;
I don't understand this block and the one below. Mainly, why isn't this needed
previously with just the mojo URL loading, and why we need it when we turn on
data pipe?

https://codereview.chromium.org/2813243002/diff/1/content/child/web_url_loade...
File content/child/web_url_loader_impl.h (right):

https://codereview.chromium.org/2813243002/diff/1/content/child/web_url_loade...
content/child/web_url_loader_impl.h:41: int total_transferred = 0;
nit: comment

https://codereview.chromium.org/2813243002/diff/1/content/common/frame_messag...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/2813243002/diff/1/content/common/frame_messag...
content/common/frame_messages.h:917: // Same as FrameMsg_CommitNavigation, but
used when --enable-network-service.
why not just one IPC with both parameters?

[scottmg, 2017-04-12 18:42:41]

https://codereview.chromium.org/2813243002/diff/1/content/child/web_url_loade...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/2813243002/diff/1/content/child/web_url_loade...
content/child/web_url_loader_impl.cc:818: stream_override_->total_transferred +=
data_length;
On 2017/04/12 18:13:45, jam wrote:
> I don't understand this block and the one below. Mainly, why isn't this needed
> previously with just the mojo URL loading, and why we need it when we turn on
> data pipe?

I didn't understand why this was needed either. I'll debug a bit to figure out
what's going on here.

https://codereview.chromium.org/2813243002/diff/1/content/common/frame_messag...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/2813243002/diff/1/content/common/frame_messag...
content/common/frame_messages.h:917: // Same as FrameMsg_CommitNavigation, but
used when --enable-network-service.
On 2017/04/12 18:13:45, jam wrote:
> why not just one IPC with both parameters?

Sure, that's probably better to match the rest.

[jam, 2017-04-12 20:36:32]

https://codereview.chromium.org/2813243002/diff/1/content/child/web_url_loade...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/2813243002/diff/1/content/child/web_url_loade...
content/child/web_url_loader_impl.cc:818: stream_override_->total_transferred +=
data_length;
On 2017/04/12 18:42:41, scottmg wrote:
> On 2017/04/12 18:13:45, jam wrote:
> > I don't understand this block and the one below. Mainly, why isn't this
needed
> > previously with just the mojo URL loading, and why we need it when we turn
on
> > data pipe?
> 
> I didn't understand why this was needed either. I'll debug a bit to figure out
> what's going on here.

(per chat) please add this comment, or something like it, cause this was not
intuitive :)

Since ResourceDispatcher::ContinueForNavigation called OnComplete immediately,
it didn't have the size of the resource immediately. So as data is read off the
data pipe, keep track of how much we're reading.




Also, to avoid changing behavior for non-network-service path, only do this
block and below when stream_url.is_empty() (i.e. using the data pipe).

[scottmg, 2017-04-12 20:37:12]

https://codereview.chromium.org/2813243002/diff/1/content/common/frame_messag...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/2813243002/diff/1/content/common/frame_messag...
content/common/frame_messages.h:917: // Same as FrameMsg_CommitNavigation, but
used when --enable-network-service.
On 2017/04/12 18:13:45, jam wrote:
> why not just one IPC with both parameters?

Done.

[scottmg, 2017-04-12 20:42:53]

Description was changed from

==========
WIP Not ready: nav hookup via mojo data pipe

Mostly kinuko's work from https://codereview.chromium.org/2797443005/ !

BUG=598073,705744
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
network service: pass PlzNavigate resulting data via mojo data pipe

Mostly kinuko's work from https://codereview.chromium.org/2797443005/ !

For now, maintains the old/current path of passing the data from PlzNav
via a stream and adds the mojo data pipe as another option. This is to
avoid changing anything that might affect PlzNav performance, etc. while
it is in the middle of being launched.

BUG=598073,705744
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[scottmg, 2017-04-12 20:43:10]

On 2017/04/12 20:36:32, jam wrote:
>
https://codereview.chromium.org/2813243002/diff/1/content/child/web_url_loade...
> File content/child/web_url_loader_impl.cc (right):
> 
>
https://codereview.chromium.org/2813243002/diff/1/content/child/web_url_loade...
> content/child/web_url_loader_impl.cc:818: stream_override_->total_transferred
+=
> data_length;
> On 2017/04/12 18:42:41, scottmg wrote:
> > On 2017/04/12 18:13:45, jam wrote:
> > > I don't understand this block and the one below. Mainly, why isn't this
> needed
> > > previously with just the mojo URL loading, and why we need it when we turn
> on
> > > data pipe?
> > 
> > I didn't understand why this was needed either. I'll debug a bit to figure
out
> > what's going on here.
> 
> (per chat) please add this comment, or something like it, cause this was not
> intuitive :)
> 
> Since ResourceDispatcher::ContinueForNavigation called OnComplete immediately,
> it didn't have the size of the resource immediately. So as data is read off
the
> data pipe, keep track of how much we're reading.
> 

Done.
 
> 
> 
> Also, to avoid changing behavior for non-network-service path, only do this
> block and below when stream_url.is_empty() (i.e. using the data pipe).

Done.

[scottmg, 2017-04-12 20:45:08]

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

[scottmg, 2017-04-12 20:45:09]

Daniel, could you take a look at the IPC?

    content\common\content_param_traits.h
    content\common\frame_messages.h
    ipc\ipc_mojo_param_traits.cc
    ipc\ipc_mojo_param_traits.h

[scottmg, 2017-04-12 20:47:33]

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

[commit-bot: I haz the power, 2017-04-12 20:48:12]

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-12 20:49:17]

lgtm

https://codereview.chromium.org/2813243002/diff/40001/content/child/web_url_l...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/2813243002/diff/40001/content/child/web_url_l...
content/child/web_url_loader_impl.cc:818: // Since
ResourceDispatcher::ContinueForNavigation called OnComplete
nit: i would put this inside the next if statement, as only then is data pipe
being used

https://codereview.chromium.org/2813243002/diff/40001/content/child/web_url_l...
content/child/web_url_loader_impl.cc:870: // TODO(kinuko|scottmg|jam): This is
wrong. https://crbug.com/598073.
nit: 598073 is for network service, while this code is for plznavigate. so i'd
either take out that bug or make a new one to track this

[scottmg, 2017-04-12 20:51:38]

Thanks

https://codereview.chromium.org/2813243002/diff/40001/content/child/web_url_l...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/2813243002/diff/40001/content/child/web_url_l...
content/child/web_url_loader_impl.cc:818: // Since
ResourceDispatcher::ContinueForNavigation called OnComplete
On 2017/04/12 20:49:17, jam wrote:
> nit: i would put this inside the next if statement, as only then is data pipe
> being used

Should be if (a&&b) anyway, fixed.

https://codereview.chromium.org/2813243002/diff/40001/content/child/web_url_l...
content/child/web_url_loader_impl.cc:870: // TODO(kinuko|scottmg|jam): This is
wrong. https://crbug.com/598073.
On 2017/04/12 20:49:17, jam wrote:
> nit: 598073 is for network service, while this code is for plznavigate. so i'd
> either take out that bug or make a new one to track this

Put it on the data pipe one, 705744.

[scottmg, 2017-04-12 20:51:51]

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

[commit-bot: I haz the power, 2017-04-12 20:52:02]

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-12 21:04:12]

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

[commit-bot: I haz the power, 2017-04-12 21:04:13]

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

[scottmg, 2017-04-12 21:10:19]

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

[commit-bot: I haz the power, 2017-04-12 21:11:37]

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

[dcheng, 2017-04-12 21:18:16]

ipc lgtm

https://codereview.chromium.org/2813243002/diff/60001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_network_service.cc (right):

https://codereview.chromium.org/2813243002/diff/60001/content/browser/loader/...
content/browser/loader/navigation_url_loader_network_service.cc:72: response_ =
new ResourceResponse();
Nit: base::MakeShared<ResourceResponse>();

https://codereview.chromium.org/2813243002/diff/60001/content/browser/loader/...
content/browser/loader/navigation_url_loader_network_service.cc:107: response_,
std::unique_ptr<StreamHandle>(), std::move(body), SSLStatus(),
Nit: nullptr

https://codereview.chromium.org/2813243002/diff/60001/content/child/resource_...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/2813243002/diff/60001/content/child/resource_...
content/child/resource_dispatcher.cc:644:
pending_requests_[request_id]->url_loader_client = std::move(client);
Nit:
pending_requested_[request_id]->url_loader_client =
    base::MakeUnique<URLLoaderClientImpl>(request_id, this, task_runnner);

should fit on two lines.

https://codereview.chromium.org/2813243002/diff/60001/content/child/web_url_l...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/2813243002/diff/60001/content/child/web_url_l...
content/child/web_url_loader_impl.cc:817: if (stream_override_.get() &&
stream_override_->stream_url.is_empty()) {
Nit: no .get()

https://codereview.chromium.org/2813243002/diff/60001/content/child/web_url_l...
content/child/web_url_loader_impl.cc:866: if (stream_override_.get() &&
stream_override_->stream_url.is_empty()) {
Ditto: no .get()

https://codereview.chromium.org/2813243002/diff/60001/ipc/ipc_mojo_param_trai...
File ipc/ipc_mojo_param_traits.cc (right):

https://codereview.chromium.org/2813243002/diff/60001/ipc/ipc_mojo_param_trai...
ipc/ipc_mojo_param_traits.cc:80: LOG(ERROR) << "Failed to read attachment for
message pipe.";
Nit: DLOG(ERROR) here and 87

[scottmg, 2017-04-12 21:25:30]

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

[commit-bot: I haz the power, 2017-04-12 21:25:43]

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

[scottmg, 2017-04-12 21:27:52]

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

[commit-bot: I haz the power, 2017-04-12 21:28:27]

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

[scottmg, 2017-04-12 21:28:51]

Thanks!

https://codereview.chromium.org/2813243002/diff/60001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_network_service.cc (right):

https://codereview.chromium.org/2813243002/diff/60001/content/browser/loader/...
content/browser/loader/navigation_url_loader_network_service.cc:72: response_ =
new ResourceResponse();
On 2017/04/12 21:18:16, dcheng wrote:
> Nit: base::MakeShared<ResourceResponse>();

Doh, more of them! Done.

(For my edification... I can see the upside in the last CL of MakeUnique for
locals, etc. since there's less repetition of the typename... why is MakeShared
preferable here?)

https://codereview.chromium.org/2813243002/diff/60001/content/browser/loader/...
content/browser/loader/navigation_url_loader_network_service.cc:107: response_,
std::unique_ptr<StreamHandle>(), std::move(body), SSLStatus(),
On 2017/04/12 21:18:16, dcheng wrote:
> Nit: nullptr

Done.

https://codereview.chromium.org/2813243002/diff/60001/content/child/resource_...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/2813243002/diff/60001/content/child/resource_...
content/child/resource_dispatcher.cc:644:
pending_requests_[request_id]->url_loader_client = std::move(client);
On 2017/04/12 21:18:16, dcheng wrote:
> Nit:
> pending_requested_[request_id]->url_loader_client =
>     base::MakeUnique<URLLoaderClientImpl>(request_id, this, task_runnner);
> 
> should fit on two lines.

Done.

https://codereview.chromium.org/2813243002/diff/60001/content/child/web_url_l...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/2813243002/diff/60001/content/child/web_url_l...
content/child/web_url_loader_impl.cc:817: if (stream_override_.get() &&
stream_override_->stream_url.is_empty()) {
On 2017/04/12 21:18:16, dcheng wrote:
> Nit: no .get()

Done.

https://codereview.chromium.org/2813243002/diff/60001/content/child/web_url_l...
content/child/web_url_loader_impl.cc:866: if (stream_override_.get() &&
stream_override_->stream_url.is_empty()) {
On 2017/04/12 21:18:16, dcheng wrote:
> Ditto: no .get()

Done.

https://codereview.chromium.org/2813243002/diff/60001/ipc/ipc_mojo_param_trai...
File ipc/ipc_mojo_param_traits.cc (right):

https://codereview.chromium.org/2813243002/diff/60001/ipc/ipc_mojo_param_trai...
ipc/ipc_mojo_param_traits.cc:80: LOG(ERROR) << "Failed to read attachment for
message pipe.";
On 2017/04/12 21:18:16, dcheng wrote:
> Nit: DLOG(ERROR) here and 87

Done.

[dcheng, 2017-04-12 21:31:11]

https://codereview.chromium.org/2813243002/diff/60001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_network_service.cc (right):

https://codereview.chromium.org/2813243002/diff/60001/content/browser/loader/...
content/browser/loader/navigation_url_loader_network_service.cc:72: response_ =
new ResourceResponse();
On 2017/04/12 21:28:50, scottmg wrote:
> On 2017/04/12 21:18:16, dcheng wrote:
> > Nit: base::MakeShared<ResourceResponse>();
> 
> Doh, more of them! Done.
> 
> (For my edification... I can see the upside in the last CL of MakeUnique for
> locals, etc. since there's less repetition of the typename... why is
MakeShared
> preferable here?)

in my opinion, it's nice not to have a bare new, since a reader doesn't have to
check to see if the field is a raw pointer, scoped_refptr, or something else.

[scottmg, 2017-04-12 21:35:28]

https://codereview.chromium.org/2813243002/diff/60001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_network_service.cc (right):

https://codereview.chromium.org/2813243002/diff/60001/content/browser/loader/...
content/browser/loader/navigation_url_loader_network_service.cc:72: response_ =
new ResourceResponse();
On 2017/04/12 21:31:11, dcheng wrote:
> On 2017/04/12 21:28:50, scottmg wrote:
> > On 2017/04/12 21:18:16, dcheng wrote:
> > > Nit: base::MakeShared<ResourceResponse>();
> > 
> > Doh, more of them! Done.
> > 
> > (For my edification... I can see the upside in the last CL of MakeUnique for
> > locals, etc. since there's less repetition of the typename... why is
> MakeShared
> > preferable here?)
> 
> in my opinion, it's nice not to have a bare new, since a reader doesn't have
to
> check to see if the field is a raw pointer, scoped_refptr, or something else.

so ideally someday, the only bare 'new's would represent raw pointers in your
imagining? makes sense, thanks.

[scottmg, 2017-04-12 21:35:38]

The CQ bit was unchecked by scottmg@chromium.org

[scottmg, 2017-04-12 21:37:20]

scottmg@chromium.org changed reviewers:
+ kinuko@chromium.org

[scottmg, 2017-04-12 21:37:20]

Thanks for the patch Kinuko!

Sorry for making it uglier to land before we could fully switch over to only
data pipe. :/ Hopefully pretty soon.

[scottmg, 2017-04-12 21:37:25]

The CQ bit was checked by scottmg@chromium.org

[scottmg, 2017-04-12 21:37:26]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2813243002/#ps120001
(title: "ahem")

[commit-bot: I haz the power, 2017-04-12 21:38:28]

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

[jam, 2017-04-12 21:51:47]

Description was changed from

==========
network service: pass PlzNavigate resulting data via mojo data pipe

Mostly kinuko's work from https://codereview.chromium.org/2797443005/ !

For now, maintains the old/current path of passing the data from PlzNav
via a stream and adds the mojo data pipe as another option. This is to
avoid changing anything that might affect PlzNav performance, etc. while
it is in the middle of being launched.

BUG=598073,705744
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
network service: pass PlzNavigate resulting data via mojo data pipe

Mostly Kinuko's work from https://codereview.chromium.org/2797443005/ !

For now, maintains the old/current path of passing the data from PlzNav
via a stream and adds the mojo data pipe as another option. This is to
avoid changing anything that might affect PlzNav performance, etc. while
it is in the middle of being launched.

BUG=598073,705744
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

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

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1492033045981640,
"parent_rev": "474250dafbf9967b9ea41d4c423216ca0e54567d", "commit_rev":
"efb697309fb4c64a99e8c38b0448a4b4bb97886a"}

[commit-bot: I haz the power, 2017-04-12 22:38:41]

Description was changed from

==========
network service: pass PlzNavigate resulting data via mojo data pipe

Mostly Kinuko's work from https://codereview.chromium.org/2797443005/ !

For now, maintains the old/current path of passing the data from PlzNav
via a stream and adds the mojo data pipe as another option. This is to
avoid changing anything that might affect PlzNav performance, etc. while
it is in the middle of being launched.

BUG=598073,705744
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
network service: pass PlzNavigate resulting data via mojo data pipe

Mostly Kinuko's work from https://codereview.chromium.org/2797443005/ !

For now, maintains the old/current path of passing the data from PlzNav
via a stream and adds the mojo data pipe as another option. This is to
avoid changing anything that might affect PlzNav performance, etc. while
it is in the middle of being launched.

BUG=598073,705744
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2813243002
Cr-Commit-Position: refs/heads/master@{#464180}
Committed:
https://chromium.googlesource.com/chromium/src/+/efb697309fb4c64a99e8c38b0448...
==========

[commit-bot: I haz the power, 2017-04-12 22:38:45]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/efb697309fb4c64a99e8c38b0448...

[kinuko, 2017-04-13 00:25:45]

lgtm, hope we can clean these up soon too :)