Eddy Bruel [:ejpbruel] Reporter
	Description • 12 years ago

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)

	Luke Wagner [:luke]
	Comment 1 • 12 years ago

Cool!  I'm glad that proxies are getting all this tender loving attention.

	Eddy Bruel [:ejpbruel] 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.

	Bobby Holley (:bholley)
	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).

	Bobby Holley (:bholley)
	Comment 4 • 11 years ago

I don't think we want to do this anymore.