Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1639953002:
Network error interstitial update - add suggestions list (Closed)
|
Created:
4 years, 11 months ago by edwardjung Modified:
4 years, 10 months ago CC:
chromium-reviews, davidben Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNetwork error interstitial update - add suggestions list
+ Add suggestions list to summary area
+ Merge Link Doctor suggestions into the main suggestions list.
BUG=518975, 589405
Committed: https://crrev.com/f3d772c9d29ea403856a804b0bc136150d844a0f
Cr-Commit-Position: refs/heads/master@{#377784}
Patch Set 1 #Patch Set 2 : Move link doctor suggestions from below details section #Patch Set 3 : Clean up styling #
Total comments: 14
Patch Set 4 : Address review comments #Patch Set 5 : Fix browser tests #Patch Set 6 : Merge, neterrors moved to components #Patch Set 7 : #
Total comments: 56
Patch Set 8 : Address review comments + Fix generic icon #
Total comments: 7
Patch Set 9 : Return standalone suggestions early. #
Total comments: 34
Patch Set 10 : #
Total comments: 3
Patch Set 11 : Add extra DCHECKS for standalone suggestions #Patch Set 12 : Fix patch errors - remove c/r/resources/neterror.css #
Total comments: 1
Patch Set 13 : Fix the move of neterror.css #Patch Set 14 : Fix iOS/Android bug #Patch Set 15 : Fix nit #Patch Set 16 : Fix browser and unit tests #
Total comments: 2
Patch Set 17 : Remove view policies suggestion #
Total comments: 6
Patch Set 18 : Remove redundant DCHECKs #Messages
Total messages: 36 (12 generated)
Description was changed from ========== Network error interstitial update phase 2 - Add suggestions list to summary area BUG=518975 ========== to ========== Network error interstitial update - add suggestions list + Add suggestions list to summary area + Merge Link Doctor suggestions into the main suggestions list. BUG=518975 ==========
edwardjung@chromium.org changed reviewers: + mmenke@chromium.org
Mmenke, please take a look at this suggestions list implementation. In addition to the suggestions list we went through, the link doctor suggestions are now surfaced as a list when the service is enabled.
On 2016/02/11 13:33:07, edwardjung wrote: > Mmenke, please take a look at this suggestions list implementation. In addition > to the suggestions list we went through, the link doctor suggestions are now > surfaced as a list when the service is enabled. Don't think I'll get to this today - should have time tomorrow.
https://codereview.chromium.org/1639953002/diff/40001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.html (right): https://codereview.chromium.org/1639953002/diff/40001/chrome/renderer/resourc... chrome/renderer/resources/neterror.html:25: <li jsselect="suggestionsList" jsvalues=".innerHTML:summary" jsdisplay="summary"></li> I don't think this jsdisplay gets us anything? At least I hope we're not adding empty suggestions to the list. https://codereview.chromium.org/1639953002/diff/40001/components/error_page/c... File components/error_page/common/localized_error.cc (right): https://codereview.chromium.org/1639953002/diff/40001/components/error_page/c... components/error_page/common/localized_error.cc:887: error_strings->Set("suggestions", suggestions); Erm...Using "suggestions" in some cases and "suggestionsList" in others, with suggestions being below the fold, and suggestionsList being above seems a bit weird to me, for two reasons: 1) If I'm reading this right, we're displaying the override suggestions above the fold, and the default ones below the fold? This should be documents with "override_suggestions"...And it should probably be renamed as well (above_the_fold_suggestions?) 2) Those are really bad names. If we really want to do this, maybe "suggestions" / "suggestionsList" -> "belowTheFoldSuggestions" / "aboveTheFoldSuggstions"? https://codereview.chromium.org/1639953002/diff/40001/components/error_page/c... components/error_page/common/localized_error.cc:896: base::DictionaryValue* suggestion_search = new base::DictionaryValue; search_suggestion? https://codereview.chromium.org/1639953002/diff/40001/components/error_page/c... components/error_page/common/localized_error.cc:939: options.suggestions, locale); Use braces https://codereview.chromium.org/1639953002/diff/40001/components/error_page/c... components/error_page/common/localized_error.cc:939: options.suggestions, locale); This code is just too confusing. I'm really having trouble figuring out what's going on, or what we have two sets of strings for some random subset of suggestions. Some one have a below the fold suggestion and some only have an above the fold suggestion? https://codereview.chromium.org/1639953002/diff/40001/components/error_page_s... File components/error_page_strings.grdp (right): https://codereview.chromium.org/1639953002/diff/40001/components/error_page_s... components/error_page_strings.grdp:196: <ph name="HOST_NAME"><strong jscontent="hostName"></strong><ex>www.whatever.com</ex></ph>’s server <ph name="BEGIN_ABBR"><abbr jsvalues="title:dnsDefinition"></ph>DNS address<ph name="END_ABBR"></abbr></ph> could not be found. I'm just going to assume this is right. https://codereview.chromium.org/1639953002/diff/40001/net/base/net_errors.h File net/base/net_errors.h (right): https://codereview.chromium.org/1639953002/diff/40001/net/base/net_errors.h#n... net/base/net_errors.h:40: NET_EXPORT std::string HttpErrorCodeToString(int error); Do we really need this in net? Any reason not to use a friendly formatted string for these? Just to be consistent with the unfriendly net error strings?
Yes it's got pretty confusing. Really it's a single set of suggestions. In some cases the suggestion is sufficiently complicated that we provide extra details (especially in those circumstances when you're offline). Screenshots here might help. https://code.google.com/p/chromium/issues/detail?id=518975#c17 I tried to make it easier to understand. Open to other suggestions , extra rearranging of the code, more comments. https://codereview.chromium.org/1639953002/diff/40001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.html (right): https://codereview.chromium.org/1639953002/diff/40001/chrome/renderer/resourc... chrome/renderer/resources/neterror.html:25: <li jsselect="suggestionsList" jsvalues=".innerHTML:summary" jsdisplay="summary"></li> On 2016/02/12 17:25:54, mmenke wrote: > I don't think this jsdisplay gets us anything? At least I hope we're not adding > empty suggestions to the list. True, removed https://codereview.chromium.org/1639953002/diff/40001/components/error_page/c... File components/error_page/common/localized_error.cc (right): https://codereview.chromium.org/1639953002/diff/40001/components/error_page/c... components/error_page/common/localized_error.cc:887: error_strings->Set("suggestions", suggestions); On 2016/02/12 17:25:54, mmenke wrote: > Erm...Using "suggestions" in some cases and "suggestionsList" in others, with > suggestions being below the fold, and suggestionsList being above seems a bit > weird to me, for two reasons: > > 1) If I'm reading this right, we're displaying the override suggestions above > the fold, and the default ones below the fold? This should be documents with > "override_suggestions"...And it should probably be renamed as well > (above_the_fold_suggestions?) Not quite, override suggestions appear above the fold as a list. Some of the suggestions have extra explanation / details which appear under the details link. > 2) Those are really bad names. If we really want to do this, maybe > "suggestions" / "suggestionsList" -> "belowTheFoldSuggestions" / > "aboveTheFoldSuggstions"? I renamed them to 'suggestionsSummaryList' and 'suggestionsDetails' to make it clearer. https://codereview.chromium.org/1639953002/diff/40001/components/error_page/c... components/error_page/common/localized_error.cc:896: base::DictionaryValue* suggestion_search = new base::DictionaryValue; On 2016/02/12 17:25:54, mmenke wrote: > search_suggestion? Done. https://codereview.chromium.org/1639953002/diff/40001/components/error_page/c... components/error_page/common/localized_error.cc:939: options.suggestions, locale); On 2016/02/12 17:25:54, mmenke wrote: > Use braces Done. https://codereview.chromium.org/1639953002/diff/40001/components/error_page/c... components/error_page/common/localized_error.cc:939: options.suggestions, locale); On 2016/02/12 17:25:54, mmenke wrote: > This code is just too confusing. I'm really having trouble figuring out what's > going on, or what we have two sets of strings for some random subset of > suggestions. Some one have a below the fold suggestion and some only have an > above the fold suggestion? Valid point. I've moved the suggestions details into it's own function. I'm considering moving all the call to action button string assignment to another function too. Do you think that's worth doing? https://codereview.chromium.org/1639953002/diff/40001/components/error_page_s... File components/error_page_strings.grdp (right): https://codereview.chromium.org/1639953002/diff/40001/components/error_page_s... components/error_page_strings.grdp:196: <ph name="HOST_NAME"><strong jscontent="hostName"></strong><ex>www.whatever.com</ex></ph>’s server <ph name="BEGIN_ABBR"><abbr jsvalues="title:dnsDefinition"></ph>DNS address<ph name="END_ABBR"></abbr></ph> could not be found. On 2016/02/12 17:25:54, mmenke wrote: > I'm just going to assume this is right. This was a correction to the current format where there was confusion around BEGIN_ABBR reported by translators, this change exposes the string "DNS address". From: HOST_NAME’s server BEGIN_ABBR could not be found. To: HOST_NAME’s server BEGIN_ABBR DNS adddress END_ABBR could not be found. https://codereview.chromium.org/1639953002/diff/40001/net/base/net_errors.h File net/base/net_errors.h (right): https://codereview.chromium.org/1639953002/diff/40001/net/base/net_errors.h#n... net/base/net_errors.h:40: NET_EXPORT std::string HttpErrorCodeToString(int error); On 2016/02/12 17:25:54, mmenke wrote: > Do we really need this in net? Any reason not to use a friendly formatted > string for these? Just to be consistent with the unfriendly net error strings? I guess not. Confirmed with rachelis to change to "HTTP ERROR 123"
Sorry for slowness, I'm running a bit behind on reviews. I'll try to get to this today.
First pass. Want to do another careful pass tomorrow, and if there's anything to resolve after that, quick skims at changes should be fine. https://codereview.chromium.org/1639953002/diff/120001/chrome/browser/errorpa... File chrome/browser/errorpage_browsertest.cc (right): https://codereview.chromium.org/1639953002/diff/120001/chrome/browser/errorpa... chrome/browser/errorpage_browsertest.cc:131: ToggleHelpBox(browser); Not needed any more, since the next function doesn't use it? https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... File components/error_page/common/localized_error.cc (right): https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... components/error_page/common/localized_error.cc:105: SUGGEST_RELOAD, SUGGEST_CHECK_CONNECTION | SUGGEST_FIREWALL_CONFIG | 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. https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... components/error_page/common/localized_error.cc:558: suggestions_summary_list->Append(suggestion_list_item); 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. https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... components/error_page/common/localized_error.cc:562: suggestions & SUGGEST_FIREWALL_CONFIG) { 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. https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... components/error_page/common/localized_error.cc:566: IDS_ERRORPAGES_SUGGESTION_CHECK_PROXY_FIREWALL_DNS_SUMMARY)); 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). https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... components/error_page/common/localized_error.cc:573: IDS_ERRORPAGES_SUGGESTION_CHECK_FIREWALL_ANTIVIRUS_SUMMARY)); AV vs ANTIVIRUS - should use consistent naming. https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... components/error_page/common/localized_error.cc:601: IDS_ERRORPAGES_SUGGESTION_CONTACT_ADMIN_SUMMARY_WITH_PREFIX)); 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. https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... components/error_page/common/localized_error.cc:613: IDS_ERRORPAGES_SUGGESTION_LEARNMORE_BODY); BODY -> SUMMARY https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... components/error_page/common/localized_error.cc:644: IDS_ERRORPAGES_SUGGESTION_TURN_OFF_AIRPLANE)); _SUMMARY? https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... components/error_page/common/localized_error.cc:651: IDS_ERRORPAGES_SUGGESTION_TURN_ON_DATA)); _SUMMARY? https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... components/error_page/common/localized_error.cc:658: IDS_ERRORPAGES_SUGGESTION_CHECKING_SIGNAL)); _SUMMARY? https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... components/error_page/common/localized_error.cc:709: // Otherwise we rely on the reload button being used. Don't use "we" in comments. https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... components/error_page/common/localized_error.cc:710: if (suggestions_summary_list->GetSize() > 0 && suggestions & SUGGEST_RELOAD) { !suggestions_summary_list->GetSize().empty() https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... components/error_page/common/localized_error.cc:719: // for the 'Incomplete enrollment setup'. 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. https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... components/error_page/common/localized_error.cc:733: nit: Remove blank lines. https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... components/error_page/common/localized_error.cc:816: suggestions_details->Append(suggest_unsupported_cipher); 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? https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... components/error_page/common/localized_error.cc:993: l10n_util::GetStringUTF16(IDS_ERRORPAGES_SUGGESTION_GOOGLE_SEARCH)); +_SUMMARY? https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... components/error_page/common/localized_error.cc:1019: IDS_ERRORPAGES_SUGGESTION_RELOAD_REPOST_BODY)); _BODY -> _SUMMARY? https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... File components/error_page/renderer/net_error_helper_core.cc (right): https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... components/error_page/renderer/net_error_helper_core.cc:50: const int kUrlCorrectionsToDisplay = 1; +kMaxUrl...? https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... components/error_page/renderer/net_error_helper_core.cc:342: if (params->override_suggestions->GetSize() == kUrlCorrectionsToDisplay) { Think this should be >=, for clarity. Also, this seems to be in the wrong place. It should be before this loop, next to the url_correction.empty() check - this loop is looking up what to do for the current correction, it's not looping through all the corrections. Note that it already has a "break;" as the last line. https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... components/error_page/renderer/net_error_helper_core.cc:344: } Remove braces https://codereview.chromium.org/1639953002/diff/120001/components/error_page_... File components/error_page_strings.grdp (right): https://codereview.chromium.org/1639953002/diff/120001/components/error_page_... components/error_page_strings.grdp:64: <message name="IDS_ERRORPAGES_SUGGESTION_RELOAD_REPOST_HEADER" desc="When a page fails to load, sometimes we suggest reloading a page. In the case that reloading the page requires resubmitting data to a website, we use this as a header above a short set of instructions."> No longer used? https://codereview.chromium.org/1639953002/diff/120001/components/error_page_... components/error_page_strings.grdp:498: <message name="IDS_ERRORPAGES_SUGGESTION_RELOAD_SUMMARY" desc="The message displayed in a list of suggestions following a network error, suggesting the user should try reload the page."> Should have these in the same order they appear in LocalizedError. https://codereview.chromium.org/1639953002/diff/120001/components/error_page_... components/error_page_strings.grdp:498: <message name="IDS_ERRORPAGES_SUGGESTION_RELOAD_SUMMARY" desc="The message displayed in a list of suggestions following a network error, suggesting the user should try reload the page."> I think these descriptions should mention the "Try:" prompt - may affect inflection in various languages. Remember that translators apparently get these strings in isolation, possibly out of order (And even if they're in order, I assume they only see new strings, so if we add a suggestion, they wouldn't have context there). https://codereview.chromium.org/1639953002/diff/120001/components/error_page_... components/error_page_strings.grdp:505: <ph name="BEGIN_LINK"><a href="javascript:toggleHelpBox()"></ph>Checking the proxy address and the firewall<ph name="END_LINK"></a><ex></a></ex></ph> Note that the issue with proxies may be the PAC file, rather than the proxy address. I'd use "configuration" or "setup" here, or just remove "address". Same for DS_ERRORPAGES_SUGGESTION_CHECK_PROXY_FIREWALL_DNS_SUMMARY. https://codereview.chromium.org/1639953002/diff/120001/components/error_page_... components/error_page_strings.grdp:508: <ph name="BEGIN_LINK"><a href="#buttons" onclick="javascript:toggleHelpBox()"></ph>Checking the firewall and antivirus configuration<ph name="END_LINK"></a><ex></a></ex></ph> the? I suggest removing "the" and pluralizing configuration - we don't know if none are present, 1 of are are present, or multiple of each are present. https://codereview.chromium.org/1639953002/diff/120001/components/error_page_... components/error_page_strings.grdp:513: <message name="IDS_ERRORPAGES_SUGGESTION_CLEAR_COOKIES_SUMMARY" desc="The message displayed in a list of suggestions following a network error suggesting the user should We also provide a link to the help center to learn more about the failure."> This desc is not a complete sentence. https://codereview.chromium.org/1639953002/diff/120001/components/neterror/re... File components/neterror/resources/neterror.html (right): https://codereview.chromium.org/1639953002/diff/120001/components/neterror/re... components/neterror/resources/neterror.html:23: <p jsvalues=".innerHTML:suggestionSummaryListHeader"></p> BUG: suggestionsSummaryListHeader. (Notice the lowercase s is missing before the capital one)
Mmenke, please take another look. https://codereview.chromium.org/1639953002/diff/120001/chrome/browser/errorpa... File chrome/browser/errorpage_browsertest.cc (right): https://codereview.chromium.org/1639953002/diff/120001/chrome/browser/errorpa... chrome/browser/errorpage_browsertest.cc:131: ToggleHelpBox(browser); On 2016/02/18 19:51:18, mmenke wrote: > Not needed any more, since the next function doesn't use it? Done. https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... File components/error_page/common/localized_error.cc (right): https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... components/error_page/common/localized_error.cc:105: SUGGEST_RELOAD, SUGGEST_CHECK_CONNECTION | SUGGEST_FIREWALL_CONFIG | On 2016/02/18 19:51:18, mmenke wrote: > 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. Done. https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... components/error_page/common/localized_error.cc:558: suggestions_summary_list->Append(suggestion_list_item); On 2016/02/18 19:51:18, mmenke wrote: > 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. Done. Created two helper functions to help here and in the details suggestions function. https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... components/error_page/common/localized_error.cc:562: suggestions & SUGGEST_FIREWALL_CONFIG) { On 2016/02/18 19:51:19, mmenke wrote: > 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. Done. https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... components/error_page/common/localized_error.cc:566: IDS_ERRORPAGES_SUGGESTION_CHECK_PROXY_FIREWALL_DNS_SUMMARY)); On 2016/02/18 19:51:19, mmenke wrote: > 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). Made it clearer specifying the proxy suggestion in the logic. https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... components/error_page/common/localized_error.cc:573: IDS_ERRORPAGES_SUGGESTION_CHECK_FIREWALL_ANTIVIRUS_SUMMARY)); On 2016/02/18 19:51:18, mmenke wrote: > AV vs ANTIVIRUS - should use consistent naming. Changed to ANTIVIRUS https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... components/error_page/common/localized_error.cc:601: IDS_ERRORPAGES_SUGGESTION_CONTACT_ADMIN_SUMMARY_WITH_PREFIX)); On 2016/02/18 19:51:19, mmenke wrote: > 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. Added a separate suggestion to reduce reliance on checking against error code. https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... components/error_page/common/localized_error.cc:613: IDS_ERRORPAGES_SUGGESTION_LEARNMORE_BODY); On 2016/02/18 19:51:18, mmenke wrote: > BODY -> SUMMARY Done. https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... components/error_page/common/localized_error.cc:644: IDS_ERRORPAGES_SUGGESTION_TURN_OFF_AIRPLANE)); On 2016/02/18 19:51:18, mmenke wrote: > _SUMMARY? Done. https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... components/error_page/common/localized_error.cc:651: IDS_ERRORPAGES_SUGGESTION_TURN_ON_DATA)); On 2016/02/18 19:51:19, mmenke wrote: > _SUMMARY? Done. https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... components/error_page/common/localized_error.cc:658: IDS_ERRORPAGES_SUGGESTION_CHECKING_SIGNAL)); On 2016/02/18 19:51:18, mmenke wrote: > _SUMMARY? Done. https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... components/error_page/common/localized_error.cc:709: // Otherwise we rely on the reload button being used. On 2016/02/18 19:51:19, mmenke wrote: > Don't use "we" in comments. Done. https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... components/error_page/common/localized_error.cc:710: if (suggestions_summary_list->GetSize() > 0 && suggestions & SUGGEST_RELOAD) { On 2016/02/18 19:51:18, mmenke wrote: > !suggestions_summary_list->GetSize().empty() Done. https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... components/error_page/common/localized_error.cc:719: // for the 'Incomplete enrollment setup'. On 2016/02/18 19:51:18, mmenke wrote: > 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." Corrected > Did you really want >= 1? Otherwise, having any of the suggestions that are in > gerund form without the "try" prefix seems weird. 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. https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... components/error_page/common/localized_error.cc:733: On 2016/02/18 19:51:18, mmenke wrote: > nit: Remove blank lines. Done. https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... components/error_page/common/localized_error.cc:816: suggestions_details->Append(suggest_unsupported_cipher); On 2016/02/18 19:51:18, mmenke wrote: > 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? 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. https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... components/error_page/common/localized_error.cc:993: l10n_util::GetStringUTF16(IDS_ERRORPAGES_SUGGESTION_GOOGLE_SEARCH)); On 2016/02/18 19:51:18, mmenke wrote: > +_SUMMARY? Done. https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... components/error_page/common/localized_error.cc:1019: IDS_ERRORPAGES_SUGGESTION_RELOAD_REPOST_BODY)); On 2016/02/18 19:51:19, mmenke wrote: > _BODY -> _SUMMARY? Done. https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... File components/error_page/renderer/net_error_helper_core.cc (right): https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... components/error_page/renderer/net_error_helper_core.cc:50: const int kUrlCorrectionsToDisplay = 1; On 2016/02/18 19:51:19, mmenke wrote: > +kMaxUrl...? Done. https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... components/error_page/renderer/net_error_helper_core.cc:342: if (params->override_suggestions->GetSize() == kUrlCorrectionsToDisplay) { On 2016/02/18 19:51:19, mmenke wrote: > Think this should be >=, for clarity. Also, this seems to be in the wrong > place. It should be before this loop, next to the url_correction.empty() check > - this loop is looking up what to do for the current correction, it's not > looping through all the corrections. Note that it already has a "break;" as the > last line. Thanks for spotting this. I actually needed to use continue rather than break as there is no guarantee of the suggestion order. https://codereview.chromium.org/1639953002/diff/120001/components/error_page/... components/error_page/renderer/net_error_helper_core.cc:344: } On 2016/02/18 19:51:19, mmenke wrote: > Remove braces Done. https://codereview.chromium.org/1639953002/diff/120001/components/error_page_... File components/error_page_strings.grdp (right): https://codereview.chromium.org/1639953002/diff/120001/components/error_page_... components/error_page_strings.grdp:64: <message name="IDS_ERRORPAGES_SUGGESTION_RELOAD_REPOST_HEADER" desc="When a page fails to load, sometimes we suggest reloading a page. In the case that reloading the page requires resubmitting data to a website, we use this as a header above a short set of instructions."> On 2016/02/18 19:51:19, mmenke wrote: > No longer used? Removed https://codereview.chromium.org/1639953002/diff/120001/components/error_page_... components/error_page_strings.grdp:498: <message name="IDS_ERRORPAGES_SUGGESTION_RELOAD_SUMMARY" desc="The message displayed in a list of suggestions following a network error, suggesting the user should try reload the page."> On 2016/02/18 19:51:19, mmenke wrote: > I think these descriptions should mention the "Try:" prompt - may affect > inflection in various languages. Remember that translators apparently get these > strings in isolation, possibly out of order (And even if they're in order, I > assume they only see new strings, so if we add a suggestion, they wouldn't have > context there). Done. https://codereview.chromium.org/1639953002/diff/120001/components/error_page_... components/error_page_strings.grdp:498: <message name="IDS_ERRORPAGES_SUGGESTION_RELOAD_SUMMARY" desc="The message displayed in a list of suggestions following a network error, suggesting the user should try reload the page."> On 2016/02/18 19:51:19, mmenke wrote: > Should have these in the same order they appear in LocalizedError. Done. https://codereview.chromium.org/1639953002/diff/120001/components/error_page_... components/error_page_strings.grdp:505: <ph name="BEGIN_LINK"><a href="javascript:toggleHelpBox()"></ph>Checking the proxy address and the firewall<ph name="END_LINK"></a><ex></a></ex></ph> On 2016/02/18 19:51:19, mmenke wrote: > Note that the issue with proxies may be the PAC file, rather than the proxy > address. I'd use "configuration" or "setup" here, or just remove "address". > Same for DS_ERRORPAGES_SUGGESTION_CHECK_PROXY_FIREWALL_DNS_SUMMARY. Done. https://codereview.chromium.org/1639953002/diff/120001/components/error_page_... components/error_page_strings.grdp:508: <ph name="BEGIN_LINK"><a href="#buttons" onclick="javascript:toggleHelpBox()"></ph>Checking the firewall and antivirus configuration<ph name="END_LINK"></a><ex></a></ex></ph> On 2016/02/18 19:51:19, mmenke wrote: > the? I suggest removing "the" and pluralizing configuration - we don't know if > none are present, 1 of are are present, or multiple of each are present. Done. https://codereview.chromium.org/1639953002/diff/120001/components/error_page_... components/error_page_strings.grdp:513: <message name="IDS_ERRORPAGES_SUGGESTION_CLEAR_COOKIES_SUMMARY" desc="The message displayed in a list of suggestions following a network error suggesting the user should We also provide a link to the help center to learn more about the failure."> On 2016/02/18 19:51:19, mmenke wrote: > This desc is not a complete sentence. Fixed. https://codereview.chromium.org/1639953002/diff/120001/components/neterror/re... File components/neterror/resources/neterror.html (right): https://codereview.chromium.org/1639953002/diff/120001/components/neterror/re... components/neterror/resources/neterror.html:23: <p jsvalues=".innerHTML:suggestionSummaryListHeader"></p> On 2016/02/18 19:51:19, mmenke wrote: > BUG: suggestionsSummaryListHeader. (Notice the lowercase s is missing before > the capital one) Thanks. Fixed.
Quick responses. Been reviewing most of the day, so not sure I'll get to a full (hopefully final) pass today, though I do want to to finish this up. https://codereview.chromium.org/1639953002/diff/140001/components/error_page/... File components/error_page/common/localized_error.cc (right): https://codereview.chromium.org/1639953002/diff/140001/components/error_page/... components/error_page/common/localized_error.cc:553: std::string path, const std::string& path.... Could also use const char* or even const base::StringPiece&. https://codereview.chromium.org/1639953002/diff/140001/components/error_page/... components/error_page/common/localized_error.cc:600: if (suggestions & SUGGEST_CONTACT_ADMINISTRATOR_STANDALONE) { Per earlier comment, think it makes sense to early return here...And DCHECK that suggestions_summary_list is empty. Then you can also remove the "suggestions_summary_list->GetSize() > 1" check at the bottom - being stand alone is more a property of the message, with special formatting, rather than a property of only having one suggestion. https://codereview.chromium.org/1639953002/diff/140001/components/error_page/... components/error_page/common/localized_error.cc:712: nit: Remove blank line. https://codereview.chromium.org/1639953002/diff/140001/components/error_page/... File components/error_page/renderer/net_error_helper_core.cc (right): https://codereview.chromium.org/1639953002/diff/140001/components/error_page/... components/error_page/renderer/net_error_helper_core.cc:339: continue; nit: Use braces when the condition takes multiple lines.
Made the change for standalone suggestions. Moved it to the top of the function so it doesn't need to go through the other suggestions logic. https://codereview.chromium.org/1639953002/diff/140001/components/error_page/... File components/error_page/common/localized_error.cc (right): https://codereview.chromium.org/1639953002/diff/140001/components/error_page/... components/error_page/common/localized_error.cc:553: std::string path, On 2016/02/19 20:04:33, mmenke wrote: > const std::string& path.... Could also use const char* or even const > base::StringPiece&. Done. https://codereview.chromium.org/1639953002/diff/140001/components/error_page/... components/error_page/common/localized_error.cc:600: if (suggestions & SUGGEST_CONTACT_ADMINISTRATOR_STANDALONE) { On 2016/02/19 20:04:33, mmenke wrote: > Per earlier comment, think it makes sense to early return here...And DCHECK that > suggestions_summary_list is empty. Then you can also remove the > "suggestions_summary_list->GetSize() > 1" check at the bottom - being stand > alone is more a property of the message, with special formatting, rather than a > property of only having one suggestion. Done. https://codereview.chromium.org/1639953002/diff/140001/components/error_page/... components/error_page/common/localized_error.cc:712: On 2016/02/19 20:04:33, mmenke wrote: > nit: Remove blank line. Done.
Last set of comments. Resolve this, and I'll skim over the changes and stamp. https://codereview.chromium.org/1639953002/diff/160001/components/error_page/... File components/error_page/common/localized_error.cc (right): https://codereview.chromium.org/1639953002/diff/160001/components/error_page/... components/error_page/common/localized_error.cc:65: // Standalone suggestion which includes 'Try' prefix to the sentence. Just to be perfectly clear, suggest "Standalone suggestion which cannot be mixed with others. Includes 'Try' prefix to the bulleted suggestion." And something similar for SUGGEST_COMPLETE_SETUP (More on that further down) https://codereview.chromium.org/1639953002/diff/160001/components/error_page/... components/error_page/common/localized_error.cc:562: list->Append(suggestion_list_item); optional nit: Suggest braces on both clauses here - the net team generally uses them for if/else's, at least. https://codereview.chromium.org/1639953002/diff/160001/components/error_page/... components/error_page/common/localized_error.cc:600: } Maybe add: } else { DCHECK(!(suggestions & SUGGESTION_PROXY_CONFIG)); DCHECK(!(suggestions & SUGGESTION_FIREWALL_CONFIG)); DCHECK(!(suggestions & SUGGESTION_DNS_CONFIG)); } Both for documentation that all those paths are handled, and to catch the addition of any suggestions that don't fit into the current set of combinations with strings. https://codereview.chromium.org/1639953002/diff/160001/components/error_page/... components/error_page/common/localized_error.cc:607: if (suggestions & SUGGEST_LEARNMORE) { Should this be the last suggestion? https://codereview.chromium.org/1639953002/diff/160001/components/error_page/... components/error_page/common/localized_error.cc:621: default: NOTREACHED()? https://codereview.chromium.org/1639953002/diff/160001/components/error_page/... components/error_page/common/localized_error.cc:625: if (learn_more_url.is_valid()) { I don't think we need this check - generally, the preference is to not handle things that can't happen. If this breaks, we should fix it, not paper over it. Probably a good idea to keep this as a DCHECK, rather than a conditional. I know this is what the the old code did. https://codereview.chromium.org/1639953002/diff/160001/components/error_page/... components/error_page/common/localized_error.cc:657: if (suggestions & SUGGEST_COMPLETE_SETUP) { These don't mix with other suggestions...Suggest renaming to SUGGEST_COMPLETE_SETUP_STANDALONE, and putting it up top with the other STANDALONE case, with an early return. Then get rid of the SUGGEST_COMPLETE_SETUP check below. https://codereview.chromium.org/1639953002/diff/160001/components/error_page/... components/error_page/common/localized_error.cc:661: IDS_ERRORPAGES_SUGGESTION_DIAGNOSE_CONNECTION_SUMMARY, false); BUG: These two suggestions are the same. I think one should be IDS_ERRORPAGES_SUGGESTION_COMPLETE_SETUP_SUMMARY. https://codereview.chromium.org/1639953002/diff/160001/components/error_page/... components/error_page/common/localized_error.cc:671: if (!suggestions_summary_list->empty() && suggestions & SUGGEST_RELOAD) { nit: suggestions & SUGGEST_RELOAD => (suggestions & SUGGEST_RELOAD) https://codereview.chromium.org/1639953002/diff/160001/components/error_page/... components/error_page/common/localized_error.cc:671: if (!suggestions_summary_list->empty() && suggestions & SUGGEST_RELOAD) { Should we also show this if the page was generated by a POST (And thus has no reload button)? https://codereview.chromium.org/1639953002/diff/160001/components/error_page/... components/error_page/common/localized_error.cc:680: l10n_util::GetStringUTF16(IDS_ERRORPAGES_SUGGESTION_LIST_HEADER)); Fix indent: These should be indented 4 relative to relative to error_strings, and SUGGEST_COMPLETE_SETUP should go on the next line. Or you can just use git cl format. https://codereview.chromium.org/1639953002/diff/160001/components/error_page/... components/error_page/common/localized_error.cc:690: bool append_standard_menu_items) { Fix indent: These should be indented 4 https://codereview.chromium.org/1639953002/diff/160001/components/error_page/... components/error_page/common/localized_error.cc:695: suggestion_list_item = new base::DictionaryValue; suggest braces https://codereview.chromium.org/1639953002/diff/160001/components/error_page/... components/error_page/common/localized_error.cc:699: l10n_util::GetStringUTF16(header_message_id)); Add braces around body of the if https://codereview.chromium.org/1639953002/diff/160001/components/error_page/... components/error_page/common/localized_error.cc:702: l10n_util::GetStringUTF16(body_message_id)); Add braces https://codereview.chromium.org/1639953002/diff/160001/components/error_page/... components/error_page/common/localized_error.cc:925: base::ListValue* suggestions_summary_list = NULL; NULL -> nullptr (x2)? Not worth going around and changing it everywhere, but since you're adding lines here... https://codereview.chromium.org/1639953002/diff/160001/components/error_page_... File components/error_page_strings.grdp (right): https://codereview.chromium.org/1639953002/diff/160001/components/error_page_... components/error_page_strings.grdp:518: </message> Should this really appear in the try list? It doesn't quite fit with the others (And starting with "Learning" seems a bit weird). https://codereview.chromium.org/1639953002/diff/160001/components/error_page_... components/error_page_strings.grdp:555: Search Google for <ph name="LINK"><a jsvalues="href:searchUrl;.jstdata:$this" onclick="linkClicked(this.jstdata)" jscontent="searchTerms" id="search-link"><ex>Example search terms</ex></a></ph> Should this be "Searching", and mention the "Try" in the desc?
Thanks for the quick turnaround. Gone through and addressed your comments. https://codereview.chromium.org/1639953002/diff/160001/components/error_page/... File components/error_page/common/localized_error.cc (right): https://codereview.chromium.org/1639953002/diff/160001/components/error_page/... components/error_page/common/localized_error.cc:65: // Standalone suggestion which includes 'Try' prefix to the sentence. On 2016/02/22 19:09:41, mmenke wrote: > Just to be perfectly clear, suggest "Standalone suggestion which cannot be mixed > with others. Includes 'Try' prefix to the bulleted suggestion." And something > similar for SUGGEST_COMPLETE_SETUP (More on that further down) Done. https://codereview.chromium.org/1639953002/diff/160001/components/error_page/... components/error_page/common/localized_error.cc:562: list->Append(suggestion_list_item); On 2016/02/22 19:09:41, mmenke wrote: > optional nit: Suggest braces on both clauses here - the net team generally uses > them for if/else's, at least. Done. https://codereview.chromium.org/1639953002/diff/160001/components/error_page/... components/error_page/common/localized_error.cc:600: } On 2016/02/22 19:09:41, mmenke wrote: > Maybe add: > > } else { > DCHECK(!(suggestions & SUGGESTION_PROXY_CONFIG)); > DCHECK(!(suggestions & SUGGESTION_FIREWALL_CONFIG)); > DCHECK(!(suggestions & SUGGESTION_DNS_CONFIG)); > } > > Both for documentation that all those paths are handled, and to catch the > addition of any suggestions that don't fit into the current set of combinations > with strings. Done. https://codereview.chromium.org/1639953002/diff/160001/components/error_page/... components/error_page/common/localized_error.cc:607: if (suggestions & SUGGEST_LEARNMORE) { On 2016/02/22 19:09:41, mmenke wrote: > Should this be the last suggestion? Done. https://codereview.chromium.org/1639953002/diff/160001/components/error_page/... components/error_page/common/localized_error.cc:621: default: On 2016/02/22 19:09:41, mmenke wrote: > NOTREACHED()? Done. https://codereview.chromium.org/1639953002/diff/160001/components/error_page/... components/error_page/common/localized_error.cc:625: if (learn_more_url.is_valid()) { On 2016/02/22 19:09:41, mmenke wrote: > I don't think we need this check - generally, the preference is to not handle > things that can't happen. If this breaks, we should fix it, not paper over it. > > Probably a good idea to keep this as a DCHECK, rather than a conditional. > > I know this is what the the old code did. Done. https://codereview.chromium.org/1639953002/diff/160001/components/error_page/... components/error_page/common/localized_error.cc:657: if (suggestions & SUGGEST_COMPLETE_SETUP) { On 2016/02/22 19:09:41, mmenke wrote: > These don't mix with other suggestions...Suggest renaming to > SUGGEST_COMPLETE_SETUP_STANDALONE, and putting it up top with the other > STANDALONE case, with an early return. Then get rid of the > SUGGEST_COMPLETE_SETUP check below. Done. https://codereview.chromium.org/1639953002/diff/160001/components/error_page/... components/error_page/common/localized_error.cc:661: IDS_ERRORPAGES_SUGGESTION_DIAGNOSE_CONNECTION_SUMMARY, false); On 2016/02/22 19:09:41, mmenke wrote: > BUG: These two suggestions are the same. I think one should be > IDS_ERRORPAGES_SUGGESTION_COMPLETE_SETUP_SUMMARY. Done. https://codereview.chromium.org/1639953002/diff/160001/components/error_page/... components/error_page/common/localized_error.cc:671: if (!suggestions_summary_list->empty() && suggestions & SUGGEST_RELOAD) { On 2016/02/22 19:09:41, mmenke wrote: > nit: suggestions & SUGGEST_RELOAD => (suggestions & SUGGEST_RELOAD) Done. https://codereview.chromium.org/1639953002/diff/160001/components/error_page/... components/error_page/common/localized_error.cc:671: if (!suggestions_summary_list->empty() && suggestions & SUGGEST_RELOAD) { On 2016/02/22 19:09:41, mmenke wrote: > Should we also show this if the page was generated by a POST (And thus has no > reload button)? Reload with post has a different action. Created a specific standalone suggestion for reload - SUGGEST_RELOAD_STANDALONE. Added the logic to the top of the function with the other standalone suggestions. https://codereview.chromium.org/1639953002/diff/160001/components/error_page/... components/error_page/common/localized_error.cc:680: l10n_util::GetStringUTF16(IDS_ERRORPAGES_SUGGESTION_LIST_HEADER)); On 2016/02/22 19:09:41, mmenke wrote: > Fix indent: These should be indented 4 relative to relative to error_strings, > and SUGGEST_COMPLETE_SETUP should go on the next line. Or you can just use git > cl format. Done. https://codereview.chromium.org/1639953002/diff/160001/components/error_page/... components/error_page/common/localized_error.cc:690: bool append_standard_menu_items) { On 2016/02/22 19:09:41, mmenke wrote: > Fix indent: These should be indented 4 Done. https://codereview.chromium.org/1639953002/diff/160001/components/error_page/... components/error_page/common/localized_error.cc:695: suggestion_list_item = new base::DictionaryValue; On 2016/02/22 19:09:41, mmenke wrote: > suggest braces Done. https://codereview.chromium.org/1639953002/diff/160001/components/error_page/... components/error_page/common/localized_error.cc:699: l10n_util::GetStringUTF16(header_message_id)); On 2016/02/22 19:09:41, mmenke wrote: > Add braces around body of the if Done. https://codereview.chromium.org/1639953002/diff/160001/components/error_page/... components/error_page/common/localized_error.cc:702: l10n_util::GetStringUTF16(body_message_id)); On 2016/02/22 19:09:41, mmenke wrote: > Add braces Done. https://codereview.chromium.org/1639953002/diff/160001/components/error_page/... components/error_page/common/localized_error.cc:925: base::ListValue* suggestions_summary_list = NULL; On 2016/02/22 19:09:41, mmenke wrote: > NULL -> nullptr (x2)? Not worth going around and changing it everywhere, but > since you're adding lines here... Done. https://codereview.chromium.org/1639953002/diff/180001/components/error_page/... File components/error_page/common/localized_error.cc (right): https://codereview.chromium.org/1639953002/diff/180001/components/error_page/... components/error_page/common/localized_error.cc:595: if (suggestions & SUGGEST_RELOAD_STANDALONE) { I've moved the reload post suggestion here as a standalone from GetStrings assuming there would be no other suggestions.
LGTM! https://codereview.chromium.org/1639953002/diff/180001/components/error_page/... File components/error_page/common/localized_error.cc (right): https://codereview.chromium.org/1639953002/diff/180001/components/error_page/... components/error_page/common/localized_error.cc:580: DCHECK(suggestions_summary_list->empty()); optional: For all these 3 standalone cases, maybe DCHECK(!(suggestions & ~SUGGEST_CONTACT_ADMINISTRATOR_STANDALONE)); and the logical equivalents for the next 2?
edwardjung@chromium.org changed reviewers: + jochen@chromium.org
Thanks! @jochen, could you rubber stamp? https://codereview.chromium.org/1639953002/diff/180001/components/error_page/... File components/error_page/common/localized_error.cc (right): https://codereview.chromium.org/1639953002/diff/180001/components/error_page/... components/error_page/common/localized_error.cc:580: DCHECK(suggestions_summary_list->empty()); On 2016/02/23 15:43:24, mmenke wrote: > optional: For all these 3 standalone cases, maybe DCHECK(!(suggestions & > ~SUGGEST_CONTACT_ADMINISTRATOR_STANDALONE)); and the logical equivalents for the > next 2? Done
Description was changed from ========== Network error interstitial update - add suggestions list + Add suggestions list to summary area + Merge Link Doctor suggestions into the main suggestions list. BUG=518975 ========== to ========== Network error interstitial update - add suggestions list + Add suggestions list to summary area + Merge Link Doctor suggestions into the main suggestions list. BUG=518975,589405 ==========
lgtm https://codereview.chromium.org/1639953002/diff/220001/components/error_page/... File components/error_page/renderer/net_error_helper_core.cc (right): https://codereview.chromium.org/1639953002/diff/220001/components/error_page/... components/error_page/renderer/net_error_helper_core.cc:339: continue; nit, add { }
The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1639953002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1639953002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
mmenke, FYI, I made a few small changes to the policy tests that were failing. Also removed the view policies suggestion as that was failing the standalone dchecks for ERR_BLOCKED_BY_ADMINISTRATOR. I've commented the changes. Hope it looks okay. https://codereview.chromium.org/1639953002/diff/300001/chrome/browser/net/dns... File chrome/browser/net/dns_probe_browsertest.cc (right): https://codereview.chromium.org/1639953002/diff/300001/chrome/browser/net/dns... chrome/browser/net/dns_probe_browsertest.cc:653: EXPECT_FALSE(PageContains("http://mock.http/title2.html")); Changed URLs as showing only the first URL suggestion. https://codereview.chromium.org/1639953002/diff/300001/components/error_page/... File components/error_page/renderer/net_error_helper_core_unittest.cc (right): https://codereview.chromium.org/1639953002/diff/300001/components/error_page/... components/error_page/renderer/net_error_helper_core_unittest.cc:345: EXPECT_EQ(1u, last_error_page_params()->override_suggestions->GetSize()); Only showing 1 suggestion now. https://codereview.chromium.org/1639953002/diff/320001/components/error_page/... File components/error_page/common/localized_error.cc (right): https://codereview.chromium.org/1639953002/diff/320001/components/error_page/... components/error_page/common/localized_error.cc:790: error_code == net::ERR_BLOCKED_BY_ADMINISTRATOR) { Removed the view policies suggestion so the standalone suggestions logic is correct. ERR_BLOCKED_BY_ADMINISTRATOR had the view policies suggestion as well as contact admin so failed the standalone dchecks.
https://codereview.chromium.org/1639953002/diff/320001/components/error_page/... File components/error_page/common/localized_error.cc (right): https://codereview.chromium.org/1639953002/diff/320001/components/error_page/... components/error_page/common/localized_error.cc:582: DCHECK(suggestions & ~SUGGEST_RELOAD_STANDALONE); These two aren't needed (They should be !(suggestions & SUGGEST_BLAH)), but they're redundant with the above DCHECK, anyways) https://codereview.chromium.org/1639953002/diff/320001/components/error_page/... components/error_page/common/localized_error.cc:592: DCHECK(suggestions & ~SUGGEST_RELOAD_STANDALONE); These last two are redundant. https://codereview.chromium.org/1639953002/diff/320001/components/error_page/... components/error_page/common/localized_error.cc:604: DCHECK(suggestions & ~SUGGEST_COMPLETE_SETUP_STANDALONE); These last two are redundant. https://codereview.chromium.org/1639953002/diff/320001/components/error_page/... components/error_page/common/localized_error.cc:790: error_code == net::ERR_BLOCKED_BY_ADMINISTRATOR) { On 2016/02/25 19:00:03, edwardjung wrote: > Removed the view policies suggestion so the standalone suggestions logic is > correct. ERR_BLOCKED_BY_ADMINISTRATOR had the view policies suggestion as well > as contact admin so failed the standalone dchecks. Ahh...forgot that the standalone was just for above the fold messages...LGTM, but kinda ugly. :(
https://codereview.chromium.org/1639953002/diff/320001/components/error_page/... File components/error_page/common/localized_error.cc (right): https://codereview.chromium.org/1639953002/diff/320001/components/error_page/... components/error_page/common/localized_error.cc:582: DCHECK(suggestions & ~SUGGEST_RELOAD_STANDALONE); On 2016/02/25 22:01:03, mmenke wrote: > These two aren't needed (They should be !(suggestions & SUGGEST_BLAH)), but > they're redundant with the above DCHECK, anyways) Done.
The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1639953002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1639953002/340001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by edwardjung@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1639953002/#ps340001 (title: "Remove redundant DCHECKs")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1639953002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1639953002/340001
Message was sent while issue was closed.
Description was changed from ========== Network error interstitial update - add suggestions list + Add suggestions list to summary area + Merge Link Doctor suggestions into the main suggestions list. BUG=518975,589405 ========== to ========== Network error interstitial update - add suggestions list + Add suggestions list to summary area + Merge Link Doctor suggestions into the main suggestions list. BUG=518975,589405 ==========
Message was sent while issue was closed.
Committed patchset #18 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== Network error interstitial update - add suggestions list + Add suggestions list to summary area + Merge Link Doctor suggestions into the main suggestions list. BUG=518975,589405 ========== to ========== Network error interstitial update - add suggestions list + Add suggestions list to summary area + Merge Link Doctor suggestions into the main suggestions list. BUG=518975,589405 Committed: https://crrev.com/f3d772c9d29ea403856a804b0bc136150d844a0f Cr-Commit-Position: refs/heads/master@{#377784} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/f3d772c9d29ea403856a804b0bc136150d844a0f Cr-Commit-Position: refs/heads/master@{#377784} |
