We'll need to wait for Bug 1203292 to finish the buttons that we'd like to track.

Ash, in the new UI, is there only one spot to "clear" permission prompts?

(In reply to Peter Dolanjski [:pdol] from comment #2) > Ash, in the new UI, is there only one spot to "clear" permission prompts? yes in the control panel for all the newly migrated permissions. For the legacy ones like plug-ins it will be its own doorhanger...

Review commit: https://reviewboard.mozilla.org/r/68296/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/68296/

This adds a telemetry probe for measuring how many times a user clears a permission in the control center. We have a Histogram which is keyed by permission id (geo, camera, microphone, etc.) and has the following values: 0: total number of all clears 1: total number of clearing permanently allowed permissions 2: total number of clearing permanently blocked permissions 3: total number of clearing temporarily allowed permissions 4: total number of clearing temporarily blocked permissions Since we don't have the concept of temporarily allowed/blocked permissions yet, we can either choose to leave this patch open until then or merge it now and add them later. Peter: does the described histogram look useful enough for you? If I recall correctly, we said we wanted to go only for aggregate numbers for now, since we also have aggregate numbers for doorhanger interaction. Let me know if you imagined something else. :)

Comment on attachment 8776559 [details] Bug 1286118 - Add telemetry probe to measure changes to permissions after initial prompt. https://reviewboard.mozilla.org/r/68296/#review65648 ::: browser/base/content/browser.js:7323 (Diff revision 1) > + > + // Set telemetry values for clearing a permission > + let histogram = Services.telemetry.getKeyedHistogramById("WEB_PERMISSION_CLEARED"); > + > + // 0 : clear any permission (always set) > + histogram.add(aPermission.id, 0); I don't see the value of recording this, isn't this something we get by just adding up the other values recorded in that histogram? I think you wanted something like: histogram.add("(all)", permissionType); This would match what was done at: http://searchfox.org/mozilla-central/rev/3df383b8552c1f8059f5c21258388ddb5a2f33d0/toolkit/modules/PopupNotifications.jsm#152 ::: browser/base/content/browser.js:7326 (Diff revision 1) > + > + // 0 : clear any permission (always set) > + histogram.add(aPermission.id, 0); > + > + let permissionType; > + if (aPermission.state === SitePermissions.ALLOW) { The JS code in browser/ uses == rather than ===, unless there's a specific reason to need ===. ::: toolkit/components/telemetry/Histograms.json:10027 (Diff revision 1) > "description": "When restoring tabs on startup, reading from sessionstore.js failed, even though the file exists and is not containing an explicitly empty window.", > "cpp_guard": "ANDROID" > + }, > + "WEB_PERMISSION_CLEARED": { > + "alert_emails": ["firefox-dev@mozilla.org"], > + "bug_numbers": [1242013], not the right bug number ::: toolkit/components/telemetry/Histograms.json:10032 (Diff revision 1) > + "bug_numbers": [1242013], > + "expires_in_version": "55", > + "kind": "enumerated", > + "keyed": true, > + "n_values": 6, > + "description": "Web permissions that were revoked in the site control center." We likely want this description to give enough details to interpret the values when looking at the histogram. See the description of POPUP_NOTIFICATION_STATS for a good example.

Comment on attachment 8776559 [details] Bug 1286118 - Add telemetry probe to measure changes to permissions after initial prompt. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68296/diff/1-2/

Comment on attachment 8776559 [details] Bug 1286118 - Add telemetry probe to measure changes to permissions after initial prompt. https://reviewboard.mozilla.org/r/68296/#review65658 The code looks good to me, but you need an additional data collection review (see https://wiki.mozilla.org/Firefox/Data_Collection). ::: browser/base/content/browser.js:7330 (Diff revision 2) > + permissionType = 1; > + } else if (aPermission.state == SitePermissions.BLOCK) { > + // 2 : clear permanently blocked permission > + permissionType = 2; > + } > + // 3 : TODO clear temporary allowed permission I don't know if we want to land with this comment, or wait a little bit (bug 1206233 will add the temporarily allowed state soon). ::: toolkit/components/telemetry/Histograms.json:10032 (Diff revision 2) > + "bug_numbers": [1286118], > + "expires_in_version": "55", > + "kind": "enumerated", > + "keyed": true, > + "n_values": 6, > + "description": "Number of revoke actions on web permissions in the site control center, keyed by permission id. Values represent the permission type that was revoked. (1=permanently allowed, 2=permanently blocked, 3=temporarily allowed, 4=temporarily blocked)" nit: we could make this a bit more concise without losing any meaning: web permissions -> permissions site control center -> control center permanently allowed -> allowed permanently blocked -> blocked

Comment on attachment 8776559 [details] Bug 1286118 - Add telemetry probe to measure changes to permissions after initial prompt. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68296/diff/2-3/

(In reply to Florian Quèze [:florian] [:flo] from comment #8) > Comment on attachment 8776559 [details] > Bug 1286118 - Add telemetry probe to measure changes to permissions after > initial prompt. > > https://reviewboard.mozilla.org/r/68296/#review65658 > > The code looks good to me, but you need an additional data collection review > (see https://wiki.mozilla.org/Firefox/Data_Collection). > > ::: browser/base/content/browser.js:7330 > (Diff revision 2) > > + permissionType = 1; > > + } else if (aPermission.state == SitePermissions.BLOCK) { > > + // 2 : clear permanently blocked permission > > + permissionType = 2; > > + } > > + // 3 : TODO clear temporary allowed permission > > I don't know if we want to land with this comment, or wait a little bit (bug > 1206233 will add the temporarily allowed state soon). Since we're at the start of the 51 release how about we wait a bit to see if the required bugs arrive in time and if not we merge this in time so that it rides the trains. > permanently allowed -> allowed > permanently blocked -> blocked With these two I think it's better to be explicit.

(In reply to Johann Hofmann [:johannh] from comment #5) > Peter: does the described histogram look useful enough for you? If I recall > correctly, we said we wanted to go only for aggregate numbers for now, since > we also have aggregate numbers for doorhanger interaction. Let me know if > you imagined something else. :) This looks sufficient to me. Thanks!

Comment on attachment 8776559 [details] Bug 1286118 - Add telemetry probe to measure changes to permissions after initial prompt. bsmedberg, I'd like to ask for data collection approval for this probe. (Do you need anything else?) Thanks!

https://hg.mozilla.org/integration/fx-team/rev/27ffae997b83a5bfeaf0967e692074cb097d94e8 Bug 1286118 - Add telemetry probe to measure changes to permissions after initial prompt. r=florian data-review=bsmedberg

Backed out for failing browser_devices_get_user_media.js: https://hg.mozilla.org/integration/fx-team/rev/aa1a7886af087c5c13cd087ac1c67dac7e553492 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=27ffae997b83a5bfeaf0967e692074cb097d94e8 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=11389004&repo=fx-team 07:19:57 INFO - 201 INFO TEST-PASS | browser/base/content/test/webrtc/browser_devices_get_user_media.js | popup WebRTC indicator visible - 07:19:57 INFO - 202 INFO TEST-PASS | browser/base/content/test/webrtc/browser_devices_get_user_media.js | video global indicator attribute as expected - 07:19:57 INFO - 203 INFO TEST-PASS | browser/base/content/test/webrtc/browser_devices_get_user_media.js | audio global indicator attribute as expected - 07:19:57 INFO - 204 INFO TEST-PASS | browser/base/content/test/webrtc/browser_devices_get_user_media.js | screen global indicator attribute as expected - 07:19:57 INFO - 205 INFO TEST-PASS | browser/base/content/test/webrtc/browser_devices_get_user_media.js | only one global indicator window - 07:19:57 INFO - 206 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/webrtc/browser_devices_get_user_media.js | uncaught exception - Error: Not a number at gIdentityHandler._createPermissionItem/<@chrome://browser/content/browser.js:7437:7 07:19:57 INFO - stopSharing@chrome://mochitests/content/browser/browser/base/content/test/webrtc/head.js:341:3

Thanks for the backout, this was a small oversight that should be easy to fix!

Peter, I think this is mostly a decision for you: The tests were failing because we didn't correctly handle cases where the permission type that was cleared is not categorized by us as permanent (or temporary in the future). Once we add support for temporary states this should not happen too much, but depending on future implementations there could always be some freak state that doesn't fit our categorization of perm/temp. What are we supposed to do with these? We can either a) ignore and not report them at all b) set them to the value of 0, meaning "others" and include them in the telemetry report Which do you prefer? Thanks!

(In reply to Johann Hofmann [:johannh] from comment #16) > What are we supposed to do with these? We can either > > a) ignore and not report them at all > b) set them to the value of 0, meaning "others" and include them in the > telemetry report > > Which do you prefer? I'd suggest b, so that we know they are occurring and can take action if the volume is high.

Sounds like a good choice, let's do it!

https://hg.mozilla.org/integration/fx-team/rev/7a5d8ff573bc76752e92d7e8a8016be90c8bc8fd Bug 1286118 followup - Remove accidentally added if clause. r=me

https://hg.mozilla.org/mozilla-central/rev/9431390a4d87 https://hg.mozilla.org/mozilla-central/rev/7a5d8ff573bc