Simplify security_interstitials::ControllerClient and other related classes

chrome/browser/safe_browsing/safe_browsing_blocking_page.cc

Index: chrome/browser/safe_browsing/safe_browsing_blocking_page.cc
diff --git a/chrome/browser/safe_browsing/safe_browsing_blocking_page.cc b/chrome/browser/safe_browsing/safe_browsing_blocking_page.cc
index 2d01a22d7d48718743e9c1168e5d4f6ab66bd764..3ca33042c029b1b5cb856dab3fc3a2feeae7f6dc 100644
--- a/chrome/browser/safe_browsing/safe_browsing_blocking_page.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_blocking_page.cc
@@ -120,6 +120,32 @@ class SafeBrowsingBlockingPageFactoryImpl
   DISALLOW_COPY_AND_ASSIGN(SafeBrowsingBlockingPageFactoryImpl);
 };
 
+// static
+std::unique_ptr<ChromeMetricsHelper>
+SafeBrowsingBlockingPage::CreateMetricsHelper(
+    WebContents* web_contents,
+    const UnsafeResourceList& unsafe_resources) {
+  SBInterstitialReason interstitial_reason =
+      SafeBrowsingBlockingPage::GetInterstitialReason(unsafe_resources);

[felt, 2016/09/06 20:33:19]

Doesn't this mean that now GetInterstitialReason is called twice in the same
constructor, in two different places?

[meacer, 2016/09/06 20:40:16]

Yes, unfortunately that's true since we can't compute these values before
calling the base class constructor.

Well, we actually can, by adding a static factory method, like I added for
SSLBlockingPage::Create. But the reason I did it there and not here is that
SSLBlockingPage requires the return value of IsOverridable function, and that
function does a preference lookup which I don't want to do twice. Whereas here,
these static calls are fast so it doesn't hurt to do them twice.

+  GURL request_url(unsafe_resources[0].url);
+  security_interstitials::MetricsHelper::ReportDetails reporting_info;
+  reporting_info.metric_prefix = SafeBrowsingBlockingPage::GetMetricPrefix(
+      unsafe_resources, interstitial_reason);
+  reporting_info.extra_suffix =
+      SafeBrowsingBlockingPage::GetExtraMetricsSuffix(unsafe_resources);
+  reporting_info.rappor_prefix =
+      SafeBrowsingBlockingPage::GetRapporPrefix(interstitial_reason);
+  reporting_info.deprecated_rappor_prefix =
+      SafeBrowsingBlockingPage::GetDeprecatedRapporPrefix(interstitial_reason);
+  reporting_info.rappor_report_type =
+      rappor::LOW_FREQUENCY_SAFEBROWSING_RAPPOR_TYPE;
+  reporting_info.deprecated_rappor_report_type =
+      rappor::SAFEBROWSING_RAPPOR_TYPE;
+  return std::unique_ptr<ChromeMetricsHelper>(new ChromeMetricsHelper(
+      web_contents, request_url, reporting_info,
+      SafeBrowsingBlockingPage::GetSamplingEventName(interstitial_reason)));
+}
+
 static base::LazyInstance<SafeBrowsingBlockingPageFactoryImpl>
     g_safe_browsing_blocking_page_factory_impl = LAZY_INSTANCE_INITIALIZER;
 
@@ -128,25 +154,18 @@ content::InterstitialPageDelegate::TypeID
     SafeBrowsingBlockingPage::kTypeForTesting =
         &SafeBrowsingBlockingPage::kTypeForTesting;
 
