Closed Bug 1197998 Opened 4 years ago Closed 4 years ago

Add use counters for Push interfaces.

Categories

(Core :: DOM: Push Notifications, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: nsm, Assigned: nsm, Mentored, NeedInfo)

References

Details

(Whiteboard: [good first bug])

Attachments

(1 file)

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.
Whiteboard: [good first bug]
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.
Attachment #8656930 - Flags: review?(nfroyd)
Attachment #8656930 - Flags: review?(kcambridge)
Attachment #8656930 - Flags: review?(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
Attachment #8656930 - Flags: review?(jst) → review+
mochitests seem to be ok with workers.
(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 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
Attachment #8656930 - Flags: review?(kcambridge) → review+
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?
Attachment #8656930 - Flags: review?(nfroyd) → review+
https://hg.mozilla.org/mozilla-central/rev/2fd91dc12a42
Assignee: nobody → nsm.nikhil
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Blocks: 1204608
NI - should we request uplift into fx42
Flags: needinfo?(wmaggs)
Flags: needinfo?(rweiss)
I vote yes.  :wmaggs: is final signoff.
Flags: needinfo?(rweiss)
You need to log in before you can comment on or make changes to this bug.