Closed Bug 947014 Opened 12 years ago Closed 11 years ago

Allow Wrapper::New to take a prototype

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: efaust, Assigned: efaust)

Details

(Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

Attached patch wrapperProto.patch (obsolete) — Splinter Review
As a nit in bug 926012, Bobby was unhappy with the WrapperWithProto::New duplicating, rather than leveraging the code in Wrapper::New. Attached is a patch which will rectify that problem, on top of the machinery introduced later in bug 924720. I didn't know where exactly to put it, since it has so many dependencies on code that hasn't landed yet, but here we are...
Attachment #8343422 - Flags: review?(bobbyholley+bmo)
Comment on attachment 8343422 [details] [diff] [review] wrapperProto.patch Review of attachment 8343422 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsproxy.h @@ +437,5 @@ > + * the stack, it's easy to make a GC mistake. WrapperOptions > + * makes no guarantees about marking this saved proto pointer. > + * Since Wrapper::New guarantees that there are no GCing calls > + * between its invocation and NewProxyObject(), this should be > + * relatively safe. Just...be careful out there, OK? Erm, not so happy about this. Waiting for the static analysis to catch changes in Wrapper::New isn't great, because the poor programmer won't find out until {s}he pushes. Let's add an additional Wrapper constructor that takes a JSContext, and uses to construct proto_, which is a Maybe<RootedObject>. setProto will then assert that the these WrapperOptions were constructed with a cx, and thus are "object ready".
Attachment #8343422 - Flags: review?(bobbyholley+bmo) → review-
Implement the suggestion above. Note that WrapperOption(cx) construction sites will have to setStandardClass() for themselves, but that's OK, as it's the same behavior as NewProxyObject()
Attachment #8343422 - Attachment is obsolete: true
Attachment #8343490 - Flags: review?(bobbyholley+bmo)
Comment on attachment 8343490 [details] [diff] [review] Now with less footgun Review of attachment 8343490 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for doing this. r=bholley with comments addressed. ::: js/src/jsproxy.h @@ +443,5 @@ > > class MOZ_STACK_CLASS WrapperOptions : public ProxyOptions { > public: > + WrapperOptions() : ProxyOptions(false, nullptr), > + proto_() I don't understand why you need the proto_(), here and below. @@ +448,3 @@ > {} > + > + WrapperOptions(JSContext *cx) : ProxyOptions(false, nullptr), Please provide ample documentation for this constructor and the setProto accessor explaining the situation. @@ +456,5 @@ > + JSObject *proto() const { > + return proto_.empty() ? js::Proxy::LazyProto : proto_.ref(); > + } > + WrapperOptions &setProto(JSObject *argProto) { > + proto_.ref() = argProto; I know it's implicit here, but let's explicitly MOZ_ASSERT(!proto_.empty()) first.
Attachment #8343490 - Flags: review?(bobbyholley+bmo) → review+
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: