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..c64e7edeb217415e276ea91230262cfc3fe83c29 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(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
@@ -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,22 @@ 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") {
-    // content::WaitForRenderFrameReady sends this message when the page
-    // load completes. Ignore it.
-    return;
-  }
+void SafeBrowsingBlockingPage::CommandReceived(const std::string& page_cmd) {
+  int command = 0;
+  bool retval = base::StringToInt(page_cmd, &command);
+  DCHECK(retval);
 
-  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);
@@ -257,7 +239,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 +257,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 +269,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 +294,10 @@ 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;
-  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);
@@ -357,7 +320,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;