Closed Bug 952453 Opened 6 years ago Closed 2 years ago

Remove mozNotification (DesktopNotification) API

Categories

(Core :: DOM: Core & HTML, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: gerard-majax, Assigned: qdot)

References

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(2 files, 1 obsolete file)

This API is deprecated. This bug is about totally removing it when the time has come.
Blocks: 952454
Doing a quick grep, it seems the only place that still uses mozNotification.createNotification is share/js/notification_helper.js. A further grep reveals the following apps using it:

costcontrol
calendar
dialer
email??
wappush
system

We should either fix NotificationHelper to use the new Notification API, assuming we can do so without breaking existing functionality. Or file bugs in each of these apps so we can get the eyes of module owners on this issue. This is starting to hurt us, supporting both API's in gecko while we try to add features to Web Notifications.

Alex, any ideas on the approach we should take here?
Flags: needinfo?(lissyx+mozillians)
(In reply to Michael Henretty [:mhenretty] from comment #1)
> Doing a quick grep, it seems the only place that still uses
> mozNotification.createNotification is share/js/notification_helper.js. A
> further grep reveals the following apps using it:
> 
> costcontrol
> calendar
> dialer
> email??
> wappush
> system
> 
> We should either fix NotificationHelper to use the new Notification API,
> assuming we can do so without breaking existing functionality. Or file bugs
> in each of these apps so we can get the eyes of module owners on this issue.
> This is starting to hurt us, supporting both API's in gecko while we try to
> add features to Web Notifications.
> 
> Alex, any ideas on the approach we should take here?

Bugs have been filed a long time ago already. Someone needs to push for this.
Flags: needinfo?(lissyx+mozillians)
Those are blocking bug 938541
Depends on: 938541
Flags: needinfo?(mhenretty)
Thanks for the info. We need this!
Flags: needinfo?(mhenretty)
We can now remove the API completely. :)
(In reply to Zibi Braniecki [:gandalf] from comment #5)
> We can now remove the API completely. :)

I think the proper wording is that we can start the API self-destruction process: there may be content using it (apps on the marketplace), so I doubt we can just kill it right now, but rather start deprecating and killing it soon.
Absolutely :) 

How do we start the process? Can we mark it as deprecated/fire a warning?
Summary: Remove mozNotification API → Remove mozNotification (DesktopNotification) API
mozNotification has been pref'd off on desktop since January 2011, and it's no longer used in FxOS since that's no longer in the codebase either. Taking and removing.
Assignee: nobody → kyle
Attachment #8935979 - Flags: review?(amarchesini)
Attachment #8935980 - Flags: review?(amarchesini)
Comment on attachment 8935979 [details]
Bug 952453 - Remove mozNotification API

https://reviewboard.mozilla.org/r/206834/#review212654
Attachment #8935979 - Flags: review?(amarchesini) → review+
Comment on attachment 8935980 [details]
Bug 952453 - Move all notification tests to dom/notification

https://reviewboard.mozilla.org/r/206836/#review212656

Unrelated. Can you move this in a separate bug? And land it. Thanks!
Attachment #8935980 - Flags: review?(amarchesini) → review+
Moving test cleanup to Bug 1424571
Missed service worker test updates in bug 1424571. Fixed but rerunning on try first just in case.
Flags: needinfo?(kyle)
Attachment #8935980 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/8fd1d4a79a48
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Depends on: 1427996
Comment on attachment 8935979 [details]
Bug 952453 - Remove mozNotification API

https://reviewboard.mozilla.org/r/206834/#review215938

Hello,

I would recommend that this bug get backed out because of the below issues. In the future please get review from peers for all relevant modules as I suspect they would have caught this issue (in this case Cocoa ones).

Unfortunately reviewboard doesn't show the contents of deleted tests but I looked at them and don't think many should have been deleted just because they use navigator.mozNotification rather than window.Notification since some of the behaviour being tested was about shared code. Some of the deleted tests didn't even use mozNotification.
* test_system_principal.xul wasn't even using mozNotification but wasn't in any .ini file but it should be added to a chrome.ini instead of deleted.
* test_notification_tag.html is still relevant AFAICT and wasn't using mozNotification
* test_leak_windowClose.html is probably stil relevant if switched to window.Notification
* etc.

I do see that the tests were not being run (bug 1043918) due to a mistake in bug 899574 but that's something that should be fixed IMO. Our test coverage of notifications is already quite weak IMO and I'm worried that we're making it worse.

::: widget/cocoa/OSXNotificationCenter.mm
(Diff revision 1)
> -- (void)userNotificationCenter:(id<FakeNSUserNotificationCenter>)center
> -        didDeliverNotification:(id<FakeNSUserNotification>)notification
> -{
> -
> -}
> -

None of the Cocoa could should have been removed as it's not specific to the mozNotification API, it's used for the standard `new window.Notification` API and possibly push notifications.

This has caused bug 1427996 but also regressed behaviour that each of the delegate methods was providing (close events, showing notifications when the application is focused, handling of the alternate actions dropdown to open prefs or disable for the site).
Attachment #8935979 - Flags: review-
Ok, was looking at this right now anyways, I'll back it out now and follow up here.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Duplicate of this bug: 1427996
Try run for backout happening at https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1e57aacf77f67ffa13aeef083c7e1694b9898c1. After that passes, I'll land backout on inbound, then will remove cocoa changes, add a "this is used indirectly, don't delete" comment on it so no one tries this again without asking, and fix up the tests. Will keep :MattN as reviewer for now since they're aware of the situation.
Pushed by kmachulis@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6500c02faddf
Backing out 8fd1d4a79a48 due to notification bustage on MacOS
Ok, updated tests that were deleted in the first version of the patch in this bug:

- test_basic_notification - already covered by dom/notifications/test/mochitest/test_notification_basics.html
- test_basic_notification_click - already covered by dom/notifications/test/mochitest/test_notification_basics.html
- test_leak_windowClose - leaks would be triggered in test_notification_basics, so calling it already covered.
- test_system_principal - moved to dom/notifications/test/browser/test_notification_system_principal.html
- test_notification_tag - moved to dom/notifications/test/mochitest/test_notification_tag.html
Attachment #8935979 - Attachment is obsolete: true
Comment on attachment 8935979 [details]
Bug 952453 - Remove mozNotification API

https://reviewboard.mozilla.org/r/206834/#review216318

Note that you may want/need to re-add :baku as a reviewer (the r+ should carry over) since I think there may be hooks to ensure DOM review on test_interfaces.js but I may be misremembering.

Thanks

::: dom/notification/test/mochitest/test_notification_tag.html:1
(Diff revision 2)
> +<!DOCTYPE HTML>

Nit: should this have been an `hg mv` too?
Attachment #8935979 - Flags: review?(MattN+bmo) → review+
Since I'm using git-cinnabar and git doesn't like move + change, I just split this into 2 patches. We move in the first one, change in the second.
Pushed by kmachulis@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/781a6bc83dde
Remove mozNotification API; r=mattn r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0895bd719ee
Fix test_notification_tag to point to correct location for create_notification.html; r=mattn
Went ahead and pushed this, as the patch division is no different than the originally reviewed code, and I'd really like to get Mac user's notifications working again. :)
Comment on attachment 8940355 [details]
Bug 952453 - Fix test_notification_tag to point to correct location for create_notification.html;

https://reviewboard.mozilla.org/r/210624/#review216982

Thanks
Attachment #8940355 - Flags: review?(MattN+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/781a6bc83dde
https://hg.mozilla.org/mozilla-central/rev/e0895bd719ee
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Duplicate of this bug: 1425918
I've documented this by archiving the property page and adding a note to the Fx59 rel notes:

https://developer.mozilla.org/en-US/docs/Archive/API/Navigator/mozNotification
https://developer.mozilla.org/en-US/Firefox/Releases/59#APIs_2

We never documented the DesktopNotification interface.

Let me know if that looks OK. Thanks!
Flags: needinfo?(kyle)
lgtm, thanks!
Flags: needinfo?(kyle)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.