Adding the Finch code for the certificate error reporter experiment

chrome/browser/ssl/ssl_browser_tests.cc

Index: chrome/browser/ssl/ssl_browser_tests.cc
diff --git a/chrome/browser/ssl/ssl_browser_tests.cc b/chrome/browser/ssl/ssl_browser_tests.cc
index ba69fe78eb8b47aee9d8ebc1bb8ecb8fff4a3cc8..81244f59364da6b5bc59f53e0781453fde49de8a 100644
--- a/chrome/browser/ssl/ssl_browser_tests.cc
+++ b/chrome/browser/ssl/ssl_browser_tests.cc
@@ -6,6 +6,7 @@
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/command_line.h"
+#include "base/metrics/field_trial.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
@@ -31,6 +32,7 @@
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
+#include "components/variations/variations_associated_data.h"
 #include "components/web_modal/web_contents_modal_dialog_manager.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/interstitial_page.h"
@@ -473,6 +475,14 @@ class SSLUITest : public InProcessBrowserTest {
     }
   }
 
+  // Helper function to set the Finch parameter for HTTPS error reporting
+  std::map<std::string, std::string> SetFinchPossiblySendParam(

[estark, 2015/04/02 19:55:56]

As a matter of style, I don't think we're generally supposed to pass around
objects by value (both the return value and the |value| argument here). Why not
do something like:

void SetCertReportingFinchConfig(const std::string& group_name, const
std::string& param_value) {
  base::FieldTrialList::CreateFieldTrial(...);
  // initialize |params| here
  variations::AssociateVariationParams(...);
}

[fahl, 2015/04/02 21:38:04]

Done.

+      const std::string value) {
+    std::map<std::string, std::string> params;
+    params["possibly_send"] = value;
+    return params;
+  }
+
   net::SpawnedTestServer https_server_;
   net::SpawnedTestServer https_server_expired_;
   net::SpawnedTestServer https_server_mismatched_;
@@ -518,12 +528,6 @@ class SSLUITestIgnoreLocalhostCertErrors : public SSLUITest {
 class SSLUITestWithExtendedReporting : public SSLUITest {
  public:
   SSLUITestWithExtendedReporting() : SSLUITest() {}
-
-  void SetUpCommandLine(base::CommandLine* command_line) override {
-    // Enable a checkbox on SSL interstitials that allows users to opt
-    // in to reporting invalid certificate chains.
-    command_line->AppendSwitch(switches::kEnableInvalidCertCollection);
-  }
 };
 
 // Visits a regular page over http.
@@ -1123,73 +1127,141 @@ IN_PROC_BROWSER_TEST_F(SSLUITest, MAYBE_TestDisplaysInsecureContent) {
                           AuthState::DISPLAYED_INSECURE_CONTENT);
 }
 
