Last Comment Bug 641552 - Provide hook for adding APIs to window.navigator
: Provide hook for adding APIs to window.navigator
Status: RESOLVED FIXED
[QA?]
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: :Ehsan Akhgari
:
:
Mentors:
: 649039 652610 (view as bug list)
Depends on:
Blocks: 609043 649038
  Show dependency treegraph
 
Reported: 2011-03-14 10:30 PDT by Philipp von Weitershausen [:philikon]
Modified: 2011-12-06 13:00 PST (History)
25 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
9+


Attachments
Fix. (17.39 KB, patch)
2011-08-10 14:23 PDT, Johnny Stenback (:jst, jst@mozilla.com)
peterv: review+
Details | Diff | Splinter Review
fill the hash to allow use of the new category in component manifests (549 bytes, patch)
2011-08-24 09:41 PDT, [:fabrice] Fabrice Desré
jst: review+
Details | Diff | Splinter Review
Fix with comments addressed (25.89 KB, patch)
2011-09-06 13:02 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review

Description Philipp von Weitershausen [:philikon] 2011-03-14 10:30:42 PDT
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] 2011-03-14 10:33:17 PDT
This is a dup of some old bug, I think.
IIRC some 3xxxxx bug.
Comment 2 Olli Pettay [:smaug] 2011-03-14 10:33:46 PDT
Timeless might remember
Comment 3 Philipp von Weitershausen [:philikon] 2011-03-14 10:40:00 PDT
(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) 2011-03-14 11:07:35 PDT
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 2011-03-14 12:16:11 PDT
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 2011-03-15 03:50:33 PDT
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 7 Shane Caraveo (:mixedpuppy) 2011-04-11 10:11:25 PDT
*** Bug 649039 has been marked as a duplicate of this bug. ***
Comment 8 Shane Caraveo (:mixedpuppy) 2011-04-11 10:28:00 PDT
(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] 2011-04-12 11:18:38 PDT
bug 622500 is looking pretty similar to this
Comment 10 Johnny Stenback (:jst, jst@mozilla.com) 2011-07-28 17:11:33 PDT
*** Bug 652610 has been marked as a duplicate of this bug. ***
Comment 11 Philipp von Weitershausen [:philikon] 2011-07-28 17:15:56 PDT
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 2011-07-28 17:18:52 PDT
Sounds great to me.
Comment 13 David Dahl :ddahl 2011-07-28 18:04:26 PDT
(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] 2011-07-28 18:12:41 PDT
(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 2011-07-28 18:56:10 PDT
(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, jst@mozilla.com) 2011-07-29 17:01:07 PDT
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] 2011-07-29 19:15:33 PDT
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, jst@mozilla.com) 2011-08-10 14:23:33 PDT
Created attachment 552216 [details] [diff] [review]
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é 2011-08-10 15:22:05 PDT
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, jst@mozilla.com) 2011-08-10 15:23:12 PDT
Yes, it does!
Comment 21 [:fabrice] Fabrice Desré 2011-08-24 09:41:41 PDT
Created attachment 555436 [details] [diff] [review]
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] 2011-08-25 13:26:40 PDT
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 :Ehsan Akhgari 2011-09-06 13:02:25 PDT
Created attachment 558562 [details] [diff] [review]
Fix with comments addressed
Comment 24 :Ehsan Akhgari 2011-09-06 13:03:28 PDT
This is now waiting for jst to review the other patch.
Comment 25 Johnny Stenback (:jst, jst@mozilla.com) 2011-09-06 22:55:06 PDT
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, jst@mozilla.com) 2011-09-06 22:56:18 PDT
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] 2011-12-06 13:00:22 PST
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

Note You need to log in before you can comment on or make changes to this bug.