Add retry information to certificate reports.

chrome/browser/safe_browsing/certificate_reporting_service_test_utils.cc

Index: chrome/browser/safe_browsing/certificate_reporting_service_test_utils.cc
diff --git a/chrome/browser/safe_browsing/certificate_reporting_service_test_utils.cc b/chrome/browser/safe_browsing/certificate_reporting_service_test_utils.cc
index d7e2f4b6dfc6d493f69bd4ab04254873ac7f233c..0a81fdd946f83b4c8923073e0c350935a7f8356e 100644
--- a/chrome/browser/safe_browsing/certificate_reporting_service_test_utils.cc
+++ b/chrome/browser/safe_browsing/certificate_reporting_service_test_utils.cc
@@ -49,36 +49,18 @@ std::string GetReportContents(net::URLRequest* request,
                 AEAD_ECDH_AES_128_CTR_HMAC_SHA256,
             encrypted_request.algorithm());
   std::string decrypted_report;
-  certificate_reporting::ErrorReporter::DecryptErrorReport(
-      server_private_key, encrypted_request, &decrypted_report);
+  EXPECT_TRUE(certificate_reporting::ErrorReporter::DecryptErrorReport(
+      server_private_key, encrypted_request, &decrypted_report));
   return decrypted_report;
 }
 
-// Checks that the serialized reports in |observed_reports| have the same
-// hostnames as |expected_hostnames|.
-void CompareHostnames(const std::set<std::string>& expected_hostnames,
-                      const std::set<std::string>& observed_reports,
-                      const std::string& comparison_type) {
-  std::set<std::string> observed_hostnames;
-  for (const std::string& serialized_report : observed_reports) {
-    certificate_reporting::ErrorReport report;
-    ASSERT_TRUE(report.InitializeFromString(serialized_report));
-    observed_hostnames.insert(report.hostname());
-  }
-  EXPECT_EQ(expected_hostnames, observed_hostnames)
-      << "Comparison failed for " << comparison_type << " reports.";
-}
-
 void WaitReports(
     certificate_reporting_test_utils::RequestObserver* observer,
     const certificate_reporting_test_utils::ReportExpectation& expectation) {
   observer->Wait(expectation.num_reports());
-  CompareHostnames(expectation.successful_reports,
-                   observer->successful_reports(), "successful");
-  CompareHostnames(expectation.failed_reports, observer->failed_reports(),
-                   "failed");
-  CompareHostnames(expectation.delayed_reports, observer->delayed_reports(),
-                   "delayed");
+  EXPECT_EQ(expectation.successful_reports, observer->successful_reports());
+  EXPECT_EQ(expectation.failed_reports, observer->failed_reports());
+  EXPECT_EQ(expectation.delayed_reports, observer->delayed_reports());
   observer->ClearObservedReports();
 }
 
@@ -86,6 +68,27 @@ void WaitReports(
 
 namespace certificate_reporting_test_utils {
 
+ObservedReport::ObservedReport(const std::string& hostname, bool is_retried)
+    : hostname(hostname), is_retried(is_retried) {}
+
+// static
+ObservedReport ObservedReport::Retried(const std::string& hostname) {
+  return ObservedReport(hostname, true);
+}
+
+// static
+ObservedReport ObservedReport::NotRetried(const std::string& hostname) {
+  return ObservedReport(hostname, false);
+}
+
+bool ObservedReport::operator<(const ObservedReport& other) const {

[estark, 2017/01/13 23:26:22]

Shouldn't these compare is_retried as well? (If that's not necessary for some
reason, maybe leave a comment in the .h)

[meacer, 2017/01/17 22:24:27]

This was keying on the hostname, leaving is_retried out. But your comment made
me realize what we want is actually a map instead of a set, so changed std::set
to unordered map. ObservedReport struct is no longer needed.

+  return hostname < other.hostname;
+}
+
+bool ObservedReport::operator==(const ObservedReport& other) const {
+  return hostname == other.hostname;
+}
+
 RequestObserver::RequestObserver()
     : num_events_to_wait_for_(0u), num_received_events_(0u) {}
 RequestObserver::~RequestObserver() {}
@@ -109,18 +112,18 @@ void RequestObserver::Wait(unsigned int num_events_to_wait_for) {
   }
 }
 
-void RequestObserver::OnRequest(const std::string& serialized_report,
+void RequestObserver::OnRequest(const ObservedReport& observed_report,
                                 ReportSendingResult report_type) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   switch (report_type) {
     case REPORTS_SUCCESSFUL:
-      successful_reports_.insert(serialized_report);
+      successful_reports_.insert(observed_report);
       break;
     case REPORTS_FAIL:
-      failed_reports_.insert(serialized_report);
+      failed_reports_.insert(observed_report);
       break;
     case REPORTS_DELAY:
-      delayed_reports_.insert(serialized_report);
+      delayed_reports_.insert(observed_report);
       break;
   }
 
@@ -138,17 +141,17 @@ void RequestObserver::OnRequest(const std::string& serialized_report,
   }
 }
 
