net: split net/ssl out of net/base

net: split net/ssl out of net/base

Also moves transport_security_state files to net/http.

This change also updates the callers.

R=wtc
TBR=rsleevi,lambroslambrou,tony

BUG=70818
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=188119

[Paweł Hajdan Jr., 2013-03-08 20:23:55]

Alright, I'm pretty sure you'll have some feedback about which files should go
where.

For now I've made it so that net/base doesn't have dependencies outside of it
(not sure why presubmit check didn't complain about net/dns dependency - I may
need to temporarily allow it though).

The rest is just fixing the callers and trivial updates to DEPS files which
already allowed inclusion of net/base.

[Ryan Sleevi, 2013-03-08 20:43:20]

On 2013/03/08 20:23:55, Paweł Hajdan Jr. wrote:
> Alright, I'm pretty sure you'll have some feedback about which files should go
> where.
> 
> For now I've made it so that net/base doesn't have dependencies outside of it
> (not sure why presubmit check didn't complain about net/dns dependency - I may
> need to temporarily allow it though).
> 
> The rest is just fixing the callers and trivial updates to DEPS files which
> already allowed inclusion of net/base.

Why didn't you move any of the Cert Verifier bits? Do you see that going
somewhere else? It's hard to visualize your proposed layout in the giant patch,
would it be possible to just put together a quick page detailing what you see
net/base looking like after all the moves, so that it can be looked at
holistically? I imagine everything else will be mechnical beyond that point.

[Ryan Sleevi, 2013-03-08 20:44:36]

On 2013/03/08 20:43:20, Ryan Sleevi wrote:
> On 2013/03/08 20:23:55, Paweł Hajdan Jr. wrote:
> > Alright, I'm pretty sure you'll have some feedback about which files should
go
> > where.
> > 
> > For now I've made it so that net/base doesn't have dependencies outside of
it
> > (not sure why presubmit check didn't complain about net/dns dependency - I
may
> > need to temporarily allow it though).
> > 
> > The rest is just fixing the callers and trivial updates to DEPS files which
> > already allowed inclusion of net/base.
> 
> Why didn't you move any of the Cert Verifier bits? Do you see that going
> somewhere else? It's hard to visualize your proposed layout in the giant
patch,
> would it be possible to just put together a quick page detailing what you see
> net/base looking like after all the moves, so that it can be looked at
> holistically? I imagine everything else will be mechnical beyond that point.

And in case it wasn't clear why this comment - you moved stuff like cert stores
(well, some of it - not the openssl bits), but didn't move the X.509 stuff.

[Paweł Hajdan Jr., 2013-03-08 22:19:42]

On 2013/03/08 20:44:36, Ryan Sleevi wrote:
> On 2013/03/08 20:43:20, Ryan Sleevi wrote:
> > Why didn't you move any of the Cert Verifier bits? Do you see that going
> > somewhere else? It's hard to visualize your proposed layout in the giant
> patch,
> > would it be possible to just put together a quick page detailing what you
see
> > net/base looking like after all the moves, so that it can be looked at
> > holistically? I imagine everything else will be mechnical beyond that point.
> 
> And in case it wasn't clear why this comment - you moved stuff like cert
stores
> (well, some of it - not the openssl bits), but didn't move the X.509 stuff.

I think I was just unsure where to put them. net/cert could possibly a better
location, and it is mentioned in the bug.

It's rather hard for me to create an image of net after _all_ the moves, not
just this one, since there may be additional dependencies I'm now aware of or
just things that come up in reviews (and I usually find it better to discuss a
patch than an idea).

I'm using this text from the bug as a guideline:

> Based on scanning through file names, here are what look like the biggest
themes:
>
> - certificates (~40 files)
> - host resolving (dns) (~30 files)
> - ssl utilities (~30 ssl_* files)
>
> There are also a couple other smaller themes that might be appropriate as
their own subdirectories directories:
>
>  - cookies (~10)
>  - mimeutil (~10)

We can also use the e-mail thread with some additional questions I've asked
about the move for a more high-level discussion.

[wtc, 2013-03-12 23:14:36]

Patch set 1 LGTM. Thanks!

