Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Description • 11 years ago

Once JS-implemented interfaces can be described in WebIDL (is this already possible?) switch Push to use it.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 1 • 11 years ago

Yes, that's possible. See RTCPeerConnection, for example.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 3 • 11 years ago

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 ^

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 4 • 11 years ago

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?

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 5 • 11 years ago

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

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 6 • 11 years ago

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?

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 7 • 11 years ago

(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

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 8 • 11 years ago

Try 2:

https://hg.mozilla.org/integration/mozilla-inbound/rev/769c7e06a1f0
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1a2caba50bf

	Boris Zbarsky [:bzbarsky]
	Comment 9 • 11 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 10 • 11 years ago

Oh, and please add some tests!

	Boris Zbarsky [:bzbarsky]
	Comment 11 • 11 years ago

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.  :(

	Andrew McCreight [:mccr8]
	Comment 12 • 11 years ago

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.

	Carsten Book [:Tomcat]
	Comment 13 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/769c7e06a1f0
https://hg.mozilla.org/mozilla-central/rev/b1a2caba50bf

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 14 • 11 years ago

(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?

	Boris Zbarsky [:bzbarsky]
	Comment 15 • 11 years ago

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.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 16 • 11 years ago

This fixes the checks that were removed from PushManager init().

	Boris Zbarsky [:bzbarsky]
	Comment 17 • 11 years ago

Comment on attachment 786636 [details] [diff] [review]
Add permission check to navigator.push load.

Makes sense.  r=me

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 18 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/7583b9cff540

	Carsten Book [:Tomcat]
	Comment 19 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/7583b9cff540