Bug 843264. Allow returning sequences of non-primitive types from callback methods. r=mccr8
authorBoris Zbarsky <bzbarsky@mit.edu>
Wed, 24 Apr 2013 14:59:14 -0400
changeset 140733 96803f97e1ee214f70544cb2db2b3d179e594063
parent 140732 be8219e1ae5faea153a52882dc21e08272a61d1a
child 140734 9129688d03ded873225bc3b13ef01a653d5dc6fa
push id2579
push userakeybl@mozilla.com
push dateMon, 24 Jun 2013 18:52:47 +0000
treeherdermozilla-beta@b69b7de8a05a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmccr8
bugs843264
milestone23.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 843264. Allow returning sequences of non-primitive types from callback methods. r=mccr8
dom/bindings/Codegen.py
dom/bindings/test/TestCodeGen.webidl
dom/bindings/test/TestJSImplGen.webidl
--- a/dom/bindings/Codegen.py
+++ b/dom/bindings/Codegen.py
@@ -2265,31 +2265,33 @@ if (NS_FAILED(rv) || !wrappedJS) {
 nsCOMPtr<${nativeType}> tmp = do_QueryObject(wrappedJS.get());
 if (!tmp) {
 ${codeOnFailure}
 }
 ${target} = tmp.forget();""").substitute(self.substitution)
 
 # If this function is modified, modify CGNativeMember.getArg and
 # CGNativeMember.getRetvalInfo accordingly.  The latter cares about the decltype
-# and holdertype we end up using.
+# and holdertype we end up using, because it needs to be able to return the code
+# that will convert those to the actual return value of the callback function.
 def getJSToNativeConversionTemplate(type, descriptorProvider, failureCode=None,
                                     isDefinitelyObject=False,
                                     isMember=False,
                                     isOptional=False,
                                     invalidEnumValueFatal=True,
                                     defaultValue=None,
                                     treatNullAs="Default",
                                     treatUndefinedAs="Default",
                                     isEnforceRange=False,
                                     isClamp=False,
                                     isNullOrUndefined=False,
                                     exceptionCode=None,
                                     lenientFloatCode=None,
-                                    allowTreatNonCallableAsNull=False):
+                                    allowTreatNonCallableAsNull=False,
+                                    isCallbackReturnValue=False):
     """
     Get a template for converting a JS value to a native object based on the
     given type and descriptor.  If failureCode is given, then we're actually
     testing whether we can convert the argument to the desired type.  That
     means that failures to convert due to the JS value being the wrong type of
     value need to use failureCode instead of throwing exceptions.  Failures to
     convert that are due to JS exceptions (from toString or valueOf methods) or
     out of memory conditions need to throw exceptions no matter what
@@ -2323,16 +2325,20 @@ def getJSToNativeConversionTemplate(type
     value is out of range.
 
     If lenientFloatCode is not None, it should be used in cases when
     we're a non-finite float that's not unrestricted.
 
     If allowTreatNonCallableAsNull is true, then [TreatNonCallableAsNull]
     extended attributes on nullable callback functions will be honored.
 
+    If isCallbackReturnValue is true, then the declType may be adjusted to make
+    it easier to return from a callback.  Since that type is never directly
+    observable by any consumers of the callback code, this is OK.
+
     The return value from this function is a tuple consisting of four things:
 
     1)  A string representing the conversion code.  This will have template
         substitution performed on it as follows:
 
           ${val} replaced by an expression for the JS::Value in question
           ${valPtr} is a pointer to the JS::Value in question
           ${holderName} replaced by the holder's name, if any
@@ -2485,41 +2491,42 @@ def getJSToNativeConversionTemplate(type
         # deal with this, we typically map WebIDL sequences to our Sequence
         # type, which is in fact memmovable.  The one exception is when we're
         # passing in a sequence directly as an argument without any sort of
         # optional or nullable complexity going on.  In that situation, we can
         # use an AutoSequence instead.  We have to keep using Sequence in the
         # nullable and optional cases because we don't want to leak the
         # AutoSequence type to consumers, which would be unavoidable with
         # Nullable<AutoSequence> or Optional<AutoSequence>.
-        if isMember or isOptional or nullable:
+        if isMember or isOptional or nullable or isCallbackReturnValue:
             sequenceClass = "Sequence"
         else:
             sequenceClass = "AutoSequence"
 
         (elementTemplate, elementDeclType,
          elementHolderType, dealWithOptional) = getJSToNativeConversionTemplate(
             elementType, descriptorProvider, isMember=True,
-            exceptionCode=exceptionCode, lenientFloatCode=lenientFloatCode)
+            exceptionCode=exceptionCode, lenientFloatCode=lenientFloatCode,
+            isCallbackReturnValue=isCallbackReturnValue)
         if dealWithOptional:
             raise TypeError("Shouldn't have optional things in sequences")
         if elementHolderType is not None:
             raise TypeError("Shouldn't need holders for sequences")
 
         typeName = CGTemplatedType(sequenceClass, elementDeclType)
         sequenceType = typeName.define()
         if nullable:
             typeName = CGTemplatedType("Nullable", typeName)
             arrayRef = "const_cast<Nullable<" + sequenceType + " >& >(${declName}).SetValue()"
         else:
             arrayRef = "${declName}"
         # If we're optional or a member, the const will come from the Optional
         # or whatever we're a member of.
         mutableTypeName = typeName
-        if not isOptional and not isMember:
+        if not (isOptional or isMember or isCallbackReturnValue):
             typeName = CGWrapper(typeName, pre="const ")
 
         # NOTE: Keep this in sync with variadic conversions as needed
         templateBody = ("""JSObject* seq = &${val}.toObject();\n
 if (!IsArrayLike(cx, seq)) {
 %s
 }
 uint32_t length;
@@ -2769,24 +2776,24 @@ for (uint32_t i = 0; i < length; ++i) {
         assert not isEnforceRange and not isClamp
 
         descriptor = descriptorProvider.getDescriptor(
             type.unroll().inner.identifier.name)
 
         if (descriptor.interface.isCallback() and
             descriptor.interface.identifier.name != "EventListener"):
             if descriptor.workers:
-                if type.nullable():
+                if type.nullable() or isCallbackReturnValue:
                     declType = CGGeneric("JSObject*")
                 else:
                     declType = CGGeneric("NonNull<JSObject>")
                 conversion = "  ${declName} = &${val}.toObject();\n"
             else:
                 name = descriptor.interface.identifier.name
-                if type.nullable():
+                if type.nullable() or isCallbackReturnValue:
                     declType = CGGeneric("nsRefPtr<%s>" % name);
                 else:
                     declType = CGGeneric("OwningNonNull<%s>" % name)
                 conversion = (
                     "  bool inited;\n"
                     "  ${declName} = new %s(cx, ${obj}, &${val}.toObject(), &inited);\n"
                     "  if (!inited) {\n"
                     "%s\n"
@@ -2794,18 +2801,21 @@ for (uint32_t i = 0; i < length; ++i) {
             template = wrapObjectTemplate(conversion, type,
                                           "${declName} = nullptr",
                                           failureCode)
             return (template, declType, None, isOptional)
 
         # This is an interface that we implement as a concrete class
         # or an XPCOM interface.
 
-        # Allow null pointers for nullable types and old-binding classes
-        argIsPointer = type.nullable() or type.unroll().inner.isExternal()
+        # Allow null pointers for nullable types and old-binding classes, and
+        # use an nsRefPtr or raw pointer for callback return values to make
+        # them easier to return.
+        argIsPointer = (type.nullable() or type.unroll().inner.isExternal() or
+                        isCallbackReturnValue)
 
         # Sequences and non-worker callbacks have to hold a strong ref to the
         # thing being passed down.
         forceOwningType = (descriptor.interface.isCallback() and
                            not descriptor.workers) or isMember
 
         typeName = descriptor.nativeType
         typePtr = typeName + "*"
@@ -3878,17 +3888,17 @@ def dictionaryNeedsCx(dictionary, descri
     return (any(typeNeedsCx(m.type, descriptorProvider) for m in dictionary.members) or
         (dictionary.parent and dictionaryNeedsCx(dictionary.parent, descriptorProvider)))
 
 # Returns a tuple consisting of a CGThing containing the type of the return
 # value, or None if there is no need for a return value, and a boolean signaling
 # whether the return value is passed in an out parameter.
 #
 # Whenever this is modified, please update CGNativeMember.getRetvalInfo as
-# needed
+# needed to keep the types compatible.
 def getRetvalDeclarationForType(returnType, descriptorProvider,
                                 resultAlreadyAddRefed,
                                 isMember=False):
     if returnType is None or returnType.isVoid():
         # Nothing to declare
         return None, False
     if returnType.isPrimitive() and returnType.tag() in builtinNames:
         result = CGGeneric(builtinNames[returnType.tag()])
@@ -7772,19 +7782,19 @@ class CGNativeMember(ClassMethod):
                 # The decl is an OwningNonNull or nsRefPtr, depending
                 # on whether we're nullable.
                 returnCode = "return ${declName}.forget();"
             elif type.nullable():
                 # Decl is a raw pointer
                 returnCode = ("NS_IF_ADDREF(${declName});\n"
                               "return ${declName};")
             else:
-                # Decl is a NonNull.
-                returnCode = ("NS_ADDREF(${declName}.Ptr());\n"
-                              "return ${declName}.Ptr();")
+                # Decl is a non-null raw pointer.
+                returnCode = ("NS_ADDREF(${declName});\n"
+                              "return ${declName};")
             return result.define(), "nullptr", returnCode
         if type.isCallback():
             return ("already_AddRefed<%s>" % type.unroll().identifier.name,
                     "nullptr", "return ${declName}.forget();")
         if type.isAny():
             return "JS::Value", "JS::UndefinedValue()", "return ${declName};"
         if type.isObject():
             if type.nullable():
@@ -7794,35 +7804,29 @@ class CGNativeMember(ClassMethod):
             return "JSObject*", "nullptr", returnCode
         if type.isSpiderMonkeyInterface():
             if type.nullable():
                 returnCode = "return ${declName} ? ${declName}->Obj() : nullptr;"
             else:
                 returnCode = ("return static_cast<%s&>(${declName}).Obj();" % type.name)
             return "JSObject*", "nullptr", returnCode
         if type.isSequence():
+            # If we want to handle sequence-of-sequences return values, we're
+            # going to need to fix example codegen to not produce nsTArray<void>
+            # for the relevant argument...
             assert not isMember
-            # Outparam.  Copying is a bit suboptimal, but hard to avoid: We
-            # could SwapElements if we cast away const, but even then our
-            # Sequence is an auto-array, and will tend to copy.  The good news
-            # is that callbacks that return sequences should be pretty rare
-            # anyway, and if we have to we can rejigger the codegen here to use
-            # a non-const non-auto array if it's ever necessary.
-            # In any case, we only support sequences of primitive values for
-            # returning from a callback, for now.
-            if not type.unroll().isPrimitive():
-                return "void", ""
+            # Outparam.
             if type.nullable():
                 returnCode = ("if (${declName}.IsNull()) {\n"
                               "  retval.SetNull();\n"
                               "} else {\n"
-                              "  retval.SetValue() = ${declName}.Value();\n"
+                              "  retval.SetValue().SwapElements(${declName}.Value());\n"
                               "}")
             else:
-                returnCode = "retval = ${declName};"
+                returnCode = "retval.SwapElements(${declName});"
             return "void", "", returnCode
         raise TypeError("Don't know how to declare return value for %s" %
                         type)
 
     def getArgs(self, returnType, argList):
         args = [self.getArg(arg) for arg in argList]
         # Now the outparams
         if returnType.isString():
@@ -8710,17 +8714,18 @@ class CallbackMember(CGNativeMember):
             # wrapping things into our current compartment (that of mCallback)
             # is what we want.
             "obj": "nullptr"
             }
 
         convertType = instantiateJSToNativeConversionTemplate(
             getJSToNativeConversionTemplate(self.retvalType,
                                             self.descriptor,
-                                            exceptionCode=self.exceptionCode),
+                                            exceptionCode=self.exceptionCode,
+                                            isCallbackReturnValue=True),
             replacements)
         assignRetval = string.Template(
             self.getRetvalInfo(self.retvalType,
                                False)[2]).substitute(replacements)
         return convertType.define() + "\n" + assignRetval
 
     def getArgConversions(self):
         # Just reget the arglist from self.originalSig, because our superclasses
--- a/dom/bindings/test/TestCodeGen.webidl
+++ b/dom/bindings/test/TestCodeGen.webidl
@@ -15,16 +15,25 @@ interface TestRenamedInterface {
 
 callback interface TestCallbackInterface {
   readonly attribute long foo;
   attribute DOMString bar;
   void doSomething();
   long doSomethingElse(DOMString arg, TestInterface otherArg);
   void doSequenceLongArg(sequence<long> arg);
   void doSequenceStringArg(sequence<DOMString> arg);
+  sequence<long> getSequenceOfLong();
+  sequence<TestInterface> getSequenceOfInterfaces();
+  sequence<TestInterface>? getNullableSequenceOfInterfaces();
+  sequence<TestInterface?> getSequenceOfNullableInterfaces();
+  sequence<TestInterface?>? getNullableSequenceOfNullableInterfaces();
+  sequence<TestCallbackInterface> getSequenceOfCallbackInterfaces();
+  sequence<TestCallbackInterface>? getNullableSequenceOfCallbackInterfaces();
+  sequence<TestCallbackInterface?> getSequenceOfNullableCallbackInterfaces();
+  sequence<TestCallbackInterface?>? getNullableSequenceOfNullableCallbackInterfaces();
 };
 
 callback interface TestSingleOperationCallbackInterface {
   TestInterface doSomething(short arg, sequence<double> anotherArg);
 };
 
 enum TestEnum {
   "a",
--- a/dom/bindings/test/TestJSImplGen.webidl
+++ b/dom/bindings/test/TestJSImplGen.webidl
@@ -136,25 +136,24 @@ interface TestJSImplInterface {
   void passOptionalSelfWithDefault(optional TestJSImplInterface? arg = null);
 
   // Non-wrapper-cache interface types
   [Creator]
   TestNonWrapperCacheInterface receiveNonWrapperCacheInterface();
   [Creator]
   TestNonWrapperCacheInterface? receiveNullableNonWrapperCacheInterface();
 
-  // Can't return sequences of interfaces from callback interface methods.  See bug 843264.
-  //[Creator]
-  //sequence<TestNonWrapperCacheInterface> receiveNonWrapperCacheInterfaceSequence();
-  //[Creator]
-  //sequence<TestNonWrapperCacheInterface?> receiveNullableNonWrapperCacheInterfaceSequence();
-  //[Creator]
-  //sequence<TestNonWrapperCacheInterface>? receiveNonWrapperCacheInterfaceNullableSequence();
-  //[Creator]
-  //sequence<TestNonWrapperCacheInterface?>? receiveNullableNonWrapperCacheInterfaceNullableSequence();
+  [Creator]
+  sequence<TestNonWrapperCacheInterface> receiveNonWrapperCacheInterfaceSequence();
+  [Creator]
+  sequence<TestNonWrapperCacheInterface?> receiveNullableNonWrapperCacheInterfaceSequence();
+  [Creator]
+  sequence<TestNonWrapperCacheInterface>? receiveNonWrapperCacheInterfaceNullableSequence();
+  [Creator]
+  sequence<TestNonWrapperCacheInterface?>? receiveNullableNonWrapperCacheInterfaceNullableSequence();
 
   // Non-castable interface types
   IndirectlyImplementedInterface receiveOther();
   IndirectlyImplementedInterface? receiveNullableOther();
   // Callback interface ignores 'resultNotAddRefed'. See bug 843272.
   //IndirectlyImplementedInterface receiveWeakOther();
   //IndirectlyImplementedInterface? receiveWeakNullableOther();
 
@@ -215,41 +214,38 @@ interface TestJSImplInterface {
   sequence<long>? receiveNullableSequence();
   sequence<long?> receiveSequenceOfNullableInts();
   sequence<long?>? receiveNullableSequenceOfNullableInts();
   void passSequence(sequence<long> arg);
   void passNullableSequence(sequence<long>? arg);
   void passSequenceOfNullableInts(sequence<long?> arg);
   void passOptionalSequenceOfNullableInts(optional sequence<long?> arg);
   void passOptionalNullableSequenceOfNullableInts(optional sequence<long?>? arg);
-  // Can't return sequences of interfaces from callback interface methods.  See bug 843264.
-  //sequence<TestJSImplInterface> receiveCastableObjectSequence();
-  //sequence<TestCallbackInterface> receiveCallbackObjectSequence();
-  //sequence<TestJSImplInterface?> receiveNullableCastableObjectSequence();
-  //sequence<TestCallbackInterface?> receiveNullableCallbackObjectSequence();
-  //sequence<TestJSImplInterface>? receiveCastableObjectNullableSequence();
-  //sequence<TestJSImplInterface?>? receiveNullableCastableObjectNullableSequence();
-  // Callback interface ignores 'resultNotAddRefed'. See bug 843272.
-  //sequence<TestJSImplInterface> receiveWeakCastableObjectSequence();
-  //sequence<TestJSImplInterface?> receiveWeakNullableCastableObjectSequence();
-  //sequence<TestJSImplInterface>? receiveWeakCastableObjectNullableSequence();
-  //sequence<TestJSImplInterface?>? receiveWeakNullableCastableObjectNullableSequence();
+  sequence<TestJSImplInterface> receiveCastableObjectSequence();
+  sequence<TestCallbackInterface> receiveCallbackObjectSequence();
+  sequence<TestJSImplInterface?> receiveNullableCastableObjectSequence();
+  sequence<TestCallbackInterface?> receiveNullableCallbackObjectSequence();
+  sequence<TestJSImplInterface>? receiveCastableObjectNullableSequence();
+  sequence<TestJSImplInterface?>? receiveNullableCastableObjectNullableSequence();
+  sequence<TestJSImplInterface> receiveWeakCastableObjectSequence();
+  sequence<TestJSImplInterface?> receiveWeakNullableCastableObjectSequence();
+  sequence<TestJSImplInterface>? receiveWeakCastableObjectNullableSequence();
+  sequence<TestJSImplInterface?>? receiveWeakNullableCastableObjectNullableSequence();
   void passCastableObjectSequence(sequence<TestJSImplInterface> arg);
   void passNullableCastableObjectSequence(sequence<TestJSImplInterface?> arg);
   void passCastableObjectNullableSequence(sequence<TestJSImplInterface>? arg);
   void passNullableCastableObjectNullableSequence(sequence<TestJSImplInterface?>? arg);
   void passOptionalSequence(optional sequence<long> arg);
   void passOptionalNullableSequence(optional sequence<long>? arg);
   void passOptionalNullableSequenceWithDefaultValue(optional sequence<long>? arg = null);
   void passOptionalObjectSequence(optional sequence<TestJSImplInterface> arg);
   void passExternalInterfaceSequence(sequence<TestExternalInterface> arg);
   void passNullableExternalInterfaceSequence(sequence<TestExternalInterface?> arg);
 
-  // Can't return sequences of interfaces from callback interface methods.  See bug 843264.
-  //sequence<DOMString> receiveStringSequence();
+  sequence<DOMString> receiveStringSequence();
   // Callback interface problem.  See bug 843261.
   //void passStringSequence(sequence<DOMString> arg);
   // "Can't handle sequence member 'any'; need to sort out rooting issues"
   //sequence<any> receiveAnySequence();
   //sequence<any>? receiveNullableAnySequence();
 
   void passSequenceOfSequences(sequence<sequence<long>> arg);
   //sequence<sequence<long>> receiveSequenceOfSequences();