Comment on attachment 726777 [details] [diff] [review] Patch v1 Review of attachment 726777 [details] [diff] [review]: ----------------------------------------------------------------- This doubtless broke horribly in the Great Handle-ification Of All Things Proxy And Wrapper. The bustage is probably extensive but not too difficult to work through. As always, if you judge that a re-review is needed for an updated version, feel free to request it. ::: js/public/CallArgs.h @@ +241,5 @@ > return argv_[i]; > } > > /* Returns a mutable handle for the i-th zero-indexed argument. */ > + MutableHandleValue handleAt(unsigned i) const { Hmm, this was a MutableHandleValue? Maybe, I guess. I think it'd be better for arguments not to be implicit mutable roots, and have people use Rooted rather than rely on hairy rooting that only can work if enough arguments were provided, but I guess that's for another bug. ::: js/src/jswrapper.cpp @@ +496,3 @@ > if (!cx->compartment->wrap(cx, &arg)) > return false; > + args[n] = arg; If we're going to say the args are mutable, and actually use that, right now, just pass &arg[n] and get rid of the RootedValue and reassignment. ::: js/xpconnect/wrappers/XrayWrapper.cpp @@ +1220,2 @@ > return false; > + args.setThis(thisv); Ugh. setThis(), per comments, is dodgy and mostly shouldn't be used. So this will have to be a temporary hackaround, that'll get fixed up later to something more desirable, that likely doesn't exist just yet. @@ +1220,3 @@ > return false; > + args.setThis(thisv); > + for (size_t n = 0; n < args.length(); ++n) { Dunno why s/i/n/, but whatever.

It wasn't just evilpie who bitrotted me, but bholley did too, in a more annoying way.

Comment on attachment 728642 [details] [diff] [review] Patch v2 Review of attachment 728642 [details] [diff] [review]: ----------------------------------------------------------------- Possibly I should look at the computeThis change again. ::: js/public/CallArgs.h @@ +141,5 @@ > // MOZ_ASSERT(!argv_[-1].isMagic(JS_IS_CONSTRUCTING)); > return HandleValue::fromMarkedLocation(&argv_[-1]); > } > > + Value computeThis(JSContext *cx) const { jorendorff and I discussed computeThis behavior wrt CallArgs recently, and we concluded that we shouldn't expose this in CallArgs -- callers should handle the null/undefined case explicitly. (This really only matters to methods that might be called on the global object, without qualification. That's basically only methods on Window, which bz says can be code-genned accordingly, and some of the oddballs on Object.prototype, like __{define,lookup}{G,S}etter__. And moderately-generic cases like this one.) Let's go with this for now, to keep moving forward. But I'd like a followup to remove this method, and replace the one use of it here with a JS::ToObject method (that acts like the spec -- note this is different from JS_ValueToObject; yay non-spec names and non-spec algorithms). ::: js/src/jsproxy.cpp @@ +2116,3 @@ > }; > RootedValue thisValue(cx, ObjectValue(*handler)); > + return Invoke(cx, thisValue, trap, 3, argv, args.rval().address()); Make that ArrayLength(argv) while you're here. @@ +2153,3 @@ > }; > RootedValue thisValue(cx, ObjectValue(*handler)); > + return Invoke(cx, thisValue, trap, 2, constructArgv, args.rval().address()); ArrayLength(constructArgv) ::: js/xpconnect/wrappers/XrayWrapper.cpp @@ +1113,5 @@ > // Run the resolve hook of the wrapped native. > XPCWrappedNative *wn = getWN(wrapper); > if (NATIVE_HAS_FLAG(wn, WantCall)) { > + XPCCallContext ccx(JS_CALLER, cx, wrapper, nullptr, JSID_VOID, args.length(), args.array(), > + args.rval().address()); Hmm. XPCCallContext may want to take const CallArgs& too. Followup-land. @@ +1118,5 @@ > if (!ccx.IsValid()) > return false; > bool ok = true; > + nsresult rv = wn->GetScriptableInfo()->GetCallback()->Call( > + wn, cx, wrapper, args.length(), args.array(), args.rval().address(), &ok); And this Call hook as well.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3) > Comment on attachment 728642 [details] [diff] [review] > Patch v2 > > Review of attachment 728642 [details] [diff] [review]: > ----------------------------------------------------------------- > > Possibly I should look at the computeThis change again. > > ::: js/public/CallArgs.h > @@ +141,5 @@ > > // MOZ_ASSERT(!argv_[-1].isMagic(JS_IS_CONSTRUCTING)); > > return HandleValue::fromMarkedLocation(&argv_[-1]); > > } > > > > + Value computeThis(JSContext *cx) const { > > jorendorff and I discussed computeThis behavior wrt CallArgs recently, and > we concluded that we shouldn't expose this in CallArgs -- callers should > handle the null/undefined case explicitly. (This really only matters to > methods that might be called on the global object, without qualification. > That's basically only methods on Window, which bz says can be code-genned > accordingly, and some of the oddballs on Object.prototype, like > __{define,lookup}{G,S}etter__. And moderately-generic cases like this one.) > > Let's go with this for now, to keep moving forward. But I'd like a followup > to remove this method, and replace the one use of it here with a > JS::ToObject method (that acts like the spec -- note this is different from > JS_ValueToObject; yay non-spec names and non-spec algorithms). Will do. > ::: js/xpconnect/wrappers/XrayWrapper.cpp > @@ +1113,5 @@ > > // Run the resolve hook of the wrapped native. > > XPCWrappedNative *wn = getWN(wrapper); > > if (NATIVE_HAS_FLAG(wn, WantCall)) { > > + XPCCallContext ccx(JS_CALLER, cx, wrapper, nullptr, JSID_VOID, args.length(), args.array(), > > + args.rval().address()); > > Hmm. XPCCallContext may want to take const CallArgs& too. Followup-land. Got a patch for that. > @@ +1118,5 @@ > > if (!ccx.IsValid()) > > return false; > > bool ok = true; > > + nsresult rv = wn->GetScriptableInfo()->GetCallback()->Call( > > + wn, cx, wrapper, args.length(), args.array(), args.rval().address(), &ok); > > And this Call hook as well. But I hope we can remove all these hooks in a couple of months, so I'm going to let it die as ugly as it is.

Filed bug 859445 about comment 3.