This moves the old bindings into mozilla::dom::oldproxybindings for now. It remove the templatized proxyhandler, we just generate them now. I'm not too happy about using IsNull to check whether a property is present or not, but I'll probably remove that in a followup (IndexedGetter/NamedGetter should probably take a |bool& found| instead). The IsNull stuff should be equivalent with what we have right now. This doesn't support separate Creator and Setter, shouldn't be too hard to implement but it's a followup because we don't have a need for it.

Comment on attachment 636915 [details] [diff] [review] v1 Review of attachment 636915 [details] [diff] [review]: ----------------------------------------------------------------- And something about docs :) ::: dom/bindings/Codegen.py @@ +140,5 @@ > return "" > def define(self): > + if self.descriptor.concrete and self.descriptor.proxy: > + settersDeclare = """JSBool InvalidateProtoShape_add(JSContext* cx, JSHandleObject obj, JSHandleId id, jsval* vp); > +JSBool InvalidateProtoShape_set(JSContext* cx, JSHandleObject obj, JSHandleId id, JSBool strict, jsval* vp); JS::Value @@ +144,5 @@ > +JSBool InvalidateProtoShape_set(JSContext* cx, JSHandleObject obj, JSHandleId id, JSBool strict, jsval* vp); > +""" > + settersDefine = """ > +JSBool > +InvalidateProtoShape_add(JSContext* cx, JSHandleObject obj, JSHandleId id, jsval* vp) And here @@ +149,5 @@ > +{ > + // We need to make sure that this is our prototype object, not a DOM object > + // with this prototype object on its prototype chain. > + if (JSID_IS_STRING(id) && JS_InstanceOf(cx, obj, &PrototypeClass, NULL)) > + js::SetReservedSlot(obj, 0, js::PrivateUint32Value(CHECK_CACHE)); {} @@ +150,5 @@ > + // We need to make sure that this is our prototype object, not a DOM object > + // with this prototype object on its prototype chain. > + if (JSID_IS_STRING(id) && JS_InstanceOf(cx, obj, &PrototypeClass, NULL)) > + js::SetReservedSlot(obj, 0, js::PrivateUint32Value(CHECK_CACHE)); > + return JS_TRUE; true @@ +948,5 @@ > def __init__(self, descriptor, name, static): > PropertyDefiner.__init__(self, descriptor, name) > > + # FIXME We should be able to check for special operations without an > + # indentifier. For now we check if the name starts with __ identifier @@ +1362,5 @@ > + if self.descriptor.proxy: > + create = """ JSObject *obj = NewProxyObject(aCx, &DOMProxyHandler::instance, > + JS::PrivateValue(aObject), proto, parent); > + if (!obj) > + return NULL; {} @@ +2193,5 @@ > > return (template, declType, None, isOptional) > > if not type.isPrimitive(): > + raise TypeError("Need conversion for argument type '%s'" % str(type)) This shouldn't be necessary @@ +2604,4 @@ > any(typeNeedsCx(t) for t in type.unroll().flatMemberTypes)))) > > # Returns a CGThing containing the type of the return value, or None > # if there is no need for a return value. Update this @@ +2632,5 @@ > if returnType.isCallback(): > # XXXbz we're going to assume that callback types are always > # nullable for now. > + return CGGeneric("JSObject*"), False > + if returnType.tag() is IDLType.Tags.any: This is where we fix isAny(), right? @@ +2659,5 @@ > > class CGCallGenerator(CGThing): > """ > A class to generate an actual call to a C++ object. Assumes that the C++ > object is stored in a variable named "self". ... whose name is given by the |object| argument. @@ +2699,5 @@ > self.cgRoot = CGList([], "\n") > > call = CGGeneric(nativeMethodName) > if static: > + call = CGWrapper(call, pre="%s::" % (descriptorProvider.nativeType)) Shouldn't need the parens anymore @@ +2789,5 @@ > + def getArgumentNames(self): > + args = [] > + for (i, a) in enumerate(self.arguments): > + args.append((a, "arg" + str(i))) > + return args return [(a, "arg%i" % i) for (i, a) in enumerate(self.arguments)] @@ +3717,5 @@ > 'args': args, > 'const': ' const' if self.const else '', > 'body': body }) > > +class ClassConstructor(ClassItem): Some documentation for this class, please @@ +3719,5 @@ > 'body': body }) > > +class ClassConstructor(ClassItem): > + def __init__(self, args, inline=False, bodyInHeader=False, > + visibility="private", baseConstructors=None, body=None): And what these arguments all are @@ +3743,5 @@ > + initialize = m.getBody() > + if initialize: > + items.append(m.name + "(" + initialize + ")") > + > + if len(items) > 0: if items: @@ +3756,5 @@ > + args = ', '.join([str(a) for a in self.args]) > + if self.bodyInHeader: > + body = ' ' + self.getBody(); > + body = stripTrailingWhitespace(body.replace('\n', '\n ')) > + if len(body) > 0: if body: @@ +3776,5 @@ > + args = ', '.join([str(a) for a in self.args]) > + > + body = ' ' + self.getBody() > + body = '\n' + stripTrailingWhitespace(body.replace('\n', '\n ')) > + if len(body) > 0: Same @@ +4117,5 @@ > def define(self): > # Header only > return '' > > +class CGProxySpecialOperation(CGPerSignatureCall): Documentation, please @@ +4125,5 @@ > + signature = operation.signatures()[0] > + extendedAttributes = descriptor.getExtendedAttributes(operation) > + > + returnType = signature[0] > + arguments = signature[1] Maybe returnType, arguments = signature @@ +4133,5 @@ > + len(arguments)) > + > + if operation.isSetter() or operation.isCreator(): > + if len(arguments) > 2: > + raise TypeError("Setter or creator with more than 2 arguments?") Are those things that could go into the parser? (Followup is fine.) @@ +4146,5 @@ > + self.cgRoot.prepend(instantiateJSToNativeConversionTemplate(template, templateValues)) > + else: > + if len(arguments) > 1: > + raise TypeError("Getter or deleter with more than 1 argument?") > + def getArgumentNames(self): Newlines between those functions, please? @@ +4147,5 @@ > + else: > + if len(arguments) > 1: > + raise TypeError("Getter or deleter with more than 1 argument?") > + def getArgumentNames(self): > + return map(lambda a: (a, a.identifier.name), self.arguments) return [(a, a.identifier.name) for a in self.arguments] @@ +4153,5 @@ > + errorReport = CGPerSignatureCall.getErrorReport(self) > + if not self.idlNode.isGetter(): > + return errorReport > + return CGIfWrapper(errorReport, > + "rv.ErrorCode() != NS_ERROR_DOM_INDEX_SIZE_ERR") Hmm, what's this for? @@ +4174,5 @@ > + "}") > + elif self.isFallible(): > + wrap = ("if (rv.ErrorCode() != NS_ERROR_DOM_INDEX_SIZE_ERR) {\n" + > + CGIndenter(CGGeneric(wrap)).define() + "\n" + > + "}") And this @@ +4177,5 @@ > + CGIndenter(CGGeneric(wrap)).define() + "\n" + > + "}") > + return "\n" + wrap > + > +class CGProxyIndexedGetter(CGProxySpecialOperation): These all want docs @@ +4204,5 @@ > + return "" > + def definition_body(self): > + return " return js::IsProxy(obj) && js::GetProxyHandler(obj) == &DOMProxyHandler::instance;" > + > +class CGProxyUwrap(CGAbstractMethod): Unwrap? @@ +4213,5 @@ > + return "" > + def definition_body(self): > + return """ if (xpc::WrapperFactory::IsXrayWrapper(obj)) > + obj = js::UnwrapObject(obj); > + JS_ASSERT(IsProxy(obj)); MOZ_ASSERT @@ +4333,5 @@ > + templateValues = {'jsvalRef': 'desc->value', 'jsvalPtr': '&desc->value', > + 'obj': 'proxy', 'successCode': fillDescriptor} > + namedGet = ("\n" + > + "if (!set && JSID_IS_STRING(id) && !HasPropertyOnPrototype(cx, proxy, id)) {\n" + > + " jsval nameVal = STRING_TO_JSVAL(JSID_TO_STRING(id));\n" + JS::Value @@ +4334,5 @@ > + 'obj': 'proxy', 'successCode': fillDescriptor} > + namedGet = ("\n" + > + "if (!set && JSID_IS_STRING(id) && !HasPropertyOnPrototype(cx, proxy, id)) {\n" + > + " jsval nameVal = STRING_TO_JSVAL(JSID_TO_STRING(id));\n" + > + " xpc_qsDOMString name(cx, nameVal, &nameVal,\n" + Followup to use ConvertJSValueToString, please @@ +4347,5 @@ > + "}\n") % (self.descriptor.nativeType) > + else: > + namedGet = "" > + > + return setOrIndexedGet + """JSObject *expando; * to the left @@ +4389,5 @@ > + if not self.descriptor.operations['NamedCreator'] is namedSetter: > + raise TypeError("Can't handle creator that's different from the setter") > + set += ("if (JSID_IS_STRING(id)) {\n" + > + " jsval nameVal = STRING_TO_JSVAL(JSID_TO_STRING(id));\n" + > + " xpc_qsDOMString name(cx, nameVal, &nameVal,\n" + Same @@ +4412,5 @@ > + def getBody(self): > + indexedGetter = self.descriptor.operations['IndexedGetter'] > + if indexedGetter: > + addIndices = """PRUint32 length; > +UnwrapProxy(proxy)->GetLength(&length); We have a GetLength() without outparam now, no? @@ +4430,5 @@ > + !js::GetPropertyNames(cx, expando, JSITER_OWNONLY | JSITER_HIDDEN, &props)) { > + return false; > +} > + > +// FIXME: Add named items Is this filed? Bug number please. @@ +4456,5 @@ > + namedGetter = self.descriptor.operations['NamedGetter'] > + if namedGetter: > + named = ("if (JSID_IS_STRING(id) && !HasPropertyOnPrototype(cx, proxy, id)) {\n" + > + " jsval nameVal = STRING_TO_JSVAL(JSID_TO_STRING(id));\n" + > + " xpc_qsDOMString name(cx, nameVal, &nameVal,\n" + Again @@ +4527,5 @@ > + namedGetter = self.descriptor.operations['NamedGetter'] > + if namedGetter: > + getNamed = ("if (JSID_IS_STRING(id)) {\n" + > + " jsval nameVal = STRING_TO_JSVAL(JSID_TO_STRING(id));\n" + > + " xpc_qsDOMString name(cx, nameVal, &nameVal,\n" + And here @@ +4531,5 @@ > + " xpc_qsDOMString name(cx, nameVal, &nameVal,\n" + > + " xpc_qsDOMString::eDefaultNullBehavior,\n" + > + " xpc_qsDOMString::eDefaultUndefinedBehavior);\n" + > + " if (!name.IsValid())\n" + > + " return false;\n" + {} @@ +4540,5 @@ > + else: > + getNamed = "" > + > + return """NS_ASSERTION(!xpc::WrapperFactory::IsXrayWrapper(proxy), > + "Should not have a XrayWrapper here"); MOZ_ASSERT? @@ +4616,5 @@ > + indexToId) % (self.descriptor.nativeType) > + else: > + get = indexToId + """ > + > +JSObject *expando = GetExpandoObject(proxy); * to the left @@ +4629,5 @@ > + } > +}""" > + > + return """NS_ASSERTION(!xpc::WrapperFactory::IsXrayWrapper(proxy), > + "Should not have a XrayWrapper here"); MOZ_ASSERT? @@ +4662,5 @@ > + length = """ > + if (id == s_length_id) { > + if (vp) { > + PRUint32 length; > + UnwrapProxy(proxy)->GetLength(&length); Same about GetLength() @@ +4663,5 @@ > + if (id == s_length_id) { > + if (vp) { > + PRUint32 length; > + UnwrapProxy(proxy)->GetLength(&length); > + JS_ASSERT(int32_t(length) >= 0); MOZ_ASSERT, or use UINT_TO_JSVAL, I guess @@ +4725,5 @@ > + if (!ac.enter(cx, proxy)) { > + return false; > + } > +} > +JS_ASSERT(IsProxy(proxy)); MOZ_ASSERT @@ +4728,5 @@ > +} > +JS_ASSERT(IsProxy(proxy)); > + > +bool found; > +// We ignore an error from GetPropertyOnPrototype. Can we do that? ::: dom/bindings/DOMJSProxyHandler.cpp @@ +20,5 @@ > +#include "jsapi.h" > +#include "jsatom.h" > + > +using namespace JS; > +using namespace mozilla::dom; No need @@ +28,5 @@ > + > +jsid s_length_id = JSID_VOID; > + > +bool > +DefineStaticJSVal(JSContext* cx, jsid& id, const char* string) This is InternJSString from BindingUtils.h, AFAICT. @@ +57,5 @@ > +{ > + bool enabled; > + bool defined = aDefine(cx, obj, &enabled); > + NS_ASSERTION(!defined || enabled, > + "We defined a constructor but the new bindings are disabled?"); MOZ_ASSERT? @@ +88,5 @@ > + } > + > + JSCompartment* compartment = js::GetObjectCompartment(obj); > + xpc::CompartmentPrivate* priv = > + static_cast<xpc::CompartmentPrivate*>(JS_GetCompartmentPrivate(compartment)); xpc::CompartmentPrivate* priv = xpc::GetCompartmentPrivate(obj); @@ +147,5 @@ > + JSBool b = true; > + > + JSObject* expando; > + if (!xpc::WrapperFactory::IsXrayWrapper(proxy) && (expando = GetExpandoObject(proxy))) { > + jsval v; JS::Value (or Value, I guess) @@ +216,5 @@ > +JSString* > +DOMProxyHandler::obj_toString(JSContext* cx, const char* className) > +{ > + size_t nchars = 9 + strlen(className); /* 9 for "[object ]" */ > + jschar *chars = (jschar *)JS_malloc(cx, (nchars + 1) * sizeof(jschar)); static_cast<>, and * to the left @@ +221,5 @@ > + if (!chars) { > + return NULL; > + } > + > + const char *prefix = "[object "; Star @@ +223,5 @@ > + } > + > + const char *prefix = "[object "; > + nchars = 0; > + while ((chars[nchars] = (jschar)*prefix) != 0) { Cast @@ +240,5 @@ > + return str; > +} > + > +bool > +ProtoIsClean(JSContext* cx, JSObject* proto, const MethodsAndAttributes& properties, A documentation comment about what this does? @@ +324,5 @@ > + const MethodsAndAttributes& properties, jsid id_, bool* found, > + JS::Value* vp) > +{ > + JS::RootedObject proxy(cx, proxy_); > + JS::RootedId id(cx, id_); You're inconsistent about using 'JS::' here @@ +330,5 @@ > + uint32_t cache = js::GetReservedSlot(proto, 0).toPrivateUint32(); > + if (MOZ_LIKELY(cache == USE_CACHE)) { > +#ifdef DEBUG > + bool isClean; > + JS_ASSERT(ProtoIsClean(cx, proto, properties, &isClean) && isClean); MOZ_ASSERT @@ +333,5 @@ > + bool isClean; > + JS_ASSERT(ProtoIsClean(cx, proto, properties, &isClean) && isClean); > +#endif > + } > + else if (cache == CHECK_CACHE) { Cuddle @@ +344,5 @@ > + return true; > + } > + js::SetReservedSlot(proto, 0, JS::PrivateUint32Value(USE_CACHE)); > + } > + else { Cuddle @@ +406,5 @@ > + jsval idval; > + double array_index; > + int32_t i; > + if (!::JS_IdToValue(cx, id, &idval) || > + !::JS_ValueToNumber(cx, idval, &array_index) || Do we need to propagate an exception if these fail? @@ +414,5 @@ > + > + return i; > +} > + > +} // namespace dom ::: dom/bindings/DOMJSProxyHandler.h @@ +70,5 @@ > + MethodsAndAttributes mMethodsAndAttributes; > + MethodsAndAttributes mChromeMethodsAndAttributes; > +}; > + > +class DOMProxyHandler : public js::BaseProxyHandler { { on the next line @@ +73,5 @@ > + > +class DOMProxyHandler : public js::BaseProxyHandler { > +public: > + DOMProxyHandler(const DOMProxyHandlerData& aData) : js::BaseProxyHandler(ProxyFamily()), > + mData(aData) I prefer DOMProxyHandler(const DOMProxyHandlerData& aData) : js::BaseProxyHandler(ProxyFamily()) , mData(aData) @@ +79,5 @@ > + } > + > + bool getPropertyDescriptor(JSContext* cx, JSObject* proxy, jsid id, bool set, > + JSPropertyDescriptor* desc); > + bool defineProperty(JSContext *cx, JSObject *proxy, jsid id, * to the left all around here @@ +90,5 @@ > + using js::BaseProxyHandler::obj_toString; > + > + static js::Shape* GetProtoShape(JSObject* obj) > + { > + //JS_ASSERT(objIsList(obj)); MOZ_ASSERT, and bug number for uncommenting them? @@ +91,5 @@ > + > + static js::Shape* GetProtoShape(JSObject* obj) > + { > + //JS_ASSERT(objIsList(obj)); > + return (js::Shape*)js::GetProxyExtra(obj, JSPROXYSLOT_PROTOSHAPE).toPrivate(); static_cast @@ +100,5 @@ > + js::SetProxyExtra(obj, JSPROXYSLOT_PROTOSHAPE, JS::PrivateValue(shape)); > + } > + static JSObject* GetExpandoObject(JSObject* obj) > + { > + NS_ASSERTION(IsDOMProxy(obj), "expected a DOM proxy object"); MOZ_ASSERT @@ +132,5 @@ > + > +inline int32_t > +GetArrayIndexFromId(JSContext *cx, jsid id) > +{ > + if (NS_LIKELY(JSID_IS_INT(id))) MOZ_LIKELY @@ +133,5 @@ > +inline int32_t > +GetArrayIndexFromId(JSContext *cx, jsid id) > +{ > + if (NS_LIKELY(JSID_IS_INT(id))) > + return JSID_TO_INT(id); {} @@ +137,5 @@ > + return JSID_TO_INT(id); > + if (NS_LIKELY(id == s_length_id)) > + return -1; > + if (NS_LIKELY(JSID_IS_ATOM(id))) { > + JSAtom *atom = JSID_TO_ATOM(id); * to the left @@ +140,5 @@ > + if (NS_LIKELY(JSID_IS_ATOM(id))) { > + JSAtom *atom = JSID_TO_ATOM(id); > + jschar s = *js::GetAtomChars(atom); > + if (NS_LIKELY((unsigned)s >= 'a' && (unsigned)s <= 'z')) > + return -1; {} @@ +169,5 @@ > + > +JSObject * > +EnsureExpandoObject(JSContext *cx, JSObject *obj); > + > +} // namespace dom ::: js/xpconnect/src/XPCJSRuntime.cpp @@ +2049,2 @@ > if (!ok) > return false; The && is inconsistent; do two checks? ::: js/xpconnect/src/XPCWrappedNativeScope.cpp @@ +228,5 @@ > + native = static_cast<XPCWrappedNative*>(wn.get())->GetIdentityObject(); > + } > + else { > + native = nsnull; > + } No parens

