641552 - Provide hook for adding APIs to window.navigator

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Philipp von Weitershausen [:philikon]]

With nsIDOMGlobalPropertyInitializer and the JavaScript-global-property category it's pretty easy to add additional properties to DOM windows, but not to the 'navigator' object. Several upcoming projects add new APIs to 'navigator' and currently facilitate the 'injector.js' hack [1]. These are:

* Open Web Apps (Labs)
* F1 (MoMo/Services)
* Push Notifications (Services)
* Identity (Services)

All of these have a roadmap of their own to become part of the product. Before they do, we should figure out how to make the 'navigator' object pluggable for new APIs. One way would be to go the same route as with the 'window' object. Another way would be to special case Open Web Apps and have all other services expose themselves as a Web App through its API.

[1] https://github.com/mozilla/f1/blob/develop/extensions/firefox-share/src/modules/injector.js

Comment 1

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

This is a dup of some old bug, I think.
IIRC some 3xxxxx bug.

Comment 2

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

Timeless might remember

Comment 3

[Philipp von Weitershausen [:philikon]]

(In reply to comment #1)
> This is a dup of some old bug, I think.

Maybe. If we decide to make 'navigator' pluggable. If we decide to just add an API for Open Web Apps and route everything else through that (provided that's feasible), this bug should probably morphed into doing that.

Comment 4

[Shane Caraveo (:mixedpuppy)]

I think there are use cases where routing through webapps may not the right thing to do.  In any case, the webapps use of the navigator object needs to settle.

With f1, I need to support a share api off the navigator object from the addon.  I think this is one area where going through webapps doesn't make sense.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't understand the webapps discussion. I.e. the last sentence of comment 0, and all of comment 4.

Besides that, I think this sounds like a obvious good idea.

Comment 6

[timeless]

my guess is that the bug referenced useragent because the easiest driver was site specific useragents. i'm too burnt out to seek it.

Comment 8

[Shane Caraveo (:mixedpuppy)]

(In reply to comment #5)
> I don't understand the webapps discussion. I.e. the last sentence of comment 0,
> and all of comment 4.

OpenWebApps is also using injector.js, with modifications to inject api's based on the manifest file for the webapp.  We're talking about whether or not injection onto the navigator object should only happen via openwebapps.  

I hope that helps.

Comment 9

[Bryan Clark (DevTools PM) [:clarkbw]]

bug 622500 is looking pretty similar to this

Comment 11

[Philipp von Weitershausen [:philikon]]

So, my proposal is to create a JS XPCOM component that (like Labs's injector.js code) observes 'content-document-global-created' and then queries the components manager for components registered for the 'JavaScript-navigator-property' category, adding them to the navigator object. So to consumers this would work exactly the same as adding stuff to the 'window' object.

Thoughts?

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Sounds great to me.

Comment 13

[David Dahl :ddahl]

(In reply to comment #11)
> So, my proposal is to create a JS XPCOM component that (like Labs's
> injector.js code) observes 'content-document-global-created' and then
> queries the components manager for components registered for the
> 'JavaScript-navigator-property' category, adding them to the navigator
> object. So to consumers this would work exactly the same as adding stuff to
> the 'window' object.
> 
> Thoughts?

I have had wrapper issues in mochitests doing this in a pure JS way. I could not access any properties I added to navigator from chrome priv. code in a browser chrome test. Maybe it was my code doing something stupid, but this method will not add the new plugged properties to the navigator interface.

Comment 14

[Philipp von Weitershausen [:philikon]]

(In reply to comment #13)
> I have had wrapper issues in mochitests doing this in a pure JS way. I could
> not access any properties I added to navigator from chrome priv. code in a
> browser chrome test. Maybe it was my code doing something stupid, but this
> method will not add the new plugged properties to the navigator interface.

I think using win.navigator.wrappedJSObject would fix this?

Comment 15

[David Dahl :ddahl]

(In reply to comment #14)
> (In reply to comment #13)
> > I have had wrapper issues in mochitests doing this in a pure JS way. I could
> > not access any properties I added to navigator from chrome priv. code in a
> > browser chrome test. Maybe it was my code doing something stupid, but this
> > method will not add the new plugged properties to the navigator interface.
> 
> I think using win.navigator.wrappedJSObject would fix this?

That was the problem, it was not fixing it. I will put together a test.

Comment 16

[Johnny Stenback (:jst)]

Phil, I think we can do this fairly easily in native code as well, and if we do that we can trivially make it so that we do *no* extra work for pages that don't actually use any of these features, which I feel is somewhat important. IMO we need to at least make it so that instantiation of the components that expose the various APIs happens dynamically, and while that's possible by injecting getters etc onto the navigator object it seems like that could be a fair bit of code compared to doing this in C++ like we do for global properties.

Comment 17

[Philipp von Weitershausen [:philikon]]

Thanks, Johnny. That's fine. I just wanted to be constructive and offer a solution. If you guys want to go a different route that turns out to be more efficient, even better. If nobody from your team is able to do it, I'd be willing to tackle this with a bit of shepherding. This is blocking new stuff slated for eventual core landings (Open Web Apps to name one), so it'd be nice to get this thing started.

Comment 18

[Johnny Stenback (:jst)]

Attachment: Fix.

This got a bit nasty, in that the namespace manager stuff was never set up to deal with anything other than global names, so this feels pretty shoehorned, but I think a bunch of this will change around when we do new bindings for the DOM, so I'm not sure it's worth cleaning this up too much right now.

This patch works, passes mochitest, and even includes its own test! Please give this a spin and let me know if any issues are found...

Comment 19

[[:fabrice] Fabrice Desré]

Very nice! I'd like to use this for bug 609043.

Does it also calls init() if the object implements nsIDOMGlobalPropertyInitializer (http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMClassInfo.cpp#6631) ?

Comment 20

[Johnny Stenback (:jst)]

Yes, it does!

Comment 21

[[:fabrice] Fabrice Desré]

Attachment: fill the hash to allow use of the new category in component manifests

Small additional patch to be able to not only dynamically register components withe the component manager but also declare them as navigator properties in components manifests.

Comment 22

[Peter Van der Beken [:peterv]]

Comment on attachment 552216 [details] [diff] [review]
Fix.

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

::: dom/base/nsDOMClassInfo.cpp
@@ +7169,5 @@
> +
> +  nameSpaceManager->LookupNavigatorName(name, &name_struct);
> +
> +  if (!name_struct ||
> +      name_struct->mType != nsGlobalNameStruct::eTypeNavigatorProperty) {

Just assert that the type is nsGlobalNameStruct::eTypeNavigatorProperty, anything else would be really bogus, no?

@@ +7180,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  jsval prop_val = JSVAL_VOID; // Property value.
> +
> +  nsCOMPtr<nsIXPConnectJSObjectHolder> holder;

I'd declare the holder where you use it.

@@ +7190,5 @@
> +    nsCOMPtr<nsIXPConnectWrappedNative> global_wrapper;
> +    nsContentUtils::XPConnect()->
> +      GetWrappedNativeOfJSObject(cx, global, getter_AddRefs(global_wrapper));
> +
> +    nsCOMPtr<nsIDOMWindow> window(do_QueryWrappedNative(global_wrapper));

I think you can do

  nsISupports *globalNative = XPConnect()->GetNativeOfWrapper(cx, global);
  nsCOMPtr<nsIDOMWindow> window = do_QueryInterface(globalNative);

::: dom/tests/mochitest/bugs/test_bug641552.html
@@ +30,5 @@
> +
> +SimpleTest.executeSoon(function () {
> +  ok(window.randomname, "window.randomname should return an object");
> +  ok(window.navigator.randomname1, "navigator.randomname1 should return an object");
> +  ok(window.navigator.randomname2, "navigator.randomname2 should return an object");

Should we check that the properties are actually objects?

Comment 23

[(no longer active)]

Attachment: Fix with comments addressed

Comment 24

[(no longer active)]

This is now waiting for jst to review the other patch.

Comment 25

[Johnny Stenback (:jst)]

Comment on attachment 555436 [details] [diff] [review]
fill the hash to allow use of the new category in component manifests

r=jst

Comment 26

[Johnny Stenback (:jst)]

Thanks Ehsan for updating the patch!

http://hg.mozilla.org/mozilla-central/rev/09935ede3c77
http://hg.mozilla.org/mozilla-central/rev/7fbe71f2b56e

Comment 27

[Eric Shepherd [:sheppy]]

This is mentioned on Firefox 9 for developers, and is documented here:

https://developer.mozilla.org/En/Developer_Guide/Adding_APIs_to_the_navigator_object