Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2165733003:
Add revocation reporter to permission util (Closed)
|
Created:
4 years, 5 months ago by stefanocs Modified:
4 years, 4 months ago CC:
chromium-reviews, mlamouri+watch-permissions_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@add-source-ui-to-permission-report Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd revocation reporter to permission util
A new class ScopedRevocationReporter is added to permission_util.h to replace PermissionUmaUtil::SetContentSettingAndRecordRevocation for reporting revocations.
The revocation metrics are sent when the initial resolved content setting is allow and changed when ScopedRevocationReporter is destructed.
BUG=613883, 469221
Committed: https://crrev.com/d3dae8da916baaaa8cd155e73440bc8b6b6019e1
Cr-Commit-Position: refs/heads/master@{#408338}
Patch Set 1 #Patch Set 2 : fix typos #Patch Set 3 : Add is empty check #Patch Set 4 : merge #
Total comments: 4
Patch Set 5 : Add missing return #
Total comments: 12
Patch Set 6 : Update #Patch Set 7 : rebase #Patch Set 8 : fix typo #Patch Set 9 : add include #Patch Set 10 : Add missing include #Patch Set 11 : Remove references from url type #
Total comments: 2
Patch Set 12 : Rebase #
Dependent Patchsets: Messages
Total messages: 49 (33 generated)
Description was changed from ========== Add revocation reporter to permission util This class replaces PermissionUmaUtil::SetContentSettingAndRecordRevocation to report revocations. The revocation metrics are sent when the initial resolved content setting is allow and it is changed when RevocationReporter is destructed. BUG=613883 ========== to ========== Add revocation reporter to permission util This class replaces PermissionUmaUtil::SetContentSettingAndRecordRevocation to report revocations. The revocation metrics are sent when the initial resolved content setting is allow and changed when RevocationReporter is destructed. BUG=613883 ==========
stefanocs@google.com changed reviewers: + kcarattini@chromium.org, raymes@chromium.org, tsergeant@chromium.org
Hi all, Please review this cl. Thanks!
https://codereview.chromium.org/2165733003/diff/60001/chrome/browser/permissi... File chrome/browser/permissions/permission_util.cc (right): https://codereview.chromium.org/2165733003/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_util.cc:115: GURL(primary_pattern.ToString()), Is this version of the constructor currently used? It seems useful, but I'd be worried about just converting the pattern to a string. What happens if the pattern has a wildcard in it?
https://codereview.chromium.org/2165733003/diff/60001/chrome/browser/permissi... File chrome/browser/permissions/permission_util.cc (right): https://codereview.chromium.org/2165733003/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_util.cc:115: GURL(primary_pattern.ToString()), On 2016/07/21 01:32:52, tsergeant wrote: > Is this version of the constructor currently used? It seems useful, but I'd be > worried about just converting the pattern to a string. What happens if the > pattern has a wildcard in it? Not in this cl, but we are planning to use this for recording revocation in content settings (I'm sure if this constructor should be added on that cl instead). The revocation should only be recorded if the pattern string itself is a valid url (checked in the first constructor).
lgtm https://codereview.chromium.org/2165733003/diff/60001/chrome/browser/permissi... File chrome/browser/permissions/permission_util.cc (right): https://codereview.chromium.org/2165733003/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_util.cc:115: GURL(primary_pattern.ToString()), On 2016/07/21 01:45:46, stefanocs wrote: > On 2016/07/21 01:32:52, tsergeant wrote: > > Is this version of the constructor currently used? It seems useful, but I'd be > > worried about just converting the pattern to a string. What happens if the > > pattern has a wildcard in it? > > Not in this cl, but we are planning to use this for recording revocation in > content settings (I'm sure if this constructor should be added on that cl > instead). The revocation should only be recorded if the pattern string itself is > a valid url (checked in the first constructor). Alright, sounds good to me.
Just some minor comments https://codereview.chromium.org/2165733003/diff/60001/chrome/browser/permissi... File chrome/browser/permissions/permission_util.h (right): https://codereview.chromium.org/2165733003/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_util.h:51: std::string resource_identifier, nit: do we need to pass this in? https://codereview.chromium.org/2165733003/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_util.cc (right): https://codereview.chromium.org/2165733003/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_util.cc:89: : profile_(profile), nit: Rather than passing the profile, should we pass the HostContentSettingsMap? It will probably be needed by the caller anyway and it saves getting to it from here. https://codereview.chromium.org/2165733003/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_util.cc:104: is_initially_allowed_ = (initial_content_setting == CONTENT_SETTING_ALLOW); nit: no () needed https://codereview.chromium.org/2165733003/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_util.cc:122: source_ui) {} nit: can we leave this constructor out until the next CL? https://codereview.chromium.org/2165733003/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_util_unittest.cc (right): https://codereview.chromium.org/2165733003/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_util_unittest.cc:42: PermissionUtil::RevocationReporter scoped_revocation_reporter( Calling the class ScopedRevocationReporter is actually a good idea I think https://codereview.chromium.org/2165733003/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_util_unittest.cc:70: CONTENT_SETTING_DEFAULT); These tests don't exercise the new constructor. But I'd suggest we add that when it is used. https://codereview.chromium.org/2165733003/diff/80001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/2165733003/diff/80001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:959: std::string(), Will the resource_identifier argument be used in a followup CL? Maybe remove it for now too.
Thanks! https://codereview.chromium.org/2165733003/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_util.cc (right): https://codereview.chromium.org/2165733003/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_util.cc:89: : profile_(profile), On 2016/07/21 23:08:01, raymes wrote: > nit: Rather than passing the profile, should we pass the HostContentSettingsMap? > It will probably be needed by the caller anyway and it saves getting to it from > here. We need the profile to check if the user opted into the permission reporting in permission uma util. https://codereview.chromium.org/2165733003/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_util.cc:104: is_initially_allowed_ = (initial_content_setting == CONTENT_SETTING_ALLOW); On 2016/07/21 23:08:01, raymes wrote: > nit: no () needed Done. https://codereview.chromium.org/2165733003/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_util.cc:122: source_ui) {} On 2016/07/21 23:08:01, raymes wrote: > nit: can we leave this constructor out until the next CL? Done. https://codereview.chromium.org/2165733003/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_util_unittest.cc (right): https://codereview.chromium.org/2165733003/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_util_unittest.cc:42: PermissionUtil::RevocationReporter scoped_revocation_reporter( On 2016/07/21 23:08:01, raymes wrote: > Calling the class ScopedRevocationReporter is actually a good idea I think Done. https://codereview.chromium.org/2165733003/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_util_unittest.cc:70: CONTENT_SETTING_DEFAULT); On 2016/07/21 23:08:01, raymes wrote: > These tests don't exercise the new constructor. But I'd suggest we add that when > it is used. Acknowledged. https://codereview.chromium.org/2165733003/diff/80001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/2165733003/diff/80001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:959: std::string(), On 2016/07/21 23:08:01, raymes wrote: > Will the resource_identifier argument be used in a followup CL? Maybe remove it > for now too. Done. Probably not, so I am removing this for now.
The CQ bit was checked by stefanocs@google.com 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_asan_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 stefanocs@google.com 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 stefanocs@google.com 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_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 stefanocs@google.com 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.
lgtm
lgtm
stefanocs@google.com changed reviewers: + bauerb@chromium.org, dtrainor@chromium.org
Thanks! dtrainor@chromium.org: Please review changes in chrome/browser/android/preferences/website_preference_bridge.cc bauerb@chromium.org: Please review changes in chrome/browser/ui/content_settings/content_setting_bubble_model.cc
lgtm
The CQ bit was checked by stefanocs@google.com 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/2165733003/diff/200001/chrome/browser/permiss... File chrome/browser/permissions/permission_util.h (left): https://codereview.chromium.org/2165733003/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_util.h:46: // TODO(tsergeant): This is a temporary solution to begin gathering metrics. @tseargeant, I take it you consider this TODO completed?
Description was changed from ========== Add revocation reporter to permission util This class replaces PermissionUmaUtil::SetContentSettingAndRecordRevocation to report revocations. The revocation metrics are sent when the initial resolved content setting is allow and changed when RevocationReporter is destructed. BUG=613883 ========== to ========== Add revocation reporter to permission util This class replaces PermissionUmaUtil::SetContentSettingAndRecordRevocation to report revocations. The revocation metrics are sent when the initial resolved content setting is allow and changed when RevocationReporter is destructed. BUG=613883,469221 ==========
https://codereview.chromium.org/2165733003/diff/200001/chrome/browser/permiss... File chrome/browser/permissions/permission_util.h (left): https://codereview.chromium.org/2165733003/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_util.h:46: // TODO(tsergeant): This is a temporary solution to begin gathering metrics. On 2016/07/26 03:49:07, kcarattini wrote: > @tseargeant, I take it you consider this TODO completed? Yup, I think this is a better long-term solution, so I'm happy for the TODO to disappear!
Description was changed from ========== Add revocation reporter to permission util This class replaces PermissionUmaUtil::SetContentSettingAndRecordRevocation to report revocations. The revocation metrics are sent when the initial resolved content setting is allow and changed when RevocationReporter is destructed. BUG=613883,469221 ========== to ========== Add revocation reporter to permission util A new class ScopedRevocationReporter is added to permission_util.h to replace PermissionUmaUtil::SetContentSettingAndRecordRevocation for reporting revocations. The revocation metrics are sent when the initial resolved content setting is allow and changed when ScopedRevocationReporter is destructed. BUG=613883,469221 ==========
website_preferences_bridge lgtm
The CQ bit was checked by stefanocs@google.com 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 stefanocs@google.com 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 stefanocs@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from tsergeant@chromium.org, bauerb@chromium.org, kcarattini@chromium.org, raymes@chromium.org, dtrainor@chromium.org Link to the patchset: https://codereview.chromium.org/2165733003/#ps220001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add revocation reporter to permission util A new class ScopedRevocationReporter is added to permission_util.h to replace PermissionUmaUtil::SetContentSettingAndRecordRevocation for reporting revocations. The revocation metrics are sent when the initial resolved content setting is allow and changed when ScopedRevocationReporter is destructed. BUG=613883,469221 ========== to ========== Add revocation reporter to permission util A new class ScopedRevocationReporter is added to permission_util.h to replace PermissionUmaUtil::SetContentSettingAndRecordRevocation for reporting revocations. The revocation metrics are sent when the initial resolved content setting is allow and changed when ScopedRevocationReporter is destructed. BUG=613883,469221 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Add revocation reporter to permission util A new class ScopedRevocationReporter is added to permission_util.h to replace PermissionUmaUtil::SetContentSettingAndRecordRevocation for reporting revocations. The revocation metrics are sent when the initial resolved content setting is allow and changed when ScopedRevocationReporter is destructed. BUG=613883,469221 ========== to ========== Add revocation reporter to permission util A new class ScopedRevocationReporter is added to permission_util.h to replace PermissionUmaUtil::SetContentSettingAndRecordRevocation for reporting revocations. The revocation metrics are sent when the initial resolved content setting is allow and changed when ScopedRevocationReporter is destructed. BUG=613883,469221 Committed: https://crrev.com/d3dae8da916baaaa8cd155e73440bc8b6b6019e1 Cr-Commit-Position: refs/heads/master@{#408338} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/d3dae8da916baaaa8cd155e73440bc8b6b6019e1 Cr-Commit-Position: refs/heads/master@{#408338} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
