Currently in the engine, there is a single JSClass for all ObjectProxies. This actively inhibits the DOM from storing relevant Class-scoped information about objects that happen to be implemented as proxies. Boris has proposed we move to a model in which we rely on a flag check to see if a JSClass represents a proxy, and implement some macros which allow easy class implementation of non-overridden features. Preliminary discussions at the Toronto Summit gave at least verbal approval of such a notion from both Jason and Jan.

Sounds to me like a good incremental step toward proxies as the only way to have custom objects! \o/

According to my research (but mostly Waldo), these flags are juts unused in the engine. Since we are gonna need one to do this anyway, start by freeing up what we can.

Comment on attachment 824956 [details] [diff] [review] Part 0: Free up a JSClass Flag Review of attachment 824956 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/public/Class.h @@ +536,5 @@ > #define JSCLASS_IS_GLOBAL (1<<(JSCLASS_HIGH_FLAGS_SHIFT+1)) > #define JSCLASS_INTERNAL_FLAG2 (1<<(JSCLASS_HIGH_FLAGS_SHIFT+2)) > #define JSCLASS_INTERNAL_FLAG3 (1<<(JSCLASS_HIGH_FLAGS_SHIFT+3)) > > +// Bits 21 and 22 unused. 21/22 don't signal (1<<(JSCLASS_HIGH_FLAGS_SHIFT+(4 or 5))) to me. I'd just rename the #defines to JSCLASS_RESERVEDFLAG[12] with a comment /* not used at present */ above.

https://tbpl.mozilla.org/?tree=Try&rev=74284c8a7087 All the changes needed to write some macros for proxy class generation and call it a win. Green.

Oh, don't forget to update https://developer.mozilla.org/en-US/docs/SpiderMonkey/31 for this. For better or worse, https://developer.mozilla.org/en-US/docs/SpiderMonkey/JSAPI_Reference/JSClass.flags never mentioned this flags. Oh well.

Comment on attachment 8343408 [details] [diff] [review] Part 2: Standardize OuterWindowProxy to use the proxy Convert and HasInstance stubs Review of attachment 8343408 [details] [diff] [review]: ----------------------------------------------------------------- r=bholley with the defaultValue override on nsOuterWindowProxy removed. ::: dom/base/nsGlobalWindow.cpp @@ +915,5 @@ > +nsOuterWindowProxy::defaultValue(JSContext *cx, JS::Handle<JSObject*> proxy, > + JSType hint, JS::MutableHandle<JS::Value> vp) > +{ > + return js::BaseProxyHandler::defaultValue(cx, proxy, hint, vp); > +} We don't need this override. nsOuterWindowProxy is not a security wrapper, so the inherited behavior from Wrapper is fine.

Comment on attachment 8343414 [details] [diff] [review] Part 6: Allow callers of Wrapper::New to specify a proxy class Eric and I discussed a way to shrink this patch on IRC. He's going to do that.

Comment on attachment 8343409 [details] [diff] [review] Part 3: Allow future Proxy JSClasses to safely have more than 4 reserved slots. Review of attachment 8343409 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsproxy.cpp @@ +3060,5 @@ > + /* > + * Allow for people to add extra slots to "proxy" classes, without allowing > + * them to set their own trace hook. Trace the extras. > + */ > + unsigned numSlots = JSCLASS_RESERVED_SLOTS(obj->getClass()); It's a bit clearer to use |proxy| instead of |obj| here. ::: js/src/jsproxy.h @@ +349,5 @@ > */ > const uint32_t PROXY_PRIVATE_SLOT = 0; > const uint32_t PROXY_HANDLER_SLOT = 1; > const uint32_t PROXY_EXTRA_SLOT = 2; > +const uint32_t PROXY_NUM_SLOTS = 4; I haven't seen the other patches in this bug, but currently we have two places where we hard-code 4 as the number of reserved slots in jsproxy.cpp. Can you replace those with this const if that's still relevant? ::: js/src/vm/ProxyObject.cpp @@ +95,5 @@ > NukeSlot(this, EXTRA_SLOT + 1); > + > + /* Allow people to add their own number of reserved slots beyond the expected 4 */ > + unsigned numSlots = JSCLASS_RESERVED_SLOTS(getClass()); > + for (unsigned i = PROXY_NUM_SLOTS; i < numSlots; i++) It should be safe to start at 0 and eliminate the NukeSlot calls above. ::: js/src/vm/ProxyObject.h @@ +70,5 @@ > JS_ASSERT(n == 0 || n == 1); > return &getReservedSlotRef(EXTRA_SLOT + n); > } > > + HeapSlot *slotOfClassSpecific(size_t n) { This name is not so good. How about classSpecificSlot?

