Last Comment Bug 740064 - Refactor XrayWrapper
: Refactor XrayWrapper
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Peter Van der Beken [:peterv]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-28 11:17 PDT by Peter Van der Beken [:peterv]
Modified: 2012-03-31 19:26 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (58.06 KB, patch)
2012-03-28 11:28 PDT, Peter Van der Beken [:peterv]
bobbyholley: review+
Details | Diff | Review

Description Peter Van der Beken [:peterv] 2012-03-28 11:17:46 PDT
We're adding a third type of XrayWrappers, we should consolidate the two existing ones and templatize.
Comment 1 Peter Van der Beken [:peterv] 2012-03-28 11:28:41 PDT
Created attachment 610216 [details] [diff] [review]
v1
Comment 2 Bobby Holley (PTO through June 13) 2012-03-28 15:07:33 PDT
Comment on attachment 610216 [details] [diff] [review]
v1

+class XPCWrappedNativeXrayTraits
...
+    static JSObject* getInnerObject(JSObject *wrapper);

Can we inline this, or drop the inline in ProxyXrayTraits, so that the two are consistent?

+class ProxyXrayTraits
...
+private:
+    static JSObject* getHolderObject(JSContext *cx, JSObject *wrapper,
+                                     bool createHolder)
+    {
+        if (!js::GetProxyExtra(wrapper, 0).isUndefined())
+            return &js::GetProxyExtra(wrapper, 0).toObject();
+
+        if (!createHolder)
+            return nsnull;
+
+        return createHolderObject(cx, wrapper);
+    }

It sure would be nice to do this eagerly so that we can drop the |cx| and simplify the signature situation with getHolderObject. Add that to the followup list?

-    NS_ASSERTION(js::GetObjectJSClass(holder) == &HolderClass, "expected a native property holder object");
+    NS_ASSERTION(js::GetObjectJSClass(holder) == &HolderClass,
+                 "expected a native property holder object");
...
-    NS_ASSERTION(js::GetObjectJSClass(holder) == &HolderClass, "expected a native property holder object");
+    NS_ASSERTION(js::GetObjectJSClass(holder) == &HolderClass,
+                 "expected a native property holder object");
...
-    NS_ASSERTION(js::GetObjectJSClass(holder) == &HolderClass, "expected a native property holder object");
+    NS_ASSERTION(js::GetObjectJSClass(holder) == &HolderClass,
+                 "expected a native property holder object");

As long as we're changing these, let's just turn them into MOZ_ASSERTS so that we can kill
the string entirely. There are some more of these scattered throughout the file.


+class AutoLeaveHelper

Thanks for making this non-templated. :-)



+template <typename Base, typename Traits>
+XrayWrapper<Base, Traits>::XrayWrapper(unsigned flags)
+  : Base(flags | WrapperFactory::IS_XRAY_WRAPPER_FLAG)
+{
+}
+
+template <typename Base, typename Traits>
+XrayWrapper<Base, Traits>::~XrayWrapper()
+{
+}

Can we just inline these?


 
-            if (!JS_GetPropertyDescriptorById(cx, wnObject, id,
+            if (!JS_GetPropertyDescriptorById(cx, obj, id,
                                               (set ? JSRESOLVE_ASSIGNING : 0) | JSRESOLVE_QUALIFIED,
-                                              desc))
+                                              desc)) {

Just FYI: There was recently a JS style guide change to make the open brace here go on its own line.
 
-// The 'holder' here isn't actually of [[Class]] HolderClass like those used by
-// XrayWrapper. Instead, it's a funny hybrid of the 'expando' and 'holder'
-// properties. However, we store it in the same slot. Exercise caution.

I think it would be good to preserve this comment, but maybe that's just because I wrote it.
 
+typedef XrayWrapper<js::CrossCompartmentWrapper, ProxyXrayTraits > XrayProxy;

The XrayProxy we use in PXOW should inherit js::CrossCompartmentSecurityWrapper. Looks like that was already broken though...
Comment 3 Peter Van der Beken [:peterv] 2012-03-30 20:38:06 PDT
(In reply to Bobby Holley (:bholley) from comment #2)
> Can we inline this, or drop the inline in ProxyXrayTraits, so that the two
> are consistent?

This involved moving more code around, so I didn't do it.

> +template <typename Base, typename Traits>
> +XrayWrapper<Base, Traits>::XrayWrapper(unsigned flags)
> +  : Base(flags | WrapperFactory::IS_XRAY_WRAPPER_FLAG)
> +{
> +}
> +
> +template <typename Base, typename Traits>
> +XrayWrapper<Base, Traits>::~XrayWrapper()
> +{
> +}
> 
> Can we just inline these?


I ended up not doing this, because it means more includes in the header.

> The XrayProxy we use in PXOW should inherit
> js::CrossCompartmentSecurityWrapper. Looks like that was already broken
> though...

Followup it is.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1faf67b119e6
Comment 4 Ed Morley [:emorley] 2012-03-31 19:26:56 PDT
https://hg.mozilla.org/mozilla-central/rev/1faf67b119e6

Note You need to log in before you can comment on or make changes to this bug.