Split cert reporter class into report building/serializing and sending

Split cert reporter class into report building/serializing and sending

The pre-existing |CertificateErrorReporter| class (in
//chrome/browser/net) is now only in charge of sending reports over the
network. A new class (|CertificateErrorReport| in //chrome/browser/ssl)
is in charge of building and serializing the reports.

The motivation for this change is to allow reports to include
interstitial-specific information (such as the type of interstitial that
was shown, whether the user clicked through, etc.). So as to avoid
introducing interstitial knowledge into //c/b/net, all the report
building and serializing knowledge (including the report protobuf) has
been moved into //c/b/ssl. |SSLBlockingPage| now sends a serialized report
through |ChromeContentBrowserClient| to the SafeBrowsing UIManager to be
sent over the network.

|ChromeFraudulentCertificateReporter| (responsible for reporting
Google-property pinning violations) has also been moved into //c/b/ssl
so that it can use the new |CertificateErrorReport| class to build
reports before sending them with a |CertificateErrorReporter|.

BUG=462713, 461588
Committed: https://crrev.com/4282f117db4ddb6b307efad849ec6635095e9cd1
Cr-Commit-Position: refs/heads/master@{#329723}

[estark, 2015-05-06 22:45:03]

estark@chromium.org changed reviewers:
+ rsleevi@chromium.org

[estark, 2015-05-06 22:45:03]

Ryan: this CL moves cert error report building/serializing into //c/b/ssl, and
keeps report sending in //c/b/net, as discussed in
https://codereview.chromium.org/1076273002/. Not yet adding additional
interstitial info to the reports, just doing the refactoring here. Could you
please take a look?

[Ryan Sleevi, 2015-05-06 23:03:39]

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

[Ryan Sleevi, 2015-05-06 23:03:39]

I'm going to randomly make Eric's life difficult by asking him to take point on
reviewing this (so we can all get familiarity). I've added Nick as FYI. Once
Eric has a go, I'll take a stab as well.

[estark, 2015-05-06 23:05:07]

Haha, ok. Sorry, Eric. :D Let me know if you'd like to chat more so I can give
you more context.

[eroman, 2015-05-07 00:16:32]

Sure I can take a look. Not sure if I will get to it tonight though.

[estark, 2015-05-11 16:44:38]

On 2015/05/07 00:16:32, eroman wrote:
> Sure I can take a look. Not sure if I will get to it tonight though.

friendly ping, eroman :)

[eroman, 2015-05-12 00:27:52]

mostly looks good just a couple comments.
thanks for your patience :)

https://codereview.chromium.org/1117173004/diff/20001/chrome/browser/net/cert...
File chrome/browser/net/certificate_error_reporter.cc (right):

https://codereview.chromium.org/1117173004/diff/20001/chrome/browser/net/cert...
chrome/browser/net/certificate_error_reporter.cc:121: if
(upload_url_.SchemeIsCryptographic()) {
Is this uploading reports to Google? under what circumstances would the
upload_url_ not be https? (Not directly related to your refactor, but still
curious)

https://codereview.chromium.org/1117173004/diff/20001/chrome/browser/net/cert...
File chrome/browser/net/certificate_error_reporter.h (right):

https://codereview.chromium.org/1117173004/diff/20001/chrome/browser/net/cert...
chrome/browser/net/certificate_error_reporter.h:74: const std::string&
serialized_report);
Please add some description of what serialized_report contains (data contents,
as well as the format). It doesn't need to be fully specified, but there should
be some general pointers to its value.

https://codereview.chromium.org/1117173004/diff/20001/chrome/browser/net/cert...
chrome/browser/net/certificate_error_reporter.h:96: // Send a serialized report
(encrypted or not) to the report
nit: All of these comments should be descriptive "Sends a serialized ..." rather
than imperative "Send a serialized". See
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Com...

https://codereview.chromium.org/1117173004/diff/20001/chrome/browser/net/encr...
File chrome/browser/net/encrypted_cert_logger.proto (right):

https://codereview.chromium.org/1117173004/diff/20001/chrome/browser/net/encr...
chrome/browser/net/encrypted_cert_logger.proto:25: UNKNOWN_ALGORITHM = 0;
when is 0 intended to be used?

https://codereview.chromium.org/1117173004/diff/20001/chrome/browser/ssl/cert...
File chrome/browser/ssl/certificate_error_report.cc (right):

https://codereview.chromium.org/1117173004/diff/20001/chrome/browser/ssl/cert...
chrome/browser/ssl/certificate_error_report.cc:22:
cert_report_.set_time_usec(now.ToInternalValue());
Is it the case that base::Time's internal value uses the same epoch on all
platforms?

I vaguely seem to remember we changed base::Time on Windows to use the unix
epoch too, but please confirm.

https://codereview.chromium.org/1117173004/diff/20001/chrome/browser/ssl/cert...
chrome/browser/ssl/certificate_error_report.cc:23:
cert_report_.set_hostname(hostname);
is the assumption that hostname will be ASCII?

https://codereview.chromium.org/1117173004/diff/20001/chrome/browser/ssl/cert...
chrome/browser/ssl/certificate_error_report.cc:27: LOG(ERROR) << "Could not get
PEM encoded chain.";
nit: Maybe I am just paranoid, but anytime I see macros not surrounded by braces
I get scared. Feel free to do nothing if the expansion is safe, but I suggest
putting curlies around this single-line if

https://codereview.chromium.org/1117173004/diff/20001/chrome/browser/ssl/cert...
chrome/browser/ssl/certificate_error_report.cc:31: *cert_chain +=
pem_encoded_chain[i];
would std::string::append() be better than += ?

Probably won't make any real difference, but I suspect += will be allocating
more objects.

Could get all crazy with reserve() and the like, but probably not worth it. Will
leave this up to your judgement based on the expected sizes and frequency of
use.

https://codereview.chromium.org/1117173004/diff/20001/chrome/browser/ssl/cert...
chrome/browser/ssl/certificate_error_report.cc:45:
cert_report_.SerializeToString(output);
This can return a failure. Why is it not necessary to check?

https://codereview.chromium.org/1117173004/diff/20001/chrome/browser/ssl/cert...
File chrome/browser/ssl/certificate_error_report.h (right):

https://codereview.chromium.org/1117173004/diff/20001/chrome/browser/ssl/cert...
chrome/browser/ssl/certificate_error_report.h:25: // Construct an empty report.
nit: Constructs

https://codereview.chromium.org/1117173004/diff/20001/chrome/browser/ssl/cert...
chrome/browser/ssl/certificate_error_report.h:28: // Construct a report for the
given |hostname| using the SSL
Same comment throughout, see reference to styleguide

https://codereview.chromium.org/1117173004/diff/20001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_cert_reporter.h (left):

https://codereview.chromium.org/1117173004/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_cert_reporter.h:22: // been sent off (not necessarily
after a response has been received).
I assume the reference to |callback| was obsolete? And completion is best-effort

