Replace IsDelegate with IsDelegateOfObject

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P3
normal
RESOLVED FIXED
Last year
11 months ago

People

(Reporter: anba, Assigned: khyperia)

Tracking

Trunk
mozilla63
Points:
---

Firefox Tracking Flags

(firefox61 wontfix, firefox62 wontfix, firefox63 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

We have |js::IsDelegate(JSContext*, HandleObject, const js::Value&, bool*)| and |js::IsDelegateOfObject(JSContext*, HandleObject, JSObject*, bool*)| [1], but |IsDelegate(...)| is always called with an Object value [2,3], which means both callers could directly call |IsDelegateOfObject(...)| and |IsDelegate(...)| can be removed.

As a follow-up we could then consider if makes sense to rename IsDelegateOfObject to IsDelegate.


[1] https://searchfox.org/mozilla-central/rev/7e663b9fa578d425684ce2560e5fa2464f504b34/js/src/vm/JSObject.cpp#3180,3190
[2] https://searchfox.org/mozilla-central/rev/7e663b9fa578d425684ce2560e5fa2464f504b34/js/src/builtin/Object.cpp#942
[3] https://searchfox.org/mozilla-central/rev/7e663b9fa578d425684ce2560e5fa2464f504b34/js/src/vm/JSFunction.cpp#775
Priority: -- → P3
(In reply to André Bargull [:anba] from comment #0)
> As a follow-up we could then consider if makes sense to rename
> IsDelegateOfObject to IsDelegate.

I'd suggest IsPrototypeOf or HasOnProtoChain...
Assignee: nobody → khyperia
Status: NEW → ASSIGNED
Attachment #9003556 - Flags: review?(jdemooij)
Comment on attachment 9003556 [details] [diff] [review]
Remove js::IsDelegate, rename IsDelegateOfObject to IsPrototypeOf.

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

\o/

::: js/src/jit/CodeGenerator.cpp
@@ +12197,2 @@
>  static bool
> +IsPrototype(JSContext* cx, HandleObject protoObj, HandleObject obj, bool* res)

Pre-existing: this wrapper function was necessary at some point for a different reason, but it no longer is. Mind removing it and using IsPrototypeOf directly in the code below? You'll have to change IsPrototypeFn to have a matching signature, JSObject* instead of HandleObject, to get it to compile. Thanks!
Attachment #9003556 - Flags: review?(jdemooij) → review+
Attachment #9003556 - Attachment is obsolete: true
(In reply to Jan de Mooij [:jandem] from comment #4)
> Pre-existing: this wrapper function was necessary at some point for a
> different reason, but it no longer is. Mind removing it and using
> IsPrototypeOf directly in the code below? You'll have to change
> IsPrototypeFn to have a matching signature, JSObject* instead of
> HandleObject, to get it to compile. Thanks!

I'm confused on why the callsite of that function pointer didn't have to get updated to use JSObject instead of HandleObject... from your comment, doing nothing seems like the right thing to do, but I want to make sure.
Attachment #9003810 - Flags: review?(jdemooij)
Comment on attachment 9003810 [details] [diff] [review]
Remove js::IsDelegate, rename IsDelegateOfObject to IsPrototypeOf.

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

Thanks!
Attachment #9003810 - Flags: review?(jdemooij) → review+
(In reply to Ashley Hauck [:khyperia] from comment #6)
> I'm confused on why the callsite of that function pointer didn't have to get
> updated to use JSObject instead of HandleObject... from your comment, doing
> nothing seems like the right thing to do, but I want to make sure.

We have the JSObject* in a register (objReg) here:

https://searchfox.org/mozilla-central/rev/e126996d9b0a3d7653de205898517f4f5b632e7f/js/src/jit/CodeGenerator.cpp#12108-12110

The static VMFunction definition is used to create a trampoline for this particular C++ function. The trampoline uses the signature to decide how to pass the argument. For HandleObject it knows to pass a pointer-to-JSObject* instead of the JSObject*:

https://searchfox.org/mozilla-central/rev/e126996d9b0a3d7653de205898517f4f5b632e7f/js/src/jit/VMFunctions.h#373-375

So we generate slightly different code for JSObject* vs HandleObject but that's handled automatically.
(In reply to Jan de Mooij [:jandem] from comment #8)
> So we generate slightly different code for JSObject* vs HandleObject but
> that's handled automatically.

Thanks for the explanation!
Keywords: checkin-needed
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba637657bbf8
Remove js::IsDelegate, rename IsDelegateOfObject to IsPrototypeOf. r=jandem
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ba637657bbf8
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.