Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
979893003:
Refactor ChromeFraudulentCertReporter for code reuse by SSL reporting (Closed)
DescriptionRefactor ChromeFraudulentCertReporter for code reuse by SSL reporting
This CL introduces two new classes: SSLInterstitialCertificateReporter,
which will be used to send certificate reports from SSL blocking pages,
and ChromeCertificateReporter, which implements functionality that
SSLInterstitialCertificateReporter and
ChromeFraudulentCertificateReporter share.
BUG=461588, 462713
Committed: https://crrev.com/4c9dc455208c9526888497525aba8a294f456edf
Cr-Commit-Position: refs/heads/master@{#320375}
Patch Set 1 #
Total comments: 20
Patch Set 2 : rsleevi's comments: remove SSLInterstitialCertReporter, ChromeFraudCertReporter uses CertificateErr… #Patch Set 3 : remove stray newline #
Total comments: 11
Patch Set 4 : rsleevi's comments + fix CreateURLRequest confusion #
Total comments: 20
Patch Set 5 : rsleevi's comments #
Total comments: 16
Patch Set 6 : rsleevi comments + unit test for SendReport #Patch Set 7 : add more unit tests for CertificateErrorReporter #Patch Set 8 : fix GURL #include placement #
Total comments: 37
Patch Set 9 : style fixes, don't use SetUrlRequestMocksEnabled, move RunLoop into tests #Patch Set 10 : add missing include #
Total comments: 2
Patch Set 11 : document test_mail_google_com.pem format, add a RunUntilIdle, style fixes #
Total comments: 56
Patch Set 12 : add tests for uploaded data, move callback to network delegate, other requested fixes #Patch Set 13 : update a comment #
Total comments: 19
Patch Set 14 : rebase #Patch Set 15 : test style fixes #
Total comments: 14
Patch Set 16 : style fixes, add comments #Patch Set 17 : remove FILE_PATH_LITERAL; ImportCertFromFile uses AppendASCII #Patch Set 18 : use URLRequestMockDataJob to avoid IO in unit test #Messages
Total messages: 61 (10 generated)
estark@chromium.org changed reviewers: + felt@chromium.org, rsleevi@chromium.org
This is an attempted refactoring as suggested by Ryan in https://codereview.chromium.org/935663004/. It preserves net::FraudulentCertificateReporter as an interface for reporting pinning violations, which is implemented by ChromeFraudulentCertificateReporter, which shares code with SSLInterstitialCertificateReporter. I ran into a couple questions that I left inline. Thanks! https://codereview.chromium.org/979893003/diff/1/chrome/browser/net/chrome_ce... File chrome/browser/net/chrome_certificate_reporter.h (right): https://codereview.chromium.org/979893003/diff/1/chrome/browser/net/chrome_ce... chrome/browser/net/chrome_certificate_reporter.h:23: class ChromeCertificateReporter : public net::URLRequest::Delegate { Is this name confusingly similar to ChromeFraudulentCertificateReporter? Should I rename to ChromeCertificateReporterImpl or...? https://codereview.chromium.org/979893003/diff/1/chrome/browser/net/chrome_ce... File chrome/browser/net/chrome_certificate_reporter_unittest.cc (right): https://codereview.chromium.org/979893003/diff/1/chrome/browser/net/chrome_ce... chrome/browser/net/chrome_certificate_reporter_unittest.cc:60: I want to add another test here that tests that the report actually gets sent. My plan was that I would create a TestURLRequestContext that overrides GetURLRequest to return a TestURLRequest that asserts that the upload data is as expected, but that would require making URLRequestContext::GetURLRequest virtual -- would that be okay?
By the way, once this general direction is ironed out, I'll rebase the related CLs on top of this (https://codereview.chromium.org/935663004/, https://codereview.chromium.org/967383003/, https://codereview.chromium.org/975623002/ [rough draft]).
https://codereview.chromium.org/979893003/diff/1/chrome/browser/net/chrome_ce... File chrome/browser/net/chrome_certificate_reporter.h (right): https://codereview.chromium.org/979893003/diff/1/chrome/browser/net/chrome_ce... chrome/browser/net/chrome_certificate_reporter.h:23: class ChromeCertificateReporter : public net::URLRequest::Delegate { On 2015/03/04 17:33:49, emily wrote: > Is this name confusingly similar to ChromeFraudulentCertificateReporter? Should > I rename to ChromeCertificateReporterImpl or...? It might make sense to name them something like: ChromeCertificateReporterImpl GooglePinningViolationReporter ExtendedDomainVIolationReporter or maybe not that, but something that distinguishes between what the purposes are. https://codereview.chromium.org/979893003/diff/1/chrome/browser/ssl/ssl_inter... File chrome/browser/ssl/ssl_interstitial_certificate_reporter.h (right): https://codereview.chromium.org/979893003/diff/1/chrome/browser/ssl/ssl_inter... chrome/browser/ssl/ssl_interstitial_certificate_reporter.h:20: class SSLInterstitialCertificateReporter Small detail, but I'm not sure if this is the best name for it. The thing that really distinguishes this from the pinning violation report is the opt-in (and therefore the expanded nature of the reports), not where it's invoked from. It might be more appropriate to name it ExtendedCertificateReporter, OptionalCertificateReporter, or something else.
https://codereview.chromium.org/979893003/diff/1/chrome/browser/net/chrome_ce... File chrome/browser/net/chrome_certificate_reporter.cc (right): https://codereview.chromium.org/979893003/diff/1/chrome/browser/net/chrome_ce... chrome/browser/net/chrome_certificate_reporter.cc:26: const std::string& upload_url) Pass the upload_url as a GURL https://codereview.chromium.org/979893003/diff/1/chrome/browser/net/chrome_ce... chrome/browser/net/chrome_certificate_reporter.cc:27: : request_context_(request_context), upload_url_(upload_url) { What if someone gives us an empty GURL(). Is that valid? [Hint: I don't think it should be] https://codereview.chromium.org/979893003/diff/1/chrome/browser/net/chrome_ce... chrome/browser/net/chrome_certificate_reporter.cc:52: // appealing to the user. This is just wrong/misleading. Let's just delete it :) Or file a bug on it. https://codereview.chromium.org/979893003/diff/1/chrome/browser/net/chrome_ce... chrome/browser/net/chrome_certificate_reporter.cc:57: "422516 ChromeCertificateReporter::OnResponseStarted")); I'm going to suggest this be removed; at least, moved into the ChromeFraudulentCertReporter, and not reported for the opt-in case https://codereview.chromium.org/979893003/diff/1/chrome/browser/net/chrome_ce... File chrome/browser/net/chrome_certificate_reporter.h (right): https://codereview.chromium.org/979893003/diff/1/chrome/browser/net/chrome_ce... chrome/browser/net/chrome_certificate_reporter.h:23: class ChromeCertificateReporter : public net::URLRequest::Delegate { On 2015/03/04 19:15:47, felt wrote: > On 2015/03/04 17:33:49, emily wrote: > > Is this name confusingly similar to ChromeFraudulentCertificateReporter? > Should > > I rename to ChromeCertificateReporterImpl or...? > > It might make sense to name them something like: > > ChromeCertificateReporterImpl > GooglePinningViolationReporter Eh, Chris named it for a reason, but I suppose that reason isn't exactly secret :) > ExtendedDomainVIolationReporter > > or maybe not that, but something that distinguishes between what the purposes > are. https://codereview.chromium.org/979893003/diff/1/chrome/browser/net/chrome_ce... chrome/browser/net/chrome_certificate_reporter.h:23: class ChromeCertificateReporter : public net::URLRequest::Delegate { CertificateErrorReporter ? https://codereview.chromium.org/979893003/diff/1/chrome/browser/net/chrome_fr... File chrome/browser/net/chrome_fraudulent_certificate_reporter.h (right): https://codereview.chromium.org/979893003/diff/1/chrome/browser/net/chrome_fr... chrome/browser/net/chrome_fraudulent_certificate_reporter.h:23: public ChromeCertificateReporter { In general, multiple implementation inheritance is BAD NEWS. UI code is somewhat of a special snowflake here, but that's because the methods DON'T shadow eachother (like SendReport does) That is, there should be net::FraudulentCertificateReporter CertificateErrorReporter ChromeFraudulentCertificateReporter - implements net::FraudulentCertificateReporter - *uses* CertificateErrorReporter SSLBlockingPage (since it's in //chrome) should use CertificateErrorReporter directly SSLBlockingPage should *not* depend on the net::FraudulentCertificateReporter (which making it depend on this for the ChromeCertificateReporter is an API smell) If you want to know more about why it smells, this is the I in SOLID (Interface Segregation Principle). That is, it's better to have two interfaces (one for net::FraudulentCertificateReporter and one for the SSLBlockingPage::Reporter, to say hypothetical) than one generic interface. Similarly, the D (Dependency Inversion Principle) is that you should depend on abstractions. On a more practical level, this is dangerously close to the Diamond Problem.
https://codereview.chromium.org/979893003/diff/1/chrome/browser/net/chrome_ce... File chrome/browser/net/chrome_certificate_reporter.cc (right): https://codereview.chromium.org/979893003/diff/1/chrome/browser/net/chrome_ce... chrome/browser/net/chrome_certificate_reporter.cc:52: // appealing to the user. On 2015/03/04 19:31:01, Ryan Sleevi wrote: > This is just wrong/misleading. Let's just delete it :) > > Or file a bug on it. Do you mean the whole comment, or the last part ("appealing to the user")?
On 2015/03/04 19:58:46, emily wrote: > https://codereview.chromium.org/979893003/diff/1/chrome/browser/net/chrome_ce... > File chrome/browser/net/chrome_certificate_reporter.cc (right): > > https://codereview.chromium.org/979893003/diff/1/chrome/browser/net/chrome_ce... > chrome/browser/net/chrome_certificate_reporter.cc:52: // appealing to the user. > On 2015/03/04 19:31:01, Ryan Sleevi wrote: > > This is just wrong/misleading. Let's just delete it :) > > > > Or file a bug on it. > > Do you mean the whole comment, or the last part ("appealing to the user")? Well, I just meant that it's been over a year. The "real soon" isn't exactly coming real soon. If we do want to do that, perhaps file a bug and leave it available. Or just accept that it's a known limitation and no need to bother suggesting we'll be doing better.
https://codereview.chromium.org/979893003/diff/1/chrome/browser/net/chrome_ce... File chrome/browser/net/chrome_certificate_reporter.cc (right): https://codereview.chromium.org/979893003/diff/1/chrome/browser/net/chrome_ce... chrome/browser/net/chrome_certificate_reporter.cc:26: const std::string& upload_url) On 2015/03/04 19:31:01, Ryan Sleevi wrote: > Pass the upload_url as a GURL Done. https://codereview.chromium.org/979893003/diff/1/chrome/browser/net/chrome_ce... chrome/browser/net/chrome_certificate_reporter.cc:27: : request_context_(request_context), upload_url_(upload_url) { On 2015/03/04 19:31:01, Ryan Sleevi wrote: > What if someone gives us an empty GURL(). Is that valid? > > [Hint: I don't think it should be] Is DCHECK the right way to handle an empty GURL? That's what I've done in this patch set. https://codereview.chromium.org/979893003/diff/1/chrome/browser/net/chrome_ce... chrome/browser/net/chrome_certificate_reporter.cc:52: // appealing to the user. On 2015/03/04 19:31:01, Ryan Sleevi wrote: > This is just wrong/misleading. Let's just delete it :) > > Or file a bug on it. Done (deleted it). https://codereview.chromium.org/979893003/diff/1/chrome/browser/net/chrome_ce... chrome/browser/net/chrome_certificate_reporter.cc:57: "422516 ChromeCertificateReporter::OnResponseStarted")); On 2015/03/04 19:31:01, Ryan Sleevi wrote: > I'm going to suggest this be removed; at least, moved into the > ChromeFraudulentCertReporter, and not reported for the opt-in case Done. https://codereview.chromium.org/979893003/diff/1/chrome/browser/net/chrome_ce... File chrome/browser/net/chrome_certificate_reporter.h (right): https://codereview.chromium.org/979893003/diff/1/chrome/browser/net/chrome_ce... chrome/browser/net/chrome_certificate_reporter.h:23: class ChromeCertificateReporter : public net::URLRequest::Delegate { On 2015/03/04 19:31:01, Ryan Sleevi wrote: > On 2015/03/04 19:15:47, felt wrote: > > On 2015/03/04 17:33:49, emily wrote: > > > Is this name confusingly similar to ChromeFraudulentCertificateReporter? > > Should > > > I rename to ChromeCertificateReporterImpl or...? > > > > It might make sense to name them something like: > > > > ChromeCertificateReporterImpl > > GooglePinningViolationReporter > > Eh, Chris named it for a reason, but I suppose that reason isn't exactly secret > :) > > > ExtendedDomainVIolationReporter > > > > or maybe not that, but something that distinguishes between what the purposes > > are. > Now it's ChromeFraudulentCertificateReporter and CertificateErrorReporter, and SSLInterstiitialCertificateReporter is gone. https://codereview.chromium.org/979893003/diff/1/chrome/browser/net/chrome_fr... File chrome/browser/net/chrome_fraudulent_certificate_reporter.h (right): https://codereview.chromium.org/979893003/diff/1/chrome/browser/net/chrome_fr... chrome/browser/net/chrome_fraudulent_certificate_reporter.h:23: public ChromeCertificateReporter { On 2015/03/04 19:31:01, Ryan Sleevi wrote: > In general, multiple implementation inheritance is BAD NEWS. UI code is somewhat > of a special snowflake here, but that's because the methods DON'T shadow > eachother (like SendReport does) > > That is, there should be > > net::FraudulentCertificateReporter > CertificateErrorReporter > ChromeFraudulentCertificateReporter - implements > net::FraudulentCertificateReporter > - *uses* CertificateErrorReporter > > SSLBlockingPage (since it's in //chrome) should use CertificateErrorReporter > directly > SSLBlockingPage should *not* depend on the net::FraudulentCertificateReporter > (which making it depend on this for the ChromeCertificateReporter is an API > smell) > > If you want to know more about why it smells, this is the I in SOLID (Interface > Segregation Principle). That is, it's better to have two interfaces (one for > net::FraudulentCertificateReporter and one for the SSLBlockingPage::Reporter, to > say hypothetical) than one generic interface. Similarly, the D (Dependency > Inversion Principle) is that you should depend on abstractions. > > On a more practical level, this is dangerously close to the Diamond Problem. Done, if by "ChromeFraudulentCertificateReporter uses CertificateErrorReporter" you mean "ChromeFraudulentCertificateReporter has a CertificateErrorReporter member variable that it uses to implement net::FraudulentCertificateReport and net::URLRequest::Delegate" -- is that what you mean? https://codereview.chromium.org/979893003/diff/1/chrome/browser/ssl/ssl_inter... File chrome/browser/ssl/ssl_interstitial_certificate_reporter.h (right): https://codereview.chromium.org/979893003/diff/1/chrome/browser/ssl/ssl_inter... chrome/browser/ssl/ssl_interstitial_certificate_reporter.h:20: class SSLInterstitialCertificateReporter On 2015/03/04 19:15:47, felt wrote: > Small detail, but I'm not sure if this is the best name for it. The thing that > really distinguishes this from the pinning violation report is the opt-in (and > therefore the expanded nature of the reports), not where it's invoked from. It > might be more appropriate to name it ExtendedCertificateReporter, > OptionalCertificateReporter, or something else. This class is gone now (see previous comment).
ping rsleevi! (sorry to be annoying... we have a fair amount of stuff blocking on this CL)
Thanks for the ping :) Never hestitate with those. https://codereview.chromium.org/979893003/diff/40001/chrome/browser/net/certi... File chrome/browser/net/certificate_error_reporter.cc (right): https://codereview.chromium.org/979893003/diff/40001/chrome/browser/net/certi... chrome/browser/net/certificate_error_reporter.cc:60: DVLOG(20) << "SSL report for " << hostname << ":\n" << out << "\n\n"; DVLOG(3) should be sufficient. That's the max noisiness. If someone complains, well, they can use DLOG filtering. https://codereview.chromium.org/979893003/diff/40001/chrome/browser/net/certi... File chrome/browser/net/certificate_error_reporter.h (right): https://codereview.chromium.org/979893003/diff/40001/chrome/browser/net/certi... chrome/browser/net/certificate_error_reporter.h:30: enum ReportType { STYLE: These go first ( http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O... ) https://codereview.chromium.org/979893003/diff/40001/chrome/browser/net/certi... chrome/browser/net/certificate_error_reporter.h:36: // type. Used by SendReport. Documentation: From reading the header, I'm not really sure what this does or why it does it. From a layering bit "Used by SendReport" seems to be unnecessary/too much detail. https://codereview.chromium.org/979893003/diff/40001/chrome/browser/net/certi... chrome/browser/net/certificate_error_reporter.h:53: static void BuildReport(const std::string& hostname, Why is this static protected? It seems like these are only protected for unit tests, in which case, you should really move them to private and use gtest_prod_util's FRIEND_TEST_ALL_PREFIXES https://codereview.chromium.org/979893003/diff/40001/chrome/browser/net/certi... chrome/browser/net/certificate_error_reporter.h:61: net::URLRequestContext* const request_context_; STYLE: Data members should be private ( http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Access_Control )
Patchset #4 (id:60001) has been deleted
Thanks Ryan! https://codereview.chromium.org/979893003/diff/40001/chrome/browser/net/certi... File chrome/browser/net/certificate_error_reporter.cc (right): https://codereview.chromium.org/979893003/diff/40001/chrome/browser/net/certi... chrome/browser/net/certificate_error_reporter.cc:60: DVLOG(20) << "SSL report for " << hostname << ":\n" << out << "\n\n"; On 2015/03/06 00:21:32, Ryan Sleevi wrote: > DVLOG(3) should be sufficient. That's the max noisiness. If someone complains, > well, they can use DLOG filtering. Done. https://codereview.chromium.org/979893003/diff/40001/chrome/browser/net/certi... File chrome/browser/net/certificate_error_reporter.h (right): https://codereview.chromium.org/979893003/diff/40001/chrome/browser/net/certi... chrome/browser/net/certificate_error_reporter.h:30: enum ReportType { On 2015/03/06 00:21:32, Ryan Sleevi wrote: > STYLE: These go first ( > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O... > ) Done. https://codereview.chromium.org/979893003/diff/40001/chrome/browser/net/certi... chrome/browser/net/certificate_error_reporter.h:36: // type. Used by SendReport. On 2015/03/06 00:21:32, Ryan Sleevi wrote: > Documentation: From reading the header, I'm not really sure what this does or > why it does it. > > From a layering bit "Used by SendReport" seems to be unnecessary/too much > detail. Ok, this was actually a bit broken. CertificateErrorReporter::CreateURLRequest was being called by ChromeFraudulentCertificateReporter::CreateURLRequest, which was overridden by MockReporter in chrome_fraudulent_certificate_reporter_unittest.cc with the intention of using a mock URLRequest. But MockReporter::CreateURLRequest wasn't actually getting called anywhere anymore in this refactoring (since it's now CertificateErrorReporter::SendReport which calls calls CertificateErrorReporter::CreateURLRequest). I changed things around so that the unit test creates a mock CertificateErrorReporter that creates mock URLRequests, and added a hook for the unit test to set the CertificateErrorReporter that ChromeFraudulentCertificateReporter uses. This allows the unit test to use mock URLRequests as it's intended to, but the downside is that it's no longer easy for production code to subclass ChromeFraudulentCertificateReporter and override CreateURLRequest. On the other hand, hopefully that downside doesn't matter because there wasn't any production code actually doing that. I updated the comment on CertificateErrorReporter::CreateURLRequest, made it private (no reason for it to be public, as far as I can tell), and removed ChromeFraudulentCertificateReporter::CreateURLRequest. https://codereview.chromium.org/979893003/diff/40001/chrome/browser/net/certi... chrome/browser/net/certificate_error_reporter.h:53: static void BuildReport(const std::string& hostname, On 2015/03/06 00:21:32, Ryan Sleevi wrote: > Why is this static protected? > > It seems like these are only protected for unit tests, in which case, you should > really move them to private and use gtest_prod_util's FRIEND_TEST_ALL_PREFIXES Done. (Were you also asking why is it static, not just why is it protected? Should it not be static?) https://codereview.chromium.org/979893003/diff/40001/chrome/browser/net/certi... chrome/browser/net/certificate_error_reporter.h:61: net::URLRequestContext* const request_context_; On 2015/03/06 00:21:32, Ryan Sleevi wrote: > STYLE: Data members should be private ( > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Access_Control ) Done. https://codereview.chromium.org/979893003/diff/40001/chrome/browser/net/certi... chrome/browser/net/certificate_error_reporter.h:61: net::URLRequestContext* const request_context_; On 2015/03/06 00:21:32, Ryan Sleevi wrote: > STYLE: Data members should be private ( > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Access_Control ) Done.
More comments :) https://codereview.chromium.org/979893003/diff/80001/chrome/browser/net/certi... File chrome/browser/net/certificate_error_reporter.cc (right): https://codereview.chromium.org/979893003/diff/80001/chrome/browser/net/certi... chrome/browser/net/certificate_error_reporter.cc:46: case REPORT_TYPE_EXTENDED_REPORTING: Not something I'm worried about for this CL, since I know you're working on the other bits, but normally when splitting this up, we'd leave out the REPORT_TYPE_EXTENDED_REPORTING from the enum and implementation until your secondary CL (which would then add and wire it up) Just more of a note for the future in terms of "where do I split the CL" https://codereview.chromium.org/979893003/diff/80001/chrome/browser/net/certi... File chrome/browser/net/certificate_error_reporter.h (right): https://codereview.chromium.org/979893003/diff/80001/chrome/browser/net/certi... chrome/browser/net/certificate_error_reporter.h:31: explicit CertificateErrorReporter(net::URLRequestContext* request_context, You don't need explicit here because it's a two-arg ctor. https://codereview.chromium.org/979893003/diff/80001/chrome/browser/net/certi... File chrome/browser/net/certificate_error_reporter_unittest.cc (right): https://codereview.chromium.org/979893003/diff/80001/chrome/browser/net/certi... chrome/browser/net/certificate_error_reporter_unittest.cc:19: const std::string kHostname = "test.mail.google.com"; You should put all of these (including the test) in an unnamed namespace, so that you don't run into any ODR issues https://codereview.chromium.org/979893003/diff/80001/chrome/browser/net/certi... chrome/browser/net/certificate_error_reporter_unittest.cc:21: static SSLInfo GetSSLInfo() { /static/ is not needed here (and perversely, undesirable) once you move it into an unnamed namespace https://codereview.chromium.org/979893003/diff/80001/chrome/browser/net/certi... chrome/browser/net/certificate_error_reporter_unittest.cc:39: TEST(CertificateErrorReporterTest, ReportIsBuiltCorrectly) { This test is mostly testing the implementation detail, rather than testing the public API. That is, it'd be good to have a test that SendReport is working as documented. For example, if you create a reporter with a URL for an endpoint, does it create a new URLRequest when you SendReport()? Does it do the right thing if that URL fails to load? What happens if you have an outstanding request and you delete the Reporter - does it properly cancel the request? (Yes, these are all tests that should have been written for the original code) Because this is refactoring existing code, I'm inclined to give you a short-term pass on the testing, but it would be great to address these soon. (Yes, this is the cost of touching code; someone inevitably asks for moar tests even when you just Want To Land Things) https://codereview.chromium.org/979893003/diff/80001/chrome/browser/net/chrom... File chrome/browser/net/chrome_fraudulent_certificate_reporter.h (right): https://codereview.chromium.org/979893003/diff/80001/chrome/browser/net/chrom... chrome/browser/net/chrome_fraudulent_certificate_reporter.h:23: public net::URLRequest::Delegate { You no longer need to be a net::URLRequest::Delegate, since nothing ever passes this to URLRequests https://codereview.chromium.org/979893003/diff/80001/chrome/browser/net/chrom... chrome/browser/net/chrome_fraudulent_certificate_reporter.h:31: scoped_ptr<CertificateErrorReporter> certificate_reporter); Then you can make this constructor private and friend test again. Note that again, the comment is describing "why", rather than "how" // Useful for tests to use a mock reporter. https://codereview.chromium.org/979893003/diff/80001/chrome/browser/net/chrom... chrome/browser/net/chrome_fraudulent_certificate_reporter.h:41: void OnReadCompleted(net::URLRequest* request, int bytes_read) override; You can delete these two methods https://codereview.chromium.org/979893003/diff/80001/chrome/browser/net/chrom... File chrome/browser/net/chrome_fraudulent_certificate_reporter_unittest.cc (right): https://codereview.chromium.org/979893003/diff/80001/chrome/browser/net/chrom... chrome/browser/net/chrome_fraudulent_certificate_reporter_unittest.cc:128: CertificateErrorReporter::SendReport(type, hostname, ssl_info); You don't actually need to forward this on, do you? It's just a mock - it doesn't actually implement the functionality (beyond the minimum necessary)
https://codereview.chromium.org/979893003/diff/80001/chrome/browser/net/certi... File chrome/browser/net/certificate_error_reporter_unittest.cc (right): https://codereview.chromium.org/979893003/diff/80001/chrome/browser/net/certi... chrome/browser/net/certificate_error_reporter_unittest.cc:39: TEST(CertificateErrorReporterTest, ReportIsBuiltCorrectly) { On 2015/03/06 19:23:13, Ryan Sleevi wrote: > This test is mostly testing the implementation detail, rather than testing the > public API. > > That is, it'd be good to have a test that SendReport is working as documented. > > For example, if you create a reporter with a URL for an endpoint, does it create > a new URLRequest when you SendReport()? Does it do the right thing if that URL > fails to load? What happens if you have an outstanding request and you delete > the Reporter - does it properly cancel the request? > > (Yes, these are all tests that should have been written for the original code) > > Because this is refactoring existing code, I'm inclined to give you a short-term > pass on the testing, but it would be great to address these soon. > > (Yes, this is the cost of touching code; someone inevitably asks for moar tests > even when you just Want To Land Things) I had a question about writing those kinds of tests in the first patch set that you might have missed: https://codereview.chromium.org/979893003/diff/1/chrome/browser/net/chrome_ce...
rsleevi@chromium.org changed reviewers: + mmenke@chromium.org
D'oh! I missed that comment. +mmenke, who loves tests and will know where best to point you for testing that a URLRequest is made. Which I should learn. https://codereview.chromium.org/979893003/diff/1/chrome/browser/net/chrome_ce... File chrome/browser/net/chrome_certificate_reporter_unittest.cc (right): https://codereview.chromium.org/979893003/diff/1/chrome/browser/net/chrome_ce... chrome/browser/net/chrome_certificate_reporter_unittest.cc:60: On 2015/03/04 17:33:49, emily wrote: > I want to add another test here that tests that the report actually gets sent. > My plan was that I would create a TestURLRequestContext that overrides > GetURLRequest to return a TestURLRequest that asserts that the upload data is as > expected, but that would require making URLRequestContext::GetURLRequest virtual > -- would that be okay? Testing the data is as expected is testing the implementation, not the interface; I wouldn't recommend that. mmenke@ will know whatever the "Right Way" to test this interface would be; I can never keep all our testing straight.
https://codereview.chromium.org/979893003/diff/80001/chrome/browser/net/certi... File chrome/browser/net/certificate_error_reporter_unittest.cc (right): https://codereview.chromium.org/979893003/diff/80001/chrome/browser/net/certi... chrome/browser/net/certificate_error_reporter_unittest.cc:19: const std::string kHostname = "test.mail.google.com"; On 2015/03/06 19:23:13, Ryan Sleevi wrote: > You should put all of these (including the test) in an unnamed namespace, so > that you don't run into any ODR issues If I put the test in an unnamed namespace, I can't FRIEND_TEST_ALL_PREFIXES it, right? https://codereview.chromium.org/979893003/diff/80001/chrome/browser/net/chrom... File chrome/browser/net/chrome_fraudulent_certificate_reporter.h (right): https://codereview.chromium.org/979893003/diff/80001/chrome/browser/net/chrom... chrome/browser/net/chrome_fraudulent_certificate_reporter.h:31: scoped_ptr<CertificateErrorReporter> certificate_reporter); On 2015/03/06 19:23:14, Ryan Sleevi wrote: > Then you can make this constructor private and friend test again. > Friending ChromeFraudulentCertificateReporterTest doesn't work because the constructor is actually called from the helper function DoMockReportIsSent (or at least I think that's why). It's not safe to construct a MockReporter (which requires a TestURLRequestContext) from ChromeFraudulentCertificateReporterTest not on the IO thread, right? https://codereview.chromium.org/979893003/diff/80001/chrome/browser/net/chrom... File chrome/browser/net/chrome_fraudulent_certificate_reporter_unittest.cc (right): https://codereview.chromium.org/979893003/diff/80001/chrome/browser/net/chrom... chrome/browser/net/chrome_fraudulent_certificate_reporter_unittest.cc:128: CertificateErrorReporter::SendReport(type, hostname, ssl_info); On 2015/03/06 19:23:14, Ryan Sleevi wrote: > You don't actually need to forward this on, do you? It's just a mock - it > doesn't actually implement the functionality (beyond the minimum necessary) To be honest I don't really understand what any of the tests in this file are supposed to be testing other than that the certificate reporting code doesn't crash, which is why I left it in (and that e.g. IsGooglePinnedProperty works, but this file seems like a lot of unnecessary code just to test that). I'm probably misunderstanding something, but for example, DoReportIsSent basically just seems to test that IsGoodSSLInfo(GetGoodSSLInfo()) == true, which doesn't really have anything to do with ChromeFraudulentCertificateReporter, and DoMockReportIsSent seems to only test that GetGoodSSLInfo().is_valid() == true and that if you pass a non-empty string to a method, the argument is non-empty.
https://codereview.chromium.org/979893003/diff/80001/chrome/browser/net/certi... File chrome/browser/net/certificate_error_reporter.cc (right): https://codereview.chromium.org/979893003/diff/80001/chrome/browser/net/certi... chrome/browser/net/certificate_error_reporter.cc:46: case REPORT_TYPE_EXTENDED_REPORTING: On 2015/03/06 19:23:13, Ryan Sleevi wrote: > Not something I'm worried about for this CL, since I know you're working on the > other bits, but normally when splitting this up, we'd leave out the > REPORT_TYPE_EXTENDED_REPORTING from the enum and implementation until your > secondary CL (which would then add and wire it up) > > Just more of a note for the future in terms of "where do I split the CL" Acknowledged. https://codereview.chromium.org/979893003/diff/80001/chrome/browser/net/certi... File chrome/browser/net/certificate_error_reporter.h (right): https://codereview.chromium.org/979893003/diff/80001/chrome/browser/net/certi... chrome/browser/net/certificate_error_reporter.h:31: explicit CertificateErrorReporter(net::URLRequestContext* request_context, On 2015/03/06 19:23:13, Ryan Sleevi wrote: > You don't need explicit here because it's a two-arg ctor. Done. https://codereview.chromium.org/979893003/diff/80001/chrome/browser/net/certi... File chrome/browser/net/certificate_error_reporter_unittest.cc (right): https://codereview.chromium.org/979893003/diff/80001/chrome/browser/net/certi... chrome/browser/net/certificate_error_reporter_unittest.cc:19: const std::string kHostname = "test.mail.google.com"; On 2015/03/06 19:52:06, emily wrote: > On 2015/03/06 19:23:13, Ryan Sleevi wrote: > > You should put all of these (including the test) in an unnamed namespace, so > > that you don't run into any ODR issues > > If I put the test in an unnamed namespace, I can't FRIEND_TEST_ALL_PREFIXES it, > right? Done, except for the test for above reason. https://codereview.chromium.org/979893003/diff/80001/chrome/browser/net/certi... chrome/browser/net/certificate_error_reporter_unittest.cc:21: static SSLInfo GetSSLInfo() { On 2015/03/06 19:23:13, Ryan Sleevi wrote: > /static/ is not needed here (and perversely, undesirable) once you move it into > an unnamed namespace Done. https://codereview.chromium.org/979893003/diff/80001/chrome/browser/net/chrom... File chrome/browser/net/chrome_fraudulent_certificate_reporter.h (right): https://codereview.chromium.org/979893003/diff/80001/chrome/browser/net/chrom... chrome/browser/net/chrome_fraudulent_certificate_reporter.h:23: public net::URLRequest::Delegate { On 2015/03/06 19:23:13, Ryan Sleevi wrote: > You no longer need to be a net::URLRequest::Delegate, since nothing ever passes > this to URLRequests Done. https://codereview.chromium.org/979893003/diff/80001/chrome/browser/net/chrom... chrome/browser/net/chrome_fraudulent_certificate_reporter.h:41: void OnReadCompleted(net::URLRequest* request, int bytes_read) override; On 2015/03/06 19:23:13, Ryan Sleevi wrote: > You can delete these two methods Done.
On 2015/03/06 19:32:31, Ryan Sleevi wrote: > D'oh! I missed that comment. > > +mmenke, who loves tests and will know where best to point you for testing that > a URLRequest is made. Which I should learn. Unfortunately, things are pretty complicated and ugly here. There are a bunch of different things done in different tests, depending on just what features the test needs and when the test was written. Just what do you want to do here? Some possible options: 1) Make sure a request is made, possibly checking the URL is as expected. 2) Make sure a request completes successfully, possibly checking the URL is as expected. 3) Use some URL with pre-determined response headers/body 4) Use some URL that simulates network errors. 5) Have one particular do 3) or 4) (i.e. instead of a standard test URL returning a particular result, have the test set what URL returns what result). Most tests currently inject their own URLRequestJobFactory / URLRequestInterceptor to do 1), 3), and 4), but that's a little ugly. It also can't do 2) unless you create your own URLRequestJob subclass as well. 1 and 2 alone can be done with just a NetworkDelegate, which can easily be added to the TestURLRequestContext you use for your unit test (Doing this in browser tests doesn't really work, since you don't create the request context). If the test can control the full URL, 3 and 4 and be done fairly easy with standard methods. If you want to do 5), things get a little more complicated, but still not too bad. If you want a particular URL to return a particular result that can change (Return an error and then succeed on retry, for example), you're back to injecting your own URLRequestJobFactory / URLRequestInterceptor. > https://codereview.chromium.org/979893003/diff/1/chrome/browser/net/chrome_ce... > File chrome/browser/net/chrome_certificate_reporter_unittest.cc (right): > > https://codereview.chromium.org/979893003/diff/1/chrome/browser/net/chrome_ce... > chrome/browser/net/chrome_certificate_reporter_unittest.cc:60: > On 2015/03/04 17:33:49, emily wrote: > > I want to add another test here that tests that the report actually gets sent. > > My plan was that I would create a TestURLRequestContext that overrides > > GetURLRequest to return a TestURLRequest that asserts that the upload data is > as > > expected, but that would require making URLRequestContext::GetURLRequest > virtual > > -- would that be okay? > > Testing the data is as expected is testing the implementation, not the > interface; I wouldn't recommend that. > > mmenke@ will know whatever the "Right Way" to test this interface would be; I > can never keep all our testing straight.
https://codereview.chromium.org/979893003/diff/80001/chrome/browser/net/certi... File chrome/browser/net/certificate_error_reporter_unittest.cc (right): https://codereview.chromium.org/979893003/diff/80001/chrome/browser/net/certi... chrome/browser/net/certificate_error_reporter_unittest.cc:19: const std::string kHostname = "test.mail.google.com"; On 2015/03/06 20:10:02, emily wrote: > Done, except for the test for above reason. Ah, right, because the test is friended you can't. https://codereview.chromium.org/979893003/diff/100001/chrome/browser/net/cert... File chrome/browser/net/certificate_error_reporter.cc (right): https://codereview.chromium.org/979893003/diff/100001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter.cc:117: } You've got inconsistent bracing here and the rest of the file (e.g. lines 119&120 Probably pick one and stick with it ;) https://codereview.chromium.org/979893003/diff/100001/chrome/browser/net/cert... File chrome/browser/net/certificate_error_reporter_unittest.cc (right): https://codereview.chromium.org/979893003/diff/100001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:19: const std::string kHostname = "test.mail.google.com"; s/std::string kHostname/const char kHostname[]/ (Don't have static data members) https://codereview.chromium.org/979893003/diff/100001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:26: info.pinning_failure_log = "dummy failure log"; Should this be a constant (line 26 vs 51) https://codereview.chromium.org/979893003/diff/100001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:48: EXPECT_EQ(request.hostname(), kHostname); You only use this constant once. Were you intending to use it in multiple tests? https://codereview.chromium.org/979893003/diff/100001/chrome/browser/net/chro... File chrome/browser/net/chrome_fraudulent_certificate_reporter.cc (right): https://codereview.chromium.org/979893003/diff/100001/chrome/browser/net/chro... chrome/browser/net/chrome_fraudulent_certificate_reporter.cc:23: GURL(kFraudulentCertificateUploadEndpoint))) { You should explicitly include GURL's header https://codereview.chromium.org/979893003/diff/100001/chrome/browser/net/chro... File chrome/browser/net/chrome_fraudulent_certificate_reporter.h (right): https://codereview.chromium.org/979893003/diff/100001/chrome/browser/net/chro... chrome/browser/net/chrome_fraudulent_certificate_reporter.h:11: #include "chrome/browser/net/certificate_error_reporter.h" You can forward declare this class https://codereview.chromium.org/979893003/diff/100001/chrome/browser/net/chro... chrome/browser/net/chrome_fraudulent_certificate_reporter.h:13: #include "net/url_request/url_request.h" You no longer need this include https://codereview.chromium.org/979893003/diff/100001/chrome/browser/net/chro... chrome/browser/net/chrome_fraudulent_certificate_reporter.h:29: scoped_ptr<CertificateErrorReporter> certificate_reporter); You need to include scoped_ptr's header
Thanks mmenke! Knowing about NetworkDelegate is really helpful. On 2015/03/06 20:26:13, mmenke wrote: > <snip> > If the test can control the full URL, 3 and 4 and be done fairly easy with > standard methods. > Could you please elaborate on standard methods or point me to an example if one comes to mind? (I'm new.)
https://codereview.chromium.org/979893003/diff/100001/chrome/browser/net/cert... File chrome/browser/net/certificate_error_reporter.cc (right): https://codereview.chromium.org/979893003/diff/100001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter.cc:117: } On 2015/03/06 21:32:47, Ryan Sleevi wrote: > You've got inconsistent bracing here and the rest of the file (e.g. lines > 119&120 > > Probably pick one and stick with it ;) grumble grumble I didn't write it but fiiiiine :P https://codereview.chromium.org/979893003/diff/100001/chrome/browser/net/cert... File chrome/browser/net/certificate_error_reporter_unittest.cc (right): https://codereview.chromium.org/979893003/diff/100001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:19: const std::string kHostname = "test.mail.google.com"; On 2015/03/06 21:32:47, Ryan Sleevi wrote: > s/std::string kHostname/const char kHostname[]/ > > (Don't have static data members) Done. https://codereview.chromium.org/979893003/diff/100001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:26: info.pinning_failure_log = "dummy failure log"; On 2015/03/06 21:32:47, Ryan Sleevi wrote: > Should this be a constant (line 26 vs 51) Done. https://codereview.chromium.org/979893003/diff/100001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:48: EXPECT_EQ(request.hostname(), kHostname); On 2015/03/06 21:32:47, Ryan Sleevi wrote: > You only use this constant once. Were you intending to use it in multiple tests? Yes, and I am now. https://codereview.chromium.org/979893003/diff/100001/chrome/browser/net/chro... File chrome/browser/net/chrome_fraudulent_certificate_reporter.cc (right): https://codereview.chromium.org/979893003/diff/100001/chrome/browser/net/chro... chrome/browser/net/chrome_fraudulent_certificate_reporter.cc:23: GURL(kFraudulentCertificateUploadEndpoint))) { On 2015/03/06 21:32:47, Ryan Sleevi wrote: > You should explicitly include GURL's header Done. https://codereview.chromium.org/979893003/diff/100001/chrome/browser/net/chro... File chrome/browser/net/chrome_fraudulent_certificate_reporter.h (right): https://codereview.chromium.org/979893003/diff/100001/chrome/browser/net/chro... chrome/browser/net/chrome_fraudulent_certificate_reporter.h:11: #include "chrome/browser/net/certificate_error_reporter.h" On 2015/03/06 21:32:47, Ryan Sleevi wrote: > You can forward declare this class I don't think I can, because of the scoped_ptr to it. https://codereview.chromium.org/979893003/diff/100001/chrome/browser/net/chro... chrome/browser/net/chrome_fraudulent_certificate_reporter.h:13: #include "net/url_request/url_request.h" On 2015/03/06 21:32:47, Ryan Sleevi wrote: > You no longer need this include Done. https://codereview.chromium.org/979893003/diff/100001/chrome/browser/net/chro... chrome/browser/net/chrome_fraudulent_certificate_reporter.h:29: scoped_ptr<CertificateErrorReporter> certificate_reporter); On 2015/03/06 21:32:47, Ryan Sleevi wrote: > You need to include scoped_ptr's header Done.
On 2015/03/07 00:34:49, emily wrote: > Thanks mmenke! Knowing about NetworkDelegate is really helpful. > > On 2015/03/06 20:26:13, mmenke wrote: > > <snip> > > If the test can control the full URL, 3 and 4 and be done fairly easy with > > standard methods. > > > > Could you please elaborate on standard methods or point me to an example if one > comes to mind? (I'm new.) Sorry, should have been clearer... I was hoping you could tell me a bit more about what specifically you need to do here. I just wanted to say there are a lot of ways to do things, depending on just what you need.
On 2015/03/07 01:46:03, mmenke wrote: > On 2015/03/07 00:34:49, emily wrote: > > Thanks mmenke! Knowing about NetworkDelegate is really helpful. > > > > On 2015/03/06 20:26:13, mmenke wrote: > > > <snip> > > > If the test can control the full URL, 3 and 4 and be done fairly easy with > > > standard methods. > > > > > > > Could you please elaborate on standard methods or point me to an example if > one > > comes to mind? (I'm new.) > > Sorry, should have been clearer... I was hoping you could tell me a bit more > about what specifically you need to do here. I just wanted to say there are a > lot of ways to do things, depending on just what you need. Oh, of course. So I have a class that exposes a method that sends a URLRequest to a Safe Browsing server. I'd like to test the following things (the test can control the full URL): 1. That a URLRequest gets created when the method gets called; I think I figured out how to test this with a NetworkDelegate 2. That pending requests get cleaned up when the object that created them gets deleted 3. That requests get cleaned up properly if the server returns an error, connection is refused, etc. Does that help?
On 2015/03/09 03:07:23, emily wrote:
> On 2015/03/07 01:46:03, mmenke wrote:
> > On 2015/03/07 00:34:49, emily wrote:
> > > Thanks mmenke! Knowing about NetworkDelegate is really helpful.
> > >
> > > On 2015/03/06 20:26:13, mmenke wrote:
> > > > <snip>
> > > > If the test can control the full URL, 3 and 4 and be done fairly easy
with
> > > > standard methods.
> > > >
> > >
> > > Could you please elaborate on standard methods or point me to an example
if
> > one
> > > comes to mind? (I'm new.)
> >
> > Sorry, should have been clearer... I was hoping you could tell me a bit
more
> > about what specifically you need to do here. I just wanted to say there are
a
> > lot of ways to do things, depending on just what you need.
>
> Oh, of course. So I have a class that exposes a method that sends a URLRequest
> to a Safe Browsing server. I'd like to test the following things (the test can
> control the full URL):
> 1. That a URLRequest gets created when the method gets called; I think I
figured
> out how to test this with a NetworkDelegate
> 2. That pending requests get cleaned up when the object that created them gets
> deleted
> 3. That requests get cleaned up properly if the server returns an error,
> connection is refused, etc.
> Does that help?
NetworkDelegate can also check that the request is destroyed (See
OnURLRequestDestroyed).
To simulate the the successful/failed responses, the simplest thing to do is
call:
net::URLRequestFailedJob::AddUrlHandler();
base::FilePath root_http;
PathService::Get(chrome::DIR_TEST_DATA, &root_http);
net::URLRequestMockHTTPJob::AddUrlHandlers(
root_http, BrowserThread::GetBlockingPool());
// This returns an HTTPS url that will fail with ERR_FAILED. You can use any
error you want.
// There's also an HTTP variant.
net::URLRequestFailedJob::GetMockHttpsUrl(net::ERR_FAILED);
// Use this if you want a URL to return a particular response.
// The file it responds with should be in chrome/tests/data/<some subdir>
// There's also an HTTP variant.
net::URLRequestMockHTTPJob::GetMockHttpsUrl(<relative path, starting with some
subdir above>);
And then to clean up, in the test's destructor:
net::URLRequestFilter::ClearHandlers();
The relevant mock classes live in net/test/url_request/ (URLRequestFilter is in
net/url_request/url_request_filter.h)
There's also URLRequestMockDataJob, which you may be able to use instead of the
mock HTTP jobs.
Don't hesitate to ask me if you need more detail, or run into any issues.
On 2015/03/09 14:32:31, mmenke wrote: > On 2015/03/09 03:07:23, emily wrote: > > On 2015/03/07 01:46:03, mmenke wrote: > > > On 2015/03/07 00:34:49, emily wrote: > > > > Thanks mmenke! Knowing about NetworkDelegate is really helpful. > > > > > > > > On 2015/03/06 20:26:13, mmenke wrote: > > > > > <snip> > > > > > If the test can control the full URL, 3 and 4 and be done fairly easy > with > > > > > standard methods. > > > > > > > > > > > > > Could you please elaborate on standard methods or point me to an example > if > > > one > > > > comes to mind? (I'm new.) > > > > > > Sorry, should have been clearer... I was hoping you could tell me a bit > more > > > about what specifically you need to do here. I just wanted to say there are > a > > > lot of ways to do things, depending on just what you need. > > > > Oh, of course. So I have a class that exposes a method that sends a URLRequest > > to a Safe Browsing server. I'd like to test the following things (the test can > > control the full URL): > > 1. That a URLRequest gets created when the method gets called; I think I > figured > > out how to test this with a NetworkDelegate > > 2. That pending requests get cleaned up when the object that created them gets > > deleted > > 3. That requests get cleaned up properly if the server returns an error, > > connection is refused, etc. > > Does that help? > > NetworkDelegate can also check that the request is destroyed (See > OnURLRequestDestroyed). > > To simulate the the successful/failed responses, the simplest thing to do is > call: > > net::URLRequestFailedJob::AddUrlHandler(); > base::FilePath root_http; > PathService::Get(chrome::DIR_TEST_DATA, &root_http); > net::URLRequestMockHTTPJob::AddUrlHandlers( > root_http, BrowserThread::GetBlockingPool()); > > // This returns an HTTPS url that will fail with ERR_FAILED. You can use any > error you want. > // There's also an HTTP variant. > net::URLRequestFailedJob::GetMockHttpsUrl(net::ERR_FAILED); > > // Use this if you want a URL to return a particular response. > // The file it responds with should be in chrome/tests/data/<some subdir> > // There's also an HTTP variant. > net::URLRequestMockHTTPJob::GetMockHttpsUrl(<relative path, starting with some > subdir above>); > > And then to clean up, in the test's destructor: > net::URLRequestFilter::ClearHandlers(); > > The relevant mock classes live in net/test/url_request/ (URLRequestFilter is in > net/url_request/url_request_filter.h) > > There's also URLRequestMockDataJob, which you may be able to use instead of the > mock HTTP jobs. > > Don't hesitate to ask me if you need more detail, or run into any issues. Thank you mmenke, this is really helpful! I think I was able to write the tests I wanted to write with these pointers. If you don't mind I just had one more question for you, about how to think about threading when writing tests like these. I tried to write these tests in 3 different ways: 1. By constructing a TestBrowserThread for IO and posting a task to it that created the requests. I was using https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/net... as an example. This seemed to result in the requests hanging; I never got |OnResponseStarted| callbacks for them and I couldn't figure out why. 2. By running the test directly in TEST_F(...) (not posting a task to a separate thread). This failed for understandable reasons; it hit a DCHECK in |SetUrlRequestMocksEnabled| checking that we're on an IO thread. 3. Then I added a |io_thread_| instance variable to the test class, using https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/sig... as an example. This seemed to magically make everything work, but I'm not sure what exactly it did or why. Do you have any words of wisdom to shed some light on what was going on in #1 or #3?
rsleevi, I added some more tests and addressed your latest round of comments, so I think this is ready for another look when you have a chance.
Matt, can you take a closer look at the tests? https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/cert... File chrome/browser/net/certificate_error_reporter.h (right): https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter.h:32: const GURL& upload_url); Missing include for "url/gurl.h" ( needed because line 70 has it as a member) https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter.h:41: const net::SSLInfo& ssl_info); Missing forward declaration of SSLInfo https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter.h:54: virtual scoped_ptr<net::URLRequest> CreateURLRequest( Missing include of scoped_ptr (As you fix all these, make sure to update the .cc to either not include what's in the .h or include what's forward declared) https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter.h:59: void SendCertLoggerRequest(CertLoggerRequest& request); STYLE: Not allowed to pass non-const references - http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Arg... https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter.h:64: CertLoggerRequest& out_request); STYLE: Not allowed to pass non-const references http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Arg... https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/cert... File chrome/browser/net/certificate_error_reporter_unittest.cc (right): https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:34: const char kTestCertFilename[] = "test_mail_google_com.pem"; const base::FilePath::CharType kTestCertFilename[] = FILE_PATH_LITERAL("test_mail_google.com.pem") https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:45: std::string GetPEMEncodedChain() { This seems to indirectly couple the test with the implementation of X509Certificate::GetPEMEncodedChain() That is, there is no strong API guarantee that the ingested format will be the same as the output format. What are you exactly trying to test here? That it produces a known-gold output? Or merely that the certificate(s) are present in the encoded response? https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:96: base::MessageLoopForUI message_loop_; This is usually a warning sign. I believe you probably want a TestBrowserThreadBundle https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:107: CertificateErrorReporter::OnResponseStarted(request); Do you actually need to call this for your unit tests to be correct? https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:113: run_loop_->Run(); If the response is never started, this will never quit, will it? Note: I generally dislike putting RunLoops in anything other than the test / it's fixture. It becomes extremely easy to accidentally nest RunLoops when putting them in dependent objects and, while 'supported', can end up hiding subtle bugs. For example, you could mitigate this by passing in a QuitClosure to be post when OnResponseStarted() is executed, such that it becomes TestReporter(net::URLRequestContext* context, const GURL& upload_url, const base::Closure& ...) { void OnResponseStarted(...) override { latest_request_url_ = request->url(); base::ThreadTaskRunnerHandle::Get()->PostTask(closure_); } } and move the base::RunLoop into the test themselves (or the wrapped in the test fixture somehow) https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:153: delete reporter; This seems to couple tightly with the RunLoop integration. That is, it assumes that SendReport() will never PostTask to ensure its cleanup of things. by foisting the base::RunLoop out, you could do something like RunUntilIdle() to make sure any cleanup tasks are appropriately executed before testing that cleanup happened. https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/chro... File chrome/browser/net/chrome_fraudulent_certificate_reporter.h (right): https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/chro... chrome/browser/net/chrome_fraudulent_certificate_reporter.h:12: #include "chrome/browser/net/certificate_error_reporter.h" You can forward declare this entire class. https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/chro... chrome/browser/net/chrome_fraudulent_certificate_reporter.h:31: ~ChromeFraudulentCertificateReporter() override {} Should not be inline; should be in the .cc file
On 2015/03/09 21:49:49, emily wrote: > On 2015/03/09 14:32:31, mmenke wrote: > > On 2015/03/09 03:07:23, emily wrote: > > > On 2015/03/07 01:46:03, mmenke wrote: > > > > On 2015/03/07 00:34:49, emily wrote: > > > > > Thanks mmenke! Knowing about NetworkDelegate is really helpful. > > > > > > > > > > On 2015/03/06 20:26:13, mmenke wrote: > > > > > > <snip> > > > > > > If the test can control the full URL, 3 and 4 and be done fairly easy > > with > > > > > > standard methods. > > > > > > > > > > > > > > > > Could you please elaborate on standard methods or point me to an example > > if > > > > one > > > > > comes to mind? (I'm new.) > > > > > > > > Sorry, should have been clearer... I was hoping you could tell me a bit > > more > > > > about what specifically you need to do here. I just wanted to say there > are > > a > > > > lot of ways to do things, depending on just what you need. > > > > > > Oh, of course. So I have a class that exposes a method that sends a > URLRequest > > > to a Safe Browsing server. I'd like to test the following things (the test > can > > > control the full URL): > > > 1. That a URLRequest gets created when the method gets called; I think I > > figured > > > out how to test this with a NetworkDelegate > > > 2. That pending requests get cleaned up when the object that created them > gets > > > deleted > > > 3. That requests get cleaned up properly if the server returns an error, > > > connection is refused, etc. > > > Does that help? > > > > NetworkDelegate can also check that the request is destroyed (See > > OnURLRequestDestroyed). > > > > To simulate the the successful/failed responses, the simplest thing to do is > > call: > > > > net::URLRequestFailedJob::AddUrlHandler(); > > base::FilePath root_http; > > PathService::Get(chrome::DIR_TEST_DATA, &root_http); > > net::URLRequestMockHTTPJob::AddUrlHandlers( > > root_http, BrowserThread::GetBlockingPool()); > > > > // This returns an HTTPS url that will fail with ERR_FAILED. You can use any > > error you want. > > // There's also an HTTP variant. > > net::URLRequestFailedJob::GetMockHttpsUrl(net::ERR_FAILED); > > > > // Use this if you want a URL to return a particular response. > > // The file it responds with should be in chrome/tests/data/<some subdir> > > // There's also an HTTP variant. > > net::URLRequestMockHTTPJob::GetMockHttpsUrl(<relative path, starting with some > > subdir above>); > > > > And then to clean up, in the test's destructor: > > net::URLRequestFilter::ClearHandlers(); > > > > The relevant mock classes live in net/test/url_request/ (URLRequestFilter is > in > > net/url_request/url_request_filter.h) > > > > There's also URLRequestMockDataJob, which you may be able to use instead of > the > > mock HTTP jobs. > > > > Don't hesitate to ask me if you need more detail, or run into any issues. > > Thank you mmenke, this is really helpful! I think I was able to write the tests > I wanted to write with these pointers. If you don't mind I just had one more > question for you, about how to think about threading when writing tests like > these. I tried to write these tests in 3 different ways: > 1. By constructing a TestBrowserThread for IO and posting a task to it that > created the requests. I was using > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/net... > as an example. This seemed to result in the requests hanging; I never got > |OnResponseStarted| callbacks for them and I couldn't figure out why. > 2. By running the test directly in TEST_F(...) (not posting a task to a separate > thread). This failed for understandable reasons; it hit a DCHECK in > |SetUrlRequestMocksEnabled| checking that we're on an IO thread. > 3. Then I added a |io_thread_| instance variable to the test class, using > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/sig... > as an example. This seemed to magically make everything work, but I'm not sure > what exactly it did or why. > > Do you have any words of wisdom to shed some light on what was going on in #1 or > #3? The IO thread thing was why I didn't suggest you use SetUrlRequestMocksEnabled to set up tests. In #1 were you using RunUntilIdle? If so, that's the problem. If not, I'd have to see the code (Which I'm happy to take a look at - getting a better understanding MessageLoops is certainly not a waste of time).
On 2015/03/09 22:14:07, Ryan Sleevi wrote: > Matt, can you take a closer look at the tests? Will do, tomorrow.
https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/cert... File chrome/browser/net/certificate_error_reporter.h (right): https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter.h:32: const GURL& upload_url); On 2015/03/09 22:14:06, Ryan Sleevi wrote: > Missing include for "url/gurl.h" ( needed because line 70 has it as a member) Done. Is there a tool that you use to catch these or just by looking at it? https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter.h:41: const net::SSLInfo& ssl_info); On 2015/03/09 22:14:06, Ryan Sleevi wrote: > Missing forward declaration of SSLInfo Done. https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter.h:54: virtual scoped_ptr<net::URLRequest> CreateURLRequest( On 2015/03/09 22:14:06, Ryan Sleevi wrote: > Missing include of scoped_ptr > > (As you fix all these, make sure to update the .cc to either not include what's > in the .h or include what's forward declared) Done. https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter.h:59: void SendCertLoggerRequest(CertLoggerRequest& request); On 2015/03/09 22:14:06, Ryan Sleevi wrote: > STYLE: Not allowed to pass non-const references - > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Arg... Done. https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter.h:64: CertLoggerRequest& out_request); On 2015/03/09 22:14:06, Ryan Sleevi wrote: > STYLE: Not allowed to pass non-const references > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Arg... Done. https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/cert... File chrome/browser/net/certificate_error_reporter_unittest.cc (right): https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:34: const char kTestCertFilename[] = "test_mail_google_com.pem"; On 2015/03/09 22:14:06, Ryan Sleevi wrote: > const base::FilePath::CharType kTestCertFilename[] = > FILE_PATH_LITERAL("test_mail_google.com.pem") Done. https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:45: std::string GetPEMEncodedChain() { On 2015/03/09 22:14:07, Ryan Sleevi wrote: > This seems to indirectly couple the test with the implementation of > X509Certificate::GetPEMEncodedChain() > > That is, there is no strong API guarantee that the ingested format will be the > same as the output format. > > What are you exactly trying to test here? That it produces a known-gold output? > Or merely that the certificate(s) are present in the encoded response? Well, I'd like to test that the output is in the format that the server expects it. So that is more like known-gold I suppose. Does that mean I should paste in the exact data I expect here? https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:96: base::MessageLoopForUI message_loop_; On 2015/03/09 22:14:07, Ryan Sleevi wrote: > This is usually a warning sign. I believe you probably want a > TestBrowserThreadBundle Done. https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:107: CertificateErrorReporter::OnResponseStarted(request); On 2015/03/09 22:14:06, Ryan Sleevi wrote: > Do you actually need to call this for your unit tests to be correct? I think so, at least for some of them. |CertificateErrorReporter::OnResponseStarted| calls |RequestComplete| which is what cleans up the request, which is what one of the tests checks for. https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:113: run_loop_->Run(); On 2015/03/09 22:14:07, Ryan Sleevi wrote: > If the response is never started, this will never quit, will it? Is there any way to avoid that? It seems like a fundamental problem of testing async code. > > Note: I generally dislike putting RunLoops in anything other than the test / > it's fixture. It becomes extremely easy to accidentally nest RunLoops when > putting them in dependent objects and, while 'supported', can end up hiding > subtle bugs. > > For example, you could mitigate this by passing in a QuitClosure to be post when > OnResponseStarted() is executed, such that it becomes > > TestReporter(net::URLRequestContext* context, const GURL& upload_url, const > base::Closure& ...) { > > void OnResponseStarted(...) override { > latest_request_url_ = request->url(); > base::ThreadTaskRunnerHandle::Get()->PostTask(closure_); > } > } > > and move the base::RunLoop into the test themselves (or the wrapped in the test > fixture somehow) Done. https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:153: delete reporter; On 2015/03/09 22:14:06, Ryan Sleevi wrote: > This seems to couple tightly with the RunLoop integration. That is, it assumes > that SendReport() will never PostTask to ensure its cleanup of things. > > by foisting the base::RunLoop out, you could do something like RunUntilIdle() to > make sure any cleanup tasks are appropriately executed before testing that > cleanup happened. I need some clarification on this one. IIUC RunUntilIdle() will ensure that there are no cleanup tasks pending on the current thread, but the reporter could post a task to a different thread to do its cleanup, right? (Or post a task to a different thread which posts one back to the original thread that does the cleanup, or something like that.) Would RunUntilIdle() be sufficient in that case? Perhaps I should add a |cleanup_callback_for_testing_| to CertificateErrorReporter? Moreover this seems to apply to the next test as well (ErroredRequestGetsDeleted), i.e. who's to say that SendReport doesn't wait 10 minutes before cleaning up completed requests. https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/chro... File chrome/browser/net/chrome_fraudulent_certificate_reporter.h (right): https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/chro... chrome/browser/net/chrome_fraudulent_certificate_reporter.h:12: #include "chrome/browser/net/certificate_error_reporter.h" On 2015/03/09 22:14:07, Ryan Sleevi wrote: > You can forward declare this entire class. That doesn't seem to be possible with a scoped_ptr to it. https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/chro... chrome/browser/net/chrome_fraudulent_certificate_reporter.h:31: ~ChromeFraudulentCertificateReporter() override {} On 2015/03/09 22:14:07, Ryan Sleevi wrote: > Should not be inline; should be in the .cc file Done.
https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/cert... File chrome/browser/net/certificate_error_reporter_unittest.cc (right): https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:45: std::string GetPEMEncodedChain() { On 2015/03/09 23:21:17, emily wrote: > Well, I'd like to test that the output is in the format that the server expects > it. So that is more like known-gold I suppose. Does that mean I should paste in > the exact data I expect here? I think it's fine; in that case, however, you should make sure the documentation for test_mail_google_com.pem specifies that the format is intentional (usually in the README file), since normally we provide diagnostic output in the .pem files to make codesearch easier. https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:107: CertificateErrorReporter::OnResponseStarted(request); On 2015/03/09 23:21:17, emily wrote: > I think so, at least for some of them. > |CertificateErrorReporter::OnResponseStarted| calls |RequestComplete| which is > what cleans up the request, which is what one of the tests checks for. So from a design perspective then, it's usually seen as too much coupling when you have to call a base class's behaviour from a derived class. That is, if you have behaviour that is common for a base class, but then derived classes may override just a subset of that, then another strategy is to have protected-virtual methods that derived classes can override, but are called from the base class's concrete implementation. An example (albeit perhaps unnecessary complex) is to consider CertVerifyProc::Verify and CertVerifyProc::VerifyInternal. Verify() encompasses the common functionality of the base class, while VerifyInternal is left for derived classes to implement the necessary concrete functionality. Any time you see code that directly calls a method on its parent class, it's usually a sign that the base class is exposing the wrong interface / the derived class is doing something in an error prone way. (A more complex scenario is to do something like URLRequest, which allows Delegate()s to implement the concrete derived behaviour, but then have all the base behaviour itself. When it wants to allow a "derived" thing to override behaviour, it calls into to the Delegate. However, I'm not sure that's necessary here) For testing code, I'm not sure if the readability hit of "doing it right" is worth the cost, and so I think I'm OK with this code as is, with the caveat of everything I mentioned above. However, Matt may feel differently / have more to add here. https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:113: run_loop_->Run(); On 2015/03/09 23:21:17, emily wrote: > On 2015/03/09 22:14:07, Ryan Sleevi wrote: > > If the response is never started, this will never quit, will it? > > Is there any way to avoid that? It seems like a fundamental problem of testing > async code. RunUntilIdle() is usually a good way to avoid this problem. For other tests, sometimes it helps to setup a Timer to force-quit. Usually I just make sure people think about it and document the expectations and hope someone yells if/when this test starts flaking (by hanging) https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:153: delete reporter; On 2015/03/09 23:21:17, emily wrote: > On 2015/03/09 22:14:06, Ryan Sleevi wrote: > > This seems to couple tightly with the RunLoop integration. That is, it assumes > > that SendReport() will never PostTask to ensure its cleanup of things. > > > > by foisting the base::RunLoop out, you could do something like RunUntilIdle() > to > > make sure any cleanup tasks are appropriately executed before testing that > > cleanup happened. > > I need some clarification on this one. IIUC RunUntilIdle() will ensure that > there are no cleanup tasks pending on the current thread, but the reporter could > post a task to a different thread to do its cleanup, right? (Or post a task to a > different thread which posts one back to the original thread that does the > cleanup, or something like that.) Would RunUntilIdle() be sufficient in that > case? > > Perhaps I should add a |cleanup_callback_for_testing_| to > CertificateErrorReporter? Well, if you're using a TestBrowserThreadBundle, it has the nice (intentional) side-effect of running all the threads on the same message loop, thus RunUntilIdle covers all of these interactions quite nicely. It's why using TBTB is nice :) [ISTR it also has helpers to wait until child threads are pumped, if you do need to force distinct/native threads] > > Moreover this seems to apply to the next test as well > (ErroredRequestGetsDeleted), i.e. who's to say that SendReport doesn't wait 10 > minutes before cleaning up completed requests. Yeah, but I'm willing to live with that, because I'm a hypocrite and only yell about the tightest of tight coupling and leave the simpler/more annoying stuff as a "Meh" ;)
https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/cert... File chrome/browser/net/certificate_error_reporter.h (right): https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter.h:32: const GURL& upload_url); On 2015/03/09 23:21:17, emily wrote: > Done. Is there a tool that you use to catch these or just by looking at it? Just by looking. There used to be Include What You Use (IWYU), but that's largely orphaned / doesn't work well on the Chrome codebase. https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/chro... File chrome/browser/net/chrome_fraudulent_certificate_reporter.h (right): https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/chro... chrome/browser/net/chrome_fraudulent_certificate_reporter.h:12: #include "chrome/browser/net/certificate_error_reporter.h" On 2015/03/09 23:21:17, emily wrote: > On 2015/03/09 22:14:07, Ryan Sleevi wrote: > > You can forward declare this entire class. > > That doesn't seem to be possible with a scoped_ptr to it. It does when you move the dtor to the .cc (since the dtor is invoking the scoped_ptr<T> dtor, which requires T be concrete; by moving the dtor from the .h to the .cc, you can forward declare here in the .h) https://codereview.chromium.org/979893003/diff/200001/chrome/browser/net/cert... File chrome/browser/net/certificate_error_reporter_unittest.cc (right): https://codereview.chromium.org/979893003/diff/200001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:114: content::TestBrowserThreadBundle thread_bundle_; Should make sure this is the first member, since either lines 111 or 112 may attempt to use BrowserThread and would fail.
https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/cert... File chrome/browser/net/certificate_error_reporter_unittest.cc (right): https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:45: std::string GetPEMEncodedChain() { On 2015/03/09 23:40:36, Ryan Sleevi wrote: > On 2015/03/09 23:21:17, emily wrote: > > Well, I'd like to test that the output is in the format that the server > expects > > it. So that is more like known-gold I suppose. Does that mean I should paste > in > > the exact data I expect here? > > I think it's fine; in that case, however, you should make sure the documentation > for test_mail_google_com.pem specifies that the format is intentional (usually > in the README file), since normally we provide diagnostic output in the .pem > files to make codesearch easier. Done. https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:113: run_loop_->Run(); On 2015/03/09 23:40:36, Ryan Sleevi wrote: > On 2015/03/09 23:21:17, emily wrote: > > On 2015/03/09 22:14:07, Ryan Sleevi wrote: > > > If the response is never started, this will never quit, will it? > > > > Is there any way to avoid that? It seems like a fundamental problem of testing > > async code. > > RunUntilIdle() is usually a good way to avoid this problem. > > For other tests, sometimes it helps to setup a Timer to force-quit. Usually I > just make sure people think about it and document the expectations and hope > someone yells if/when this test starts flaking (by hanging) So are you saying that all the `run_loop.Run`s should be `run_loop.RunUntilIdle`s? That doesn't seem to work for SendReportSendsRequest; when I change it to run_loop.RunUntilIdle() the test fails because the EXPECTs run before |OnResponseStarted| runs. According to my understanding from what you said below about TestBrowserThreadBundle, I would have thought that RunUntilIdle would work... https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:153: delete reporter; On 2015/03/09 23:40:36, Ryan Sleevi wrote: > On 2015/03/09 23:21:17, emily wrote: > > On 2015/03/09 22:14:06, Ryan Sleevi wrote: > > > This seems to couple tightly with the RunLoop integration. That is, it > assumes > > > that SendReport() will never PostTask to ensure its cleanup of things. > > > > > > by foisting the base::RunLoop out, you could do something like > RunUntilIdle() > > to > > > make sure any cleanup tasks are appropriately executed before testing that > > > cleanup happened. > > > > I need some clarification on this one. IIUC RunUntilIdle() will ensure that > > there are no cleanup tasks pending on the current thread, but the reporter > could > > post a task to a different thread to do its cleanup, right? (Or post a task to > a > > different thread which posts one back to the original thread that does the > > cleanup, or something like that.) Would RunUntilIdle() be sufficient in that > > case? > > > > Perhaps I should add a |cleanup_callback_for_testing_| to > > CertificateErrorReporter? > > Well, if you're using a TestBrowserThreadBundle, it has the nice (intentional) > side-effect of running all the threads on the same message loop, thus > RunUntilIdle covers all of these interactions quite nicely. It's why using TBTB > is nice :) [ISTR it also has helpers to wait until child threads are pumped, if > you do need to force distinct/native threads] > Ah, I see. Done. https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/chro... File chrome/browser/net/chrome_fraudulent_certificate_reporter.h (right): https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/chro... chrome/browser/net/chrome_fraudulent_certificate_reporter.h:12: #include "chrome/browser/net/certificate_error_reporter.h" On 2015/03/09 23:44:56, Ryan Sleevi wrote: > On 2015/03/09 23:21:17, emily wrote: > > On 2015/03/09 22:14:07, Ryan Sleevi wrote: > > > You can forward declare this entire class. > > > > That doesn't seem to be possible with a scoped_ptr to it. > > It does when you move the dtor to the .cc (since the dtor is invoking the > scoped_ptr<T> dtor, which requires T be concrete; by moving the dtor from the .h > to the .cc, you can forward declare here in the .h) Done. https://codereview.chromium.org/979893003/diff/200001/chrome/browser/net/cert... File chrome/browser/net/certificate_error_reporter_unittest.cc (right): https://codereview.chromium.org/979893003/diff/200001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:114: content::TestBrowserThreadBundle thread_bundle_; On 2015/03/09 23:44:56, Ryan Sleevi wrote: > Should make sure this is the first member, since either lines 111 or 112 may > attempt to use BrowserThread and would fail. Done.
One comment, before I review the tests. https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/cert... File chrome/browser/net/certificate_error_reporter_unittest.cc (right): https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:113: run_loop_->Run(); On 2015/03/10 00:07:38, emily wrote: > On 2015/03/09 23:40:36, Ryan Sleevi wrote: > > On 2015/03/09 23:21:17, emily wrote: > > > On 2015/03/09 22:14:07, Ryan Sleevi wrote: > > > > If the response is never started, this will never quit, will it? > > > > > > Is there any way to avoid that? It seems like a fundamental problem of > testing > > > async code. > > > > RunUntilIdle() is usually a good way to avoid this problem. > > > > For other tests, sometimes it helps to setup a Timer to force-quit. Usually I > > just make sure people think about it and document the expectations and hope > > someone yells if/when this test starts flaking (by hanging) > > So are you saying that all the `run_loop.Run`s should be > `run_loop.RunUntilIdle`s? That doesn't seem to work for SendReportSendsRequest; > when I change it to run_loop.RunUntilIdle() the test fails because the EXPECTs > run before |OnResponseStarted| runs. According to my understanding from what you > said below about TestBrowserThreadBundle, I would have thought that RunUntilIdle > would work... I think RunUntilIdle() is a bit of an anti-pattern. You're depending on the underlying code to do all its work on the current thread, posting tasks without delay, which is not actually a part of the documented URLRequest interface, and is not a property of all URLRequest types. Changes in net/ can break this assumption, and would require updating all these tests. I've run into this problem in the past, and would prefer to minimize the chance of it happening again. It's much more robust to just wait until something happens. This does result in tests timing out on failure, but I think that's better than tests breaking when net/ code changes, particularly in ways that don't actually require API doc updates. Also worth noting that if too many tests timeout (And maybe fail as well, not sure), buildbots will abort a test run, so 1000 broken tests all taking 30 seconds to timeout won't break the entire testing infrastructure.
https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... File chrome/browser/net/certificate_error_reporter.cc (right): https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter.cc:95: net::ElementsUploadDataStream::CreateWithReader(reader.Pass(), 0)); The tests should check the contents of this, and the method used by the request. https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... File chrome/browser/net/certificate_error_reporter_unittest.cc (right): https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:74: TestNetworkDelegate() : NetworkDelegateImpl() { 0-argument parent class constructor does not need to be explicitly invoked. https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:76: destroyed_requests_ = 0; These should be in the initializer list. https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:78: ~TestNetworkDelegate() {} override https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:87: void OnURLRequestDestroyed(URLRequest* request) { destroyed_requests_++; } override https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:90: int GetNumDestroyedRequests() { return destroyed_requests_; } these should both be const, and have the same name of the variable they return. https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:94: int destroyed_requests_; These should match the number of the getters. Add num_ prefixes to both? https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:99: ~CertificateErrorReporterTest() { override https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:105: CertificateErrorReporterTest() : context_(true) { Optional: Suggest just making this public, before the destructor, to make this easier to read. https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:111: content::TestBrowserThreadBundle thread_bundle_; The Google style guide requires all class variables to be private. If you need to access these, can just add accessor methods. https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:116: class TestReporter : public CertificateErrorReporter { Need a virtual destructor in child classes. https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:119: : CertificateErrorReporter(context, upload_url), latest_request_url_() { Remove latest_request_url_() - the no argument constructor will be called by default, no need to explicitly include it. https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:120: response_started_callback_ = base::Bind(&base::DoNothing); This should be in an initializer list. https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:127: latest_request_url_(), Remove latest_request_url_() https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:134: response_started_callback_); Can invoke this synchronously with just response_started_callback_.Run(). Should also check if it's empty() (is_empty()? is_null()? Can't remember what it's called) before invoking the callback. https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:137: GURL& GetLatestRequestURL() { return latest_request_url_; } This should be: const GURL& latest_request_url() const { (+2 consts, change naming convention). https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:148: base::Closure callback = run_loop.QuitClosure(); optional nit: No need to store this on the stack. Can just inline run_loop.QuitClosure() in the TestReporter constructor call. Same goes for ErroredRequestGetsDeleted. https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:177: delete reporter; Rather than calling delete, I suggest putting it in a scoped_ptr<TestReporter) on creation, and then resetting the scoped_ptr. It's not a critical issue here, but it results in less error prone code in general. https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:179: run_loop.RunUntilIdle(); Is this needed? https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:202: Since we're using a std::set for requests, we should have a test that sends multiple requests, ideally one test that does it sequentially and one that does it simultaneously. https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:205: namespace chrome_browser_net { Having a test in a separate namespace is weird. Should just use using directives, or write it out when needed.
Questions for mmenke and rsleevi inline. I'm at an offsite today but will make the actual changes ASAP. https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... File chrome/browser/net/certificate_error_reporter.cc (right): https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter.cc:95: net::ElementsUploadDataStream::CreateWithReader(reader.Pass(), 0)); On 2015/03/10 15:08:45, mmenke wrote: > The tests should check the contents of this, and the method used by the request. rsleevi advised against this on the grounds that it would be testing the implementation, not the interface. OTOH, I'm no longer sure I see the distinction; isn't SendReport using a URLRequest also part of the implementation, not the interface? And checking the upload data could be framed as testing the interface between the browser and the server. rsleevi, thoughts? https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... File chrome/browser/net/certificate_error_reporter_unittest.cc (right): https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:179: run_loop.RunUntilIdle(); On 2015/03/10 15:08:46, mmenke wrote: > Is this needed? rsleevi advised to add it to avoid coupling with the reporter implementation, e.g. the reporter destructor could theoretically post a task to do the cleanup. https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:205: namespace chrome_browser_net { On 2015/03/10 15:08:46, mmenke wrote: > Having a test in a separate namespace is weird. Should just use using > directives, or write it out when needed. Sorry, don't follow. This test is in the chrome_browser_net namespace so that CertificateErrorReporter can friend it. Is there an alternative way to do that?
On 2015/03/10 14:25:46, mmenke wrote: > One comment, before I review the tests. > > https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/cert... > File chrome/browser/net/certificate_error_reporter_unittest.cc (right): > > https://codereview.chromium.org/979893003/diff/160001/chrome/browser/net/cert... > chrome/browser/net/certificate_error_reporter_unittest.cc:113: run_loop_->Run(); > On 2015/03/10 00:07:38, emily wrote: > > On 2015/03/09 23:40:36, Ryan Sleevi wrote: > > > On 2015/03/09 23:21:17, emily wrote: > > > > On 2015/03/09 22:14:07, Ryan Sleevi wrote: > > > > > If the response is never started, this will never quit, will it? > > > > > > > > Is there any way to avoid that? It seems like a fundamental problem of > > testing > > > > async code. > > > > > > RunUntilIdle() is usually a good way to avoid this problem. > > > > > > For other tests, sometimes it helps to setup a Timer to force-quit. Usually > I > > > just make sure people think about it and document the expectations and hope > > > someone yells if/when this test starts flaking (by hanging) > > > > So are you saying that all the `run_loop.Run`s should be > > `run_loop.RunUntilIdle`s? That doesn't seem to work for > SendReportSendsRequest; > > when I change it to run_loop.RunUntilIdle() the test fails because the EXPECTs > > run before |OnResponseStarted| runs. According to my understanding from what > you > > said below about TestBrowserThreadBundle, I would have thought that > RunUntilIdle > > would work... > > I think RunUntilIdle() is a bit of an anti-pattern. You're depending on the > underlying code to do all its work on the current thread, posting tasks without > delay, which is not actually a part of the documented URLRequest interface, and > is not a property of all URLRequest types. Changes in net/ can break this > assumption, and would require updating all these tests. I've run into this > problem in the past, and would prefer to minimize the chance of it happening > again. > > It's much more robust to just wait until something happens. This does result in > tests timing out on failure, but I think that's better than tests breaking when > net/ code changes, particularly in ways that don't actually require API doc > updates. Also worth noting that if too many tests timeout (And maybe fail as > well, not sure), buildbots will abort a test run, so 1000 broken tests all > taking 30 seconds to timeout won't break the entire testing infrastructure. So I should add callbacks to the CertificateErrorReporter class to trigger at the points that I'm testing for? e.g. CertificateErrorReporter should have report_sent_callback_for_testing_ and requests_cleaned_up_callback_for_testing_, etc.?
Quick responses to your points (All quite reasonable). https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... File chrome/browser/net/certificate_error_reporter.cc (right): https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter.cc:95: net::ElementsUploadDataStream::CreateWithReader(reader.Pass(), 0)); On 2015/03/10 15:54:15, emily wrote: > On 2015/03/10 15:08:45, mmenke wrote: > > The tests should check the contents of this, and the method used by the > request. > > rsleevi advised against this on the grounds that it would be testing the > implementation, not the interface. > > OTOH, I'm no longer sure I see the distinction; isn't SendReport using a > URLRequest also part of the implementation, not the interface? And checking the > upload data could be framed as testing the interface between the browser and the > server. > > rsleevi, thoughts? We need to be sure we are sending sane data. Without any test of that, we could have regressions here that would never be caught by a test. https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... File chrome/browser/net/certificate_error_reporter_unittest.cc (right): https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:179: run_loop.RunUntilIdle(); On 2015/03/10 15:54:15, emily wrote: > On 2015/03/10 15:08:46, mmenke wrote: > > Is this needed? > > rsleevi advised to add it to avoid coupling with the reporter implementation, > e.g. the reporter destructor could theoretically post a task to do the cleanup. Of course, it could *also* post a delayed task to do that. :) If we're really concerned about that, we should make the NetworkDelegate have the ability to wait until a request is destroyed (Note that you could actually put all the waiting logic in the NetworkDelegate, and then just wait for destruction on all tests. Would make this test look more like the others, too). Anyhow, just a suggestion. https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:205: namespace chrome_browser_net { On 2015/03/10 15:54:15, emily wrote: > On 2015/03/10 15:08:46, mmenke wrote: > > Having a test in a separate namespace is weird. Should just use using > > directives, or write it out when needed. > > Sorry, don't follow. This test is in the chrome_browser_net namespace so that > CertificateErrorReporter can friend it. Is there an alternative way to do that? Sorry, didn't notice that. I'm not a fan of friending tests. Could make the method protected, and then have the test subclass expose it. I think it's simpler than having an exhaustive list of tests in the public header, and it also makes it clearer what internal state is poked at by tests, but Ryan may disagree, and I'll defer to him here. However...It's also worth noting this is most definitely not part of the public interface. I think it would make more sense to check the upload data this results in us sending, rather than that private fields are populated as expected.
Quick responses to Matt's quick responses https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... File chrome/browser/net/certificate_error_reporter_unittest.cc (right): https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:111: content::TestBrowserThreadBundle thread_bundle_; On 2015/03/10 15:08:46, mmenke wrote: > The Google style guide requires all class variables to be private. If you need > to access these, can just add accessor methods. Matt, test fixtures are explicitly allowed to be Protected - http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Access_Control https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:134: response_started_callback_); On 2015/03/10 15:08:46, mmenke wrote: > Can invoke this synchronously with just response_started_callback_.Run(). > Should also check if it's empty() (is_empty()? is_null()? Can't remember what > it's called) before invoking the callback. I'm not sure I'd agree on the is_empty. By not checking, you make the API contract clearer than it must do something (and even base::DoNothing is doing something) I suggested PostTask as more idiomatic / reflective of how URLRequest works (in that, in my experience, it tends to PostTask work to clear the recursion stack). I defer to Matt if he disagrees. https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:179: run_loop.RunUntilIdle(); On 2015/03/10 16:08:52, mmenke wrote: > Of course, it could *also* post a delayed task to do that. :) If we're really > concerned about that, we should make the NetworkDelegate have the ability to > wait until a request is destroyed (Note that you could actually put all the > waiting logic in the NetworkDelegate, and then just wait for destruction on all > tests. Would make this test look more like the others, too). Anyhow, just a > suggestion. Too Many Idioms (to the tune of Too Many Cooks) I suspect that adding the waiting logic to the NetworkDelegate is a non-trivial task, and I'm not sufficiently familiar with the NetworkDelegate to know whether the added behaviour would be adding complexity down the road - that I defer to Matt whether it's sufficiently "worth it" for this use case. Given Matt's concerns, I'm OK with removing this pump - if/when this test fails, we'll revisit it. Hopefully someone will keep an eye on it from being flaky, since we no longer have tools that automatically inform us of this (e.g. by breaking the tree) https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:205: namespace chrome_browser_net { On 2015/03/10 16:08:52, mmenke wrote: > Sorry, didn't notice that. I'm not a fan of friending tests. Could make the > method protected, and then have the test subclass expose it. I think it's > simpler than having an exhaustive list of tests in the public header, and it > also makes it clearer what internal state is poked at by tests, but Ryan may > disagree, and I'll defer to him here. Sorry for the mixed messaging :) I do tend to disagree; I've seen too much subclassing when made protected, so strongly prefer friending tests (or test fixtures, which is the other way to avoid having to exhaustively enumerate all tests.) I usually use 3 as the rule - if I'm having to friend 3 tests, I should really be friending the fixture itself and then let the fixture marshal that method for the concrete tests. > > However...It's also worth noting this is most definitely not part of the public > interface. I think it would make more sense to check the upload data this > results in us sending, rather than that private fields are populated as > expected. That seems quite reasonable, and decouples further.
Quick responses to Ryan's quick responses to my quick responses. https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... File chrome/browser/net/certificate_error_reporter_unittest.cc (right): https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:111: content::TestBrowserThreadBundle thread_bundle_; On 2015/03/10 17:39:30, Ryan Sleevi wrote: > On 2015/03/10 15:08:46, mmenke wrote: > > The Google style guide requires all class variables to be private. If you > need > > to access these, can just add accessor methods. > > Matt, test fixtures are explicitly allowed to be Protected - > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Access_Control Hrm...Did the old version say that? I admittedly haven't read through it since it was updated a couple years back. https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:134: response_started_callback_); On 2015/03/10 17:39:30, Ryan Sleevi wrote: > On 2015/03/10 15:08:46, mmenke wrote: > > Can invoke this synchronously with just response_started_callback_.Run(). > > Should also check if it's empty() (is_empty()? is_null()? Can't remember > what > > it's called) before invoking the callback. > > I'm not sure I'd agree on the is_empty. By not checking, you make the API > contract clearer than it must do something (and even base::DoNothing is doing > something) You're right - I hadn't realized that the other constructor set a callback that does nothing (Was actually wondering why it didn't crash in that case). I've never seen base::DoNothing used before, but I'm fine with it. > I suggested PostTask as more idiomatic / reflective of how URLRequest works (in > that, in my experience, it tends to PostTask work to clear the recursion stack). > I defer to Matt if he disagrees. My thinking is that just seems kinda weird to me to do post a task to the thread the TestReporter is called on, rather than taking one as input (Admittedly, it would be the same thread), anyhow, I don't have a strong opinion here, just seemed a little unusual. https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:179: run_loop.RunUntilIdle(); On 2015/03/10 17:39:30, Ryan Sleevi wrote: > On 2015/03/10 16:08:52, mmenke wrote: > > Of course, it could *also* post a delayed task to do that. :) If we're > really > > concerned about that, we should make the NetworkDelegate have the ability to > > wait until a request is destroyed (Note that you could actually put all the > > waiting logic in the NetworkDelegate, and then just wait for destruction on > all > > tests. Would make this test look more like the others, too). Anyhow, just a > > suggestion. > > Too Many Idioms (to the tune of Too Many Cooks) > > I suspect that adding the waiting logic to the NetworkDelegate is a non-trivial > task, and I'm not sufficiently familiar with the NetworkDelegate to know whether > the added behaviour would be adding complexity down the road - that I defer to > Matt whether it's sufficiently "worth it" for this use case. > > Given Matt's concerns, I'm OK with removing this pump - if/when this test fails, > we'll revisit it. Hopefully someone will keep an eye on it from being flaky, > since we no longer have tools that automatically inform us of this (e.g. by > breaking the tree) I'm not suggesting adding anything to the main NetworkDelegate class, just to TestNetworkDelegate defined in this file. Could just moving the callback we're currently passing to TestReporter to TestNetworkDelegate, and call it on destruction. That would also solve the weirdness of OnResponseStarted, without having to add a virtual method in production code that does nothing, just so the test fixture can override it. I like to keep production code as clean as we reasonably can when it comes to extra stuff to help with testing.
https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... File chrome/browser/net/certificate_error_reporter_unittest.cc (right): https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:74: TestNetworkDelegate() : NetworkDelegateImpl() { On 2015/03/10 15:08:46, mmenke wrote: > 0-argument parent class constructor does not need to be explicitly invoked. Done. https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:76: destroyed_requests_ = 0; On 2015/03/10 15:08:46, mmenke wrote: > These should be in the initializer list. Done. https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:78: ~TestNetworkDelegate() {} On 2015/03/10 15:08:45, mmenke wrote: > override Done. https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:87: void OnURLRequestDestroyed(URLRequest* request) { destroyed_requests_++; } On 2015/03/10 15:08:46, mmenke wrote: > override Done. https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:90: int GetNumDestroyedRequests() { return destroyed_requests_; } On 2015/03/10 15:08:46, mmenke wrote: > these should both be const, and have the same name of the variable they return. Done. https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:94: int destroyed_requests_; On 2015/03/10 15:08:46, mmenke wrote: > These should match the number of the getters. Add num_ prefixes to both? Done. https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:99: ~CertificateErrorReporterTest() { On 2015/03/10 15:08:45, mmenke wrote: > override Done. https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:105: CertificateErrorReporterTest() : context_(true) { On 2015/03/10 15:08:46, mmenke wrote: > Optional: Suggest just making this public, before the destructor, to make this > easier to read. Done. https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:111: content::TestBrowserThreadBundle thread_bundle_; On 2015/03/10 15:08:46, mmenke wrote: > The Google style guide requires all class variables to be private. If you need > to access these, can just add accessor methods. Done. https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:116: class TestReporter : public CertificateErrorReporter { On 2015/03/10 15:08:45, mmenke wrote: > Need a virtual destructor in child classes. This class no longer exists. I moved the callback to be called from |OnURLRequestDestroyed| in the network delegate subclass as suggested, and keep track of the information about the latest request there as well. https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:119: : CertificateErrorReporter(context, upload_url), latest_request_url_() { On 2015/03/10 15:08:46, mmenke wrote: > Remove latest_request_url_() - the no argument constructor will be called by > default, no need to explicitly include it. no longer applicable here, but noted for future https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:120: response_started_callback_ = base::Bind(&base::DoNothing); On 2015/03/10 15:08:46, mmenke wrote: > This should be in an initializer list. Done (in the network delegate subclass now). https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:127: latest_request_url_(), On 2015/03/10 15:08:45, mmenke wrote: > Remove latest_request_url_() (no longer applicable) https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:134: response_started_callback_); On 2015/03/10 17:52:32, mmenke wrote: > On 2015/03/10 17:39:30, Ryan Sleevi wrote: > > On 2015/03/10 15:08:46, mmenke wrote: > > > Can invoke this synchronously with just response_started_callback_.Run(). > > > Should also check if it's empty() (is_empty()? is_null()? Can't remember > > what > > > it's called) before invoking the callback. > > > > I'm not sure I'd agree on the is_empty. By not checking, you make the API > > contract clearer than it must do something (and even base::DoNothing is doing > > something) > > You're right - I hadn't realized that the other constructor set a callback that > does nothing (Was actually wondering why it didn't crash in that case). I've > never seen base::DoNothing used before, but I'm fine with it. > > > I suggested PostTask as more idiomatic / reflective of how URLRequest works > (in > > that, in my experience, it tends to PostTask work to clear the recursion > stack). > > I defer to Matt if he disagrees. > > My thinking is that just seems kinda weird to me to do post a task to the thread > the TestReporter is called on, rather than taking one as input (Admittedly, it > would be the same thread), anyhow, I don't have a strong opinion here, just > seemed a little unusual. I think this discussion is no longer applicable, since the callback is now called from |OnURLRequestDestroyed|. https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:137: GURL& GetLatestRequestURL() { return latest_request_url_; } On 2015/03/10 15:08:46, mmenke wrote: > This should be: const GURL& latest_request_url() const { > (+2 consts, change naming convention). (no longer applicable) https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:148: base::Closure callback = run_loop.QuitClosure(); On 2015/03/10 15:08:45, mmenke wrote: > optional nit: No need to store this on the stack. Can just inline > run_loop.QuitClosure() in the TestReporter constructor call. Same goes for > ErroredRequestGetsDeleted. Done. https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:177: delete reporter; On 2015/03/10 15:08:45, mmenke wrote: > Rather than calling delete, I suggest putting it in a scoped_ptr<TestReporter) > on creation, and then resetting the scoped_ptr. It's not a critical issue here, > but it results in less error prone code in general. Done. https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:179: run_loop.RunUntilIdle(); On 2015/03/10 17:52:33, mmenke wrote: > On 2015/03/10 17:39:30, Ryan Sleevi wrote: > > On 2015/03/10 16:08:52, mmenke wrote: > > > Of course, it could *also* post a delayed task to do that. :) If we're > > really > > > concerned about that, we should make the NetworkDelegate have the ability to > > > wait until a request is destroyed (Note that you could actually put all the > > > waiting logic in the NetworkDelegate, and then just wait for destruction on > > all > > > tests. Would make this test look more like the others, too). Anyhow, just > a > > > suggestion. > > > > Too Many Idioms (to the tune of Too Many Cooks) > > > > I suspect that adding the waiting logic to the NetworkDelegate is a > non-trivial > > task, and I'm not sufficiently familiar with the NetworkDelegate to know > whether > > the added behaviour would be adding complexity down the road - that I defer to > > Matt whether it's sufficiently "worth it" for this use case. > > > > Given Matt's concerns, I'm OK with removing this pump - if/when this test > fails, > > we'll revisit it. Hopefully someone will keep an eye on it from being flaky, > > since we no longer have tools that automatically inform us of this (e.g. by > > breaking the tree) > > I'm not suggesting adding anything to the main NetworkDelegate class, just to > TestNetworkDelegate defined in this file. Could just moving the callback we're > currently passing to TestReporter to TestNetworkDelegate, and call it on > destruction. That would also solve the weirdness of OnResponseStarted, without > having to add a virtual method in production code that does nothing, just so the > test fixture can override it. I like to keep production code as clean as we > reasonably can when it comes to extra stuff to help with testing. Done -- the callback is now called from |OnURLRequestDestroyed|. https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:202: On 2015/03/10 15:08:46, mmenke wrote: > Since we're using a std::set for requests, we should have a test that sends > multiple requests, ideally one test that does it sequentially and one that does > it simultaneously. If it's okay with you, I'd like to do this in a follow-up CL, because: 1. landing this CL will unblock a lot of other work and allow me to parallelize more efficiently (e.g. writing these additional tests while waiting for code reviews on other work), and 2. all these tests are for pre-existing code that has been around for a long time without tests. I acknowledge that it's important to have tests while moving this code around because it's possible that something might have broken in the refactoring, so I do solemnly swear to prioritize them, but I think it would be more efficient to do them in a separate CL. What do you think? https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:205: namespace chrome_browser_net { On 2015/03/10 17:39:30, Ryan Sleevi wrote: > On 2015/03/10 16:08:52, mmenke wrote: > > Sorry, didn't notice that. I'm not a fan of friending tests. Could make the > > method protected, and then have the test subclass expose it. I think it's > > simpler than having an exhaustive list of tests in the public header, and it > > also makes it clearer what internal state is poked at by tests, but Ryan may > > disagree, and I'll defer to him here. > > Sorry for the mixed messaging :) I do tend to disagree; I've seen too much > subclassing when made protected, so strongly prefer friending tests (or test > fixtures, which is the other way to avoid having to exhaustively enumerate all > tests.) I usually use 3 as the rule - if I'm having to friend 3 tests, I should > really be friending the fixture itself and then let the fixture marshal that > method for the concrete tests. > > > > > However...It's also worth noting this is most definitely not part of the > public > > interface. I think it would make more sense to check the upload data this > > results in us sending, rather than that private fields are populated as > > expected. > > That seems quite reasonable, and decouples further. Done -- added a check that the uploaded report is as expected. https://codereview.chromium.org/979893003/diff/260001/chrome/browser/net/cert... File chrome/browser/net/certificate_error_reporter_unittest.cc (right): https://codereview.chromium.org/979893003/diff/260001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:100: class CertificateErrorReporterTestNetworkDelegate : public NetworkDelegateImpl { Is this name too disgustingly long? I was trying to make it hint that this network delegate does stuff specific to the certificate error reporter tests (e.g. checking that the uploaded data matches |kHostname| and |GetSSLInfo|).
certificate_error_reporter_unittest.cc LGTM, modulo nits. Didn't look at anything else. https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... File chrome/browser/net/certificate_error_reporter_unittest.cc (right): https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:202: On 2015/03/11 05:58:34, emily wrote: > On 2015/03/10 15:08:46, mmenke wrote: > > Since we're using a std::set for requests, we should have a test that sends > > multiple requests, ideally one test that does it sequentially and one that > does > > it simultaneously. > > If it's okay with you, I'd like to do this in a follow-up CL, because: 1. > landing this CL will unblock a lot of other work and allow me to parallelize > more efficiently (e.g. writing these additional tests while waiting for code > reviews on other work), and 2. all these tests are for pre-existing code that > has been around for a long time without tests. I acknowledge that it's important > to have tests while moving this code around because it's possible that something > might have broken in the refactoring, so I do solemnly swear to prioritize them, > but I think it would be more efficient to do them in a separate CL. What do you > think? If this were a CL adding a feature, I'd definitely want it in before landing. Since it's refactoring and adding tests to old code, I still think it's a good idea, but not something I'm going to block this CL on (I'm also not really an owner of any this code, so happily defer to Ryan's judgment, regardless). https://codereview.chromium.org/979893003/diff/260001/chrome/browser/net/cert... File chrome/browser/net/certificate_error_reporter.h (right): https://codereview.chromium.org/979893003/diff/260001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter.h:73: DISALLOW_COPY_AND_ASSIGN(CertificateErrorReporter); nit: Include macros.h for DISALLOW_COPY_AND_ASSIGN. https://codereview.chromium.org/979893003/diff/260001/chrome/browser/net/cert... File chrome/browser/net/certificate_error_reporter_unittest.cc (right): https://codereview.chromium.org/979893003/diff/260001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:79: EXPECT_EQ(upload->GetElementReaders()->size(), 1u); For EXPECT_EQ and ASSERT_EQ, the expected value should go before the actual one. Goes for this entire method. https://codereview.chromium.org/979893003/diff/260001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:100: class CertificateErrorReporterTestNetworkDelegate : public NetworkDelegateImpl { On 2015/03/11 05:58:34, emily wrote: > Is this name too disgustingly long? I was trying to make it hint that this > network delegate does stuff specific to the certificate error reporter tests > (e.g. checking that the uploaded data matches |kHostname| and |GetSSLInfo|). I'm fine with long names. Maybe call it TestCertificateErrorReporterNetworkDelegate instead? (There are a bunch of TestNetworkDelegates, so moving the Test makes it easier to find, and doesn't come up on searches for the other test delegates). https://codereview.chromium.org/979893003/diff/260001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:123: void SetURLRequestDestroyedCallback(base::Closure callback) { nit: const base::Closure& https://codereview.chromium.org/979893003/diff/260001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:127: void SetExpectedURL(const GURL& url) { expected_url_ = url; } nit: set_expected_url. Also more comment to call the argument |expected_url|. Should probably do the same for SetURLRequestDestroyedCallback as well. Optionally, could take both as constructor parameters and make them const, but I think it's fine, either way. https://codereview.chromium.org/979893003/diff/260001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:134: GURL expected_url_; nit: DISALLOW_COPY_AND_ASSIGN(CertificateErrorReporterTestNetworkDelegate); (After a blank line) And include macros.h" https://codereview.chromium.org/979893003/diff/260001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:147: chrome_browser_net::SetUrlRequestMocksEnabled(false); This isn't needed - it does the same thing as the EnableUrlRequestMocks call above. You can also remove the header for it (url_request_mock_util.h) https://codereview.chromium.org/979893003/diff/260001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:157: content::TestBrowserThreadBundle thread_bundle_; Do we still needs this? Think we can get away with just a base::MessageLoop now. Fine if we need it, but if not, like to keep the dependencies to a minimum. https://codereview.chromium.org/979893003/diff/260001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:159: CertificateErrorReporterTestNetworkDelegate network_delegate_; The network delegate should be before the context, so it gets destroyed first (Context depends on the network delegate). Doesn't really matter for these tests, but it's a good habit to be in.
https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... File chrome/browser/net/certificate_error_reporter_unittest.cc (right): https://codereview.chromium.org/979893003/diff/220001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:202: On 2015/03/11 15:34:28, mmenke wrote: > On 2015/03/11 05:58:34, emily wrote: > > On 2015/03/10 15:08:46, mmenke wrote: > > > Since we're using a std::set for requests, we should have a test that sends > > > multiple requests, ideally one test that does it sequentially and one that > > does > > > it simultaneously. > > > > If it's okay with you, I'd like to do this in a follow-up CL, because: 1. > > landing this CL will unblock a lot of other work and allow me to parallelize > > more efficiently (e.g. writing these additional tests while waiting for code > > reviews on other work), and 2. all these tests are for pre-existing code that > > has been around for a long time without tests. I acknowledge that it's > important > > to have tests while moving this code around because it's possible that > something > > might have broken in the refactoring, so I do solemnly swear to prioritize > them, > > but I think it would be more efficient to do them in a separate CL. What do > you > > think? > > If this were a CL adding a feature, I'd definitely want it in before landing. > Since it's refactoring and adding tests to old code, I still think it's a good > idea, but not something I'm going to block this CL on (I'm also not really an > owner of any this code, so happily defer to Ryan's judgment, regardless). My $0.02: - This CL is blocking a lot of other work, - This specific CL is a fairly straightforward refactor of old code; if the tests reveal any problems, they will be pre-existing problems, and - I trust Emily to write the tests in short order.
Thanks mmenke, nits addressed. https://codereview.chromium.org/979893003/diff/260001/chrome/browser/net/cert... File chrome/browser/net/certificate_error_reporter.h (right): https://codereview.chromium.org/979893003/diff/260001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter.h:73: DISALLOW_COPY_AND_ASSIGN(CertificateErrorReporter); On 2015/03/11 15:34:28, mmenke wrote: > nit: Include macros.h for DISALLOW_COPY_AND_ASSIGN. Done. https://codereview.chromium.org/979893003/diff/260001/chrome/browser/net/cert... File chrome/browser/net/certificate_error_reporter_unittest.cc (right): https://codereview.chromium.org/979893003/diff/260001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:79: EXPECT_EQ(upload->GetElementReaders()->size(), 1u); On 2015/03/11 15:34:28, mmenke wrote: > For EXPECT_EQ and ASSERT_EQ, the expected value should go before the actual one. > Goes for this entire method. Done. https://codereview.chromium.org/979893003/diff/260001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:100: class CertificateErrorReporterTestNetworkDelegate : public NetworkDelegateImpl { On 2015/03/11 15:34:28, mmenke wrote: > On 2015/03/11 05:58:34, emily wrote: > > Is this name too disgustingly long? I was trying to make it hint that this > > network delegate does stuff specific to the certificate error reporter tests > > (e.g. checking that the uploaded data matches |kHostname| and |GetSSLInfo|). > > I'm fine with long names. Maybe call it > TestCertificateErrorReporterNetworkDelegate instead? (There are a bunch of > TestNetworkDelegates, so moving the Test makes it easier to find, and doesn't > come up on searches for the other test delegates). Done. https://codereview.chromium.org/979893003/diff/260001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:123: void SetURLRequestDestroyedCallback(base::Closure callback) { On 2015/03/11 15:34:28, mmenke wrote: > nit: const base::Closure& Done. https://codereview.chromium.org/979893003/diff/260001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:127: void SetExpectedURL(const GURL& url) { expected_url_ = url; } On 2015/03/11 15:34:28, mmenke wrote: > nit: set_expected_url. Also more comment to call the argument |expected_url|. > Should probably do the same for SetURLRequestDestroyedCallback as well. > > Optionally, could take both as constructor parameters and make them const, but I > think it's fine, either way. Done. https://codereview.chromium.org/979893003/diff/260001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:134: GURL expected_url_; On 2015/03/11 15:34:28, mmenke wrote: > nit: > > DISALLOW_COPY_AND_ASSIGN(CertificateErrorReporterTestNetworkDelegate); > (After a blank line) > > And include macros.h" Done. https://codereview.chromium.org/979893003/diff/260001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:147: chrome_browser_net::SetUrlRequestMocksEnabled(false); On 2015/03/11 15:34:28, mmenke wrote: > This isn't needed - it does the same thing as the EnableUrlRequestMocks call > above. You can also remove the header for it (url_request_mock_util.h) Done. https://codereview.chromium.org/979893003/diff/260001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:157: content::TestBrowserThreadBundle thread_bundle_; On 2015/03/11 15:34:28, mmenke wrote: > Do we still needs this? Think we can get away with just a base::MessageLoop > now. Fine if we need it, but if not, like to keep the dependencies to a > minimum. Done. https://codereview.chromium.org/979893003/diff/260001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:159: CertificateErrorReporterTestNetworkDelegate network_delegate_; On 2015/03/11 15:34:28, mmenke wrote: > The network delegate should be before the context, so it gets destroyed first > (Context depends on the network delegate). Doesn't really matter for these > tests, but it's a good habit to be in. Done.
Thanks Matt for reviewing the tests. I really need to learn the idioms for URLRequest so I don't have to punt to you all the time. LGTM, but I mostly have nits on comments/documentation/style, but no need to block on a round-trip from me. https://codereview.chromium.org/979893003/diff/300001/chrome/browser/net/cert... File chrome/browser/net/certificate_error_reporter.cc (right): https://codereview.chromium.org/979893003/diff/300001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter.cc:117: } nit: If you received advice otherwise, disregard my comment, but AIUI, //chrome/browser/net generally follows the //net style of omitting braces for single-line conditionals (Yes, the original code was inconsistent; I'm slightly preferable to dropping the {}, but it's a nit) https://codereview.chromium.org/979893003/diff/300001/chrome/browser/net/cert... File chrome/browser/net/certificate_error_reporter.h (right): https://codereview.chromium.org/979893003/diff/300001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter.h:32: }; Enhance. Enhance. (With documentation) https://codereview.chromium.org/979893003/diff/300001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter.h:34: CertificateErrorReporter(net::URLRequestContext* request_context, Document // Create a certificate error reporter that will send certificate error reports to |upload_url|, // using |request_context| as the context for such reports. (Or something; Matt or Adrienne can probably word something less awkward) https://codereview.chromium.org/979893003/diff/300001/chrome/browser/net/cert... File chrome/browser/net/certificate_error_reporter_unittest.cc (right): https://codereview.chromium.org/979893003/diff/300001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:43: const char kTestCertFilename[] = FILE_PATH_LITERAL("test_mail_google_com.pem"); You need to have this use base::FilePath::CharType kTestCertFilename[], or else this won't compile on Windows (where file paths are 16-bit unicode, not 8-bit), and use .Append() on 56 Alternatively, drop the FILE_PATH_LITERAL and continue to use .AppendASCII on line 56 (the caveat is an extra UTF-8 -> 16 conversion on Win, but *shrug*, this is test code) https://codereview.chromium.org/979893003/diff/300001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:45: SSLInfo GetSSLInfo() { pedantic nit: GetTestSSLInfo()? https://codereview.chromium.org/979893003/diff/300001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:64: if (enable) { if (!enable) return base::FilePath root_http; ... Prefer early returns; they avoid added nesting complexity https://codereview.chromium.org/979893003/diff/300001/chrome/browser/net/chro... File chrome/browser/net/chrome_fraudulent_certificate_reporter.cc (right): https://codereview.chromium.org/979893003/diff/300001/chrome/browser/net/chro... chrome/browser/net/chrome_fraudulent_certificate_reporter.cc:17: const char kFraudulentCertificateUploadEndpoint[] = Since this is moving into internal linkage (assuming I'm reading the CL correctly), it would be better to move this into an unnamed namespace within this file. Just in case :) (Namespaces + const always give me trouble on the correct linkages; I *believe* const implies static implies internal for a POD in a named namespace, but I can never remember the rules 100%)
Thanks rsleevi! felt@, do you want to take another look before I commit? https://codereview.chromium.org/979893003/diff/300001/chrome/browser/net/cert... File chrome/browser/net/certificate_error_reporter.cc (right): https://codereview.chromium.org/979893003/diff/300001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter.cc:117: } On 2015/03/12 05:15:05, Ryan Sleevi wrote: > nit: If you received advice otherwise, disregard my comment, but AIUI, > //chrome/browser/net generally follows the //net style of omitting braces for > single-line conditionals > > (Yes, the original code was inconsistent; I'm slightly preferable to dropping > the {}, but it's a nit) Done. https://codereview.chromium.org/979893003/diff/300001/chrome/browser/net/cert... File chrome/browser/net/certificate_error_reporter.h (right): https://codereview.chromium.org/979893003/diff/300001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter.h:32: }; On 2015/03/12 05:15:05, Ryan Sleevi wrote: > Enhance. Enhance. > > > (With documentation) Done. https://codereview.chromium.org/979893003/diff/300001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter.h:34: CertificateErrorReporter(net::URLRequestContext* request_context, On 2015/03/12 05:15:05, Ryan Sleevi wrote: > Document > > // Create a certificate error reporter that will send certificate error reports > to |upload_url|, > // using |request_context| as the context for such reports. > > (Or something; Matt or Adrienne can probably word something less awkward) Done. https://codereview.chromium.org/979893003/diff/300001/chrome/browser/net/cert... File chrome/browser/net/certificate_error_reporter_unittest.cc (right): https://codereview.chromium.org/979893003/diff/300001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:43: const char kTestCertFilename[] = FILE_PATH_LITERAL("test_mail_google_com.pem"); On 2015/03/12 05:15:05, Ryan Sleevi wrote: > You need to have this use base::FilePath::CharType kTestCertFilename[], or else > this won't compile on Windows (where file paths are 16-bit unicode, not 8-bit), > and use .Append() on 56 > > Alternatively, drop the FILE_PATH_LITERAL and continue to use .AppendASCII on > line 56 (the caveat is an extra UTF-8 -> 16 conversion on Win, but *shrug*, this > is test code) Done. https://codereview.chromium.org/979893003/diff/300001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:45: SSLInfo GetSSLInfo() { On 2015/03/12 05:15:05, Ryan Sleevi wrote: > pedantic nit: GetTestSSLInfo()? Done. https://codereview.chromium.org/979893003/diff/300001/chrome/browser/net/cert... chrome/browser/net/certificate_error_reporter_unittest.cc:64: if (enable) { On 2015/03/12 05:15:05, Ryan Sleevi wrote: > if (!enable) > return > > base::FilePath root_http; > ... > > Prefer early returns; they avoid added nesting complexity Done. https://codereview.chromium.org/979893003/diff/300001/chrome/browser/net/chro... File chrome/browser/net/chrome_fraudulent_certificate_reporter.cc (right): https://codereview.chromium.org/979893003/diff/300001/chrome/browser/net/chro... chrome/browser/net/chrome_fraudulent_certificate_reporter.cc:17: const char kFraudulentCertificateUploadEndpoint[] = On 2015/03/12 05:15:05, Ryan Sleevi wrote: > Since this is moving into internal linkage (assuming I'm reading the CL > correctly), it would be better to move this into an unnamed namespace within > this file. Just in case :) > > (Namespaces + const always give me trouble on the correct linkages; I *believe* > const implies static implies internal for a POD in a named namespace, but I can > never remember the rules 100%) Done.
On 2015/03/12 14:21:49, emily wrote: > Thanks rsleevi! > > felt@, do you want to take another look before I commit? I trust that Ryan and Matt have been thorough, go ahead and commit. I'll take a look at the other CLs once they're rebased. > > https://codereview.chromium.org/979893003/diff/300001/chrome/browser/net/cert... > File chrome/browser/net/certificate_error_reporter.cc (right): > > https://codereview.chromium.org/979893003/diff/300001/chrome/browser/net/cert... > chrome/browser/net/certificate_error_reporter.cc:117: } > On 2015/03/12 05:15:05, Ryan Sleevi wrote: > > nit: If you received advice otherwise, disregard my comment, but AIUI, > > //chrome/browser/net generally follows the //net style of omitting braces for > > single-line conditionals > > > > (Yes, the original code was inconsistent; I'm slightly preferable to dropping > > the {}, but it's a nit) > > Done. > > https://codereview.chromium.org/979893003/diff/300001/chrome/browser/net/cert... > File chrome/browser/net/certificate_error_reporter.h (right): > > https://codereview.chromium.org/979893003/diff/300001/chrome/browser/net/cert... > chrome/browser/net/certificate_error_reporter.h:32: }; > On 2015/03/12 05:15:05, Ryan Sleevi wrote: > > Enhance. Enhance. > > > > > > (With documentation) > > Done. > > https://codereview.chromium.org/979893003/diff/300001/chrome/browser/net/cert... > chrome/browser/net/certificate_error_reporter.h:34: > CertificateErrorReporter(net::URLRequestContext* request_context, > On 2015/03/12 05:15:05, Ryan Sleevi wrote: > > Document > > > > // Create a certificate error reporter that will send certificate error > reports > > to |upload_url|, > > // using |request_context| as the context for such reports. > > > > (Or something; Matt or Adrienne can probably word something less awkward) > > Done. > > https://codereview.chromium.org/979893003/diff/300001/chrome/browser/net/cert... > File chrome/browser/net/certificate_error_reporter_unittest.cc (right): > > https://codereview.chromium.org/979893003/diff/300001/chrome/browser/net/cert... > chrome/browser/net/certificate_error_reporter_unittest.cc:43: const char > kTestCertFilename[] = FILE_PATH_LITERAL("test_mail_google_com.pem"); > On 2015/03/12 05:15:05, Ryan Sleevi wrote: > > You need to have this use base::FilePath::CharType kTestCertFilename[], or > else > > this won't compile on Windows (where file paths are 16-bit unicode, not > 8-bit), > > and use .Append() on 56 > > > > Alternatively, drop the FILE_PATH_LITERAL and continue to use .AppendASCII on > > line 56 (the caveat is an extra UTF-8 -> 16 conversion on Win, but *shrug*, > this > > is test code) > > Done. > > https://codereview.chromium.org/979893003/diff/300001/chrome/browser/net/cert... > chrome/browser/net/certificate_error_reporter_unittest.cc:45: SSLInfo > GetSSLInfo() { > On 2015/03/12 05:15:05, Ryan Sleevi wrote: > > pedantic nit: GetTestSSLInfo()? > > Done. > > https://codereview.chromium.org/979893003/diff/300001/chrome/browser/net/cert... > chrome/browser/net/certificate_error_reporter_unittest.cc:64: if (enable) { > On 2015/03/12 05:15:05, Ryan Sleevi wrote: > > if (!enable) > > return > > > > base::FilePath root_http; > > ... > > > > Prefer early returns; they avoid added nesting complexity > > Done. > > https://codereview.chromium.org/979893003/diff/300001/chrome/browser/net/chro... > File chrome/browser/net/chrome_fraudulent_certificate_reporter.cc (right): > > https://codereview.chromium.org/979893003/diff/300001/chrome/browser/net/chro... > chrome/browser/net/chrome_fraudulent_certificate_reporter.cc:17: const char > kFraudulentCertificateUploadEndpoint[] = > On 2015/03/12 05:15:05, Ryan Sleevi wrote: > > Since this is moving into internal linkage (assuming I'm reading the CL > > correctly), it would be better to move this into an unnamed namespace within > > this file. Just in case :) > > > > (Namespaces + const always give me trouble on the correct linkages; I > *believe* > > const implies static implies internal for a POD in a named namespace, but I > can > > never remember the rules 100%) > > Done.
The CQ bit was checked by estark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org, rsleevi@chromium.org Link to the patchset: https://codereview.chromium.org/979893003/#ps320001 (title: "style fixes, add comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/979893003/320001
The CQ bit was checked by estark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/979893003/#ps340001 (title: "remove FILE_PATH_LITERAL; ImportCertFromFile uses AppendASCII")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/979893003/340001
Patchset #18 (id:360001) has been deleted
The CQ bit was checked by estark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/979893003/#ps380001 (title: "use URLRequestMockDataJob to avoid IO in unit test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/979893003/380001
Message was sent while issue was closed.
Committed patchset #18 (id:380001)
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/4c9dc455208c9526888497525aba8a294f456edf Cr-Commit-Position: refs/heads/master@{#320375} |