Comment on attachment 8343491 [details] [diff] [review] Part 6: Allow callers of Wrapper::New to specify a proxy class v2 Review of attachment 8343491 [details] [diff] [review]: ----------------------------------------------------------------- This is much nicer. ::: js/src/jsproxy.h @@ +411,5 @@ > > class MOZ_STACK_CLASS ProxyOptions { > + protected: > + /* protected constructor for subclass */ > + ProxyOptions(bool argSingleton, const Class *argClasp) Isn't the convention |singletonArg| and |claspArg|?

Comment on attachment 8343413 [details] [diff] [review] Part 5: Allow callers of NewProxyObject() to specify a proxy class Review of attachment 8343413 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsproxy.h @@ +397,5 @@ > > class MOZ_STACK_CLASS ProxyOptions { > public: > + ProxyOptions() : singleton_(false), > + clasp_(UncallableProxyClassPtr) Fix indentation, please

Comment on attachment 8343417 [details] [diff] [review] Part 7: Rename OuterWindowProxy::class_ nsOuterWindowProxyClass and move it to dom/base/nsGlobalWindow.cpp Review of attachment 8343417 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsGlobalWindow.cpp @@ +667,5 @@ > bool AppendIndexedPropertyNames(JSContext *cx, JSObject *proxy, > JS::AutoIdVector &props); > }; > > +const js::Class nsOuterWindowProxyClass = { Don't use ns prefixes in new code.

Comment on attachment 8343405 [details] [diff] [review] Part 1: Use newly freed flags to implement IsProxy() Review of attachment 8343405 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/IonMacroAssembler.h @@ +925,4 @@ > > + branchTest32(Assembler::NonZero, flags, Imm32(JSCLASS_IS_PROXY), slowCheck); > + > + test32(flags, Imm32(JSCLASS_EMULATES_UNDEFINED)); We could combine these two into a single less-than check, or similar, if the two bits were the largest bits in flags (or similar if these were the lowest bits). Would it win anything? Might be worth a followup to at least investigate. As far as I know, aside from the fields of bits used for JSProtoKey, there are no constraints on the ordering of the flags. ::: js/src/jsproxy.h @@ +319,5 @@ > extern JS_FRIEND_DATA(const js::Class* const) OuterWindowProxyClassPtr; > > inline bool IsProxyClass(const Class *clasp) > { > + return clasp->isProxy(); Please inline this into all callers and get rid of the method. Followup patch, rs=me on it.

Comment on attachment 8343412 [details] [diff] [review] Part 4: Create handy new friendapi macros for proxy class creation Review of attachment 8343412 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsfriendapi.h @@ +228,5 @@ > /* > + * Helper Macros for creating JSClasses that function as proxies. > + * > + * NB: The macro invocation must be surrounded by braces, so as to > + * allow for potention JSClass extensions. Rather than say "must be", give an example in the comment of exactly how to use the macro. Right now if I wanted to use this I'd have to go find a user and copy it, or do some very careful reading of what this expands to. Also, if this comment sticks around past that (which it probably shouldn't -- it should be clear from the example how to use it), "potential". @@ +244,5 @@ > +#define PROXY_CLASS_WITH_EXT(name, extraSlots, flags, callOp, constructOp, ext) \ > + { \ > + name, \ > + js::Class::NON_NATIVE | \ > + JSCLASS_IS_PROXY | \ Weird alignment.

Comment on attachment 8343413 [details] [diff] [review] Part 5: Allow callers of NewProxyObject() to specify a proxy class Review of attachment 8343413 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsproxy.h @@ +413,5 @@ > + ProxyOptions &setClass(const Class *argClasp) { > + clasp_ = argClasp; > + return *this; > + } > + ProxyOptions &setStandardClass(bool callable) { This name, going with this argument, is weird. Maybe "selectDefaultClass" instead? ::: js/src/vm/ProxyObject.h @@ +84,5 @@ > + > + // proxy_Trace is just a trivial wrapper around ProxyObject::trace for > + // friend api exposure. > + return clasp->isProxy() && > + clasp->flags & JSCLASS_IMPLEMENTS_BARRIERS && I think we parenthesize & expressions because the bitwise precedence is screwy.

PROXY_MINIMUM_SLOTS seems like a clearer name than PROXY_NUM_SLOTS, to me.

Once this all lands, can we move the proxy handler pointer into the JSClass instead of sticking it in a slot on the object? Becuase then we could also get rid of the proxy private/extra slots and just have proxies have the same reserved slot setup as normal objects... If that seems reasonable, I'll file a followup to do it.

I guess the main drawbacks would be the extra entry in JSClass for all other objects and the fact that you have to pointer-chase a bit more to get to the handler. But you need less bitshifting as you do it, so who knows whether it's any slower in practice.

(In reply to Boris Zbarsky [:bz] from comment #25) > Once this all lands, can we move the proxy handler pointer into the JSClass > instead of sticking it in a slot on the object? Yes please. This will surely not be any slower in practice.

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/32aeb054f757 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/488ab195f1ae remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/d038c9b4c7dc remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/1a0f8903f3ce remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ae3ceb166be1 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/aac3d84c27c0 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/d3af8463dddc remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/c1bd9e2e41b2 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/beb52f820ac5

https://hg.mozilla.org/mozilla-central/rev/32aeb054f757 https://hg.mozilla.org/mozilla-central/rev/488ab195f1ae https://hg.mozilla.org/mozilla-central/rev/d038c9b4c7dc https://hg.mozilla.org/mozilla-central/rev/1a0f8903f3ce https://hg.mozilla.org/mozilla-central/rev/ae3ceb166be1 https://hg.mozilla.org/mozilla-central/rev/aac3d84c27c0 https://hg.mozilla.org/mozilla-central/rev/d3af8463dddc https://hg.mozilla.org/mozilla-central/rev/c1bd9e2e41b2 https://hg.mozilla.org/mozilla-central/rev/beb52f820ac5