Comment on attachment 748584 [details] [diff] [review] Patch (v1) Review of attachment 748584 [details] [diff] [review]: ----------------------------------------------------------------- JS::RootedObject and friends should not be used outside js/. Use JS::Rooted<JSObject*>, and don't forget to get review from a DOM peer.

(In reply to comment #1) > Comment on attachment 748584 [details] [diff] [review] > --> https://bugzilla.mozilla.org/attachment.cgi?id=748584 > Patch (v1) > > Review of attachment 748584 [details] [diff] [review]: > ----------------------------------------------------------------- > > JS::RootedObject and friends should not be used outside js/. Why? > Use JS::Rooted<JSObject*>, and don't forget to get review from a DOM peer. Hrm, OK. Till has been reviewing all my patches so far, but I guess I can ask bz too.

Comment on attachment 748584 [details] [diff] [review] Patch (v1) The JS folks keep telling us to not use JS::Rooted* and JS::Handle*... I think they want to remove them or something, not sure. So JS::Rooted<T> and JS::Handle<T> it is. >+ JS::RootedObject handler(aCx, jsl->GetHandler().Ptr()->Callable()); I suspect this can be: JS::Handle<JSObject*> handler(jsl->GetHandler().Ptr()->Callable()); if you change CallbackFunction::Callable to return a JS::Handle<JSObject*>. Which it should easily be able to do, since CallbackObject::Callback returns one. >+ JS::Rooted<JSObject*> value(cx, &v.get().toObject()); Does &v.toObject() not do the right thing? :( r=me with the nits fixed.

(In reply to Boris Zbarsky (:bz) from comment #3) > The JS folks keep telling us to not use JS::Rooted* and JS::Handle*... I > think they want to remove them or something, not sure. So JS::Rooted<T> and > JS::Handle<T> it is. Hmm, ok. Seems like the JS folks should align their opinions on this, then. ;) I'm sure we'll be able to do that during the next week.

> Seems like the JS folks should align their opinions on this, then <shrug>. I was told that had already happened. Have fun with it at the work week, in any case. ;)

Oh well, so maybe I'm just not up to speed on what's going on there. That is pretty likely, actually. I'll find out tomorrow, I guess. :)

FWIW I've been adding JS::Rooted* stuff relentlessly over the past couple of weeks. I hope that has not made people's lives harder. Also I do hope that JS::Rooted* stuff don't get removed. I think they are more beautiful than JS::Rooted<JSObject*> and the like. :-)

(In reply to Boris Zbarsky (:bz) from comment #3) > >+ JS::Rooted<JSObject*> value(cx, &v.get().toObject()); > > Does &v.toObject() not do the right thing? :( It actually does!

This fixes the build problem on nsGlobalWindow.cpp.

Comment on attachment 748894 [details] [diff] [review] Patch (v2) Review of attachment 748894 [details] [diff] [review]: ----------------------------------------------------------------- cool ::: content/events/src/nsEventListenerManager.cpp @@ +857,5 @@ > JS::CompileOptions options(cx); > options.setFileAndLine(url.get(), lineNo) > .setVersion(SCRIPTVERSION_DEFAULT); > > + JS::Rooted<JSObject*> rootedNull(cx, nullptr); // See bug 781070. You can use JS::NullPtr now. Here and below. ::: content/events/src/nsEventListenerService.cpp @@ +94,5 @@ > } > > nsCOMPtr<nsIJSEventListener> jsl = do_QueryInterface(mListener); > if (jsl) { > + JS::Handle<JSObject*> handler(jsl->GetHandler().Ptr()->Callable()); Callable() returns a handler, I suppose?

Forgot to remove the rootedNull local variable: http://hg.mozilla.org/integration/mozilla-inbound/rev/d0d2e13e8b83

And backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/9595c81d7591

So this patch broke the build like this: https://tbpl.mozilla.org/php/getParsedLog.php?id=22910497&full=1&branch=mozilla-inbound#error0 I suspect this is the thing that fails to compile (I cannot repro locally when building with either clang or gcc, yikes!): <http://hg.mozilla.org/mozilla-central/annotate/c80dc6ffe865/dom/base/nsGlobalWindow.cpp#l11797> Boris, any idea how I should fix up this code now that Callable returns a Handle<JSObject*>?

Perhaps replace this: vp->setObjectOrNull(h ? h->Callable() : nullptr); with: vp->setObjectOrNull(h ? h->Callable().get() : nullptr); ? You will want the same in nsINode.cpp, HTMLBodyElement.cpp, and HTMLFrameSetElement.cpp...

That does the trick. Thanks! https://hg.mozilla.org/integration/mozilla-inbound/rev/2c347b02cf44