https://codereview.chromium.org/1117173004/diff/20001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_cert_reporter.h (right):

https://codereview.chromium.org/1117173004/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_cert_reporter.h:20: // Send a serialized certificate
report to the report collection
Sends

[estark, 2015-05-12 20:42:16]

Thanks eroman.

https://codereview.chromium.org/1117173004/diff/20001/chrome/browser/net/cert...
File chrome/browser/net/certificate_error_reporter.cc (right):

https://codereview.chromium.org/1117173004/diff/20001/chrome/browser/net/cert...
chrome/browser/net/certificate_error_reporter.cc:121: if
(upload_url_.SchemeIsCryptographic()) {
On 2015/05/12 00:27:51, eroman wrote:
> Is this uploading reports to Google? under what circumstances would the
> upload_url_ not be https? (Not directly related to your refactor, but still
> curious)

Eventually, all uploads will be over HTTP, not HTTPS. Our goal is to collect
information about SSL warnings that users are seeing, and we very much want to
include data from users who might not be able to make any valid HTTPS
connections at all. At the moment we'll be sending reports (encrypted) over HTTP
on platforms where we have BoringSSL crypto, and we're sending them over HTTPS
on platforms where we don't yet have BoringSSL crypto.

(And yes, all the reports are being sent to Google servers.)

https://codereview.chromium.org/1117173004/diff/20001/chrome/browser/net/cert...
File chrome/browser/net/certificate_error_reporter.h (right):

https://codereview.chromium.org/1117173004/diff/20001/chrome/browser/net/cert...
chrome/browser/net/certificate_error_reporter.h:74: const std::string&
serialized_report);
On 2015/05/12 00:27:51, eroman wrote:
> Please add some description of what serialized_report contains (data contents,
> as well as the format). It doesn't need to be fully specified, but there
should
> be some general pointers to its value.