-SafeBrowsingBlockingPage::SafeBrowsingBlockingPage(
-    SafeBrowsingUIManager* ui_manager,
-    WebContents* web_contents,
-    const GURL& main_frame_url,
-    const UnsafeResourceList& unsafe_resources)
-    : SecurityInterstitialPage(web_contents, unsafe_resources[0].url),
-      threat_details_proceed_delay_ms_(kThreatDetailsProceedDelayMilliSeconds),
-      ui_manager_(ui_manager),
-      is_main_frame_load_blocked_(IsMainPageLoadBlocked(unsafe_resources)),
-      main_frame_url_(main_frame_url),
-      unsafe_resources_(unsafe_resources),
-      proceeded_(false) {
+// static
+SafeBrowsingBlockingPage::SBInterstitialReason
+SafeBrowsingBlockingPage::GetInterstitialReason(
+    const SafeBrowsingBlockingPage::UnsafeResourceList& unsafe_resources) {
   bool malware = false;
   bool harmful = false;
   bool phishing = false;
-  for (UnsafeResourceList::const_iterator iter = unsafe_resources_.begin();
-       iter != unsafe_resources_.end(); ++iter) {
-    const UnsafeResource& resource = *iter;
-    SBThreatType threat_type = resource.threat_type;
+  for (SafeBrowsingBlockingPage::UnsafeResourceList::const_iterator iter =
+           unsafe_resources.begin();
+       iter != unsafe_resources.end(); ++iter) {
+    const SafeBrowsingUIManager::UnsafeResource& resource = *iter;
+    safe_browsing::SBThreatType threat_type = resource.threat_type;
     if (threat_type == SB_THREAT_TYPE_URL_MALWARE ||
         threat_type == SB_THREAT_TYPE_CLIENT_SIDE_MALWARE_URL) {
       malware = true;
@@ -160,30 +179,34 @@ SafeBrowsingBlockingPage::SafeBrowsingBlockingPage(
   }
   DCHECK(phishing || malware || harmful);
   if (malware)
-    interstitial_reason_ = SB_REASON_MALWARE;
+    return SafeBrowsingBlockingPage::SB_REASON_MALWARE;
   else if (harmful)
-    interstitial_reason_ = SB_REASON_HARMFUL;
-  else
-    interstitial_reason_ = SB_REASON_PHISHING;
+    return SafeBrowsingBlockingPage::SB_REASON_HARMFUL;
+  return SafeBrowsingBlockingPage::SB_REASON_PHISHING;
+}
 
-  // This must be done after calculating |interstitial_reason_| above.
-  security_interstitials::MetricsHelper::ReportDetails reporting_info;
-  reporting_info.metric_prefix = GetMetricPrefix();
-  reporting_info.extra_suffix = GetExtraMetricsSuffix();
-  reporting_info.rappor_prefix = GetRapporPrefix();
-  reporting_info.deprecated_rappor_prefix = GetDeprecatedRapporPrefix();
-  reporting_info.rappor_report_type =
-      rappor::LOW_FREQUENCY_SAFEBROWSING_RAPPOR_TYPE;
-  reporting_info.deprecated_rappor_report_type =
-      rappor::SAFEBROWSING_RAPPOR_TYPE;
-  set_metrics_helper(base::MakeUnique<ChromeMetricsHelper>(
-      web_contents, request_url(), reporting_info, GetSamplingEventName()));
-  metrics_helper()->RecordUserDecision(
+SafeBrowsingBlockingPage::SafeBrowsingBlockingPage(
+    SafeBrowsingUIManager* ui_manager,
+    WebContents* web_contents,
+    const GURL& main_frame_url,
+    const UnsafeResourceList& unsafe_resources)
+    : SecurityInterstitialPage(
+          web_contents,
+          unsafe_resources[0].url,
+          CreateMetricsHelper(web_contents, unsafe_resources)),
+      threat_details_proceed_delay_ms_(kThreatDetailsProceedDelayMilliSeconds),
+      ui_manager_(ui_manager),
+      is_main_frame_load_blocked_(IsMainPageLoadBlocked(unsafe_resources)),
+      main_frame_url_(main_frame_url),
+      unsafe_resources_(unsafe_resources),
+      proceeded_(false),
+      interstitial_reason_(GetInterstitialReason(unsafe_resources)) {
+  controller()->metrics_helper()->RecordUserDecision(
       security_interstitials::MetricsHelper::SHOW);
-  metrics_helper()->RecordUserInteraction(
+  controller()->metrics_helper()->RecordUserInteraction(
       security_interstitials::MetricsHelper::TOTAL_VISITS);
   if (IsPrefEnabled(prefs::kSafeBrowsingProceedAnywayDisabled)) {
-    metrics_helper()->RecordUserDecision(
+    controller()->metrics_helper()->RecordUserDecision(
         security_interstitials::MetricsHelper::PROCEEDING_DISABLED);
   }
 
@@ -358,7 +381,7 @@ void SafeBrowsingBlockingPage::OnProceed() {
   proceeded_ = true;
   // Send the threat details, if we opted to.
   FinishThreatDetails(threat_details_proceed_delay_ms_, true, /* did_proceed */
-                      metrics_helper()->NumVisits());
+                      controller()->metrics_helper()->NumVisits());
 
   ui_manager_->OnBlockingPageDone(unsafe_resources_, true);
 
@@ -401,13 +424,13 @@ void SafeBrowsingBlockingPage::OnDontProceed() {
     return;
 
   if (!IsPrefEnabled(prefs::kSafeBrowsingProceedAnywayDisabled)) {
-    metrics_helper()->RecordUserDecision(
+    controller()->metrics_helper()->RecordUserDecision(
         security_interstitials::MetricsHelper::DONT_PROCEED);
   }
 
   // Send the malware details, if we opted to.
   FinishThreatDetails(0, false /* did_proceed */,
-                      metrics_helper()->NumVisits());  // No delay
+                      controller()->metrics_helper()->NumVisits());  // No delay
 
   ui_manager_->OnBlockingPageDone(unsafe_resources_, false);
 
@@ -448,7 +471,7 @@ void SafeBrowsingBlockingPage::FinishThreatDetails(int64_t delay_ms,
   if (!enabled)
     return;
 
-  metrics_helper()->RecordUserInteraction(
+  controller()->metrics_helper()->RecordUserInteraction(
       security_interstitials::MetricsHelper::EXTENDED_REPORTING_IS_ENABLED);
   // Finish the malware details collection, send it over.
   BrowserThread::PostDelayedTask(
@@ -523,16 +546,19 @@ bool SafeBrowsingBlockingPage::IsMainPageLoadBlocked(
          unsafe_resources[0].IsMainPageLoadBlocked();
 }
 
-std::string SafeBrowsingBlockingPage::GetMetricPrefix() const {
-  bool primary_subresource = unsafe_resources_[0].is_subresource;
-  switch (interstitial_reason_) {
+// static
+std::string SafeBrowsingBlockingPage::GetMetricPrefix(
+    const UnsafeResourceList& unsafe_resources,
+    SBInterstitialReason interstitial_reason) {
+  bool primary_subresource = unsafe_resources[0].is_subresource;
+  switch (interstitial_reason) {
     case SB_REASON_MALWARE:
       return primary_subresource ? "malware_subresource" : "malware";
     case SB_REASON_HARMFUL:
       return primary_subresource ? "harmful_subresource" : "harmful";
     case SB_REASON_PHISHING:
       ThreatPatternType threat_pattern_type =
-          unsafe_resources_[0].threat_metadata.threat_pattern_type;
+          unsafe_resources[0].threat_metadata.threat_pattern_type;
       if (threat_pattern_type == ThreatPatternType::PHISHING ||
           threat_pattern_type == ThreatPatternType::NONE)
         return primary_subresource ? "phishing_subresource" : "phishing";
@@ -549,8 +575,10 @@ std::string SafeBrowsingBlockingPage::GetMetricPrefix() const {
 }
 
 // We populate a parallel set of metrics to differentiate some threat sources.
-std::string SafeBrowsingBlockingPage::GetExtraMetricsSuffix() const {
-  switch (unsafe_resources_[0].threat_source) {
+// static
+std::string SafeBrowsingBlockingPage::GetExtraMetricsSuffix(
+    const UnsafeResourceList& unsafe_resources) {
+  switch (unsafe_resources[0].threat_source) {
     case safe_browsing::ThreatSource::DATA_SAVER:
       return "from_data_saver";
     case safe_browsing::ThreatSource::REMOTE:
@@ -569,8 +597,10 @@ std::string SafeBrowsingBlockingPage::GetExtraMetricsSuffix() const {
   return std::string();
 }
 
-std::string SafeBrowsingBlockingPage::GetRapporPrefix() const {
-  switch (interstitial_reason_) {
+// static
+std::string SafeBrowsingBlockingPage::GetRapporPrefix(
+    SBInterstitialReason interstitial_reason) {
+  switch (interstitial_reason) {
     case SB_REASON_MALWARE:
       return "malware2";
     case SB_REASON_HARMFUL:
@@ -582,8 +612,10 @@ std::string SafeBrowsingBlockingPage::GetRapporPrefix() const {
   return std::string();
 }
 
-std::string SafeBrowsingBlockingPage::GetDeprecatedRapporPrefix() const {
-  switch (interstitial_reason_) {
+// static
+std::string SafeBrowsingBlockingPage::GetDeprecatedRapporPrefix(
+    SBInterstitialReason interstitial_reason) {
+  switch (interstitial_reason) {
     case SB_REASON_MALWARE:
       return "malware";
     case SB_REASON_HARMFUL:
@@ -595,8 +627,10 @@ std::string SafeBrowsingBlockingPage::GetDeprecatedRapporPrefix() const {
   return std::string();
 }
 
-std::string SafeBrowsingBlockingPage::GetSamplingEventName() const {
-  switch (interstitial_reason_) {
+// static
+std::string SafeBrowsingBlockingPage::GetSamplingEventName(
+    SBInterstitialReason interstitial_reason) {
+  switch (interstitial_reason) {
     case SB_REASON_MALWARE:
       return kEventNameMalware;
     case SB_REASON_HARMFUL: