Replace //net TypeConverters with StructTraits.

Replace //net TypeConverters with StructTraits.

BUG=621383
Committed: https://crrev.com/170917fcc907d4e4c9b13d2a367dc5da2ac6b406
Cr-Commit-Position: refs/heads/master@{#424110}

[Sam McNally, 2016-06-23 06:02:34]

Patchset #2 (id:20001) has been deleted

[Sam McNally, 2016-06-23 06:02:51]

Patchset #1 (id:1) has been deleted

[Sam McNally, 2016-06-23 06:58:07]

Patchset #1 (id:40001) has been deleted

[Sam McNally, 2016-06-24 01:06:37]

sammc@chromium.org changed reviewers:
+ eroman@chromium.org

[eroman, 2016-06-24 18:48:09]

lgtm

https://codereview.chromium.org/2083463002/diff/40002/net/dns/OWNERS
File net/dns/OWNERS (right):

https://codereview.chromium.org/2083463002/diff/40002/net/dns/OWNERS#newcode1
net/dns/OWNERS:1: per-file *_type_converter*.*=set noparent
Can these two lines be deleted now?

https://codereview.chromium.org/2083463002/diff/40002/net/dns/mojo_host_resol...
File net/dns/mojo_host_resolver_impl.cc (right):

https://codereview.chromium.org/2083463002/diff/40002/net/dns/mojo_host_resol...
net/dns/mojo_host_resolver_impl.cc:62: HostResolver::RequestInfo&
host_request_info = *request_info;
Can you instead delete this line and just directly use request_info-> throughout
?

https://codereview.chromium.org/2083463002/diff/40002/net/proxy/OWNERS
File net/proxy/OWNERS (right):

https://codereview.chromium.org/2083463002/diff/40002/net/proxy/OWNERS#newcode1
net/proxy/OWNERS:1: per-file *_type_converter*.*=set noparent
can these be deleted?

[Sam McNally, 2016-06-27 01:41:30]

sammc@chromium.org changed reviewers:
+ amistry@chromium.org, dcheng@chromium.org

[Sam McNally, 2016-06-27 01:41:31]

+amistry for mojo/

+dcheng for mojom and struct traits

https://codereview.chromium.org/2083463002/diff/40002/net/dns/OWNERS
File net/dns/OWNERS (right):

https://codereview.chromium.org/2083463002/diff/40002/net/dns/OWNERS#newcode1
net/dns/OWNERS:1: per-file *_type_converter*.*=set noparent
On 2016/06/24 18:48:09, eroman wrote:
> Can these two lines be deleted now?

Done.

https://codereview.chromium.org/2083463002/diff/40002/net/dns/mojo_host_resol...
File net/dns/mojo_host_resolver_impl.cc (right):

https://codereview.chromium.org/2083463002/diff/40002/net/dns/mojo_host_resol...
net/dns/mojo_host_resolver_impl.cc:62: HostResolver::RequestInfo&
host_request_info = *request_info;
On 2016/06/24 18:48:09, eroman wrote:
> Can you instead delete this line and just directly use request_info->
throughout
> ?

Done.

https://codereview.chromium.org/2083463002/diff/40002/net/proxy/OWNERS
File net/proxy/OWNERS (right):

https://codereview.chromium.org/2083463002/diff/40002/net/proxy/OWNERS#newcode1
net/proxy/OWNERS:1: per-file *_type_converter*.*=set noparent
On 2016/06/24 18:48:09, eroman wrote:
> can these be deleted?

Done.

[Anand Mistry (off Chromium), 2016-06-27 01:49:20]

LGTM for mojo/

[dcheng, 2016-06-27 03:50:45]

https://codereview.chromium.org/2083463002/diff/90001/net/dns/mojo_host_struc...
File net/dns/mojo_host_struct_traits.cc (right):

https://codereview.chromium.org/2083463002/diff/90001/net/dns/mojo_host_struc...
net/dns/mojo_host_struct_traits.cc:68: }
Nit: newline

https://codereview.chromium.org/2083463002/diff/90001/net/dns/mojo_host_struc...
File net/dns/mojo_host_struct_traits.h (right):

https://codereview.chromium.org/2083463002/diff/90001/net/dns/mojo_host_struc...
net/dns/mojo_host_struct_traits.h:65: std::vector<net::IPEndPoint>
address_list(obj.begin(), obj.end());
My understanding is this gets called multiple times. Is it worth it to add an
endpoints accessor to AddressList?

https://codereview.chromium.org/2083463002/diff/90001/net/interfaces/proxy_re...
File net/interfaces/proxy_resolver_service.mojom (right):

