Adding the Finch code for the certificate error reporter experiment

chrome/browser/ssl/ssl_blocking_page.cc

Index: chrome/browser/ssl/ssl_blocking_page.cc
diff --git a/chrome/browser/ssl/ssl_blocking_page.cc b/chrome/browser/ssl/ssl_blocking_page.cc
index e6083f8768ecf1a12308fb8d693f8b38bc584562..da782e256f6afd68c9d864286ae391f219752935 100644
--- a/chrome/browser/ssl/ssl_blocking_page.cc
+++ b/chrome/browser/ssl/ssl_blocking_page.cc
@@ -15,6 +15,7 @@
 #include "base/metrics/histogram.h"
 #include "base/prefs/pref_service.h"
 #include "base/process/launch.h"
+#include "base/rand_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_piece.h"
 #include "base/strings/string_util.h"
@@ -35,6 +36,7 @@
 #include "chrome/grit/chromium_strings.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/google/core/browser/google_util.h"
+#include "components/variations/variations_associated_data.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/cert_store.h"
 #include "content/public/browser/interstitial_page.h"
@@ -100,6 +102,32 @@ enum SSLExpirationAndDecision {
 // Rappor prefix
 const char kSSLRapporPrefix[] = "ssl";
 
+// Check whether to report certificate verification errors to Google
+bool ReportCertificateErrors(bool in_incognito) {
+  // Check Finch parameters
+  const std::string show =
+      base::FieldTrialList::FindFullName("ReportCertificateErrors");
+  if (show.compare("ShowAndPossiblySend") == 0) {
+    const std::string param = variations::GetVariationParamValue(
+        "ReportCertificateErrors", "possibly_send");
+    if (param.compare("") != 0) {
+      double possiblySend;
+      LOG(ERROR) << "RandValue: " << base::RandDouble();

[estark, 2015/04/02 18:33:34]

any reason to leave this in?

[fahl, 2015/04/02 19:24:58]

Done.

+      if (base::StringToDouble(param, &possiblySend))

[estark, 2015/04/02 18:33:34]

This logic looks funny to me... |ReportCertificateErrors(false)| will return
true if |StringToDouble| returns false. Also I think this function is more
complicated than it needs to be. We call |ShowCertificateReporterCheckbox|
immediately before calling |ReportCertificateErrors| so I don't think we need to
check the group name and incognito again.

Maybe something like this would be cleaner and more correct:

// Returns whether the user should report a given certificate error, assuming
that they are opted in to extended reporting and are being shown the checkbox on
the interstitial.
bool ShouldReportCertificateError(bool in_incognito) {
  DCHECK(ShouldShowCertificateReporterCheckbox(in_incognito));
  const std::string param = ...
  if (param.compare("") != 0) {
    double possiblySend;
    if (base::StringToDouble(param, &possiblySend))
      return base::RandDouble() <= possiblySend;
  }
  return false;
}

And to be on the safe side we should perhaps add a regression test that sets a
parameter to an invalid value ("abc"?) and checks that the report doesn't get
sent in that case.

[fahl, 2015/04/02 19:24:58]

Done.

+        return base::RandDouble() <= possiblySend;
+    }
+  }
+  return !in_incognito;
+}
+
+// Check whether to show the certificate reporter checkbox
+bool ShowCertificateReporterCheckbox(bool in_incognito) {
+  // Check Finch parameters
+  return base::FieldTrialList::FindFullName("ReportCertificateErrors")
+                 .compare("ShowAndPossiblySend") == 0 &&
+         !in_incognito;
+}
+
 void RecordSSLExpirationPageEventState(bool expired_but_previously_allowed,
                                        bool proceed,
                                        bool overridable) {
@@ -457,9 +485,8 @@ void SSLBlockingPage::PopulateExtendedReportingOption(
     base::DictionaryValue* load_time_data) {
   // Only show the checkbox if not off-the-record and if the
   // command-line option is set.
-  const bool show = !web_contents()->GetBrowserContext()->IsOffTheRecord() &&
-                    base::CommandLine::ForCurrentProcess()->HasSwitch(
-                        switches::kEnableInvalidCertCollection);
+  const bool show = ShowCertificateReporterCheckbox(
+      web_contents()->GetBrowserContext()->IsOffTheRecord());
 
   load_time_data->SetBoolean(interstitials::kDisplayCheckBox, show);
   if (!show)
@@ -645,11 +672,9 @@ void SSLBlockingPage::FinishCertCollection() {
   base::ScopedClosureRunner scoped_callback(
       certificate_report_callback_for_testing_);
 
-  if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kEnableInvalidCertCollection) ||
-      web_contents()->GetBrowserContext()->IsOffTheRecord()) {
+  if (!ShowCertificateReporterCheckbox(
+          web_contents()->GetBrowserContext()->IsOffTheRecord()))
     return;
-  }
 
   const bool enabled =
       IsPrefEnabled(prefs::kSafeBrowsingExtendedReportingEnabled);
@@ -660,11 +685,13 @@ void SSLBlockingPage::FinishCertCollection() {
   metrics_helper()->RecordUserInteraction(
       SecurityInterstitialMetricsHelper::EXTENDED_REPORTING_IS_ENABLED);
 
-  if (certificate_report_callback_for_testing_.is_null())
-    scoped_callback.Reset(base::Bind(&base::DoNothing));
-
-  safe_browsing_ui_manager_->ReportInvalidCertificateChain(
-      request_url().host(), ssl_info_, scoped_callback.Release());
+  if (ReportCertificateErrors(
+          web_contents()->GetBrowserContext()->IsOffTheRecord())) {
+    if (certificate_report_callback_for_testing_.is_null())
+      scoped_callback.Reset(base::Bind(&base::DoNothing));
+    safe_browsing_ui_manager_->ReportInvalidCertificateChain(
+        request_url().host(), ssl_info_, scoped_callback.Release());
+  }
 }
 
 // static