https://codereview.chromium.org/12680003/diff/1/chrome/browser/browsing_data/...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/12680003/diff/1/chrome/browser/browsing_data/...
chrome/browser/browsing_data/browsing_data_remover.cc:64: #include
"net/ssl/transport_security_state.h"

transport_security_state.h could also be moved to net/http.

https://codereview.chromium.org/12680003/diff/1/content/browser/loader/resour...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/12680003/diff/1/content/browser/loader/resour...
content/browser/loader/resource_loader.cc:24: #include
"net/base/client_cert_store.h"

Did you miss net/base/client_cert_store.h? It seems closely
related to net/ssl/client_cert_store_impl.h.

https://codereview.chromium.org/12680003/diff/1/net/base/DEPS
File net/base/DEPS (right):

https://codereview.chromium.org/12680003/diff/1/net/base/DEPS#newcode3
net/base/DEPS:3: "+net/base",

Does this mean we cannot include any headers in net except
the headers in net/base?

https://codereview.chromium.org/12680003/diff/1/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/12680003/diff/1/net/net.gyp#newcode127
net/net.gyp:127: 'ssl/client_cert_store_impl_win.cc',

Are you planning to sort these files in a separate CL?

https://codereview.chromium.org/12680003/diff/1/net/ssl/client_cert_store_imp...
File net/ssl/client_cert_store_impl_win.cc (right):

https://codereview.chromium.org/12680003/diff/1/net/ssl/client_cert_store_imp...
net/ssl/client_cert_store_impl_win.cc:12: #include <security.h>

In general, be careful about moving system headers or
third-party library headers because they may need to be
included in a particular order. For example, <windows.h>
must be included before <wincrypt.h>.

https://codereview.chromium.org/12680003/diff/1/net/ssl/default_server_bound_...
File net/ssl/default_server_bound_cert_store.h (right):

https://codereview.chromium.org/12680003/diff/1/net/ssl/default_server_bound_...
net/ssl/default_server_bound_cert_store.h:205: #endif  //
NET_DEFAULT_ORIGIN_BOUND_CERT_STORE_H_

Please fix the NET_DEFAULT_ORIGIN_BOUND_CERT_STORE_H_ in this
comment.

[tfarina, 2013-03-12 23:55:28]

https://codereview.chromium.org/12680003/diff/1/net/base/DEPS
File net/base/DEPS (right):

https://codereview.chromium.org/12680003/diff/1/net/base/DEPS#newcode3
net/base/DEPS:3: "+net/base",
On 2013/03/12 23:14:37, wtc wrote:
> 
> Does this mean we cannot include any headers in net except
> the headers in net/base?
no, it means net/base can only include headers from within net/base, i.e., it
won't be possible to include net/ftp in net/base/address_list.h (picked
randomly).

[Paweł Hajdan Jr., 2013-03-13 22:59:52]

Ryan, if you have any comments, please let me know. I know you're OOO but I've
seen you replying to some e-mails. :)

I'd prefer to land this change sooner than later (e.g. because of merges), and
anything can still be fixed after the move.

https://codereview.chromium.org/12680003/diff/1/chrome/browser/browsing_data/...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/12680003/diff/1/chrome/browser/browsing_data/...
chrome/browser/browsing_data/browsing_data_remover.cc:64: #include
"net/ssl/transport_security_state.h"
On 2013/03/12 23:14:37, wtc wrote:
> 
> transport_security_state.h could also be moved to net/http.

Done.

https://codereview.chromium.org/12680003/diff/1/content/browser/loader/resour...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/12680003/diff/1/content/browser/loader/resour...
content/browser/loader/resource_loader.cc:24: #include
"net/base/client_cert_store.h"
On 2013/03/12 23:14:37, wtc wrote:
> 
> Did you miss net/base/client_cert_store.h? It seems closely
> related to net/ssl/client_cert_store_impl.h.

Done.

https://codereview.chromium.org/12680003/diff/1/net/base/DEPS
File net/base/DEPS (right):

https://codereview.chromium.org/12680003/diff/1/net/base/DEPS#newcode3
net/base/DEPS:3: "+net/base",
On 2013/03/12 23:14:37, wtc wrote:
> 
> Does this mean we cannot include any headers in net except
> the headers in net/base?

