Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2790473004:
Permissions: Clear embargo if user changes an embargoed permission's setting. (Closed)
DescriptionPermissions: Clear embargo if user changes an embargoed permission's setting.
Allow the embargo status of a permission to be cleared when the user changes the
setting for an embargoed permission to "Ask" or "Allow". However, the system
will still remember that the setting was previously embargoed, so if another
permission prompt is dismissed for that permission, it will be re-embargoed.
Permissions embargoed because they are blacklisted will be re-embargoed if they
are still on the blacklist during the next permission request.
BUG=704771
TEST=Run Chrome with the command line flag
--enable-features=BlockPromptsIfDismissedOften and navigate to
https://permission.site. Place the notification permission under embargo by
clicking the "Notifications" button three times until the permission prompt no
longer appears. In the page info bubble, the "Notifications" drop-down should
say "block". Choose "Allow" instead, which should update the bubble to allow
notifications and ask for the page to be reloaded. Then change the drop-down to
"Ask", which should also update the notifications permission back to "Ask".
Click the "Notifications" button on the page again and notice the permission
prompt to ask for notifications permission is shown again.
Review-Url: https://codereview.chromium.org/2790473004
Cr-Commit-Position: refs/heads/master@{#462733}
Committed: https://chromium.googlesource.com/chromium/src/+/7131c1fee08ad62e85476acfd3604765bc997f39
Patch Set 1 #
Total comments: 16
Patch Set 2 : Review comments. #Patch Set 3 : Hook up for permissions on Android. #
Total comments: 4
Patch Set 4 : Review comments. #
Total comments: 2
Patch Set 5 : Review comments. #
Messages
Total messages: 49 (36 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: This issue passed the CQ dry run.
patricialor@chromium.org changed reviewers: + dominickn@chromium.org
Hey Dom, do you think it's also worth adding a couple tests with OnPermissionChanged() and RequestPermission() in page_info_unittest.cc or something? Not sure if this is enough. PTAL otherwise though, thank you :)
Thanks Patti! Looks pretty good! Mostly nits. https://codereview.chromium.org/2790473004/diff/1/chrome/browser/permissions/... File chrome/browser/permissions/permission_decision_auto_blocker.cc (right): https://codereview.chromium.org/2790473004/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker.cc:311: DCHECK(permission_dict->RemoveWithoutPathExpansion( I think you need to do this outside a DCHECK, or else it might be compiled out when DCHECKs aren't enabled. https://codereview.chromium.org/2790473004/diff/1/chrome/browser/permissions/... File chrome/browser/permissions/permission_decision_auto_blocker.h (right): https://codereview.chromium.org/2790473004/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker.h:100: void RemoveEmbargoByURL(const GURL& url, ContentSettingsType permission); Nit: call this RemoveEmbargoByUrl. "Removes the embargo status for |url|." https://codereview.chromium.org/2790473004/diff/1/chrome/browser/permissions/... File chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc (right): https://codereview.chromium.org/2790473004/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:162: // Check removing the the emargo for a single permission on a site works, and sp. embargo https://codereview.chromium.org/2790473004/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:165: TEST_F(PermissionDecisionAutoBlockerUnitTest, RemoveEmbargoByURL) { Url https://codereview.chromium.org/2790473004/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:170: autoblocker()->RecordDismissAndEmbargo(url1, I'd EXPECT_FALSE / EXPECT_TRUE all of these calls (returns true when it's actually embargoed). https://codereview.chromium.org/2790473004/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:256: autoblocker()->RecordDismissAndEmbargo(url, EXPECT_TRUE / EXPECT_FALSE these calls https://codereview.chromium.org/2790473004/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:273: Nit: verify that it's not under embargo here https://codereview.chromium.org/2790473004/diff/1/chrome/browser/ui/page_info... File chrome/browser/ui/page_info/page_info.cc (right): https://codereview.chromium.org/2790473004/diff/1/chrome/browser/ui/page_info... chrome/browser/ui/page_info/page_info.cc:307: this->profile_, this->site_url_, this->site_url_, type, Nit: the this-> usage here and below is weird (only place in this file). It's not very C++. Can we remove this-> here and below?
One more thing - I think you can hook this up on Android as well in chrome/browser/android/preferences/website_preference_bridge.cc::SetSettingForOrigin. If you're okay with doing it here that's cool, otherwise a follow up or I can look at it. :)
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...
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_...) 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 patricialor@chromium.org to run a CQ dry run
Patchset #3 (id:40001) has been deleted
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_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Patchset #3 (id:60001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks Dom, PTAL - I did a bit of refactoring of the existing code as well to avoid too much code duplication, because stuff needs to get called in two places on Android (notifications vs everything else). Tested manually on Android and it works, but let me know if you wanted to add tests there too. https://codereview.chromium.org/2790473004/diff/1/chrome/browser/permissions/... File chrome/browser/permissions/permission_decision_auto_blocker.cc (right): https://codereview.chromium.org/2790473004/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker.cc:311: DCHECK(permission_dict->RemoveWithoutPathExpansion( On 2017/04/03 01:35:30, dominickn wrote: > I think you need to do this outside a DCHECK, or else it might be compiled out > when DCHECKs aren't enabled. Oh, you're totally right. Thanks Dom, fixed. https://codereview.chromium.org/2790473004/diff/1/chrome/browser/permissions/... File chrome/browser/permissions/permission_decision_auto_blocker.h (right): https://codereview.chromium.org/2790473004/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker.h:100: void RemoveEmbargoByURL(const GURL& url, ContentSettingsType permission); On 2017/04/03 01:35:30, dominickn wrote: > Nit: call this RemoveEmbargoByUrl. > > "Removes the embargo status for |url|." Done. https://codereview.chromium.org/2790473004/diff/1/chrome/browser/permissions/... File chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc (right): https://codereview.chromium.org/2790473004/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:162: // Check removing the the emargo for a single permission on a site works, and On 2017/04/03 01:35:31, dominickn wrote: > sp. embargo Done. https://codereview.chromium.org/2790473004/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:165: TEST_F(PermissionDecisionAutoBlockerUnitTest, RemoveEmbargoByURL) { On 2017/04/03 01:35:30, dominickn wrote: > Url Done. https://codereview.chromium.org/2790473004/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:170: autoblocker()->RecordDismissAndEmbargo(url1, On 2017/04/03 01:35:31, dominickn wrote: > I'd EXPECT_FALSE / EXPECT_TRUE all of these calls (returns true when it's > actually embargoed). Done. https://codereview.chromium.org/2790473004/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:256: autoblocker()->RecordDismissAndEmbargo(url, On 2017/04/03 01:35:31, dominickn wrote: > EXPECT_TRUE / EXPECT_FALSE these calls Done. https://codereview.chromium.org/2790473004/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:273: On 2017/04/03 01:35:31, dominickn wrote: > Nit: verify that it's not under embargo here Done. https://codereview.chromium.org/2790473004/diff/1/chrome/browser/ui/page_info... File chrome/browser/ui/page_info/page_info.cc (right): https://codereview.chromium.org/2790473004/diff/1/chrome/browser/ui/page_info... chrome/browser/ui/page_info/page_info.cc:307: this->profile_, this->site_url_, this->site_url_, type, On 2017/04/03 01:35:31, dominickn wrote: > Nit: the this-> usage here and below is weird (only place in this file). It's > not very C++. Can we remove this-> here and below? Sorry, I thought for some reason there was a reason for that and was trying to be consistent - it doesn't actually look like there is though. Fixed, thanks!
https://codereview.chromium.org/2790473004/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker.cc (right): https://codereview.chromium.org/2790473004/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:317: PermissionManager::Get(profile_)->GetPermissionStatus( Instead of calling PermissionManager (and thus requiring an embedder URL), you can just call: PermissionResult result = GetEmbargoResult(origin_url, permission); (that's what PermissionManager eventually calls into to check embargo) Then you can remove the embedder URL from everywhere, since embargo doesn't care about it (and I'm pretty sure it's actually empty for all permissions now). You also don't need the cyclic header include back to permission_manager.h https://codereview.chromium.org/2790473004/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:319: switch (permission_result.source) { I think it's clearer to just do if (result.source != PermissionStatusSource::MULTIPLE_DISMISSALS || result.source != PermissionStatusSource::SAFE_BROWSING_BLACKLIST) { return; }
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2790473004/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker.cc (right): https://codereview.chromium.org/2790473004/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:317: PermissionManager::Get(profile_)->GetPermissionStatus( On 2017/04/05 08:50:19, dominickn wrote: > Instead of calling PermissionManager (and thus requiring an embedder URL), you > can just call: > > PermissionResult result = GetEmbargoResult(origin_url, permission); > > (that's what PermissionManager eventually calls into to check embargo) > > Then you can remove the embedder URL from everywhere, since embargo doesn't care > about it (and I'm pretty sure it's actually empty for all permissions now). You > also don't need the cyclic header include back to permission_manager.h Done, thanks Dom :) https://codereview.chromium.org/2790473004/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:319: switch (permission_result.source) { On 2017/04/05 08:50:19, dominickn wrote: > I think it's clearer to just do > > if (result.source != PermissionStatusSource::MULTIPLE_DISMISSALS || > result.source != PermissionStatusSource::SAFE_BROWSING_BLACKLIST) { > return; > } Done.
lgtm, thanks!
patricialor@chromium.org changed reviewers: + dfalcantara@chromium.org, raymes@chromium.org
Thanks Dom! dfalcantara@chromium.org: Please review changes in chrome/browser/android/* raymes@chromium.org: Please review changes in chrome/browser/permissions/* and chrome/browser/ui/page_info/*
lgtm https://codereview.chromium.org/2790473004/diff/100001/chrome/browser/permiss... File chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc (right): https://codereview.chromium.org/2790473004/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:211: // Notification's default setting is |CONTENT_SETTING_ASK|. nit: I think it's not that notifications default is CONTENT_SETTING_ASK but GetEmbargoResult will always return ASK if it's not embargoed
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 Raymes! https://codereview.chromium.org/2790473004/diff/100001/chrome/browser/permiss... File chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc (right): https://codereview.chromium.org/2790473004/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:211: // Notification's default setting is |CONTENT_SETTING_ASK|. On 2017/04/06 04:45:35, raymes wrote: > nit: I think it's not that notifications default is CONTENT_SETTING_ASK but > GetEmbargoResult will always return ASK if it's not embargoed Oh, you're totally right - thanks. Updated the comment.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
c/b/android lgtm
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, raymes@chromium.org Link to the patchset: https://codereview.chromium.org/2790473004/#ps120001 (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": 120001, "attempt_start_ts": 1491524519680090,
"parent_rev": "aee0c7172b135b405d7b0b8f8a92077e02052ff6", "commit_rev":
"7131c1fee08ad62e85476acfd3604765bc997f39"}
Message was sent while issue was closed.
Description was changed from ========== Permissions: Clear embargo if user changes an embargoed permission's setting. Allow the embargo status of a permission to be cleared when the user changes the setting for an embargoed permission to "Ask" or "Allow". However, the system will still remember that the setting was previously embargoed, so if another permission prompt is dismissed for that permission, it will be re-embargoed. Permissions embargoed because they are blacklisted will be re-embargoed if they are still on the blacklist during the next permission request. BUG=704771 TEST=Run Chrome with the command line flag --enable-features=BlockPromptsIfDismissedOften and navigate to https://permission.site. Place the notification permission under embargo by clicking the "Notifications" button three times until the permission prompt no longer appears. In the page info bubble, the "Notifications" drop-down should say "block". Choose "Allow" instead, which should update the bubble to allow notifications and ask for the page to be reloaded. Then change the drop-down to "Ask", which should also update the notifications permission back to "Ask". Click the "Notifications" button on the page again and notice the permission prompt to ask for notifications permission is shown again. ========== to ========== Permissions: Clear embargo if user changes an embargoed permission's setting. Allow the embargo status of a permission to be cleared when the user changes the setting for an embargoed permission to "Ask" or "Allow". However, the system will still remember that the setting was previously embargoed, so if another permission prompt is dismissed for that permission, it will be re-embargoed. Permissions embargoed because they are blacklisted will be re-embargoed if they are still on the blacklist during the next permission request. BUG=704771 TEST=Run Chrome with the command line flag --enable-features=BlockPromptsIfDismissedOften and navigate to https://permission.site. Place the notification permission under embargo by clicking the "Notifications" button three times until the permission prompt no longer appears. In the page info bubble, the "Notifications" drop-down should say "block". Choose "Allow" instead, which should update the bubble to allow notifications and ask for the page to be reloaded. Then change the drop-down to "Ask", which should also update the notifications permission back to "Ask". Click the "Notifications" button on the page again and notice the permission prompt to ask for notifications permission is shown again. Review-Url: https://codereview.chromium.org/2790473004 Cr-Commit-Position: refs/heads/master@{#462733} Committed: https://chromium.googlesource.com/chromium/src/+/7131c1fee08ad62e85476acfd360... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001) as https://chromium.googlesource.com/chromium/src/+/7131c1fee08ad62e85476acfd360... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
