Style/readability clean-up for certificate reporting code

Various clean-ups and style fixes for certificate reporting classes and tests.

BUG=

Review URL: https://codereview.chromium.org/1210053006

Cr-Commit-Position: refs/heads/master@{#342449}

3 files changed, 43 insertions, 59 deletions

diff --git a/net/url_request/certificate_report_sender.cc b/net/url_request/certificate_report_sender.cc
index 0361a32..fd5d11c 100644
--- a/net/url_request/certificate_report_sender.cc
+++ b/net/url_request/certificate_report_sender.cc
@@ -21,6 +21,7 @@ CertificateReportSender::CertificateReportSender(
       cookies_preference_(cookies_preference) {}
 
 CertificateReportSender::~CertificateReportSender() {
+  // Cancel all of the uncompleted requests.
   STLDeleteElements(&inflight_requests_);
 }
 
@@ -43,7 +44,11 @@ void CertificateReportSender::Send(const GURL& report_uri,
 void CertificateReportSender::OnResponseStarted(URLRequest* request) {
   // TODO(estark): call a callback so that the caller can print a
   // warning on failure.
-  RequestComplete(request);
+  DVLOG(1) << "Failed to send certificate report for " << request->url().host();
+
+  CHECK_GT(inflight_requests_.erase(request), 0u);
+  // Clean up the request, which cancels it.
+  delete request;
 }
 
 void CertificateReportSender::OnReadCompleted(URLRequest* request,
@@ -66,11 +71,4 @@ scoped_ptr<URLRequest> CertificateReportSender::CreateURLRequest(
   return request.Pass();
 }
 
-void CertificateReportSender::RequestComplete(URLRequest* request) {
-  std::set<URLRequest*>::iterator i = inflight_requests_.find(request);
-  CHECK(i != inflight_requests_.end());
-  scoped_ptr<URLRequest> url_request(*i);
-  inflight_requests_.erase(i);
-}
-
 }  // namespace net
diff --git a/net/url_request/certificate_report_sender.h b/net/url_request/certificate_report_sender.h
index 9073836..dac8a57 100644
--- a/net/url_request/certificate_report_sender.h
+++ b/net/url_request/certificate_report_sender.h
@@ -34,17 +34,17 @@ class NET_EXPORT CertificateReportSender
 
   // Constructs a CertificateReportSender that sends reports with the
   // given |request_context| and includes or excludes cookies based on
-  // |cookies_preference|. Ownership of |request_context| is not
-  // transferred, so it must outlive the CertificateReportSender.
+  // |cookies_preference|. |request_context| must outlive the
+  // CertificateReportSender.
   CertificateReportSender(URLRequestContext* request_context,
                           CookiesPreference cookies_preference);
 
   ~CertificateReportSender() override;
 
-  // TransportSecurityState::ReportSender
+  // TransportSecurityState::ReportSender implementation.
   void Send(const GURL& report_uri, const std::string& report) override;
 
-  // net::URLRequest::Delegate
+  // net::URLRequest::Delegate implementation.
   void OnResponseStarted(URLRequest* request) override;
   void OnReadCompleted(URLRequest* request, int bytes_read) override;
 
@@ -58,9 +58,6 @@ class NET_EXPORT CertificateReportSender
       net::URLRequestContext* context,
       const GURL& report_uri);
 
-  // Performs post-report cleanup.
-  void RequestComplete(net::URLRequest* request);
-
   net::URLRequestContext* const request_context_;
 
   CookiesPreference cookies_preference_;
diff --git a/net/url_request/certificate_report_sender_unittest.cc b/net/url_request/certificate_report_sender_unittest.cc
index ca652f7..0024bcb 100644
--- a/net/url_request/certificate_report_sender_unittest.cc
+++ b/net/url_request/certificate_report_sender_unittest.cc
@@ -21,7 +21,6 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace net {
-
 namespace {
 
 const char kDummyReport[] = "foo.test";
@@ -31,20 +30,11 @@ void MarkURLRequestDestroyed(bool* url_request_destroyed) {
   *url_request_destroyed = true;
 }
 
-void SetUrlRequestMocksEnabled(bool enable) {
-  URLRequestFilter::GetInstance()->ClearHandlers();
-  if (!enable)
-    return;
-
-  URLRequestFailedJob::AddUrlHandler();
-  URLRequestMockDataJob::AddUrlHandler();
-}
-
-// Check that data uploaded in the request matches the test report
-// data. The sent reports will be erased from |expect_reports|.
-void CheckUploadData(URLRequest* request,
+// Checks that data uploaded in the request matches the test report
+// data. Erases the sent reports from |expect_reports|.
+void CheckUploadData(const URLRequest& request,
                      std::set<std::string>* expect_reports) {
-  const UploadDataStream* upload = request->get_upload();
+  const UploadDataStream* upload = request.get_upload();
   ASSERT_TRUE(upload);
   ASSERT_TRUE(upload->GetElementReaders());
   ASSERT_EQ(1u, upload->GetElementReaders()->size());
@@ -69,20 +59,16 @@ class TestCertificateReportSenderNetworkDelegate : public NetworkDelegateImpl {
         num_requests_(0),
         expect_cookies_(false) {}
 
-  ~TestCertificateReportSenderNetworkDelegate() override {}
-
   void ExpectReport(const std::string& report) {
     expect_reports_.insert(report);
   }
 
-  void set_all_url_requests_destroyed_callback(
-      const base::Closure& all_url_requests_destroyed_callback) {
-    all_url_requests_destroyed_callback_ = all_url_requests_destroyed_callback;
+  void set_all_url_requests_destroyed_callback(const base::Closure& callback) {
+    all_url_requests_destroyed_callback_ = callback;
   }
 
-  void set_url_request_destroyed_callback(
-      const base::Closure& url_request_destroyed_callback) {
-    url_request_destroyed_callback_ = url_request_destroyed_callback;
+  void set_url_request_destroyed_callback(const base::Closure& callback) {
+    url_request_destroyed_callback_ = callback;
   }
 
   void set_expect_url(const GURL& expect_url) { expect_url_ = expect_url; }
@@ -94,7 +80,7 @@ class TestCertificateReportSenderNetworkDelegate : public NetworkDelegateImpl {
     expect_cookies_ = expect_cookies;
   }
 
-  // NetworkDelegateImpl implementation
+  // NetworkDelegateImpl implementation.
   int OnBeforeURLRequest(URLRequest* request,
                          const CompletionCallback& callback,
                          GURL* new_url) override {
@@ -110,7 +96,10 @@ class TestCertificateReportSenderNetworkDelegate : public NetworkDelegateImpl {
       EXPECT_TRUE(request->load_flags() & LOAD_DO_NOT_SAVE_COOKIES);
     }
 
-    CheckUploadData(request, &expect_reports_);
+    CheckUploadData(*request, &expect_reports_);
+
+    // Unconditionally return OK, since the sender ignores the results
+    // anyway.
     return OK;
   }
 
@@ -134,17 +123,17 @@ class TestCertificateReportSenderNetworkDelegate : public NetworkDelegateImpl {
 class CertificateReportSenderTest : public ::testing::Test {
  public:
   CertificateReportSenderTest() : context_(true) {
-    SetUrlRequestMocksEnabled(true);
     context_.set_network_delegate(&network_delegate_);
     context_.Init();
   }
 
-  ~CertificateReportSenderTest() override { SetUrlRequestMocksEnabled(false); }
-
-  TestCertificateReportSenderNetworkDelegate* network_delegate() {
-    return &network_delegate_;
+  void SetUp() override {
+    URLRequestFailedJob::AddUrlHandler();
+    URLRequestMockDataJob::AddUrlHandler();
   }
 
+  void TearDown() override { URLRequestFilter::GetInstance()->ClearHandlers(); }
+
   TestURLRequestContext* context() { return &context_; }
 
  protected:
@@ -171,8 +160,9 @@ class CertificateReportSenderTest : public ::testing::Test {
     EXPECT_EQ(request_sequence_number + 1, network_delegate_.num_requests());
   }
 
- private:
   TestCertificateReportSenderNetworkDelegate network_delegate_;
+
+ private:
   TestURLRequestContext context_;
 };
 
@@ -195,47 +185,47 @@ TEST_F(CertificateReportSenderTest, SendMultipleReportsSequentially) {
 
 TEST_F(CertificateReportSenderTest, SendMultipleReportsSimultaneously) {
   base::RunLoop run_loop;
-  network_delegate()->set_all_url_requests_destroyed_callback(
+  network_delegate_.set_all_url_requests_destroyed_callback(
       run_loop.QuitClosure());
 
   GURL url = URLRequestMockDataJob::GetMockHttpsUrl("dummy data", 1);
-  network_delegate()->set_expect_url(url);
-  network_delegate()->ExpectReport(kDummyReport);
-  network_delegate()->ExpectReport(kSecondDummyReport);
+  network_delegate_.set_expect_url(url);
+  network_delegate_.ExpectReport(kDummyReport);
+  network_delegate_.ExpectReport(kSecondDummyReport);
 
   CertificateReportSender reporter(
       context(), CertificateReportSender::DO_NOT_SEND_COOKIES);
 
-  EXPECT_EQ(0u, network_delegate()->num_requests());
+  EXPECT_EQ(0u, network_delegate_.num_requests());
 
   reporter.Send(url, kDummyReport);
   reporter.Send(url, kSecondDummyReport);
 
   run_loop.Run();
 
-  EXPECT_EQ(2u, network_delegate()->num_requests());
+  EXPECT_EQ(2u, network_delegate_.num_requests());
 }
 
 // Test that pending URLRequests get cleaned up when the report sender
 // is deleted.
 TEST_F(CertificateReportSenderTest, PendingRequestGetsDeleted) {
   bool url_request_destroyed = false;
-  network_delegate()->set_url_request_destroyed_callback(base::Bind(
+  network_delegate_.set_url_request_destroyed_callback(base::Bind(
       &MarkURLRequestDestroyed, base::Unretained(&url_request_destroyed)));
 
   GURL url = URLRequestFailedJob::GetMockHttpUrlWithFailurePhase(
       URLRequestFailedJob::START, ERR_IO_PENDING);
-  network_delegate()->set_expect_url(url);
-  network_delegate()->ExpectReport(kDummyReport);
+  network_delegate_.set_expect_url(url);
+  network_delegate_.ExpectReport(kDummyReport);
 
-  EXPECT_EQ(0u, network_delegate()->num_requests());
+  EXPECT_EQ(0u, network_delegate_.num_requests());
 
   scoped_ptr<CertificateReportSender> reporter(new CertificateReportSender(
       context(), CertificateReportSender::DO_NOT_SEND_COOKIES));
   reporter->Send(url, kDummyReport);
   reporter.reset();
 
-  EXPECT_EQ(1u, network_delegate()->num_requests());
+  EXPECT_EQ(1u, network_delegate_.num_requests());
   EXPECT_TRUE(url_request_destroyed);
 }
 
@@ -256,7 +246,7 @@ TEST_F(CertificateReportSenderTest, SendCookiesPreference) {
   CertificateReportSender reporter(context(),
                                    CertificateReportSender::SEND_COOKIES);
 
-  network_delegate()->set_expect_cookies(true);
+  network_delegate_.set_expect_cookies(true);
   SendReport(&reporter, kDummyReport, url, 0);
 }
 
@@ -265,10 +255,9 @@ TEST_F(CertificateReportSenderTest, DoNotSendCookiesPreference) {
   CertificateReportSender reporter(
       context(), CertificateReportSender::DO_NOT_SEND_COOKIES);
 
-  network_delegate()->set_expect_cookies(false);
+  network_delegate_.set_expect_cookies(false);
   SendReport(&reporter, kDummyReport, url, 0);
 }
 
 }  // namespace
-
 }  // namespace net