Done.

https://codereview.chromium.org/1117173004/diff/20001/chrome/browser/net/cert...
chrome/browser/net/certificate_error_reporter.h:96: // Send a serialized report
(encrypted or not) to the report
On 2015/05/12 00:27:51, eroman wrote:
> nit: All of these comments should be descriptive "Sends a serialized ..."
rather
> than imperative "Send a serialized". See
>
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Com...

Done.

https://codereview.chromium.org/1117173004/diff/20001/chrome/browser/net/encr...
File chrome/browser/net/encrypted_cert_logger.proto (right):

https://codereview.chromium.org/1117173004/diff/20001/chrome/browser/net/encr...
chrome/browser/net/encrypted_cert_logger.proto:25: UNKNOWN_ALGORITHM = 0;
On 2015/05/12 00:27:51, eroman wrote:
> when is 0 intended to be used?

Good question... I sort of copied this from the corresponding server-side
protobuf without thinking too much about it.

https://codereview.chromium.org/1117173004/diff/20001/chrome/browser/ssl/cert...
File chrome/browser/ssl/certificate_error_report.cc (right):

https://codereview.chromium.org/1117173004/diff/20001/chrome/browser/ssl/cert...
chrome/browser/ssl/certificate_error_report.cc:22:
cert_report_.set_time_usec(now.ToInternalValue());
On 2015/05/12 00:27:51, eroman wrote:
> Is it the case that base::Time's internal value uses the same epoch on all
> platforms?
> 
> I vaguely seem to remember we changed base::Time on Windows to use the unix
> epoch too, but please confirm.

It looks like it actually uses the Windows epoch on all platforms, IIUC. e.g.
https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time_pos...

https://codereview.chromium.org/1117173004/diff/20001/chrome/browser/ssl/cert...
chrome/browser/ssl/certificate_error_report.cc:23:
cert_report_.set_hostname(hostname);
On 2015/05/12 00:27:51, eroman wrote:
> is the assumption that hostname will be ASCII?

I'm not sure... I think in practice only ASCII will get passed in here, but I
don't see that anything would break if it were non-ASCII. Are you suggesting
that we should document this function as taking an ASCII-only |hostname|?

https://codereview.chromium.org/1117173004/diff/20001/chrome/browser/ssl/cert...
chrome/browser/ssl/certificate_error_report.cc:27: LOG(ERROR) << "Could not get
PEM encoded chain.";
On 2015/05/12 00:27:51, eroman wrote:
> nit: Maybe I am just paranoid, but anytime I see macros not surrounded by
braces
> I get scared. Feel free to do nothing if the expansion is safe, but I suggest
> putting curlies around this single-line if

Done.

https://codereview.chromium.org/1117173004/diff/20001/chrome/browser/ssl/cert...
chrome/browser/ssl/certificate_error_report.cc:31: *cert_chain +=
pem_encoded_chain[i];
On 2015/05/12 00:27:51, eroman wrote:
> would std::string::append() be better than += ?
> 
> Probably won't make any real difference, but I suspect += will be allocating
> more objects.
> 
> Could get all crazy with reserve() and the like, but probably not worth it.
Will
> leave this up to your judgement based on the expected sizes and frequency of
> use.

Changed to use std::string::append() but I don't think it's worth trying to
optimize any further than that. This code only runs occasionally (on SSL
interstitials or pinning violations on Google properties).

https://codereview.chromium.org/1117173004/diff/20001/chrome/browser/ssl/cert...
chrome/browser/ssl/certificate_error_report.cc:45:
cert_report_.SerializeToString(output);
On 2015/05/12 00:27:51, eroman wrote:
> This can return a failure. Why is it not necessary to check?

Oh I didn't realize it returned a value. Changed this method to return a bool.

https://codereview.chromium.org/1117173004/diff/20001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_cert_reporter.h (left):

