Network error interstitial update - add suggestions list

components/error_page/common/localized_error.cc

Index: components/error_page/common/localized_error.cc
diff --git a/components/error_page/common/localized_error.cc b/components/error_page/common/localized_error.cc
index 88950141f68df566c2d2100788a1697cd3d82320..8b8df1dd18cd0cf7ba174e1587afb5d3e2b67654 100644
--- a/components/error_page/common/localized_error.cc
+++ b/components/error_page/common/localized_error.cc
@@ -45,23 +45,26 @@ namespace {
 const unsigned int kErrorPagesNoDetails = 0;
 
 static const char kRedirectLoopLearnMoreUrl[] =
-    "https://support.google.com/chrome/answer/95626";
+    "https://support.google.com/chrome?p=rl_error";
 static const char kWeakDHKeyLearnMoreUrl[] =
     "https://support.google.com/chrome?p=dh_error";
 static const int kGoogleCachedCopySuggestionType = 0;
 
 enum NAV_SUGGESTIONS {
-  SUGGEST_NONE                  = 0,
-  SUGGEST_RELOAD                = 1 << 0,
-  SUGGEST_CHECK_CONNECTION      = 1 << 1,
-  SUGGEST_DNS_CONFIG            = 1 << 2,
-  SUGGEST_FIREWALL_CONFIG       = 1 << 3,
-  SUGGEST_PROXY_CONFIG          = 1 << 4,
-  SUGGEST_DISABLE_EXTENSION     = 1 << 5,
-  SUGGEST_LEARNMORE             = 1 << 6,
-  SUGGEST_VIEW_POLICIES         = 1 << 7,
-  SUGGEST_CONTACT_ADMINISTRATOR = 1 << 8,
-  SUGGEST_UNSUPPORTED_CIPHER    = 1 << 9,
+  SUGGEST_NONE                   = 0,
+  SUGGEST_RELOAD                 = 1 << 0,
+  SUGGEST_CHECK_CONNECTION       = 1 << 1,
+  SUGGEST_DNS_CONFIG             = 1 << 2,
+  SUGGEST_FIREWALL_CONFIG        = 1 << 3,
+  SUGGEST_PROXY_CONFIG           = 1 << 4,
+  SUGGEST_DISABLE_EXTENSION      = 1 << 5,
+  SUGGEST_LEARNMORE              = 1 << 6,
+  SUGGEST_VIEW_POLICIES          = 1 << 7,
+  SUGGEST_CONTACT_ADMINISTRATOR  = 1 << 8,
+  SUGGEST_UNSUPPORTED_CIPHER     = 1 << 9,
+  SUGGEST_AV_CONFIG              = 1 << 10,
+  SUGGEST_OFFLINE_CHECKS         = 1 << 11,
+  SUGGEST_COMPLETE_SETUP         = 1 << 12,
 };
 
 struct LocalizedErrorMap {
@@ -99,7 +102,8 @@ const LocalizedErrorMap net_error_options[] = {
    IDS_ERRORPAGES_HEADING_NOT_AVAILABLE,
    IDS_ERRORPAGES_SUMMARY_CONNECTION_CLOSED,
    IDS_ERRORPAGES_DETAILS_CONNECTION_CLOSED,
-   SUGGEST_RELOAD,
+   SUGGEST_RELOAD, SUGGEST_CHECK_CONNECTION | SUGGEST_FIREWALL_CONFIG |

[mmenke, 2016/02/18 19:51:18]

BUG:  That comma should be an |.

Suggest removing error_explanation_id from the struct in this CL - it's not
being used, and can lead to bugs like this one.

[edwardjung, 2016/02/19 18:18:22]

Done.

+       SUGGEST_PROXY_CONFIG,
   },
   {net::ERR_CONNECTION_RESET,
    IDS_ERRORPAGES_TITLE_NOT_AVAILABLE,
@@ -144,28 +148,28 @@ const LocalizedErrorMap net_error_options[] = {
    IDS_ERRORPAGES_HEADING_NOT_AVAILABLE,
    IDS_ERRORPAGES_SUMMARY_ADDRESS_UNREACHABLE,
    IDS_ERRORPAGES_DETAILS_ADDRESS_UNREACHABLE,
-   SUGGEST_RELOAD | SUGGEST_FIREWALL_CONFIG | SUGGEST_PROXY_CONFIG,
+   SUGGEST_RELOAD,
   },
   {net::ERR_NETWORK_ACCESS_DENIED,
    IDS_ERRORPAGES_TITLE_NOT_AVAILABLE,
    IDS_ERRORPAGES_HEADING_NETWORK_ACCESS_DENIED,
    IDS_ERRORPAGES_SUMMARY_NETWORK_ACCESS_DENIED,
    IDS_ERRORPAGES_DETAILS_NETWORK_ACCESS_DENIED,
-   SUGGEST_FIREWALL_CONFIG,
+   SUGGEST_CHECK_CONNECTION | SUGGEST_FIREWALL_CONFIG | SUGGEST_AV_CONFIG,
   },
   {net::ERR_PROXY_CONNECTION_FAILED,
    IDS_ERRORPAGES_TITLE_NOT_AVAILABLE,
    IDS_ERRORPAGES_HEADING_INTERNET_DISCONNECTED,
    IDS_ERRORPAGES_SUMMARY_PROXY_CONNECTION_FAILED,
    IDS_ERRORPAGES_DETAILS_PROXY_CONNECTION_FAILED,
-   SUGGEST_PROXY_CONFIG,
+   SUGGEST_PROXY_CONFIG | SUGGEST_CONTACT_ADMINISTRATOR,
   },
   {net::ERR_INTERNET_DISCONNECTED,
    IDS_ERRORPAGES_TITLE_NOT_AVAILABLE,
    IDS_ERRORPAGES_HEADING_INTERNET_DISCONNECTED,
    IDS_ERRORPAGES_SUMMARY_INTERNET_DISCONNECTED,
    IDS_ERRORPAGES_DETAILS_INTERNET_DISCONNECTED,
-   SUGGEST_CHECK_CONNECTION | SUGGEST_FIREWALL_CONFIG,
+   SUGGEST_OFFLINE_CHECKS,
   },
   {net::ERR_FILE_NOT_FOUND,
    IDS_ERRORPAGES_TITLE_LOAD_FAILED,
@@ -249,14 +253,14 @@ const LocalizedErrorMap net_error_options[] = {
    IDS_ERRORPAGES_HEADING_INSECURE_CONNECTION,
    IDS_ERRORPAGES_SUMMARY_INVALID_RESPONSE,
    IDS_ERRORPAGES_DETAILS_SSL_PROTOCOL_ERROR,
-   SUGGEST_NONE,
+   SUGGEST_RELOAD,
   },
   {net::ERR_BAD_SSL_CLIENT_AUTH_CERT,
    IDS_ERRORPAGES_TITLE_LOAD_FAILED,
    IDS_ERRORPAGES_HEADING_INSECURE_CONNECTION,
    IDS_ERRORPAGES_SUMMARY_BAD_SSL_CLIENT_AUTH_CERT,
    IDS_ERRORPAGES_DETAILS_BAD_SSL_CLIENT_AUTH_CERT,
-   SUGGEST_NONE,
+   SUGGEST_CONTACT_ADMINISTRATOR,
   },
   {net::ERR_SSL_WEAK_SERVER_EPHEMERAL_DH_KEY,
    IDS_ERRORPAGES_TITLE_LOAD_FAILED,
@@ -277,7 +281,7 @@ const LocalizedErrorMap net_error_options[] = {
    IDS_ERRORPAGES_HEADING_NOT_AVAILABLE,
    IDS_ERRORPAGES_SUMMARY_NOT_AVAILABLE,
    IDS_ERRORPAGES_DETAILS_TEMPORARILY_THROTTLED,
-   SUGGEST_NONE,
+   SUGGEST_DISABLE_EXTENSION,
   },
   {net::ERR_BLOCKED_BY_CLIENT,
    IDS_ERRORPAGES_TITLE_BLOCKED,
@@ -291,7 +295,7 @@ const LocalizedErrorMap net_error_options[] = {
    IDS_ERRORPAGES_HEADING_CONNECTION_INTERRUPTED,
    IDS_ERRORPAGES_SUMMARY_NETWORK_CHANGED,
    IDS_ERRORPAGES_DETAILS_NETWORK_CHANGED,
-   SUGGEST_RELOAD | SUGGEST_CHECK_CONNECTION,
+   SUGGEST_RELOAD,
   },
   {net::ERR_BLOCKED_BY_ADMINISTRATOR,
    IDS_ERRORPAGES_TITLE_BLOCKED,
@@ -305,12 +309,12 @@ const LocalizedErrorMap net_error_options[] = {
    IDS_ERRORPAGES_HEADING_INTERNET_DISCONNECTED,
    IDS_ERRORPAGES_SUMMARY_BLOCKED_ENROLLMENT_CHECK_PENDING,
    IDS_ERRORPAGES_DETAILS_BLOCKED_ENROLLMENT_CHECK_PENDING,
-   SUGGEST_CHECK_CONNECTION,
+   SUGGEST_COMPLETE_SETUP,
   },
   {net::ERR_SSL_FALLBACK_BEYOND_MINIMUM_VERSION,
    IDS_ERRORPAGES_TITLE_LOAD_FAILED,
    IDS_ERRORPAGES_HEADING_INSECURE_CONNECTION,
-   IDS_ERRORPAGES_SUMMARY_SSL_SECURITY_ERROR,
+   IDS_ERRORPAGES_SUMMARY_INVALID_RESPONSE,
    IDS_ERRORPAGES_DETAILS_SSL_FALLBACK_BEYOND_MINIMUM_VERSION,
    SUGGEST_NONE,
   },
@@ -355,7 +359,7 @@ const LocalizedErrorMap http_error_options[] = {
    IDS_ERRORPAGES_HEADING_ACCESS_DENIED,
    IDS_ERRORPAGES_SUMMARY_FORBIDDEN,
    IDS_ERRORPAGES_DETAILS_FORBIDDEN,
-   SUGGEST_NONE,
+   SUGGEST_RELOAD,
   },
   {410,
    IDS_ERRORPAGES_TITLE_NOT_FOUND,
@@ -431,7 +435,7 @@ const LocalizedErrorMap dns_probe_error_options[] = {
    IDS_ERRORPAGES_HEADING_INTERNET_DISCONNECTED,
    IDS_ERRORPAGES_SUMMARY_INTERNET_DISCONNECTED,
    IDS_ERRORPAGES_DETAILS_INTERNET_DISCONNECTED,
-   SUGGEST_CHECK_CONNECTION | SUGGEST_FIREWALL_CONFIG,
+   SUGGEST_OFFLINE_CHECKS,
   },
   {error_page::DNS_PROBE_FINISHED_BAD_CONFIG,
    IDS_ERRORPAGES_TITLE_NOT_AVAILABLE,
@@ -511,11 +515,11 @@ const char* GetIconClassForError(const std::string& error_domain,
 
 // If the first suggestion is for a Google cache copy link. Promote the
 // suggestion to a separate set of strings for displaying as a button.
-void AddGoogleCachedCopyButton(base::ListValue* suggestions,
+void AddGoogleCachedCopyButton(base::ListValue* suggestions_summary_list,
                                base::DictionaryValue* error_strings) {
-  if (!suggestions->empty()) {
+  if (!suggestions_summary_list->empty()) {
     base::DictionaryValue* suggestion;
-    suggestions->GetDictionary(0, &suggestion);
+    suggestions_summary_list->GetDictionary(0, &suggestion);
     int type = -1;
     suggestion->GetInteger("type", &type);
 
@@ -534,11 +538,289 @@ void AddGoogleCachedCopyButton(base::ListValue* suggestions,
 
       // Remove the item from suggestions dictionary so that it does not get
       // displayed by the template in the details section.
-      suggestions->Remove(0, nullptr);
+      suggestions_summary_list->Remove(0, nullptr);
     }
   }
 }
 
+// Creates a list of suggestions that a user may try to resolve a particular
+// network error. Appears above the fold underneath heading and intro paragraph.
+void GetSuggestionsSummaryList(int error_code,
+                               base::DictionaryValue* error_strings,
+                               int suggestions,
+                               const std::string& locale,
+                               base::ListValue* suggestions_summary_list) {
+  if (suggestions & SUGGEST_CHECK_CONNECTION) {
+    base::DictionaryValue* suggestion_list_item = new base::DictionaryValue;
+    suggestion_list_item->SetString("summary",
+        l10n_util::GetStringUTF16(
+            IDS_ERRORPAGES_SUGGESTION_CHECK_CONNECTION_SUMMARY));
+    suggestions_summary_list->Append(suggestion_list_item);

[mmenke, 2016/02/18 19:51:18]

Hrm...We do this so much, may be worth adding a helper function.  Could either
do something like "AddSingleEntryDictionary(suggestion_list_item, "summary",
IDS_ERRORPAGES_SUGGESTION_CHECK_CONNECTION_SUMMARY);" or just simplify the
SetString call...Guess it doesn't really get us that much.

[edwardjung, 2016/02/19 18:18:22]

Done. Created two helper functions to help here and in the details suggestions
function.

+  }
+
+  if (suggestions & SUGGEST_DNS_CONFIG &&
+      suggestions & SUGGEST_FIREWALL_CONFIG) {

[mmenke, 2016/02/18 19:51:19]

This is correct, but I'd suggest doing:
(suggestions & SUGGEST_DNS_CONFIG) &&
(suggestions & SUGGEST_FIREWALL_CONFIG).

Operator order for bitwise operations is weird, so think it's best not to have
code rely on it.

[edwardjung, 2016/02/19 18:18:21]

Done.

+    base::DictionaryValue* suggestion_list_item = new base::DictionaryValue;
+    suggestion_list_item->SetString("summary",
+        l10n_util::GetStringUTF16(
+            IDS_ERRORPAGES_SUGGESTION_CHECK_PROXY_FIREWALL_DNS_SUMMARY));

[mmenke, 2016/02/18 19:51:19]

So if we want to suggest checking DNS and firewall, we throw in proxy for free? 
(Note that DNS_PROBE_FINISHED_BAD_CONFIG has DNS and firewall bits set, but not
proxy).

[edwardjung, 2016/02/19 18:18:22]

Made it clearer specifying the proxy suggestion in the logic. 

+    suggestions_summary_list->Append(suggestion_list_item);
+  } else if (suggestions & SUGGEST_FIREWALL_CONFIG &&
+             suggestions & SUGGEST_AV_CONFIG) {
+    base::DictionaryValue* suggestion_list_item = new base::DictionaryValue;
+    suggestion_list_item->SetString("summary",
+        l10n_util::GetStringUTF16(
+            IDS_ERRORPAGES_SUGGESTION_CHECK_FIREWALL_ANTIVIRUS_SUMMARY));

[mmenke, 2016/02/18 19:51:18]

AV vs ANTIVIRUS - should use consistent naming.

[edwardjung, 2016/02/19 18:18:22]

Changed to ANTIVIRUS

+    suggestions_summary_list->Append(suggestion_list_item);
+  } else if (suggestions & SUGGEST_PROXY_CONFIG &&
+             suggestions & SUGGEST_FIREWALL_CONFIG) {
+    base::DictionaryValue* suggestion_list_item = new base::DictionaryValue;
+    suggestion_list_item->SetString("summary",
+        l10n_util::GetStringUTF16(
+            IDS_ERRORPAGES_SUGGESTION_CHECK_PROXY_FIREWALL_SUMMARY));
+    suggestions_summary_list->Append(suggestion_list_item);
+  } else if (suggestions & SUGGEST_PROXY_CONFIG) {
+    base::DictionaryValue* suggestion_list_item = new base::DictionaryValue;
+    suggestion_list_item->SetString("summary",
+        l10n_util::GetStringUTF16(
+            IDS_ERRORPAGES_SUGGESTION_CHECK_PROXY_ADDRESS_SUMMARY));
+    suggestions_summary_list->Append(suggestion_list_item);
+  }
+
+  if (suggestions & SUGGEST_CONTACT_ADMINISTRATOR) {
+    base::DictionaryValue* suggestion_list_item = new base::DictionaryValue;
+    if (error_code == net::ERR_PROXY_CONNECTION_FAILED) {
+      // Unprefixed suggestion which occurs in a list.
+      suggestion_list_item->SetString("summary",
+          l10n_util::GetStringUTF16(
+              IDS_ERRORPAGES_SUGGESTION_CONTACT_ADMIN_SUMMARY));
+    } else {
+      // Standalone suggestion which includes 'Try' prefix to the sentence.
+      suggestion_list_item->SetString("summary",
+          l10n_util::GetStringUTF16(
+              IDS_ERRORPAGES_SUGGESTION_CONTACT_ADMIN_SUMMARY_WITH_PREFIX));

[mmenke, 2016/02/18 19:51:19]

The fact that this one magic suggestion appears alone, and requires us to be
displaying no other suggestions to work suggest to me that we put this case
first, and have an early return to make sure this suggestion never regresses.

Actually, since it's so magic, suggest a separate SUGGEST_BLAH value for it as
well, rather than adding the ERR_PROXY_CONNECTION_FAILED check here.  Or could
check (suggestions & ~SUGGEST_CONTACT_ADMINISTRATOR), too.  The
ERR_PROXY_CONNECTION_FAILED check seems very regression prone.

[edwardjung, 2016/02/19 18:18:22]

Added a separate suggestion to reduce reliance on checking against error code.

+    }
+    suggestions_summary_list->Append(suggestion_list_item);
+  }
+
+  if (suggestions & SUGGEST_LEARNMORE) {
+    GURL learn_more_url;
+    base::string16 suggestion_string;
+    switch (error_code) {
+      case net::ERR_SSL_WEAK_SERVER_EPHEMERAL_DH_KEY:
+        learn_more_url = GURL(kWeakDHKeyLearnMoreUrl);
+        suggestion_string = l10n_util::GetStringUTF16(
+            IDS_ERRORPAGES_SUGGESTION_LEARNMORE_BODY);

[mmenke, 2016/02/18 19:51:18]

BODY -> SUMMARY

[edwardjung, 2016/02/19 18:18:22]

Done.

+        break;
+      case net::ERR_TOO_MANY_REDIRECTS:
+        learn_more_url = GURL(kRedirectLoopLearnMoreUrl);
+        suggestion_string = l10n_util::GetStringUTF16(
+              IDS_ERRORPAGES_SUGGESTION_CLEAR_COOKIES_SUMMARY);
+        break;
+      default:
+        break;
+    }
+
+    if (learn_more_url.is_valid()) {
+      // Add the language parameter to the URL.
+      std::string query = learn_more_url.query() + "&hl=" + locale;
+      GURL::Replacements repl;
+      repl.SetQueryStr(query);
+      learn_more_url = learn_more_url.ReplaceComponents(repl);
+
+      base::DictionaryValue* suggestion_list_item = new base::DictionaryValue;
+      suggestion_list_item->SetString("summary", suggestion_string);
+      suggestion_list_item->SetString("learnMoreUrl", learn_more_url.spec());
+      suggestions_summary_list->Append(suggestion_list_item);
+    }
+  }
+
+  if (suggestions & SUGGEST_OFFLINE_CHECKS) {
+#if defined(OS_ANDROID) or defined(OS_IOS)
+    base::DictionaryValue* suggestion_list_item_airplane =
+        new base::DictionaryValue;
+    suggestion_list_item_airplane->SetString("summary",
+        l10n_util::GetStringUTF16(
+            IDS_ERRORPAGES_SUGGESTION_TURN_OFF_AIRPLANE));

[mmenke, 2016/02/18 19:51:18]

_SUMMARY?

[edwardjung, 2016/02/19 18:18:22]

Done.

+    suggestions_summary_list->Append(suggestion_list_item_airplane);
+
+    base::DictionaryValue* suggestion_list_item_data =
+        new base::DictionaryValue;
+    suggestion_list_item_data->SetString("summary",
+        l10n_util::GetStringUTF16(
+            IDS_ERRORPAGES_SUGGESTION_TURN_ON_DATA));

[mmenke, 2016/02/18 19:51:19]

_SUMMARY?

[edwardjung, 2016/02/19 18:18:22]

Done.

+    suggestions_summary_list->Append(suggestion_list_item_data);
+
+    base::DictionaryValue* suggestion_list_item_signal =
+        new base::DictionaryValue;
+    suggestion_list_item_signal->SetString("summary",
+        l10n_util::GetStringUTF16(
+            IDS_ERRORPAGES_SUGGESTION_CHECKING_SIGNAL));

[mmenke, 2016/02/18 19:51:18]

_SUMMARY?

[edwardjung, 2016/02/19 18:18:22]

Done.

+    suggestions_summary_list->Append(suggestion_list_item_signal);
+#else
+    base::DictionaryValue* suggestion_list_item_cables =
+        new base::DictionaryValue;
+    suggestion_list_item_cables->SetString("summary",
+        l10n_util::GetStringUTF16(
+            IDS_ERRORPAGES_SUGGESTION_CHECK_CABLES_SUMMARY));
+    suggestions_summary_list->Append(suggestion_list_item_cables);
+
+    base::DictionaryValue* suggestion_list_item_hardware =
+        new base::DictionaryValue;
+    suggestion_list_item_hardware->SetString("summary",
+        l10n_util::GetStringUTF16(
+            IDS_ERRORPAGES_SUGGESTION_RESET_HARDWARE_SUMMARY));
+    suggestions_summary_list->Append(suggestion_list_item_hardware);
+
+    base::DictionaryValue* suggestion_list_item_wifi =
+         new base::DictionaryValue;
+    suggestion_list_item_wifi->SetString("summary",
+        l10n_util::GetStringUTF16(
+            IDS_ERRORPAGES_SUGGESTION_CHECK_WIFI_SUMMARY));
+    suggestions_summary_list->Append(suggestion_list_item_wifi);
+#endif
+  }
+
+  if (suggestions & SUGGEST_COMPLETE_SETUP) {
+    base::DictionaryValue* suggestion_diagnose = new base::DictionaryValue;
+    suggestion_diagnose->SetString("summary",
+        l10n_util::GetStringUTF16(
+            IDS_ERRORPAGES_SUGGESTION_DIAGNOSE_CONNECTION_SUMMARY));
+    suggestions_summary_list->Append(suggestion_diagnose);
+
+    base::DictionaryValue* suggestion_complete_setup =
+        new base::DictionaryValue;
+    suggestion_complete_setup->SetString("summary",
+        l10n_util::GetStringUTF16(
+            IDS_ERRORPAGES_SUGGESTION_COMPLETE_SETUP_SUMMARY));
+    suggestions_summary_list->Append(suggestion_complete_setup);
+  }
+
+  if (suggestions & SUGGEST_DISABLE_EXTENSION) {
+    base::DictionaryValue* suggest_disable_extension =
+        new base::DictionaryValue;
+    suggest_disable_extension->SetString("summary",
+        l10n_util::GetStringUTF16(
+            IDS_ERRORPAGES_SUGGESTION_DISABLE_EXTENSION_SUMMARY));
+    suggestions_summary_list->Append(suggest_disable_extension);
+  }
+
+  // Only add a explicit reload suggestion if there are other suggestions.
+  // Otherwise we rely on the reload button being used.

[mmenke, 2016/02/18 19:51:19]

Don't use "we" in comments.

[edwardjung, 2016/02/19 18:18:22]

Done.

+  if (suggestions_summary_list->GetSize() > 0 && suggestions & SUGGEST_RELOAD) {

[mmenke, 2016/02/18 19:51:18]

!suggestions_summary_list->GetSize().empty()

[edwardjung, 2016/02/19 18:18:22]

Done.

+    base::DictionaryValue* suggestion_list_item = new base::DictionaryValue;
+    suggestion_list_item->SetString("summary",
+        l10n_util::GetStringUTF16(IDS_ERRORPAGES_SUGGESTION_RELOAD_SUMMARY));
+    // Add as first item.
+    suggestions_summary_list->Insert(0, suggestion_list_item);
+  }
+
+  // Add list prefix header if there are suggestions except
+  // for the 'Incomplete enrollment setup'.

[mmenke, 2016/02/18 19:51:18]

This comment doesn't seem quite correct.  It should be "Add list prefix if there
are multiple suggestions and no suggestion is to complete the enrollment setup."

Did you really want >= 1?  Otherwise, having any of the suggestions that are in
gerund form without the "try" prefix seems weird.

[edwardjung, 2016/02/19 18:18:22]

Corrected
For all single suggestions they would have 'Try' in the sentence. There is only
one instance of this -SUGGEST_CONTACT_ADMINISTRATOR_STANDALONE. All the other
cases where we only suggest reload, only the button is displayed, no suggestion
text.

For single suggestions the list formatting is removed and the suggestion is just
in a paragraph.

+  if (suggestions_summary_list->GetSize() > 1 &&
+      !(suggestions & SUGGEST_COMPLETE_SETUP)) {
+    error_strings->SetString("suggestionsSummaryListHeader",
+            l10n_util::GetStringUTF16(IDS_ERRORPAGES_SUGGESTION_LIST_HEADER));
+  }
+}
+
+// Certain suggestions have supporting details which get displayed under
+// the "Details" button.
+void AddSuggestionsDetails(int error_code,
+                           base::DictionaryValue* error_strings,
+                           int suggestions,
+                           base::ListValue* suggestions_details) {
+

[mmenke, 2016/02/18 19:51:18]

nit:  Remove blank lines.

[edwardjung, 2016/02/19 18:18:22]

Done.

+  if (suggestions & SUGGEST_CHECK_CONNECTION) {
+    base::DictionaryValue* suggest_check_connection = new base::DictionaryValue;
+    suggest_check_connection->SetString("header",
+        l10n_util::GetStringUTF16(
+            IDS_ERRORPAGES_SUGGESTION_CHECK_CONNECTION_HEADER));
+    suggest_check_connection->SetString("body",
+        l10n_util::GetStringUTF16(
+            IDS_ERRORPAGES_SUGGESTION_CHECK_CONNECTION_BODY));
+    suggestions_details->Append(suggest_check_connection);
+  }
+
+  if (suggestions & SUGGEST_DNS_CONFIG) {
+    base::DictionaryValue* suggest_dns_config = new base::DictionaryValue;
+    suggest_dns_config->SetString("header",
+        l10n_util::GetStringUTF16(
+            IDS_ERRORPAGES_SUGGESTION_DNS_CONFIG_HEADER));
+    suggest_dns_config->SetString("body",
+        l10n_util::GetStringUTF16(
+            IDS_ERRORPAGES_SUGGESTION_DNS_CONFIG_BODY));
+    suggestions_details->Append(suggest_dns_config);
+
+    base::DictionaryValue* suggest_network_prediction =
+        GetStandardMenuItemsText();
+    suggest_network_prediction->SetString("header",
+        l10n_util::GetStringUTF16(
+            IDS_ERRORPAGES_SUGGESTION_NETWORK_PREDICTION_HEADER));
+    suggest_network_prediction->SetString("body",
+        l10n_util::GetStringUTF16(
+            IDS_ERRORPAGES_SUGGESTION_NETWORK_PREDICTION_BODY));
+    suggest_network_prediction->SetString(
+        "noNetworkPredictionTitle",
+        l10n_util::GetStringUTF16(
+            IDS_NETWORK_PREDICTION_ENABLED_DESCRIPTION));
+    suggestions_details->Append(suggest_network_prediction);
+  }
+
+  if (suggestions & SUGGEST_FIREWALL_CONFIG) {
+    base::DictionaryValue* suggest_firewall_config = new base::DictionaryValue;
+    suggest_firewall_config->SetString("header",
+        l10n_util::GetStringUTF16(
+            IDS_ERRORPAGES_SUGGESTION_FIREWALL_CONFIG_HEADER));
+    suggest_firewall_config->SetString("body",
+        l10n_util::GetStringUTF16(
+            IDS_ERRORPAGES_SUGGESTION_FIREWALL_CONFIG_BODY));
+    suggestions_details->Append(suggest_firewall_config);
+  }
+
+  if (suggestions & SUGGEST_PROXY_CONFIG) {
+    base::DictionaryValue* suggest_proxy_config = GetStandardMenuItemsText();
+    suggest_proxy_config->SetString("header",
+        l10n_util::GetStringUTF16(
+            IDS_ERRORPAGES_SUGGESTION_PROXY_CONFIG_HEADER));
+    suggest_proxy_config->SetString("body",
+        l10n_util::GetStringFUTF16(IDS_ERRORPAGES_SUGGESTION_PROXY_CONFIG_BODY,
+            l10n_util::GetStringUTF16(
+                IDS_ERRORPAGES_SUGGESTION_PROXY_DISABLE_PLATFORM)));
+    suggest_proxy_config->SetString("proxyTitle",
+        l10n_util::GetStringUTF16(IDS_OPTIONS_PROXIES_CONFIGURE_BUTTON));
+
+    suggestions_details->Append(suggest_proxy_config);
+  }
+
+  if (suggestions & SUGGEST_VIEW_POLICIES) {
+    base::DictionaryValue* suggest_view_policies = new base::DictionaryValue;
+    suggest_view_policies->SetString(
+        "header",
+        l10n_util::GetStringUTF16(
+            IDS_ERRORPAGES_SUGGESTION_VIEW_POLICIES_HEADER));
+    suggest_view_policies->SetString(
+        "body",
+        l10n_util::GetStringUTF16(
+            IDS_ERRORPAGES_SUGGESTION_VIEW_POLICIES_BODY));
+    suggestions_details->Append(suggest_view_policies);
+  }
+
+  if (suggestions & SUGGEST_UNSUPPORTED_CIPHER) {
+    base::DictionaryValue* suggest_unsupported_cipher =
+        new base::DictionaryValue;
+    suggest_unsupported_cipher->SetString(
+        "body",
+        l10n_util::GetStringUTF16(
+            IDS_ERRORPAGES_SUGGESTION_UNSUPPORTED_CIPHER));
+    suggestions_details->Append(suggest_unsupported_cipher);

[mmenke, 2016/02/18 19:51:18]

Hrm...Know this was like that before, but this is the only one with no header. 
Should it just be a header instead, or is it correct as-is?

[edwardjung, 2016/02/19 18:18:22]

Yeah, headers are currently bold so we don't want the whole sentence in bold.
Checking with rachelis whether we should introduce a header. Seems to make
sense.

+  }
+}
+
+std::string HttpErrorCodeToString(int error) {
+  return std::string("HTTP ERROR ") + base::IntToString(error);
+}
+
 }  // namespace
 
 const char LocalizedError::kHttpErrorDomain[] = "http";
@@ -651,7 +933,7 @@ void LocalizedError::GetStrings(int error_code,
     error_string = base::ASCIIToUTF16(ascii_error_string);
   } else {
     DCHECK_EQ(LocalizedError::kHttpErrorDomain, error_domain);
-    error_string = base::IntToString16(error_code);
+    error_string = base::ASCIIToUTF16(HttpErrorCodeToString(error_code));
   }
   error_strings->SetString("errorCode", error_string);
 
@@ -689,23 +971,32 @@ void LocalizedError::GetStrings(int error_code,
     params->suggest_reload = !!(options.suggestions & SUGGEST_RELOAD);
   }
 
-  base::ListValue* suggestions = NULL;
+  base::ListValue* suggestions_details = NULL;
+  base::ListValue* suggestions_summary_list = NULL;
+
   bool use_default_suggestions = true;
   if (!params->override_suggestions) {
-    suggestions = new base::ListValue();
+    suggestions_details = new base::ListValue();
+    suggestions_summary_list = new base::ListValue();
+    // Detailed suggestion information.
+    error_strings->Set("suggestionsDetails", suggestions_details);
   } else {
-    suggestions = params->override_suggestions.release();
+    suggestions_summary_list = params->override_suggestions.release();
     use_default_suggestions = false;
-    AddGoogleCachedCopyButton(suggestions, error_strings);
+    AddGoogleCachedCopyButton(suggestions_summary_list, error_strings);
   }
-  error_strings->Set("suggestions", suggestions);
+  error_strings->Set("suggestionsSummaryList", suggestions_summary_list);
 
   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);
+    base::DictionaryValue* search_suggestion = new base::DictionaryValue;
+    search_suggestion->SetString("summary",
+          l10n_util::GetStringUTF16(IDS_ERRORPAGES_SUGGESTION_GOOGLE_SEARCH));

[mmenke, 2016/02/18 19:51:18]

+_SUMMARY?

[edwardjung, 2016/02/19 18:18:22]

Done.

+    search_suggestion->SetString("searchUrl", params->search_url.spec() +
+                                 params->search_terms);
+    search_suggestion->SetString("searchTerms", params->search_terms);
+    search_suggestion->SetInteger("trackingId",
+                                  params->search_tracking_id);
+    suggestions_summary_list->Append(search_suggestion);
   }
 
   // Add the reload suggestion, if needed.
@@ -723,15 +1014,13 @@ void LocalizedError::GetStrings(int error_code,
       // TODO(mmenke):  Make the reload button bring up the repost confirmation
       //                dialog for pages resulting from posts.
       base::DictionaryValue* suggest_reload_repost = new base::DictionaryValue;
-      suggest_reload_repost->SetString("header",
-          l10n_util::GetStringUTF16(
-              IDS_ERRORPAGES_SUGGESTION_RELOAD_REPOST_HEADER));
-      suggest_reload_repost->SetString("body",
+      suggest_reload_repost->SetString("summary",
           l10n_util::GetStringUTF16(
               IDS_ERRORPAGES_SUGGESTION_RELOAD_REPOST_BODY));

[mmenke, 2016/02/18 19:51:19]

_BODY -> _SUMMARY?

[edwardjung, 2016/02/19 18:18:22]

Done.

+
       // Add at the front, so it appears before other suggestions, in the case
       // suggestions are being overridden by |params|.
-      suggestions->Insert(0, suggest_reload_repost);
+      suggestions_summary_list->Insert(0, suggest_reload_repost);
     }
   }
 
@@ -739,6 +1028,13 @@ void LocalizedError::GetStrings(int error_code,
   if (!use_default_suggestions)
     return;
 
+  // Add default suggestions and any relevant supporting details.
+  GetSuggestionsSummaryList(error_code, error_strings, options.suggestions,
+                            locale, suggestions_summary_list);
+  AddSuggestionsDetails(error_code, error_strings, options.suggestions,
+                        suggestions_details);
+
+  // Add action buttons.
   const std::string& show_saved_copy_value =
       command_line->GetSwitchValueASCII(error_page::switches::kShowSavedCopy);
   bool show_saved_copy_primary =
@@ -800,143 +1096,6 @@ void LocalizedError::GetStrings(int error_code,
     error_strings->SetString(
         "diagnose", l10n_util::GetStringUTF16(IDS_ERRORPAGES_BUTTON_DIAGNOSE));
   }
-
-  if (options.suggestions & SUGGEST_CHECK_CONNECTION) {
-    base::DictionaryValue* suggest_check_connection = new base::DictionaryValue;
-    suggest_check_connection->SetString("header",
-        l10n_util::GetStringUTF16(
-            IDS_ERRORPAGES_SUGGESTION_CHECK_CONNECTION_HEADER));
-    suggest_check_connection->SetString("body",
-        l10n_util::GetStringUTF16(
-            IDS_ERRORPAGES_SUGGESTION_CHECK_CONNECTION_BODY));
-    suggestions->Append(suggest_check_connection);
-  }
-
-  if (options.suggestions & SUGGEST_DNS_CONFIG) {
-    base::DictionaryValue* suggest_dns_config = new base::DictionaryValue;
-    suggest_dns_config->SetString("header",
-        l10n_util::GetStringUTF16(
-            IDS_ERRORPAGES_SUGGESTION_DNS_CONFIG_HEADER));
-    suggest_dns_config->SetString("body",
-        l10n_util::GetStringUTF16(
-            IDS_ERRORPAGES_SUGGESTION_DNS_CONFIG_BODY));
-    suggestions->Append(suggest_dns_config);
-
-    base::DictionaryValue* suggest_network_prediction =
-        GetStandardMenuItemsText();
-    suggest_network_prediction->SetString("header",
-        l10n_util::GetStringUTF16(
-            IDS_ERRORPAGES_SUGGESTION_NETWORK_PREDICTION_HEADER));
-    suggest_network_prediction->SetString("body",
-        l10n_util::GetStringUTF16(
-            IDS_ERRORPAGES_SUGGESTION_NETWORK_PREDICTION_BODY));
-    suggest_network_prediction->SetString(
-        "noNetworkPredictionTitle",
-        l10n_util::GetStringUTF16(
-            IDS_NETWORK_PREDICTION_ENABLED_DESCRIPTION));
-    suggestions->Append(suggest_network_prediction);
-  }
-
-  // TODO(crbug.com/584615): Does it make sense to show all of these
-  // suggestions on mobile? Several of them seem irrelevant in the mobile
-  // context.
-  if (options.suggestions & SUGGEST_FIREWALL_CONFIG) {
-    base::DictionaryValue* suggest_firewall_config = new base::DictionaryValue;
-    suggest_firewall_config->SetString("header",
-        l10n_util::GetStringUTF16(
-            IDS_ERRORPAGES_SUGGESTION_FIREWALL_CONFIG_HEADER));
-    suggest_firewall_config->SetString("body",
-        l10n_util::GetStringUTF16(
-            IDS_ERRORPAGES_SUGGESTION_FIREWALL_CONFIG_BODY));
-    suggestions->Append(suggest_firewall_config);
-  }
-
-  if (options.suggestions & SUGGEST_PROXY_CONFIG) {
-    base::DictionaryValue* suggest_proxy_config = GetStandardMenuItemsText();
-    suggest_proxy_config->SetString("header",
-        l10n_util::GetStringUTF16(
-            IDS_ERRORPAGES_SUGGESTION_PROXY_CONFIG_HEADER));
-    suggest_proxy_config->SetString("body",
-        l10n_util::GetStringFUTF16(IDS_ERRORPAGES_SUGGESTION_PROXY_CONFIG_BODY,
-            l10n_util::GetStringUTF16(
-                IDS_ERRORPAGES_SUGGESTION_PROXY_DISABLE_PLATFORM)));
-    suggest_proxy_config->SetString("proxyTitle",
-        l10n_util::GetStringUTF16(IDS_OPTIONS_PROXIES_CONFIGURE_BUTTON));
-
-    suggestions->Append(suggest_proxy_config);
-  }
-
-  if (options.suggestions & SUGGEST_DISABLE_EXTENSION) {
-    base::DictionaryValue* suggest_disable_extension =
-        new base::DictionaryValue;
-    // There's only a header for this suggestion.
-    suggest_disable_extension->SetString("header",
-        l10n_util::GetStringUTF16(
-            IDS_ERRORPAGES_SUGGESTION_DISABLE_EXTENSION_HEADER));
-    suggestions->Append(suggest_disable_extension);
-  }
-
-  if (options.suggestions & SUGGEST_VIEW_POLICIES) {
-    base::DictionaryValue* suggest_view_policies = new base::DictionaryValue;
-    suggest_view_policies->SetString(
-        "header",
-        l10n_util::GetStringUTF16(
-            IDS_ERRORPAGES_SUGGESTION_VIEW_POLICIES_HEADER));
-    suggest_view_policies->SetString(
-        "body",
-        l10n_util::GetStringUTF16(
-            IDS_ERRORPAGES_SUGGESTION_VIEW_POLICIES_BODY));
-    suggestions->Append(suggest_view_policies);
-  }
-
-  if (options.suggestions & SUGGEST_CONTACT_ADMINISTRATOR) {
-    base::DictionaryValue* suggest_contant_administrator =
-        new base::DictionaryValue;
-    suggest_contant_administrator->SetString(
-        "body",
-        l10n_util::GetStringUTF16(
-            IDS_ERRORPAGES_SUGGESTION_CONTACT_ADMINISTRATOR_BODY));
-    suggestions->Append(suggest_contant_administrator);
-  }
-
-  if (options.suggestions & SUGGEST_LEARNMORE) {
-    GURL learn_more_url;
-    switch (options.error_code) {
-      case net::ERR_TOO_MANY_REDIRECTS:
-        learn_more_url = GURL(kRedirectLoopLearnMoreUrl);
-        break;
-      case net::ERR_SSL_WEAK_SERVER_EPHEMERAL_DH_KEY:
-        learn_more_url = GURL(kWeakDHKeyLearnMoreUrl);
-        break;
-      default:
-        break;
-    }
-
-    if (learn_more_url.is_valid()) {
-      // Add the language parameter to the URL.
-      std::string query = learn_more_url.query() + "&hl=" + locale;
-      GURL::Replacements repl;
-      repl.SetQueryStr(query);
-      learn_more_url = learn_more_url.ReplaceComponents(repl);
-
-      base::DictionaryValue* suggest_learn_more = new base::DictionaryValue;
-      // There's only a body for this suggestion.
-      suggest_learn_more->SetString("body",
-          l10n_util::GetStringUTF16(IDS_ERRORPAGES_SUGGESTION_LEARNMORE_BODY));
-      suggest_learn_more->SetString("learnMoreUrl", learn_more_url.spec());
-      suggestions->Append(suggest_learn_more);
-    }
-  }
-
-  if (options.suggestions & SUGGEST_UNSUPPORTED_CIPHER) {
-    base::DictionaryValue* suggest_unsupported_cipher =
-        new base::DictionaryValue;
-    suggest_unsupported_cipher->SetString(
-        "body",
-        l10n_util::GetStringUTF16(
-            IDS_ERRORPAGES_SUGGESTION_UNSUPPORTED_CIPHER));
-    suggestions->Append(suggest_unsupported_cipher);
-  }
 }
 
 base::string16 LocalizedError::GetErrorDetails(const std::string& error_domain,