Closed Bug 1449985 Opened 2 years ago Closed 2 years ago

Replace IsDelegate with IsDelegateOfObject

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- fixed

People

(Reporter: anba, Assigned: khyperia)

Details

Attachments

(1 file, 1 obsolete file)

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: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.