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(10); "

[meacer, 2015/03/06 18:29:22]

How about using |sendCommand(%d)| here and filling the actual enum value in
|PopulateExtendedReportingOption|? One fewer harcoded value.

[felt, 2015/03/07 04:02:39]

Done.

     "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);
-  }
+void SafeBrowsingBlockingPage::CommandReceived(const std::string& page_cmd) {
+  int command = 0;
+  bool retval = base::StringToInt(page_cmd, &command);
+  DCHECK(retval);
 
-  if (command == "pageLoadComplete") {
-    // content::WaitForRenderFrameReady sends this message when the page
-    // load completes. Ignore it.
-    return;
-  }
-
-  if (command == kDoReportCommand) {
+  if (command == CMD_DO_REPORT) {
     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;
-  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();

[meacer, 2015/03/06 18:29:22]

While you are at it, could you have a reference to
unsafe_resources_[element_index] and use that below? It would make the code
below cleaner.

[felt, 2015/03/07 04:02:39]

Done.

[meacer, 2015/03/07 05:23:09]

Why not |const UnsafeResource& unsafe_resource = ...|? Looks okay as it is but
could be just one less copy.

[felt, 2015/03/07 05:50:04]

Sure. Leaving the int as a separate definition because it will need to be
defined later, shouldn't always be 0.

-  if (command == kShowDiagnosticCommand) {
+  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 ==
                SB_THREAT_TYPE_CLIENT_SIDE_MALWARE_URL ||
            unsafe_resources_[element_index].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 331 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);
 }