1183853 - Rename hasPermission() to permissionState()

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect)

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: MozReview Request: Bug 1183853 - Rename hasPermission() to permissionState(). r?mt

Bug 1183853 - Rename hasPermission() to permissionState(). r?mt

Comment 2

[Martin Thomson [:mt:]]

Comment on attachment 8633697 [details]
MozReview Request: Bug 1183853 - Rename hasPermission() to permissionState(). r?mt

https://reviewboard.mozilla.org/r/13247/#review11885

Ship It!

::: dom/push/Push.js:316
(Diff revision 1)
>      }.bind(this));

Would you mind changing this to an => function?

Comment 3

[Martin Thomson [:mt:]]

In case it's not already obvious, I'm not a DOM peer, so my review is not sufficient to land WebIDL changes.

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8633697 [details]
MozReview Request: Bug 1183853 - Rename hasPermission() to permissionState(). r?mt

Bug 1183853 - Rename hasPermission() to permissionState(). r?mt

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Olli for DOM peer sign off.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8633697 [details]
MozReview Request: Bug 1183853 - Rename hasPermission() to permissionState(). r?mt

So we're still missing the PushSubscriptionOptions param.

Comment 7

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

I believe :mt is against this and the intention is to drop it from the spec?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

oh, I meant the patch is ok, and I was assuming we'll add the param later, if needed.

Comment 9

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/2fcda90e38a23b489b4fd5281cf79dc745ea2117
changeset:  2fcda90e38a23b489b4fd5281cf79dc745ea2117
user:       Nikhil Marathe <[e-mail]>
date:       Tue Jul 14 14:27:32 2015 -0700
description:
Bug 1183853 - Rename hasPermission() to permissionState(). r=mt,smaug

Comment 10

[Martin Thomson [:mt:]]

Yeah, I'm still against it, but only really the specific option that Google have implemented.  People developing against chrome will pass a dictionary; we shouldn't choke on that.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2fcda90e38a2

Comment 12

[Kohei Yoshino]

Site compat doc: https://www.fxsitecompat.com/en-US/docs/2015/pushmanager-haspermission-has-been-removed/

Comment 13

[Kohei Yoshino]

Hmm, the Push API has been implemented with Firefox 42, so it should have no compatibility impact. Removing the compat doc.

Comment 14

[Chris Mills [:cmills]]

hasPermission() has been marked deprecated:

https://developer.mozilla.org/en-US/docs/Web/API/PushManager/hasPermission

permissionState() has been documented:

https://developer.mozilla.org/en-US/docs/Web/API/PushManager/permissionState

A note has been added to the relevant release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/42

Thanks!