Apparently we can do this as soon as all nodes are converted.

I can't wait :-) Is this something you're comfortable doing, Ms2ger? I'm happy to either write or review the patches. I'd like to separate these patches a little bit, so that we can bisect in case we run into any weird regressions. Here's the rough order I'm thinking of: 0 - Stop passing ALLOW_SLIM_WRAPPERS from precreate (or maybe this will be done already?) 1 - Stop creating slim wrappers in XPCWrappedNative.cpp. 2 - Stop morphing slim wrappers. 3 - Remove other miscellaneous checks for slim wrappers. 4 - Remove slim wrapper proto infrastructure. 5 - Remove IS_*_WRAPPER{_OBJECT}, and move all consumers to IS_WRAPPER_CLASS. 6 - Renamed the "multislot" to the "XrayExpandoSlot" or somesuch. Oh, and this should definitely land on top of bug 658909. ;-)

I actually have most of this done, I'll have to split it up into chunks but shouldn't be too hard. We should probably wait until all nodes are converted to WebIDL, which will be real soon now. (In reply to Bobby Holley (:bholley) from comment #1) > 5 - Remove IS_*_WRAPPER{_OBJECT}, and move all consumers to IS_WRAPPER_CLASS. So you don't want a variant of the macro that takes an object? Saves on typing. Also, I wonder if we should rename IS_WRAPPER_CLASS to IS_WN_WRAPPER_CLASS?

(In reply to Peter Van der Beken [:peterv] from comment #2) > I actually have most of this done, I'll have to split it up into chunks but > shouldn't be too hard. We should probably wait until all nodes are converted > to WebIDL, which will be real soon now. Awesome! > (In reply to Bobby Holley (:bholley) from comment #1) > > 5 - Remove IS_*_WRAPPER{_OBJECT}, and move all consumers to IS_WRAPPER_CLASS. > > So you don't want a variant of the macro that takes an object? Saves on > typing. Also, I wonder if we should rename IS_WRAPPER_CLASS to > IS_WN_WRAPPER_CLASS? That's fair. If we're renaming the macros anyway, what about: IS_WN_REFLECTOR(obj) IS_WN_CLASS(clasp) ?

Comment on attachment 761461 [details] [diff] [review] Stop morphing slim wrappers v1.1 Review of attachment 761461 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/nsNodeUtils.cpp @@ +515,5 @@ > if (elem) { > elem->RecompileScriptEventListeners(); > } > > + JS::RootedObject wrapper(cx); JS::Rooted<JSObject*> in content/, please @@ +516,5 @@ > elem->RecompileScriptEventListeners(); > } > > + JS::RootedObject wrapper(cx); > + if (cx && (wrapper = aNode->GetWrapper())) { If cx is null, we already crashed in the RootedObject constructor, no? ::: js/xpconnect/src/XPCWrappedNative.cpp @@ +727,1 @@ > NS_ADDREF(wrapper); I guess wrapper can't be null here? ::: js/xpconnect/src/nsXPConnect.cpp @@ +1223,5 @@ > > RootedObject aJSObj(aJSContext, aJSObjArg); > + aJSObj = js::CheckedUnwrap(aJSObj, /* stopAtOuter = */ false); > + if (aJSObj && IS_WN_WRAPPER(aJSObj)) { > + NS_IF_ADDREF(*_retval = XPCWrappedNative::Get(aJSObj)); This changes behaviour (throwing -> not throwing) if this returns null, right?

Comment on attachment 761466 [details] [diff] [review] Remove various slim wrapper checks and code v1 Review of attachment 761466 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/src/XPCConvert.cpp @@ +872,5 @@ > rv = XPCWrappedNative::GetNewOrUsed(aHelper, xpcscope, iface, > getter_AddRefs(strongWrapper)); > > wrapper = strongWrapper; > + } else { This should be in the previous patch ::: js/xpconnect/src/XPCJSRuntime.cpp @@ +2544,3 @@ > > // For pre-Paris DOM bindings objects, we only support Node. > if (nsCOMPtr<nsINode> node = do_QueryInterface(supports)) { We should remove this after form is done; want to file that?

Comment on attachment 761469 [details] [diff] [review] Remove slim wrapper proto code and logging code v1 Review of attachment 761469 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/src/XPCWrappedNative.cpp @@ +1357,5 @@ > // Cross-scope means cross-compartment. > MOZ_ASSERT(js::GetObjectCompartment(aOldScope->GetGlobalJSObject()) != > js::GetObjectCompartment(aNewScope->GetGlobalJSObject())); > NS_ASSERTION(aNewParent, "won't be able to find the new parent"); > NS_ASSERTION(wrapper, "can't transplant slim wrappers"); This message could use fixing ::: js/xpconnect/src/XPCWrappedNativeJSOps.cpp @@ +798,5 @@ > } \ > if (!IS_WRAPPER_CLASS(js::GetObjectClass(unwrapped))) { \ > return Throw(NS_ERROR_XPC_BAD_OP_ON_WN_PROTO, cx); \ > } \ > + wrapper = XPCWrappedNative::Get(unwrapped); \ Want to declare wrapper here? ::: js/xpconnect/src/nsXPConnect.cpp @@ -1945,5 @@ > return result; > } > } > } > - } else { The if can go too

Comment on attachment 761478 [details] [diff] [review] Remove slim wrapper macros and rename WN macros v1.1 Review of attachment 761478 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/src/XPCWrappedNative.cpp @@ +1501,5 @@ > if (!JS_SetParent(cx, flat, aNewParent)) > MOZ_CRASH(); > > JSObject *nw; > + if ((nw = wrapper->GetWrapper()) && Might as well do the assignment above the if ::: js/xpconnect/src/nsXPConnect.cpp @@ -1927,2 @@ > > - if (IS_WN_WRAPPER_OBJECT(obj)) { Ah, you're doing this here. Maybe do this in the patch where I mentioned it, though? ::: js/xpconnect/src/xpcpublic.h @@ +84,5 @@ > > +// If IS_WN_CLASS for the JSClass of an object is true, the object is a > +// wrappednative wrapper, holding the XPCWrappedNative in its private slot. > + > +static inline bool IS_WN_CLASS(js::Class* clazz) Is this a bad time to suggest renaming these functions to not use all-upper-case?

Comment on attachment 761461 [details] [diff] [review] Stop morphing slim wrappers v1.1 Review of attachment 761461 [details] [diff] [review]: ----------------------------------------------------------------- This was a satisfying patch to review. :-) r=bholley ::: content/base/src/nsNodeUtils.cpp @@ +516,5 @@ > elem->RecompileScriptEventListeners(); > } > > + JS::RootedObject wrapper(cx); > + if (cx && (wrapper = aNode->GetWrapper())) { I think you want to check aReparentScope here. ::: js/xpconnect/src/nsXPConnect.cpp @@ +1223,5 @@ > > RootedObject aJSObj(aJSContext, aJSObjArg); > + aJSObj = js::CheckedUnwrap(aJSObj, /* stopAtOuter = */ false); > + if (aJSObj && IS_WN_WRAPPER(aJSObj)) { > + NS_IF_ADDREF(*_retval = XPCWrappedNative::Get(aJSObj)); > This changes behaviour (throwing -> not throwing) if this returns null, right? I don't follow Ms2ger's comment here.

(In reply to Bobby Holley (:bholley) from comment #21) > Comment on attachment 761461 [details] [diff] [review] > Stop morphing slim wrappers v1.1 > > Review of attachment 761461 [details] [diff] [review]: > ----------------------------------------------------------------- > > This was a satisfying patch to review. :-) r=bholley No kidding. > ::: js/xpconnect/src/nsXPConnect.cpp > @@ +1223,5 @@ > > > > RootedObject aJSObj(aJSContext, aJSObjArg); > > + aJSObj = js::CheckedUnwrap(aJSObj, /* stopAtOuter = */ false); > > + if (aJSObj && IS_WN_WRAPPER(aJSObj)) { > > + NS_IF_ADDREF(*_retval = XPCWrappedNative::Get(aJSObj)); > > > This changes behaviour (throwing -> not throwing) if this returns null, right? > > I don't follow Ms2ger's comment here. Let me try again. What we had before was nsIXPConnectWrappedNative* wrapper = XPCWrappedNative::GetAndMorphWrappedNativeOfJSObject(aJSContext, aJSObj); if (wrapper) { ... return NS_OK; } ... return NS_ERROR_FAILURE; with GetAndMorphWrappedNativeOfJSObject returning null if aJSObj && IS_WN_WRAPPER(aJSObj) && !XPCWrappedNative::Get(aJSObj). (I guess XPCWrappedNative::Get(aJSObj) really shouldn't be null, though.)

(In reply to :Ms2ger from comment #22) > Let me try again. What we had before was > > nsIXPConnectWrappedNative* wrapper = > XPCWrappedNative::GetAndMorphWrappedNativeOfJSObject(aJSContext, aJSObj); > if (wrapper) { > ... > return NS_OK; > } > ... > return NS_ERROR_FAILURE; > > with GetAndMorphWrappedNativeOfJSObject returning null if aJSObj && > IS_WN_WRAPPER(aJSObj) && !XPCWrappedNative::Get(aJSObj). (I guess > XPCWrappedNative::Get(aJSObj) really shouldn't be null, though.) Oh I see - you're just talking about XPCWrappedNative::Get? I don't think we have to worry about that.

Comment on attachment 761466 [details] [diff] [review] Remove various slim wrapper checks and code v1 Review of attachment 761466 [details] [diff] [review]: ----------------------------------------------------------------- r=bholley with comments addressed ::: dom/bindings/BindingUtils.h @@ +635,5 @@ > return true; > } > > *vp = JS::ObjectValue(*obj); > + return sameCompartment || JS_WrapValue(cx, vp); This is not equivalent. I think you just want |return JS_WrapValue(cx, vp)| here, unless the old code was wrong. ::: js/xpconnect/src/XPCConvert.cpp @@ +872,5 @@ > rv = XPCWrappedNative::GetNewOrUsed(aHelper, xpcscope, iface, > getter_AddRefs(strongWrapper)); > > wrapper = strongWrapper; > + } else { Can we assert IS_WN_WRAPPER_OBJECT here? ::: js/xpconnect/src/XPCQuickStubs.cpp @@ +564,5 @@ > *tearoff = (XPCWrappedNativeTearOff*) js::GetObjectPrivate(obj); > obj = js::GetObjectParent(obj); > } > > // If we've got a WN or slim wrapper, store things the way callers expect. Comment needs updating. @@ +567,5 @@ > > // If we've got a WN or slim wrapper, store things the way callers expect. > // Otherwise, leave things null and return. > + if (IS_WRAPPER_CLASS(clasp)) > + *wrapper = (XPCWrappedNative*) js::GetObjectPrivate(obj); Might as well switch to XPCWrappedNative::Get here.

Comment on attachment 761478 [details] [diff] [review] Remove slim wrapper macros and rename WN macros v1.1 Review of attachment 761478 [details] [diff] [review]: ----------------------------------------------------------------- r=bholley ::: js/xpconnect/src/xpcpublic.h @@ +97,5 @@ > #define WRAPPER_MULTISLOT 0 > > +// Use these functions if IS_WN_CLASS(GetObjectClass(obj)) might be false. > +// Avoid calling them if IS_WN_CLASS(GetObjectClass(obj)) can only be true, as > +// we'd do a redundant call to IS_WN_CLASS. This comment doesn't make sense anymore. These checks are now identical modulo the argument type.

Comment on attachment 761481 [details] [diff] [review] Rename WRAPPER_MULTISLOT v1 Review of attachment 761481 [details] [diff] [review]: ----------------------------------------------------------------- r=bholley Thanks for doing this, Peter, and doublethanks for splitting the patches so nicely. It made them very easy and enjoyable to review. :-) ::: js/xpconnect/src/XPCWrappedNative.cpp @@ +1058,5 @@ > AutoJSContext cx; > > + // For all WNs, we want to make sure that the expando chain slot starts out > + // as null. > + JS_SetReservedSlot(mFlatJSObject, WN_XRAYEXPANDOCHAIN_SLOT, JSVAL_NULL); I think this constraint could be relaxed now, given that we no longer need to differentiate between undefined and null. But that would probably require changing some code in XrayWrapper, and it isn't really important. Don't worry about it.

(In reply to :Ms2ger from comment #17) > JS::Rooted<JSObject*> in content/, please Done. > If cx is null, we already crashed in the RootedObject constructor, no? Yeah. > I guess wrapper can't be null here? Not more that it could in the past. > This changes behaviour (throwing -> not throwing) if this returns null, > right? Yeah, but I don't think we'll hit that. > This should be in the previous patch I'm not going to move things around anymore, unless something is explicitly broken. > This message could use fixing Removed the assertion, it became meaningless anyway. > Want to declare wrapper here? Done. > Might as well do the assignment above the if Done. > Ah, you're doing this here. Maybe do this in the patch where I mentioned it, > though? No, see above. > Is this a bad time to suggest renaming these functions to not use > all-upper-case? Yeah, good idea. Unfortunately I forgot before pushing :-(. > > + JS::RootedObject wrapper(cx); > > + if (cx && (wrapper = aNode->GetWrapper())) { > > I think you want to check aReparentScope here. Yes, done. > This is not equivalent. I think you just want |return JS_WrapValue(cx, vp)| > here, unless the old code was wrong. Good catch! > Can we assert IS_WN_WRAPPER_OBJECT here? Done. > Comment needs updating. Done. > Might as well switch to XPCWrappedNative::Get here. Done. > This comment doesn't make sense anymore. These checks are now identical > modulo the argument type. Removed the comment. > I think this constraint could be relaxed now, given that we no longer need > to differentiate between undefined and null. Well, it would mostly mean changing Get/SetWNExpandoChain, right? And using ObjectOrNullValue and toObjectOrNull is pretty nice there. https://hg.mozilla.org/integration/mozilla-inbound/rev/fb09320a1b30 https://hg.mozilla.org/integration/mozilla-inbound/rev/1b34a34b2538 https://hg.mozilla.org/integration/mozilla-inbound/rev/1c5330960d0c https://hg.mozilla.org/integration/mozilla-inbound/rev/dda67b97f819 https://hg.mozilla.org/integration/mozilla-inbound/rev/90634822b94d https://hg.mozilla.org/integration/mozilla-inbound/rev/aed026a38816 https://hg.mozilla.org/integration/mozilla-inbound/rev/d1b5389de54e

https://hg.mozilla.org/mozilla-central/rev/fb09320a1b30 https://hg.mozilla.org/mozilla-central/rev/1b34a34b2538 https://hg.mozilla.org/mozilla-central/rev/1c5330960d0c https://hg.mozilla.org/mozilla-central/rev/dda67b97f819 https://hg.mozilla.org/mozilla-central/rev/90634822b94d https://hg.mozilla.org/mozilla-central/rev/aed026a38816 https://hg.mozilla.org/mozilla-central/rev/d1b5389de54e

\o/