Bug 143752 - [Streams API] Implement ReadableStreamController constructor
Summary: [Streams API] Implement ReadableStreamController constructor
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on: 143608
Blocks: 138967
  Show dependency treegraph
 
Reported: 2015-04-15 03:43 PDT by youenn fablet
Modified: 2015-05-31 04:29 PDT (History)
1 user (show)

See Also:


Attachments
Patch (10.71 KB, patch)
2015-04-15 03:51 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Improving test coverage (11.48 KB, patch)
2015-04-17 08:13 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Using CustomConstructor (6.67 KB, patch)
2015-05-28 13:48 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (6.62 KB, patch)
2015-05-31 03:36 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2015-04-15 03:43:39 PDT
ReadableStreamController should not be visible but its constructor can be retrieved from ReadableStreamController object prototype.
This constructor should throw exception.
Comment 1 youenn fablet 2015-04-15 03:51:55 PDT
Created attachment 250783 [details]
Patch
Comment 2 youenn fablet 2015-04-17 08:13:02 PDT
Created attachment 251024 [details]
Improving test coverage
Comment 3 Benjamin Poulain 2015-04-21 20:45:26 PDT
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?
Comment 4 youenn fablet 2015-04-28 05:59:49 PDT
(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.
Comment 5 youenn fablet 2015-05-28 13:48:32 PDT
Created attachment 253860 [details]
Using CustomConstructor
Comment 6 youenn fablet 2015-05-28 14:17:12 PDT
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.
Comment 7 youenn fablet 2015-05-31 03:36:05 PDT
Created attachment 253976 [details]
Patch for landing
Comment 8 WebKit Commit Bot 2015-05-31 04:29:45 PDT
Comment on attachment 253976 [details]
Patch for landing

Clearing flags on attachment: 253976

Committed r185039: <http://trac.webkit.org/changeset/185039>
Comment 9 WebKit Commit Bot 2015-05-31 04:29:49 PDT
All reviewed patches have been landed.  Closing bug.