Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2214393003:
Change network error titles to just the hostname (Closed)
|
Created:
4 years, 4 months ago by edwardjung Modified:
4 years, 4 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. |
DescriptionChange network error titles to just the hostname
BUG=630190
Committed: https://crrev.com/e76b58098fd472f255d3d9eda75a95ccbfce9abc
Cr-Commit-Position: refs/heads/master@{#411315}
Patch Set 1 #Patch Set 2 : git upload error #Patch Set 3 : Change network error titles to just the hostname #Patch Set 4 : Fix merge errors #Patch Set 5 : Fix browser tests #Patch Set 6 : Remove unused title string #
Total comments: 2
Patch Set 7 : Show full path when url is not http / https #Patch Set 8 : Browser test fix #
Total comments: 2
Patch Set 9 : Add missing braces #
Total comments: 4
Patch Set 10 : Fix nits #Patch Set 11 : Fix merge conflict #
Messages
Total messages: 34 (24 generated)
Description was changed from ========== Change network error titles to just the hostname BUG= ========== to ========== Change network error titles to just the hostname BUG=630190 ==========
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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
edwardjung@chromium.org changed reviewers: + bauerb@chromium.org, mmenke@chromium.org, tnagel@chromium.org
Hi all PTAL, @bauerb and @tnagel I've updated tests that check against the network error page title. mmenke: components/error_page/common/localized_error.cc components/error_page_strings.grdp bauerb: chrome/browser/content_settings/content_settings_browsertest.cc tnagel: chrome/browser/policy/policy_browsertest.cc
https://codereview.chromium.org/2214393003/diff/100001/components/error_page/... File components/error_page/common/localized_error.cc (right): https://codereview.chromium.org/2214393003/diff/100001/components/error_page/... components/error_page/common/localized_error.cc:864: error_strings->SetString("title", host_name); Should use the full spec() if !failed_url().SchemeIsHttpOrHttps(). Just look at what's displayed for file URLs, with this code. :) I suppose not including the port or scheme is consistent with what we do in the omnibox, though from a power user/security-relevant information standpoint, I'd prefer using url.Origin().
lgtm
https://codereview.chromium.org/2214393003/diff/100001/components/error_page/... File components/error_page/common/localized_error.cc (right): https://codereview.chromium.org/2214393003/diff/100001/components/error_page/... components/error_page/common/localized_error.cc:864: error_strings->SetString("title", host_name); On 2016/08/05 15:03:43, mmenke wrote: > Should use the full spec() if !failed_url().SchemeIsHttpOrHttps(). Just look at > what's displayed for file URLs, with this code. :) > > I suppose not including the port or scheme is consistent with what we do in the > omnibox, though from a power user/security-relevant information standpoint, I'd > prefer using url.Origin(). Changed it to do a scheme check. https / broken https is still being shown in the omnibox where it matters more. The priority is to allow few extra characters for the hostname in the title when the tab width shrinks with multiple tabs open. This should also help power users.
LGTM https://codereview.chromium.org/2214393003/diff/140001/chrome/browser/policy/... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/2214393003/diff/140001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:380: title = base::UTF8ToUTF16(url.spec()); Use braces on if / else bodies, when one of them takes up more than one line. https://codereview.chromium.org/2214393003/diff/140001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:397: title = base::UTF8ToUTF16(url.spec()); Use braces
On 2016/08/08 14:23:49, mmenke wrote: > LGTM > > https://codereview.chromium.org/2214393003/diff/140001/chrome/browser/policy/... > File chrome/browser/policy/policy_browsertest.cc (right): > > https://codereview.chromium.org/2214393003/diff/140001/chrome/browser/policy/... > chrome/browser/policy/policy_browsertest.cc:380: title = > base::UTF8ToUTF16(url.spec()); > Use braces on if / else bodies, when one of them takes up more than one line. > > https://codereview.chromium.org/2214393003/diff/140001/chrome/browser/policy/... > chrome/browser/policy/policy_browsertest.cc:397: title = > base::UTF8ToUTF16(url.spec()); > Use braces Done. @tnagel can I get your approval for policy_browsertest.cc Thanks.
policy_browsertest.cc lgtm % comments https://codereview.chromium.org/2214393003/diff/160001/chrome/browser/policy/... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/2214393003/diff/160001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:375: base::string16 title; To make this more understandable, could you maybe explain that |title| contains the page title for the blocked page? And/or maybe rename to |blocked_page_title| or |title_if_blocked| or something similar? https://codereview.chromium.org/2214393003/diff/160001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:379: // Local file paths show the full URL. Nit: Please drop duplicate blank. https://codereview.chromium.org/2214393003/diff/160001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:393: base::string16 title; Same as above. https://codereview.chromium.org/2214393003/diff/160001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:397: // Local file paths show the full URL. Nit: Please drop duplicate blank.
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_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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 bauerb@chromium.org, mmenke@chromium.org, tnagel@chromium.org Link to the patchset: https://codereview.chromium.org/2214393003/#ps200001 (title: "Fix merge conflict")
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 ========== Change network error titles to just the hostname BUG=630190 ========== to ========== Change network error titles to just the hostname BUG=630190 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Change network error titles to just the hostname BUG=630190 ========== to ========== Change network error titles to just the hostname BUG=630190 Committed: https://crrev.com/e76b58098fd472f255d3d9eda75a95ccbfce9abc Cr-Commit-Position: refs/heads/master@{#411315} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/e76b58098fd472f255d3d9eda75a95ccbfce9abc Cr-Commit-Position: refs/heads/master@{#411315} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
