Bug 143752

Created attachment 250783 [details] Patch

Created attachment 251024 [details] Improving test coverage

Comment on attachment 251024 [details] Improving test coverage View in context: https://bugs.webkit.org/attachment.cgi?id=251024&action=review Any way to change the bindings to support Custom constructor function instead? > Source/WebCore/bindings/js/JSReadableStreamControllerCustom.cpp:127 > + constructor = JSReadableStreamControllerConstructor::create(exec->vm(), JSReadableStreamControllerConstructor::createStructure(exec->vm(), globalObject, globalObject->objectPrototype()), globalObject); Couldn't you just use JSFunction::create()? Why subclass DOMConstructorObject here?

(In reply to comment #3) > Comment on attachment 251024 [details] > Improving test coverage > > View in context: > https://bugs.webkit.org/attachment.cgi?id=251024&action=review > > Any way to change the bindings to support Custom constructor function > instead? That is certainly doable, but: - WebIDL spec is not allowing to put Constructor and NoInterfaceObject . Since CustomConstructor is semantically equivalent to Constructor, that would - It might be better to agree on how the code looks like and then generate it automatically - Is it a case that happens often enough to add it into the binding generator? - I am no expert in the bindings (althouhgh I'd like to have some time to study it). > > > Source/WebCore/bindings/js/JSReadableStreamControllerCustom.cpp:127 > > + constructor = JSReadableStreamControllerConstructor::create(exec->vm(), JSReadableStreamControllerConstructor::createStructure(exec->vm(), globalObject, globalObject->objectPrototype()), globalObject); > > Couldn't you just use JSFunction::create()? I wrote the code the way it is so that the class name would be settable. A call to toString would get "[object ReadableStreamControllerConstructor]" which seems to align with ReadableStreamController objects. Going the function way, we would get "function() {}". I guess that in most case, this does not change anything, so I am fine either way. > Why subclass DOMConstructorObject here? This is just closer to currently binding generated code. We could go with DOMConstructorObject parent class or above I guess.

Created attachment 253860 [details] Using CustomConstructor

Comment on attachment 253860 [details] Using CustomConstructor View in context: https://bugs.webkit.org/attachment.cgi?id=253860&action=review > LayoutTests/streams/readable-stream.html:42 > + assert_equals(propDesc.writable, false, 'method should be writable'); Some typos I forgot after copy/pasting the code here: rename 'method' to 'property', 'writable' to 'not writable' and 'enumerable' to 'not enumerable' I plan to fix that when landing patch.

Created attachment 253976 [details] Patch for landing

Comment on attachment 253976 [details] Patch for landing Clearing flags on attachment: 253976 Committed r185039: <http://trac.webkit.org/changeset/185039>

All reviewed patches have been landed. Closing bug.