Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2884813003:
[subresource_filter] Add custom strings/behavior on Desktop Page Info (Closed)
|
Created:
3 years, 7 months ago by Charlie Harrison Modified:
3 years, 7 months ago CC:
chromium-reviews, lgarron+watch_chromium.org, raymes+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[subresource_filter] Add custom strings/behavior on Desktop Page Info
This CL adds the following to the OIB (Page Info):
- The menu only has two items (no default), with custom strings
- The selected text uses user managed strings instead of default string
(e.g. "Block" instead of "Block (default)"
BUG=689487
Review-Url: https://codereview.chromium.org/2884813003
Cr-Commit-Position: refs/heads/master@{#473704}
Committed: https://chromium.googlesource.com/chromium/src/+/e2aed4aa8b104427ef7568065d19606d8e0dc16e
Patch Set 1 #Patch Set 2 : Add a unit test #
Total comments: 9
Patch Set 3 : raymes review #
Total comments: 6
Patch Set 4 : raymes review #Patch Set 5 : remove Allow string #
Total comments: 2
Patch Set 6 : lgarron review #Patch Set 7 : add period #
Total comments: 5
Messages
Total messages: 51 (29 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...
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: + lgarron@chromium.org, raymes@chromium.org
lgarron, raymes: would you take a look please? Thanks!
> "Allow" instead of "Allow (default)" Wouldn't "Block" be the default here? Or was this just an example? (Just want to make sure I'm not missing something :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2884813003/diff/20001/chrome/browser/ui/page_... File chrome/browser/ui/page_info/page_info_ui.cc (right): https://codereview.chromium.org/2884813003/diff/20001/chrome/browser/ui/page_... chrome/browser/ui/page_info/page_info_ui.cc:294: button_text_ids = kPermissionButtonTextIDUserManaged; nit: Is this needed? I don't think the source could ever be anything other than a user-sourced setting. https://codereview.chromium.org/2884813003/diff/20001/chrome/browser/ui/page_... File chrome/browser/ui/page_info/permission_menu_model.cc (right): https://codereview.chromium.org/2884813003/diff/20001/chrome/browser/ui/page_... chrome/browser/ui/page_info/permission_menu_model.cc:99: label = l10n_util::GetStringUTF16(IDS_PAGE_INFO_SUBRESOURCE_FILTER_ALLOW); Why can't we just leave this as IDS_PAGE_INFO_MENU_ITEM_ALLOW ? https://codereview.chromium.org/2884813003/diff/20001/chrome/browser/ui/page_... chrome/browser/ui/page_info/permission_menu_model.cc:122: label = l10n_util::GetStringUTF16(IDS_PAGE_INFO_SUBRESOURCE_FILTER_BLOCK); I think we can move this up above the if (..) statement. If that flag is enabled, I think the strings will just be "Allow" and "Block" for all permissions, which seems appropriate here?
https://codereview.chromium.org/2884813003/diff/20001/chrome/browser/ui/page_... File chrome/browser/ui/page_info/page_info_ui.cc (right): https://codereview.chromium.org/2884813003/diff/20001/chrome/browser/ui/page_... chrome/browser/ui/page_info/page_info_ui.cc:294: button_text_ids = kPermissionButtonTextIDUserManaged; On 2017/05/15 23:41:21, raymes wrote: > nit: Is this needed? I don't think the source could ever be anything other than > a user-sourced setting. Oh I see - you're avoiding the default case, nevermind.
Description was changed from ========== [subresource_filter] Add custom strings/behavior on Desktop Page Info This CL adds the following to the OIB (Page Info): - The menu only has two items (no default), with custom strings - The selected text uses user managed strings instead of default string (e.g. "Allow" instead of "Allow (default)" BUG=689487 ========== to ========== [subresource_filter] Add custom strings/behavior on Desktop Page Info This CL adds the following to the OIB (Page Info): - The menu only has two items (no default), with custom strings - The selected text uses user managed strings instead of default string (e.g. "Block" instead of "Block (default)" BUG=689487 ==========
https://codereview.chromium.org/2884813003/diff/20001/chrome/browser/ui/page_... File chrome/browser/ui/page_info/page_info_ui.cc (right): https://codereview.chromium.org/2884813003/diff/20001/chrome/browser/ui/page_... chrome/browser/ui/page_info/page_info_ui.cc:294: button_text_ids = kPermissionButtonTextIDUserManaged; On 2017/05/15 23:43:39, raymes wrote: > On 2017/05/15 23:41:21, raymes wrote: > > nit: Is this needed? I don't think the source could ever be anything other > than > > a user-sourced setting. > > Oh I see - you're avoiding the default case, nevermind. Acknowledged. https://codereview.chromium.org/2884813003/diff/20001/chrome/browser/ui/page_... File chrome/browser/ui/page_info/permission_menu_model.cc (right): https://codereview.chromium.org/2884813003/diff/20001/chrome/browser/ui/page_... chrome/browser/ui/page_info/permission_menu_model.cc:99: label = l10n_util::GetStringUTF16(IDS_PAGE_INFO_SUBRESOURCE_FILTER_ALLOW); On 2017/05/15 23:41:21, raymes wrote: > Why can't we just leave this as IDS_PAGE_INFO_MENU_ITEM_ALLOW ? We will eventually want the string to be slightly different than that one, but for now they are identical as a placeholder. This way when we land the final string we only have to update the resource file. https://codereview.chromium.org/2884813003/diff/20001/chrome/browser/ui/page_... chrome/browser/ui/page_info/permission_menu_model.cc:122: label = l10n_util::GetStringUTF16(IDS_PAGE_INFO_SUBRESOURCE_FILTER_BLOCK); On 2017/05/15 23:41:21, raymes wrote: > I think we can move this up above the if (..) statement. If that flag is > enabled, I think the strings will just be "Allow" and "Block" for all > permissions, which seems appropriate here? You mean the IsSecondaryUiMaterial if statement? Done.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
lgarron: note the special casing here which you may not be too excited about :( I don't think it's ideal either, however I think we will be able to address this when we look into addressing the points in https://docs.google.com/document/d/1XxLt9tlNizWDMbO_gWegVnZs_kNM1anI1vR56fjRh... and bring more consistency. Specifically I think we will be able to remove the "default" option from the drop-down for all permissions and possibly remove the (Default) text. https://codereview.chromium.org/2884813003/diff/20001/chrome/browser/ui/page_... File chrome/browser/ui/page_info/permission_menu_model.cc (right): https://codereview.chromium.org/2884813003/diff/20001/chrome/browser/ui/page_... chrome/browser/ui/page_info/permission_menu_model.cc:99: label = l10n_util::GetStringUTF16(IDS_PAGE_INFO_SUBRESOURCE_FILTER_ALLOW); On 2017/05/16 01:47:26, Charlie Harrison wrote: > On 2017/05/15 23:41:21, raymes wrote: > > Why can't we just leave this as IDS_PAGE_INFO_MENU_ITEM_ALLOW ? > > We will eventually want the string to be slightly different than that one, but > for now they are identical as a placeholder. This way when we land the final > string we only have to update the resource file. Oh - I thought it would always be the same as before? How will it be different?
https://codereview.chromium.org/2884813003/diff/20001/chrome/browser/ui/page_... File chrome/browser/ui/page_info/permission_menu_model.cc (right): https://codereview.chromium.org/2884813003/diff/20001/chrome/browser/ui/page_... chrome/browser/ui/page_info/permission_menu_model.cc:99: label = l10n_util::GetStringUTF16(IDS_PAGE_INFO_SUBRESOURCE_FILTER_ALLOW); On 2017/05/16 06:19:51, raymes wrote: > On 2017/05/16 01:47:26, Charlie Harrison wrote: > > On 2017/05/15 23:41:21, raymes wrote: > > > Why can't we just leave this as IDS_PAGE_INFO_MENU_ITEM_ALLOW ? > > > > We will eventually want the string to be slightly different than that one, but > > for now they are identical as a placeholder. This way when we land the final > > string we only have to update the resource file. > > Oh - I thought it would always be the same as before? How will it be different? Followed up offline.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Special-casing does make me sad, but I defer to raymes@ on c/b/ui/page_info for this CL. https://codereview.chromium.org/2884813003/diff/40001/components/page_info_st... File components/page_info_strings.grdp (right): https://codereview.chromium.org/2884813003/diff/40001/components/page_info_st... components/page_info_strings.grdp:263: <message name="IDS_PAGE_INFO_SUBRESOURCE_FILTER_ALLOW" desc="The text of the menu item of a permissions menu on the Page Info UI for the subresource filter permission in Allow mode" translateable="false"> Is this meant to be translateable="false"? Also, I'm wondering why the strings are so long. Why can't we just use "Allow" and "Block" like we do for other permissions with the same meaning? (We used to have longer strings, but shortened them to make Page UI simpler.)
https://codereview.chromium.org/2884813003/diff/40001/components/page_info_st... File components/page_info_strings.grdp (right): https://codereview.chromium.org/2884813003/diff/40001/components/page_info_st... components/page_info_strings.grdp:263: <message name="IDS_PAGE_INFO_SUBRESOURCE_FILTER_ALLOW" desc="The text of the menu item of a permissions menu on the Page Info UI for the subresource filter permission in Allow mode" translateable="false"> On 2017/05/16 20:59:35, lgarron wrote: > Is this meant to be translateable="false"? > > Also, I'm wondering why the strings are so long. Why can't we just use "Allow" > and "Block" like we do for other permissions with the same meaning? > > (We used to have longer strings, but shortened them to make Page UI simpler.) These are the menu elements, which currently match the existing menu element strings (IDS_PAGE_INFO_MENU_ITEM_ALLOW). I've matched them exactly here to use them as placeholders, with translateable=false because these aren't the final strings. The string when the menu element is selected will still be "Allow" or "Block", just not when the menu is expanded.
lgtm https://codereview.chromium.org/2884813003/diff/40001/chrome/browser/ui/page_... File chrome/browser/ui/page_info/permission_menu_model.cc (right): https://codereview.chromium.org/2884813003/diff/40001/chrome/browser/ui/page_... chrome/browser/ui/page_info/permission_menu_model.cc:99: label = l10n_util::GetStringUTF16(IDS_PAGE_INFO_SUBRESOURCE_FILTER_ALLOW); Let's move this up above the if (ui::MaterialDesignController::IsSecondaryUiMaterial()) too. I'd still prefer just to stick to the prior string. But it's not worth blocking for now, we can revisit later if needed. https://codereview.chromium.org/2884813003/diff/40001/components/page_info_st... File components/page_info_strings.grdp (right): https://codereview.chromium.org/2884813003/diff/40001/components/page_info_st... components/page_info_strings.grdp:263: <message name="IDS_PAGE_INFO_SUBRESOURCE_FILTER_ALLOW" desc="The text of the menu item of a permissions menu on the Page Info UI for the subresource filter permission in Allow mode" translateable="false"> On 2017/05/16 21:04:42, Charlie Harrison wrote: > On 2017/05/16 20:59:35, lgarron wrote: > > Is this meant to be translateable="false"? > > > > Also, I'm wondering why the strings are so long. Why can't we just use "Allow" > > and "Block" like we do for other permissions with the same meaning? > > > > (We used to have longer strings, but shortened them to make Page UI simpler.) > > These are the menu elements, which currently match the existing menu element > strings (IDS_PAGE_INFO_MENU_ITEM_ALLOW). I've matched them exactly here to use > them as placeholders, with translateable=false because these aren't the final > strings. > > The string when the menu element is selected will still be "Allow" or "Block", > just not when the menu is expanded. I chatted with lgarron offline and he was thinking of the strings in the drop-down for harmony (which I have been mentioning). Once we switch to harmony we can just use Allow/Block in the drop down for everything (including this setting).
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...
Thanks raymes! lgarron: Could you PTAL at page_info_strings? https://codereview.chromium.org/2884813003/diff/40001/chrome/browser/ui/page_... File chrome/browser/ui/page_info/permission_menu_model.cc (right): https://codereview.chromium.org/2884813003/diff/40001/chrome/browser/ui/page_... chrome/browser/ui/page_info/permission_menu_model.cc:99: label = l10n_util::GetStringUTF16(IDS_PAGE_INFO_SUBRESOURCE_FILTER_ALLOW); On 2017/05/17 00:39:12, raymes wrote: > Let's move this up above the > if (ui::MaterialDesignController::IsSecondaryUiMaterial()) > > too. I'd still prefer just to stick to the prior string. But it's not worth > blocking for now, we can revisit later if needed. Done. I'm keeping the string the same until we reach a decision but I'll remove it if we decide to go with the standard. https://codereview.chromium.org/2884813003/diff/40001/components/page_info_st... File components/page_info_strings.grdp (right): https://codereview.chromium.org/2884813003/diff/40001/components/page_info_st... components/page_info_strings.grdp:263: <message name="IDS_PAGE_INFO_SUBRESOURCE_FILTER_ALLOW" desc="The text of the menu item of a permissions menu on the Page Info UI for the subresource filter permission in Allow mode" translateable="false"> On 2017/05/17 00:39:12, raymes wrote: > On 2017/05/16 21:04:42, Charlie Harrison wrote: > > On 2017/05/16 20:59:35, lgarron wrote: > > > Is this meant to be translateable="false"? > > > > > > Also, I'm wondering why the strings are so long. Why can't we just use > "Allow" > > > and "Block" like we do for other permissions with the same meaning? > > > > > > (We used to have longer strings, but shortened them to make Page UI > simpler.) > > > > These are the menu elements, which currently match the existing menu element > > strings (IDS_PAGE_INFO_MENU_ITEM_ALLOW). I've matched them exactly here to use > > them as placeholders, with translateable=false because these aren't the final > > strings. > > > > The string when the menu element is selected will still be "Allow" or "Block", > > just not when the menu is expanded. > > I chatted with lgarron offline and he was thinking of the strings in the > drop-down for harmony (which I have been mentioning). Once we switch to harmony > we can just use Allow/Block in the drop down for everything (including this > setting). Acknowledged.
Looks like the string issue was resolved, so I'll remove the Allow string in a moment.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
OK I've removed the Allow string per discussion. PTAL?
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.
https://codereview.chromium.org/2884813003/diff/80001/components/page_info_st... File components/page_info_strings.grdp (right): https://codereview.chromium.org/2884813003/diff/80001/components/page_info_st... components/page_info_strings.grdp:263: <message name="IDS_PAGE_INFO_SUBRESOURCE_FILTER_BLOCK" desc="The text of the menu item of a permissions menu on the Page Info UI for the subresource filter permission in Block mode" translateable="false"> I don't think we should use translateable="false" to indicate a placeholder. That invites shipping a mistake. (Also, since it matches IDS_PAGE_INFO_MENU_ITEM_BLOCK and there isn't a `meaning` attribute, it incurs zero translation cost.) Could we instead add a comment above it? <!-- IDS_PAGE_INFO_SUBRESOURCE_FILTER_BLOCK is currently the same as IDS_PAGE_INFO_MENU_ITEM_BLOCK, but we use a different string identifier in the code for the flexibility to change it in the future. --> Also, I know I didn't point it out before, but nit: IDS_PAGE_INFO_MENU_ITEM_SUBRESOURCE_FILTER_BLOCK would be more consistent with the other strings.
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Thanks lgarron! https://codereview.chromium.org/2884813003/diff/80001/components/page_info_st... File components/page_info_strings.grdp (right): https://codereview.chromium.org/2884813003/diff/80001/components/page_info_st... components/page_info_strings.grdp:263: <message name="IDS_PAGE_INFO_SUBRESOURCE_FILTER_BLOCK" desc="The text of the menu item of a permissions menu on the Page Info UI for the subresource filter permission in Block mode" translateable="false"> On 2017/05/17 20:16:45, lgarron wrote: > I don't think we should use translateable="false" to indicate a placeholder. > That invites shipping a mistake. > (Also, since it matches IDS_PAGE_INFO_MENU_ITEM_BLOCK and there isn't a > `meaning` attribute, it incurs zero translation cost.) > > Could we instead add a comment above it? > <!-- IDS_PAGE_INFO_SUBRESOURCE_FILTER_BLOCK is currently the same as > IDS_PAGE_INFO_MENU_ITEM_BLOCK, but we use a different string identifier in the > code for the flexibility to change it in the future. --> Sure, Done. > > Also, I know I didn't point it out before, but nit: > IDS_PAGE_INFO_MENU_ITEM_SUBRESOURCE_FILTER_BLOCK would be more consistent with > the other strings. Done, thanks
LGTM; thanks for your patience with the strings! https://codereview.chromium.org/2884813003/diff/120001/components/page_info_s... File components/page_info_strings.grdp (right): https://codereview.chromium.org/2884813003/diff/120001/components/page_info_s... components/page_info_strings.grdp:263: <!-- IDS_PAGE_INFO_MENU_ITEM_SUBRESOURCE_FILTER_BLOCK is currently the same as IDS_PAGE_INFO_MENU_ITEM_BLOCK, but we use a different string identifier in the code for the flexibility to change it in the future. --> Nit: Update this to IDS_PAGE_INFO_MENU_ITEM_SUBRESOURCE_FILTER_BLOCK
https://codereview.chromium.org/2884813003/diff/120001/components/page_info_s... File components/page_info_strings.grdp (right): https://codereview.chromium.org/2884813003/diff/120001/components/page_info_s... components/page_info_strings.grdp:263: <!-- IDS_PAGE_INFO_MENU_ITEM_SUBRESOURCE_FILTER_BLOCK is currently the same as IDS_PAGE_INFO_MENU_ITEM_BLOCK, but we use a different string identifier in the code for the flexibility to change it in the future. --> On 2017/05/17 20:27:23, lgarron wrote: > Nit: Update this to IDS_PAGE_INFO_MENU_ITEM_SUBRESOURCE_FILTER_BLOCK Sorry I'm not sure I understand this comment?
https://codereview.chromium.org/2884813003/diff/120001/components/page_info_s... File components/page_info_strings.grdp (right): https://codereview.chromium.org/2884813003/diff/120001/components/page_info_s... components/page_info_strings.grdp:263: <!-- IDS_PAGE_INFO_MENU_ITEM_SUBRESOURCE_FILTER_BLOCK is currently the same as IDS_PAGE_INFO_MENU_ITEM_BLOCK, but we use a different string identifier in the code for the flexibility to change it in the future. --> On 2017/05/17 20:29:49, Charlie Harrison wrote: > On 2017/05/17 20:27:23, lgarron wrote: > > Nit: Update this to IDS_PAGE_INFO_MENU_ITEM_SUBRESOURCE_FILTER_BLOCK > > Sorry I'm not sure I understand this comment? lgarron: Quick ping?
lgtm https://codereview.chromium.org/2884813003/diff/120001/components/page_info_s... File components/page_info_strings.grdp (right): https://codereview.chromium.org/2884813003/diff/120001/components/page_info_s... components/page_info_strings.grdp:263: <!-- IDS_PAGE_INFO_MENU_ITEM_SUBRESOURCE_FILTER_BLOCK is currently the same as IDS_PAGE_INFO_MENU_ITEM_BLOCK, but we use a different string identifier in the code for the flexibility to change it in the future. --> On 2017/05/17 at 20:29:49, Charlie Harrison wrote: > On 2017/05/17 20:27:23, lgarron wrote: > > Nit: Update this to IDS_PAGE_INFO_MENU_ITEM_SUBRESOURCE_FILTER_BLOCK > > Sorry I'm not sure I understand this comment? Sorry, I glanced at the strings wrong, and it looked like it was out of sync. All good now.
https://codereview.chromium.org/2884813003/diff/120001/components/page_info_s... File components/page_info_strings.grdp (right): https://codereview.chromium.org/2884813003/diff/120001/components/page_info_s... components/page_info_strings.grdp:263: <!-- IDS_PAGE_INFO_MENU_ITEM_SUBRESOURCE_FILTER_BLOCK is currently the same as IDS_PAGE_INFO_MENU_ITEM_BLOCK, but we use a different string identifier in the code for the flexibility to change it in the future. --> On 2017/05/22 20:09:32, lgarron wrote: > On 2017/05/17 at 20:29:49, Charlie Harrison wrote: > > On 2017/05/17 20:27:23, lgarron wrote: > > > Nit: Update this to IDS_PAGE_INFO_MENU_ITEM_SUBRESOURCE_FILTER_BLOCK > > > > Sorry I'm not sure I understand this comment? > > Sorry, I glanced at the strings wrong, and it looked like it was out of sync. > All good now. No worries, just making sure.
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org Link to the patchset: https://codereview.chromium.org/2884813003/#ps120001 (title: "add period")
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": 120001, "attempt_start_ts": 1495483824252880,
"parent_rev": "fc395879fe38b3a86febe1dfc38e72d271801398", "commit_rev":
"e2aed4aa8b104427ef7568065d19606d8e0dc16e"}
Message was sent while issue was closed.
Description was changed from ========== [subresource_filter] Add custom strings/behavior on Desktop Page Info This CL adds the following to the OIB (Page Info): - The menu only has two items (no default), with custom strings - The selected text uses user managed strings instead of default string (e.g. "Block" instead of "Block (default)" BUG=689487 ========== to ========== [subresource_filter] Add custom strings/behavior on Desktop Page Info This CL adds the following to the OIB (Page Info): - The menu only has two items (no default), with custom strings - The selected text uses user managed strings instead of default string (e.g. "Block" instead of "Block (default)" BUG=689487 Review-Url: https://codereview.chromium.org/2884813003 Cr-Commit-Position: refs/heads/master@{#473704} Committed: https://chromium.googlesource.com/chromium/src/+/e2aed4aa8b104427ef7568065d19... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/e2aed4aa8b104427ef7568065d19... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
