TM: rename JSNoopProxyHandler to JSWrapper and move it into jswrapper.[h|cpp]

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: gal, Assigned: gal)

Tracking

Trunk
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 1 obsolete attachment)

21.26 KB, patch
cdleary
: review+
Details | Diff | Splinter Review
(Assignee)

Description

8 years ago
This file will hold our new membranes and wrapper code.
(Assignee)

Comment 1

8 years ago
Created attachment 447899 [details] [diff] [review]
patch
Assignee: general → gal
(Assignee)

Updated

8 years ago
Attachment #447899 - Flags: review?(mozilla+ben)
(Assignee)

Comment 2

8 years ago
Review ping.
(Assignee)

Updated

8 years ago
Attachment #447899 - Flags: review?(mozilla+ben) → review?(cdleary)
Comment on attachment 447899 [details] [diff] [review]
patch

The singleton wrapper object is a static member of the |JSWrapper| class, so calling |JSWrapper::wrap| will only ever resolve to |JSWrapper::construct| when |singleton.construct| is called in that function -- I don't think that's what we want. The interface that was in there before, |JSWrapper<typename T extends JSWrapper>::wrap| seems to make the most sense, so you can grab |T::singleton| like it was doing. You can expose this kind of thing across C API boundaries by making a helper function per wrapper class that does something like |NopWrap = JSWrapper<JSWrapper>::wrap|.

The interface for JSNoopProxyHandler is still in there, should rip that out too.

The naming is getting confusing -- in ::wrap you have |JSWrapper *handler, JSObject *wrapper|. I get that the wrapper is the JS-land wrapper object, but still makes me double take. :-)

Classes in namespace js don't need a JS prefix, but that's probably for another bug.
Attachment #447899 - Flags: review?(cdleary)
(Assignee)

Comment 4

8 years ago
Created attachment 448302 [details] [diff] [review]
patch
Attachment #447899 - Attachment is obsolete: true
Attachment #448302 - Flags: review?(cdleary)
(Assignee)

Comment 5

8 years ago
I will leave a more complex solution to wrap() to a follow-up patch that actually specializes wrappers.
Attachment #448302 - Flags: review?(cdleary) → review+
(Assignee)

Comment 6

8 years ago
http://hg.mozilla.org/tracemonkey/rev/caf9170f2971
Whiteboard: fixed-in-tracemonkey
Blocks: 563099

Comment 7

8 years ago
http://hg.mozilla.org/mozilla-central/rev/caf9170f2971
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.