Refactor ChromeFraudulentCertReporter for code reuse by SSL reporting

chrome/browser/net/chrome_fraudulent_certificate_reporter.h

Index: chrome/browser/net/chrome_fraudulent_certificate_reporter.h
diff --git a/chrome/browser/net/chrome_fraudulent_certificate_reporter.h b/chrome/browser/net/chrome_fraudulent_certificate_reporter.h
index c9874768f1e818aeccfa58e670851e2dfc4f55ed..791e03d932cef1e8aa6ccae3c7afdb3ee8d17056 100644
--- a/chrome/browser/net/chrome_fraudulent_certificate_reporter.h
+++ b/chrome/browser/net/chrome_fraudulent_certificate_reporter.h
@@ -8,6 +8,7 @@
 #include <set>
 #include <string>
 
+#include "chrome/browser/net/chrome_certificate_reporter.h"
 #include "net/url_request/fraudulent_certificate_reporter.h"
 #include "net/url_request/url_request.h"
 
@@ -19,37 +20,17 @@ namespace chrome_browser_net {
 
 class ChromeFraudulentCertificateReporter
     : public net::FraudulentCertificateReporter,
-      public net::URLRequest::Delegate {
+      public ChromeCertificateReporter {

[Ryan Sleevi, 2015/03/04 19:31:01]

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.

[estark, 2015/03/04 22:11:57]

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?

  public:
   explicit ChromeFraudulentCertificateReporter(
       net::URLRequestContext* request_context);
 
-  ~ChromeFraudulentCertificateReporter() override;
+  ~ChromeFraudulentCertificateReporter() override {}
 
-  // Allows users of this class to override this and set their own URLRequest
-  // type. Used by SendReport.
-  virtual scoped_ptr<net::URLRequest> CreateURLRequest(
-      net::URLRequestContext* context);
-
-  // net::FraudulentCertificateReporter
+  // net::FraudulentCertificateReporter, ChromeCertificateReporter
   void SendReport(const std::string& hostname,
                   const net::SSLInfo& ssl_info) override;
 
-  // net::URLRequest::Delegate
-  void OnResponseStarted(net::URLRequest* request) override;
-  void OnReadCompleted(net::URLRequest* request, int bytes_read) override;
-
- protected:
-  net::URLRequestContext* const request_context_;
-
- 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(ChromeFraudulentCertificateReporter);
 };