Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2799883004:
Permissions: Customize permission decision strings to ask/allow/block settings. (Closed)
DescriptionPermissions: Customize permission decision strings to ask/allow/block settings.
Follow up to r461944, which added permission decision strings for permission
decisions made by enterprise policy, extensions, or embargo. This change adds
custom permission decision strings clearer for each 'Ask'/'Allow'/'Block'
setting where previously, there was only one string for each setting source
(i.e., policy, extension, embargo).
See
https://docs.google.com/document/d/1TaoszQOgqhoxZkV1zAHDapam-LIOxGBIUfqU6npoPWs/edit?usp=sharing
for the relevant document and screenshots.
BUG=679877
Review-Url: https://codereview.chromium.org/2799883004
Cr-Commit-Position: refs/heads/master@{#465489}
Committed: https://chromium.googlesource.com/chromium/src/+/b7c5f428d57f3cc842a5838f2da34c05436bb244
Patch Set 1 #Patch Set 2 : Give components_strings.grd more resource ids. #
Total comments: 2
Patch Set 3 : Update string descriptions. #
Total comments: 6
Patch Set 4 : Add back ASK_BY_EXTENSION strings to the code. #Patch Set 5 : Revert updates to resource_ids. #Patch Set 6 : Update resource_ids to add 100 starting from 15930. #
Messages
Total messages: 48 (35 generated)
The CQ bit was checked by patricialor@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-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by patricialor@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...
Description was changed from ========== Permissions: Customize permission decision strings to ask/allow/block settings. Follow up to r461944, which added permission decision strings for permission decisions made by enterprise policy, extensions, or embargo. This change adds custom permission decision strings clearer for each 'Ask'/'Allow'/'Block' setting where previously, there was only one string for each setting source (i.e., policy, extension, embargo). BUG=679877 ========== to ========== Permissions: Customize permission decision strings to ask/allow/block settings. Follow up to r461944, which added permission decision strings for permission decisions made by enterprise policy, extensions, or embargo. This change adds custom permission decision strings clearer for each 'Ask'/'Allow'/'Block' setting where previously, there was only one string for each setting source (i.e., policy, extension, embargo). See https://docs.google.com/document/d/1TaoszQOgqhoxZkV1zAHDapam-LIOxGBIUfqU6npoP... for the relevant document and screenshots. BUG=679877 ==========
patricialor@chromium.org changed reviewers: + dominickn@chromium.org
Hey Dom, PTAL? I updated the screenshots in the doc linked in the description, and most of the changes to page_info.cc was from the revision before r461944 (see https://chromium.googlesource.com/chromium/src/+/9ae05eaa32132bf42b7112acc194...). Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgarron@chromium.org changed reviewers: + lgarron@chromium.org
https://codereview.chromium.org/2799883004/diff/20001/components/page_info_st... File components/page_info_strings.grdp (right): https://codereview.chromium.org/2799883004/diff/20001/components/page_info_st... components/page_info_strings.grdp:116: <message name="IDS_PAGE_INFO_PERMISSION_ALLOWED_BY_POLICY" desc="The label used underneath a permission listed in the Website Settings popup if the permission was explicitly set by the user's enterprise policy."> Nit: s/Website Settings popup/Page Info bubble/g
The CQ bit was checked by patricialor@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/2799883004/diff/20001/components/page_info_st... File components/page_info_strings.grdp (right): https://codereview.chromium.org/2799883004/diff/20001/components/page_info_st... components/page_info_strings.grdp:116: <message name="IDS_PAGE_INFO_PERMISSION_ALLOWED_BY_POLICY" desc="The label used underneath a permission listed in the Website Settings popup if the permission was explicitly set by the user's enterprise policy."> On 2017/04/07 21:05:49, lgarron wrote: > Nit: s/Website Settings popup/Page Info bubble/g Oops - thanks, I forgot I copy pasted these and didn't update the string descriptions. Updated now.
non-OWNER lgtm. Thanks Patti!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
It seems I accidentally added myself as a reviewer; did you want me to review this yet?
Oops, sorry - yes, lgarron@ please review! Also +thakis, PTAL for owner's review on tools/gritsettings/resource_ids. Thanks all!
patricialor@chromium.org changed reviewers: + thakis@chromium.org
Actually +thakis, PTAL for tools/gritsettings/resource_ids. Thanks!
lgtm, sorry about the delay.
LGT, but make sure to add "ask" for extensions. https://codereview.chromium.org/2799883004/diff/40001/chrome/browser/ui/page_... File chrome/browser/ui/page_info/page_info_ui.cc (right): https://codereview.chromium.org/2799883004/diff/40001/chrome/browser/ui/page_... chrome/browser/ui/page_info/page_info_ui.cc:35: IDS_PAGE_INFO_PERMISSION_ASK_BY_POLICY, The enterprise policy control page doesn't seem to allow "ask". However, I would be okay leaving it in, in case we somehow get an enterprise policy that contains it, by accident/legacy/some mechanism to set the policy I don't know. https://codereview.chromium.org/2799883004/diff/40001/chrome/browser/ui/page_... chrome/browser/ui/page_info/page_info_ui.cc:48: kInvalidResourceID, ASK is possible: https://developer.chrome.com/extensions/contentSettings You can test it using https://developer.chrome.com/extensions/samples#search:contentsettings https://codereview.chromium.org/2799883004/diff/40001/tools/gritsettings/reso... File tools/gritsettings/resource_ids (right): https://codereview.chromium.org/2799883004/diff/40001/tools/gritsettings/reso... tools/gritsettings/resource_ids:169: "includes": [16000], Is this still needed?
lgtm
The CQ bit was checked by patricialor@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 patricialor@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 patricialor@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...
Patchset #5 (id:80001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by patricialor@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.
Thanks for the reviews Nico and Lucas! https://codereview.chromium.org/2799883004/diff/40001/chrome/browser/ui/page_... File chrome/browser/ui/page_info/page_info_ui.cc (right): https://codereview.chromium.org/2799883004/diff/40001/chrome/browser/ui/page_... chrome/browser/ui/page_info/page_info_ui.cc:35: IDS_PAGE_INFO_PERMISSION_ASK_BY_POLICY, On 2017/04/18 06:40:00, lgarron wrote: > The enterprise policy control page doesn't seem to allow "ask". > However, I would be okay leaving it in, in case we somehow get an enterprise > policy that contains it, by accident/legacy/some mechanism to set the policy I > don't know. Acknowledged. https://codereview.chromium.org/2799883004/diff/40001/chrome/browser/ui/page_... chrome/browser/ui/page_info/page_info_ui.cc:48: kInvalidResourceID, On 2017/04/18 06:40:00, lgarron wrote: > ASK is possible: https://developer.chrome.com/extensions/contentSettings > > You can test it using > https://developer.chrome.com/extensions/samples#search:contentsettings Oops, thanks for digging deeper here - fixed. https://codereview.chromium.org/2799883004/diff/40001/tools/gritsettings/reso... File tools/gritsettings/resource_ids (right): https://codereview.chromium.org/2799883004/diff/40001/tools/gritsettings/reso... tools/gritsettings/resource_ids:169: "includes": [16000], On 2017/04/18 06:40:00, lgarron wrote: > Is this still needed? Oh, it looks like the id clashes just start from 15930 now - I've reverted the id changes for this and policy_templates.grd below.
The CQ bit was checked by patricialor@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dominickn@chromium.org, lgarron@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2799883004/#ps120001 (title: "Update resource_ids to add 100 starting from 15930.")
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": 1492575467255200,
"parent_rev": "387a67d23d6fb927769b793d322838515e5b3795", "commit_rev":
"b7c5f428d57f3cc842a5838f2da34c05436bb244"}
Message was sent while issue was closed.
Description was changed from ========== Permissions: Customize permission decision strings to ask/allow/block settings. Follow up to r461944, which added permission decision strings for permission decisions made by enterprise policy, extensions, or embargo. This change adds custom permission decision strings clearer for each 'Ask'/'Allow'/'Block' setting where previously, there was only one string for each setting source (i.e., policy, extension, embargo). See https://docs.google.com/document/d/1TaoszQOgqhoxZkV1zAHDapam-LIOxGBIUfqU6npoP... for the relevant document and screenshots. BUG=679877 ========== to ========== Permissions: Customize permission decision strings to ask/allow/block settings. Follow up to r461944, which added permission decision strings for permission decisions made by enterprise policy, extensions, or embargo. This change adds custom permission decision strings clearer for each 'Ask'/'Allow'/'Block' setting where previously, there was only one string for each setting source (i.e., policy, extension, embargo). See https://docs.google.com/document/d/1TaoszQOgqhoxZkV1zAHDapam-LIOxGBIUfqU6npoP... for the relevant document and screenshots. BUG=679877 Review-Url: https://codereview.chromium.org/2799883004 Cr-Commit-Position: refs/heads/master@{#465489} Committed: https://chromium.googlesource.com/chromium/src/+/b7c5f428d57f3cc842a5838f2da3... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as https://chromium.googlesource.com/chromium/src/+/b7c5f428d57f3cc842a5838f2da3... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
