Summary: | [Streams API] Implement ReadableStreamController constructor | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||||||
Component: | WebCore Misc. | Assignee: | youenn fablet <youennf> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | commit-queue | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | 143608 | ||||||||||||
Bug Blocks: | 138967 | ||||||||||||
Attachments: |
|
Description
youenn fablet
2015-04-15 03:43:39 PDT
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. |