Closed Bug 641552 Opened 14 years ago Closed 13 years ago

Provide hook for adding APIs to window.navigator

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla9
Tracking Status
firefox9 --- fixed
fennec 9+ ---

People

(Reporter: philikon, Assigned: ehsan.akhgari)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [QA?])

Attachments

(2 files, 1 obsolete file)

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
This is a dup of some old bug, I think. IIRC some 3xxxxx bug.
Timeless might remember
(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.
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.
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.
my guess is that the bug referenced useragent because the easiest driver was site specific useragents. i'm too burnt out to seek it.
Blocks: 649038
(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.
bug 622500 is looking pretty similar to this
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?
(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.
(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?
(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.
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.
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.
Attached patch Fix. (obsolete) — Splinter Review
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...
Attachment #552216 - Flags: review?(peterv)
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) ?
Yes, it does!
Blocks: 609043
tracking-fennec: --- → 9+
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.
Attachment #555436 - Flags: review?(jst)
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?
Attachment #552216 - Flags: review?(peterv) → review+
Assignee: nobody → ehsan
Attachment #552216 - Attachment is obsolete: true
This is now waiting for jst to review the other patch.
Comment on attachment 555436 [details] [diff] [review] fill the hash to allow use of the new category in component manifests r=jst
Attachment #555436 - Flags: review?(jst) → review+
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Keywords: dev-doc-needed
Target Milestone: --- → mozilla9
Version: unspecified → Trunk
Whiteboard: [QA?]
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
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: