Lina Butler [:lina] Assignee
	Description • 8 years ago

Per https://notifications.spec.whatwg.org/#dom-notification-permission, the callback argument is deprecated.

`Notification.requestPermission` should return a promise fulfilled with the permission, invoking the callback if given.

	Lina Butler [:lina] Assignee
	Comment 1 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/31671/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31671/

	Andrea Marchesini [:baku]
	Comment 2 • 8 years ago

Comment on attachment 8710151 [details]
MozReview Request: Bug 1241278 - Change `Notification.requestPermission()` to return a promise. r=baku

https://reviewboard.mozilla.org/r/31671/#review28459

Do we have any plan to remove the callback completely? In case, file a bug as follow up.

::: dom/notification/Notification.cpp:254
(Diff revision 1)
>    {

MOZ_ASSERT(aPromise)

::: dom/notification/Notification.cpp:650
(Diff revision 1)
>  NotificationPermissionRequest::CallCallback()

rename this to ResolvePromise()

	Lina Butler [:lina] Assignee
	Comment 3 • 8 years ago

Comment on attachment 8710151 [details]
MozReview Request: Bug 1241278 - Change `Notification.requestPermission()` to return a promise. r=baku

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31671/diff/1-2/

	Lina Butler [:lina] Assignee
	Comment 4 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/31791/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31791/

	Lina Butler [:lina] Assignee
	Comment 5 • 8 years ago

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

	Pulsebot
	Comment 6 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/86d2d6d0500e

	Phil Ringnalda (:philor)
	Comment 7 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/86d2d6d0500e

	Vladan Djeric (:vladan)
	Comment 8 • 8 years ago

Comment on attachment 8710545 [details]
MozReview Request: Bug 1241278 - Record telemetry for notification permission callback usage. r?vladan

https://reviewboard.mozilla.org/r/31791/#review28965

::: toolkit/components/telemetry/Histograms.json:10019
(Diff revision 1)
> +    "kind": "count",

This will generate a histogram of the # of times the callback version of this API was called per Firefox session.

Due to the nature of count histograms, the 0 bucket will be omitted, so you will actually get a distribution of the # of times this version of the function was called per session (among sessions that called this version of the function at least once).

Will that be enough to gauge its popularity? Would it be better to count these with a boolean histogram, so that you end up with a ratio of sessions that used the callback version vs the promise version?

::: toolkit/components/telemetry/Histograms.json:10020
(Diff revision 1)
> +    "description": "Usage of the deprecated Notification.requestPermission() callback argument"

add an alert_emails field

	Lina Butler [:lina] Assignee
	Comment 9 • 8 years ago

https://reviewboard.mozilla.org/r/31791/#review28965

> This will generate a histogram of the # of times the callback version of this API was called per Firefox session.
> 
> Due to the nature of count histograms, the 0 bucket will be omitted, so you will actually get a distribution of the # of times this version of the function was called per session (among sessions that called this version of the function at least once).
> 
> Will that be enough to gauge its popularity? Would it be better to count these with a boolean histogram, so that you end up with a ratio of sessions that used the callback version vs the promise version?

You're right; I'd like to know which version is used, not the number of callback uses. Thank you!

	Lina Butler [:lina] Assignee
	Comment 10 • 8 years ago

Comment on attachment 8710545 [details]
MozReview Request: Bug 1241278 - Record telemetry for notification permission callback usage. r?vladan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31791/diff/1-2/

	Vladan Djeric (:vladan)
	Comment 11 • 8 years ago

Comment on attachment 8710545 [details]
MozReview Request: Bug 1241278 - Record telemetry for notification permission callback usage. r?vladan

https://reviewboard.mozilla.org/r/31791/#review29479

	Pulsebot
	Comment 12 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/76f015b2176f

	Phil Ringnalda (:philor)
	Comment 13 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/76f015b2176f

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

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/callback-function-of-notification-requestpermission-has-been-deprecated/

	Chris Mills [:cmills]
	Comment 15 • 8 years ago

Thanks for the help in updating the MDN docs, Kit!

I've expanded the details a little bit, to include browser compat information, and showing the older syntax alongside the promise just for completeness:

https://developer.mozilla.org/en-US/docs/Web/API/Notification/requestPermission

I've also updated compat info and code syntax in other places:

https://developer.mozilla.org/en-US/docs/Web/API/Notifications_API
https://developer.mozilla.org/en-US/docs/Web/API/notification
https://developer.mozilla.org/en-US/docs/Web/API/Notifications_API/Using_the_Notifications_API

I've also updated my silly emogotchi demo to use the new syntax:

http://mdn.github.io/emogotchi/

And finally, I've added a note to the relevant release notes:

	Chris Mills [:cmills]
	Comment 16 • 8 years ago

*ahem*

  ...

And finally, I've added a note to the relevant release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/47#Others