Refactor BaseBlockingPage to reduce duplicated code

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 b6a9663ddd160481c2e19b6434792b2124d87825..2b9c9b01750345eb31471c63dd51c719e56b608b 100644
--- a/chrome/browser/safe_browsing/safe_browsing_blocking_page.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_blocking_page.cc
@@ -6,16 +6,13 @@
 
 #include "chrome/browser/safe_browsing/safe_browsing_blocking_page.h"
 
-#include "base/command_line.h"
 #include "base/lazy_instance.h"
-#include "chrome/browser/browser_process.h"
 #include "chrome/browser/interstitials/chrome_controller_client.h"
 #include "chrome/browser/interstitials/chrome_metrics_helper.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/renderer_preferences_util.h"
 #include "chrome/browser/safe_browsing/threat_details.h"
 #include "chrome/common/pref_names.h"
-#include "chrome/common/url_constants.h"
 #include "components/prefs/pref_service.h"
 #include "components/safe_browsing_db/safe_browsing_prefs.h"
 #include "content/public/browser/browser_thread.h"
@@ -33,12 +30,6 @@ namespace safe_browsing {
 
 namespace {
 
-// After a safe browsing interstitial where the user opted-in to the report
-// but clicked "proceed anyway", we delay the call to
-// ThreatDetails::FinishCollection() by this much time (in
-// milliseconds).
-const int64_t kThreatDetailsProceedDelayMilliSeconds = 3000;
-
 // Constants for the Experience Sampling instrumentation.
 const char kEventNameMalware[] = "safebrowsing_interstitial_";
 const char kEventNameHarmful[] = "harmful_interstitial_";
@@ -115,9 +106,8 @@ SafeBrowsingBlockingPage::SafeBrowsingBlockingPage(
           web_contents,
           unsafe_resources[0].url,
           unsafe_resources,
-          CreateControllerClient(web_contents, unsafe_resources),
-          display_options),
-      threat_details_proceed_delay_ms_(kThreatDetailsProceedDelayMilliSeconds) {
+          CreateControllerClient(web_contents, unsafe_resources, ui_manager),
+          display_options) {
   // Start computing threat details. They will be sent only
   // if the user opts-in on the blocking page later.
   // If there's more than one malicious resources, it means the user
@@ -152,40 +142,27 @@ void SafeBrowsingBlockingPage::OverrideRendererPrefs(
       prefs, profile, web_contents());
 }
 
-void SafeBrowsingBlockingPage::OnProceed() {
-  set_proceeded(true);
-  UpdateMetricsAfterSecurityInterstitial();
-
-  // Send the threat details, if we opted to.
-  FinishThreatDetails(
-      base::TimeDelta::FromMilliseconds(threat_details_proceed_delay_ms_),
-      true, /* did_proceed */
-      controller()->metrics_helper()->NumVisits());
-
-  ui_manager()->OnBlockingPageDone(unsafe_resources(), true, web_contents(),
-                                   main_frame_url());
-
+void SafeBrowsingBlockingPage::HandleSubresourcesAfterProceed() {
   // 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:
+    UnsafeResourceList unsafe_resources = iter->second;
     content::NavigationEntry* entry =
-        iter->second[0].GetNavigationEntryForResource();
+        unsafe_resources[0].GetNavigationEntryForResource();
     // Build an interstitial for all the unsafe resources notifications.
     // Don't show it now as showing an interstitial while an interstitial is
     // already showing would cause DontProceed() to be invoked.
-    blocking_page = factory_->CreateSafeBrowsingPage(
+    SafeBrowsingBlockingPage* blocking_page = factory_->CreateSafeBrowsingPage(
         ui_manager(), web_contents(), entry ? entry->GetURL() : GURL(),
-        iter->second);
+        unsafe_resources);
     unsafe_resource_map->erase(iter);
-  }
 
-  // Now that this interstitial is gone, we can show the new one.
-  if (blocking_page)
+    // Now that this interstitial is gone, we can show the new one.
     blocking_page->Show();
+  }
 }
 
 content::InterstitialPageDelegate::TypeID
@@ -237,8 +214,12 @@ void SafeBrowsingBlockingPage::ShowBlockingPage(
   DVLOG(1) << __func__ << " " << unsafe_resource.url.spec();
   WebContents* web_contents = unsafe_resource.web_contents_getter.Run();
 
-  if (!InterstitialPage::GetInterstitialPage(web_contents) ||
-      !unsafe_resource.is_subresource) {
+  if (InterstitialPage::GetInterstitialPage(web_contents) &&
+      unsafe_resource.is_subresource) {
+    // This is an interstitial for a page's resource, let's queue it.
+    UnsafeResourceMap* unsafe_resource_map = GetUnsafeResourcesMap();
+    (*unsafe_resource_map)[web_contents].push_back(unsafe_resource);
+  } else {
     // There is no interstitial currently showing in that tab, or we are about
     // to display a new one for the main frame. If there is already an
     // interstitial, showing the new one will automatically hide the old one.
@@ -248,12 +229,7 @@ void SafeBrowsingBlockingPage::ShowBlockingPage(
         CreateBlockingPage(ui_manager, web_contents,
                            entry ? entry->GetURL() : GURL(), unsafe_resource);
     blocking_page->Show();
-    return;
   }
-
-  // This is an interstitial for a page's resource, let's queue it.
-  UnsafeResourceMap* unsafe_resource_map = GetUnsafeResourcesMap();
-  (*unsafe_resource_map)[web_contents].push_back(unsafe_resource);
 }
 
 // static
@@ -272,34 +248,24 @@ std::string SafeBrowsingBlockingPage::GetSamplingEventName(
 }
 
 // static
-std::unique_ptr<security_interstitials::SecurityInterstitialControllerClient>
+std::unique_ptr<SecurityInterstitialControllerClient>
 SafeBrowsingBlockingPage::CreateControllerClient(
     WebContents* web_contents,
-    const UnsafeResourceList& unsafe_resources) {
-  SafeBrowsingErrorUI::SBInterstitialReason interstitial_reason =
-      GetInterstitialReason(unsafe_resources);
-  GURL request_url(unsafe_resources[0].url);
-  security_interstitials::MetricsHelper::ReportDetails reporting_info;
-  reporting_info.metric_prefix =
-      GetMetricPrefix(unsafe_resources, interstitial_reason);
-  reporting_info.extra_suffix = GetExtraMetricsSuffix(unsafe_resources);
-
-  std::unique_ptr<ChromeMetricsHelper> metrics_helper =
-      base::MakeUnique<ChromeMetricsHelper>(
-          web_contents, request_url, reporting_info,
-          GetSamplingEventName(interstitial_reason));
-
+    const UnsafeResourceList& unsafe_resources,
+    const BaseUIManager* ui_manager) {
   Profile* profile = Profile::FromBrowserContext(
       web_contents->GetBrowserContext());
   DCHECK(profile);
 
-  return base::MakeUnique<
-      security_interstitials::SecurityInterstitialControllerClient>(
-          web_contents,
-          std::move(metrics_helper),
-          profile->GetPrefs(),
-          g_browser_process->GetApplicationLocale(),
-          GURL(chrome::kChromeUINewTabURL));
+  std::unique_ptr<ChromeMetricsHelper> metrics_helper =
+      base::MakeUnique<ChromeMetricsHelper>(
+          web_contents, unsafe_resources[0].url,
+          GetReportingInfo(unsafe_resources),
+          GetSamplingEventName(GetInterstitialReason(unsafe_resources)));
+
+  return base::MakeUnique<SecurityInterstitialControllerClient>(
+      web_contents, std::move(metrics_helper), profile->GetPrefs(),
+      ui_manager->app_locale(), ui_manager->default_safe_page());
 }
 
 }  // namespace safe_browsing