Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1401073002:
Add Rappor reporting for grant/deny/cancel/ignore of Mediastream permissions (Closed)
|
Created:
5 years, 2 months ago by tsergeant Modified:
5 years, 1 month ago Reviewers:
tommi (sloooow) - chröme, Steven Holte, mlamouri (slow - plz ping), raymes, kcarattini, Ilya Sherman CC:
chrome-apps-syd-reviews_chromium.org, chromium-reviews, feature-media-reviews_chromium.org, mcasas+watch_chromium.org, mlamouri+watch-permissions_chromium.org, posciak+watch_chromium.org, raymes Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor UMA and add Rappor metrics for grant/deny/cancel of Media permissions
Depends on https://codereview.chromium.org/1388343003
BUG=477833
Committed: https://crrev.com/d91d3942758065ca7b0a609ecc83357598bc86b7
Cr-Commit-Position: refs/heads/master@{#358784}
Patch Set 1 #Patch Set 2 : #
Total comments: 6
Patch Set 3 : Use existing rappor code #
Total comments: 7
Patch Set 4 : #Patch Set 5 : Call PermissionContextUmaUtil methods #
Total comments: 11
Patch Set 6 : Rename to *Capture #
Total comments: 7
Patch Set 7 : Change histogram suffix, use base::Bind #Patch Set 8 : Rebase #
Total comments: 11
Patch Set 9 : Fix nits #Patch Set 10 : Remove extra histogram suffixes #
Total comments: 5
Patch Set 11 : Fix nit #Patch Set 12 : Fix rebase #Patch Set 13 : Replace unusual case with todo #
Messages
Total messages: 44 (10 generated)
Patchset #2 (id:20001) has been deleted
tsergeant@chromium.org changed reviewers: + kcarattini@chromium.org
PTAL! https://codereview.chromium.org/1401073002/diff/40001/chrome/browser/media/me... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/1401073002/diff/40001/chrome/browser/media/me... chrome/browser/media/media_stream_devices_controller.cc:76: void RecordPermissionActionImpl(const GURL& request_origin, It's possible for the request to be a from a chrome-extension:// or chrome:// origin. Should these be filtered out?
Thanks for doing this, Tim! https://codereview.chromium.org/1401073002/diff/40001/chrome/browser/media/me... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/1401073002/diff/40001/chrome/browser/media/me... chrome/browser/media/media_stream_devices_controller.cc:76: void RecordPermissionActionImpl(const GURL& request_origin, On 2015/10/12 04:06:44, tsergeant wrote: > It's possible for the request to be a from a chrome-extension:// or chrome:// > origin. Should these be filtered out? Good question. To be honest, I'd be interested in seeing the numbers, so let's leave it for now. https://codereview.chromium.org/1401073002/diff/40001/chrome/browser/media/me... chrome/browser/media/media_stream_devices_controller.cc:100: RecordPermissionActionImpl(request.security_origin, action, "Camera"); I'm exporting a GetString method from permission_context_uma_util in another cl -- you should use that here too. https://codereview.chromium.org/1401073002/diff/40001/tools/metrics/rappor/ra... File tools/metrics/rappor/rappor.xml (right): https://codereview.chromium.org/1401073002/diff/40001/tools/metrics/rappor/ra... tools/metrics/rappor/rappor.xml:473: <owner>kcarattini@chromium.org</owner> Go ahead and put your username here (and below)
mlamouri@chromium.org changed reviewers: + mlamouri@chromium.org
Wouldn't that make sense to use PermissionContextUmaUtil directly?
Yup, I've refactored this so that media_stream_devices_controller calls the Rappor code from PermissionContextUmaUtil. PTAL! https://codereview.chromium.org/1401073002/diff/40001/chrome/browser/media/me... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/1401073002/diff/40001/chrome/browser/media/me... chrome/browser/media/media_stream_devices_controller.cc:100: RecordPermissionActionImpl(request.security_origin, action, "Camera"); On 2015/10/12 at 06:13:28, kcarattini wrote: > I'm exporting a GetString method from permission_context_uma_util in another cl -- you should use that here too. Done. https://codereview.chromium.org/1401073002/diff/40001/tools/metrics/rappor/ra... File tools/metrics/rappor/rappor.xml (right): https://codereview.chromium.org/1401073002/diff/40001/tools/metrics/rappor/ra... tools/metrics/rappor/rappor.xml:473: <owner>kcarattini@chromium.org</owner> On 2015/10/12 at 06:13:28, kcarattini wrote: > Go ahead and put your username here (and below) Done.
lgtm
https://codereview.chromium.org/1401073002/diff/60001/chrome/browser/media/me... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/1401073002/diff/60001/chrome/browser/media/me... chrome/browser/media/media_stream_devices_controller.cc:143: RecordPermissionAction(request_, PermissionAction::IGNORED); Couldn't you used PermissionContextUmaUtil::PermissionIgnored(); https://codereview.chromium.org/1401073002/diff/60001/chrome/browser/media/me... chrome/browser/media/media_stream_devices_controller.cc:232: RecordPermissionAction(request_, PermissionAction::GRANTED); ditto with Granted() https://codereview.chromium.org/1401073002/diff/60001/chrome/browser/media/me... chrome/browser/media/media_stream_devices_controller.cc:243: RecordPermissionAction(request_, PermissionAction::DENIED); ditto with Denied() https://codereview.chromium.org/1401073002/diff/60001/chrome/browser/media/me... chrome/browser/media/media_stream_devices_controller.cc:254: RecordPermissionAction(request_, PermissionAction::DISMISSED); ditto with Dismissed()
https://codereview.chromium.org/1401073002/diff/60001/chrome/browser/media/me... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/1401073002/diff/60001/chrome/browser/media/me... chrome/browser/media/media_stream_devices_controller.cc:143: RecordPermissionAction(request_, PermissionAction::IGNORED); On 2015/10/13 at 10:00:43, Mounir Lamouri wrote: > Couldn't you used PermissionContextUmaUtil::PermissionIgnored(); Calling RecordPermissionAction only records the multi-d metric. PermissionIgnored() and friends call a -different- RecordPermissionAction which records some extra things which we don't necessarily want for MediaStream: UMA metrics (a variant of which are already recorded for MediaStream) and deprecated single-dimension Rappor metrics. I can rename the RecordPermissionAction method on line 74 to make the difference between the two methods clearer, but I don't think it makes a lot of sense to do any further refactoring to PermissionContextUmaUtil at this stage.
tsergeant@chromium.org changed reviewers: + raymes@chromium.org
tsergeant@chromium.org changed reviewers: - raymes@chromium.org
https://codereview.chromium.org/1401073002/diff/60001/chrome/browser/media/me... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/1401073002/diff/60001/chrome/browser/media/me... chrome/browser/media/media_stream_devices_controller.cc:143: RecordPermissionAction(request_, PermissionAction::IGNORED); On 2015/10/13 at 23:41:20, tsergeant wrote: > On 2015/10/13 at 10:00:43, Mounir Lamouri wrote: > > Couldn't you used PermissionContextUmaUtil::PermissionIgnored(); > > Calling RecordPermissionAction only records the multi-d metric. > > PermissionIgnored() and friends call a -different- RecordPermissionAction which records some extra things which we don't necessarily want for MediaStream: UMA metrics (a variant of which are already recorded for MediaStream) and deprecated single-dimension Rappor metrics. > > I can rename the RecordPermissionAction method on line 74 to make the difference between the two methods clearer, but I don't think it makes a lot of sense to do any further refactoring to PermissionContextUmaUtil at this stage. Would it make sense to deprecate Media.DevicePermission* in favour of using the centralized system?
https://codereview.chromium.org/1401073002/diff/60001/chrome/browser/media/me... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/1401073002/diff/60001/chrome/browser/media/me... chrome/browser/media/media_stream_devices_controller.cc:143: RecordPermissionAction(request_, PermissionAction::IGNORED); On 2015/10/15 12:49:51, Mounir Lamouri wrote: > On 2015/10/13 at 23:41:20, tsergeant wrote: > > On 2015/10/13 at 10:00:43, Mounir Lamouri wrote: > > > Couldn't you used PermissionContextUmaUtil::PermissionIgnored(); > > > > Calling RecordPermissionAction only records the multi-d metric. > > > > PermissionIgnored() and friends call a -different- RecordPermissionAction > which records some extra things which we don't necessarily want for MediaStream: > UMA metrics (a variant of which are already recorded for MediaStream) and > deprecated single-dimension Rappor metrics. > > > > I can rename the RecordPermissionAction method on line 74 to make the > difference between the two methods clearer, but I don't think it makes a lot of > sense to do any further refactoring to PermissionContextUmaUtil at this stage. > > Would it make sense to deprecate Media.DevicePermission* in favour of using the > centralized system? By centralized system do you mean use permission_context_uma_util? I do think that makes sense but is probably out of scope for this cl. Are there any Media.DevicePermissions RAPPOR metrics?
On further thought, I decided it wasn't too much additional work to unify this entirely in the permissions_context_uma_util code. There's still a little nastiness from needing to call them once for each permission, but it's not too bad overall. PTAL!
https://codereview.chromium.org/1401073002/diff/100001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_uma_util.cc (right): https://codereview.chromium.org/1401073002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_context_uma_util.cc:74: if (permission == CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA || Could you explain? https://codereview.chromium.org/1401073002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_context_uma_util.cc:113: "ContentSettings.PermissionActionsInsecureOrigin_MediaStreamCamera", Could you name these Permissions.Actions.VideoCapture? I think we should move these ContentSettings.* to Permissions.*. Maybe we could start with this one. Would you be interested to do the follow-up? https://codereview.chromium.org/1401073002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_context_uma_util.cc:121: "ContentSettings.PermissionActionsInsecureOrigin_MediaStreamMic", Permissions.Actions.AudioCapture? https://codereview.chromium.org/1401073002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_context_uma_util.cc:351: return "MediaStreamMic"; Please, rename to AudioCapture. There is a CL in progress to use PermissionType in this method. https://codereview.chromium.org/1401073002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_context_uma_util.cc:353: return "MediaStreamCamera"; ditto with VideoCapture. https://codereview.chromium.org/1401073002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1401073002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:78688: + <suffix name="MediaStreamMic" label="Microphone permission actions"/> nit: could you name these AudioCapture and VideoCapture?
https://codereview.chromium.org/1401073002/diff/100001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_uma_util.cc (right): https://codereview.chromium.org/1401073002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_context_uma_util.cc:74: if (permission == CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA || On 2015/10/19 at 10:42:19, Mounir Lamouri wrote: > Could you explain? Added a comment. https://codereview.chromium.org/1401073002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_context_uma_util.cc:113: "ContentSettings.PermissionActionsInsecureOrigin_MediaStreamCamera", On 2015/10/19 at 10:42:19, Mounir Lamouri wrote: > Could you name these Permissions.Actions.VideoCapture? I think we should move these ContentSettings.* to Permissions.*. Maybe we could start with this one. Would you be interested to do the follow-up? Sure, sounds good. I can follow-up next week. I've gone with "Permissions.Action_AudioCapture", aiming to match Rappor's "Permissions.Action.AudioCapture", except that I can't change the existing underscore separator. https://codereview.chromium.org/1401073002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1401073002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:78688: + <suffix name="MediaStreamMic" label="Microphone permission actions"/> On 2015/10/19 at 10:42:19, Mounir Lamouri wrote: > nit: could you name these AudioCapture and VideoCapture? Done. https://codereview.chromium.org/1401073002/diff/120001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_uma_util.cc (right): https://codereview.chromium.org/1401073002/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_context_uma_util.cc:348: return "VideoCapture"; Kendra, does it affect you if I change these values? I know these values were only added by you in https://codereview.chromium.org/1388343003.
lgtm with histograms suffix changed. Thanks! :) https://codereview.chromium.org/1401073002/diff/100001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_uma_util.cc (right): https://codereview.chromium.org/1401073002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_context_uma_util.cc:113: "ContentSettings.PermissionActionsInsecureOrigin_MediaStreamCamera", On 2015/10/20 at 06:38:43, tsergeant wrote: > On 2015/10/19 at 10:42:19, Mounir Lamouri wrote: > > Could you name these Permissions.Actions.VideoCapture? I think we should move these ContentSettings.* to Permissions.*. Maybe we could start with this one. Would you be interested to do the follow-up? > > Sure, sounds good. I can follow-up next week. > > I've gone with "Permissions.Action_AudioCapture", aiming to match Rappor's "Permissions.Action.AudioCapture", except that I can't change the existing underscore separator. Would it make sense to rename the current PermissionTypes suffix ContentSettingsType and create a new PermissionTypes suffix that uses "." as a separator? It seems that the suffixes names aren't really used, right? https://codereview.chromium.org/1401073002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1401073002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:31092: + <owner>miguelg@chromium.org</owner> nit: add me, please :)
raymes@chromium.org changed reviewers: + raymes@chromium.org
https://codereview.chromium.org/1401073002/diff/120001/chrome/browser/media/m... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/1401073002/diff/120001/chrome/browser/media/m... chrome/browser/media/media_stream_devices_controller.cc:69: PermissionActionFunction action_function) { optional: most code I see in Chrome would use a base::Callback instead of a function pointer: base::Callback<void(ContentSettingsType, const GURL&)> Then at the callsites, base::Bind(...)
https://codereview.chromium.org/1401073002/diff/120001/chrome/browser/media/m... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/1401073002/diff/120001/chrome/browser/media/m... chrome/browser/media/media_stream_devices_controller.cc:69: PermissionActionFunction action_function) { On 2015/10/20 at 23:06:07, raymes wrote: > optional: most code I see in Chrome would use a base::Callback instead of a function pointer: > base::Callback<void(ContentSettingsType, const GURL&)> > > Then at the callsites, base::Bind(...) +1. Completely missed that.
tsergeant@chromium.org changed reviewers: + isherman@chromium.org, tommi@chromium.org
+isherman@ for tools/metrics/ Are there any problems with renaming a histogram suffix? And +tommi for chrome/browser/media/media_stream_devices_controller.cc https://codereview.chromium.org/1401073002/diff/100001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_uma_util.cc (right): https://codereview.chromium.org/1401073002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_context_uma_util.cc:113: "ContentSettings.PermissionActionsInsecureOrigin_MediaStreamCamera", On 2015/10/20 at 11:00:40, Mounir Lamouri wrote: > On 2015/10/20 at 06:38:43, tsergeant wrote: > > On 2015/10/19 at 10:42:19, Mounir Lamouri wrote: > > > Could you name these Permissions.Actions.VideoCapture? I think we should move these ContentSettings.* to Permissions.*. Maybe we could start with this one. Would you be interested to do the follow-up? > > > > Sure, sounds good. I can follow-up next week. > > > > I've gone with "Permissions.Action_AudioCapture", aiming to match Rappor's "Permissions.Action.AudioCapture", except that I can't change the existing underscore separator. > > Would it make sense to rename the current PermissionTypes suffix ContentSettingsType and create a new PermissionTypes suffix that uses "." as a separator? It seems that the suffixes names aren't really used, right? Done. https://codereview.chromium.org/1401073002/diff/120001/chrome/browser/media/m... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/1401073002/diff/120001/chrome/browser/media/m... chrome/browser/media/media_stream_devices_controller.cc:69: PermissionActionFunction action_function) { On 2015/10/20 at 23:07:34, Mounir Lamouri wrote: > On 2015/10/20 at 23:06:07, raymes wrote: > > optional: most code I see in Chrome would use a base::Callback instead of a function pointer: > > base::Callback<void(ContentSettingsType, const GURL&)> > > > > Then at the callsites, base::Bind(...) > > +1. Completely missed that. Done. https://codereview.chromium.org/1401073002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1401073002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:31092: + <owner>miguelg@chromium.org</owner> On 2015/10/20 at 11:00:41, Mounir Lamouri wrote: > nit: add me, please :) Done.
https://codereview.chromium.org/1401073002/diff/120001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_uma_util.cc (right): https://codereview.chromium.org/1401073002/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_context_uma_util.cc:348: return "VideoCapture"; On 2015/10/20 06:38:43, tsergeant wrote: > Kendra, does it affect you if I change these values? I know these values were > only added by you in https://codereview.chromium.org/1388343003. I'll update my cl to match. Thanks.
https://codereview.chromium.org/1401073002/diff/160001/chrome/browser/media/m... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/1401073002/diff/160001/chrome/browser/media/m... chrome/browser/media/media_stream_devices_controller.cc:66: PermissionActionFunction; nit: using PermissionActionCallback = [...]; https://codereview.chromium.org/1401073002/diff/160001/chrome/browser/media/m... chrome/browser/media/media_stream_devices_controller.cc:70: PermissionActionFunction action_function) { nit: s/action_function/callback/
isherman@chromium.org changed reviewers: + holte@chromium.org
+Steve for rappor metrics https://codereview.chromium.org/1401073002/diff/160001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/1401073002/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:79028: <suffix name="DurableStorage" label="Durable Storage permission actions"/> Where are these recorded?
https://codereview.chromium.org/1401073002/diff/160001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_uma_util.cc (right): https://codereview.chromium.org/1401073002/diff/160001/chrome/browser/permiss... chrome/browser/permissions/permission_context_uma_util.cc:78: return ""; Did these get added to GetPermissionString in another CL?
https://codereview.chromium.org/1401073002/diff/160001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_uma_util.cc (right): https://codereview.chromium.org/1401073002/diff/160001/chrome/browser/permiss... chrome/browser/permissions/permission_context_uma_util.cc:78: return ""; On 2015/10/22 00:22:11, Steven Holte wrote: > Did these get added to GetPermissionString in another CL? I believe this cl is based off https://codereview.chromium.org/1388343003/ which is still pending.
https://codereview.chromium.org/1401073002/diff/160001/chrome/browser/media/m... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/1401073002/diff/160001/chrome/browser/media/m... chrome/browser/media/media_stream_devices_controller.cc:66: PermissionActionFunction; On 2015/10/21 at 08:34:08, Mounir Lamouri wrote: > nit: > using PermissionActionCallback = [...]; Done, thanks. https://codereview.chromium.org/1401073002/diff/160001/chrome/browser/media/m... chrome/browser/media/media_stream_devices_controller.cc:70: PermissionActionFunction action_function) { On 2015/10/21 at 08:34:08, Mounir Lamouri wrote: > nit: s/action_function/callback/ Done https://codereview.chromium.org/1401073002/diff/160001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_uma_util.cc (right): https://codereview.chromium.org/1401073002/diff/160001/chrome/browser/permiss... chrome/browser/permissions/permission_context_uma_util.cc:78: return ""; On 2015/10/26 at 01:42:08, kcarattini wrote: > On 2015/10/22 00:22:11, Steven Holte wrote: > > Did these get added to GetPermissionString in another CL? > > I believe this cl is based off https://codereview.chromium.org/1388343003/ which is still pending. Yup! https://codereview.chromium.org/1401073002/diff/160001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/1401073002/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:79028: <suffix name="DurableStorage" label="Durable Storage permission actions"/> On 2015/10/21 at 22:41:15, Ilya Sherman wrote: > Where are these recorded? Nowhere yet. The intention is replace "ContentSettings.PermissionActions_<something>" with "Permissions.Action.<something>" (discussed back in patchset 5).
lgtm
On 2015/10/26 18:35:32, Steven Holte wrote: > lgtm Ping isherman@ and tommi@ -- can you please take another look?
Thanks for the ping! I missed your reply previously. https://codereview.chromium.org/1401073002/diff/160001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/1401073002/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:79028: <suffix name="DurableStorage" label="Durable Storage permission actions"/> On 2015/10/26 03:21:19, tsergeant wrote: > On 2015/10/21 at 22:41:15, Ilya Sherman wrote: > > Where are these recorded? > > Nowhere yet. The intention is replace > "ContentSettings.PermissionActions_<something>" with > "Permissions.Action.<something>" (discussed back in patchset 5). If these are not recorded, then let's not add them to histograms.xml.
Thanks for reviewing! https://codereview.chromium.org/1401073002/diff/160001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/1401073002/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:79028: <suffix name="DurableStorage" label="Durable Storage permission actions"/> On 2015/10/29 00:20:15, Ilya Sherman wrote: > On 2015/10/26 03:21:19, tsergeant wrote: > > On 2015/10/21 at 22:41:15, Ilya Sherman wrote: > > > Where are these recorded? > > > > Nowhere yet. The intention is replace > > "ContentSettings.PermissionActions_<something>" with > > "Permissions.Action.<something>" (discussed back in patchset 5). > > If these are not recorded, then let's not add them to histograms.xml. Done.
lgtm https://codereview.chromium.org/1401073002/diff/200001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_uma_util.cc (right): https://codereview.chromium.org/1401073002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_uma_util.cc:77: permission == CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC) nit: {}
histograms lgtm
https://codereview.chromium.org/1401073002/diff/200001/chrome/browser/media/m... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/1401073002/diff/200001/chrome/browser/media/m... chrome/browser/media/media_stream_devices_controller.cc:177: RecordPermissionAction( Is this actually a user decision or is it forced by another mechanism?
https://codereview.chromium.org/1401073002/diff/200001/chrome/browser/media/m... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/1401073002/diff/200001/chrome/browser/media/m... chrome/browser/media/media_stream_devices_controller.cc:177: RecordPermissionAction( On 2015/11/03 at 04:19:30, kcarattini wrote: > Is this actually a user decision or is it forced by another mechanism? It appears to be to do with Android Permissions not allowing a permission to be granted. It's possible that this is indirectly tied to a user action (ie, user Denies an Android-level request for Camera and this bubbles down to the Chrome-level permission request here), but I'm not actually sure. https://codereview.chromium.org/1401073002/diff/200001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_uma_util.cc (right): https://codereview.chromium.org/1401073002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_uma_util.cc:77: permission == CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC) On 2015/10/29 at 08:30:16, tommi wrote: > nit: {} Done.
The CQ bit was checked by tsergeant@chromium.org
https://codereview.chromium.org/1401073002/diff/200001/chrome/browser/media/m... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/1401073002/diff/200001/chrome/browser/media/m... chrome/browser/media/media_stream_devices_controller.cc:177: RecordPermissionAction( On 2015/11/05 at 02:34:42, tsergeant wrote: > On 2015/11/03 at 04:19:30, kcarattini wrote: > > Is this actually a user decision or is it forced by another mechanism? > > It appears to be to do with Android Permissions not allowing a permission to be granted. > > It's possible that this is indirectly tied to a user action (ie, user Denies an Android-level request for Camera and this bubbles down to the Chrome-level permission request here), but I'm not actually sure. As discussed, I've replaced this with a TODO while we decide how best to handle this case.
The patchset sent to the CQ was uploaded after l-g-t-m from kcarattini@chromium.org, mlamouri@chromium.org, holte@chromium.org, tommi@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1401073002/#ps260001 (title: "Replace unusual case with todo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1401073002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1401073002/260001
Message was sent while issue was closed.
Committed patchset #13 (id:260001)
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/d91d3942758065ca7b0a609ecc83357598bc86b7 Cr-Commit-Position: refs/heads/master@{#358784} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
