Wire up CertificateReportingService to handle report uploads

chrome/browser/safe_browsing/certificate_reporting_service.cc

Index: chrome/browser/safe_browsing/certificate_reporting_service.cc
diff --git a/chrome/browser/safe_browsing/certificate_reporting_service.cc b/chrome/browser/safe_browsing/certificate_reporting_service.cc
index 0a84e0e1ca0c623e4ede060c8c97ce627306754c..275b577d3800b7792a9862ada9e67e6f64962514 100644
--- a/chrome/browser/safe_browsing/certificate_reporting_service.cc
+++ b/chrome/browser/safe_browsing/certificate_reporting_service.cc
@@ -3,9 +3,14 @@
 // found in the LICENSE file.
 
 #include "base/bind_helpers.h"
+#include "base/metrics/histogram_macros.h"
+#include "base/metrics/sparse_histogram.h"
 #include "base/time/clock.h"
-#include "base/time/default_clock.h"
+#include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/safe_browsing/certificate_reporting_service.h"
+#include "chrome/browser/safe_browsing/safe_browsing_service.h"
+#include "components/prefs/pref_service.h"
+#include "components/safe_browsing_db/safe_browsing_prefs.h"
 #include "content/public/browser/browser_thread.h"
 
 namespace {
@@ -23,6 +28,49 @@ bool ReportCompareFunc(const CertificateReportingService::Report& item1,
   return item1.creation_time > item2.creation_time;
 }
 
+// Records an UMA histogram of the net errors when certificate reports
+// fail to send.
+void RecordUMAOnFailure(int net_error) {
+  UMA_HISTOGRAM_SPARSE_SLOWLY("SSL.CertificateErrorReportFailure", -net_error);
+}
+
+// Observes SafeBrowsing preferences and notifies CertificateReportingService
+// when preferences change. There is one instance of this class per
+// CertificateReportingService and each instance is owned by the
+// CertificateReportingService it notifies.
+class SafeBrowsingPreferenceObserver
+    : public CertificateReportingService::PreferenceObserver {

[estark, 2016/12/16 23:39:04]

I'm wondering if these abstractions are needed (the PreferenceObserver interface
and the SBPreferenceObserver implementation). Could you instead just give the
CertificateReportingService a safe_browsing_state_subscription_ member that
calls a CertificateReportingService method as its callback?

[meacer, 2016/12/19 23:16:45]

Good point, removed. This used to be created by ChromeContentBrowserClient but
since there is no need to do that anymore, we don't need this abstraction.

+ public:
+  SafeBrowsingPreferenceObserver(
+      const PrefService& prefs,
+      safe_browsing::SafeBrowsingService* safe_browsing_service,
+      CertificateReportingService* certificate_reporting_service)
+      : safe_browsing_service_(safe_browsing_service),
+        prefs_(prefs),
+        certificate_reporting_service_(certificate_reporting_service),
+        safe_browsing_state_subscription_(
+            safe_browsing_service->RegisterStateCallback(
+                base::Bind(&SafeBrowsingPreferenceObserver::OnPreferenceChanged,
+                           base::Unretained(this)))) {}
+
+  ~SafeBrowsingPreferenceObserver() override {}
+
+  // CertificateReportingService::PreferenceObserver implementation:
+  void OnPreferenceChanged() override {
+    const bool enabled = safe_browsing_service_ &&
+                         safe_browsing_service_->enabled_by_prefs() &&
+                         safe_browsing::IsExtendedReportingEnabled(prefs_);
+    certificate_reporting_service_->SetEnabled(enabled);
+  }
+
+ private:
+  const safe_browsing::SafeBrowsingService* safe_browsing_service_;
+  const PrefService& prefs_;
+  CertificateReportingService* certificate_reporting_service_;
+  std::unique_ptr<safe_browsing::SafeBrowsingService::StateSubscription>
+      safe_browsing_state_subscription_;
+};
+
 }  // namespace
 
 CertificateReportingService::BoundedReportList::BoundedReportList(
@@ -128,6 +176,7 @@ void CertificateReportingService::Reporter::ErrorCallback(int report_id,
                                                           const GURL& url,
                                                           int error) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  RecordUMAOnFailure(error);
   if (retries_enabled_) {
     auto it = inflight_reports_.find(report_id);
     DCHECK(it != inflight_reports_.end());
@@ -142,26 +191,37 @@ void CertificateReportingService::Reporter::SuccessCallback(int report_id) {
 }
 
 CertificateReportingService::CertificateReportingService(
+    safe_browsing::SafeBrowsingService* safe_browsing_service,
     scoped_refptr<net::URLRequestContextGetter> url_request_context_getter,
+    Profile* profile,
     uint8_t server_public_key[/* 32 */],
     uint32_t server_public_key_version,
     size_t max_queued_report_count,
     base::TimeDelta max_report_age,
-    std::unique_ptr<base::Clock> clock)
+    base::Clock* clock)
     : enabled_(true),
       url_request_context_(nullptr),
       max_queued_report_count_(max_queued_report_count),
       max_report_age_(max_report_age),
-      clock_(std::move(clock)),
-      made_send_attempt_(false),
+      clock_(clock),
       server_public_key_(server_public_key),
       server_public_key_version_(server_public_key_version) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  DCHECK(clock_);
+  // Observe changes in SafeBrowsing preferences.
+  preference_observer_.reset(new SafeBrowsingPreferenceObserver(
+      *profile->GetPrefs(), safe_browsing_service, this));
+
+  // Subscribe to SafeBrowsing shutdown notifications.
+  safe_browsing_service_shutdown_subscription_ =
+      safe_browsing_service->RegisterShutdownCallback(base::Bind(
+          &CertificateReportingService::Shutdown, base::Unretained(this)));
+
   content::BrowserThread::PostTask(
       content::BrowserThread::IO, FROM_HERE,
       base::Bind(&CertificateReportingService::InitializeOnIOThread,
                  base::Unretained(this), enabled_, url_request_context_getter,
-                 max_queued_report_count_, max_report_age_, clock_.get(),
+                 max_queued_report_count_, max_report_age_, clock_,
                  server_public_key_, server_public_key_version_));
 }
 
