Peter Dolanjski [:pdol] Reporter
	Description • 8 years ago

      No description provided.

	Johann Hofmann [:johannh] Assignee
	Comment 1 • 8 years ago

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

	Peter Dolanjski [:pdol] Reporter
	Comment 2 • 8 years ago

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

	Ash Grigas
	Comment 3 • 8 years ago

(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...

	Johann Hofmann [:johannh] Assignee
	Comment 4 • 8 years ago

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

	Johann Hofmann [:johannh] Assignee
	Comment 5 • 8 years ago

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. :)

	Florian Quèze [:florian]
	Comment 6 • 8 years ago

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.

	Johann Hofmann [:johannh] Assignee
	Comment 7 • 8 years ago

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/

	Florian Quèze [:florian]
	Comment 8 • 8 years ago

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

	Johann Hofmann [:johannh] Assignee
	Comment 9 • 8 years ago

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/

	Johann Hofmann [:johannh] Assignee
	Comment 10 • 8 years ago

(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.

	Peter Dolanjski [:pdol] Reporter
	Comment 11 • 8 years ago

(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!

	Johann Hofmann [:johannh] Assignee
	Comment 12 • 8 years ago

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!

	Johann Hofmann [:johannh] Assignee
	Comment 13 • 8 years ago

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

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 14 • 8 years ago

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

	Johann Hofmann [:johannh] Assignee
	Comment 15 • 8 years ago

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

	Johann Hofmann [:johannh] Assignee
	Comment 16 • 8 years ago

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!

	Peter Dolanjski [:pdol] Reporter
	Comment 17 • 8 years ago

(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.

	Johann Hofmann [:johannh] Assignee
	Comment 18 • 8 years ago

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

	Johann Hofmann [:johannh] Assignee
	Comment 20 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9431390a4d87

	Johann Hofmann [:johannh] Assignee
	Comment 21 • 8 years ago

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

	Phil Ringnalda (:philor)
	Comment 22 • 8 years ago
bugherder

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