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

chrome/common/localized_error.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.
 
 #include "chrome/common/localized_error.h"
 
 #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"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/grit/chromium_strings.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/error_page/common/error_page_params.h"
 #include "components/error_page/common/net_error_info.h"
@@ skipping 644 matching lines @@
     params->suggest_reload = !!(options.suggestions & SUGGEST_RELOAD);
   }
 
   base::ListValue* suggestions = NULL;
   bool use_default_suggestions = true;
   if (!params->override_suggestions) {
     suggestions = new base::ListValue();
   } 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);
 
   if (params->search_url.is_valid()) {
     error_strings->SetString("searchHeader",
         l10n_util::GetStringUTF16(IDS_ERRORPAGES_SUGGESTION_GOOGLE_SEARCH));
     error_strings->SetString("searchUrl", params->search_url.spec());
     error_strings->SetString("searchTerms", params->search_terms);
     error_strings->SetInteger("searchTrackingId", params->search_tracking_id);
@@ skipping 236 matching lines @@
   GURL learn_more_url(kAppWarningLearnMoreUrl);
   base::DictionaryValue* suggest_learn_more = new base::DictionaryValue();
   suggest_learn_more->SetString("msg",
                                 l10n_util::GetStringUTF16(
                                     IDS_ERRORPAGES_SUGGESTION_LEARNMORE_BODY));
   suggest_learn_more->SetString("learnMoreUrl", learn_more_url.spec());
   error_strings->Set("suggestionsLearnMore", suggest_learn_more);
 #endif  // defined(OS_CHROMEOS)
 }
 #endif