Replace IsDelegate with IsDelegateOfObject

RESOLVED FIXED in Firefox 63

Status

()

P3
normal
RESOLVED FIXED
9 months ago
4 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)

(Reporter)

Description

9 months ago
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)

Updated

4 months ago
Assignee: nobody → khyperia
Status: NEW → ASSIGNED
(Assignee)

Comment 2

4 months ago
Created attachment 9003556 [details] [diff] [review]
Remove js::IsDelegate, rename IsDelegateOfObject to IsPrototypeOf.
(Assignee)

Updated

4 months ago
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+
(Assignee)

Comment 5

4 months ago
Created attachment 9003810 [details] [diff] [review]
Remove js::IsDelegate, rename IsDelegateOfObject to IsPrototypeOf.
(Assignee)

Updated

4 months ago
Attachment #9003556 - Attachment is obsolete: true
(Assignee)

Comment 6

4 months ago
(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.
(Assignee)

Updated

4 months ago
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.
(Assignee)

Comment 9

4 months ago
(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

Comment 10

4 months ago
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

Comment 11

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ba637657bbf8
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
status-firefox61: affected → wontfix
status-firefox62: --- → wontfix
You need to log in before you can comment on or make changes to this bug.