https://codereview.chromium.org/2083463002/diff/90001/net/interfaces/proxy_re...
net/interfaces/proxy_resolver_service.mojom:44: ReportResult(int32 error,
ProxyInfo proxy_info);
How hard is it to make this work in the bindings without the wrapper struct?

Right now, there's basically two ways to write things: if you're passing a
struct already, you can use array<ProxyServer> or ProxyInfo, but if you're
passing it directly, you have to write ProxyInfo. Can we make this so there's
one canonical way to write this, without the wrapper struct?

https://codereview.chromium.org/2083463002/diff/90001/net/proxy/mojo_proxy_st...
File net/proxy/mojo_proxy_struct_traits.h (right):

https://codereview.chromium.org/2083463002/diff/90001/net/proxy/mojo_proxy_st...
net/proxy/mojo_proxy_struct_traits.h:38: if (s.scheme() ==
net::ProxyServer::SCHEME_DIRECT ||
Let's move this to the .cc, since it's not just a one-liner.

Alternatively, do we need to do these checks? Maybe ProxyServer should just
initialize HostPairPort appropriately in this case (if it doesn't already)

[Sam McNally, 2016-06-29 01:42:07]

https://codereview.chromium.org/2083463002/diff/90001/net/dns/mojo_host_struc...
File net/dns/mojo_host_struct_traits.cc (right):

https://codereview.chromium.org/2083463002/diff/90001/net/dns/mojo_host_struc...
net/dns/mojo_host_struct_traits.cc:68: }
On 2016/06/27 03:50:45, dcheng wrote:
> Nit: newline

Done.

https://codereview.chromium.org/2083463002/diff/90001/net/dns/mojo_host_struc...
File net/dns/mojo_host_struct_traits.h (right):

https://codereview.chromium.org/2083463002/diff/90001/net/dns/mojo_host_struc...
net/dns/mojo_host_struct_traits.h:65: std::vector<net::IPEndPoint>
address_list(obj.begin(), obj.end());
On 2016/06/27 03:50:45, dcheng wrote:
> My understanding is this gets called multiple times. Is it worth it to add an
> endpoints accessor to AddressList?

Done.

https://codereview.chromium.org/2083463002/diff/90001/net/interfaces/proxy_re...
File net/interfaces/proxy_resolver_service.mojom (right):

https://codereview.chromium.org/2083463002/diff/90001/net/interfaces/proxy_re...
net/interfaces/proxy_resolver_service.mojom:44: ReportResult(int32 error,
ProxyInfo proxy_info);
On 2016/06/27 03:50:45, dcheng wrote:
> How hard is it to make this work in the bindings without the wrapper struct?
> 
> Right now, there's basically two ways to write things: if you're passing a
> struct already, you can use array<ProxyServer> or ProxyInfo, but if you're
> passing it directly, you have to write ProxyInfo. Can we make this so there's
> one canonical way to write this, without the wrapper struct?

It's possible to use array<ProxyServer> directly, but I don't think it's
possible to typemap an array. Each user would have to manually convert between
ProxyInfo and mojo::Array<ProxyServer>. The existing ProxyResolver interfaces
only really care about ProxyInfo so this would just make the glue more verbose.

https://codereview.chromium.org/2083463002/diff/90001/net/proxy/mojo_proxy_st...
File net/proxy/mojo_proxy_struct_traits.h (right):

https://codereview.chromium.org/2083463002/diff/90001/net/proxy/mojo_proxy_st...
net/proxy/mojo_proxy_struct_traits.h:38: if (s.scheme() ==
net::ProxyServer::SCHEME_DIRECT ||
On 2016/06/27 03:50:45, dcheng wrote:
> Let's move this to the .cc, since it's not just a one-liner.

Done.
> 
> Alternatively, do we need to do these checks? Maybe ProxyServer should just
> initialize HostPairPort appropriately in this case (if it doesn't already)

[Sam McNally, 2016-10-04 03:39:27]

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

[commit-bot: I haz the power, 2016-10-04 03:39: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, 2016-10-04 03:51:50]

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

[commit-bot: I haz the power, 2016-10-04 03:51: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...)

[Sam McNally, 2016-10-04 06:52:04]

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

[commit-bot: I haz the power, 2016-10-04 06:52:19]

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, 2016-10-04 06:54:34]

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

[commit-bot: I haz the power, 2016-10-04 06:54:35]

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...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[Sam McNally, 2016-10-04 23:51:20]

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

[commit-bot: I haz the power, 2016-10-04 23:51:58]

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, 2016-10-05 00:58:04]

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

[commit-bot: I haz the power, 2016-10-05 00:58:05]

Dry run: This issue passed the CQ dry run.

[Sam McNally, 2016-10-05 01:34:43]

Patchset #6 (id:150001) has been deleted

[Sam McNally, 2016-10-05 01:34:59]

Patchset #5 (id:130001) has been deleted

[Sam McNally, 2016-10-05 01:35:27]

dcheng: ping

[dcheng, 2016-10-05 05:59:33]

Thanks for doing this cleanup. The array thing is a little sad, but we can poke
at that outside this CL.

https://codereview.chromium.org/2083463002/diff/170001/net/dns/mojo_host_reso...
File net/dns/mojo_host_resolver_impl.h (right):

https://codereview.chromium.org/2083463002/diff/170001/net/dns/mojo_host_reso...
net/dns/mojo_host_resolver_impl.h:31: void
Resolve(std::unique_ptr<HostResolver::RequestInfo> request_info,
Out of curiosity, why pass by std::unique_ptr?

https://codereview.chromium.org/2083463002/diff/170001/net/proxy/mojo_proxy_s...
File net/proxy/mojo_proxy_struct_traits.cc (right):

https://codereview.chromium.org/2083463002/diff/170001/net/proxy/mojo_proxy_s...
net/proxy/mojo_proxy_struct_traits.cc:73: return base::StringPiece();
Is it valid for these conditions to be true and host() to return a non-empty
string? A quick look suggests that just returning them directly should Just
Work.

My personal preference, assuming that ProxyServer maintains this invariant, is
to just DCHECK() this in the serialization (and inline it)

Similar for the port() getter.

(Sorry I should have been more explicit about this in the previous reply)

[Sam McNally, 2016-10-05 22:48:37]

https://codereview.chromium.org/2083463002/diff/170001/net/dns/mojo_host_reso...
File net/dns/mojo_host_resolver_impl.h (right):

https://codereview.chromium.org/2083463002/diff/170001/net/dns/mojo_host_reso...
net/dns/mojo_host_resolver_impl.h:31: void
Resolve(std::unique_ptr<HostResolver::RequestInfo> request_info,
On 2016/10/05 05:59:33, dcheng wrote:
> Out of curiosity, why pass by std::unique_ptr?

HostResolver::RequestInfo isn't default constructible and typemapped types have
to be.

https://codereview.chromium.org/2083463002/diff/170001/net/proxy/mojo_proxy_s...
File net/proxy/mojo_proxy_struct_traits.cc (right):

https://codereview.chromium.org/2083463002/diff/170001/net/proxy/mojo_proxy_s...
net/proxy/mojo_proxy_struct_traits.cc:73: return base::StringPiece();
On 2016/10/05 05:59:33, dcheng wrote:
> Is it valid for these conditions to be true and host() to return a non-empty
> string? A quick look suggests that just returning them directly should Just
> Work.
> 
> My personal preference, assuming that ProxyServer maintains this invariant, is
> to just DCHECK() this in the serialization (and inline it)
> 
> Similar for the port() getter.
> 
> (Sorry I should have been more explicit about this in the previous reply)

ProxyServer::host_port_pair() DCHECKs that it's valid and not direct:
https://cs.chromium.org/chromium/src/net/proxy/proxy_server.cc?sq=package:chr....

[dcheng, 2016-10-05 23:18:27]

My personal preference would be to make RequestInfo default constructible, since
that would make things look nicer overall.

LGTM either way though.

[Sam McNally, 2016-10-06 07:13:48]

sammc@chromium.org changed reviewers:
+ rockot@chromium.org

[Sam McNally, 2016-10-06 07:13:48]

+rockot for //mojo

[Ken Rockot(use gerrit already), 2016-10-07 16:14:59]

lgtm

[Sam McNally, 2016-10-09 22:24:17]

The CQ bit was checked by sammc@chromium.org

[Sam McNally, 2016-10-09 22:24:17]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org,
amistry@chromium.org
Link to the patchset: https://codereview.chromium.org/2083463002/#ps170001
(title: "rebase")

[commit-bot: I haz the power, 2016-10-09 22:24:25]

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, 2016-10-09 23:26:27]

Committed patchset #5 (id:170001)

[commit-bot: I haz the power, 2016-10-09 23:28:18]

Description was changed from

==========
Replace //net TypeConverters with StructTraits.

BUG=621383
==========

to

==========
Replace //net TypeConverters with StructTraits.

BUG=621383

Committed: https://crrev.com/170917fcc907d4e4c9b13d2a367dc5da2ac6b406
Cr-Commit-Position: refs/heads/master@{#424110}
==========

[commit-bot: I haz the power, 2016-10-09 23:28:20]

Patchset 5 (id:??) landed as
https://crrev.com/170917fcc907d4e4c9b13d2a367dc5da2ac6b406
Cr-Commit-Position: refs/heads/master@{#424110}