Refactor ChromeFraudulentCertReporter for code reuse by SSL reporting

chrome/browser/net/chrome_fraudulent_certificate_reporter_unittest.cc

Index: chrome/browser/net/chrome_fraudulent_certificate_reporter_unittest.cc
diff --git a/chrome/browser/net/chrome_fraudulent_certificate_reporter_unittest.cc b/chrome/browser/net/chrome_fraudulent_certificate_reporter_unittest.cc
index 6aff91899b2c0bfdb209acaddd441db5d0c429bf..12366f09e444647a35762cd9ad004c6031bf5070 100644
--- a/chrome/browser/net/chrome_fraudulent_certificate_reporter_unittest.cc
+++ b/chrome/browser/net/chrome_fraudulent_certificate_reporter_unittest.cc
@@ -112,13 +112,23 @@ class NotSendingTestReporter : public TestReporter {
   }
 };
 
-// A ChromeFraudulentCertificateReporter that uses a MockURLRequest, but is
+// A CertificateErrorReporter that uses a MockURLRequest, but is
 // otherwise normal: reports are constructed and sent in the usual way.
-class MockReporter : public ChromeFraudulentCertificateReporter {
+class MockReporter : public CertificateErrorReporter {
  public:
   explicit MockReporter(net::URLRequestContext* request_context)
-    : ChromeFraudulentCertificateReporter(request_context) {}
+      : CertificateErrorReporter(request_context, GURL("http://example.com")) {}
 
+  void SendReport(ReportType type,
+                  const std::string& hostname,
+                  const net::SSLInfo& ssl_info) override {
+    EXPECT_EQ(type, REPORT_TYPE_PINNING_VIOLATION);
+    EXPECT_FALSE(hostname.empty());
+    EXPECT_TRUE(ssl_info.is_valid());
+    CertificateErrorReporter::SendReport(type, hostname, ssl_info);

[Ryan Sleevi, 2015/03/06 19:23:14]

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)

[estark, 2015/03/06 19:52:06]

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.

+  }
+
+ private:
   scoped_ptr<net::URLRequest> CreateURLRequest(
       net::URLRequestContext* context) override {
     return context->CreateRequest(GURL(std::string()),
@@ -126,13 +136,6 @@ class MockReporter : public ChromeFraudulentCertificateReporter {
                                   NULL,
                                   NULL);
   }
-
-  void SendReport(const std::string& hostname,
-                  const net::SSLInfo& ssl_info) override {
-    DCHECK(!hostname.empty());
-    DCHECK(ssl_info.is_valid());
-    ChromeFraudulentCertificateReporter::SendReport(hostname, ssl_info);
-  }
 };
 
 static void DoReportIsSent() {
@@ -151,7 +154,8 @@ static void DoReportIsNotSent() {
 
 static void DoMockReportIsSent() {
   net::TestURLRequestContext context;
-  MockReporter reporter(&context);
+  scoped_ptr<MockReporter> error_reporter(new MockReporter(&context));
+  ChromeFraudulentCertificateReporter reporter(error_reporter.Pass());
   SSLInfo info = GetGoodSSLInfo();
   reporter.SendReport("mail.google.com", info);
 }