Make commands consistent across security interstitials

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 67f2cc4dd0b3e7f6a96a07e1f5f7ca75a267bf5f..6e5ae0dfec67138dbdfcfde2231a30b9ba889921 100644
--- a/chrome/browser/safe_browsing/safe_browsing_blocking_page.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_blocking_page.cc
@@ -69,7 +69,7 @@ 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
@@ -78,16 +78,6 @@ const char kPrivacyLinkHtml[] =
 // 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";
@@ -216,30 +206,28 @@ bool SafeBrowsingBlockingPage::CanShowMalwareDetailsOption() {
 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);
@@ -257,7 +245,7 @@ void SafeBrowsingBlockingPage::CommandReceived(const std::string& cmd) {
     return;
   }
 
-  if (command == kShowPrivacyCommand) {
+  if (command == CMD_OPEN_REPORTING_PRIVACY) {
     // User pressed "Safe Browsing privacy policy".
     metrics_helper_->RecordUserInteraction(
         SecurityInterstitialMetricsHelper::SHOW_PRIVACY_POLICY);
@@ -275,7 +263,7 @@ void SafeBrowsingBlockingPage::CommandReceived(const std::string& cmd) {
   }
 
   bool proceed_blocked = false;
-  if (command == kProceedCommand) {
+  if (command == CMD_PROCEED) {
     if (IsPrefEnabled(prefs::kSafeBrowsingProceedAnywayDisabled)) {
       proceed_blocked = true;
     } else {
@@ -287,7 +275,7 @@ void SafeBrowsingBlockingPage::CommandReceived(const std::string& cmd) {
     }
   }
 
-  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_) {
@@ -312,29 +300,11 @@ void SafeBrowsingBlockingPage::CommandReceived(const std::string& cmd) {
     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);
@@ -344,12 +314,10 @@ void SafeBrowsingBlockingPage::CommandReceived(const std::string& cmd) {
     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);
@@ -357,7 +325,7 @@ void SafeBrowsingBlockingPage::CommandReceived(const std::string& cmd) {
     return;
   }
 
-  if (command == kExpandedSeeMoreCommand) {
+  if (command == CMD_SHOW_MORE_SECTION) {
     metrics_helper_->RecordUserInteraction(
         SecurityInterstitialMetricsHelper::SHOW_ADVANCED);
     return;
@@ -628,6 +596,7 @@ void SafeBrowsingBlockingPage::PopulateExtendedReportingOption(
 
   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(