Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2743423004:
Permissions: Show the reason for permission decisions made on the user's behalf. (Closed)
|
Created:
3 years, 9 months ago by Patti Lor Modified:
3 years, 8 months ago CC:
chrome-apps-syd-reviews_chromium.org, chromium-reviews, markusheintz_, msramek+watch_chromium.org, raymes+watch_chromium.org, srahim+watch_chromium.org, tfarina Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionPermissions: Show the reason for permission decisions made on the user's behalf.
A decision for a permission may be allowed or blocked on behalf of the user,
either from one of the user's installed extensions, enforced by an enterprise
policy, or via embargo. Currently, the former two reasons (extensions or policy)
will be indicated to the user by custom strings in the menubutton or combobox
associated with that permission in the website settings pop-up.
This patch will put the reason for a permission decision being made underneath
a permission in gray text and add strings to tell if the permission was blocked
because of embargo for Views and Cocoa (Mac). See
https://docs.google.com/document/d/1TaoszQOgqhoxZkV1zAHDapam-LIOxGBIUfqU6npoPWs/edit?usp=sharing
for the relevant document and screenshots.
BUG=679877
TEST=For the embargo string: Run Chrome with the command line flag
--enable-features=BlockPromptsIfDismissedOften. Navigate to
https://permission.site. Click the "Notifications" button and press escape, and
repeat this three times in a row until the permission prompt no longer appears.
Opening the page info bubble should show the text "Automatically blocked"
underneath the "Notification" permission.
For the extensions string: Run Chrome and install the "Toggle Javascript"
extension. Click on the icon to enable it, then open page info. Underneath
Javascript, it should say "Controlled by an extension".
For the policy string: No easy way to test this cross-platform.
Review-Url: https://codereview.chromium.org/2743423004
Cr-Commit-Position: refs/heads/master@{#461944}
Committed: https://chromium.googlesource.com/chromium/src/+/217bb29f809ae73e537ec9c0eba5ac65f48fd873
Patch Set 1 #Patch Set 2 : Rebase back onto origin/master. #Patch Set 3 : Cleanup. #Patch Set 4 : Update to work for Mac as well. #Patch Set 5 : Rebase. #Patch Set 6 : Rebase againnn #
Total comments: 14
Patch Set 7 : Rebase with review comments. #
Total comments: 22
Patch Set 8 : Review comments. #
Messages
Total messages: 57 (42 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: 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 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: Try jobs failed on following builders: 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 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: Try jobs failed on following builders: 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 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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) 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...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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.
Description was changed from ========== Views/Permissions: Show reason for permission decisions made on user's behalf. A decision for a permission may be allowed or blocked on behalf of the user, either from one of the user's installed extensions, enforced by an enterprise policy, or via embargo. Currently, the former two reasons (extensions or policy) will be indicated to the user by custom strings in the menubutton or combobox associated with that permission in the website settings pop-up. This patch will put the reason for a permission decision being made underneath a permission in gray text and add strings to tell if the permission was blocked because of embargo. BUG=679877 ========== to ========== Permissions: Show the reason for permission decisions made on the user's behalf. A decision for a permission may be allowed or blocked on behalf of the user, either from one of the user's installed extensions, enforced by an enterprise policy, or via embargo. Currently, the former two reasons (extensions or policy) will be indicated to the user by custom strings in the menubutton or combobox associated with that permission in the website settings pop-up. This patch will put the reason for a permission decision being made underneath a permission in gray text and add strings to tell if the permission was blocked because of embargo for Views and Cocoa (Mac). BUG=679877 ==========
patricialor@chromium.org changed reviewers: + dominickn@chromium.org
Hi Dom, PTAL?
Looks pretty good, I just have nits. We'll need to follow up to make sure Ask on the embargoed permissions resets embargo. :) https://codereview.chromium.org/2743423004/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/page_info/website_settings_bubble_controller.mm (right): https://codereview.chromium.org/2743423004/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/page_info/website_settings_bubble_controller.mm:917: // Show the permission decision reason, if it was not the user. "Show the reason for the permission decision in a new row if it did not come from the user"? https://codereview.chromium.org/2743423004/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/page_info/website_settings_bubble_controller_unittest.mm (right): https://codereview.chromium.org/2743423004/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/page_info/website_settings_bubble_controller_unittest.mm:292: Should you add a test for the embargoed permissions here to ensure they have enabled buttons? https://codereview.chromium.org/2743423004/diff/100001/chrome/browser/ui/page... File chrome/browser/ui/page_info/website_settings_ui.cc (right): https://codereview.chromium.org/2743423004/diff/100001/chrome/browser/ui/page... chrome/browser/ui/page_info/website_settings_ui.cc:22: #include "ui/gfx/image/image.h" #include "url/gurl.h" https://codereview.chromium.org/2743423004/diff/100001/chrome/browser/ui/page... chrome/browser/ui/page_info/website_settings_ui.cc:292: if (message_id != kInvalidResourceID) Minor nit: I would flip the conditional to make it if (message_id == kInvalidResourceID) return base::string16() https://codereview.chromium.org/2743423004/diff/100001/chrome/browser/ui/page... File chrome/browser/ui/page_info/website_settings_ui.h (right): https://codereview.chromium.org/2743423004/diff/100001/chrome/browser/ui/page... chrome/browser/ui/page_info/website_settings_ui.h:18: class Profile; class GURL; https://codereview.chromium.org/2743423004/diff/100001/chrome/browser/ui/page... chrome/browser/ui/page_info/website_settings_ui.h:160: static base::string16 PermissionDecisionReasonToString( ToUIString for consistency? https://codereview.chromium.org/2743423004/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/page_info/permission_selector_row.cc (right): https://codereview.chromium.org/2743423004/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/page_info/permission_selector_row.cc:268: // Show the permission decision reason, if it was not the user. Same comment as above
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.
Hey Dom, PTAL - soz for the wait, had to rebase again. https://codereview.chromium.org/2743423004/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/page_info/website_settings_bubble_controller.mm (right): https://codereview.chromium.org/2743423004/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/page_info/website_settings_bubble_controller.mm:917: // Show the permission decision reason, if it was not the user. On 2017/03/26 23:46:24, dominickn wrote: > "Show the reason for the permission decision in a new row if it did not come > from the user"? Done. https://codereview.chromium.org/2743423004/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/page_info/website_settings_bubble_controller_unittest.mm (right): https://codereview.chromium.org/2743423004/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/page_info/website_settings_bubble_controller_unittest.mm:292: On 2017/03/26 23:46:24, dominickn wrote: > Should you add a test for the embargoed permissions here to ensure they have > enabled buttons? I think it's possible, but since we can't tell if something is embargoed from PermissionInfo alone, I would have to set up a way to manually set the embargo status from this test, which might be a bit of work. Do you think it's worth looking into / it makes sense in this file? https://codereview.chromium.org/2743423004/diff/100001/chrome/browser/ui/page... File chrome/browser/ui/page_info/website_settings_ui.cc (right): https://codereview.chromium.org/2743423004/diff/100001/chrome/browser/ui/page... chrome/browser/ui/page_info/website_settings_ui.cc:22: #include "ui/gfx/image/image.h" On 2017/03/26 23:46:24, dominickn wrote: > #include "url/gurl.h" Done. https://codereview.chromium.org/2743423004/diff/100001/chrome/browser/ui/page... chrome/browser/ui/page_info/website_settings_ui.cc:292: if (message_id != kInvalidResourceID) On 2017/03/26 23:46:24, dominickn wrote: > Minor nit: I would flip the conditional to make it > > if (message_id == kInvalidResourceID) > return base::string16() Done. https://codereview.chromium.org/2743423004/diff/100001/chrome/browser/ui/page... File chrome/browser/ui/page_info/website_settings_ui.h (right): https://codereview.chromium.org/2743423004/diff/100001/chrome/browser/ui/page... chrome/browser/ui/page_info/website_settings_ui.h:18: class Profile; On 2017/03/26 23:46:24, dominickn wrote: > class GURL; Done. https://codereview.chromium.org/2743423004/diff/100001/chrome/browser/ui/page... chrome/browser/ui/page_info/website_settings_ui.h:160: static base::string16 PermissionDecisionReasonToString( On 2017/03/26 23:46:24, dominickn wrote: > ToUIString for consistency? Done. https://codereview.chromium.org/2743423004/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/page_info/permission_selector_row.cc (right): https://codereview.chromium.org/2743423004/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/page_info/permission_selector_row.cc:268: // Show the permission decision reason, if it was not the user. On 2017/03/26 23:46:24, dominickn wrote: > Same comment as above Done.
non-OWNER lgtm, thanks!
patricialor@chromium.org changed reviewers: + lgarron@chromium.org
Hi lgarron, PTAL for owner's review? Thank you!
lgarron@chromium.org changed reviewers: + msw@chromium.org, rsesek@chromium.org
+rsesek@ for Cocoa and msw@ for Views. Looks pretty reasonable overall (and looks good in practice), but I had some comments throughout. Could you also add a TEST= case for testing as many conditions as can be tested using a normal build? (I was only able to test the "blocked by extension" case, after some effort to find an extension that uses the contentSettings extension API.) https://codereview.chromium.org/2743423004/diff/120001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2743423004/diff/120001/chrome/app/generated_r... chrome/app/generated_resources.grd:281: + <message name="IDS_WEBSITE_SETTINGS_PERMISSION_SET_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."> These should be called IDS_PAGE_INFO_* now. Ideally they should also be added to pageinfo_strings.grdp, but feel free to leave them here until https://crbug.com/663975 https://codereview.chromium.org/2743423004/diff/120001/chrome/app/generated_r... chrome/app/generated_resources.grd:282: + Set by enterprise policy I still have opinions about these strings, but I've brought it up on the email thread. https://codereview.chromium.org/2743423004/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.mm (right): https://codereview.chromium.org/2743423004/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.mm:909: int maxY = NSMaxY(buttonFrame); I know the existing code uses maxY, but mainly to mean "the very bottom", and but I'm finding it hard to follow the calculations in here. Is it possible to keep using point.y? That is, something like: point.y = NSHeight(label.frame); if (...) { point.y += kPermissionsDecisionVerticalSpacing; ... point.y += label.frame.size.height; } return NSMakePoint(NSMaxX(buttonFrame), point.y); https://codereview.chromium.org/2743423004/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.mm:916: withSize:[NSFont systemFontSize] Looking at the mocks, it seems to be this should be smallSystemFontSize. https://codereview.chromium.org/2743423004/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.mm:920: label.textColor = skia::SkColorToSRGBNSColor(SK_ColorGRAY); Could you share the color across platforms by e.g. creating a GetPermissionDecisionTextColor() function in PageInfoUI? https://codereview.chromium.org/2743423004/diff/120001/chrome/browser/ui/page... File chrome/browser/ui/page_info/page_info_ui.cc (right): https://codereview.chromium.org/2743423004/diff/120001/chrome/browser/ui/page... chrome/browser/ui/page_info/page_info_ui.cc:228: Nit: avoid adding unneeded whitespace. https://codereview.chromium.org/2743423004/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/page_info/permission_selector_row.cc (right): https://codereview.chromium.org/2743423004/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/page_info/permission_selector_row.cc:277: permission_decision_reason->SetEnabledColor(SK_ColorGRAY); Note that "High Contrast Black" mode on Windows has a black background for Page Info. This shade of gray actually has pretty good contrast [1]; did the specs explicitly cover this case, though? [1] http://leaverou.github.io/contrast-ratio/#black-on-%23888888 https://codereview.chromium.org/2743423004/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/page_info/permission_selector_row.cc:280: DCHECK(!!column_set); More of a question for me to learn: Is this pattern common? Other Page Info DCHECKs just pass the pointer directly, and I can't find documentation about the recommended approach.
Description was changed from ========== Permissions: Show the reason for permission decisions made on the user's behalf. A decision for a permission may be allowed or blocked on behalf of the user, either from one of the user's installed extensions, enforced by an enterprise policy, or via embargo. Currently, the former two reasons (extensions or policy) will be indicated to the user by custom strings in the menubutton or combobox associated with that permission in the website settings pop-up. This patch will put the reason for a permission decision being made underneath a permission in gray text and add strings to tell if the permission was blocked because of embargo for Views and Cocoa (Mac). BUG=679877 ========== to ========== Permissions: Show the reason for permission decisions made on the user's behalf. A decision for a permission may be allowed or blocked on behalf of the user, either from one of the user's installed extensions, enforced by an enterprise policy, or via embargo. Currently, the former two reasons (extensions or policy) will be indicated to the user by custom strings in the menubutton or combobox associated with that permission in the website settings pop-up. This patch will put the reason for a permission decision being made underneath a permission in gray text and add strings to tell if the permission was blocked because of embargo for Views and Cocoa (Mac). BUG=679877 TEST=For the embargo string: Run Chrome with the command line flag --enable-features=BlockPromptsIfDismissedOften. Navigate to https://permission.site. Click the "Notifications" button and press escape, and repeat this three times in a row until the permission prompt no longer appears. Opening the page info bubble should show the text "Automatically blocked" underneath the "Notification" permission. For the extensions string: Run Chrome and install the "Toggle Javascript" extension. Click on the icon to enable it, then open page info. Underneath Javascript, it should say "Controlled by an extension". For the policy string: No easy way to test this cross-platform. ==========
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...
Thanks for the review, PTAL. https://codereview.chromium.org/2743423004/diff/120001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2743423004/diff/120001/chrome/app/generated_r... chrome/app/generated_resources.grd:281: + <message name="IDS_WEBSITE_SETTINGS_PERMISSION_SET_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/03/29 05:49:26, lgarron wrote: > These should be called IDS_PAGE_INFO_* now. > > Ideally they should also be added to pageinfo_strings.grdp, but feel free to > leave them here until https://crbug.com/663975 Ah, thank you - lost in multiple rebases. Fixed, but left the strings in this file. Out of curiousity, is there a reason why some of them are PAGE_INFO and others are PAGEINFO? https://codereview.chromium.org/2743423004/diff/120001/chrome/app/generated_r... chrome/app/generated_resources.grd:282: + Set by enterprise policy On 2017/03/29 05:49:26, lgarron wrote: > I still have opinions about these strings, but I've brought it up on the email > thread. Changed this to use "Controlled by" as you suggested. Thanks! https://codereview.chromium.org/2743423004/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.mm (right): https://codereview.chromium.org/2743423004/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.mm:909: int maxY = NSMaxY(buttonFrame); On 2017/03/29 05:49:26, lgarron wrote: > I know the existing code uses maxY, but mainly to mean "the very bottom", and > but I'm finding it hard to follow the calculations in here. Is it possible to > keep using point.y? > > That is, something like: > > point.y = NSHeight(label.frame); > if (...) { > point.y += kPermissionsDecisionVerticalSpacing; > ... > point.y += label.frame.size.height; > } > > return NSMakePoint(NSMaxX(buttonFrame), point.y); > Done, thanks for the suggestions. I added a comment too to try and make it clearer. https://codereview.chromium.org/2743423004/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.mm:916: withSize:[NSFont systemFontSize] On 2017/03/29 05:49:26, lgarron wrote: > Looking at the mocks, it seems to be this should be smallSystemFontSize. I think there was a misunderstanding here - by mocks, do you mean the screenshots that were shared with the permissions team? Those are taken after having this CL patched in. The only other mocks I know of are "fully-MDified", and since the rest of the UI is not at that stage yet, it was agreed by Dom & UX that we would have an interim state which looked like the screenshots in the relevant email. I'm leaving the font size as it currently is now since I am assuming you would like page info to look like those screenshots, but here is a screenshot of what it would look like if the font was smallSystemFontSize. https://drive.google.com/open?id=0BzEa5HU1aAqBYkRRaE1oNHN0V1U Let me know if that's what you meant. https://codereview.chromium.org/2743423004/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.mm:920: label.textColor = skia::SkColorToSRGBNSColor(SK_ColorGRAY); On 2017/03/29 05:49:26, lgarron wrote: > Could you share the color across platforms by e.g. creating a > GetPermissionDecisionTextColor() function in PageInfoUI? Done. https://codereview.chromium.org/2743423004/diff/120001/chrome/browser/ui/page... File chrome/browser/ui/page_info/page_info_ui.cc (right): https://codereview.chromium.org/2743423004/diff/120001/chrome/browser/ui/page... chrome/browser/ui/page_info/page_info_ui.cc:228: On 2017/03/29 05:49:26, lgarron wrote: > Nit: avoid adding unneeded whitespace. Done. https://codereview.chromium.org/2743423004/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/page_info/permission_selector_row.cc (right): https://codereview.chromium.org/2743423004/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/page_info/permission_selector_row.cc:277: permission_decision_reason->SetEnabledColor(SK_ColorGRAY); On 2017/03/29 05:49:26, lgarron wrote: > Note that "High Contrast Black" mode on Windows has a black background for Page > Info. This shade of gray actually has pretty good contrast [1]; did the specs > explicitly cover this case, though? > > [1] http://leaverou.github.io/contrast-ratio/#black-on-%23888888 No, they don't - there actually aren't any for non-MD page info. Thanks for bringing this up and checking the contrast though. https://codereview.chromium.org/2743423004/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/page_info/permission_selector_row.cc:280: DCHECK(!!column_set); On 2017/03/29 05:49:26, lgarron wrote: > More of a question for me to learn: Is this pattern common? > Other Page Info DCHECKs just pass the pointer directly, and I can't find > documentation about the recommended approach. Good point - I've seen it before but I don't think it was common. Codesearch says there are only 15 instances places where "DCHECK(!!" is found, so I'll change it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
cocoa/ LGTM
views lgtm
Description was changed from ========== Permissions: Show the reason for permission decisions made on the user's behalf. A decision for a permission may be allowed or blocked on behalf of the user, either from one of the user's installed extensions, enforced by an enterprise policy, or via embargo. Currently, the former two reasons (extensions or policy) will be indicated to the user by custom strings in the menubutton or combobox associated with that permission in the website settings pop-up. This patch will put the reason for a permission decision being made underneath a permission in gray text and add strings to tell if the permission was blocked because of embargo for Views and Cocoa (Mac). BUG=679877 TEST=For the embargo string: Run Chrome with the command line flag --enable-features=BlockPromptsIfDismissedOften. Navigate to https://permission.site. Click the "Notifications" button and press escape, and repeat this three times in a row until the permission prompt no longer appears. Opening the page info bubble should show the text "Automatically blocked" underneath the "Notification" permission. For the extensions string: Run Chrome and install the "Toggle Javascript" extension. Click on the icon to enable it, then open page info. Underneath Javascript, it should say "Controlled by an extension". For the policy string: No easy way to test this cross-platform. ========== to ========== Permissions: Show the reason for permission decisions made on the user's behalf. A decision for a permission may be allowed or blocked on behalf of the user, either from one of the user's installed extensions, enforced by an enterprise policy, or via embargo. Currently, the former two reasons (extensions or policy) will be indicated to the user by custom strings in the menubutton or combobox associated with that permission in the website settings pop-up. This patch will put the reason for a permission decision being made underneath a permission in gray text and add strings to tell if the permission was blocked because of embargo for Views and Cocoa (Mac). See https://docs.google.com/document/d/1TaoszQOgqhoxZkV1zAHDapam-LIOxGBIUfqU6npoP... for the relevant document and screenshots. BUG=679877 TEST=For the embargo string: Run Chrome with the command line flag --enable-features=BlockPromptsIfDismissedOften. Navigate to https://permission.site. Click the "Notifications" button and press escape, and repeat this three times in a row until the permission prompt no longer appears. Opening the page info bubble should show the text "Automatically blocked" underneath the "Notification" permission. For the extensions string: Run Chrome and install the "Toggle Javascript" extension. Click on the icon to enable it, then open page info. Underneath Javascript, it should say "Controlled by an extension". For the policy string: No easy way to test this cross-platform. ==========
Ping for lgarron - are you fine with everything here? I've updated the CL description to include a link to the document, which has screenshots of the new strings. Thanks!
LGTM, although I filed a follow-up bug for myself about font sizes. https://codereview.chromium.org/2743423004/diff/120001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2743423004/diff/120001/chrome/app/generated_r... chrome/app/generated_resources.grd:281: + <message name="IDS_WEBSITE_SETTINGS_PERMISSION_SET_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/03/30 at 03:27:29, Patti Lor wrote: > On 2017/03/29 05:49:26, lgarron wrote: > > These should be called IDS_PAGE_INFO_* now. > > > > Ideally they should also be added to pageinfo_strings.grdp, but feel free to > > leave them here until https://crbug.com/663975 > > Ah, thank you - lost in multiple rebases. Fixed, but left the strings in this file. Out of curiousity, is there a reason why some of them are PAGE_INFO and others are PAGEINFO? Historical inconsistency. I've been renaming constants and files to use underscores whenever I touch them. https://codereview.chromium.org/2743423004/diff/120001/chrome/app/generated_r... chrome/app/generated_resources.grd:282: + Set by enterprise policy On 2017/03/30 at 03:27:29, Patti Lor wrote: > On 2017/03/29 05:49:26, lgarron wrote: > > I still have opinions about these strings, but I've brought it up on the email > > thread. > > Changed this to use "Controlled by" as you suggested. Thanks! Sounds like these are still a bit up in the air, but I think "Controlled" is the best choice for landing right now. Thanks. :-) https://codereview.chromium.org/2743423004/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.mm (right): https://codereview.chromium.org/2743423004/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.mm:916: withSize:[NSFont systemFontSize] On 2017/03/30 at 03:27:29, Patti Lor wrote: > On 2017/03/29 05:49:26, lgarron wrote: > > Looking at the mocks, it seems to be this should be smallSystemFontSize. > > I think there was a misunderstanding here - by mocks, do you mean the screenshots that were shared with the permissions team? Those are taken after having this CL patched in. The only other mocks I know of are "fully-MDified", and since the rest of the UI is not at that stage yet, it was agreed by Dom & UX that we would have an interim state which looked like the screenshots in the relevant email. I'm leaving the font size as it currently is now since I am assuming you would like page info to look like those screenshots, but here is a screenshot of what it would look like if the font was smallSystemFontSize. https://drive.google.com/open?id=0BzEa5HU1aAqBYkRRaE1oNHN0V1U > > Let me know if that's what you meant. Ah. So, this is partially my fault. Per the specs I was using [1], the permission label should use a larger font, while the dropdown is a smaller font. For some reason, the Views implementation doesn't have that. The screenshot uses the smaller font for the small part, but it's MacViews, which uses a smaller font for the permission label, which I must have missed changing to a larger label for https://crbug.com/512442 I've filed https://crbug.com/708358 for that. My expectation is that decision text would be small, matching the dropdown. (If not, then the Views permission label and decision string should both be made larger.) It would be nice to confirm with a designer, though. Feel free to pick one or the other for now. [1] https://bugs.chromium.org/p/chromium/issues/detail?id=512442&desc=2#c16
Thanks for all the details-oriented assistance here lgarron! :)
Thanks all for the reviews! https://codereview.chromium.org/2743423004/diff/120001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2743423004/diff/120001/chrome/app/generated_r... chrome/app/generated_resources.grd:281: + <message name="IDS_WEBSITE_SETTINGS_PERMISSION_SET_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/05 00:37:46, lgarron wrote: > On 2017/03/30 at 03:27:29, Patti Lor wrote: > > On 2017/03/29 05:49:26, lgarron wrote: > > > These should be called IDS_PAGE_INFO_* now. > > > > > > Ideally they should also be added to pageinfo_strings.grdp, but feel free to > > > leave them here until https://crbug.com/663975 > > > > Ah, thank you - lost in multiple rebases. Fixed, but left the strings in this > file. Out of curiousity, is there a reason why some of them are PAGE_INFO and > others are PAGEINFO? > > Historical inconsistency. I've been renaming constants and files to use > underscores whenever I touch them. Ah ok, they're all supposed to be PAGE_INFO. Thanks for the FYI :) https://codereview.chromium.org/2743423004/diff/120001/chrome/app/generated_r... chrome/app/generated_resources.grd:282: + Set by enterprise policy On 2017/04/05 00:37:46, lgarron wrote: > On 2017/03/30 at 03:27:29, Patti Lor wrote: > > On 2017/03/29 05:49:26, lgarron wrote: > > > I still have opinions about these strings, but I've brought it up on the > email > > > thread. > > > > Changed this to use "Controlled by" as you suggested. Thanks! > > Sounds like these are still a bit up in the air, but I think "Controlled" is the > best choice for landing right now. Thanks. :-) Acknowledged. https://codereview.chromium.org/2743423004/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.mm (right): https://codereview.chromium.org/2743423004/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.mm:916: withSize:[NSFont systemFontSize] On 2017/04/05 00:37:46, lgarron wrote: > On 2017/03/30 at 03:27:29, Patti Lor wrote: > > On 2017/03/29 05:49:26, lgarron wrote: > > > Looking at the mocks, it seems to be this should be smallSystemFontSize. > > > > I think there was a misunderstanding here - by mocks, do you mean the > screenshots that were shared with the permissions team? Those are taken after > having this CL patched in. The only other mocks I know of are "fully-MDified", > and since the rest of the UI is not at that stage yet, it was agreed by Dom & UX > that we would have an interim state which looked like the screenshots in the > relevant email. I'm leaving the font size as it currently is now since I am > assuming you would like page info to look like those screenshots, but here is a > screenshot of what it would look like if the font was smallSystemFontSize. > https://drive.google.com/open?id=0BzEa5HU1aAqBYkRRaE1oNHN0V1U > > > > Let me know if that's what you meant. > > Ah. So, this is partially my fault. > Per the specs I was using [1], the permission label should use a larger font, > while the dropdown is a smaller font. For some reason, the Views implementation > doesn't have that. > > The screenshot uses the smaller font for the small part, but it's MacViews, > which uses a smaller font for the permission label, which I must have missed > changing to a larger label for https://crbug.com/512442 > I've filed https://crbug.com/708358 for that. > > My expectation is that decision text would be small, matching the dropdown. (If > not, then the Views permission label and decision string should both be made > larger.) It would be nice to confirm with a designer, though. Feel free to pick > one or the other for now. > > [1] https://bugs.chromium.org/p/chromium/issues/detail?id=512442&desc=2#c16 Thanks for clarifying & following up on the font sizes :) I'll leave things as they are now since it sounds like there will need to be a change later either way.
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 Link to the patchset: https://codereview.chromium.org/2743423004/#ps140001 (title: "Review comments.")
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": 140001, "attempt_start_ts": 1491354254036100,
"parent_rev": "9ae05eaa32132bf42b7112acc1948f43142c548f", "commit_rev":
"217bb29f809ae73e537ec9c0eba5ac65f48fd873"}
Message was sent while issue was closed.
Description was changed from ========== Permissions: Show the reason for permission decisions made on the user's behalf. A decision for a permission may be allowed or blocked on behalf of the user, either from one of the user's installed extensions, enforced by an enterprise policy, or via embargo. Currently, the former two reasons (extensions or policy) will be indicated to the user by custom strings in the menubutton or combobox associated with that permission in the website settings pop-up. This patch will put the reason for a permission decision being made underneath a permission in gray text and add strings to tell if the permission was blocked because of embargo for Views and Cocoa (Mac). See https://docs.google.com/document/d/1TaoszQOgqhoxZkV1zAHDapam-LIOxGBIUfqU6npoP... for the relevant document and screenshots. BUG=679877 TEST=For the embargo string: Run Chrome with the command line flag --enable-features=BlockPromptsIfDismissedOften. Navigate to https://permission.site. Click the "Notifications" button and press escape, and repeat this three times in a row until the permission prompt no longer appears. Opening the page info bubble should show the text "Automatically blocked" underneath the "Notification" permission. For the extensions string: Run Chrome and install the "Toggle Javascript" extension. Click on the icon to enable it, then open page info. Underneath Javascript, it should say "Controlled by an extension". For the policy string: No easy way to test this cross-platform. ========== to ========== Permissions: Show the reason for permission decisions made on the user's behalf. A decision for a permission may be allowed or blocked on behalf of the user, either from one of the user's installed extensions, enforced by an enterprise policy, or via embargo. Currently, the former two reasons (extensions or policy) will be indicated to the user by custom strings in the menubutton or combobox associated with that permission in the website settings pop-up. This patch will put the reason for a permission decision being made underneath a permission in gray text and add strings to tell if the permission was blocked because of embargo for Views and Cocoa (Mac). See https://docs.google.com/document/d/1TaoszQOgqhoxZkV1zAHDapam-LIOxGBIUfqU6npoP... for the relevant document and screenshots. BUG=679877 TEST=For the embargo string: Run Chrome with the command line flag --enable-features=BlockPromptsIfDismissedOften. Navigate to https://permission.site. Click the "Notifications" button and press escape, and repeat this three times in a row until the permission prompt no longer appears. Opening the page info bubble should show the text "Automatically blocked" underneath the "Notification" permission. For the extensions string: Run Chrome and install the "Toggle Javascript" extension. Click on the icon to enable it, then open page info. Underneath Javascript, it should say "Controlled by an extension". For the policy string: No easy way to test this cross-platform. Review-Url: https://codereview.chromium.org/2743423004 Cr-Commit-Position: refs/heads/master@{#461944} Committed: https://chromium.googlesource.com/chromium/src/+/217bb29f809ae73e537ec9c0eba5... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/217bb29f809ae73e537ec9c0eba5... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