https://codereview.chromium.org/1117173004/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_cert_reporter.h:22: // been sent off (not necessarily
after a response has been received).
On 2015/05/12 00:27:52, eroman wrote:
> I assume the reference to |callback| was obsolete? And completion is
best-effort

Yes, that's right.

[eroman, 2015-05-12 23:24:47]

lgtm

https://codereview.chromium.org/1117173004/diff/80001/chrome/browser/ssl/cert...
File chrome/browser/ssl/certificate_error_report.h (right):

https://codereview.chromium.org/1117173004/diff/80001/chrome/browser/ssl/cert...
chrome/browser/ssl/certificate_error_report.h:40: // Serializes the report. The
output will be a serialized
Could you describe the return value too?

[estark, 2015-05-13 00:46:01]

Thanks eroman.

rsleevi: would you like to take a look now? I could also ask felt to take a look
at //c/b/ssl if you'd prefer.

https://codereview.chromium.org/1117173004/diff/80001/chrome/browser/ssl/cert...
File chrome/browser/ssl/certificate_error_report.h (right):

https://codereview.chromium.org/1117173004/diff/80001/chrome/browser/ssl/cert...
chrome/browser/ssl/certificate_error_report.h:40: // Serializes the report. The
output will be a serialized
On 2015/05/12 23:24:47, eroman wrote:
> Could you describe the return value too?

Done.

