Make commands consistent across security interstitials

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 51 matching lines @@
     "http://safebrowsing.clients.google.com/safebrowsing/diagnostic?site=%s&client=chromium";
 #endif
 
 // URL for malware and phishing, V2.
 const char kLearnMoreMalwareUrlV2[] =
     "https://www.google.com/transparencyreport/safebrowsing/";
 const char kLearnMorePhishingUrlV2[] =
     "https://www.google.com/transparencyreport/safebrowsing/";
 
 const char kPrivacyLinkHtml[] =
-    "<a id=\"privacy-link\" href=\"\" onclick=\"sendCommand('showPrivacy'); "
+    "<a id=\"privacy-link\" href=\"\" onclick=\"sendCommand(%d); "
     "return false;\" onmousedown=\"return false;\">%s</a>";
 
 // After a malware interstitial where the user opted-in to the report
 // but clicked "proceed anyway", we delay the call to
 // MalwareDetails::FinishCollection() by this much time (in
 // milliseconds).
 const int64 kMalwareDetailsProceedDelayMilliSeconds = 3000;
 
-// The commands returned by the page when the user performs an action.
-const char kDoReportCommand[] = "doReport";
-const char kDontReportCommand[] = "dontReport";
-const char kExpandedSeeMoreCommand[] = "expandedSeeMore";
-const char kLearnMoreCommand[] = "learnMore2";
-const char kProceedCommand[] = "proceed";
-const char kShowDiagnosticCommand[] = "showDiagnostic";
-const char kShowPrivacyCommand[] = "showPrivacy";
-const char kTakeMeBackCommand[] = "takeMeBack";
-
 // Other constants used to communicate with the JavaScript.
 const char kBoxChecked[] = "boxchecked";
 const char kDisplayCheckBox[] = "displaycheckbox";
 
 // Constants for the Experience Sampling instrumentation.
 const char kEventNameMalware[] = "safebrowsing_interstitial_";
 const char kEventNameHarmful[] = "harmful_interstitial_";
 const char kEventNamePhishing[] = "phishing_interstitial_";
 const char kEventNameOther[] = "safebrowsing_other_interstitial_";
 
@@ skipping 108 matching lines @@
 }
 
 bool SafeBrowsingBlockingPage::CanShowMalwareDetailsOption() {
   return (!web_contents()->GetBrowserContext()->IsOffTheRecord() &&
           web_contents()->GetURL().SchemeIs(url::kHttpScheme));
 }
 
 SafeBrowsingBlockingPage::~SafeBrowsingBlockingPage() {
 }
 
