net: extract net/cert out of net/base

net: extract net/cert out of net/base

This introduces the following dependency of net/base on things outside:

net/base/openssl_client_key_store.cc:#include "net/cert/x509_certificate.h"

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

[Paweł Hajdan Jr., 2013-03-26 20:18:38]

Please review:

Ryan: entire CL
Sergey: remoting
Tony: webkit

[alexeypa (please no reviews), 2013-03-26 20:30:32]

remoting/* - lgtm.

[tony, 2013-03-26 20:48:59]

webkit lgtm

[Ryan Sleevi, 2013-03-27 17:21:24]

As mentioned via e-mail, I think the OpenSSLClientKeyStore needs to land either
in cert/ (as it manages certificate private keys) or in socket/ (as it manages
*client* certificate private keys). I suspect cert/ is the better place - and
that will affect some of the stuff you've already moved into socket/

Other than that, I've attempted to focus on the changes to the GYP and DEPS
file, and only took a cursory look at the rest of the files, since they should
all have been mechanically handled.

https://codereview.chromium.org/13006020/diff/1/net/cert/test_certificate_data.h
File net/cert/test_certificate_data.h (right):

https://codereview.chromium.org/13006020/diff/1/net/cert/test_certificate_dat...
net/cert/test_certificate_data.h:8: static const char kNistSPKIHash[] =
I find it a bit inconsistent that you split up cert_test_util but didn't move
test_certificate_data or test_root_certs

Was there an intentional reason, or was it just overlooked?

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

https://codereview.chromium.org/13006020/diff/1/net/net.gyp#newcode128
net/net.gyp:128: 'cert/crl_set.h',
Please sort all modifications to this file.

https://codereview.chromium.org/13006020/diff/1/remoting/host/DEPS
File remoting/host/DEPS (right):

https://codereview.chromium.org/13006020/diff/1/remoting/host/DEPS#newcode3
remoting/host/DEPS:3: "+net",
Please justify this change.

The previous code was extremely explicit in what it depended on, including the
comment. You have now made it free-for-all. This does not seem an appropriate
change if our goal is to limit scope/dependencies - which it is.

https://codereview.chromium.org/13006020/diff/1/webkit/DEPS
File webkit/DEPS (right):

https://codereview.chromium.org/13006020/diff/1/webkit/DEPS#newcode41
webkit/DEPS:41: "+net",
Can you explain why you made this change?

The previous code excluded the following:
net/android
net/curvecp
net/disk_cache
net/ocsp
net/quic
net/server
net/spdy
net/third_party
net/tools
net/udp

[tony, 2013-03-27 19:49:52]

https://codereview.chromium.org/13006020/diff/1/webkit/DEPS
File webkit/DEPS (right):

https://codereview.chromium.org/13006020/diff/1/webkit/DEPS#newcode41
webkit/DEPS:41: "+net",
On 2013/03/27 17:21:25, Ryan Sleevi wrote:
> Can you explain why you made this change?
> 
> The previous code excluded the following:
> net/android
> net/curvecp
> net/disk_cache
> net/ocsp
> net/quic
> net/server
> net/spdy
> net/third_party
> net/tools
> net/udp

I suggested this change. Most of the other cases where we have a DEPS entry for
net we just include the whole directory.  It doesn't seem useful to say you can
only use some parts of net since webkit depends on the net library, which pulls
in files from many of the net subdirectories.

[Paweł Hajdan Jr., 2013-03-28 00:25:34]

PTAL

If possible I'd like to sort out openssl-related issues in a separate CL, just
so it's easier to think about dependencies of net/base on other directories.

If you still think this should be done in this CL, it's fine for me - it's just
my preference that I think would work better.

https://codereview.chromium.org/13006020/diff/1/net/cert/test_certificate_data.h
File net/cert/test_certificate_data.h (right):

https://codereview.chromium.org/13006020/diff/1/net/cert/test_certificate_dat...
net/cert/test_certificate_data.h:8: static const char kNistSPKIHash[] =
On 2013/03/27 17:21:25, Ryan Sleevi wrote:
> I find it a bit inconsistent that you split up cert_test_util but didn't move
> test_certificate_data or test_root_certs
> 
> Was there an intentional reason, or was it just overlooked?

The reason was minimalism. Moving cert_test_util to net/test was to avoid
dependency of net/base on net/cert - for tests it's fine to depend on net/test.

After your comment I've moved all test support files to net/test.

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

https://codereview.chromium.org/13006020/diff/1/net/net.gyp#newcode128
net/net.gyp:128: 'cert/crl_set.h',
On 2013/03/27 17:21:25, Ryan Sleevi wrote:
> Please sort all modifications to this file.

Done.

https://codereview.chromium.org/13006020/diff/1/remoting/host/DEPS
File remoting/host/DEPS (right):

https://codereview.chromium.org/13006020/diff/1/remoting/host/DEPS#newcode3
remoting/host/DEPS:3: "+net",
On 2013/03/27 17:21:25, Ryan Sleevi wrote:
> Please justify this change.

1. remoting OWNER has approved this
2. Tony suggested a similar change for webkit in one of previous CL, so I gave
it a try here as well

[Ryan Sleevi, 2013-03-28 00:31:45]

I haven't checked all of the net/ files, but this one stands out, so it will
require another patch.

I'm still not convinced on the DEPS changes - I think in general it's better to
be as specific as possible, so then every time we add new dependencies we're
actually forced to think about the design, but I suspect I'm outvoted here (and
certainly, by OWNERS who have more vesting in their code)

https://codereview.chromium.org/13006020/diff/23001/net/cert/cert_verify_proc...
File net/cert/cert_verify_proc_win.cc (right):

https://codereview.chromium.org/13006020/diff/23001/net/cert/cert_verify_proc...
net/cert/cert_verify_proc_win.cc:26: #include "net/test/test_root_certs.h"
BUG: It appears you cannot move this to net/test, because now net/cert depends
on net/test (the latter is built as part of net_test_support, not net)

[joth, 2013-03-28 00:45:02]

aw/ lgtm

[Paweł Hajdan Jr., 2013-03-28 14:57:55]

PTAL

[Ryan Sleevi, 2013-03-28 19:00:52]

Still some header sorting issues. I'm guessing a late move of cert_status_flags
that you may not have re-run sort-includes on?

https://codereview.chromium.org/13006020/diff/68001/net/http/http_cache_trans...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/13006020/diff/68001/net/http/http_cache_trans...
net/http/http_cache_transaction.cc:24: #include "net/cert/cert_status_flags.h"
sort

https://codereview.chromium.org/13006020/diff/68001/net/http/http_cache_unitt...
File net/http/http_cache_unittest.cc (right):

https://codereview.chromium.org/13006020/diff/68001/net/http/http_cache_unitt...
net/http/http_cache_unittest.cc:13: #include "net/base/cache_type.h"
sort

https://codereview.chromium.org/13006020/diff/68001/net/proxy/proxy_script_fe...
File net/proxy/proxy_script_fetcher_impl.cc (right):

https://codereview.chromium.org/13006020/diff/68001/net/proxy/proxy_script_fe...
net/proxy/proxy_script_fetcher_impl.cc:12: #include
"net/cert/cert_status_flags.h"
sort

https://codereview.chromium.org/13006020/diff/68001/net/ssl/client_cert_store...
File net/ssl/client_cert_store_impl_win.cc (right):

https://codereview.chromium.org/13006020/diff/68001/net/ssl/client_cert_store...
net/ssl/client_cert_store_impl_win.cc:12: #include <security.h>
Unless you're able to check and confirm the header ordering is safe here across
all supported Win versions, I'd encourage you to restore the original order. As
Wan-Teh pointed out in a previous review, these particular headers are sensitive
to ordering.

https://codereview.chromium.org/13006020/diff/68001/net/url_request/url_reque...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/13006020/diff/68001/net/url_request/url_reque...
net/url_request/url_request_http_job.cc:20: #include
"net/cert/cert_status_flags.h"
sort

[Paweł Hajdan Jr., 2013-03-28 19:40:05]

PTAL

https://codereview.chromium.org/13006020/diff/68001/net/http/http_cache_trans...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/13006020/diff/68001/net/http/http_cache_trans...
net/http/http_cache_transaction.cc:24: #include "net/cert/cert_status_flags.h"
On 2013/03/28 19:00:52, Ryan Sleevi wrote:
> sort

Done.

https://codereview.chromium.org/13006020/diff/68001/net/http/http_cache_unitt...
File net/http/http_cache_unittest.cc (right):

https://codereview.chromium.org/13006020/diff/68001/net/http/http_cache_unitt...
net/http/http_cache_unittest.cc:13: #include "net/base/cache_type.h"
On 2013/03/28 19:00:52, Ryan Sleevi wrote:
> sort

Done.

https://codereview.chromium.org/13006020/diff/68001/net/proxy/proxy_script_fe...
File net/proxy/proxy_script_fetcher_impl.cc (right):

https://codereview.chromium.org/13006020/diff/68001/net/proxy/proxy_script_fe...
net/proxy/proxy_script_fetcher_impl.cc:12: #include
"net/cert/cert_status_flags.h"
On 2013/03/28 19:00:52, Ryan Sleevi wrote:
> sort

Done.

https://codereview.chromium.org/13006020/diff/68001/net/ssl/client_cert_store...
File net/ssl/client_cert_store_impl_win.cc (right):

https://codereview.chromium.org/13006020/diff/68001/net/ssl/client_cert_store...
net/ssl/client_cert_store_impl_win.cc:12: #include <security.h>
On 2013/03/28 19:00:52, Ryan Sleevi wrote:
> Unless you're able to check and confirm the header ordering is safe here
across
> all supported Win versions, I'd encourage you to restore the original order.
As
> Wan-Teh pointed out in a previous review, these particular headers are
sensitive
> to ordering.

Done.

https://codereview.chromium.org/13006020/diff/68001/net/url_request/url_reque...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/13006020/diff/68001/net/url_request/url_reque...
net/url_request/url_request_http_job.cc:20: #include
"net/cert/cert_status_flags.h"
On 2013/03/28 19:00:52, Ryan Sleevi wrote:
> sort

Done.

[Ryan Sleevi, 2013-03-29 20:43:59]

net/ lgtm

[Paweł Hajdan Jr., 2013-03-29 21:48:23]

Committed patchset #7 manually as r191450.