Closed
Bug 772138
Opened 12 years ago
Closed 11 years ago
Merge wrappers with proxies
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: ejpbruel, Unassigned)
References
Details
(Whiteboard: [js:t])
Attachments
(1 file)
5.44 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
Now that ProxyHandler's have the notion of a target object, having Wrappers as a separate concept no longer makes much sense. It thus seems like a good idea to merge these two concepts. This will involve the following steps: 1. Rename Wrapper::isWrapper to BaseProxyHandler::hasTarget. This function should return true if the particular proxy has the notion of a target object, not just if its family is sWrapperFamily. 2. Move the policy enforcement traps and the corresponding enums to IndirectProxyHandler, and get rid of the Wrapper base class. 3. Refactor IndirectProxyHandler and DirectProxyHandler so that they call the policy enforcement traps/ 4. Get rid of IndirectWrapper and DirectWrapper (possibly redefine them as typedefs of the corresponding proxy handlers)
Comment 1•12 years ago
|
||
Cool! I'm glad that proxies are getting all this tender loving attention.
Updated•12 years ago
|
Whiteboard: [js:t]
Reporter | ||
Comment 2•12 years ago
|
||
Those ugly handler->toWrapper() calls can go away as soon as we move all the stuff in Wrapper over to IndirectProxyHandler.
Attachment #640850 -
Flags: review?(bobbyholley+bmo)
Comment 3•12 years ago
|
||
Comment on attachment 640850 [details] [diff] [review] Patch to be reviewed This patch as it stands is incorrect, I think, because we need to set canUnwrap to false nsOuterWindowProxy. Definitely do that. >+ inline bool hasTarget() { >+ return mHasTarget; >+ } >+ >+ inline bool canUnwrap() { >+ return mCanUnwrap; >+ }; >+ Add a comment that these are intentionally non-virtual? > JS_FRIEND_API(JSObject *) > js::UnwrapObject(JSObject *wrapped, bool stopAtOuter, unsigned *flagsp) stopAtOuter should be become "force" or something. Add that as a patch on top of this one? Can we add a MOZ_ASSERT(proxy->hasTarget()) in GetProxyTargetObject? > JS_FRIEND_API(JSObject *) > js::UnwrapObjectChecked(JSContext *cx, JSObject *obj) > { >+ BaseProxyHandler *handler; > while (obj->isWrapper() && >- !JS_UNLIKELY(!!obj->getClass()->ext.innerObject)) { >+ (handler = GetProxyHandler(obj)) && >+ (handler->canUnwrap() || !stopAtOuter)) { > JSObject *wrapper = obj; >- Wrapper *handler = Wrapper::wrapperHandler(obj); > bool rvOnFailure; >- if (!handler->enter(cx, wrapper, JSID_VOID, >- Wrapper::PUNCTURE, &rvOnFailure)) >+ if (!handler->toWrapper()->enter(cx, wrapper, JSID_VOID, >+ Wrapper::PUNCTURE, &rvOnFailure)) The ->toWrapper() stuff is a pain. Can we just move enter() into BaseProxyHandler and give it a default implementation of { return true; }? r=bholley with the requested changes (either in this patch or layered on top and landing simultaneously).
Attachment #640850 -
Flags: review?(bobbyholley+bmo) → review+
Reporter | ||
Updated•11 years ago
|
Assignee: ejpbruel → general
Comment 4•11 years ago
|
||
I don't think we want to do this anymore.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•