Adding the Finch code for the certificate error reporter experiment

chrome/browser/ssl/ssl_blocking_page.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ssl/ssl_blocking_page.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/build_time.h"
 #include "base/callback_helpers.h"
@@ skipping 62 matching lines @@
 
 using base::ASCIIToUTF16;
 using base::TimeTicks;
 using content::InterstitialPage;
 using content::InterstitialPageDelegate;
 using content::NavigationController;
 using content::NavigationEntry;
 
 namespace {
 
+bool SendFraudulentCertificateChain() {

[estark, 2015/03/27 01:22:02]

I think we should name the experiment |CertificateErrorReporter| or
|ReportCertificateErrors| or |ReportSSLCertificateErrors| (and then this
function would be |ShouldReportCertificateErrors| or something), because
"fraudulent" seems to refer to pinning violations in the code. For example
ChromeFraudulentCertificateReporter specifically reports pinning violations on
Google properties, whereas CertificateErrorReporter is the more general
certificate-reporting class that is used to report SSL errors here.

[estark, 2015/03/27 01:22:02]

Maybe put this down with the other helper functions (starting at line 108)? So
that constants and helper functions are grouped together.

[fahl, 2015/03/27 01:28:24]

Sure, I just put it there so no one forgets to remove the code one the
experiment expired. I found an experiment that expired in August last year and
still is in the code... :)

[fahl, 2015/03/27 01:59:08]

Done.

+  return base::FieldTrialList::FindFullName("FraudulentCertificateCollector") ==
+         "FCC";
+}
+
 // URL for help page.
 const char kHelpURL[] = "https://support.google.com/chrome/answer/4454607";
 
 // Constants for the Experience Sampling instrumentation.
 const char kEventNameBase[] = "ssl_interstitial_";
 const char kEventNotOverridable[] = "notoverridable_";
 const char kEventOverridable[] = "overridable_";
 
 // Events for UMA. Do not reorder or change!
 enum SSLExpirationAndDecision {
@@ skipping 560 matching lines @@
 
   const bool enabled =
       IsPrefEnabled(prefs::kSafeBrowsingExtendedReportingEnabled);
 
   if (!enabled)
     return;
 
   metrics_helper()->RecordUserInteraction(
       SecurityInterstitialMetricsHelper::EXTENDED_REPORTING_IS_ENABLED);
 
   if (certificate_report_callback_for_testing_.is_null())

[estark, 2015/03/27 01:22:01]

This bit can go inside the `if` statement as well (to avoid an extra DoNothing
when not in the experiment). Not a big deal though.

[fahl, 2015/03/27 01:59:07]

Done.

     scoped_callback.Reset(base::Bind(&base::DoNothing));
 
-  safe_browsing_ui_manager_->ReportInvalidCertificateChain(
-      request_url().host(), ssl_info_, scoped_callback.Release());
+  if (SendFraudulentCertificateChain())

[estark, 2015/03/27 01:22:01]

I would use curly braces since the body spans more than one line (consistent
with style in line 121-125).

[fahl, 2015/03/27 01:59:07]

Done.

+    safe_browsing_ui_manager_->ReportInvalidCertificateChain(
+        request_url().host(), ssl_info_, scoped_callback.Release());
 }
 
 // static
 bool SSLBlockingPage::IsOptionsOverridable(int options_mask) {
   return (options_mask & SSLBlockingPage::OVERRIDABLE) &&
          !(options_mask & SSLBlockingPage::STRICT_ENFORCEMENT);
 }