-// Test that when the checkbox is checked and the user proceeds through
-// the interstitial, the FraudulentCertificateReporter sees a request to
-// send a report.
-IN_PROC_BROWSER_TEST_F(SSLUITestWithExtendedReporting,
-                       TestBrokenHTTPSProceedWithReporting) {
+// User proceeds, checkbox is shown and checked, Finch parameter is set
+// -> we expect a report.
+IN_PROC_BROWSER_TEST_F(
+    SSLUITestWithExtendedReporting,
+    TestBrokenHTTPSProceedWithShowYesCheckYesParamYesReportYes) {
+  base::FieldTrialList::CreateFieldTrial("ReportCertificateErrors",

[estark, 2015/04/02 19:55:56]

nit: the "ReportCertificateErrors" and group name strings
("ShowAndPossiblySend", "DontShowAndDontSend") should be constants. They should
probably be constants in ssl_blocking_page.cc too, come to think of it... so
maybe declare the constants in ssl_blocking_page.h and #include them to use
here?

[fahl, 2015/04/02 21:38:04]

Done.

+                                         "ShowAndPossiblySend");
+  variations::AssociateVariationParams("ReportCertificateErrors",
+                                       "ShowAndPossiblySend",
+                                       SetFinchPossiblySendParam("1.0"));
   TestBrokenHTTPSReporting(CertificateReporting::EXTENDED_REPORTING_OPT_IN,
                            CertificateReporting::SSL_INTERSTITIAL_PROCEED,
                            CertificateReporting::CERT_REPORT_EXPECTED,
                            browser());
 }
 
-// Test that when the checkbox is checked and the user goes back (does
-// not proceed through the interstitial), the
-// FraudulentCertificateReporter sees a request to send a report.
-IN_PROC_BROWSER_TEST_F(SSLUITestWithExtendedReporting,
-                       TestBrokenHTTPSGoBackWithReporting) {
+// User goes back, checkbox is shown and checked, Finch parameter is set
+// -> we expect a report.
+IN_PROC_BROWSER_TEST_F(
+    SSLUITestWithExtendedReporting,
+    TestBrokenHTTPSGoBackWithShowYesCheckYesParamYesReportYes) {
+  base::FieldTrialList::CreateFieldTrial("ReportCertificateErrors",
+                                         "ShowAndPossiblySend");
+  variations::AssociateVariationParams("ReportCertificateErrors",
+                                       "ShowAndPossiblySend",
+                                       SetFinchPossiblySendParam("1.0"));
   TestBrokenHTTPSReporting(
       CertificateReporting::EXTENDED_REPORTING_OPT_IN,
       CertificateReporting::SSL_INTERSTITIAL_DO_NOT_PROCEED,
       CertificateReporting::CERT_REPORT_EXPECTED, browser());
 }
 
-// Test that when the checkbox is not checked and the user proceeds
-// through the interstitial, the FraudulentCertificateReporter does not
-// see a request to send a report.
-IN_PROC_BROWSER_TEST_F(SSLUITestWithExtendedReporting,
-                       TestBrokenHTTPSProceedWithNoReporting) {
+// User proceeds, checkbox is shown but unchecked, Finch parameter is set
+// -> we expect no report.
+IN_PROC_BROWSER_TEST_F(
+    SSLUITestWithExtendedReporting,
+    TestBrokenHTTPSProceedWithShowYesCheckNoParamYesReportNo) {
+  base::FieldTrialList::CreateFieldTrial("ReportCertificateErrors",
+                                         "ShowAndPossiblySend");
+  variations::AssociateVariationParams("ReportCertificateErrors",
+                                       "ShowAndPossiblySend",
+                                       SetFinchPossiblySendParam("1.0"));
   TestBrokenHTTPSReporting(
       CertificateReporting::EXTENDED_REPORTING_DO_NOT_OPT_IN,
       CertificateReporting::SSL_INTERSTITIAL_PROCEED,
       CertificateReporting::CERT_REPORT_NOT_EXPECTED, browser());
 }
 
-// Test that when the checkbox is not checked and the user does not proceed
-// through the interstitial, the FraudulentCertificateReporter does not
-// see a request to send a report.
+// User goes back, checkbox is shown but unchecked, Finch parameter is set
+// -> we expect no report.
 IN_PROC_BROWSER_TEST_F(SSLUITestWithExtendedReporting,
-                       TestBrokenHTTPSGoBackWithNoReporting) {
+                       TestBrokenHTTPSGoBackShowYesCheckNoParamYesReportNo) {
+  base::FieldTrialList::CreateFieldTrial("ReportCertificateErrors",
+                                         "ShowAndPossiblySend");
+  variations::AssociateVariationParams("ReportCertificateErrors",
+                                       "ShowAndPossiblySend",
+                                       SetFinchPossiblySendParam("1.0"));
   TestBrokenHTTPSReporting(
       CertificateReporting::EXTENDED_REPORTING_DO_NOT_OPT_IN,
       CertificateReporting::SSL_INTERSTITIAL_DO_NOT_PROCEED,
       CertificateReporting::CERT_REPORT_NOT_EXPECTED, browser());
 }
 
-// Test that when the command-line switch for reporting invalid cert
-// chains is not enabled, reports don't get sent, even if the opt-in
-// preference is set. (i.e. if a user enables invalid cert collection in
-// chrome://flags, checks the box on an interstitial, and then disables
-// the flag in chrome://flags, reports shouldn't be sent on the next
-// interstitial).
-IN_PROC_BROWSER_TEST_F(SSLUITest, TestBrokenHTTPSNoReportingWithoutSwitch) {
+// User proceeds, checkbox is shown and checked, Finch parameter is not
+// set -> we expect no report.
+IN_PROC_BROWSER_TEST_F(SSLUITestWithExtendedReporting,
+                       TestBrokenHTTPSProceedShowYesCheckYesParamNoReportNo) {
+  base::FieldTrialList::CreateFieldTrial("ReportCertificateErrors",
+                                         "ShowAndPossiblySend");
+  variations::AssociateVariationParams("ReportCertificateErrors",
+                                       "ShowAndPossiblySend",
+                                       SetFinchPossiblySendParam("-1.0"));
   TestBrokenHTTPSReporting(CertificateReporting::EXTENDED_REPORTING_OPT_IN,
                            CertificateReporting::SSL_INTERSTITIAL_PROCEED,
                            CertificateReporting::CERT_REPORT_NOT_EXPECTED,
                            browser());
 }
 
-// Test that reports don't get sent in incognito mode even if the opt-in
-// preference is set and the command-line switch is enabled.
+// User goes back, checkbox is shown and checked, Finch parameter is not set
+// -> we expect no report.
 IN_PROC_BROWSER_TEST_F(SSLUITestWithExtendedReporting,
-                       TestBrokenHTTPSNoReportingInIncognito) {
+                       TestBrokenHTTPSGoBackShowYesCheckYesParamNoReportNo) {
+  base::FieldTrialList::CreateFieldTrial("ReportCertificateErrors",
+                                         "ShowAndPossiblySend");
+  variations::AssociateVariationParams("ReportCertificateErrors",
+                                       "ShowAndPossiblySend",
+                                       SetFinchPossiblySendParam("-1.0"));
+  TestBrokenHTTPSReporting(
+      CertificateReporting::EXTENDED_REPORTING_OPT_IN,
+      CertificateReporting::SSL_INTERSTITIAL_DO_NOT_PROCEED,
+      CertificateReporting::CERT_REPORT_NOT_EXPECTED, browser());
+}
+
+// User proceeds, checkbox is not shown but checked -> we expect no report
+IN_PROC_BROWSER_TEST_F(SSLUITestWithExtendedReporting,
+                       TestBrokenHTTPSProceedShowNoCheckYesReportNo) {
+  base::FieldTrialList::CreateFieldTrial("ReportCertificateErrors",
+                                         "DontShowAndDontSend");
+  TestBrokenHTTPSReporting(CertificateReporting::EXTENDED_REPORTING_OPT_IN,
+                           CertificateReporting::SSL_INTERSTITIAL_PROCEED,
+                           CertificateReporting::CERT_REPORT_NOT_EXPECTED,
+                           browser());
+}
+
+// Browser is incognito, user proceeds, checkbox is shown and checked, Finch
+// parameter is set -> we expect no report
+IN_PROC_BROWSER_TEST_F(SSLUITestWithExtendedReporting,
+                       TestBrokenHTTPSInIncognitoReportNo) {
+  base::FieldTrialList::CreateFieldTrial("ReportCertificateErrors",
+                                         "ShowAndPossiblySend");
+  variations::AssociateVariationParams("ReportCertificateErrors",
+                                       "ShowAndPossiblySend",
+                                       SetFinchPossiblySendParam("1.0"));
   TestBrokenHTTPSReporting(CertificateReporting::EXTENDED_REPORTING_OPT_IN,
                            CertificateReporting::SSL_INTERSTITIAL_PROCEED,
                            CertificateReporting::CERT_REPORT_NOT_EXPECTED,
                            CreateIncognitoBrowser());
 }
 
+// User proceeds, checkbox is shown and checked, Finch parameter is invalid
+// -> we expect no report.
+IN_PROC_BROWSER_TEST_F(
+    SSLUITestWithExtendedReporting,
+    TestBrokenHTTPSProceedWithShowYesCheckYesParamInvalidReportNo) {
+  base::FieldTrialList::CreateFieldTrial("ReportCertificateErrors",
+                                         "ShowAndPossiblySend");
+  variations::AssociateVariationParams("ReportCertificateErrors",
+                                       "ShowAndPossiblySend",
+                                       SetFinchPossiblySendParam("abcdef"));
+  TestBrokenHTTPSReporting(CertificateReporting::EXTENDED_REPORTING_OPT_IN,
+                           CertificateReporting::SSL_INTERSTITIAL_PROCEED,
+                           CertificateReporting::CERT_REPORT_NOT_EXPECTED,
+                           browser());
+}
+
 // Visits a page that runs insecure content and tries to suppress the insecure
 // content warnings by randomizing location.hash.
 // Based on http://crbug.com/8706