Refactor ChromeFraudulentCertReporter for code reuse by SSL reporting

chrome/browser/net/certificate_error_reporter.h

Index: chrome/browser/net/certificate_error_reporter.h
diff --git a/chrome/browser/net/certificate_error_reporter.h b/chrome/browser/net/certificate_error_reporter.h
new file mode 100644
index 0000000000000000000000000000000000000000..9661805826d1a962b8dd97d0ee91814dcb2bdfad
--- /dev/null
+++ b/chrome/browser/net/certificate_error_reporter.h
@@ -0,0 +1,77 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_NET_CERTIFICATE_ERROR_REPORTER_H_
+#define CHROME_BROWSER_NET_CERTIFICATE_ERROR_REPORTER_H_
+
+#include <set>
+#include <string>
+
+#include "net/url_request/url_request.h"
+
+namespace net {
+class URLRequestContext;
+}
+
+namespace chrome_browser_net {
+
+class CertLoggerRequest;
+
+// Provides functionality for sending reports about invalid SSL
+// certificate chains to a report collection server.
+class CertificateErrorReporter : public net::URLRequest::Delegate {
+ public:
+  explicit CertificateErrorReporter(net::URLRequestContext* request_context,
+                                    const GURL& upload_url);
+
+  ~CertificateErrorReporter() override;
+
+  enum ReportType {

[Ryan Sleevi, 2015/03/06 00:21:32]

STYLE: These go first (
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...
)

[estark, 2015/03/06 02:17:42]

Done.

+    REPORT_TYPE_PINNING_VIOLATION,
+    REPORT_TYPE_EXTENDED_REPORTING
+  };
+
+  // Allows users of this class to override this and set their own URLRequest
+  // type. Used by SendReport.

[Ryan Sleevi, 2015/03/06 00:21:32]

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.

[estark, 2015/03/06 02:17:42]

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.

+  virtual scoped_ptr<net::URLRequest> CreateURLRequest(
+      net::URLRequestContext* context);
+
+  // Construct, serialize, and send a certificate reporter to the report
+  // collection server containing the |ssl_info| associated with a
+  // connection to |hostname|.
+  virtual void SendReport(ReportType type,
+                          const std::string& hostname,
+                          const net::SSLInfo& ssl_info);
+
+  // net::URLRequest::Delegate
+  void OnResponseStarted(net::URLRequest* request) override;
+  void OnReadCompleted(net::URLRequest* request, int bytes_read) override;
+
+ protected:
+  // Populate the CertLoggerRequest for a report.
+  static void BuildReport(const std::string& hostname,

[Ryan Sleevi, 2015/03/06 00:21:32]

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

[estark, 2015/03/06 02:17:41]

Done.

(Were you also asking why is it static, not just why is it protected? Should it
not be static?)

+                          const net::SSLInfo& ssl_info,
+                          CertLoggerRequest& out_request);
+
+  // Serialize and send a CertLoggerRequest protobuf to the report
+  // collection server.
+  void SendCertLoggerRequest(CertLoggerRequest& request);
+
+  net::URLRequestContext* const request_context_;

[Ryan Sleevi, 2015/03/06 00:21:32]

STYLE: Data members should be private (
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Access_Control )

[estark, 2015/03/06 02:17:42]

Done.

[estark, 2015/03/06 02:17:42]

Done.

+
+ private:
+  // Performs post-report cleanup.
+  void RequestComplete(net::URLRequest* request);
+
+  const GURL upload_url_;
+
+  // Owns the contained requests.
+  std::set<net::URLRequest*> inflight_requests_;
+
+  DISALLOW_COPY_AND_ASSIGN(CertificateErrorReporter);
+};
+
+}  // namespace chrome_browser_net
+
+#endif  // CHROME_BROWSER_NET_CERTIFICATE_ERROR_REPORTER_H_