@@ -178,7 +238,6 @@ void CertificateReportingService::Shutdown() {
 
 void CertificateReportingService::Send(const std::string& serialized_report) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  made_send_attempt_ = true;
   if (!reporter_) {
     return;
   }
@@ -190,7 +249,6 @@ void CertificateReportingService::Send(const std::string& serialized_report) {
 
 void CertificateReportingService::SendPending() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  made_send_attempt_ = true;
   if (!reporter_) {
     return;
   }
@@ -227,27 +285,6 @@ CertificateReportingService::GetReporterForTesting() const {
   return reporter_.get();
 }
 
-void CertificateReportingService::SetMaxQueuedReportCountForTesting(
-    size_t count) {
-  DCHECK(!made_send_attempt_);
-  max_queued_report_count_ = count;
-  Reset();
-}
-
-void CertificateReportingService::SetClockForTesting(
-    std::unique_ptr<base::Clock> clock) {
-  DCHECK(!made_send_attempt_);
-  clock_ = std::move(clock);
-  Reset();
-}
-
-void CertificateReportingService::SetMaxReportAgeForTesting(
-    base::TimeDelta max_report_age) {
-  DCHECK(!made_send_attempt_);
-  max_report_age_ = max_report_age;
-  Reset();
-}
-
 // static
 GURL CertificateReportingService::GetReportingURLForTesting() {
   return GURL(kExtendedReportingUploadUrl);
@@ -258,7 +295,7 @@ void CertificateReportingService::Reset() {
       content::BrowserThread::IO, FROM_HERE,
       base::Bind(&CertificateReportingService::ResetOnIOThread,
                  base::Unretained(this), enabled_, url_request_context_,
-                 max_queued_report_count_, max_report_age_, clock_.get(),
+                 max_queued_report_count_, max_report_age_, clock_,
                  server_public_key_, server_public_key_version_));
 }