Don't forget about HTMLPropertiesCollection and PropertyNodeList

Peter, I'm having a really hard time understanding how all this fits together without seeing an example of generated code. :( Would you mind attaching one? I should probably have asked for this earlier...

Comment on attachment 636915 [details] [diff] [review] v1 I'm up to about "class CGProxyIndexedGetter" in the parts I really understand. The rest I don't get yet. :( Comments so far: >+++ b/dom/bindings/BindingUtils.h >+enum DOMClassType { >+ eNonDOMObject = -1, >+ eRegularDOMObject = DOM_OBJECT_SLOT, >+ eProxyDOMObject = DOM_PROXY_OBJECT_SLOT >+}; There's no guarantee that the slots won't be the same, long term. If that's OK, it's worth documenting here (and perhaps renaming the enum to be DOMObjectSlot or something?). If we do need these to be different, we should have a static assert to that effect. >+UnwrapDOMObject(JSObject* obj, DOMClassType classType) >+ MOZ_ASSERT(classType == eRegularDOMObject || classType == eProxyDOMObject, >+ "Don't pass non-DOM objects to this function"); We should probably also assert that classType matches obj. >+template <class T, DOMClassType classType> >+inline T* >+UnwrapDOMObject(JSObject* obj) Why do we need this template? What does it gain us over the two-arg form of UnwrapDOMObject? >+UnwrapDOMObject(JSObject* obj) >+ js::Class* clasp = js::Valueify(js::GetObjectJSClass(obj)); clasp = js::GetObjectClass(obj); I would slightly prefer that we test for IsDOMClass first, I think. that said, is this function even needed anymore? Do we still have callers who are not using the two-argument (or template with two template arguments) version? >+ if (js::IsObjectProxyClass(clasp) || js::IsFunctionProxyClass(clasp)) { >+ return UnwrapDOMObject<T, eProxyDOMObject>(obj); Don't we need to check the proxy family here? >+UnwrapDOMObjectToISupports(JSObject* obj, nsISupports*& result) >+{ >+ js::Class* clasp = js::Valueify(js::GetObjectJSClass(obj)); js::GetObjectClass. >+ if (js::IsObjectProxyClass(clasp) || js::IsFunctionProxyClass(clasp)) { Again, may be better to test IsDOMClass() first. >+// Only use this with a new DOM binding object (either proxy or regular). >+inline const DOMClass* >+GetDOMClass(JSObject* obj) This is only used in debugging code. Mind making it #ifdef DEBUG with a comment that the ordering of if checks should be considered if using in opt code? And given it's debug-only, any reason to not implement it in terms of the two-argument GetDOMClass? >+inline DOMClassType >+GetDOMClass(JSObject* obj, const DOMClass*& result) I think you can implement UnwrapDOMObjectToISupports in terms of this, like so: inline bool UnwrapDOMObjectToISupports(JSObject* obj, nsISupports*& result) { const DOMClass* clasp; DOMClassType type = GetDOMClass(obj, clasp); if (type == eNonDOMObject || !clasp->mDOMObjectIsISupports) { return false; } return UnwrapDOMObject<nsISupports>(obj, type); } which should be no slower than what we have now, I hope, but avoid having multiple copies of the "find the object type" logic. >+ js::Class* clasp = js::Valueify(js::GetObjectJSClass(obj)); js::GetObjectClass. >+ if (js::IsObjectProxyClass(clasp) || js::IsFunctionProxyClass(clasp)) { Please check IsDOMClass() first. > UnwrapObject(JSContext* cx, JSObject* obj, U& value) I _think_ that if we reorder the checks above this should get no slower for non-proxy objects. Maybe worth checking. >+IsNull(Nullable<T>& obj) const ref, please. >+ClearWrapper(T* p, void*) Why is this needed? Seems unused..... And if this is _not_ needed, we can nix the template bits on the other ClearWrapper. >+ nsWrapperCache* cache; >+ CallQueryInterface(p, &cache); This will fail if called on a p which does not QI to nsWrapperCache. Or should that not happen? Can we actually get a |p| here which is an nsWrapperCache but doesn't inherit from it? I guess it depends on what the "self" type is in the finalize hook? >+++ b/dom/bindings/Codegen.py >+InvalidateProtoShape_add(JSContext* cx, JSHandleObject obj, JSHandleId id, jsval* vp) >+{ >+ // We need to make sure that this is our prototype object, not a DOM object >+ // with this prototype object on its prototype chain. I don't get this comment, or the code. And yes, I know you just copied this... But the add/set hooks of a JSClass will only ever be called on instances of that class. So you shouldn't need the JS_InstanceOf check here (though it can be asserted if desired. >@@ -512,46 +556,57 @@ class CGAbstractMethod(CGThing): >+ def __init__(self, descriptor, name, returnType, args, inline=False, static=False, decorators=[], templateArgs=None): Please document the new arguments. >+ if isinstance(decorators, list): >+ self.decorators = list(decorators) >+ else: >+ assert isinstance(decorators, str) >+ self.decorators = decorators.split() I'd rather we just always used a list. Though given that it's only used for MOZ_ALWAYS_INLINE, I would slightly prefer that we just had an alwaysInline argument that implies inline (so you don't have to avoid passing inline and then override declare()). > class MethodDefiner(PropertyDefiner): >+ # FIXME We should be able to check for special operations without an >+ # indentifier. For now we check if the name starts with __ Please file the bug? >+ call = (" JSObject *proto = " + call.replace("\n ", "\n ") + "\n" I would somewhat prefer us using CGWrapper with reindent here. Same for the else branch of this if/else. >+class CGIsMethod(CGAbstractMethod): I'm not sure what ends up actually calling this. Nothing in this patch or the generated code I see so far seems to... Is this something used later? Does it need to be non-static? Does it need to be present for non-proxies? >+class CGWrapMethod(CGAbstractMethod): >+ # XXX can we wrap if we don't have an interface prototype object? >+ assert descriptor.interface.hasInterfacePrototypeObject() No, we can't. So the assert is fine. >+ return " return Wrap(aCx, aScope, static_cast<%s*>(aObject), static_cast<nsWrapperCache*>(aObject), aTriedToWrap);" % (self.descriptor.nativeType) Why do you need the casts? Please document. > def getRetvalDeclarationForType(returnType, descriptorProvider, Please document the new return value setup. >@@ -2668,27 +2770,32 @@ class CGPerSignatureCall(CGThing): >+ def getArgumentNames(self): I'd prefer getArguments, since this is getting not just the names by also the actual IDLArguments too. >+ args = [] >+ for (i, a) in enumerate(self.arguments): >+ args.append((a, "arg" + str(i))) >+ return args Could this be written like so: return [ (a, "arg" + str(i)) for (i, a) in enumerate(self.arguments) ] ? If so, is that better or worse? ;) > def getErrorReport(self): >+ % (toStringBool(not self.descriptor.workers), >+ self.descriptor.interface.identifier.name, >+ self.idlNode.identifier.name)) Please fix the indent there. The last two lines should start even with the 't' of "toStringBool". >+class ClassConstructor(ClassItem): Please document (including expected types of arguments, please). >+ def getDecorators(self, declaring): The second arg seems to be unused. >+class CGProxySpecialOperation(CGPerSignatureCall): >+ def __init__(self, descriptor, operation): >+ signature = operation.signatures()[0] Can we assert that len(operation.signatures()) == 1? >+ CGPerSignatureCall.__init__(self, returnType, "", arguments, nativeName, >+ False, descriptor, operation, >+ len(arguments)) Please document that the len(arguments) is so that the CGPerSignatureCall won't do any argument conversion of its own. >+ argument = arguments[1] Document that arguments[0] is the string or int indicating what we're setting. >+ def getErrorReport(self): >+ return CGIfWrapper(errorReport, >+ "rv.ErrorCode() != NS_ERROR_DOM_INDEX_SIZE_ERR") This could really use a comment explaining what it's doing. >+ if self.returnType.nullable(): >+ wrap = ("if (!IsNull(result)) {\n" + >+ CGIndenter(CGGeneric(wrap)).define() + "\n" + >+ "}") >+ elif self.isFallible(): Why is this an elif? Can we not have fallible methods returning nullable things? I think this should be just an if. >+ wrap = ("if (rv.ErrorCode() != NS_ERROR_DOM_INDEX_SIZE_ERR) {\n" + Please document where NS_ERROR_DOM_INDEX_SIZE_ERR comes in.

> Would you mind attaching one? Or better yet, add some tests to the TestCodeGen.webidl which will exercise this code and then I can just look at the result?

Rebased on top of 769911. Added a boolean found argument to getters. Merged with changes to iterators. (In reply to :Ms2ger from comment #1) > JS::Value > And here > {} > true Code is going away. > identifier Done. > {} Done. > This shouldn't be necessary It was when I hit it. > Update this Done. > This is where we fix isAny(), right? Separate bug. > ... whose name is given by the |object| argument. Done. > Shouldn't need the parens anymore Done. > return [(a, "arg%i" % i) for (i, a) in enumerate(self.arguments)] Done. > Some documentation for this class, please > And what these arguments all are Done. > if items: > if body: > Same Nope. > Documentation, please Done. > returnType, arguments = signature Done. > Are those things that could go into the parser? (Followup is fine.) Removed. > Newlines between those functions, please? Nope. > return [(a, a.identifier.name) for a in self.arguments] Done, but pfff. > Hmm, what's this for? Changed. > And this Changed. > These all want docs Done. > Unwrap? Done. > MOZ_ASSERT Done. > JS::Value Done. > Followup to use ConvertJSValueToString, please Done. > * to the left Done. > Same Done. > We have a GetLength() without outparam now, no? Done. > Is this filed? Bug number please. Filed bug 772869. > Again Done. > And here Done. > {} Done. > MOZ_ASSERT? Done. > * to the left Done. > MOZ_ASSERT? Done. > Same about GetLength() > MOZ_ASSERT, or use UINT_TO_JSVAL, I guess Code is going away. > MOZ_ASSERT Done. > Can we do that? Code is going away. > No need Done. > This is InternJSString from BindingUtils.h, AFAICT. Used that. > MOZ_ASSERT? Done. > xpc::CompartmentPrivate* priv = xpc::GetCompartmentPrivate(obj); Done. > JS::Value (or Value, I guess) Done. > static_cast<>, and * to the left Done. > Star Done. > Cast Nope. > A documentation comment about what this does? > You're inconsistent about using 'JS::' here > MOZ_ASSERT > Cuddle > Cuddle Code is going away. > Do we need to propagate an exception if these fail? No. > // namespace dom Done. > { on the next line Done. > I prefer > > DOMProxyHandler(const DOMProxyHandlerData& aData) > : js::BaseProxyHandler(ProxyFamily()) > , mData(aData) Done, but with trailing ,. > * to the left all around here Done. > MOZ_ASSERT, and bug number for uncommenting them? > static_cast Code is going away. > MOZ_ASSERT Done. > MOZ_LIKELY Done. > {} Done. > * to the left Done. > {} Done. > The && is inconsistent; do two checks? Huh? Left as is, since it looks fine to me. > No parens Done. > There's no guarantee that the slots won't be the same, long term. If that's > OK, it's worth documenting here (and perhaps renaming the enum to be > DOMObjectSlot or something?). If we do need these to be different, we > should have a static assert to that effect. I documented that they can be the same and renamed to DOMObjectSlot. > We should probably also assert that classType matches obj. Done. > Why do we need this template? What does it gain us over the two-arg form of > UnwrapDOMObject? Removed templated version. > that said, is this function even needed anymore? Do we still have callers > who are not using the two-argument (or template with two template arguments) > version? Changed the remaining caller and removed. > js::GetObjectClass. Done. > Again, may be better to test IsDOMClass() first. Done. > This is only used in debugging code. Mind making it #ifdef DEBUG with a > comment that the ordering of if checks should be considered if using in opt > code? Xray code will use it. I made it check for IsDOMClass first. Not sure if that's what you mean about the if checks? > I think you can implement UnwrapDOMObjectToISupports in terms of this, like > so: Done. > js::GetObjectClass. Done. > Please check IsDOMClass() first. Done. > I _think_ that if we reorder the checks above this should get no slower for > non-proxy objects. Maybe worth checking. I agree, didn't check. Let me know if I should. > const ref, please. Code is going away. > Why is this needed? Seems unused..... And if this is _not_ needed, we can > nix the template bits on the other ClearWrapper. It is used in the finalizer for example for HTMLCollection or NodeList. > This will fail if called on a p which does not QI to nsWrapperCache. Or > should that not happen? It should not happen, we only use it is wrapperCache is set to True in the config (or rather if it is not set to False). > Can we actually get a |p| here which is an nsWrapperCache but doesn't > inherit from it? I guess it depends on what the "self" type is in the > finalize hook? Yes, see for example nsIHTMLCollection or nsINodeList. I have patches somewhere to make it possible for those to inherit from nsWrapperCache, but they're probably bitrotted at this point. > I don't get this comment, or the code. And yes, I know you just copied > this... But the add/set hooks of a JSClass will only ever be called on > instances of that class. So you shouldn't need the JS_InstanceOf check here > (though it can be asserted if desired. This code is going away, but the JS_InstanceOf check was definitely needed (in fact we missed that when I first landed this code and crashed running Dromaeo because of it). > Please document the new arguments. Done. > Though given that it's only used for MOZ_ALWAYS_INLINE, I would slightly > prefer that we just had an alwaysInline argument that implies inline (so you > don't have to avoid passing inline and then override declare()). Done, but without making alwaysInline imply inline. I don't want to inline in the header but have the function in the .cpp (which is also why I need to override declare). > Please file the bug? Bug 772822. > I would somewhat prefer us using CGWrapper with reindent here. Same for > the else branch of this if/else. Code is going away. > I'm not sure what ends up actually calling this. Nothing in this patch or > the generated code I see so far seems to... Is this something used later? > Does it need to be non-static? We do use it in later patches in nsDOMClassInfo to check whether an object is a HTMLCollection binding. > Does it need to be present for non-proxies? It seemed useful in general, but can remove it for non-proxies. Let me know if you still want that. > No, we can't. So the assert is fine. Removed comment. > Why do you need the casts? Please document. Removed the casts. > Please document the new return value setup. Done. > I'd prefer getArguments, since this is getting not just the names by also > the actual IDLArguments too. Done. > Could this be written like so: > > return [ (a, "arg" + str(i)) for (i, a) in enumerate(self.arguments) ] Done. > Please fix the indent there. The last two lines should start even with the > 't' of "toStringBool". Done. > Please document (including expected types of arguments, please). Done. > The second arg seems to be unused. Removed. > Can we assert that len(operation.signatures()) == 1? Added the assert. I think that according to the spec there could be multiple regular operations with one being special, but I don't see how that would fit into our setup with signatures. > Please document that the len(arguments) is so that the CGPerSignatureCall > won't do any argument conversion of its own. Done. > Document that arguments[0] is the string or int indicating what we're > setting. Done. > This could really use a comment explaining what it's doing. Code is going away. > Why is this an elif? Can we not have fallible methods returning nullable > things? I think this should be just an if. Code is going away. > Please document where NS_ERROR_DOM_INDEX_SIZE_ERR comes in. Code is going away. Still working on hooking up the tests.

Comment on attachment 641232 [details] [diff] [review] v2 Review of attachment 641232 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/Codegen.py @@ +524,5 @@ > > inline should be True to generate an inline method, whose body is > part of the declaration. > > + alwaysInline should be True to generate an inline method anotated with Nit: annotated

Comment on attachment 641232 [details] [diff] [review] v2 > It should not happen, we only use it is wrapperCache is set to True Can we assert that the QI gives a non-null cache then? > @@ -509,49 +520,64 @@ class CGAbstractMethod(CGThing): We should probably assert that not inline or not alwaysInline here, since we do that elif thing. I'm not sure I made the alwaysInline/inline thing clear. I think we should consider doing this: self.inline = inline or alwaysInline. If we did that, would you still need the declare/define overriding and the bodyInHeader stuff? > It seemed useful in general, but can remove it for non-proxies. Let me know > if you still want that. It just seems like extra codesize. I guess it's not a big deal either way. >+class CGDOMJSProxyHandlerData(CGThing): >+ args += " &%s::ProxyHandlerData.%s" % (toBindingNamespace(self.descriptor.prototypeChain[-2]), name) This assumes that proxies only have a proxy-based proto. I don't see why that should be true... I don't think we're making that assumption anywhere else. If I make TestIndexedGetterInterface inherit from TestInterface, we generate code that doesn't compile here. I'd be fine with explicitly making the situation when an interface with a getteror setter that's a child of an interface without such will fail codegen for now, I guess. And vice versa too. And filing a followup on making this stuff work. Presumably unifying the Xrays for proxies and non-proxies will make this work better? >+class CGDOMJSProxyHandler_getOwnPropertyDescriptor(ClassMethod): >+ writable = toStringBool(self.descriptor.operations['IndexedSetter']) >+ fillDescriptor = "FillPropertyDescriptor(desc, proxy, %s);\nreturn true;" % writable This is backwards. The last arg for FillPropertyDescriptor() is "readonly". Add a test for this? Please file followup bugs on all the cases we don't seem to handle here (creator and setter not being the same operation, [Unforgeable], [OverrideBuiltins], [NamedPropertiesObject], whatever else needs filing. >+ if self.descriptor.operations['NamedSetter']: >+ setOrIndexedGet += " if (JSID_IS_STRING(id)) {\n" Technically, nothing in WebIDL says that the id needs to be a string in the JS engine jsid sense. You can have a named setter with "-1" or "5" as a supported property name, as far as I can tell. A followup on this is probably fine. >+ writable = toStringBool(self.descriptor.operations['NamedSetter']) >+ fillDescriptor = "FillPropertyDescriptor(desc, proxy, %s);\nreturn true;" % writable Again, this is backwards. >+ " nsDependentString name;\n" This will need to be a FakeDependentString now. Same elsewhere in this patch. I wonder whether it makes sense to factor out the code chunk that deals with expandos into a function. It looks identical for all these cases, as far as I can tell. We should add some tests for cases when an interface with an indexed getter inherits from an interface from an indexed setter and so forth. >+class CGDOMJSProxyHandler_defineProperty(ClassMethod): >+ CGProxyNamedSetter(self.descriptor).define() + "\n" + You need to indent that (so CGIndenter() around the CGProxyNamedSetter). I'm not quite sure I follow the code here. Per spec, if I read it right, a define on an object with an indexed getter but no indexed setter or creator is supposed to Reject. It looks like the code here will silently define on the expando instead. That looks very odd to me. Or is this handled by the "set" case in getOwnPropertyDescriptor somehow? I really wish our hooks matched up to the spec ones better. Would sure make them easier to reason about. :( >+class CGDOMJSProxyHandler_get(ClassMethod): >+ // Even if we don't have this index, we don't forward the >+ // get on to our expando object. This behavior doesn't seem to match the spec. Is the spec wrong? And if so, is it wrong again, or just still wrong and waiting to be updated? Specifically in WebIDL 4.6.2 if the index is not a supported property index we fall through to the default [[GetOwnProperty]]. And domcore says, for NodeList for example: The object's supported property indices are the numbers in the range zero to one less than the number of nodes represented by the collection. Or is this OK because per spec we shouldn't allow setting indexed expandos at all, so we can hide them if they _do_ get set? If so, that's worth a good set of comments. Maybe a general comment in the codegen about how we translate the spec's various requirements into our proxy API? >+class CGDOMJSProxyHandler_getElementIfPresent(ClassMethod): >+ indexToId = """jsid id; >+if (!JS_IndexToId(cx, index, &id)) { >+ return false; >+}""" I don't see anything using "id" below. Why do we need it and this whole indexToId thing? >+class CGDOMJSProxyHandler_GetPropertyOnPrototype(ClassMethod): Why is this codegenned per interface? Looks to me like this doesn't depend on what interface we're working on; it can just be a helper function we write in C++ directly. >+class CGDOMJSProxyHandler_HasPropertyOnPrototype(ClassMethod): Likewise. > class CGDescriptor(CGThing): >+ m.isMethod() and not m.isStatic() and not m.identifier.name[:2] == "__"]) Can we add an isSpecialOperation() to IDLMethod and call that here or something? Similar for MethodDefiner above. Then we can fix the impl of isSpecialOperation() later as needed. >+++ b/dom/bindings/Configuration.py >+ 'IndexedDeleter': None, Do we support deleters? If not, probably better to throw on them here.... >+ iface.setUserData('hasProxyDescendant', True) No one reads that. Why do we need to do it? Is the idea to eventually only do checks for proxies during this-unwrapping if hasProxyDescendant? >+++ b/dom/bindings/DOMJSClass.h >+ const prototypes::ID mInterfaceChain[prototypes::id::_ID_Count]; I know that's what we used to have, but this is broken. It gives us room in every interface chain for _all_ the interfaces we know about... We should change the codegen for PrototypeList.h to output some constant whose value is config.maxProtoChainLength, and use that constant here. Followup bug fine for this. >+ // We store the DOM object in a reserved slot whose index is mNativeSlot. This is no longer true (and wasn't true even before this patch, I guess). Please update to reflect reality about where the DOM object is stored? >+++ b/dom/bindings/DOMJSProxyHandler.cpp >+ js::SetListBaseInformation((void*) &HandlerFamily, js::JSSLOT_PROXY_EXTRA + JSPROXYSLOT_EXPANDO); This could use a comment... >+DOMProxyHandler::EnsureExpandoObject(JSContext* cx, JSObject* obj) >+ JS_SetPrivate(expando, js::GetProxyPrivate(obj).toPrivate()); Why does the expando need to store a backpointer to the C++ object? Either remove or document, please. >+DOMProxyHandler::defineProperty(JSContext* cx, JSObject* proxy, jsid id, This could use documenting (e.g. why we examine desc->setter if JSPROP_GETTER is set!). Ideally cite the spec section we're trying to implement here... Because I still think this defineProperty stuff is somehow busted. :( >+DOMProxyHandler::enumerate(JSContext* cx, JSObject* proxy, AutoIdVector& props) This looks like it enumerates our named props (once we actually enumerate them) before the proto props. Is that correct? For that matter, what will happen when there's the same name in our named props and on the proto? At least put something in bug 772869 about updating this method? >+DOMProxyHandler::has(JSContext* cx, JSObject* proxy, jsid id, bool* bp) >+ // We have the property ourselves; no need to worry about our >+ // prototype chain. >+ if (*bp) { That comment would make more sense inside the if body. >+DOMProxyHandler::obj_toString(JSContext* cx, const char* className) >+ size_t nchars = 9 + strlen(className); /* 9 for "[object ]" */ s/9/sizeof("[object ]")-1/ or something? >+IdToInt32(JSContext* cx, jsid id) We still have a followup bug somewhere to make this suck less via better JS apis, right? >+++ b/dom/bindings/DOMJSProxyHandler.h >+ MOZ_ALWAYS_INLINE bool ShouldCacheProtoShape(JSContext* cx, JSObject* proto, bool* shouldCache) This looks unused. >+ MOZ_ALWAYS_INLINE bool NativeGet(JSContext* cx, JSObject* proxy, JSObject* proto, jsid id, bool* found, JS::Value* vp) Likewise. >+ static bool ShouldCacheProtoShape(JSContext* cx, JSObject* proto, const MethodsAndAttributes& properties, bool* shouldCache); >+ static bool NativeGet(JSContext* cx, JSObject* proxy, JSObject* proto, const MethodsAndAttributes& properties, jsid id, bool* found, JS::Value* vp); And these. >+++ b/js/xpconnect/src/XPCWrappedNative.cpp >+++ b/js/xpconnect/src/xpcpublic.h >+ (IS_SLIM_WRAPPER(wrapper) || cache->IsDOMBinding() || So this is basically relying on IsDOMBinding() implying !NeedsSOW()? r=me with the above nits.