-const std::set<std::string>& RequestObserver::successful_reports() const {
+const std::set<ObservedReport>& RequestObserver::successful_reports() const {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   return successful_reports_;
 }
 
-const std::set<std::string>& RequestObserver::failed_reports() const {
+const std::set<ObservedReport>& RequestObserver::failed_reports() const {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   return failed_reports_;
 }
 
-const std::set<std::string>& RequestObserver::delayed_reports() const {
+const std::set<ObservedReport>& RequestObserver::delayed_reports() const {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   return delayed_reports_;
 }
@@ -242,19 +245,19 @@ net::URLRequestJob* CertReportJobInterceptor::MaybeInterceptRequest(
     net::NetworkDelegate* network_delegate) const {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
 
-  const std::string uploaded_report =
+  const std::string serialized_report =
       GetReportContents(request, server_private_key_);
   content::BrowserThread::PostTask(
       content::BrowserThread::UI, FROM_HERE,
       base::Bind(&CertReportJobInterceptor::RequestCreated,
-                 weak_factory_.GetWeakPtr(), uploaded_report,
+                 weak_factory_.GetWeakPtr(), serialized_report,
                  expected_report_result_));
 
   if (expected_report_result_ == REPORTS_FAIL) {
     return new DelayableCertReportURLRequestJob(
         false, true, request, network_delegate,
         base::Bind(&CertReportJobInterceptor::RequestDestructed,
-                   base::Unretained(this), uploaded_report,
+                   base::Unretained(this), serialized_report,
                    expected_report_result_));
 
   } else if (expected_report_result_ == REPORTS_DELAY) {
@@ -263,7 +266,7 @@ net::URLRequestJob* CertReportJobInterceptor::MaybeInterceptRequest(
         new DelayableCertReportURLRequestJob(
             true, false, request, network_delegate,
             base::Bind(&CertReportJobInterceptor::RequestDestructed,
-                       base::Unretained(this), uploaded_report,
+                       base::Unretained(this), serialized_report,
                        expected_report_result_));
     delayed_request_ = job->GetWeakPtr();
     return job;
@@ -272,7 +275,7 @@ net::URLRequestJob* CertReportJobInterceptor::MaybeInterceptRequest(
   return new DelayableCertReportURLRequestJob(
       false, false, request, network_delegate,
       base::Bind(&CertReportJobInterceptor::RequestDestructed,
-                 base::Unretained(this), uploaded_report,
+                 base::Unretained(this), serialized_report,
                  expected_report_result_));
 }
 
@@ -317,16 +320,23 @@ void CertReportJobInterceptor::ResumeOnIOThread() {
 }
 
 void CertReportJobInterceptor::RequestCreated(
-    const std::string& uploaded_report,
+    const std::string& serialized_report,
     ReportSendingResult expected_report_result) const {
-  request_created_observer_.OnRequest(uploaded_report, expected_report_result);
+  certificate_reporting::ErrorReport error_report;
+  EXPECT_TRUE(error_report.InitializeFromString(serialized_report));
+  request_created_observer_.OnRequest(
+      ObservedReport(error_report.hostname(), error_report.is_retry_upload()),
+      expected_report_result);
 }
 
 void CertReportJobInterceptor::RequestDestructed(
-    const std::string& uploaded_report,
+    const std::string& serialized_report,
     ReportSendingResult expected_report_result) const {
-  request_destroyed_observer_.OnRequest(uploaded_report,
-                                        expected_report_result);
+  certificate_reporting::ErrorReport error_report;
+  EXPECT_TRUE(error_report.InitializeFromString(serialized_report));
+  request_destroyed_observer_.OnRequest(
+      ObservedReport(error_report.hostname(), error_report.is_retry_upload()),
+      expected_report_result);
 }
 
 ReportExpectation::ReportExpectation() {}
@@ -337,7 +347,7 @@ ReportExpectation::~ReportExpectation() {}
 
 // static
 ReportExpectation ReportExpectation::Successful(
-    const std::set<std::string>& reports) {
+    const std::set<ObservedReport>& reports) {
   ReportExpectation expectation;
   expectation.successful_reports = reports;
   return expectation;
@@ -345,7 +355,7 @@ ReportExpectation ReportExpectation::Successful(
 
 // static
 ReportExpectation ReportExpectation::Failed(
-    const std::set<std::string>& reports) {
+    const std::set<ObservedReport>& reports) {
   ReportExpectation expectation;
   expectation.failed_reports = reports;
   return expectation;
@@ -353,7 +363,7 @@ ReportExpectation ReportExpectation::Failed(
 
 // static
 ReportExpectation ReportExpectation::Delayed(
-    const std::set<std::string>& reports) {
+    const std::set<ObservedReport>& reports) {
   ReportExpectation expectation;
   expectation.delayed_reports = reports;
   return expectation;