Closed Bug 921612 Opened 7 years ago Closed 5 years ago

Fix PushManager WebIDL to move NavigatorProperty to Navigator WebIDL

Categories

(Core :: DOM: Push Notifications, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED INVALID

People

(Reporter: nsm, Assigned: nsm)

References

Details

Attachments

(1 file)

Assignee: nobody → nsm.nikhil
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 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.
Attachment #811365 - Flags: review?(VYV03354)
Does this build?
(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?
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.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → INVALID
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.
Flags: needinfo?(nsm.nikhil)
(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.
Flags: needinfo?(nsm.nikhil)
You need to log in before you can comment on or make changes to this bug.