sample foo.idl: [ Conditional=XXX, ] interface Foo { [RaisesException] void fooIt(sequence<unsigned long>? seq); } Generated JSFoo.cpp code: EncodedJSValue JSC_HOST_CALL jsFooPrototypeFunctionFooIt(ExecState* exec) { JSValue thisValue = exec->hostThisValue(); JSFoo* castedThis = jsDynamicCast<JSFoo*>(thisValue); if (!castedThis) return throwVMTypeError(exec); ASSERT_GC_OBJECT_INHERITS(castedThis, JSFoo::info()); Foo& impl = castedThis->impl(); if (exec->argumentCount() != 1) return throwVMError(exec, createNotEnoughArgumentsError(exec)); ExceptionCode ec = 0; Vector<unsigned> seq(toNativeArray<unsigned>(exec, exec->argument(0))); if (exec->hadException()) return JSValue::encode(jsUndefined()); impl.fooIt(seq, ec); setDOMException(exec, ec); return JSValue::encode(jsUndefined()); } In JSDOMBindings.h, "toNativeArray" bails out early if JSValue's (i.e. argument0) getObject is null, throwing a VM error "Value is not a sequence". According to WebIDL [1] there is no restriction for nullable sequences. [1] http://www.w3.org/TR/WebIDL/#dfn-nullable-type
Do we have one of these nullable sequences in our bindings already? Can we make a regression test showing that null doesn’t work for that?
Created attachment 228758 [details] patch I choose to add a bool parameter, named "acceptsNullable", to JSDOMBindings::toNativeArray method, and set it to "true" from the generated code upon the presence of "?" in the idl definition. It might be that we should simply make toNativeArray to accept "null" arrays always, similarly to toRefPtrNativeArray. Please let me know.
(In reply to comment #1) > Do we have one of these nullable sequences in our bindings already? Can we make a regression test showing that null doesn’t work for that? I am afraid it is not there yet, but this discrepancy between toNativeArray and toRefPtrNative array was what brought my attention to this.
I’m not really happy doing this patch when there is no binding yet that uses it. It prevents us from doing any real testing. I suggest we hold off on this until we are actually using it in at least one binding.
(In reply to comment #4) > I’m not really happy doing this patch when there is no binding yet that uses it. It prevents us from doing any real testing. I suggest we hold off on this until we are actually using it in at least one binding. Hi Darin. Would you be ok with a less intrusive approach/patch, where no changes to the perl code is made, as proposed in comment #2? Reasons I particularly insist are: - it seems logical to me to make toNativeArray in pair with toRefPtrNativeArray in that sense (acceptance of null as a valid sequence valid). Particularly, toRefPtrNativeArray does take "null" ok as an empty sequence, while toNativeArray does not. - there is support for nullable sequences in in the generated code for this***, so it seems logical to make both methods actually called by it (i.e. toNativeArray and toRefPtrNativeArray) to support it. - there are a couple of specifications making use of this nullable sequence: 1) https://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#options-object- concept 2) http://www.khronos.org/registry/webcl/specs/latest/1.0/#3.5 if you look at void enqueueNDRangeKernel(..., sequence<CLuint>? globalWorkOffset, sequence<CLuint> globalWorkSize, optional sequence<CLuint>? localWorkSize, optional sequence<WebCLEvent>? eventWaitList, optional WebCLEvent? event); "eventWaitList" goes through toRefPtrNativeArray code path and can take "null" just fine. globalWorkSize does through toNativeArray, and fails to accept it. ***see that if (arg.isNull() || ...) gets added.
Created attachment 228919 [details] simpler patch Patch that does not change the code generator, and simply makes toNativeArray in pair with toRefPtrNativeArray. Darin, would you mind to look?
Comment on attachment 228919 [details] simpler patch This change is completely reasonable. But it needs a real regression test, testing its effect on an actual binding. If this change has no effect on any actual generated binding, we should not make the change at this time, until it does have an effect.
(In reply to comment #5) > - there are a couple of specifications making use of this nullable sequence: > > 1) > https://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#options-object- > concept > > 2) http://www.khronos.org/registry/webcl/specs/latest/1.0/#3.5 > if you look at > void enqueueNDRangeKernel(..., > sequence<CLuint>? globalWorkOffset, > sequence<CLuint> globalWorkSize, > optional sequence<CLuint>? localWorkSize, > optional sequence<WebCLEvent>? eventWaitList, > optional WebCLEvent? event); > > "eventWaitList" goes through toRefPtrNativeArray code path and can take "null" just fine. globalWorkSize does through toNativeArray, and fails to accept it. We should land this change when we are landing one of these bindings, so the change has an actual effect on WebKit rather than a theoretical one.
Comment on attachment 228919 [details] simpler patch Ok. Removing the r? flags.
Created attachment 232133 [details] simpler patch (2) - with WebSocket.idl changes It turns out WebSocket.idl can benefit from this patch. Changes incorporated.
Comment on attachment 232133 [details] simpler patch (2) - with WebSocket.idl changes Darin, could you look again?
(In reply to comment #11) > (From update of attachment 232133 [details]) > Darin, could you look again? Friendly re-ping.
Comment on attachment 232133 [details] simpler patch (2) - with WebSocket.idl changes View in context: https://bugs.webkit.org/attachment.cgi?id=232133&action=review > Source/WebCore/bindings/scripts/test/TestObj.idl:212 > + void methodWithAndWithoutNullableSequence(sequence<unsigned long> arrayArg, sequence<unsigned long>? nullableArrayArg); > + void methodWithAndWithoutNullableSequence2(unsigned long[] arrayArg, unsigned long[]? nullableArrayArg); Should not have four spaces after "void".
Comment on attachment 232133 [details] simpler patch (2) - with WebSocket.idl changes View in context: https://bugs.webkit.org/attachment.cgi?id=232133&action=review > Source/WebCore/Modules/websockets/WebSocket.idl:37 > + Constructor(DOMString url, [Default=Undefined] optional sequence<DOMString>? protocols), Do we have test coverage for this?
Fixed by <https://webkit.org/b/131240>.