Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2180723002:
Add revocation metrics from OIB and content setting (Closed)
|
Created:
4 years, 5 months ago by stefanocs Modified:
4 years, 4 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, msramek+watch_chromium.org, michaelpg+watch-options_chromium.org, raymes+watch_chromium.org, mlamouri+watch-permissions_chromium.org, markusheintz_, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@revocation-reporter Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd revocations from OIB and content setting
Revocations from OIB are recorded from chrome/browser/ui/website_settings/website_settings.cc
Revocations from content setting are recorded from chrome/browser/ui/webui/options/content_settings_handler.cc. Revocations are only recorded from content setting if the pattern is a valid host string with no wildcards. We also refactored GetContentSettingsMap to GetProfile and GetOTRContentSettingsMap to GetOTRProfile since we need to pass in the profile to ScopedRevocationReporter.
BUG=613883, 469221
Committed: https://crrev.com/8b3490ccac018eee6b75c9f64d30700fb1d40d69
Cr-Commit-Position: refs/heads/master@{#408341}
Patch Set 1 #Patch Set 2 : Fix #Patch Set 3 : Typo #Patch Set 4 : Fix test #Patch Set 5 : Add setter for settings map #
Total comments: 4
Patch Set 6 : Remove set custom host map #
Total comments: 6
Patch Set 7 : Change Get(OTR)ContentSettingsMap to Get(OTR)Profile #
Total comments: 6
Patch Set 8 : Refactor #Patch Set 9 : Rebase #Patch Set 10 : Rebase #
Messages
Total messages: 46 (30 generated)
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_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Add revocation metrics from OIB and content setting BUG=613883 ========== to ========== Add revocations from OIB and content setting Revocations from OIB are recorded from chrome/browser/ui/website_settings/website_settings.cc Revocations from content setting are recorded from chrome/browser/ui/webui/options/content_settings_handler.cc. Revocations are only recorded from content setting if the pattern is a valid host string with no wildcards. BUG=613883 ==========
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...
stefanocs@google.com changed reviewers: + kcarattini@chromium.org, raymes@chromium.org
Hi, please review this cl. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2180723002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/content_settings_handler.cc (right): https://codereview.chromium.org/2180723002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/content_settings_handler.cc:1321: scoped_revocation_reporter.SetCustomSettingsMap(settings_map); I think we should just pass the OTR profile into the ScopedRevocationReporter - see profile->GetOffTheRecordProfile(). In fact we don't report changes to the OTR profile (see PermissionUmaUtil::IsOptedIntoPermissionActionReporting), so it's important that we pass the right profile or we will send reports when we shouldn't. https://codereview.chromium.org/2180723002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/content_settings_handler.cc:1324: ContentSettingsPattern::FromString(pattern), Can we pull this out above and have ContentSettingsPattern primary_pattern = ... ContentSettingsPattern secondary_pattern = ...
https://codereview.chromium.org/2180723002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/content_settings_handler.cc (right): https://codereview.chromium.org/2180723002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/content_settings_handler.cc:1321: scoped_revocation_reporter.SetCustomSettingsMap(settings_map); On 2016/07/26 01:18:31, raymes wrote: > I think we should just pass the OTR profile into the ScopedRevocationReporter - > see profile->GetOffTheRecordProfile(). In fact we don't report changes to the > OTR profile (see PermissionUmaUtil::IsOptedIntoPermissionActionReporting), so > it's important that we pass the right profile or we will send reports when we > shouldn't. Done. https://codereview.chromium.org/2180723002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/content_settings_handler.cc:1324: ContentSettingsPattern::FromString(pattern), On 2016/07/26 01:18:31, raymes wrote: > Can we pull this out above and have > > ContentSettingsPattern primary_pattern = ... > ContentSettingsPattern secondary_pattern = ... Done.
Description was changed from ========== Add revocations from OIB and content setting Revocations from OIB are recorded from chrome/browser/ui/website_settings/website_settings.cc Revocations from content setting are recorded from chrome/browser/ui/webui/options/content_settings_handler.cc. Revocations are only recorded from content setting if the pattern is a valid host string with no wildcards. BUG=613883 ========== to ========== Add revocations from OIB and content setting Revocations from OIB are recorded from chrome/browser/ui/website_settings/website_settings.cc Revocations from content setting are recorded from chrome/browser/ui/webui/options/content_settings_handler.cc. Revocations are only recorded from content setting if the pattern is a valid host string with no wildcards. BUG=613883,469221 ==========
kcarattini@chromium.org changed reviewers: + tsergeant@chromium.org
https://codereview.chromium.org/2180723002/diff/100001/chrome/browser/ui/webs... File chrome/browser/ui/website_settings/website_settings.cc (left): https://codereview.chromium.org/2180723002/diff/100001/chrome/browser/ui/webs... chrome/browser/ui/website_settings/website_settings.cc:292: // TODO(tsergeant): Integrate this with the revocation recording performed Tim, is this TODO completed now? I'm not really sure what you meant by it.
https://codereview.chromium.org/2180723002/diff/100001/chrome/browser/ui/webs... File chrome/browser/ui/website_settings/website_settings.cc (left): https://codereview.chromium.org/2180723002/diff/100001/chrome/browser/ui/webs... chrome/browser/ui/website_settings/website_settings.cc:292: // TODO(tsergeant): Integrate this with the revocation recording performed On 2016/07/26 03:54:20, kcarattini wrote: > Tim, is this TODO completed now? I'm not really sure what you meant by it. Yup, it's also done. It's referring to using the same PermissionUtil revocation helper here as everywhere else.
https://codereview.chromium.org/2180723002/diff/100001/chrome/browser/permiss... File chrome/browser/permissions/permission_util.h (right): https://codereview.chromium.org/2180723002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_util.h:46: // setter if necessary. This comment is incorrect now https://codereview.chromium.org/2180723002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/content_settings_handler.cc (right): https://codereview.chromium.org/2180723002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/content_settings_handler.cc:1314: profile = profile->GetOffTheRecordProfile(); I think we should probably simplify this - possibly remove the GetContentSettingsMap and GetOTRContentSettingsMap and replace with GetProfile and GetOTRProfile and change the callsites to get the HCSM from the profile. Otherwise I'm afraid we might introduce a bug (there's some extra logic in GetOTRContentSettingsMap
https://codereview.chromium.org/2180723002/diff/100001/chrome/browser/permiss... File chrome/browser/permissions/permission_util.h (right): https://codereview.chromium.org/2180723002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_util.h:46: // setter if necessary. On 2016/07/26 07:50:03, raymes wrote: > This comment is incorrect now Done. https://codereview.chromium.org/2180723002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/content_settings_handler.cc (right): https://codereview.chromium.org/2180723002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/content_settings_handler.cc:1314: profile = profile->GetOffTheRecordProfile(); On 2016/07/26 07:50:03, raymes wrote: > I think we should probably simplify this - possibly remove the > GetContentSettingsMap and GetOTRContentSettingsMap and replace with GetProfile > and GetOTRProfile and change the callsites to get the HCSM from the profile. > Otherwise I'm afraid we might introduce a bug (there's some extra logic in > GetOTRContentSettingsMap Done.
lgtm
lgtm
Description was changed from ========== Add revocations from OIB and content setting Revocations from OIB are recorded from chrome/browser/ui/website_settings/website_settings.cc Revocations from content setting are recorded from chrome/browser/ui/webui/options/content_settings_handler.cc. Revocations are only recorded from content setting if the pattern is a valid host string with no wildcards. BUG=613883,469221 ========== to ========== Add revocations from OIB and content setting Revocations from OIB are recorded from chrome/browser/ui/website_settings/website_settings.cc Revocations from content setting are recorded from chrome/browser/ui/webui/options/content_settings_handler.cc. Revocations are only recorded from content setting if the pattern is a valid host string with no wildcards. We also refactored GetContentSettingsMap to GetProfile and GetOTRContentSettingsMap to GetOTRProfile since we need to pass in the profile to ScopedRevocationReporter. BUG=613883,469221 ==========
stefanocs@google.com changed reviewers: + stevenjb@chromium.org
Thanks. stevenjb@chromium.org: Please review changes in chrome/browser/ui/webui/options/
lgtm w/ nits https://codereview.chromium.org/2180723002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/content_settings_handler.cc (right): https://codereview.chromium.org/2180723002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/content_settings_handler.cc:1314: if (profile) { if (!profile) return; https://codereview.chromium.org/2180723002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/content_settings_handler.cc:1483: NOTREACHED(); Can you replace this with a DCHECK? https://codereview.chromium.org/2180723002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/content_settings_handler.cc:1484: } else { No else.
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...
Thanks! ping Tim. https://codereview.chromium.org/2180723002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/content_settings_handler.cc (right): https://codereview.chromium.org/2180723002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/content_settings_handler.cc:1314: if (profile) { On 2016/07/27 16:37:07, stevenjb wrote: > if (!profile) return; Done. https://codereview.chromium.org/2180723002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/content_settings_handler.cc:1483: NOTREACHED(); On 2016/07/27 16:37:07, stevenjb wrote: > Can you replace this with a DCHECK? Done. https://codereview.chromium.org/2180723002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/content_settings_handler.cc:1484: } else { On 2016/07/27 16:37:07, stevenjb wrote: > No else. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
oops, 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: Try jobs failed on following builders: ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
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 kcarattini@chromium.org, stevenjb@chromium.org, raymes@chromium.org, tsergeant@chromium.org Link to the patchset: https://codereview.chromium.org/2180723002/#ps180001 (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 revocations from OIB and content setting Revocations from OIB are recorded from chrome/browser/ui/website_settings/website_settings.cc Revocations from content setting are recorded from chrome/browser/ui/webui/options/content_settings_handler.cc. Revocations are only recorded from content setting if the pattern is a valid host string with no wildcards. We also refactored GetContentSettingsMap to GetProfile and GetOTRContentSettingsMap to GetOTRProfile since we need to pass in the profile to ScopedRevocationReporter. BUG=613883,469221 ========== to ========== Add revocations from OIB and content setting Revocations from OIB are recorded from chrome/browser/ui/website_settings/website_settings.cc Revocations from content setting are recorded from chrome/browser/ui/webui/options/content_settings_handler.cc. Revocations are only recorded from content setting if the pattern is a valid host string with no wildcards. We also refactored GetContentSettingsMap to GetProfile and GetOTRContentSettingsMap to GetOTRProfile since we need to pass in the profile to ScopedRevocationReporter. BUG=613883,469221 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Add revocations from OIB and content setting Revocations from OIB are recorded from chrome/browser/ui/website_settings/website_settings.cc Revocations from content setting are recorded from chrome/browser/ui/webui/options/content_settings_handler.cc. Revocations are only recorded from content setting if the pattern is a valid host string with no wildcards. We also refactored GetContentSettingsMap to GetProfile and GetOTRContentSettingsMap to GetOTRProfile since we need to pass in the profile to ScopedRevocationReporter. BUG=613883,469221 ========== to ========== Add revocations from OIB and content setting Revocations from OIB are recorded from chrome/browser/ui/website_settings/website_settings.cc Revocations from content setting are recorded from chrome/browser/ui/webui/options/content_settings_handler.cc. Revocations are only recorded from content setting if the pattern is a valid host string with no wildcards. We also refactored GetContentSettingsMap to GetProfile and GetOTRContentSettingsMap to GetOTRProfile since we need to pass in the profile to ScopedRevocationReporter. BUG=613883,469221 Committed: https://crrev.com/8b3490ccac018eee6b75c9f64d30700fb1d40d69 Cr-Commit-Position: refs/heads/master@{#408341} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/8b3490ccac018eee6b75c9f64d30700fb1d40d69 Cr-Commit-Position: refs/heads/master@{#408341} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
