Created attachment 576965 [details] [diff] [review]
Comment on attachment 576965 [details] [diff] [review]
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?)