921612 - Fix PushManager WebIDL to move NavigatorProperty to Navigator WebIDL

Status: RESOLVED INVALID

(Core :: DOM: Push Subscriptions, defect)

Comment 1

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

Attachment: Fix PushManager WebIDL to move NavigatorProperty to Navigator WebIDL.

Would this be the right way to do it?

Comment 2

[Masatoshi Kimura [:emk]]

Comment on attachment 811365 [details] [diff] [review]
Fix PushManager WebIDL to move NavigatorProperty to Navigator WebIDL.

>-[NoInterfaceObject, NavigatorProperty="push", JSImplementation="@mozilla.org/push/PushManager;1", Func="Navigator::HasPushNotificationsSupport"]
>+[JSImplementation="@mozilla.org/push/PushManager;1"]

Do not remove NoInterfaceObject.

Comment 3

[Masatoshi Kimura [:emk]]

Comment on attachment 811365 [details] [diff] [review]
Fix PushManager WebIDL to move NavigatorProperty to Navigator WebIDL.

> >-[NoInterfaceObject, NavigatorProperty="push", JSImplementation="@mozilla.org/push/PushManager;1", Func="Navigator::HasPushNotificationsSupport"]
> >+[JSImplementation="@mozilla.org/push/PushManager;1"]
> 
> Do not remove NoInterfaceObject.

Actually PushManager didn't have [NoInterfaceObject] in the spec.
https://dvcs.w3.org/hg/push/raw-file/tip/index.html#pushmanager-interface
An actual Push API owner should review this.

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Does this build?

Comment 5

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

(In reply to :Ms2ger from comment #4)
> Does this build?

Nope, seems like this style isn't supported for JS implemented components since it tries to look for a GetPush on Navigator, which would then have to create the component and return it. That sounds like doing manually what is being done automatically. So is it worth the 'correctness' of it?

Comment 6

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

Going to mark this as INVALID if no one has objections. See comment 5 for why it doesn't work, if things have changed, I'd be happy to fix it.

Comment 7

[Boris Zbarsky [:bzbarsky]]

If the spec says this is supposed to be an IDL attribute, we should really do it that way.  The NavigatorProperty stuff is a mess that was set up for things that were already grandfathered in; it's not meant to be used for new things.  It'll lead to clearly wrong web-facing behavior when people try to hook the getter, for example.

That said, we could try to just make NavigatorProperty not be such a mess and, for example, to either automatically generate the necessary getters on Navigator or at least make them much simpler to implement.  We've talked about that before, on and off.  We could go ahead and set up something like that if this is likely to be a recurring issue.

Comment 8

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

(In reply to Not doing reviews right now from comment #7)
> If the spec says this is supposed to be an IDL attribute, we should really
> do it that way.  The NavigatorProperty stuff is a mess that was set up for
> things that were already grandfathered in; it's not meant to be used for new
> things.  It'll lead to clearly wrong web-facing behavior when people try to
> hook the getter, for example.
> 
> That said, we could try to just make NavigatorProperty not be such a mess
> and, for example, to either automatically generate the necessary getters on
> Navigator or at least make them much simpler to implement.  We've talked
> about that before, on and off.  We could go ahead and set up something like
> that if this is likely to be a recurring issue.

This is no longer relevant for push since PushManager is now exposed on ServiceWorkerRegistration.