Created attachment 576965 [details] [diff] [review] Patch v1
Comment on attachment 576965 [details] [diff] [review] Patch v1 I'm really sorry for totally failing on my promise of speedy reviews. :-( Thanks for doing this stuff! >- nsISupports createInstance(); >- nsISupports getService(); >+ [implicit_jscontext,optional_argc] jsval createInstance([optional] in jsval iid); >+ [implicit_jscontext,optional_argc] jsval getService([optional] in jsval iid); It's good that you're using jsvals here, because I just wrote a patch that makes XPConnect throw on null IIDs: bug 705875. ;-) >- rv = xpc->WrapNativeToJSVal(cx, obj, inst, nsnull, iid, true, vp, nsnull); >- if (NS_FAILED(rv) || JSVAL_IS_PRIMITIVE(*vp)) >+ rv = nsXPConnect::GetXPConnect()->WrapNativeToJSVal(aCx, obj, inst, nsnull, iid, true, aRetval, nsnull); >+ if (NS_FAILED(rv) || JSVAL_IS_PRIMITIVE(*aRetval)) Can nsXPConnect::GetXPConnect() ever return null? We check for it before, and with this patch we don't. We should either keep checking or be very sure that it's safe. Same thing in getInterface(). r=bholley once that's figured out.
So, I realized that as much as I like the aFoo naming convention, it isn't consistent with js-style, and XPConnect is supposed to be js-style. So r+ contingent on changing that as well. :-(
nsXPConnect::GetXPConnect() certainly can't be null at that point; we would already have returned when the helper failed. (I doubt it can be null otherwise; maybe on shutdown?) https://hg.mozilla.org/mozilla-central/rev/f7c8894bbdae