:gerard-majax Reporter
	Description • 11 years ago

This API is deprecated. This bug is about totally removing it when the time has come.

	Michael Henretty [:mikehenrty][:mhenretty]
	Comment 1 • 10 years ago

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?

	:gerard-majax Reporter
	Comment 2 • 10 years ago

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

	:gerard-majax Reporter
	Comment 3 • 10 years ago

Those are blocking bug 938541

	Michael Henretty [:mikehenrty][:mhenretty]
	Comment 4 • 10 years ago

Thanks for the info. We need this!

	Zibi Braniecki [:zbraniecki][:gandalf]
	Comment 5 • 10 years ago

We can now remove the API completely. :)

	:gerard-majax Reporter
	Comment 6 • 10 years ago

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

	Zibi Braniecki [:zbraniecki][:gandalf]
	Comment 7 • 10 years ago

Absolutely :) 

How do we start the process? Can we mark it as deprecated/fire a warning?

	Kohei Yoshino [:kohei]
	Comment 8 • 9 years ago

Posted the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/navigator-moznotification-will-be-removed/

	Kyle Machulis [:qdot] [:kmachulis] (INACTIVE) Assignee
	Comment 9 • 6 years ago

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.

	Kyle Machulis [:qdot] [:kmachulis] (INACTIVE) Assignee
	Comment 12 • 6 years ago

Removal passed try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd7436ee287467de6236f57c3fc76a47419616bb&selectedJob=150970312

	Kohei Yoshino [:kohei]
	Comment 13 • 6 years ago

Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2017/navigator-moznotification-has-been-removed/

	Andrea Marchesini [:baku]
	Comment 14 • 6 years ago
mozreview-review

Comment on attachment 8935979 [details]
Bug 952453 - Remove mozNotification API

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

	Andrea Marchesini [:baku]
	Comment 15 • 6 years ago
mozreview-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!

	Kyle Machulis [:qdot] [:kmachulis] (INACTIVE) Assignee
	Comment 16 • 6 years ago

Moving test cleanup to Bug 1424571

	Pulsebot
	Comment 17 • 6 years ago

Pushed by kmachulis@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e682c1841a1
Remove mozNotification; r=baku

	Stefan Hindli [:stefan_hindli]
	Comment 18 • 6 years ago

Backed out 2 changesets (bug 1424571, bug 952453) for FATAL ERROR PROCESSING MOZBUILD FILE r=backout on a CLOSED TREE

https://hg.mozilla.org/integration/mozilla-inbound/rev/f2e83780f1b36281e9ddf74cc2a67e277799f236
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=c05f6d6df5104d551828f54b86f7bf04ffe10d57&filter-classifiedState=unclassified&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception

https://treeherder.mozilla.org/logviewer.html#?job_id=151066271&repo=mozilla-inbound&lineNumber=1828

	Kyle Machulis [:qdot] [:kmachulis] (INACTIVE) Assignee
	Comment 19 • 6 years ago

Missed service worker test updates in bug 1424571. Fixed but rerunning on try first just in case.

	Pulsebot
	Comment 20 • 6 years ago

Pushed by kmachulis@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fd1d4a79a48
Remove mozNotification; r=baku

	Tiberius Oros[:tiberius_oros]
	Comment 21 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/8fd1d4a79a48

	Matthew N. [:MattN]
	Comment 22 • 6 years ago
mozreview-review

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

	Kyle Machulis [:qdot] [:kmachulis] (INACTIVE) Assignee
	Comment 23 • 6 years ago

Ok, was looking at this right now anyways, I'll back it out now and follow up here.

	Kyle Machulis [:qdot] [:kmachulis] (INACTIVE) Assignee
	Comment 25 • 6 years ago

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.

	Pulsebot
	Comment 26 • 6 years ago

Pushed by kmachulis@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6500c02faddf
Backing out 8fd1d4a79a48 due to notification bustage on MacOS

	Kyle Machulis [:qdot] [:kmachulis] (INACTIVE) Assignee
	Comment 27 • 6 years ago

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

	Matthew N. [:MattN]
	Comment 29 • 6 years ago
mozreview-review

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?

	Kyle Machulis [:qdot] [:kmachulis] (INACTIVE) Assignee
	Comment 32 • 6 years ago

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.

	Pulsebot
	Comment 33 • 6 years ago

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

	Kyle Machulis [:qdot] [:kmachulis] (INACTIVE) Assignee
	Comment 34 • 6 years ago

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

	Matthew N. [:MattN]
	Comment 35 • 6 years ago
mozreview-review

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

	Eliza Balazs [:ebalazs_]
	Comment 36 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/781a6bc83dde
https://hg.mozilla.org/mozilla-central/rev/e0895bd719ee

	Chris Mills [:cmills]
	Comment 38 • 6 years ago

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!

	Kyle Machulis [:qdot] [:kmachulis] (INACTIVE) Assignee
	Comment 39 • 6 years ago

lgtm, thanks!