-void SafeBrowsingBlockingPage::CommandReceived(const std::string& cmd) {
-  std::string command(cmd);  // Make a local copy so we can modify it.
-  // The Jasonified response has quotes, remove them.
-  if (command.length() > 1 && command[0] == '"') {
-    command = command.substr(1, command.length() - 2);
-  }
-
-  if (command == "pageLoadComplete") {
+void SafeBrowsingBlockingPage::CommandReceived(const std::string& page_cmd) {
+  if (page_cmd == "\"pageLoadComplete\"") {
     // content::WaitForRenderFrameReady sends this message when the page
     // load completes. Ignore it.
     return;
   }
 
-  if (command == kDoReportCommand) {
+  int command = 0;
+  bool retval = base::StringToInt(page_cmd, &command);
+  DCHECK(retval) << page_cmd;
+
+  if (command == CMD_DO_REPORT) {

[Bernhard Bauer, 2015/03/11 08:42:14]

I *think* you could turn this into a switch statement now. The only tricky part
I can see is the |proceed_blocked| check below, but you could solve that by
falling through to the next case if proceeding is disabled, and breaking out of
the switch statement only if it is allowed. With sufficient comments, that might
make the control flow even a bit clearer than it is right now.

[felt, 2015/03/11 15:25:09]

Done.

     SetReportingPreference(true);
     return;
   }
 
-  if (command == kDontReportCommand) {
+  if (command == CMD_DONT_REPORT) {
     SetReportingPreference(false);
     return;
   }
 
-  if (command == kLearnMoreCommand) {
+  if (command == CMD_OPEN_HELP_CENTER) {
     // User pressed "Learn more".
     metrics_helper_->RecordUserInteraction(
         SecurityInterstitialMetricsHelper::SHOW_LEARN_MORE);
     GURL learn_more_url(
         interstitial_reason_ == SB_REASON_PHISHING ?
         kLearnMorePhishingUrlV2 : kLearnMoreMalwareUrlV2);
     learn_more_url = google_util::AppendGoogleLocaleParam(
         learn_more_url, g_browser_process->GetApplicationLocale());
     OpenURLParams params(learn_more_url,
                          Referrer(),
                          CURRENT_TAB,
                          ui::PAGE_TRANSITION_LINK,
                          false);
     web_contents()->OpenURL(params);
     return;
   }
 
-  if (command == kShowPrivacyCommand) {
+  if (command == CMD_OPEN_REPORTING_PRIVACY) {
     // User pressed "Safe Browsing privacy policy".
     metrics_helper_->RecordUserInteraction(
         SecurityInterstitialMetricsHelper::SHOW_PRIVACY_POLICY);
     GURL privacy_url(
         l10n_util::GetStringUTF8(IDS_SAFE_BROWSING_PRIVACY_POLICY_URL));
     privacy_url = google_util::AppendGoogleLocaleParam(
         privacy_url, g_browser_process->GetApplicationLocale());
     OpenURLParams params(privacy_url,
                          Referrer(),
                          CURRENT_TAB,
                          ui::PAGE_TRANSITION_LINK,
                          false);
     web_contents()->OpenURL(params);
     return;
   }
 
   bool proceed_blocked = false;
-  if (command == kProceedCommand) {
+  if (command == CMD_PROCEED) {
     if (IsPrefEnabled(prefs::kSafeBrowsingProceedAnywayDisabled)) {
       proceed_blocked = true;
     } else {
       metrics_helper_->RecordUserDecision(
           SecurityInterstitialMetricsHelper::PROCEED);
       interstitial_page()->Proceed();
       // |this| has been deleted after Proceed() returns.
       return;
     }
   }
 
-  if (command == kTakeMeBackCommand || proceed_blocked) {
+  if (command == CMD_DONT_PROCEED || proceed_blocked) {
     // Don't record the user action here because there are other ways of
     // triggering DontProceed, like clicking the back button.
     if (is_main_frame_load_blocked_) {
       // If the load is blocked, we want to close the interstitial and discard
       // the pending entry.
       interstitial_page()->DontProceed();
       // |this| has been deleted after DontProceed() returns.
       return;
     }
 
     // Otherwise the offending entry has committed, and we need to go back or
     // to a safe page.  We will close the interstitial when that page commits.
     if (web_contents()->GetController().CanGoBack()) {
       web_contents()->GetController().GoBack();
     } else {
       web_contents()->GetController().LoadURL(
           GURL(chrome::kChromeUINewTabURL),
           content::Referrer(),
           ui::PAGE_TRANSITION_AUTO_TOPLEVEL,
           std::string());
     }
     return;
   }
 
-  // The "report error" and "show diagnostic" commands can have a number
-  // appended to them, which is the index of the element they apply to.
+  // TODO(felt): element_index will always be 0. See crbug.com/464732
   size_t element_index = 0;

[Bernhard Bauer, 2015/03/11 08:42:14]

Move this into the if-statement block?

[felt, 2015/03/11 15:25:09]

Done.

-  size_t colon_index = command.find(':');
-  if (colon_index != std::string::npos) {
-    DCHECK(colon_index < command.size() - 1);
-    int result_int = 0;
-    bool result = base::StringToInt(base::StringPiece(command.begin() +
-                                                      colon_index + 1,
-                                                      command.end()),
-                                    &result_int);
-    command = command.substr(0, colon_index);
-    if (result)
-      element_index = static_cast<size_t>(result_int);
-  }
-
-  if (element_index >= unsafe_resources_.size()) {
-    NOTREACHED();
-    return;
-  }
-
-  std::string bad_url_spec = unsafe_resources_[element_index].url.spec();
-  if (command == kShowDiagnosticCommand) {
+  const UnsafeResource& unsafe_resource = unsafe_resources_[element_index];
+  std::string bad_url_spec = unsafe_resource.url.spec();
+  if (command == CMD_OPEN_DIAGNOSTIC) {
     // We're going to take the user to Google's SafeBrowsing diagnostic page.
     metrics_helper_->RecordUserInteraction(
         SecurityInterstitialMetricsHelper::SHOW_DIAGNOSTIC);
     std::string diagnostic =
         base::StringPrintf(kSbDiagnosticUrl,
             net::EscapeQueryParamValue(bad_url_spec, true).c_str());
     GURL diagnostic_url(diagnostic);
     diagnostic_url = google_util::AppendGoogleLocaleParam(
         diagnostic_url, g_browser_process->GetApplicationLocale());
-    DCHECK(unsafe_resources_[element_index].threat_type ==
-               SB_THREAT_TYPE_URL_MALWARE ||
-           unsafe_resources_[element_index].threat_type ==
+    DCHECK(unsafe_resource.threat_type == SB_THREAT_TYPE_URL_MALWARE ||
+           unsafe_resource.threat_type ==
                SB_THREAT_TYPE_CLIENT_SIDE_MALWARE_URL ||
-           unsafe_resources_[element_index].threat_type ==
-               SB_THREAT_TYPE_URL_UNWANTED);
+           unsafe_resource.threat_type == SB_THREAT_TYPE_URL_UNWANTED);
     OpenURLParams params(
         diagnostic_url, Referrer(), CURRENT_TAB, ui::PAGE_TRANSITION_LINK,
         false);
     web_contents()->OpenURL(params);
     return;
   }
 
-  if (command == kExpandedSeeMoreCommand) {
+  if (command == CMD_SHOW_MORE_SECTION) {
     metrics_helper_->RecordUserInteraction(
         SecurityInterstitialMetricsHelper::SHOW_ADVANCED);
     return;
   }
 
   NOTREACHED() << "Unexpected command: " << command;
 }
 
 void SafeBrowsingBlockingPage::OverrideRendererPrefs(
       content::RendererPreferences* prefs) {
@@ skipping 250 matching lines @@
 void SafeBrowsingBlockingPage::PopulateExtendedReportingOption(
     base::DictionaryValue* load_time_data) {
   // Only show checkbox if !(HTTPS || incognito-mode).
   const bool show = CanShowMalwareDetailsOption();
   load_time_data->SetBoolean(kDisplayCheckBox, show);
   if (!show)
     return;
 
   const std::string privacy_link = base::StringPrintf(
       kPrivacyLinkHtml,
+      CMD_OPEN_REPORTING_PRIVACY,
       l10n_util::GetStringUTF8(
           IDS_SAFE_BROWSING_PRIVACY_POLICY_PAGE).c_str());
   load_time_data->SetString(
       "optInLink",
       l10n_util::GetStringFUTF16(IDS_SAFE_BROWSING_MALWARE_REPORTING_AGREE,
                                  base::UTF8ToUTF16(privacy_link)));
   load_time_data->SetBoolean(
       kBoxChecked,
       IsPrefEnabled(prefs::kSafeBrowsingExtendedReportingEnabled));
 }
@@ skipping 61 matching lines @@
   load_time_data->SetString(
       "explanationParagraph",
       l10n_util::GetStringFUTF16(IDS_PHISHING_V3_EXPLANATION_PARAGRAPH,
                                  GetFormattedHostName()));
   load_time_data->SetString(
       "finalParagraph",
       l10n_util::GetStringUTF16(IDS_PHISHING_V3_PROCEED_PARAGRAPH));
 
   PopulateExtendedReportingOption(load_time_data);
 }