Closed Bug 1351034 Opened 3 years ago Closed 3 years ago



(Core :: DOM: Push Notifications, enhancement, P3)




Tracking Status
firefox55 --- fixed


(Reporter: chutten, Assigned: swapneshks, Mentored)



(Whiteboard: [good first bug][lang=c++])


(2 files, 3 obsolete files)

The Telemetry histograms WEB_NOTIFICATION_PERMISSION_REMOVED, WEB_NOTIFICATION_SENDERS expired in Firefox 50. Since they expired they are no longer collecting or transmitting information, so they can be removed.

This bug is for the removal of the code accumulating to or otherwise using the histograms, any tests that include the histograms, the definition of the histograms in toolkit/components/telemetry/Histograms.json, and their presences in histograms-whitelist.json (if applicable).

Due to how currently works, once we remove the definitions from Histograms.json, even historic data on TMO will stop being available. Generally speaking, this isn't an issue with most probe removals, but if you suspect that it will be a problem with this one, it would be a good idea to mention it.

This bug was created with the template
Please feel free to create your own expired-probe-removal bugs!
Component: Telemetry → DOM: Push Notifications
Product: Toolkit → Core
Bryan, looping you in just in case:

* WEB_NOTIFICATION_PERMISSION_REMOVED counts the number of times a user manually changed permissions for a site in about:preferences#content, or in the control center. (Either removing the grant, or removing the block so the site can ask again). It's opt-in.

* WEB_NOTIFICATION_SENDERS is the number of unique origins that have shown a push or vanilla desktop notification. It's opt-out, so I think we had plans for it at one time?

Since these two probes already expired in 50, is it safe to assume they haven't been useful, and remove them?
Flags: needinfo?(clarkbw)
I agree that I believe we had plans for WEB_NOTIFICATION_SENDERS which I cannot recall now.  We can remove anyway as I don't know what useful question this would answer beyond other metrics.

I don't need WEB_NOTIFICATION_PERMISSION_REMOVED and as an opt-in measure it has little value anyway.
Flags: needinfo?(clarkbw)
Priority: -- → P3
Hi Chris,
I am Santosh,from India.I want to resolve this bug.But as I am new,I don't know how to start working on it.After reading the comments by Kit and Bryan,I understood what WEB_NOTIFICATION_PERMISSION_REMOVED and WEB_NOTIFICATION_SENDERS is.But still I cannot find the repository where this bug is,so that I can contribute to it.It will be very helpful if you guide me a little.I want to know where the repository is,and where to look for this bug in that repository.
Flags: needinfo?(chutten)
Happy to help!

You may wish to begin by going through some of the Getting Started documentation on MDN:

That will give you a solid background of what things are and where you might be able to find them.

From there we have tools like DXR that allow you to search the mozilla-central tree very rapidly:

For this bug I will expect two patches, one for removing each of the probes. You can use mozreview to put these patches up for review:

If you have any questions, be sure to ask on #introduction or #developers or here on the bug.
Flags: needinfo?(chutten)
Assignee: nobody → sanal5hd
Have you had a chance to look into this bug? Will you have an opportunity in the near future? If not, no worries, I can just unassign this bug and someone else can take a look.
Flags: needinfo?(sanal5hd)
Assignee: sanal5hd → nobody
Hi! Can I work on this bug?
I am a beginner and have worked on a few moz bugs but still catching up to things...
I see there is sufficient info available in the description and comments. 
So, once I get the green signal, I can start working on it!
Flags: needinfo?(chutten)
Please go ahead, and let me know if you have any questions.
Assignee: nobody → swapneshks
Flags: needinfo?(sanal5hd)
Flags: needinfo?(chutten)
Attachment #8865497 - Flags: review?(chutten)
Attached patch Remove WEB_NOTIFICATION_SENDERS (obsolete) — Splinter Review
Attachment #8865498 - Flags: review?(chutten)
Hi Chris,

I have uploaded 2 separate patches, one for each of the probes. 
Do let me know if I missed anything..
Comment on attachment 8865497 [details] [diff] [review]

Review of attachment 8865497 [details] [diff] [review]:

Redirecting to domain expert. Looks good from Telemetry's perspective.
Attachment #8865497 - Flags: review?(kit)
Attachment #8865497 - Flags: review?(chutten)
Attachment #8865497 - Flags: feedback+
Attachment #8865498 - Flags: review?(kit)
Attachment #8865498 - Flags: review?(chutten)
Attachment #8865498 - Flags: feedback+
Comment on attachment 8865497 [details] [diff] [review]

Review of attachment 8865497 [details] [diff] [review]:

Looks great, thanks for doing this, Swapnesh! If you're up for it, you can remove the entire `Observe` method (and the `nsIObserver` implementation), `AddPermissionChangeObserver`, and `RemovePermissionChangeObserver`. I don't think we need it anymore, since we're not recording telemetry for permission changes. If not, no problem; I'll file a follow-up bug.
Attachment #8865497 - Flags: review?(kit) → review+
Comment on attachment 8865498 [details] [diff] [review]

Review of attachment 8865498 [details] [diff] [review]:

`RecordSender()` and `mOrigins` can be removed, too, if you're up for it.
Attachment #8865498 - Flags: review?(kit) → review+
Sure, no issues! I'll do that and upload a new set of patches.
Attachment #8865577 - Flags: review?(kit)
Attachment #8865577 - Flags: feedback?(chutten)
New set of patches have been uploaded. Let me know if I missed any instances.
Comment on attachment 8865577 [details] [diff] [review]

Review of attachment 8865577 [details] [diff] [review]:

Looks good, though I think you'll want to change `NotificationTelemetryService` to inherit from `nsISupports`, and change the macro to `NS_IMPL_ISUPPORTS(NotificationTelemetryService, nsISupports)`. Could I see another patch with those changes, please?
Attachment #8865577 - Flags: review?(kit)
Attachment #8865577 - Flags: feedback?(chutten)
Comment on attachment 8865580 [details] [diff] [review]
Remove WEB_NOTIFICATION_SENDERS and related usages

Review of attachment 8865580 [details] [diff] [review]:

This looks good!
Attachment #8865580 - Flags: review?(kit) → review+
Attachment #8865497 - Attachment is obsolete: true
Attachment #8865498 - Attachment is obsolete: true
Requested changes have been incorporated in this patch.
Attachment #8865908 - Flags: review?(kit)
Attachment #8865577 - Attachment is obsolete: true
Comment on attachment 8865908 [details] [diff] [review]

Thanks! I fixed up commit messages and pushed to try:
Attachment #8865908 - Flags: review?(kit) → review+
Attachment #8865580 - Flags: feedback?(chutten) → feedback+
Pushed by
Remove WEB_NOTIFICATION_PERMISSION_REMOVED. f=chutten; r=kitcambridge
Remove usages associated to WEB_NOTIFICATION_SENDERS. f=chutten; r=kitcambridge
Thanks again, Swapnesh!
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(In reply to Kit Cambridge (he/him) [:kitcambridge] (UTC-8) from comment #23)
> Thanks again, Swapnesh!

My pleasure! :)
Depends on: 1389057
You need to log in before you can comment on or make changes to this bug.