[Ryan Sleevi, 2015-05-13 01:02:13]

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/net/cer...
File chrome/browser/net/certificate_error_reporter.cc (right):

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/net/cer...
chrome/browser/net/certificate_error_reporter.cc:53: crypto::HKDF
hkdf(std::string((char*)shared_secret, sizeof(shared_secret)),
drive by: Gotta use C++ casts, per
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Casting

std::string(reinterpret_cast<char*>(shared_secret), sizeof(shared_secret)

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/net/cer...
chrome/browser/net/certificate_error_reporter.cc:66: if (!aead.Seal(report,
nonce, "", &ciphertext)) {
s/""/std::string()/

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/net/cer...
chrome/browser/net/certificate_error_reporter.cc:74:
std::string((char*)public_key, sizeof(public_key)));
casts

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/net/cer...
chrome/browser/net/certificate_error_reporter.cc:190: server_private_key,
(uint8*)encrypted_report.client_public_key().data(),
casts

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/net/cer...
chrome/browser/net/certificate_error_reporter.cc:194: crypto::HKDF
hkdf(std::string((char*)shared_secret, sizeof(shared_secret)),
casts

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/net/cer...
chrome/browser/net/certificate_error_reporter.cc:206: return
aead.Open(encrypted_report.encrypted_report(), nonce, "",
s/""/std::string()/

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/net/cer...
File chrome/browser/net/certificate_error_reporter_unittest.cc (right):

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/net/cer...
chrome/browser/net/certificate_error_reporter_unittest.cc:156:
CheckUploadData(request, &expect_reports_, expect_request_encrypted_,
Note: None of these functions will abort the test if the ASSERT has failed.

https://code.google.com/p/googletest/wiki/AdvancedGuide#Using_Assertions_in_S...

Of course, you can't use that here, since you're expected to return an int. So
even if CheckUploadData fails, you'll get an int return, and you'll just keep
processing as normal.

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/ssl/cer...
File chrome/browser/ssl/certificate_error_report.h (right):

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/ssl/cer...
chrome/browser/ssl/certificate_error_report.h:10: #include
"chrome/browser/ssl/cert_logger.pb.h"
In general, including .pb.h in header files is a recipe for non-deterministic
build disasters due to transitive cross-target dependencies.

I... don't have a good summary of the issue... but could point at countless
build breaks due to protobuf & export_dependent_setting issues. Even GN hasn't
quite solved this so much as "danced around it".

To the extent possible, I would discourage you from putting this header in,
which likely means not making the CertLoggerRequest a member (or making it a
pointer member). Either are somewhat icky.

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/ssl/cer...
chrome/browser/ssl/certificate_error_report.h:14: };  // namespace net
no ;

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/ssl/cer...
chrome/browser/ssl/certificate_error_report.h:18: class CertLoggerRequest;
You can't forward declare this, because line 50 needs to allocate storage.

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/ssl/cer...
File chrome/browser/ssl/certificate_error_report_unittest.cc (right):

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/ssl/cer...
chrome/browser/ssl/certificate_error_report_unittest.cc:37:
chrome_browser_ssl::CertLoggerRequest::ERR_CERT_REVOKED;
Some appropriate newlines may help readability here

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/ssl/cer...
chrome/browser/ssl/certificate_error_report_unittest.cc:61: };
There's no need to use this common test fixture. You can just use TEST( on the
lines below, rather than TEST_F

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/ssl/chr...
File chrome/browser/ssl/chrome_fraudulent_certificate_reporter.cc (right):

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/ssl/chr...
chrome/browser/ssl/chrome_fraudulent_certificate_reporter.cc:59: }
In general, we prefer error handling first

if (!report.Serialize(...)) {
  LOG(ERROR) << blah
  return;
}

certificate_reporter_->SendReport(...)

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/ssl/ssl...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_blocking_page.cc:674: }
ditto earlier remarks about error handling

[estark, 2015-05-13 01:44:49]

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/net/cer...
File chrome/browser/net/certificate_error_reporter.cc (right):

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/net/cer...
chrome/browser/net/certificate_error_reporter.cc:53: crypto::HKDF
hkdf(std::string((char*)shared_secret, sizeof(shared_secret)),
On 2015/05/13 01:02:12, Ryan Sleevi wrote:
> drive by: Gotta use C++ casts, per
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Casting
> 
> std::string(reinterpret_cast<char*>(shared_secret), sizeof(shared_secret)

Done.

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/net/cer...
chrome/browser/net/certificate_error_reporter.cc:66: if (!aead.Seal(report,
nonce, "", &ciphertext)) {
On 2015/05/13 01:02:12, Ryan Sleevi wrote:
> s/""/std::string()/

Done.

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/net/cer...
chrome/browser/net/certificate_error_reporter.cc:74:
std::string((char*)public_key, sizeof(public_key)));
On 2015/05/13 01:02:12, Ryan Sleevi wrote:
> casts

Done.

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/net/cer...
chrome/browser/net/certificate_error_reporter.cc:190: server_private_key,
(uint8*)encrypted_report.client_public_key().data(),
On 2015/05/13 01:02:12, Ryan Sleevi wrote:
> casts

Done.

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/net/cer...
chrome/browser/net/certificate_error_reporter.cc:194: crypto::HKDF
hkdf(std::string((char*)shared_secret, sizeof(shared_secret)),
On 2015/05/13 01:02:12, Ryan Sleevi wrote:
> casts

Done.

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/net/cer...
chrome/browser/net/certificate_error_reporter.cc:206: return
aead.Open(encrypted_report.encrypted_report(), nonce, "",
On 2015/05/13 01:02:12, Ryan Sleevi wrote:
> s/""/std::string()/

Done.

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/net/cer...
File chrome/browser/net/certificate_error_reporter_unittest.cc (right):

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/net/cer...
chrome/browser/net/certificate_error_reporter_unittest.cc:156:
CheckUploadData(request, &expect_reports_, expect_request_encrypted_,
On 2015/05/13 01:02:12, Ryan Sleevi wrote:
> Note: None of these functions will abort the test if the ASSERT has failed.
> 
>
https://code.google.com/p/googletest/wiki/AdvancedGuide#Using_Assertions_in_S...
> 
> Of course, you can't use that here, since you're expected to return an int. So
> even if CheckUploadData fails, you'll get an int return, and you'll just keep
> processing as normal.

Interesting... I didn't know that, but can you clarify if you're suggesting a
change here? I think it's actually okay if the ASSERTs inside |CheckUploadData|
don't cause the test to abort (i.e. won't cause crashes).

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/ssl/cer...
File chrome/browser/ssl/certificate_error_report.h (right):

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/ssl/cer...
chrome/browser/ssl/certificate_error_report.h:10: #include
"chrome/browser/ssl/cert_logger.pb.h"
On 2015/05/13 01:02:12, Ryan Sleevi wrote:
> In general, including .pb.h in header files is a recipe for non-deterministic
> build disasters due to transitive cross-target dependencies.
> 
> I... don't have a good summary of the issue... but could point at countless
> build breaks due to protobuf & export_dependent_setting issues. Even GN hasn't
> quite solved this so much as "danced around it".
> 
> To the extent possible, I would discourage you from putting this header in,
> which likely means not making the CertLoggerRequest a member (or making it a
> pointer member). Either are somewhat icky.

Good to know; I changed it to be a pointer member.

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/ssl/cer...
chrome/browser/ssl/certificate_error_report.h:14: };  // namespace net
On 2015/05/13 01:02:12, Ryan Sleevi wrote:
> no ;

Done.

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/ssl/cer...
chrome/browser/ssl/certificate_error_report.h:18: class CertLoggerRequest;
On 2015/05/13 01:02:12, Ryan Sleevi wrote:
> You can't forward declare this, because line 50 needs to allocate storage.

Changed line 50 to be a scoped_ptr

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/ssl/cer...
File chrome/browser/ssl/certificate_error_report_unittest.cc (right):

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/ssl/cer...
chrome/browser/ssl/certificate_error_report_unittest.cc:37:
chrome_browser_ssl::CertLoggerRequest::ERR_CERT_REVOKED;
On 2015/05/13 01:02:12, Ryan Sleevi wrote:
> Some appropriate newlines may help readability here

Done. Do you think this would be a place to do |using
chrome_browser_ssl::CertLoggerRequest| to significantly improve readability?

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/ssl/cer...
chrome/browser/ssl/certificate_error_report_unittest.cc:61: };
On 2015/05/13 01:02:12, Ryan Sleevi wrote:
> There's no need to use this common test fixture. You can just use TEST( on the
> lines below, rather than TEST_F

Done.

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/ssl/chr...
File chrome/browser/ssl/chrome_fraudulent_certificate_reporter.cc (right):

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/ssl/chr...
chrome/browser/ssl/chrome_fraudulent_certificate_reporter.cc:59: }
On 2015/05/13 01:02:12, Ryan Sleevi wrote:
> In general, we prefer error handling first
> 
> if (!report.Serialize(...)) {
>   LOG(ERROR) << blah
>   return;
> }
> 
> certificate_reporter_->SendReport(...)

Done.

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/ssl/ssl...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_blocking_page.cc:674: }
On 2015/05/13 01:02:13, Ryan Sleevi wrote:
> ditto earlier remarks about error handling

Done.

[Ryan Sleevi, 2015-05-13 02:07:55]

lgtm

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/net/cer...
File chrome/browser/net/certificate_error_reporter_unittest.cc (right):

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/net/cer...
chrome/browser/net/certificate_error_reporter_unittest.cc:156:
CheckUploadData(request, &expect_reports_, expect_request_encrypted_,
On 2015/05/13 01:44:49, estark wrote:
> On 2015/05/13 01:02:12, Ryan Sleevi wrote:
> > Note: None of these functions will abort the test if the ASSERT has failed.
> > 
> >
>
https://code.google.com/p/googletest/wiki/AdvancedGuide#Using_Assertions_in_S...
> > 
> > Of course, you can't use that here, since you're expected to return an int.
So
> > even if CheckUploadData fails, you'll get an int return, and you'll just
keep
> > processing as normal.
> 
> Interesting... I didn't know that, but can you clarify if you're suggesting a
> change here? I think it's actually okay if the ASSERTs inside
|CheckUploadData|
> don't cause the test to abort (i.e. won't cause crashes).

Well, ASSERTs wont cause the test to crash either.

An ASSERT in a subroutine will just AddFailure, but it wont stop test execution.
So for example if you were ASSERT_TRUE'ing on a pointer, and then using that
pointer outside the subroutine, "bad things" will happen.

I think this is fine, but it just means all your ASSERT_BLAH outside of a test
won't actually stop the test - but they WILL stop your (void returning)
subroutine, which may not be what you want.

[estark, 2015-05-13 04:13:41]

estark@chromium.org changed reviewers:
+ mattm@chromium.org, mmenke@chromium.org

[estark, 2015-05-13 04:13:42]

Thanks rsleevi.

mattm: can you please take a look at changes in //chrome/browser/safe_browsing?

mmenke: can you please look at changes in
//chrome/browser/profiles/profile_io_data.cc?

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/net/cer...
File chrome/browser/net/certificate_error_reporter_unittest.cc (right):

https://codereview.chromium.org/1117173004/diff/100001/chrome/browser/net/cer...
chrome/browser/net/certificate_error_reporter_unittest.cc:156:
CheckUploadData(request, &expect_reports_, expect_request_encrypted_,
On 2015/05/13 02:07:55, Ryan Sleevi wrote:
> On 2015/05/13 01:44:49, estark wrote:
> > On 2015/05/13 01:02:12, Ryan Sleevi wrote:
> > > Note: None of these functions will abort the test if the ASSERT has
failed.
> > > 
> > >
> >
>
https://code.google.com/p/googletest/wiki/AdvancedGuide#Using_Assertions_in_S...
> > > 
> > > Of course, you can't use that here, since you're expected to return an
int.
> So
> > > even if CheckUploadData fails, you'll get an int return, and you'll just
> keep
> > > processing as normal.
> > 
> > Interesting... I didn't know that, but can you clarify if you're suggesting
a
> > change here? I think it's actually okay if the ASSERTs inside
> |CheckUploadData|
> > don't cause the test to abort (i.e. won't cause crashes).
> 
> Well, ASSERTs wont cause the test to crash either.
> 
> An ASSERT in a subroutine will just AddFailure, but it wont stop test
execution.
> So for example if you were ASSERT_TRUE'ing on a pointer, and then using that
> pointer outside the subroutine, "bad things" will happen.
> 
> I think this is fine, but it just means all your ASSERT_BLAH outside of a test
> won't actually stop the test - but they WILL stop your (void returning)
> subroutine, which may not be what you want.

Ah, yeah, what I meant was that I don't think a failed ASSERT inside
CheckUploadData will cause a crash or other bad things after stopping the
subroutine. So I think we're in agreement.

[estark, 2015-05-13 04:15:35]

estark@chromium.org changed reviewers:
+ sky@chromium.org

[estark, 2015-05-13 04:15:36]

sky: can you please look at the BUILD.gn files?

[mmenke, 2015-05-13 15:02:05]

profiles/ LGTM

https://codereview.chromium.org/1117173004/diff/140001/chrome/browser/ssl/cer...
File chrome/browser/ssl/certificate_error_report.h (right):

https://codereview.chromium.org/1117173004/diff/140001/chrome/browser/ssl/cer...
chrome/browser/ssl/certificate_error_report.h:16: namespace chrome_browser_ssl {
I defer to others, but I think the consensus now is not to use namespaces like
this?  The old one was just grandfathered in.

[sky, 2015-05-13 15:52:47]

LGTM

[mattm, 2015-05-13 17:35:55]

safe_browsing lgtm

[estark, 2015-05-13 17:40:38]

Thanks all!

Does anyone have a strong opinion on the issue mmenke raised? (That is, should
the new classes in //chrome/browser/ssl be defined in the chrome_browser_ssl
namespace, or just in the global namespace?) If no one objects, I'll move them
into the global namespace to be more consistent with the rest of
//chrome/browser/ssl.

https://codereview.chromium.org/1117173004/diff/140001/chrome/browser/ssl/cer...
File chrome/browser/ssl/certificate_error_report.h (right):

https://codereview.chromium.org/1117173004/diff/140001/chrome/browser/ssl/cer...
chrome/browser/ssl/certificate_error_report.h:16: namespace chrome_browser_ssl {
On 2015/05/13 15:02:05, mmenke wrote:
> I defer to others, but I think the consensus now is not to use namespaces like
> this?  The old one was just grandfathered in.

Ah, so instead we'd just put CertificateErrorReporter in the global namespace? I
suppose that's what everything else in //c/b/ssl does.

[mmenke, 2015-05-13 18:03:01]

https://codereview.chromium.org/1117173004/diff/140001/chrome/browser/ssl/cer...
File chrome/browser/ssl/certificate_error_report.h (right):

https://codereview.chromium.org/1117173004/diff/140001/chrome/browser/ssl/cer...
chrome/browser/ssl/certificate_error_report.h:16: namespace chrome_browser_ssl {
On 2015/05/13 17:40:38, estark wrote:
> On 2015/05/13 15:02:05, mmenke wrote:
> > I defer to others, but I think the consensus now is not to use namespaces
like
> > this?  The old one was just grandfathered in.
> 
> Ah, so instead we'd just put CertificateErrorReporter in the global namespace?
I
> suppose that's what everything else in //c/b/ssl does.

Correct.  That's what most things do.  I can't find the discussion on
chromium-dev that ended with the conclusion it was a bad idea - maybe the
discussion I'm thinking of was on a bug - but there's a reason most of the newer
c/b/net classes don't use the namespace.

[estark, 2015-05-13 20:38:43]

https://codereview.chromium.org/1117173004/diff/140001/chrome/browser/ssl/cer...
File chrome/browser/ssl/certificate_error_report.h (right):

https://codereview.chromium.org/1117173004/diff/140001/chrome/browser/ssl/cer...
chrome/browser/ssl/certificate_error_report.h:16: namespace chrome_browser_ssl {
On 2015/05/13 18:03:01, mmenke wrote:
> On 2015/05/13 17:40:38, estark wrote:
> > On 2015/05/13 15:02:05, mmenke wrote:
> > > I defer to others, but I think the consensus now is not to use namespaces
> like
> > > this?  The old one was just grandfathered in.
> > 
> > Ah, so instead we'd just put CertificateErrorReporter in the global
namespace?
> I
> > suppose that's what everything else in //c/b/ssl does.
> 
> Correct.  That's what most things do.  I can't find the discussion on
> chromium-dev that ended with the conclusion it was a bad idea - maybe the
> discussion I'm thinking of was on a bug - but there's a reason most of the
newer
> c/b/net classes don't use the namespace.

Done.

[estark, 2015-05-13 20:39:18]

The CQ bit was checked by estark@chromium.org

[estark, 2015-05-13 20:39:19]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org,
rsleevi@chromium.org, sky@chromium.org, mmenke@chromium.org, mattm@chromium.org
Link to the patchset: https://codereview.chromium.org/1117173004/#ps160001
(title: "move //c/b/ssl classes into global namespace")

[commit-bot: I haz the power, 2015-05-13 20:40:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1117173004/160001

[mmenke, 2015-05-13 20:40:26]

https://codereview.chromium.org/1117173004/diff/160001/chrome/browser/ssl/cer...
File chrome/browser/ssl/certificate_error_report.h (right):

https://codereview.chromium.org/1117173004/diff/160001/chrome/browser/ssl/cer...
chrome/browser/ssl/certificate_error_report.h:20: //
chrome_browser_net::CertificateErrorReporter.
nit:  --chrome_browser_net::

[estark, 2015-05-13 20:42:13]

https://codereview.chromium.org/1117173004/diff/160001/chrome/browser/ssl/cer...
File chrome/browser/ssl/certificate_error_report.h (right):

https://codereview.chromium.org/1117173004/diff/160001/chrome/browser/ssl/cer...
chrome/browser/ssl/certificate_error_report.h:20: //
chrome_browser_net::CertificateErrorReporter.
On 2015/05/13 20:40:26, mmenke wrote:
> nit:  --chrome_browser_net::

How come? CertificateErrorReporter is still defined inside chrome_browser_net.
(It was already there before this CL, though I suppose I could move it into the
global namespace as well.)

[mmenke, 2015-05-13 20:44:22]

https://codereview.chromium.org/1117173004/diff/160001/chrome/browser/ssl/cer...
File chrome/browser/ssl/certificate_error_report.h (right):

https://codereview.chromium.org/1117173004/diff/160001/chrome/browser/ssl/cer...
chrome/browser/ssl/certificate_error_report.h:20: //
chrome_browser_net::CertificateErrorReporter.
On 2015/05/13 20:42:13, estark wrote:
> On 2015/05/13 20:40:26, mmenke wrote:
> > nit:  --chrome_browser_net::
> 
> How come? CertificateErrorReporter is still defined inside chrome_browser_net.
> (It was already there before this CL, though I suppose I could move it into
the
> global namespace as well.)

Oops...sorry, got "CertificateErrorReporter" and "CertificateErrorReport"
confused.

[commit-bot: I haz the power, 2015-05-13 22:02:05]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-05-13 22:03:35]

Patchset 9 (id:??) landed as
https://crrev.com/4282f117db4ddb6b307efad849ec6635095e9cd1
Cr-Commit-Position: refs/heads/master@{#329723}