Move Google Cache copy suggestion to an action button in the neterror interstitial

chrome/common/localized_error.cc

Index: chrome/common/localized_error.cc
diff --git a/chrome/common/localized_error.cc b/chrome/common/localized_error.cc
index 0c4ad4361e590adf71d4784a78846863e490b287..351d81e53cb397b3ebdd0c1a8870654953f11b04 100644
--- a/chrome/common/localized_error.cc
+++ b/chrome/common/localized_error.cc
@@ -7,6 +7,7 @@
 #include "base/command_line.h"
 #include "base/i18n/rtl.h"
 #include "base/logging.h"
+#include "base/metrics/field_trial.h"
 #include "base/strings/string16.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
@@ -671,6 +672,41 @@ void LocalizedError::GetStrings(int error_code,
   } else {
     suggestions = params->override_suggestions.release();
     use_default_suggestions = false;
+
+    std::string fieldTrialExpType = base::FieldTrialList::FindFullName(

[felt, 2015/05/11 20:46:44]

in C++ style this should be field_trial_exp_type

[edwardjung, 2015/05/12 10:16:50]

Done.

[felt, 2015/05/12 14:15:08]

still seems to be field_trial_exp_type in the latest patchset

[edwardjung, 2015/05/13 09:59:56]

Should it be field_type_exp_type or fieldTrialExpType? I moved it to a the
kCachedCopyButtonFieldTrial const at the top.

[felt, 2015/05/13 14:32:11]

field_type_exp_type all local variables are done with underscores in C++, sorry
for my confusing comment

[edwardjung, 2015/05/13 17:34:33]

No, my fault, couldn't see the wood for the trees.

+        "EnableGoogleCachedCopyTextExperiment");
+
+    // If the first suggestion is for a Google cache copy. Promote the
+    // suggestion to a separate set of strings for displaying as a button.
+    if (!suggestions->empty() && fieldTrialExpType != "") {

[felt, 2015/05/11 20:46:43]

!field_trial_exp_type.empty()

[felt, 2015/05/11 20:46:44]

Do you also want to support a control group (e.g., the field trial will respond
with "control") for the experiment? If so the check should probably go up here.

[edwardjung, 2015/05/12 10:16:50]

Good point, will add a control.

[edwardjung, 2015/05/12 10:16:50]

Done.

+      base::DictionaryValue* suggestion;
+      suggestions->GetDictionary(0, &suggestion);
+      int type = -1;
+      suggestion->GetInteger("type", &type);
+
+      if (type == 0) {
+        base::string16 cache_url;
+        suggestion->GetString("urlCorrection", &cache_url);
+        int cache_tracking_id = -1;
+        suggestion->GetInteger("trackingId", &cache_tracking_id);
+
+        base::DictionaryValue* cache_button = new base::DictionaryValue;
+
+        if (fieldTrialExpType == "copy") {

[felt, 2015/05/11 20:46:43]

I'd recommend setting both "EnableGoogleCachedCopyTextExperiment" and "copy" as
constants in an anonymous namespace at the top. It makes it easier to look the
constants up quickly or change them later.

[edwardjung, 2015/05/12 10:16:50]

Done.

+          cache_button->SetString(
+              "msg", l10n_util::GetStringUTF16(

[felt, 2015/05/11 20:46:43]

nit: this line wrap (and the one below) looks kinda wonky. did `git cl format`
do it this way?

[edwardjung, 2015/05/12 10:16:50]

I wasn't even aware of that command. Great time saver. 

[felt, 2015/05/12 14:15:08]

always double check it, sometimes it does weird stuff. but it is useful. :)

+              IDS_ERRORPAGES_BUTTON_SHOW_CACHED_COPY));
+        } else {
+          cache_button->SetString(
+              "msg", l10n_util::GetStringUTF16(
+              IDS_ERRORPAGES_BUTTON_SHOW_CACHED_PAGE));
+        }

[felt, 2015/05/11 20:46:43]

Is there another condition besides "copy"? How might you land in this branch?

[edwardjung, 2015/05/12 10:16:50]

My thinking is there would be three sets:
+ "control" - as it is now, no change.
+ "copy" - Button with "Show cached copy"
+ "page" - Button with "Show cached page"

Should I specify these conditions.

[felt, 2015/05/12 14:15:07]

I think it's clear now in the newest version.

+        cache_button->SetString("cacheUrl", cache_url);
+        cache_button->SetInteger("trackingId", cache_tracking_id);
+        error_strings->Set("cacheButton", cache_button);
+        suggestions->Remove(0, NULL);
+      }
+    }
   }
 
   error_strings->Set("suggestions", suggestions);