1197998 - Add use counters for Push interfaces.

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect)

Description

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://groups.google.com/forum/#!topic/mozilla.dev.platform/ktoEer_cdJA for how to do this. Should be fairly easy to do.

We want to know when the PushManager is accessed and register() is called.

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: MozReview Request: Bug 1197998 - Use counters for Push subscribe, unsubscribe and permission query. r?froydnj,kitcambridge,jst

Bug 1197998 - Use counters for Push subscribe, unsubscribe and permission query. r?froydnj,kitcambridge,jst

Nathan for correct use of UseCounters. Be advised these interfaces are exposed to workers, will that be a problem?
Kit because it is for Push. Do we really need to count permissionState? My goal was to know how often sites ask permission and perhaps combine that with telemetry about approved/rejected. But perhaps that would be better served by telemetry in subscribed since that is the actual permission ask, and this is just a permission check. Happy to remove.
Johnny because of changes to webidl.

Comment 2

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

Comment 3

[Johnny Stenback (:jst)]

Comment on attachment 8656930 [details]
MozReview Request: Bug 1197998 - Use counters for Push subscribe, unsubscribe and permission query. r?froydnj,kitcambridge,jst

https://reviewboard.mozilla.org/r/18269/#review16393

WebIDL changes look good assuming the threading issues they result in are signed off on. r=jst

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

mochitests seem to be ok with workers.

Comment 5

[Lina Butler [:lina]]

(In reply to Nikhil Marathe [:nsm] (please needinfo?) from comment #1)
> Kit because it is for Push. Do we really need to count permissionState? My
> goal was to know how often sites ask permission and perhaps combine that
> with telemetry about approved/rejected. But perhaps that would be better
> served by telemetry in subscribed since that is the actual permission ask,
> and this is just a permission check. Happy to remove.

I agree; `subscribe()` + your telemetry patch is probably a better measure than the permission check by itself.

Comment 6

[Lina Butler [:lina]]

Comment on attachment 8656930 [details]
MozReview Request: Bug 1197998 - Use counters for Push subscribe, unsubscribe and permission query. r?froydnj,kitcambridge,jst

https://reviewboard.mozilla.org/r/18269/#review16443

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 8656930 [details]
MozReview Request: Bug 1197998 - Use counters for Push subscribe, unsubscribe and permission query. r?froydnj,kitcambridge,jst

https://reviewboard.mozilla.org/r/18269/#review16521

Ooo, I didn't think about workers when I wrote the use counter stuff.  Even if the generated code looks right, the main interface from the bindings code (mozilla::dom::SetDocumentAndPageUseCounter) really wants a document to attach the use counter to...so I don't think this is going to record stuff correctly from workers. :(

On the other hand, I don't think it will do any harm to have the calls in there along the worker path...they just won't do anything.  So r=me with the above caveat.

Want to file a bug on getting worker support up and running?  It might be reasonable to have separate counters for uses in workers and uses on the main thread...maybe?

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2fd91dc12a42

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2fd91dc12a42

Comment 10

[Edwin Wong [:edwong]]

NI - should we request uplift into fx42

Comment 11

[[e-mail]]

I vote yes.  :wmaggs: is final signoff.