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..c861c34ea48df0960350c11269057d193941aa6e 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"
@@ -518,12 +520,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.
@@ -1127,7 +1123,13 @@ IN_PROC_BROWSER_TEST_F(SSLUITest, MAYBE_TestDisplaysInsecureContent) {
 // the interstitial, the FraudulentCertificateReporter sees a request to
 // send a report.
 IN_PROC_BROWSER_TEST_F(SSLUITestWithExtendedReporting,
-                       TestBrokenHTTPSProceedWithReporting) {
+                       TestBrokenHTTPSProceedWithShowCheckboxAndReporting) {
+  base::FieldTrialList::CreateFieldTrial("ReportCertificateErrors",
+                                         "ShowAndPossiblySend");
+  std::map<std::string, std::string> params;
+  params["possibly_send"] = "1.0";
+  variations::AssociateVariationParams("ReportCertificateErrors",
+                                       "ShowAndPossiblySend", params);
   TestBrokenHTTPSReporting(CertificateReporting::EXTENDED_REPORTING_OPT_IN,
                            CertificateReporting::SSL_INTERSTITIAL_PROCEED,
                            CertificateReporting::CERT_REPORT_EXPECTED,
@@ -1138,7 +1140,13 @@ IN_PROC_BROWSER_TEST_F(SSLUITestWithExtendedReporting,
 // not proceed through the interstitial), the
 // FraudulentCertificateReporter sees a request to send a report.
 IN_PROC_BROWSER_TEST_F(SSLUITestWithExtendedReporting,
-                       TestBrokenHTTPSGoBackWithReporting) {
+                       TestBrokenHTTPSGoBackWithShowCheckboxAndReporting) {
+  base::FieldTrialList::CreateFieldTrial("ReportCertificateErrors",
+                                         "ShowAndPossiblySend");
+  std::map<std::string, std::string> params;
+  params["possibly_send"] = "1.0";
+  variations::AssociateVariationParams("ReportCertificateErrors",
+                                       "ShowAndPossiblySend", params);
   TestBrokenHTTPSReporting(
       CertificateReporting::EXTENDED_REPORTING_OPT_IN,
       CertificateReporting::SSL_INTERSTITIAL_DO_NOT_PROCEED,
@@ -1149,7 +1157,13 @@ IN_PROC_BROWSER_TEST_F(SSLUITestWithExtendedReporting,
 // through the interstitial, the FraudulentCertificateReporter does not
 // see a request to send a report.
 IN_PROC_BROWSER_TEST_F(SSLUITestWithExtendedReporting,
-                       TestBrokenHTTPSProceedWithNoReporting) {
+                       TestBrokenHTTPSProceedWithShowCheckboxAndNoReporting) {
+  base::FieldTrialList::CreateFieldTrial("ReportCertificateErrors",
+                                         "ShowAndPossiblySend");
+  std::map<std::string, std::string> params;
+  params["possibly_send"] = "-1.0";

[estark, 2015/04/02 01:19:51]

I think this should be -1.0, not 1.0. Same for the test below, which is why I
think you can move this setup code into SSLUITestWithExtendedReporting::SetUp
instead of repeating it in each test.

Why should it be 1.0? This test and the one below it are testing that a report
never gets sent if the user does not check the checkbox. Suppose we had a bug
where we were sending reports even if the checkbox wasn't checked; as is, this
test wouldn't catch that bug because the Finch param is set to -1.0 (never send
reports).

So I think these two tests should have params of 1.0, and then we should have an
additional test that tests that reports never get sent when the param is -1.0,
even if the user is opted in. This additional test would look like this:

//
// set param to -1.0 so reports never get sent
//
TestBrokenHTTPSReporting(CertificateReporting::EXTENDED_REPORTING_OPT_IN,
SSL_INTERSTITIAL_PROCEED, CERT_REPORT_NOT_EXPECTED, browser());

[fahl, 2015/04/02 17:45:29]

Done.

+  variations::AssociateVariationParams("ReportCertificateErrors",
+                                       "ShowAndPossiblySend", params);
   TestBrokenHTTPSReporting(
       CertificateReporting::EXTENDED_REPORTING_DO_NOT_OPT_IN,
       CertificateReporting::SSL_INTERSTITIAL_PROCEED,
@@ -1160,13 +1174,33 @@ IN_PROC_BROWSER_TEST_F(SSLUITestWithExtendedReporting,
 // through the interstitial, the FraudulentCertificateReporter does not
 // see a request to send a report.
 IN_PROC_BROWSER_TEST_F(SSLUITestWithExtendedReporting,
-                       TestBrokenHTTPSGoBackWithNoReporting) {
+                       TestBrokenHTTPSGoBackWithShowCheckboxAndNoReporting) {
+  base::FieldTrialList::CreateFieldTrial("ReportCertificateErrors",
+                                         "ShowAndPossiblySend");
+  std::map<std::string, std::string> params;
+  params["possibly_send"] = "-1.0";

[estark, 2015/04/02 01:19:51]

should be 1.0 as above

[fahl, 2015/04/02 17:45:29]

Done.

+  variations::AssociateVariationParams("ReportCertificateErrors",
+                                       "ShowAndPossiblySend", params);
   TestBrokenHTTPSReporting(
       CertificateReporting::EXTENDED_REPORTING_DO_NOT_OPT_IN,
       CertificateReporting::SSL_INTERSTITIAL_DO_NOT_PROCEED,
       CertificateReporting::CERT_REPORT_NOT_EXPECTED, browser());
 }
 
+// Test that when the checkbox is not shown and the user proceeds
+// through the interstitial, the FraudulentCertificateReporter does not
+// see a request to send a report.
+IN_PROC_BROWSER_TEST_F(
+    SSLUITestWithExtendedReporting,
+    TestBrokenHTTPSProceedWithNotShowCheckboxAndNoReporting) {
+  base::FieldTrialList::CreateFieldTrial("ReportCertificateErrors",
+                                         "DontShowAndDontSend");
+  TestBrokenHTTPSReporting(
+      CertificateReporting::EXTENDED_REPORTING_DO_NOT_OPT_IN,

[estark, 2015/04/02 01:19:51]

I think this should be OPT_IN. i.e. we want to test that even if the user is
opted in (because they were previously in ShowAndPossiblySend, for example), the
report does not get sent.

[fahl, 2015/04/02 17:45:28]

Done.

+      CertificateReporting::SSL_INTERSTITIAL_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