If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

SimplePush: Convert interfaces to WebIDL

RESOLVED FIXED in mozilla25

Status

()

Core
DOM: Push Notifications
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: nsm, Assigned: nsm)

Tracking

(Blocks: 1 bug)

unspecified
mozilla25
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

Once JS-implemented interfaces can be described in WebIDL (is this already possible?) switch Push to use it.
Yes, that's possible. See RTCPeerConnection, for example.
Blocks: 580070
Component: DOM → DOM: Device Interfaces
OS: Linux → All
Hardware: x86_64 → All

Updated

4 years ago
Component: DOM: Device Interfaces → DOM: Push Notifications
Created attachment 782892 [details] [diff] [review]
Convert SimplePush to WebIDL.
Attachment #782892 - Flags: review?(khuey)
I actually want PushManager to be a singleton on navigator.push, so I tried adding [NoInterfaceObject] (is it the right attribute?) to the IDL, but then I get this error:

0:24.96 /home/nikhil/mozilla-central-push-improv/obj-b2g-desktop/dom/bindings/RegisterBindings.cpp:817:1: error: use of undeclared identifier 'PushManagerBinding'; did you mean 'UndoManagerBind
ing'?
 0:24.96 REGISTER_NAVIGATOR_CONSTRUCTOR("push", PushManager, nullptr);
 0:24.96 ^
 0:24.96 /home/nikhil/mozilla-central-push-improv/obj-b2g-desktop/dom/bindings/RegisterBindings.cpp:394:80: note: expanded from macro 'REGISTER_NAVIGATOR_CONSTRUCTOR'
 0:24.96   aNameSpaceManager->RegisterNavigatorDOMConstructor(NS_LITERAL_STRING(_prop), _dom_class##Binding::ConstructNavigatorObject, _ctor_check);
 0:24.96                                                                                ^
 0:24.96 <scratch space>:25:1: note: expanded from here
 0:24.96 PushManagerBinding
 0:24.96 ^
Comment on attachment 782892 [details] [diff] [review]
Convert SimplePush to WebIDL.

Review of attachment 782892 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

::: dom/push/src/Push.js
@@ +16,5 @@
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/DOMRequestHelper.jsm");
>  Cu.import("resource://gre/modules/AppsUtils.jsm");
>  
> +const PUSH_CID = Components.ID("{cde1d019-fad8-4044-b141-65fb4fb7a245}");

Not that it really matters but why did the CID change?
Attachment #782892 - Flags: review?(khuey) → review+
The UUID was changed because I thought it had to be changed, but apparently that is not the case with WebIDL.

https://hg.mozilla.org/integration/mozilla-inbound/rev/675ea8aeb804
Created attachment 783398 [details] [diff] [review]
Remove dom_push.xpt from installers.

Do I also need to remove from browser/installer/removed-files...?
Or do things always stay in that so that the files don't linger on when users update?
Attachment #783398 - Flags: review?(doug.turner)
(In reply to Nikhil Marathe [:nsm] from comment #5)
> The UUID was changed because I thought it had to be changed, but apparently
> that is not the case with WebIDL.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/675ea8aeb804

Backed out for causing build failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=25933864&tree=Mozilla-Inbound

And the mention of PushManager in this test failure sounds related too? https://tbpl.mozilla.org/php/getParsedLog.php?id=25934368&tree=Mozilla-Inbound
Depends on: 899904

Updated

4 years ago
Attachment #783398 - Flags: review?(doug.turner) → review+
Try 2:

https://hg.mozilla.org/integration/mozilla-inbound/rev/769c7e06a1f0
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1a2caba50bf
This code is ... untested.  I can tell because it violates the "the init method must not return anything" requirement by returning null, thus triggering fatal asserts if anyone ever touches navigator.push.  Except in opt builds, where it does something bizarre in that case, and probably not at all what's intended.

I've made this not turn the tree orange in https://hg.mozilla.org/integration/mozilla-inbound/rev/0868beaeab71 but I suspect that the behavior is still not what you really want.  I just can't tell what you really want.
Oh, and please add some tests!
Also, Andrew, should we replace that "undefined was returned" MOZ_ASSERT with a RUNTIMEABORT or something?  Note that we have no debug b2g test automation or anything.  :(
Flags: needinfo?(continuation)
Yeah, I guess that makes sense.  If people are converting existing JS-implemented things to WebIDL, then there may be dangerous behavior happening for non-undefined return values.
Flags: needinfo?(continuation)
https://hg.mozilla.org/mozilla-central/rev/769c7e06a1f0
https://hg.mozilla.org/mozilla-central/rev/b1a2caba50bf
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
(In reply to Boris Zbarsky (:bz) from comment #9)

> I've made this not turn the tree orange in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/0868beaeab71 but I
> suspect that the behavior is still not what you really want.  I just can't
> tell what you really want.

:bz I want to not allow access to navigator.push if the app doesn't have push permission. How would I achieve this in an 'init should not return null' model?
Flags: needinfo?(bzbarsky)
By not defining the property at all if the app doesn't have permissions, so that navigator.push will return undefined and |"push" in navigator| will return false.
Flags: needinfo?(bzbarsky)
Depends on: 902257
Created attachment 786636 [details] [diff] [review]
Add permission check to navigator.push load.

This fixes the checks that were removed from PushManager init().
Attachment #786636 - Flags: review?(bzbarsky)
Comment on attachment 786636 [details] [diff] [review]
Add permission check to navigator.push load.

Makes sense.  r=me
Attachment #786636 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/7583b9cff540
https://hg.mozilla.org/mozilla-central/rev/7583b9cff540
You need to log in before you can comment on or make changes to this bug.