1270977 - Remove JS::CallReceiver

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

There's not really much reason to have a version of CallArgs that just doesn't expose arguments -- might as well have them use CallArgs.  (There are one or two places that can't use CallArgs, to be sure -- but they're pretty dodgy themselves and don't justify keeping this extra abstraction around.)

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Rejigger ThisToStringForStringProto funkiness

ThisToStringForStringProto used to overwrite |this| because it returned a raw JSString* that not all callers rooted.  We're long past the point where anyone does this, and we can just pass the usual handle and return a JSString* the caller must root.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Switch various other CallReceiver users to CallArgs

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Remove the BoxNonStrictThis overload taking const CallReceiver&

If we killed off JS_THIS and JS_THIS_OBJECT, we could eliminate the JS::detail::ComputeThis nonsense still remaining.  Out of scope for this bug, although we should do it soon enough.  Only a handful of functions on |window| want [LenientThis] that does what JS_THIS_OBJECT does.

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Remove JS::CallReceiver

This is a bit of a mess to read, unfortunately.  :-(  Not much that can be done about it.

Comment 5

[Eric Faust [:efaust]]

Comment on attachment 8749859 [details] [diff] [review]
Rejigger ThisToStringForStringProto funkiness

Review of attachment 8749859 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsstr.cpp
@@ +469,2 @@
>          }
> +    } else if (thisv.isNullOrUndefined()) {

blech. I wish we had a RequireObjectCoercible helper, instead of open-coding this her and in ToObjectFromStack and so on.

Comment 6

[Eric Faust [:efaust]]

Comment on attachment 8749860 [details] [diff] [review]
Switch various other CallReceiver users to CallArgs

Review of attachment 8749860 [details] [diff] [review]:
-----------------------------------------------------------------

APPROVED.

Comment 7

[Eric Faust [:efaust]]

Comment on attachment 8749862 [details] [diff] [review]
Remove the BoxNonStrictThis overload taking const CallReceiver&

Review of attachment 8749862 [details] [diff] [review]:
-----------------------------------------------------------------

I guess we didn't need to move JS_CreateThis to namespace JS, but it makes sense.

::: js/public/CallArgs.h
@@ +75,5 @@
> + * interface, and we're unlikely to add one (because this style of function is
> + * disfavored -- functions shouldn't be implicitly exposing the global object
> + * to arbitrary callers).  You should probably continue using |vp| directly for
> + * this case, but be aware this API will eventually be replaced with a function
> + * that will operate directly upon |args.thisv()|.

how does this , "but beware..." clase help readers structure their code differently? Perhaps continue the sentiment from the previous comment: "Perhaps use that instead", or something.

@@ +385,2 @@
>  MOZ_ALWAYS_INLINE JS::Value
>  JS_THIS(JSContext* cx, JS::Value* vp)

JS_THIS DELENDA EST. Everyone should use a CallArgs getter.

::: js/src/vm/Interpreter.cpp
@@ +91,5 @@
>  
>      if (thisv.isNullOrUndefined()) {
> +        // In embeddings with multiple realms in play, this is the realm of the
> +        // current function, presumed to be correct because otherwise there'd
> +        // be a compartment mismatch.

I think this comment does more harm than good. What other realm could we use for a scripted function? From a spec point of view, there are no other plausible choices.

Comment 8

[Eric Faust [:efaust]]

Comment on attachment 8749867 [details] [diff] [review]
Remove JS::CallReceiver

Review of attachment 8749867 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the below addressed. If it's preexisting, I'd be interested to understand how it works at all, at the moment. I wouldn't be surprised to learn it's just that people query isConstructing() at the beginning when usedRval_ is always false, and then the code is just dead in practice.

::: js/public/CallArgs.h
@@ +157,5 @@
> +    // CALLING/CONSTRUCTING-DIFFERENTIATIONS
> +
> +    bool isConstructing() const {
> +#ifdef JS_DEBUG
> +        if (this->usedRval_)

Wait, what? I know this is a copy/paste, but isn't this code senseless?

if (this->usedRval_), then calleev() should assert....

at any rate, it should clearly be |if (!this->usedRval_)|

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/daf0dcb26912
https://hg.mozilla.org/integration/mozilla-inbound/rev/870bdb6e2b8f
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4938a38f3c0
https://hg.mozilla.org/integration/mozilla-inbound/rev/5482d7b5c18f

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b086d922b1a9

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/daf0dcb26912
https://hg.mozilla.org/mozilla-central/rev/870bdb6e2b8f
https://hg.mozilla.org/mozilla-central/rev/b4938a38f3c0
https://hg.mozilla.org/mozilla-central/rev/5482d7b5c18f
https://hg.mozilla.org/mozilla-central/rev/b086d922b1a9