Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2881203002:
[subresource_filter] Add Page Info subtitle on desktop + placeholder string (Closed)
|
Created:
3 years, 7 months ago by Charlie Harrison Modified:
3 years, 6 months ago CC:
agrieve+watch_chromium.org, chromium-reviews, lgarron+watch_chromium.org, raymes+watch_chromium.org, srahim+watch_chromium.org, srahim Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[subresource_filter] Add Page Info subtitle on desktop + placeholder string
The android UI already has the subtitle but doesn't use the correct
placeholder. Since the placeholder is shared between platforms we only
need a single entry.
BUG=689487
Review-Url: https://codereview.chromium.org/2881203002
Cr-Commit-Position: refs/heads/master@{#474170}
Committed: https://chromium.googlesource.com/chromium/src/+/ba1e29e501d68b70c7cb374b90e6b758f60123aa
Patch Set 1 #
Total comments: 2
Patch Set 2 : lgarron review #
Total comments: 2
Patch Set 3 : raymes review #
Total comments: 4
Patch Set 4 : lgarron review #
Messages
Total messages: 33 (18 generated)
The CQ bit was checked by csharrison@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...
csharrison@chromium.org changed reviewers: + dtrainor@chromium.org, raymes@chromium.org
dtrainor: PTAL at the trivial string replacement in PageInfoPopup.java rames: PTAL at everything
lgarron@chromium.org changed reviewers: + lgarron@chromium.org
https://codereview.chromium.org/2881203002/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2881203002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:12163: <message name="IDS_SUBRESOURCE_FILTER_PAGE_INFO_SUBTITLE" desc="Subtitle 'policy' string to be used under the setting on desktop Page Info (aka OIB)" translateable="false" formatter_data="android_java"> Could you please make sure to put this with the other permission decision string section of page_info_strings.grdp? Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Thanks! https://codereview.chromium.org/2881203002/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2881203002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:12163: <message name="IDS_SUBRESOURCE_FILTER_PAGE_INFO_SUBTITLE" desc="Subtitle 'policy' string to be used under the setting on desktop Page Info (aka OIB)" translateable="false" formatter_data="android_java"> On 2017/05/15 18:47:11, lgarron wrote: > Could you please make sure to put this with the other permission decision string > section of page_info_strings.grdp? Thanks! Done.
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.
PageInfoPopup.java lgtm
lgtm https://codereview.chromium.org/2881203002/diff/20001/chrome/browser/ui/page_... File chrome/browser/ui/page_info/page_info_ui.cc (right): https://codereview.chromium.org/2881203002/diff/20001/chrome/browser/ui/page_... chrome/browser/ui/page_info/page_info_ui.cc:344: } nit: no {}
The CQ bit was checked by csharrison@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...
https://codereview.chromium.org/2881203002/diff/20001/chrome/browser/ui/page_... File chrome/browser/ui/page_info/page_info_ui.cc (right): https://codereview.chromium.org/2881203002/diff/20001/chrome/browser/ui/page_... chrome/browser/ui/page_info/page_info_ui.cc:344: } On 2017/05/15 23:15:50, raymes wrote: > nit: no {} Done.
lgarron: looks like I need your final LG for page_info_strings. PTAL?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2881203002/diff/40001/components/page_info_st... File components/page_info_strings.grdp (right): https://codereview.chromium.org/2881203002/diff/40001/components/page_info_st... components/page_info_strings.grdp:299: <message name="IDS_PAGE_INFO_PERMISSION_SUBRESOURCE_FILTER_SUBTITLE" desc="The label used underneath the subresource filter permission in the Page Info UI. Used on desktop and Android platforms" translateable="false" formatter_data="android_java"> Could you remove the translateable="false", like with https://codereview.chromium.org/2884813003/? I'm not familiar with formatter_data="android_java"; does that do something useful here? https://codereview.chromium.org/2881203002/diff/40001/components/page_info_st... components/page_info_strings.grdp:300: Subresource filter activated (subtitle) Nit: remove "(subtitle)", assuming that's meant as a placeholder.
https://codereview.chromium.org/2881203002/diff/40001/components/page_info_st... File components/page_info_strings.grdp (right): https://codereview.chromium.org/2881203002/diff/40001/components/page_info_st... components/page_info_strings.grdp:299: <message name="IDS_PAGE_INFO_PERMISSION_SUBRESOURCE_FILTER_SUBTITLE" desc="The label used underneath the subresource filter permission in the Page Info UI. Used on desktop and Android platforms" translateable="false" formatter_data="android_java"> On 2017/05/17 20:38:11, lgarron wrote: > Could you remove the translateable="false", like with > https://codereview.chromium.org/2884813003/ > > I'm not familiar with formatter_data="android_java"; does that do something > useful here? I don't think we should remove translateable=false for strings that are not identical to existing strings. I was told by translation folks to add that for placeholder strings that aren't really translateable. formatter_data="android_java" makes it possible to share the string on Android so it is accessible with R.string... https://codereview.chromium.org/2881203002/diff/40001/components/page_info_st... components/page_info_strings.grdp:300: Subresource filter activated (subtitle) On 2017/05/17 20:38:11, lgarron wrote: > Nit: remove "(subtitle)", assuming that's meant as a placeholder. Done.
lgarron: friendly ping
I still disagree with landing strings with translateable="false"; could you point me to documentation about that UI advice? Otherwise, what assurances do we have that the string won't reach non-English end users?
On 2017/05/23 at 22:56:28, lgarron wrote: > I still disagree with landing strings with translateable="false"; could you point me to documentation about that UI advice? > > Otherwise, what assurances do we have that the string won't reach non-English end users? csharrison@ pinged me with b/37240341, where Shimi advised adding translateable="false". Since it's unlikely we'll ship this to users untranslated out of pure accident, LGTM.
Thanks lgarron, +srahim explicitly as FYI
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org, raymes@chromium.org Link to the patchset: https://codereview.chromium.org/2881203002/#ps60001 (title: "lgarron review")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495582064626770,
"parent_rev": "bfc4a9910e0e509249b22a1fb5a86326ddfb88d0", "commit_rev":
"ba1e29e501d68b70c7cb374b90e6b758f60123aa"}
Message was sent while issue was closed.
Description was changed from ========== [subresource_filter] Add Page Info subtitle on desktop + placeholder string The android UI already has the subtitle but doesn't use the correct placeholder. Since the placeholder is shared between platforms we only need a single entry. BUG=689487 ========== to ========== [subresource_filter] Add Page Info subtitle on desktop + placeholder string The android UI already has the subtitle but doesn't use the correct placeholder. Since the placeholder is shared between platforms we only need a single entry. BUG=689487 Review-Url: https://codereview.chromium.org/2881203002 Cr-Commit-Position: refs/heads/master@{#474170} Committed: https://chromium.googlesource.com/chromium/src/+/ba1e29e501d68b70c7cb374b90e6... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/ba1e29e501d68b70c7cb374b90e6... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
