Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2047253002:
Add hooks to permission layer for permission action reporting (Closed)
|
Created:
4 years, 6 months ago by stefanocs Modified:
4 years, 5 months ago CC:
chromium-reviews, mlamouri+watch-permissions_chromium.org, grt+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@permission-reporter-implementation Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd hooks to permission layer for permission action reporting
- permissions_reporter_ added to SafeBrowsingPingManager
- Permission user actions collected by PermissionUmaUtil
Reports are only sent to opted-in users (Safe Browsing AND Chrome Tab Sync AND Chrome Pref Sync)
Only origin, permission type, and permission action are collected.
Data collected are from PROMPT source UI only.
BUG=613883
Committed: https://crrev.com/41b6de326ed85f2672e8d38720c140dbafa0862e
Cr-Commit-Position: refs/heads/master@{#405051}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 6
Patch Set 4 : Add guard to permission reporting #
Total comments: 7
Patch Set 5 #Patch Set 6 : Move function #Patch Set 7 : Send report to opted-in users only #
Total comments: 3
Patch Set 8 : Add ReportPermissionAction to UI manager #Patch Set 9 : Small comment change #
Total comments: 16
Patch Set 10 : Add profile as parameter to report permission related functions #Patch Set 11 : Add TODO comments #Patch Set 12 : Remove a redundant include #
Total comments: 12
Patch Set 13 : Add incognito check #
Total comments: 1
Patch Set 14 : fix nit: remove bracket #
Total comments: 6
Patch Set 15 : Add IsOptedInPermissionActionReporting tests #Patch Set 16 : resolve nits #
Total comments: 42
Patch Set 17 : Refactor tests #
Total comments: 5
Patch Set 18 : Modify sync preference check test #
Total comments: 3
Patch Set 19 : Add comments on push messaging permission #Patch Set 20 : Remove comments #
Total comments: 9
Patch Set 21 : Resolve nits #
Total comments: 2
Patch Set 22 : Add more expects #
Total comments: 2
Patch Set 23 : Fix wrong comment #Patch Set 24 : Add todo add browsertest #Patch Set 25 : Add profile to queue controller #Patch Set 26 : Rebase #Patch Set 27 : Use PRIORITY_PREFERENCE instead of PREFERENCE sync #
Total comments: 16
Messages
Total messages: 76 (20 generated)
Description was changed from ========== Add hooks to permission layer for permission action reporting BUG=613883 ========== to ========== Add hooks to permission layer for permission action reporting Only origin, permission type, and permission action are collected. Data collected are from PROMPT source UI only. BUG=613883 ==========
stefanocs@google.com changed reviewers: + kcarattini@chromium.org, raymes@chromium.org
You're also adding the permissions_reporter_ to the ping manager, so that should be in your cl description as well. https://codereview.chromium.org/2047253002/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_uma_util.cc (right): https://codereview.chromium.org/2047253002/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_uma_util.cc:257: safe_browsing::PermissionReporter* permission_reporter = You need to guard this with the flag you added in your first cl. https://codereview.chromium.org/2047253002/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_uma_util.cc:294: ReportPermissionAction(requesting_origin, permission, GRANTED); Can RecordPermissionAction just call ReportPermissionAction instead? Or are there times when we wouldn't want it called?
Description was changed from ========== Add hooks to permission layer for permission action reporting Only origin, permission type, and permission action are collected. Data collected are from PROMPT source UI only. BUG=613883 ========== to ========== Add hooks to permission layer for permission action reporting - permissions_reporter_ added to SafeBrowsingPingManager - Permission user actions collected by PermissionUmaUtil Only origin, permission type, and permission action are collected. Data collected are from PROMPT source UI only. BUG=613883 ==========
https://codereview.chromium.org/2047253002/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_uma_util.cc (right): https://codereview.chromium.org/2047253002/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_uma_util.cc:257: safe_browsing::PermissionReporter* permission_reporter = On 2016/06/13 21:21:09, kcarattini wrote: > You need to guard this with the flag you added in your first cl. Is this the right way to do this? https://codereview.chromium.org/2047253002/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_uma_util.cc:294: ReportPermissionAction(requesting_origin, permission, GRANTED); On 2016/06/13 21:21:09, kcarattini wrote: > Can RecordPermissionAction just call ReportPermissionAction instead? Or are > there times when we wouldn't want it called? Yes, but that means we have to add more params to the function signature when we have added SourceUI and RequestTrigger. Do you think it is ok to pass these two params to RecordPermissionAction only so that it can passed to ReportPermissionAction?
https://codereview.chromium.org/2047253002/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_uma_util.cc (right): https://codereview.chromium.org/2047253002/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_uma_util.cc:294: ReportPermissionAction(requesting_origin, permission, GRANTED); I think that's ok :) https://codereview.chromium.org/2047253002/diff/60001/chrome/browser/permissi... File chrome/browser/permissions/permission_uma_util.cc (right): https://codereview.chromium.org/2047253002/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_uma_util.cc:268: } We should only send a report if the enabled flag is passed at present. Otherwise the default (if no flags are passed) will be to send the report. https://codereview.chromium.org/2047253002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/ping_manager.cc (right): https://codereview.chromium.org/2047253002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/ping_manager.cc:149: DCHECK(permission_reporter_); nit: this isn't needed DCHECKs for null often aren't too useful because in the case where the value is null, the program will crash in a meaningful way anyway. https://codereview.chromium.org/2047253002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/ping_manager.h (right): https://codereview.chromium.org/2047253002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/ping_manager.h:65: PermissionReporter* permission_reporter(); Do you think we should have a Send function here, rather than exposing the PermissionReporter? I don't feel strongly except that it would be more aligned with the other functions in this class. In that case the flag check could live in this class too.
https://codereview.chromium.org/2047253002/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_uma_util.cc (right): https://codereview.chromium.org/2047253002/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_uma_util.cc:294: ReportPermissionAction(requesting_origin, permission, GRANTED); On 2016/06/14 02:47:11, raymes wrote: > I think that's ok :) Done. https://codereview.chromium.org/2047253002/diff/60001/chrome/browser/permissi... File chrome/browser/permissions/permission_uma_util.cc (right): https://codereview.chromium.org/2047253002/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_uma_util.cc:268: } On 2016/06/14 02:47:11, raymes wrote: > We should only send a report if the enabled flag is passed at present. Otherwise > the default (if no flags are passed) will be to send the report. Done. https://codereview.chromium.org/2047253002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/ping_manager.cc (right): https://codereview.chromium.org/2047253002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/ping_manager.cc:149: DCHECK(permission_reporter_); On 2016/06/14 02:47:11, raymes wrote: > nit: this isn't needed > > DCHECKs for null often aren't too useful because in the case where the value is > null, the program will crash in a meaningful way anyway. Done. https://codereview.chromium.org/2047253002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/ping_manager.h (right): https://codereview.chromium.org/2047253002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/ping_manager.h:65: PermissionReporter* permission_reporter(); On 2016/06/14 02:47:11, raymes wrote: > Do you think we should have a Send function here, rather than exposing the > PermissionReporter? I don't feel strongly except that it would be more aligned > with the other functions in this class. In that case the flag check could live > in this class too. Should we remove the ReportPermissionAction function from PermissionUmaUtil and let the Send function be called by the RecordPermissionAction then?
https://codereview.chromium.org/2047253002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/ping_manager.h (right): https://codereview.chromium.org/2047253002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/ping_manager.h:65: PermissionReporter* permission_reporter(); On 2016/06/14 03:37:37, stefanocs wrote: > On 2016/06/14 02:47:11, raymes wrote: > > Do you think we should have a Send function here, rather than exposing the > > PermissionReporter? I don't feel strongly except that it would be more aligned > > with the other functions in this class. In that case the flag check could live > > in this class too. > > Should we remove the ReportPermissionAction function from PermissionUmaUtil and > let the Send function be called by the RecordPermissionAction then? That seems good since the function will be so small
On 2016/06/14 03:58:16, raymes wrote: > https://codereview.chromium.org/2047253002/diff/60001/chrome/browser/safe_bro... > File chrome/browser/safe_browsing/ping_manager.h (right): > > https://codereview.chromium.org/2047253002/diff/60001/chrome/browser/safe_bro... > chrome/browser/safe_browsing/ping_manager.h:65: PermissionReporter* > permission_reporter(); > On 2016/06/14 03:37:37, stefanocs wrote: > > On 2016/06/14 02:47:11, raymes wrote: > > > Do you think we should have a Send function here, rather than exposing the > > > PermissionReporter? I don't feel strongly except that it would be more > aligned > > > with the other functions in this class. In that case the flag check could > live > > > in this class too. > > > > Should we remove the ReportPermissionAction function from PermissionUmaUtil > and > > let the Send function be called by the RecordPermissionAction then? > > That seems good since the function will be so small lgtm besides the optional comment above. I think it might be slightly nicer to do it that way but we may want to ask Kendra's opinion.
Thanks Raymes. I will be waiting for Kendra's opinion on that. Kendra told me this morning to add restrictions for sending permission reports to this CL, so I have added that too. Please take a look to confirm that it was correct. Meanwhile, I'm having a difficulty in calling ping_manager() that I have described below. https://codereview.chromium.org/2047253002/diff/120001/chrome/browser/permiss... File chrome/browser/permissions/permission_uma_util.cc (right): https://codereview.chromium.org/2047253002/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:97: Profile* profile = ProfileManager::GetLastUsedProfile(); Is this the right way to get the profile? Or should the profile object be passed to this function? https://codereview.chromium.org/2047253002/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:128: ->ping_manager() Chrome crashes when ping_manager() is called due to this assertion (https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/safe_browsi...). The crash message is "Check failed: ::content::BrowserThread::CurrentlyOn(BrowserThread::IO). Must be called on Chrome_IOThread; actually called on CrBrowserMain." Is there any workaround for this?
Description was changed from ========== Add hooks to permission layer for permission action reporting - permissions_reporter_ added to SafeBrowsingPingManager - Permission user actions collected by PermissionUmaUtil Only origin, permission type, and permission action are collected. Data collected are from PROMPT source UI only. BUG=613883 ========== to ========== Add hooks to permission layer for permission action reporting - permissions_reporter_ added to SafeBrowsingPingManager - Permission user actions collected by PermissionUmaUtil Reports are only sent to opted-in users (Safe Browsing AND Chrome Tab Sync AND Chrome Pref Sync) Only origin, permission type, and permission action are collected. Data collected are from PROMPT source UI only. BUG=613883 ==========
I added a workaround to fix the crash issue. So the issue is the ping manager cannot be called using the main thread (UI). My workaround is by calling a function in UI manager instead and then it will call the function in ping manager using IO thread. The opted-in user check is added to the function in UI manager because it has to be check in the main thread otherwise crash. If this is ok, I will update my doc. https://codereview.chromium.org/2047253002/diff/120001/chrome/browser/permiss... File chrome/browser/permissions/permission_uma_util.cc (right): https://codereview.chromium.org/2047253002/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:128: ->ping_manager() On 2016/06/15 05:31:15, stefanocs wrote: > Chrome crashes when ping_manager() is called due to this assertion > (https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/safe_browsi...). > > The crash message is "Check failed: > ::content::BrowserThread::CurrentlyOn(BrowserThread::IO). Must be called on > Chrome_IOThread; actually called on CrBrowserMain." > Is there any workaround for this? Done.
Thanks! https://codereview.chromium.org/2047253002/diff/160001/chrome/browser/safe_br... File chrome/browser/safe_browsing/ping_manager.h (right): https://codereview.chromium.org/2047253002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/ping_manager.h:15: #include "base/command_line.h" nit: is this needed here? https://codereview.chromium.org/2047253002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/ping_manager.h:25: #include "components/prefs/pref_service.h" Same with most of these? https://codereview.chromium.org/2047253002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/ping_manager.h:75: content::PermissionType permission, nit: we should include permission_type.h https://codereview.chromium.org/2047253002/diff/160001/chrome/browser/safe_br... File chrome/browser/safe_browsing/ui_manager.cc (right): https://codereview.chromium.org/2047253002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/ui_manager.cc:297: Profile* profile = ProfileManager::GetLastUsedProfile(); We should use the actual profile (as discussed). https://codereview.chromium.org/2047253002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/ui_manager.cc:300: if (!profile->GetPrefs()->GetBoolean(prefs::kSafeBrowsingEnabled)) { nit: no {} for single-line if statements (and below) https://codereview.chromium.org/2047253002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/ui_manager.cc:300: if (!profile->GetPrefs()->GetBoolean(prefs::kSafeBrowsingEnabled)) { Let's check with nparker for this check. https://codereview.chromium.org/2047253002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/ui_manager.cc:314: if (!profile_sync_service->CanSyncStart()) { Let's check with pavely about how to find out: -Whether sync is enabled -Whether tab sync is enabled -Whether pref sync is enabled https://codereview.chromium.org/2047253002/diff/160001/chrome/browser/safe_br... File chrome/browser/safe_browsing/ui_manager.h (right): https://codereview.chromium.org/2047253002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/ui_manager.h:155: content::PermissionType permission, nit: we need to include permission_type.h too
https://codereview.chromium.org/2047253002/diff/160001/chrome/browser/safe_br... File chrome/browser/safe_browsing/ping_manager.h (right): https://codereview.chromium.org/2047253002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/ping_manager.h:15: #include "base/command_line.h" On 2016/06/16 01:10:22, raymes wrote: > nit: is this needed here? Done. https://codereview.chromium.org/2047253002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/ping_manager.h:25: #include "components/prefs/pref_service.h" On 2016/06/16 01:10:22, raymes wrote: > Same with most of these? Done. https://codereview.chromium.org/2047253002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/ping_manager.h:75: content::PermissionType permission, On 2016/06/16 01:10:22, raymes wrote: > nit: we should include permission_type.h Do we still need to include that even though it was already included in permission_uma_util.h? https://codereview.chromium.org/2047253002/diff/160001/chrome/browser/safe_br... File chrome/browser/safe_browsing/ui_manager.cc (right): https://codereview.chromium.org/2047253002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/ui_manager.cc:297: Profile* profile = ProfileManager::GetLastUsedProfile(); On 2016/06/16 01:10:23, raymes wrote: > We should use the actual profile (as discussed). Done. https://codereview.chromium.org/2047253002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/ui_manager.cc:300: if (!profile->GetPrefs()->GetBoolean(prefs::kSafeBrowsingEnabled)) { On 2016/06/16 01:10:22, raymes wrote: > Let's check with nparker for this check. Acknowledged. https://codereview.chromium.org/2047253002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/ui_manager.cc:300: if (!profile->GetPrefs()->GetBoolean(prefs::kSafeBrowsingEnabled)) { On 2016/06/16 01:10:23, raymes wrote: > nit: no {} for single-line if statements (and below) Done. https://codereview.chromium.org/2047253002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/ui_manager.cc:314: if (!profile_sync_service->CanSyncStart()) { On 2016/06/16 01:10:23, raymes wrote: > Let's check with pavely about how to find out: > -Whether sync is enabled > -Whether tab sync is enabled > -Whether pref sync is enabled Acknowledged.
stefanocs@google.com changed reviewers: + nparker@chromium.org, pavely@chromium.org
+nparker@chromium.org +pavely@chromium.org https://codereview.chromium.org/2047253002/diff/220001/chrome/browser/permiss... File chrome/browser/permissions/permission_uma_util.cc (right): https://codereview.chromium.org/2047253002/diff/220001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:95: if (!profile->GetPrefs()->GetBoolean(prefs::kSafeBrowsingEnabled)) Hey Nathan, can you check if this is the correct way to check whether the safe browsing is enabled? https://codereview.chromium.org/2047253002/diff/220001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:113: if (!preferred_data_types.Has(syncer::PREFERENCES)) Hey Pavel, can you check if this is the correct way to check if Chrome Tab Sync and Chrome Pref Sync are enabled?
lgtm https://codereview.chromium.org/2047253002/diff/220001/chrome/browser/permiss... File chrome/browser/permissions/permission_uma_util.cc (right): https://codereview.chromium.org/2047253002/diff/220001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:113: if (!preferred_data_types.Has(syncer::PREFERENCES)) On 2016/06/16 01:58:40, stefanocs wrote: > Hey Pavel, can you check if this is the correct way to check if Chrome Tab Sync > and Chrome Pref Sync are enabled? Yes, this is correct way.
How about some tests that verify your logic for _when_ to report works as intended? https://codereview.chromium.org/2047253002/diff/220001/chrome/browser/permiss... File chrome/browser/permissions/permission_uma_util.cc (right): https://codereview.chromium.org/2047253002/diff/220001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:85: bool isOptedInPermissionActionReporting(Profile* profile) { Capital Is https://codereview.chromium.org/2047253002/diff/220001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:95: if (!profile->GetPrefs()->GetBoolean(prefs::kSafeBrowsingEnabled)) On 2016/06/16 01:58:40, stefanocs wrote: > Hey Nathan, can you check if this is the correct way to check whether the safe > browsing is enabled? yup https://codereview.chromium.org/2047253002/diff/220001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:98: if (!ProfileSyncServiceFactory::HasProfileSyncService(profile)) You probably should check !incognito as well, ya? e.g. https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/safe_browsi... https://codereview.chromium.org/2047253002/diff/220001/chrome/browser/safe_br... File chrome/browser/safe_browsing/ui_manager.cc (right): https://codereview.chromium.org/2047253002/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/ui_manager.cc:288: DCHECK_CURRENTLY_ON(BrowserThread::UI); nit: I'm not clear on why this _has_ to be on the UI thread, but restricting it won't cause any problems.
Thanks. Nathan, do you mean tests for IsOptedInPermissionActionReporting? https://codereview.chromium.org/2047253002/diff/220001/chrome/browser/permiss... File chrome/browser/permissions/permission_uma_util.cc (right): https://codereview.chromium.org/2047253002/diff/220001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:85: bool isOptedInPermissionActionReporting(Profile* profile) { On 2016/06/16 21:08:41, Nathan Parker wrote: > Capital Is Done. https://codereview.chromium.org/2047253002/diff/220001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:95: if (!profile->GetPrefs()->GetBoolean(prefs::kSafeBrowsingEnabled)) On 2016/06/16 21:08:41, Nathan Parker wrote: > On 2016/06/16 01:58:40, stefanocs wrote: > > Hey Nathan, can you check if this is the correct way to check whether the safe > > browsing is enabled? > > yup Acknowledged. https://codereview.chromium.org/2047253002/diff/220001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:98: if (!ProfileSyncServiceFactory::HasProfileSyncService(profile)) On 2016/06/16 21:08:40, Nathan Parker wrote: > You probably should check !incognito as well, ya? e.g. > > https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/safe_browsi... Acknowledged. https://codereview.chromium.org/2047253002/diff/220001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:113: if (!preferred_data_types.Has(syncer::PREFERENCES)) On 2016/06/16 18:59:24, pavely wrote: > On 2016/06/16 01:58:40, stefanocs wrote: > > Hey Pavel, can you check if this is the correct way to check if Chrome Tab > Sync > > and Chrome Pref Sync are enabled? > > Yes, this is correct way. Acknowledged. https://codereview.chromium.org/2047253002/diff/220001/chrome/browser/safe_br... File chrome/browser/safe_browsing/ui_manager.cc (right): https://codereview.chromium.org/2047253002/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/ui_manager.cc:288: DCHECK_CURRENTLY_ON(BrowserThread::UI); On 2016/06/16 21:08:41, Nathan Parker wrote: > nit: I'm not clear on why this _has_ to be on the UI thread, but restricting it > won't cause any problems. I'm also not clear about this but the other two reporting functions in the UI manager also have this restriction. https://codereview.chromium.org/2047253002/diff/240001/chrome/browser/permiss... File chrome/browser/permissions/permission_uma_util.cc (right): https://codereview.chromium.org/2047253002/diff/240001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:95: if (profile->GetProfileType() == Profile::INCOGNITO_PROFILE) { Nathan, can this be used instead for the incognito check?
yes, tests for IsOptedInPermissionActionReporting
lg besides adding a test https://codereview.chromium.org/2047253002/diff/160001/chrome/browser/safe_br... File chrome/browser/safe_browsing/ping_manager.h (right): https://codereview.chromium.org/2047253002/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/ping_manager.h:75: content::PermissionType permission, Yes - you either need to forward declare or #include every symbol. https://codereview.chromium.org/2047253002/diff/260001/chrome/browser/permiss... File chrome/browser/permissions/permission_bubble_request_impl.cc (right): https://codereview.chromium.org/2047253002/diff/260001/chrome/browser/permiss... chrome/browser/permissions/permission_bubble_request_impl.cc:32: // TODO(stefanocs): Pass in the non null profile. nit: the->a https://codereview.chromium.org/2047253002/diff/260001/chrome/browser/permiss... File chrome/browser/permissions/permission_infobar_delegate.cc (right): https://codereview.chromium.org/2047253002/diff/260001/chrome/browser/permiss... chrome/browser/permissions/permission_infobar_delegate.cc:15: // TODO(stefanocs): Pass in the non null profile. nit: the->a https://codereview.chromium.org/2047253002/diff/260001/chrome/browser/permiss... File chrome/browser/permissions/permission_uma_util.cc (right): https://codereview.chromium.org/2047253002/diff/260001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:92: if (!profile) nit: perhaps add a TODO here to remove this check once we have updated all the callsites.
Please review tests for IsOptedInPermissionActionReporting. Thanks. https://codereview.chromium.org/2047253002/diff/260001/chrome/browser/permiss... File chrome/browser/permissions/permission_bubble_request_impl.cc (right): https://codereview.chromium.org/2047253002/diff/260001/chrome/browser/permiss... chrome/browser/permissions/permission_bubble_request_impl.cc:32: // TODO(stefanocs): Pass in the non null profile. On 2016/06/20 02:46:33, raymes wrote: > nit: the->a Done. https://codereview.chromium.org/2047253002/diff/260001/chrome/browser/permiss... File chrome/browser/permissions/permission_infobar_delegate.cc (right): https://codereview.chromium.org/2047253002/diff/260001/chrome/browser/permiss... chrome/browser/permissions/permission_infobar_delegate.cc:15: // TODO(stefanocs): Pass in the non null profile. On 2016/06/20 02:46:33, raymes wrote: > nit: the->a Done. https://codereview.chromium.org/2047253002/diff/260001/chrome/browser/permiss... File chrome/browser/permissions/permission_uma_util.cc (right): https://codereview.chromium.org/2047253002/diff/260001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:92: if (!profile) On 2016/06/20 02:46:33, raymes wrote: > nit: perhaps add a TODO here to remove this check once we have updated all the > callsites. Done.
https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... File chrome/browser/permissions/permission_uma_util.cc (right): https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:300: // flag is not enabled. This function is probably a bit over-commented. You can probably remove this comment - the code is self-explanatory. https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:304: // Do not report if profile is null. nit: remove this https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:305: // TODO(stefanocs): Remove this check once all callsites have been updated. been updated to not pass a nullptr. https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:308: // Do not report if profile is incognito. remove https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:311: // Do not report when SafeBrowsing is not enabled. remove https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:318: // Do not report if profile can't get a profile sync service due to enabled nit: remove "enabled" https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:323: // Do not report if profile can't start sync immediately. remove https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:330: // Do not report if Chrome Tab Sync is not enabled. remove https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:333: // Do not report if Chrome Pref Sync is not enabled. remove https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:343: // Send permission action report to opted-in users. nit: remove https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... File chrome/browser/permissions/permission_uma_util_unittest.cc (right): https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:30: PermissionUmaUtilTest() : profile_(new TestingProfile()) {} It would be better to separate each check (in the test below) into a separate test function (each set of {} that you have is probably a good granularity). That would allow clearer separation of state and of what is under test. (note: I put the comment here so that you would see it before the other comments and hopefully they make more sense) https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:37: return IsOptedInPermissionActionReporting(profile_.get()); nit: I would remove this and just always use the one above https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:43: } if you have a SetUp function, you could call this from there and reset the state for each test. You could also reset the profile for each test to ensure we start in a blank state. https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:53: void SetSafeBrowsing(bool value) { nit: value->enabled https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:65: switches::kDisablePermissionActionReporting); nit: I would just call these directly from where they're needed and remove this function https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:76: profile_sync_service()->OnUserChoseDatatypes(false, selectable_data_types); We should talk to pavely to ensure this is the right way to do this for the test. https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:79: Profile* incognito_profile() { return profile_->GetOffTheRecordProfile(); } nit: I would just inline this into the callsites https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:87: profile_sync_service()->backend_initialized_ = true; nit: just inline this since it's only called from one place https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:90: ProfileSyncService* profile_sync_service() { This should probably be GetProfileSyncService. We generally only use the lowercase-underscore notation for getters of member variables https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:158: EXPECT_TRUE(IsOptedInPermissionActionReporting()); nit: this is duplicated from above https://codereview.chromium.org/2047253002/diff/300001/components/browser_syn... File components/browser_sync/browser/profile_sync_service.h (right): https://codereview.chromium.org/2047253002/diff/300001/components/browser_syn... components/browser_sync/browser/profile_sync_service.h:636: friend class PermissionUmaUtilTest; nit: move this down to where the other "friend" declarations are
https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... File chrome/browser/permissions/permission_uma_util.cc (right): https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:300: // flag is not enabled. On 2016/06/22 02:11:13, raymes wrote: > This function is probably a bit over-commented. You can probably remove this > comment - the code is self-explanatory. Done. https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:304: // Do not report if profile is null. On 2016/06/22 02:11:12, raymes wrote: > nit: remove this Done. https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:305: // TODO(stefanocs): Remove this check once all callsites have been updated. On 2016/06/22 02:11:12, raymes wrote: > been updated to not pass a nullptr. Done. https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:308: // Do not report if profile is incognito. On 2016/06/22 02:11:13, raymes wrote: > remove Done. https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:311: // Do not report when SafeBrowsing is not enabled. On 2016/06/22 02:11:12, raymes wrote: > remove Done. https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:318: // Do not report if profile can't get a profile sync service due to enabled On 2016/06/22 02:11:12, raymes wrote: > nit: remove "enabled" Done. https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:323: // Do not report if profile can't start sync immediately. On 2016/06/22 02:11:13, raymes wrote: > remove Done. https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:330: // Do not report if Chrome Tab Sync is not enabled. On 2016/06/22 02:11:13, raymes wrote: > remove Done. https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:333: // Do not report if Chrome Pref Sync is not enabled. On 2016/06/22 02:11:12, raymes wrote: > remove Done. https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:343: // Send permission action report to opted-in users. On 2016/06/22 02:11:13, raymes wrote: > nit: remove Done. https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... File chrome/browser/permissions/permission_uma_util_unittest.cc (right): https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:30: PermissionUmaUtilTest() : profile_(new TestingProfile()) {} On 2016/06/22 02:11:13, raymes wrote: > It would be better to separate each check (in the test below) into a separate > test function (each set of {} that you have is probably a good granularity). > That would allow clearer separation of state and of what is under test. > > (note: I put the comment here so that you would see it before the other comments > and hopefully they make more sense) Done. https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:37: return IsOptedInPermissionActionReporting(profile_.get()); On 2016/06/22 02:11:13, raymes wrote: > nit: I would remove this and just always use the one above Done. https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:43: } On 2016/06/22 02:11:13, raymes wrote: > if you have a SetUp function, you could call this from there and reset the state > for each test. You could also reset the profile for each test to ensure we start > in a blank state. Done. https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:53: void SetSafeBrowsing(bool value) { On 2016/06/22 02:11:13, raymes wrote: > nit: value->enabled Done. https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:65: switches::kDisablePermissionActionReporting); On 2016/06/22 02:11:13, raymes wrote: > nit: I would just call these directly from where they're needed and remove this > function Done. https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:76: profile_sync_service()->OnUserChoseDatatypes(false, selectable_data_types); On 2016/06/22 02:11:13, raymes wrote: > We should talk to pavely to ensure this is the right way to do this for the > test. Acknowledged. https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:79: Profile* incognito_profile() { return profile_->GetOffTheRecordProfile(); } On 2016/06/22 02:11:13, raymes wrote: > nit: I would just inline this into the callsites Done. https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:87: profile_sync_service()->backend_initialized_ = true; On 2016/06/22 02:11:13, raymes wrote: > nit: just inline this since it's only called from one place Done. https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:90: ProfileSyncService* profile_sync_service() { On 2016/06/22 02:11:13, raymes wrote: > This should probably be GetProfileSyncService. We generally only use the > lowercase-underscore notation for getters of member variables Done. https://codereview.chromium.org/2047253002/diff/300001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:158: EXPECT_TRUE(IsOptedInPermissionActionReporting()); On 2016/06/22 02:11:13, raymes wrote: > nit: this is duplicated from above Done. https://codereview.chromium.org/2047253002/diff/300001/components/browser_syn... File components/browser_sync/browser/profile_sync_service.h (right): https://codereview.chromium.org/2047253002/diff/300001/components/browser_syn... components/browser_sync/browser/profile_sync_service.h:636: friend class PermissionUmaUtilTest; On 2016/06/22 02:11:13, raymes wrote: > nit: move this down to where the other "friend" declarations are Done.
Hey Pavel, can you check the test for sync preference below? Thanks https://codereview.chromium.org/2047253002/diff/320001/chrome/browser/permiss... File chrome/browser/permissions/permission_uma_util_unittest.cc (right): https://codereview.chromium.org/2047253002/diff/320001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:111: IsOptedInPermissionActionReportingSyncPreferenceCheck) { Is the right way to do the sync preference test?
lgtm https://codereview.chromium.org/2047253002/diff/320001/chrome/browser/permiss... File chrome/browser/permissions/permission_uma_util_unittest.cc (right): https://codereview.chromium.org/2047253002/diff/320001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:69: TEST_F(PermissionUmaUtilTest, IsOptedInPermissionActionReporting) { IsOptedInPermissionActionReportingSignIn ? https://codereview.chromium.org/2047253002/diff/320001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:117: syncer::ModelTypeSet selectable_data_types = syncer::UserSelectableTypes(); Maybe we should start with an empty set of sync types, to ensure that just prefs+tabs is enough?
https://codereview.chromium.org/2047253002/diff/320001/chrome/browser/permiss... File chrome/browser/permissions/permission_uma_util_unittest.cc (right): https://codereview.chromium.org/2047253002/diff/320001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:69: TEST_F(PermissionUmaUtilTest, IsOptedInPermissionActionReporting) { On 2016/06/23 00:38:55, raymes wrote: > IsOptedInPermissionActionReportingSignIn ? Done. https://codereview.chromium.org/2047253002/diff/320001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:117: syncer::ModelTypeSet selectable_data_types = syncer::UserSelectableTypes(); On 2016/06/23 00:38:55, raymes wrote: > Maybe we should start with an empty set of sync types, to ensure that just > prefs+tabs is enough? Done.
lgtm
https://codereview.chromium.org/2047253002/diff/340001/chrome/browser/push_me... File chrome/browser/push_messaging/push_messaging_permission_context.cc (right): https://codereview.chromium.org/2047253002/diff/340001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_permission_context.cc:120: PermissionUmaUtil::PermissionDenied(permission_type(), requesting_origin, As we discussed, can you double check and then add a comment to the code explalining how reporting works for the complicated push messaging / notifications interaction?
https://codereview.chromium.org/2047253002/diff/340001/chrome/browser/push_me... File chrome/browser/push_messaging/push_messaging_permission_context.cc (right): https://codereview.chromium.org/2047253002/diff/340001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_permission_context.cc:120: PermissionUmaUtil::PermissionDenied(permission_type(), requesting_origin, Stefano and I are looking into this. It's a bit hard to distill the complexity down into a meaningful comment here so I'm thinking we should land this CL and file a bug for that issue?
https://codereview.chromium.org/2047253002/diff/340001/chrome/browser/push_me... File chrome/browser/push_messaging/push_messaging_permission_context.cc (right): https://codereview.chromium.org/2047253002/diff/340001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_permission_context.cc:120: PermissionUmaUtil::PermissionDenied(permission_type(), requesting_origin, On 2016/06/27 04:36:17, raymes wrote: > Stefano and I are looking into this. It's a bit hard to distill the complexity > down into a meaningful comment here so I'm thinking we should land this CL and > file a bug for that issue? Ok, has there been a bug filed for this? I didn't see one (sorry if I missed it!).
Nice job! Sorry for blocking this, just a few minor nits. I see you've written a doc about the PushMessaging complexities, so I'll review that separately. Thanks, Kendra https://codereview.chromium.org/2047253002/diff/380001/chrome/browser/permiss... File chrome/browser/permissions/permission_uma_util.cc (right): https://codereview.chromium.org/2047253002/diff/380001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:298: bool PermissionUmaUtil::IsOptedInPermissionActionReporting(Profile* profile) { Nice work on this method! https://codereview.chromium.org/2047253002/diff/380001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:324: nit: remove blank line https://codereview.chromium.org/2047253002/diff/380001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:329: return true; nit: add blank line above https://codereview.chromium.org/2047253002/diff/380001/chrome/browser/permiss... File chrome/browser/permissions/permission_uma_util.h (right): https://codereview.chromium.org/2047253002/diff/380001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.h:98: static bool IsOptedInPermissionActionReporting(Profile* profile); nit:IsOptedInPermissionActionReporting -> IsOptedIntoPermissionActionReporting https://codereview.chromium.org/2047253002/diff/380001/chrome/browser/permiss... File chrome/browser/permissions/permission_uma_util_unittest.cc (right): https://codereview.chromium.org/2047253002/diff/380001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:69: TEST_F(PermissionUmaUtilTest, IsOptedInPermissionActionReportingSignInCheck) { Can you add comments before each test fixture saying what each is testing specifically?
Thanks Kendra! https://codereview.chromium.org/2047253002/diff/380001/chrome/browser/permiss... File chrome/browser/permissions/permission_uma_util.cc (right): https://codereview.chromium.org/2047253002/diff/380001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:324: On 2016/07/06 01:44:33, kcarattini wrote: > nit: remove blank line Done. https://codereview.chromium.org/2047253002/diff/380001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.cc:329: return true; On 2016/07/06 01:44:33, kcarattini wrote: > nit: add blank line above Done. https://codereview.chromium.org/2047253002/diff/380001/chrome/browser/permiss... File chrome/browser/permissions/permission_uma_util.h (right): https://codereview.chromium.org/2047253002/diff/380001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.h:98: static bool IsOptedInPermissionActionReporting(Profile* profile); On 2016/07/06 01:44:34, kcarattini wrote: > nit:IsOptedInPermissionActionReporting -> IsOptedIntoPermissionActionReporting Done. https://codereview.chromium.org/2047253002/diff/380001/chrome/browser/permiss... File chrome/browser/permissions/permission_uma_util_unittest.cc (right): https://codereview.chromium.org/2047253002/diff/380001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:69: TEST_F(PermissionUmaUtilTest, IsOptedInPermissionActionReportingSignInCheck) { On 2016/07/06 01:44:34, kcarattini wrote: > Can you add comments before each test fixture saying what each is testing > specifically? Done.
https://codereview.chromium.org/2047253002/diff/400001/chrome/browser/permiss... File chrome/browser/permissions/permission_uma_util_unittest.cc (right): https://codereview.chromium.org/2047253002/diff/400001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:72: TEST_F(PermissionUmaUtilTest, IsOptedIntoPermissionActionReportingSignInCheck) { In all of these tests, you should first check that IsOptedInto... returns true before turning off and testing the feature that you expect to disable it.
https://codereview.chromium.org/2047253002/diff/400001/chrome/browser/permiss... File chrome/browser/permissions/permission_uma_util_unittest.cc (right): https://codereview.chromium.org/2047253002/diff/400001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:72: TEST_F(PermissionUmaUtilTest, IsOptedIntoPermissionActionReportingSignInCheck) { On 2016/07/06 07:27:05, kcarattini wrote: > In all of these tests, you should first check that IsOptedInto... returns true > before turning off and testing the feature that you expect to disable it. Done.
lgtm https://codereview.chromium.org/2047253002/diff/420001/chrome/browser/permiss... File chrome/browser/permissions/permission_uma_util_unittest.cc (right): https://codereview.chromium.org/2047253002/diff/420001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:120: // false if Safe Browsing is disabled. I think you mean Sync here.
https://codereview.chromium.org/2047253002/diff/420001/chrome/browser/permiss... File chrome/browser/permissions/permission_uma_util_unittest.cc (right): https://codereview.chromium.org/2047253002/diff/420001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:120: // false if Safe Browsing is disabled. On 2016/07/07 01:54:50, kcarattini wrote: > I think you mean Sync here. Done.
stefanocs@google.com changed reviewers: + johnme@chromium.org, sergeyu@chromium.org
sergeyu@chromium.org: Please review changes in chrome/browser/media/media_stream_devices_controller.cc johnme@chromium.org: Please review changes in chrome/browser/push_messaging/push_messaging_permission_context.cc Thanks!
lgtm
lgtm for safe_browsing
push_messaging/ lgtm
The CQ bit was checked by stefanocs@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from pavely@chromium.org, raymes@chromium.org, kcarattini@chromium.org, sergeyu@chromium.org, nparker@chromium.org Link to the patchset: https://codereview.chromium.org/2047253002/#ps460001 (title: "Add todo add browsertest")
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
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...) linux_chromium_chromeos_compile_dbg_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
The patchset sent to the CQ was uploaded after l-g-t-m from johnme@chromium.org, pavely@chromium.org, sergeyu@chromium.org, raymes@chromium.org, nparker@chromium.org, kcarattini@chromium.org Link to the patchset: https://codereview.chromium.org/2047253002/#ps480001 (title: "Add profile to queue controller")
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
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by stefanocs@google.com
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
Try jobs failed on following builders: linux_chromium_chromeos_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
The patchset sent to the CQ was uploaded after l-g-t-m from johnme@chromium.org, pavely@chromium.org, sergeyu@chromium.org, raymes@chromium.org, nparker@chromium.org, kcarattini@chromium.org Link to the patchset: https://codereview.chromium.org/2047253002/#ps520001 (title: "Use PRIORITY_PREFERENCE instead of PREFERENCE sync")
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 hooks to permission layer for permission action reporting - permissions_reporter_ added to SafeBrowsingPingManager - Permission user actions collected by PermissionUmaUtil Reports are only sent to opted-in users (Safe Browsing AND Chrome Tab Sync AND Chrome Pref Sync) Only origin, permission type, and permission action are collected. Data collected are from PROMPT source UI only. BUG=613883 ========== to ========== Add hooks to permission layer for permission action reporting - permissions_reporter_ added to SafeBrowsingPingManager - Permission user actions collected by PermissionUmaUtil Reports are only sent to opted-in users (Safe Browsing AND Chrome Tab Sync AND Chrome Pref Sync) Only origin, permission type, and permission action are collected. Data collected are from PROMPT source UI only. BUG=613883 ==========
Message was sent while issue was closed.
Committed patchset #27 (id:520001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Add hooks to permission layer for permission action reporting - permissions_reporter_ added to SafeBrowsingPingManager - Permission user actions collected by PermissionUmaUtil Reports are only sent to opted-in users (Safe Browsing AND Chrome Tab Sync AND Chrome Pref Sync) Only origin, permission type, and permission action are collected. Data collected are from PROMPT source UI only. BUG=613883 ========== to ========== Add hooks to permission layer for permission action reporting - permissions_reporter_ added to SafeBrowsingPingManager - Permission user actions collected by PermissionUmaUtil Reports are only sent to opted-in users (Safe Browsing AND Chrome Tab Sync AND Chrome Pref Sync) Only origin, permission type, and permission action are collected. Data collected are from PROMPT source UI only. BUG=613883 Committed: https://crrev.com/41b6de326ed85f2672e8d38720c140dbafa0862e Cr-Commit-Position: refs/heads/master@{#405051} ==========
Message was sent while issue was closed.
Patchset 27 (id:??) landed as https://crrev.com/41b6de326ed85f2672e8d38720c140dbafa0862e Cr-Commit-Position: refs/heads/master@{#405051}
Message was sent while issue was closed.
thestig@chromium.org changed reviewers: + thestig@chromium.org
Message was sent while issue was closed.
Sorry folks, but I have to revert. The memory.fyi free has been redder than normal all day. Detailed explanation/rant below. https://codereview.chromium.org/2047253002/diff/520001/chrome/browser/permiss... File chrome/browser/permissions/permission_uma_util_unittest.cc (right): https://codereview.chromium.org/2047253002/diff/520001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:53: base::CommandLine::Reset(); Sorry, but we cannot do this in a unit test. There are 6 places in the entire code base that calls Reset(), and the other 5 are all main() like functions. Especially when running with --single-process-tests, the unit test process is a single process shared by all the test cases. Doing this changes the global CommandLine and screws up any other code that's holding on to the about-to-be-destroyed CommandLine via a pointer. Any test cases that run after this gets called may end up calling code holding on to the old CommandLine, resulting in a use after free. See https://crbug.com/628061 for an example of this, or try running: unit_tests --gtest_filter=M*.*:P*.* --gtest_shuffle --test-launcher-bot-mode --single-process-tests --test-tiny-timeout=1000 a few times. Eventually --gtest_shuffle rolls a 12 and kaboom. Unit tests that mess with globals are a pain to track down because it is not obvious who set us up with the bomb. Fortunately I have seen this before, and it *only* took 2 hours of running test combinations to figure this out.
Message was sent while issue was closed.
A revert of this CL (patchset #27 id:520001) has been created in https://codereview.chromium.org/2145373002/ by thestig@chromium.org. The reason for reverting is: Modifying the global command line breaks other tests..
Message was sent while issue was closed.
https://codereview.chromium.org/2047253002/diff/520001/chrome/browser/permiss... File chrome/browser/permissions/permission_uma_util.h (right): https://codereview.chromium.org/2047253002/diff/520001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util.h:96: DISALLOW_IMPLICIT_CONSTRUCTORS(PermissionUmaUtil); DISALLOW macros should always be last. https://codereview.chromium.org/2047253002/diff/520001/chrome/browser/permiss... File chrome/browser/permissions/permission_uma_util_unittest.cc (right): https://codereview.chromium.org/2047253002/diff/520001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:23: const char* kTestingGaiaId = "gaia_id"; This should be: const char kFoo[] = "foo"; Because one can do: const char* kBar = "bar"; kBar = nullptr; and that's not very const. https://codereview.chromium.org/2047253002/diff/520001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:29: PermissionUmaUtilTest() : profile_(new TestingProfile()) {} I think you are creating |profile_| twice per test - here and in SetUp(). https://codereview.chromium.org/2047253002/diff/520001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:31: static bool IsOptedIntoPermissionActionReporting(Profile* profile) { Make this a member function, since you always call it for profile() ? https://codereview.chromium.org/2047253002/diff/520001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:42: static_cast<FakeSigninManagerForTesting*>( How did you get a FakeSigninManagerForTesting? Who created it? https://codereview.chromium.org/2047253002/diff/520001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:53: base::CommandLine::Reset(); You probably want base::test::ScopedCommandLine instead.
Message was sent while issue was closed.
On 2016/07/14 09:14:43, Lei Zhang (Slow Thursday) wrote: > Sorry, but we cannot do this in a unit test. There are 6 places in the entire > code base that calls Reset(), and the other 5 are all main() like functions. Rather, cs.chromium.org's xref only sees 6, but git grep found more in other unit tests. :\
Message was sent while issue was closed.
https://codereview.chromium.org/2047253002/diff/520001/chrome/browser/permiss... File chrome/browser/permissions/permission_uma_util_unittest.cc (right): https://codereview.chromium.org/2047253002/diff/520001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:23: const char* kTestingGaiaId = "gaia_id"; On 2016/07/14 09:34:59, Lei Zhang (Slow Thursday) wrote: > This should be: > > const char kFoo[] = "foo"; > > Because one can do: > > const char* kBar = "bar"; > kBar = nullptr; > > and that's not very const. even better: constexpr char kFoo[] = "foo";
Message was sent while issue was closed.
Description was changed from ========== Add hooks to permission layer for permission action reporting - permissions_reporter_ added to SafeBrowsingPingManager - Permission user actions collected by PermissionUmaUtil Reports are only sent to opted-in users (Safe Browsing AND Chrome Tab Sync AND Chrome Pref Sync) Only origin, permission type, and permission action are collected. Data collected are from PROMPT source UI only. BUG=613883 Committed: https://crrev.com/41b6de326ed85f2672e8d38720c140dbafa0862e Cr-Commit-Position: refs/heads/master@{#405051} ========== to ========== Add hooks to permission layer for permission action reporting - permissions_reporter_ added to SafeBrowsingPingManager - Permission user actions collected by PermissionUmaUtil Reports are only sent to opted-in users (Safe Browsing AND Chrome Tab Sync AND Chrome Pref Sync) Only origin, permission type, and permission action are collected. Data collected are from PROMPT source UI only. BUG=613883 Committed: https://crrev.com/41b6de326ed85f2672e8d38720c140dbafa0862e Cr-Commit-Position: refs/heads/master@{#405051} ==========
Message was sent while issue was closed.
Hey Lei Zhang, Sorry for the trouble caused, I have added a another cl 2150903002 to reland this cl and resolve this issue, could you please review that to make sure this doesn't happen again. Thanks https://codereview.chromium.org/2047253002/diff/520001/chrome/browser/permiss... File chrome/browser/permissions/permission_uma_util_unittest.cc (right): https://codereview.chromium.org/2047253002/diff/520001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:23: const char* kTestingGaiaId = "gaia_id"; On 2016/07/14 10:08:09, grt (UTC plus 2) wrote: > On 2016/07/14 09:34:59, Lei Zhang (Slow Thursday) wrote: > > This should be: > > > > const char kFoo[] = "foo"; > > > > Because one can do: > > > > const char* kBar = "bar"; > > kBar = nullptr; > > > > and that's not very const. > > even better: > > constexpr char kFoo[] = "foo"; Done. Sorry I missed this, copied this from https://cs.chromium.org/chromium/src/chrome/browser/ui/sync/one_click_signin_.... https://codereview.chromium.org/2047253002/diff/520001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:29: PermissionUmaUtilTest() : profile_(new TestingProfile()) {} On 2016/07/14 09:34:58, Lei Zhang (Slow Thursday) wrote: > I think you are creating |profile_| twice per test - here and in SetUp(). Done. https://codereview.chromium.org/2047253002/diff/520001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:31: static bool IsOptedIntoPermissionActionReporting(Profile* profile) { On 2016/07/14 09:34:58, Lei Zhang (Slow Thursday) wrote: > Make this a member function, since you always call it for profile() ? It is called for profile()->GetOffTheRecordProfile() once below. https://codereview.chromium.org/2047253002/diff/520001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:42: static_cast<FakeSigninManagerForTesting*>( On 2016/07/14 09:34:58, Lei Zhang (Slow Thursday) wrote: > How did you get a FakeSigninManagerForTesting? Who created it? It was just casted like in other unittest https://cs.chromium.org/chromium/src/chrome/browser/signin/signin_tracker_uni... https://codereview.chromium.org/2047253002/diff/520001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:53: base::CommandLine::Reset(); On 2016/07/14 09:14:42, Lei Zhang (Slow Thursday) wrote: > Sorry, but we cannot do this in a unit test. There are 6 places in the entire > code base that calls Reset(), and the other 5 are all main() like functions. > > Especially when running with --single-process-tests, the unit test process is a > single process shared by all the test cases. Doing this changes the global > CommandLine and screws up any other code that's holding on to the > about-to-be-destroyed CommandLine via a pointer. Any test cases that run after > this gets called may end up calling code holding on to the old CommandLine, > resulting in a use after free. See https://crbug.com/628061 for an example of > this, or try running: > > unit_tests --gtest_filter=M*.*:P*.* --gtest_shuffle --test-launcher-bot-mode > --single-process-tests --test-tiny-timeout=1000 > > a few times. Eventually --gtest_shuffle rolls a 12 and kaboom. Unit tests that > mess with globals are a pain to track down because it is not obvious who set us > up with the bomb. Fortunately I have seen this before, and it *only* took 2 > hours of running test combinations to figure this out. Acknowledged.
Message was sent while issue was closed.
Will look at the new CL. https://codereview.chromium.org/2047253002/diff/520001/chrome/browser/permiss... File chrome/browser/permissions/permission_uma_util_unittest.cc (right): https://codereview.chromium.org/2047253002/diff/520001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:42: static_cast<FakeSigninManagerForTesting*>( On 2016/07/14 13:29:24, stefanocs wrote: > On 2016/07/14 09:34:58, Lei Zhang (Slow Thursday) wrote: > > How did you get a FakeSigninManagerForTesting? Who created it? > > It was just casted like in other unittest > https://cs.chromium.org/chromium/src/chrome/browser/signin/signin_tracker_uni... Yes, but the SigninTrackerTest first built one ~10 lines above.
Message was sent while issue was closed.
https://codereview.chromium.org/2047253002/diff/520001/chrome/browser/permiss... File chrome/browser/permissions/permission_uma_util_unittest.cc (right): https://codereview.chromium.org/2047253002/diff/520001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:53: base::CommandLine::Reset(); On 2016/07/14 09:14:42, Lei Zhang wrote: > Sorry, but we cannot do this in a unit test. There are 6 places in the entire > code base that calls Reset(), and the other 5 are all main() like functions. > > Especially when running with --single-process-tests, the unit test process is a > single process shared by all the test cases. Doing this changes the global > CommandLine and screws up any other code that's holding on to the > about-to-be-destroyed CommandLine via a pointer. Any test cases that run after > this gets called may end up calling code holding on to the old CommandLine, > resulting in a use after free. See https://crbug.com/628061 for an example of > this, or try running: > > unit_tests --gtest_filter=M*.*:P*.* --gtest_shuffle --test-launcher-bot-mode > --single-process-tests --test-tiny-timeout=1000 > > a few times. Eventually --gtest_shuffle rolls a 12 and kaboom. Unit tests that > mess with globals are a pain to track down because it is not obvious who set us > up with the bomb. Fortunately I have seen this before, and it *only* took 2 > hours of running test combinations to figure this out. :( Really sorry for the hassle. I didn't think about this carefully and thought it would be isolated for the test (which is a silly assumption in retrospect). Should we make this scarier looking (e.g. rename it)?
Message was sent while issue was closed.
https://codereview.chromium.org/2047253002/diff/520001/chrome/browser/permiss... File chrome/browser/permissions/permission_uma_util_unittest.cc (right): https://codereview.chromium.org/2047253002/diff/520001/chrome/browser/permiss... chrome/browser/permissions/permission_uma_util_unittest.cc:53: base::CommandLine::Reset(); On 2016/07/15 02:20:40, raymes wrote: > On 2016/07/14 09:14:42, Lei Zhang wrote: > > Sorry, but we cannot do this in a unit test. There are 6 places in the entire > > code base that calls Reset(), and the other 5 are all main() like functions. > > > > Especially when running with --single-process-tests, the unit test process is > a > > single process shared by all the test cases. Doing this changes the global > > CommandLine and screws up any other code that's holding on to the > > about-to-be-destroyed CommandLine via a pointer. Any test cases that run after > > this gets called may end up calling code holding on to the old CommandLine, > > resulting in a use after free. See https://crbug.com/628061 for an example of > > this, or try running: > > > > unit_tests --gtest_filter=M*.*:P*.* --gtest_shuffle --test-launcher-bot-mode > > --single-process-tests --test-tiny-timeout=1000 > > > > a few times. Eventually --gtest_shuffle rolls a 12 and kaboom. Unit tests that > > mess with globals are a pain to track down because it is not obvious who set > us > > up with the bomb. Fortunately I have seen this before, and it *only* took 2 > > hours of running test combinations to figure this out. > > :( Really sorry for the hassle. I didn't think about this carefully and thought > it would be isolated for the test (which is a silly assumption in retrospect). > > Should we make this scarier looking (e.g. rename it)? I have a CL to add a comment to encourage the use of ScopedCommandLine. I think the general issue is tests can be sloppier about cleanup because most bots now run tests with the parallel test runner, where these types of issues are masked. The bots that run tests serially get hosed. |
