Bug 143564 – [Streams API] ReadableStreamReader should not be exposed

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

We should set the NoInterfaceObject to ReadableStreamReader as per https://streams.spec.whatwg.org/#globals

[youenn fablet]

Created attachment 250440 [details]
Patch

[Build Bot]

Comment on attachment 250440 [details]
Patch

Attachment 250440 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5129403331772416

New failing tests:
js/dom/global-constructors-attributes.html

[Build Bot]

Created attachment 250441 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 250440 [details]
Patch

Attachment 250440 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6013501948559360

New failing tests:
js/dom/global-constructors-attributes.html

[Build Bot]

Created attachment 250442 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[youenn fablet]

Created attachment 250443 [details]
Fixing expectations

[Benjamin Poulain]

Comment on attachment 250443 [details]
Fixing expectations

View in context: https://bugs.webkit.org/attachment.cgi?id=250443&action=review

> Source/WebCore/ChangeLog:9
> +        Made ReadableStreamReader not exposed to JS.
> +        Removed ReadableStreamReader constructor.

What am I missing? https://streams.spec.whatwg.org/#reader-constructor

[youenn fablet]

(In reply to comment #7)
> Comment on attachment 250443 [details]
> Fixing expectations
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250443&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        Made ReadableStreamReader not exposed to JS.
> > +        Removed ReadableStreamReader constructor.
> 
> What am I missing? https://streams.spec.whatwg.org/#reader-constructor

Well, the WebIDL spec says that the Constructor keyword should not be used with NoInterfaceObject.
The streams spec says that users should not use the constructor, since there is getReader().
There seems to be little value in keeping it.
But the implementation will also work with the constructor I guess.

[youenn fablet]

Created attachment 250604 [details]
Rebasing according new tests

[Build Bot]

Comment on attachment 250604 [details]
Rebasing according new tests

Attachment 250604 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6053874439815168

New failing tests:
js/dom/global-constructors-attributes.html

[Build Bot]

Created attachment 250606 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 250604 [details]
Rebasing according new tests

Attachment 250604 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4545507896590336

New failing tests:
js/dom/global-constructors-attributes.html

[Build Bot]

Created attachment 250609 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[youenn fablet]

Created attachment 250672 [details]
Fixing expectations

[youenn fablet]

(In reply to comment #8)
> (In reply to comment #7)
> > Comment on attachment 250443 [details]
> > Fixing expectations
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=250443&action=review
> > 
> > > Source/WebCore/ChangeLog:9
> > > +        Made ReadableStreamReader not exposed to JS.
> > > +        Removed ReadableStreamReader constructor.
> > 
> > What am I missing? https://streams.spec.whatwg.org/#reader-constructor
> 
> Well, the WebIDL spec says that the Constructor keyword should not be used
> with NoInterfaceObject.
> The streams spec says that users should not use the constructor, since there
> is getReader().
> There seems to be little value in keeping it.
> But the implementation will also work with the constructor I guess.

I was wrong.
The binding generator is not generating the constructor property if NoInterfaceObject is set, even if CustomConstructor is set.

[Benjamin Poulain]

Comment on attachment 250672 [details]
Fixing expectations

View in context: https://bugs.webkit.org/attachment.cgi?id=250672&action=review

Ok.

> Source/WebCore/ChangeLog:9
> +        Made ReadableStreamReader not exposed to JS.
> +        Removed ReadableStreamReader constructor as the binding generator does not use it when interface is not exposed.

Can you please reference https://streams.spec.whatwg.org/#globals here?

> LayoutTests/js/dom/global-constructors-attributes-expected.txt:-1005
> -PASS Object.getOwnPropertyDescriptor(global, 'ReadableStreamReader').value is ReadableStreamReader
> -PASS Object.getOwnPropertyDescriptor(global, 'ReadableStreamReader').hasOwnProperty('get') is false
> -PASS Object.getOwnPropertyDescriptor(global, 'ReadableStreamReader').hasOwnProperty('set') is false
> -PASS Object.getOwnPropertyDescriptor(global, 'ReadableStreamReader').enumerable is false
> -PASS Object.getOwnPropertyDescriptor(global, 'ReadableStreamReader').configurable is true

You could cover this by getting the constructor property of the object you get from getReader().

I think getting enumerable is valuable.

> LayoutTests/streams/reference-implementation/readable-stream-reader-expected.txt:-3
> -PASS ReadableStreamReader constructor should get a ReadableStream object as argument 

You need to adapt those tests. 

You could create a new reader after getting the constructor of an existing reader object.

> LayoutTests/streams/reference-implementation/readable-stream-reader-expected.txt:9
> +FAIL Constructing a ReadableStreamReader directly should fail if the stream is already locked (via direct construction) assert_throws: constructing directly should fail function "function () { new ReadableStreamReader(rs); }" did not throw
> +FAIL Getting a ReadableStreamReader via getReader should fail if the stream is already locked (via direct construction) assert_throws: getReader() should fail function "function () { rs.getReader(); }" did not throw
> +FAIL Constructing a ReadableStreamReader directly should fail if the stream is already locked (via getReader) assert_throws: constructing directly should fail function "function () { new ReadableStreamReader(rs); }" did not throw

What about those?

[youenn fablet]

(In reply to comment #16)
> Comment on attachment 250672 [details]
> Fixing expectations
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250672&action=review
> 
> Ok.
> 
> > Source/WebCore/ChangeLog:9
> > +        Made ReadableStreamReader not exposed to JS.
> > +        Removed ReadableStreamReader constructor as the binding generator does not use it when interface is not exposed.
> 
> Can you please reference https://streams.spec.whatwg.org/#globals here?
> 
> > LayoutTests/js/dom/global-constructors-attributes-expected.txt:-1005
> > -PASS Object.getOwnPropertyDescriptor(global, 'ReadableStreamReader').value is ReadableStreamReader
> > -PASS Object.getOwnPropertyDescriptor(global, 'ReadableStreamReader').hasOwnProperty('get') is false
> > -PASS Object.getOwnPropertyDescriptor(global, 'ReadableStreamReader').hasOwnProperty('set') is false
> > -PASS Object.getOwnPropertyDescriptor(global, 'ReadableStreamReader').enumerable is false
> > -PASS Object.getOwnPropertyDescriptor(global, 'ReadableStreamReader').configurable is true
> 
> You could cover this by getting the constructor property of the object you
> get from getReader().
> 
> I think getting enumerable is valuable.
> 
> > LayoutTests/streams/reference-implementation/readable-stream-reader-expected.txt:-3
> > -PASS ReadableStreamReader constructor should get a ReadableStream object as argument 
> 
> You need to adapt those tests. 
> 
> You could create a new reader after getting the constructor of an existing
> reader object.
> 
> > LayoutTests/streams/reference-implementation/readable-stream-reader-expected.txt:9
> > +FAIL Constructing a ReadableStreamReader directly should fail if the stream is already locked (via direct construction) assert_throws: constructing directly should fail function "function () { new ReadableStreamReader(rs); }" did not throw
> > +FAIL Getting a ReadableStreamReader via getReader should fail if the stream is already locked (via direct construction) assert_throws: getReader() should fail function "function () { rs.getReader(); }" did not throw
> > +FAIL Constructing a ReadableStreamReader directly should fail if the stream is already locked (via getReader) assert_throws: constructing directly should fail function "function () { new ReadableStreamReader(rs); }" did not throw
> 
> What about those?

A similar strategy to implement reader constructor could be done as https://bugs.webkit.org/show_bug.cgi?id=143752
That would solve those issues properly.
Can we make that as another patch?

[youenn fablet]

Created attachment 253738 [details]
Adding NoInterfaceObject to IDL

[WebKit Commit Bot]

Comment on attachment 253738 [details]
Adding NoInterfaceObject to IDL

Clearing flags on attachment: 253738

Committed r184955: <http://trac.webkit.org/changeset/184955>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.