Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2094733002:
Remove details strings used by iframes on network error pages (Closed)
|
Created:
4 years, 6 months ago by edwardjung Modified:
4 years, 5 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove details strings used by iframes on network error pages
+ Switch the string used by iframes from the the 'details' to 'summary'.
+ Switch iframe icon to a div, so consistent with the main frame icon implementation. It removes the icon border applied to images with no src which shows up before the JS is executed.
+ LocalizedError::GetErrorDetails switched to returned the 'summary' string.
BUG=620113
Committed: https://crrev.com/863b125313682291e0f5f061563004216f54f852
Cr-Commit-Position: refs/heads/master@{#402451}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Update GetErrorDetails method to return summary #
Total comments: 1
Patch Set 3 : Fix errorpage tests #
Total comments: 9
Patch Set 4 : Address review comments #Patch Set 5 : Remove unused iOS strings #
Total comments: 2
Patch Set 6 : Fix merge error #Patch Set 7 : Reinstate iOS strings #Patch Set 8 : Remove extra braces #
Messages
Total messages: 30 (12 generated)
Description was changed from ========== Remove details strings used by iframes on network error pages BUG=620113 ========== to ========== Remove details strings used by iframes on network error pages + Switch the string used by iframes from the the 'details' to 'summary'. + Switch iframe icon to a div, so consistent with the main frame icon implementation. It get's rid of the icon border applied to images with no src which shows up before the JS is executed. + LocalizedError::GetErrorDetails switched to returned the 'summary' string. BUG=620113 ==========
Description was changed from ========== Remove details strings used by iframes on network error pages + Switch the string used by iframes from the the 'details' to 'summary'. + Switch iframe icon to a div, so consistent with the main frame icon implementation. It get's rid of the icon border applied to images with no src which shows up before the JS is executed. + LocalizedError::GetErrorDetails switched to returned the 'summary' string. BUG=620113 ========== to ========== Remove details strings used by iframes on network error pages + Switch the string used by iframes from the the 'details' to 'summary'. + Switch iframe icon to a div, so consistent with the main frame icon implementation. It removes the icon border applied to images with no src which shows up before the JS is executed. + LocalizedError::GetErrorDetails switched to returned the 'summary' string. BUG=620113 ==========
edwardjung@chromium.org changed reviewers: + mmenke@chromium.org
Matt, please take a look. Thanks. https://codereview.chromium.org/2094733002/diff/1/components/neterror/resourc... File components/neterror/resources/neterror.css (right): https://codereview.chromium.org/2094733002/diff/1/components/neterror/resourc... components/neterror/resources/neterror.css:271: text-align: center; Centres the icon following the switch from an image tag to div. https://codereview.chromium.org/2094733002/diff/20001/components/error_page/c... File components/error_page/common/localized_error.cc (right): https://codereview.chromium.org/2094733002/diff/20001/components/error_page/c... components/error_page/common/localized_error.cc:1067: return l10n_util::GetStringUTF16(error_map->summary_resource_id); Do you know where this error string is being used? Is it being used outside of the error pages?
https://codereview.chromium.org/2094733002/diff/40001/components/error_page/c... File components/error_page/common/localized_error.cc (right): https://codereview.chromium.org/2094733002/diff/40001/components/error_page/c... components/error_page/common/localized_error.cc:1066: if (error_map && error_map->summary_resource_id != kEmptyMessageResourceID) Seems weird not to have any text on ERR_INTERNET_DISCONNECTED - seems a pretty clear cut failure (Applies to use for iframes, too). https://codereview.chromium.org/2094733002/diff/40001/components/error_page/c... components/error_page/common/localized_error.cc:1069: return l10n_util::GetStringUTF16(IDS_ERRORPAGES_DETAILS_UNKNOWN); Seems like we should remove this one, too, and use IDS_ERRORPAGES_SUMMARY_NOT_AVAILABLE https://codereview.chromium.org/2094733002/diff/40001/components/error_page_s... File components/error_page_strings.grdp (right): https://codereview.chromium.org/2094733002/diff/40001/components/error_page_s... components/error_page_strings.grdp:230: </message> See comment about removing this. https://codereview.chromium.org/2094733002/diff/40001/components/error_page_s... components/error_page_strings.grdp:393: Unknown File Type. I suspect these iOS ones should be removed, too. Not sure where they're plugged in, or if they're plugged in.
[+ sdefresne, blundell, for comments regarding the iOS specific error details strings, for context, we're removing the details strings and reusing the summary strings in iframes] https://codereview.chromium.org/2094733002/diff/40001/components/error_page/c... File components/error_page/common/localized_error.cc (right): https://codereview.chromium.org/2094733002/diff/40001/components/error_page/c... components/error_page/common/localized_error.cc:1066: if (error_map && error_map->summary_resource_id != kEmptyMessageResourceID) On 2016/06/23 19:04:30, mmenke wrote: > Seems weird not to have any text on ERR_INTERNET_DISCONNECTED - seems a pretty > clear cut failure (Applies to use for iframes, too). Rachel and I felt there wasn't anything to add to the heading 'There is no Internet connection' in a summary, so made an exception for internet disconnected to have no summary, rather than introducing an extraneous string. The previous string had diagnostic instructions that have been rolled into the suggestions. I've reused the heading string for the summary, but hide the summary paragraph in the CSS for offline errors. https://codereview.chromium.org/2094733002/diff/40001/components/error_page/c... components/error_page/common/localized_error.cc:1069: return l10n_util::GetStringUTF16(IDS_ERRORPAGES_DETAILS_UNKNOWN); On 2016/06/23 19:04:30, mmenke wrote: > Seems like we should remove this one, too, and use > IDS_ERRORPAGES_SUMMARY_NOT_AVAILABLE Done. https://codereview.chromium.org/2094733002/diff/40001/components/error_page_s... File components/error_page_strings.grdp (right): https://codereview.chromium.org/2094733002/diff/40001/components/error_page_s... components/error_page_strings.grdp:230: </message> On 2016/06/23 19:04:30, mmenke wrote: > See comment about removing this. Done. https://codereview.chromium.org/2094733002/diff/40001/components/error_page_s... components/error_page_strings.grdp:393: Unknown File Type. On 2016/06/23 19:04:30, mmenke wrote: > I suspect these iOS ones should be removed, too. Not sure where they're plugged > in, or if they're plugged in. [+sdefresne, blundell] for comment on whether these details strings can be removed.
Sylvain's better-positioned than I am to answer this question.
sdefresne@chromium.org changed reviewers: + sdefresne@chromium.org
https://codereview.chromium.org/2094733002/diff/40001/components/error_page_s... File components/error_page_strings.grdp (right): https://codereview.chromium.org/2094733002/diff/40001/components/error_page_s... components/error_page_strings.grdp:393: Unknown File Type. On 2016/06/27 11:11:57, edwardjung wrote: > On 2016/06/23 19:04:30, mmenke wrote: > > I suspect these iOS ones should be removed, too. Not sure where they're > plugged > > in, or if they're plugged in. > > [+sdefresne, blundell] for comment on whether these details strings can be > removed. The only iOS-specific string that is used in the downstream repository is IDS_ERRORPAGES_SUMMARY_NOT_AVAILABLE_NO_EMPHASIS. All the others (IDS_ERRORPAGES_HEADING_DOWNLOAD_FILE_TYPE_ERROR, IDS_ERRORPAGES_SUMMARY_DOWNLOAD_FILE_TYPE_ERROR, IDS_ERRORPAGES_DETAILS_DOWNLOAD_FILE_TYPE_ERROR, IDS_ERRORPAGES_HEADING_UNSUPPORTED_SCHEME_ERROR, IDS_ERRORPAGES_SUMMARY_UNSUPPORTED_SCHEME_ERROR, IDS_ERRORPAGES_DETAILS_UNSUPPORTED_SCHEME_ERROR, IDS_ERRORPAGES_TITLE_UNSAFE_PORT, IDS_ERRORPAGES_HEADING_UNSAFE_PORT, IDS_ERRORPAGES_DETAILS_UNSAFE_PORT) are not used from the downstream repository, so they can be removed if they are no longer used from chromium repository either.
https://codereview.chromium.org/2094733002/diff/80001/components/neterror/res... File components/neterror/resources/neterror.css (right): https://codereview.chromium.org/2094733002/diff/80001/components/neterror/res... components/neterror/resources/neterror.css:356: .offline #main-message > p { Where do we set the class of a DOM object to be "offline"?
Thanks sdefresne, I've removed the unused strings you specified. They aren't being used on desktop. https://codereview.chromium.org/2094733002/diff/80001/components/neterror/res... File components/neterror/resources/neterror.css (right): https://codereview.chromium.org/2094733002/diff/80001/components/neterror/res... components/neterror/resources/neterror.css:356: .offline #main-message > p { On 2016/06/27 14:24:04, mmenke wrote: > Where do we set the class of a DOM object to be "offline"? This is set in neterror.js when the icon is switched. https://cs.chromium.org/chromium/src/components/neterror/resources/neterror.j... If the icon is 'icon-offline' this adds the class and activates the offline easter egg.
Thanks! LGTM. Random side comment: Should we remove the offline game on Android? It breaks slide to reload, if you accidentally slide over the dinosaur, and then slide to reload will never work on the error page again. You have to open up the menu to reload, which is a pain. It's happened to me more than once. On 2016/06/27 15:29:08, edwardjung wrote: > Thanks sdefresne, I've removed the unused strings you specified. They aren't > being used on desktop. > > https://codereview.chromium.org/2094733002/diff/80001/components/neterror/res... > File components/neterror/resources/neterror.css (right): > > https://codereview.chromium.org/2094733002/diff/80001/components/neterror/res... > components/neterror/resources/neterror.css:356: .offline #main-message > p { > On 2016/06/27 14:24:04, mmenke wrote: > > Where do we set the class of a DOM object to be "offline"? > > This is set in neterror.js when the icon is switched. > > https://cs.chromium.org/chromium/src/components/neterror/resources/neterror.j... > > If the icon is 'icon-offline' this adds the class and activates the offline > easter egg.
On 2016/06/27 15:41:55, mmenke wrote: > Thanks! LGTM. > > Random side comment: Should we remove the offline game on Android? It breaks > slide to reload, if you accidentally slide over the dinosaur, and then slide to > reload will never work on the error page again. You have to open up the menu to > reload, which is a pain. It's happened to me more than once. > I'll file a bug on this. I'll need to check why the the pull to refresh doesn't work when the game is activated.
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
On 2016/06/27 15:29:08, edwardjung wrote: > Thanks sdefresne, I've removed the unused strings you specified. They aren't > being used on desktop. Would you mind extracting the iOS unused strings removal to a separate CL? In case I made a mistake and we need to revert the change? > https://codereview.chromium.org/2094733002/diff/80001/components/neterror/res... > File components/neterror/resources/neterror.css (right): > > https://codereview.chromium.org/2094733002/diff/80001/components/neterror/res... > components/neterror/resources/neterror.css:356: .offline #main-message > p { > On 2016/06/27 14:24:04, mmenke wrote: > > Where do we set the class of a DOM object to be "offline"? > > This is set in neterror.js when the icon is switched. > > https://cs.chromium.org/chromium/src/components/neterror/resources/neterror.j... > > If the icon is 'icon-offline' this adds the class and activates the offline > easter egg.
> Would you mind extracting the iOS unused strings removal to a separate CL? In > case I made a mistake and we need to revert the change? Sure, instated them in this CL.
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/v2/patch-status/codereview.chromium.or...
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 mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2094733002/#ps140001 (title: "Remove extra braces")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Remove details strings used by iframes on network error pages + Switch the string used by iframes from the the 'details' to 'summary'. + Switch iframe icon to a div, so consistent with the main frame icon implementation. It removes the icon border applied to images with no src which shows up before the JS is executed. + LocalizedError::GetErrorDetails switched to returned the 'summary' string. BUG=620113 ========== to ========== Remove details strings used by iframes on network error pages + Switch the string used by iframes from the the 'details' to 'summary'. + Switch iframe icon to a div, so consistent with the main frame icon implementation. It removes the icon border applied to images with no src which shows up before the JS is executed. + LocalizedError::GetErrorDetails switched to returned the 'summary' string. BUG=620113 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Remove details strings used by iframes on network error pages + Switch the string used by iframes from the the 'details' to 'summary'. + Switch iframe icon to a div, so consistent with the main frame icon implementation. It removes the icon border applied to images with no src which shows up before the JS is executed. + LocalizedError::GetErrorDetails switched to returned the 'summary' string. BUG=620113 ========== to ========== Remove details strings used by iframes on network error pages + Switch the string used by iframes from the the 'details' to 'summary'. + Switch iframe icon to a div, so consistent with the main frame icon implementation. It removes the icon border applied to images with no src which shows up before the JS is executed. + LocalizedError::GetErrorDetails switched to returned the 'summary' string. BUG=620113 Committed: https://crrev.com/863b125313682291e0f5f061563004216f54f852 Cr-Commit-Position: refs/heads/master@{#402451} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/863b125313682291e0f5f061563004216f54f852 Cr-Commit-Position: refs/heads/master@{#402451} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
