Simplify security_interstitials::ControllerClient and other related classes

chrome/browser/safe_browsing/safe_browsing_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.
 //
 // Implementation of the SafeBrowsingBlockingPage class.
 
 #include "chrome/browser/safe_browsing/safe_browsing_blocking_page.h"
 
 #include <string>
 
@@ skipping 102 matching lines @@
 
  private:
   friend struct base::DefaultLazyInstanceTraits<
       SafeBrowsingBlockingPageFactoryImpl>;
 
   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;
 
 // static
 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;
     } else if (threat_type == SB_THREAT_TYPE_URL_UNWANTED) {
       harmful = true;
     } else {
       DCHECK(threat_type == SB_THREAT_TYPE_URL_PHISHING ||
              threat_type == SB_THREAT_TYPE_CLIENT_SIDE_PHISHING_URL);
       phishing = true;
     }
   }
   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);
   }
 
   if (!is_main_frame_load_blocked_) {
     navigation_entry_index_to_remove_ =
         web_contents->GetController().GetLastCommittedEntryIndex();
   } else {
     navigation_entry_index_to_remove_ = -1;
   }
 
@@ skipping 154 matching lines @@
   Profile* profile = Profile::FromBrowserContext(
       web_contents()->GetBrowserContext());
   renderer_preferences_util::UpdateFromSystemSettings(
       prefs, profile, web_contents());
 }
 
 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);
 
   // Check to see if some new notifications of unsafe resources have been
   // received while we were showing the interstitial.
   UnsafeResourceMap* unsafe_resource_map = GetUnsafeResourcesMap();
   UnsafeResourceMap::iterator iter = unsafe_resource_map->find(web_contents());
   SafeBrowsingBlockingPage* blocking_page = NULL;
   if (iter != unsafe_resource_map->end() && !iter->second.empty()) {
     // All queued unsafe resources should be for the same page:
@@ skipping 22 matching lines @@
   return is_main_frame_load_blocked_;
 }
 
 void SafeBrowsingBlockingPage::OnDontProceed() {
   // We could have already called Proceed(), in which case we must not notify
   // the SafeBrowsingUIManager again, as the client has been deleted.
   if (proceeded_)
     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);
 
   // The user does not want to proceed, clear the queued unsafe resources
   // notifications we received while the interstitial was showing.
   UnsafeResourceMap* unsafe_resource_map = GetUnsafeResourcesMap();
   UnsafeResourceMap::iterator iter = unsafe_resource_map->find(web_contents());
   if (iter != unsafe_resource_map->end() && !iter->second.empty()) {
     ui_manager_->OnBlockingPageDone(iter->second, false);
     unsafe_resource_map->erase(iter);
@@ skipping 20 matching lines @@
                                                    int num_visits) {
   if (threat_details_.get() == NULL)
     return;  // Not all interstitials have threat details (eg., incognito mode).
 
   const bool enabled =
       IsPrefEnabled(prefs::kSafeBrowsingExtendedReportingEnabled) &&
       IsPrefEnabled(prefs::kSafeBrowsingExtendedReportingOptInAllowed);
   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(
       BrowserThread::IO, FROM_HERE,
       base::Bind(&ThreatDetails::FinishCollection, threat_details_.get(),
                  did_proceed, num_visits),
       base::TimeDelta::FromMilliseconds(delay_ms));
 }
 
 // static
@@ skipping 54 matching lines @@
 
 // static
 bool SafeBrowsingBlockingPage::IsMainPageLoadBlocked(
     const UnsafeResourceList& unsafe_resources) {
   // If there is more than one unsafe resource, the main page load must not be
   // blocked. Otherwise, check if the one resource is.
   return unsafe_resources.size() == 1 &&
          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";
       else if (threat_pattern_type == ThreatPatternType::SOCIAL_ENGINEERING_ADS)
         return primary_subresource ? "social_engineering_ads_subresource"
                                    : "social_engineering_ads";
       else if (threat_pattern_type ==
                ThreatPatternType::SOCIAL_ENGINEERING_LANDING)
         return primary_subresource ? "social_engineering_landing_subresource"
                                    : "social_engineering_landing";
   }
   NOTREACHED();
   return "unkown_metric_prefix";
 }
 
 // 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:
     case safe_browsing::ThreatSource::LOCAL_PVER3:
       // REMOTE and LOCAL_PVER3 can be distinguished in the logs
       // by platform type: Remote is mobile, local_pver3 is desktop.
       return "from_device";
     case safe_browsing::ThreatSource::LOCAL_PVER4:
       return "from_device_v4";
     case safe_browsing::ThreatSource::CLIENT_SIDE_DETECTION:
       return "from_client_side_detection";
     case safe_browsing::ThreatSource::UNKNOWN:
       break;
   }
   NOTREACHED();
   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:
       return "harmful2";
     case SB_REASON_PHISHING:
       return "phishing2";
   }
   NOTREACHED();
   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:
       return "harmful";
     case SB_REASON_PHISHING:
       return "phishing";
   }
   NOTREACHED();
   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:
       return kEventNameHarmful;
     case SB_REASON_PHISHING:
       return kEventNamePhishing;
     default:
       return kEventNameOther;
   }
 }
@@ skipping 118 matching lines @@
       l10n_util::GetStringFUTF16(IDS_PHISHING_V4_EXPLANATION_PARAGRAPH,
                                  GetFormattedHostName()));
   load_time_data->SetString(
       "finalParagraph",
       l10n_util::GetStringUTF16(IDS_PHISHING_V4_PROCEED_AND_REPORT_PARAGRAPH));
 
   PopulateExtendedReportingOption(load_time_data);
 }
 
 }  // namespace safe_browsing