I've used this to make sure I'm not making net/base accidentally use net/ssl .

However, there are many existing violation of the above, like includes of
net/dns, http, android, url_request that I think it's not worth to add the above
with a pretty large whitelist.

https://codereview.chromium.org/12680003/diff/1/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/12680003/diff/1/net/net.gyp#newcode127
net/net.gyp:127: 'ssl/client_cert_store_impl_win.cc',
On 2013/03/12 23:14:37, wtc wrote:
> 
> Are you planning to sort these files in a separate CL?

Done.

https://codereview.chromium.org/12680003/diff/1/net/ssl/client_cert_store_imp...
File net/ssl/client_cert_store_impl_win.cc (right):

https://codereview.chromium.org/12680003/diff/1/net/ssl/client_cert_store_imp...
net/ssl/client_cert_store_impl_win.cc:12: #include <security.h>
On 2013/03/12 23:14:37, wtc wrote:
> 
> In general, be careful about moving system headers or
> third-party library headers because they may need to be
> included in a particular order. For example, <windows.h>
> must be included before <wincrypt.h>.

Done.

https://codereview.chromium.org/12680003/diff/1/net/ssl/default_server_bound_...
File net/ssl/default_server_bound_cert_store.h (right):

https://codereview.chromium.org/12680003/diff/1/net/ssl/default_server_bound_...
net/ssl/default_server_bound_cert_store.h:205: #endif  //
NET_DEFAULT_ORIGIN_BOUND_CERT_STORE_H_
On 2013/03/12 23:14:37, wtc wrote:
> 
> Please fix the NET_DEFAULT_ORIGIN_BOUND_CERT_STORE_H_ in this
> comment.

Done.

[wtc, 2013-03-14 00:26:09]

Patch set 2 LGTM. I only reviewed the diffs between patch sets
1 and 2.

1. The CL's description should say you moved the
transport_security_state* files to net/http.

2. Please check in this CL soon. You don't need to wait for
rsleevi's approval. I don't have the stamina to review this
huge CL again. Thanks.

[Paweł Hajdan Jr., 2013-03-14 16:50:38]

On 2013/03/14 00:26:09, wtc wrote:
> 1. The CL's description should say you moved the
> transport_security_state* files to net/http.

Done.

> 2. Please check in this CL soon. You don't need to wait for
> rsleevi's approval. I don't have the stamina to review this
> huge CL again. Thanks.

Will do (today).

Sorry about huge change - if you find it useful, I often use the "download raw
patch set" for bulk changes like this. Except for the big moves, it allows you
to just keep scrolling down to sort of look at mechanical changes.

[Paweł Hajdan Jr., 2013-03-14 17:42:10]

Committed patchset #4 manually as r188119 (presubmit successful).

[Paweł Hajdan Jr., 2013-03-14 18:05:57]

TBR:

Ryan: entire CL
Lambros: remoting
Tony: webkit

[Lambros, 2013-03-14 18:08:42]

remoting/* lgtm

[Ryan Sleevi, 2013-03-14 18:23:00]

On 2013/03/14 18:08:42, Lambros wrote:
> remoting/* lgtm

No need for my LGTM - you got wtc's.

[wtc, 2013-03-14 18:34:02]

On 2013/03/14 18:23:00, Ryan Sleevi wrote:
>
> No need for my LGTM - you got wtc's.

Please advise on whether the transport_security_state* files should be
moved to net/ssl or net/http. This CL moved them to net/http. If you
think transport_security_state* belong in net/ssl, we can fix that.

[Ryan Sleevi, 2013-03-14 18:43:06]

On 2013/03/14 18:34:02, wtc wrote:
> On 2013/03/14 18:23:00, Ryan Sleevi wrote:
> >
> > No need for my LGTM - you got wtc's.
> 
> Please advise on whether the transport_security_state* files should be
> moved to net/ssl or net/http. This CL moved them to net/http. If you
> think transport_security_state* belong in net/ssl, we can fix that.

For now they belong in net/http, because they only deal with HSTS and HPKP,
although once Trevor works on TACK (which operates at the SSL layer), arguably
it's a bit messy.