Bug 138967 – [Streams API] [Meta] Implement ReadableStream

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

[youenn fablet]

Goal is to support https://streams.spec.whatwg.org/#rs-model

[youenn fablet]

Created attachment 242150 [details]
WIP

[youenn fablet]

(In reply to comment #1)
> Created attachment 242150 [details]
> WIP

This patch implements basic readable stream functionality (read, wait, closed and cancel). It does not cover pipe.

WIP patch in bug 138968 allows instantiating ReadableStream from XHR.

Next planned step is to add constructor, ie validate that UnderlyingSource can be implemented using JS-provided callbacks.

[Sergio Villar Senin]

Comment on attachment 242150 [details]
WIP

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

Looking good so far. Just a couple of nits.

> Source/WebCore/Modules/streams/ReadableStream.cpp:163
> +    else if (m_state == ERRORED) {

No need for the else after a block with an early return.

> Source/WebCore/Modules/streams/ReadableStream.cpp:170
> +    else if (m_state == WAITING)

Ditto.

[youenn fablet]

Created attachment 243012 [details]
Patch

[WebKit Commit Bot]

Attachment 243012 [details] did not pass style-queue:


ERROR: Source/WebCore/Configurations/FeatureDefines.xcconfig:0:  Any changes made to FeatureDefines should be made to all of them (changed file does not match Source/WebKit/mac/Configurations/FeatureDefines.xcconfig).  [featuredefines/equality] [5]
ERROR: Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:0:  Any changes made to FeatureDefines should be made to all of them (changed file does not match Source/WebKit/mac/Configurations/FeatureDefines.xcconfig).  [featuredefines/equality] [5]
ERROR: Source/WebKit2/Configurations/FeatureDefines.xcconfig:0:  Any changes made to FeatureDefines should be made to all of them (changed file does not match Source/WebKit/mac/Configurations/FeatureDefines.xcconfig).  [featuredefines/equality] [5]
Total errors found: 3 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

(In reply to comment #4)
> Created attachment 243012 [details]
> Patch

Patch implements the JS constructor and updates according the latest spec.

[Sergio Villar Senin]

Comment on attachment 243012 [details]
Patch

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

Awesome work!

The bindings stuff is a bit far from my area of expertise, so someone else should review it.

As an overall comment, we should reduce the amount of comments in ReadableStream.cpp, they make the code quite difficult to read. I understand that they're quotes from the spec but you'd better include one link per method to the spec at the most.

Also what's the plan with all the FIXME's. Are you planning to submit new versions of the patch?

> Source/WebCore/CMakeLists.txt:910
> +

Extra line.

> Source/WebCore/Modules/streams/ReadableStream.cpp:51
> +PassRefPtr<ReadableStream> ReadableStream::create(ScriptExecutionContext* scriptExecutionContext, PassRefPtr<UnderlyingSource> source)

AFAIK the ScriptExecutionContext argument is mandatory so it should be a reference instead of a pointer.

> Source/WebCore/Modules/streams/ReadableStream.cpp:59
> +ReadableStream::ReadableStream(ScriptExecutionContext* scriptExecutionContext, PassRefPtr<UnderlyingSource> source)

Ditto (pointer vs reference).

> Source/WebCore/Modules/streams/ReadableStream.cpp:213
> +    bool shouldApplyBackpressureBool = m_source->shouldApplyBackpressure(queueSize, m_shouldRethrow);

Better "shouldApplyBackpressureAsBool".

> Source/WebCore/Modules/streams/ReadableStream.cpp:250
> +    if (shouldApplyBackpressure)

No need for a local variable here, just use the returned value inside the if.

> Source/WebCore/Modules/streams/ReadableStream.cpp:265
> +PassRefPtr<JSC::ArrayBuffer> ReadableStream::read(ExceptionCode& ec)

Let's not abbreviate unnecessarily, use exceptionCode instead.

> Source/WebCore/Modules/streams/ReadableStream.cpp:275
> +        ec = 1;

Where does this magic number come from?

> Source/WebCore/Modules/streams/ReadableStream.cpp:283
> +    ASSERT(m_totalQueueSize);

Although this is perfectly fine I think adding "> 0" is much more expressive.

> Source/WebCore/Modules/streams/ReadableStream.cpp:287
> +    // m_queue may be empty if the stream is about passing JSValue.

Could you rephrase this?. Also merge this comment with the following one in a single paragraph.

> Source/WebCore/Modules/streams/ReadableStream.cpp:288
> +    // In that case, the js binding layer will get the JSValue from the js queue managerd by the corresponding JSReadableStreamUnderlyingSource.

managed.

> Source/WebCore/Modules/streams/ReadableStream.cpp:323
> +    createReadableStreamCloseFunction();

What about: m_state == Errored ? createReadableStreamErrorFunction() : createReadableStreamCloseFunction();

> Source/WebCore/Modules/streams/ReadableStream.cpp:333
> +        m_draining = true;

What happens if we call this function from another state?. Is that possible? Should not be possible then we should have an ASSERT at the beginning of the method.

> Source/WebCore/Modules/streams/UnderlyingSource.h:45
> +    virtual bool shouldApplyBackpressure(unsigned, bool&/*shouldRethrow*/) { return true; }

Let's not use comments here, if we need the argument name because the type does not have any special meaning then we should explicitly include it.

> Source/WebCore/Modules/streams/UnderlyingSource.h:48
> +    virtual bool cancel(const String&) { return true; }

As it isn't straightforward to know what that String is we should probably append cancelReason or whatever argument name here.

> Source/WebCore/bindings/js/JSReadableStreamCustom.cpp:128
> +        // Resolve the promise asynchronously

This comment is not needed.

> Source/WebCore/bindings/js/JSReadableStreamCustom.cpp:138
> +    return JSValue::encode((JSValue)jsReadableStream);

We don't normally use C-style casts.

> Source/WebCore/bindings/js/JSReadableStreamUnderlyingSource.cpp:160
> +    return (unsigned)value;

Ditto cast.

> Source/WebCore/bindings/js/JSReadableStreamUnderlyingSource.cpp:250
> +    return (RefPtr<UnderlyingSource>(adoptRef(source))).release();

You can do this in a single line.

> Source/WebCore/bindings/js/JSReadableStreamUnderlyingSource.cpp:256
> +    jsReadableStream->impl().notifyCancel();

Ditto.

> Source/WebCore/bindings/js/JSReadableStreamUnderlyingSource.cpp:263
> +    JSGlobalObject* globalObject = exec->callee()->globalObject();

We don't really need these two local variables, do everything in the return.

> Source/WebCore/bindings/js/JSReadableStreamUnderlyingSource.cpp:278
> +    JSGlobalObject* globalObject = exec->callee()->globalObject();

Ditto.

> Source/WebCore/bindings/js/JSReadableStreamUnderlyingSource.cpp:435
> +    JSGlobalObject* globalObject = exec->callee()->globalObject();

Ditto.

> Source/WebCore/bindings/js/JSReadableStreamUnderlyingSource.cpp:450
> +    JSGlobalObject* globalObject = exec->callee()->globalObject();

Ditto.

> LayoutTests/fast/streams/streams-api-cancel-error.html:11
> +description('Check ReadableStream cancel')

Nit, missing '.' at the end of the description. Apart from that the description could be a little bit more verbose.

> LayoutTests/fast/streams/streams-api-cancel-throw-exception.html:11
> +description('Check ReadableStream cancel')

Ditto.

> LayoutTests/fast/streams/streams-api-cancel.html:11
> +description('Check ReadableStream cancel')

Ditto.

> LayoutTests/fast/streams/streams-api-pull-close.html:11
> +description('Check ReadableStream pull')

Ditto.

> LayoutTests/fast/streams/streams-api-pull.html:11
> +description('Check ReadableStream pull')

Ditto.

> LayoutTests/fast/streams/streams-api-start-promise.html:11
> +description('Check ReadableStream start promise')

Ditto.

> LayoutTests/fast/streams/streams-api-start-throws-expected.txt:6
> +PASS var potato = new ReadableStream({ start:  function(enqueue, close, error) { throw new Error("error") } }) threw exception TypeError: Abrupt start call..

Two dots at the end of the message.

> LayoutTests/fast/streams/streams-api-start-throws.html:11
> +description('Check ReadableStream start throwing')

Ditto.

> LayoutTests/fast/streams/streams-api-start.html:13
> +description('Check ReadableStream start')

Ditto.

> LayoutTests/fast/streams/streams-api-wrong-arguments-expected.txt:10
> +PASS potato = new ReadableStream({ pull: { } }) threw exception TypeError: ReadableStream constructor object pull property should be functions..

Two dots at the end of the messages.

[youenn fablet]

(In reply to comment #7)
> Comment on attachment 243012 [details]
> Patch
> 

Thanks for the review.
I will process all entries and will update the patch accordingly.

> The bindings stuff is a bit far from my area of expertise, so someone else
> should review it.

Having a check at the JS binding stuff would be really good.
Do you know somebody that could do that?

> As an overall comment, we should reduce the amount of comments in
> ReadableStream.cpp, they make the code quite difficult to read. I understand
> that they're quotes from the spec but you'd better include one link per
> method to the spec at the most.

OK.

> Also what's the plan with all the FIXME's. Are you planning to submit new
> versions of the patch?

A possible plan would be to upstream to WebKit trunk something close to this patch, at least in terms of features: updating according the review(s) and making it work on Mac and compile on Win.

We would then add more tests and more features as additional patches.
As part of these, the FIXME's would be removed progressively.

[Xabier Rodríguez Calvar]

(In reply to comment #7)
> Awesome work!

Thanks!

> As an overall comment, we should reduce the amount of comments in
> ReadableStream.cpp, they make the code quite difficult to read. I understand
> that they're quotes from the spec but you'd better include one link per
> method to the spec at the most.

Would you mind reconsidering this comment? Some parts of code I saw are full of comments like this, specially the MSE, EME, multimedia, etc. They always go step by step to clarify that things are done like that because of the spec. I tried to follow the same approach:

Examples:

https://trac.webkit.org/browser/trunk/Source/WebCore/Modules/encryptedmedia/MediaKeys.cpp
https://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLMediaElement.cpp#L880

> Also what's the plan with all the FIXME's. Are you planning to submit new
> versions of the patch?

As Youenn said, we plan to do follow-ups.

> > Source/WebCore/Modules/streams/ReadableStream.cpp:283
> > +    ASSERT(m_totalQueueSize);
> Although this is perfectly fine I think adding "> 0" is much more expressive.
> 
We can change this, but accoding to the coding style:

Null, false and zero
--------------------
3. Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.

Againg ,thanks for the review. I'll probably have more comments as I begin to work in it.

[Sergio Villar Senin]

(In reply to comment #9)
> (In reply to comment #7)
> > Awesome work!
> 
> Thanks!
> 
> > As an overall comment, we should reduce the amount of comments in
> > ReadableStream.cpp, they make the code quite difficult to read. I understand
> > that they're quotes from the spec but you'd better include one link per
> > method to the spec at the most.
> 
> Would you mind reconsidering this comment? Some parts of code I saw are full
> of comments like this, specially the MSE, EME, multimedia, etc. They always
> go step by step to clarify that things are done like that because of the
> spec. I tried to follow the same approach:
> 
> Examples:
> 
> https://trac.webkit.org/browser/trunk/Source/WebCore/Modules/encryptedmedia/
> MediaKeys.cpp
> https://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLMediaElement.
> cpp#L880

I can give you hundreds of counterexamples :).

The idea is that code should be self-explanatory, that's why for example, we use long names for variables or methods. Webkit is full of spec implementations, we don't want to replicate what the specs say. It's enough to mention the spec if you wish but frankly it's quite difficult to follow the code with all those comments.

[youenn fablet]

Comment on attachment 243012 [details]
Patch

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

>> Source/WebCore/Modules/streams/ReadableStream.cpp:275
>> +        ec = 1;
> 
> Where does this magic number come from?

I will set it to TypeError for now on.
I think we will need to improve the error management as we progressively add more test cases.

>> Source/WebCore/Modules/streams/ReadableStream.cpp:333
>> +        m_draining = true;
> 
> What happens if we call this function from another state?. Is that possible? Should not be possible then we should have an ASSERT at the beginning of the method.

This may actually happen, for instance, within a pull() JS callback, close() is called twice or close() is called after error().
In that case, this function is a no-op.

>> LayoutTests/fast/streams/streams-api-cancel-error.html:11
>> +description('Check ReadableStream cancel')
> 
> Nit, missing '.' at the end of the description. Apart from that the description could be a little bit more verbose.

OK.
By the way, I am wondering whether it might be better to use testharness.js for those tests.
That would make tests easier to share with other test environments/browsers.
If a test suite is built for streams, it might be also easier to remove duplicate tests.

>> LayoutTests/fast/streams/streams-api-start-throws-expected.txt:6
>> +PASS var potato = new ReadableStream({ start:  function(enqueue, close, error) { throw new Error("error") } }) threw exception TypeError: Abrupt start call..
> 
> Two dots at the end of the message.

Internal error string messages are terminated with a dot currently.
Should we remove the dot?

[youenn fablet]

Created attachment 243357 [details]
Adding Mac/Win compilation

[Build Bot]

Comment on attachment 243357 [details]
Adding Mac/Win compilation

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

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

[Build Bot]

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

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

[youenn fablet]

Created attachment 243360 [details]
Fixing one baseline

[Build Bot]

Comment on attachment 243360 [details]
Fixing one baseline

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

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

[Build Bot]

Created attachment 243362 [details]
Archive of layout-test-results from ews103 for mac-mountainlion

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

[Build Bot]

Comment on attachment 243360 [details]
Fixing one baseline

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

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

[Build Bot]

Created attachment 243363 [details]
Archive of layout-test-results from ews106 for mac-mountainlion-wk2

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

[youenn fablet]

Created attachment 244037 [details]
Improving naming

[Build Bot]

Comment on attachment 244037 [details]
Improving naming

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

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

[Build Bot]

Created attachment 244043 [details]
Archive of layout-test-results from ews106 for mac-mountainlion-wk2

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

[youenn fablet]

Created attachment 244047 [details]
Rebasing global-constructors-attributes.html expected files

[youenn fablet]

Created attachment 244252 [details]
Fixing Windows build

[Xabier Rodríguez Calvar]

Created attachment 244595 [details]
Patch

[Build Bot]

Comment on attachment 244595 [details]
Patch

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

New failing tests:
css3/background/background-repeat-space-content.html

[Build Bot]

Created attachment 244598 [details]
Archive of layout-test-results from ews102 for mac-mountainlion

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

[Build Bot]

Comment on attachment 244595 [details]
Patch

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

New failing tests:
css3/background/background-repeat-space-content.html

[Build Bot]

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

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

[Sergio Villar Senin]

Comment on attachment 244595 [details]
Patch

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

The code looks pretty clear and well organized. In any case, as I said before, I am not an expert on JS bindings code, so I'll let others approve this.

Added just some minor nits.

> Source/WebCore/Modules/streams/ReadableStream.cpp:202
> +    unsigned queueSize = this->queueSize();

No need for this local variable.

> Source/WebCore/Modules/streams/ReadableStream.cpp:209
> +

Shouldn't this be checked before calling shouldApplyBackpressure()?

> Source/WebCore/bindings/js/JSReadableStreamSource.cpp:155
> +    // Apply default strategy?

Missing FIXME:

> Source/WebCore/bindings/js/JSReadableStreamSource.cpp:195
> +    unsigned chunkSize = source->chunkSize(exec, chunk);

Do this later just before computing shouldApplyBackPressure, we try to define variables were we use them. Also you can even consider not storing it in a local variable.

> Source/WebCore/bindings/js/JSReadableStreamSource.cpp:282
> +    // No-op function?

Missing FIXME:

> Source/WebCore/bindings/js/JSReadableStreamSource.cpp:297
> +    // No-op function?

Ditto.

[Xabier Rodríguez Calvar]

Created attachment 244602 [details]
Fixed things from the review

[Xabier Rodríguez Calvar]

(In reply to comment #30)
> > Source/WebCore/Modules/streams/ReadableStream.cpp:202
> > +    unsigned queueSize = this->queueSize();
> 
> No need for this local variable.

Done

> > Source/WebCore/Modules/streams/ReadableStream.cpp:209
> > +
> 
> Shouldn't this be checked before calling shouldApplyBackpressure()?

No, as the other call can raise an error, we check it after. Left unchanged.

> > Source/WebCore/bindings/js/JSReadableStreamSource.cpp:155
> > +    // Apply default strategy?
> 
> Missing FIXME:

It is not a missing fix me, but an incomplete comment. Corrected.

> > Source/WebCore/bindings/js/JSReadableStreamSource.cpp:195
> > +    unsigned chunkSize = source->chunkSize(exec, chunk);
> 
> Do this later just before computing shouldApplyBackPressure, we try to
> define variables were we use them. Also you can even consider not storing it
> in a local variable.

Similar to the second case, the call to source->chunkSize can cause errors, so we can only get the value, check the errors and use it later. Left unchanged.

> > Source/WebCore/bindings/js/JSReadableStreamSource.cpp:282
> > +    // No-op function?
> 
> Missing FIXME:

Ditto.

> > Source/WebCore/bindings/js/JSReadableStreamSource.cpp:297
> > +    // No-op function?
> 
> Ditto.

Ditto.

[Build Bot]

Comment on attachment 244602 [details]
Fixed things from the review

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

New failing tests:
css3/background/background-repeat-space-content.html

[Build Bot]

Created attachment 244605 [details]
Archive of layout-test-results from ews100 for mac-mountainlion

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

[Build Bot]

Comment on attachment 244602 [details]
Fixed things from the review

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

New failing tests:
css3/background/background-repeat-space-content.html

[Build Bot]

Created attachment 244606 [details]
Archive of layout-test-results from ews106 for mac-mountainlion-wk2

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

[Xabier Rodríguez Calvar]

(In reply to comment #35)
> Comment on attachment 244602 [details]
> Fixed things from the review
> 
> Attachment 244602 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://webkit-queues.appspot.com/results/6500198213746688
> 
> New failing tests:
> css3/background/background-repeat-space-content.html

I added the people responsible of developing and reviewing the changes in this test so that they can give me any idea on how the proposed patch fails with our changes.

Any hints?

I suspect we are triggering a hidden bug or something because our code is not even related to rendering.

[Sergio Villar Senin]

Comment on attachment 244602 [details]
Fixed things from the review

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

> LayoutTests/TestExpectations:-457
> -webkit.org/b/140309 css3/background/background-repeat-space-content.html [ ImageOnlyFailure ]

Is your patch fixing those? I have doubts about most of them :)

[Xabier Rodríguez Calvar]

(In reply to comment #38)
> Comment on attachment 244602 [details]
> Fixed things from the review
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=244602&action=review
> 
> > LayoutTests/TestExpectations:-457
> > -webkit.org/b/140309 css3/background/background-repeat-space-content.html [ ImageOnlyFailure ]
> 
> Is your patch fixing those? I have doubts about most of them :)

Yes, I screwed up when rebasing or something. Sorry! O:)

Removed the affected people from CC.

[Xabier Rodríguez Calvar]

Created attachment 244695 [details]
Fixed test expectations and tweaked the build systems to have things more nicely

[Xabier Rodríguez Calvar]

(In reply to comment #40)
> Created attachment 244695 [details]
> Fixed test expectations and tweaked the build systems to have things more
> nicely

Any really nice person wanting to review (with r+ happy ending if possible)?

[Xabier Rodríguez Calvar]

(In reply to comment #30)
> The code looks pretty clear and well organized. In any case, as I said
> before, I am not an expert on JS bindings code, so I'll let others approve
> this.

So it seems that the patch looks ok at Sergio's eyes, we'd need some JS bindings expert to review the JS code to get the r+. Any takers, please?

[youenn fablet]

(In reply to comment #42)
> (In reply to comment #30)
> > The code looks pretty clear and well organized. In any case, as I said
> > before, I am not an expert on JS bindings code, so I'll let others approve
> > this.
> 
> So it seems that the patch looks ok at Sergio's eyes, we'd need some JS
> bindings expert to review the JS code to get the r+. Any takers, please?

I will try to see tonight with sam weinig whether he could review or suggest somebody.

Some ASCIILitteral("") may be missing in the current patch (line 225 e.g.) that we can either fix later or as part of an after-review updated patch.

Some non-conformities were identified while working on adding more tests.
The current plan is to fix those bugs as independent small increments.

[Xabier Rodríguez Calvar]

Added people with knowledge of JSC code who can help with the review.

[youenn fablet]

Created attachment 245336 [details]
Ensuring JS objects owned by stream are not gc

[Build Bot]

Comment on attachment 245336 [details]
Ensuring JS objects owned by stream are not gc

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

New failing tests:
fast/xmlhttprequest/xmlhttprequest-responsetype-sync-request.html
js/dom/shadow-navigator-geolocation-in-strict-mode-does-not-throw.html

[Build Bot]

Created attachment 245338 [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 245336 [details]
Ensuring JS objects owned by stream are not gc

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

New failing tests:
fast/xmlhttprequest/xmlhttprequest-responsetype-sync-request.html
js/dom/shadow-navigator-geolocation-in-strict-mode-does-not-throw.html

[Build Bot]

Created attachment 245339 [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

[Xabier Rodríguez Calvar]

Created attachment 245342 [details]
Patch

Updated changelog and removed extra changelog path line

[Build Bot]

Comment on attachment 245342 [details]
Patch

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

New failing tests:
fast/xmlhttprequest/xmlhttprequest-responsetype-sync-request.html
js/dom/shadow-navigator-geolocation-in-strict-mode-does-not-throw.html

[Build Bot]

Created attachment 245345 [details]
Archive of layout-test-results from ews103 for mac-mavericks

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

[Build Bot]

Comment on attachment 245342 [details]
Patch

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

New failing tests:
fast/xmlhttprequest/xmlhttprequest-responsetype-sync-request.html
js/dom/shadow-navigator-geolocation-in-strict-mode-does-not-throw.html

[Build Bot]

Created attachment 245346 [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

[Xabier Rodríguez Calvar]

Created attachment 245350 [details]
Patch

New baselines for the console failing bugs

[Benjamin Poulain]

Comment on attachment 245350 [details]
Patch

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

Quick look.

> Source/JavaScriptCore/runtime/JSPromiseDeferred.cpp:211
> +    JSValue C = promise->constructor();

Uppercase "C" is an invalid variable name with our coding style.

Variable names should have full name, avoid abbreviating names.

> Source/WTF/wtf/FeatureDefines.h:657
> +#define ENABLE_STREAMS_API 0

Disabled by default? :(

> Source/WebCore/Modules/streams/ReadableStream.h:58
> +    enum State {

Typed enum are preferred if possible.

> Source/WebCore/bindings/js/JSReadableStreamSource.h:51
> +

Extra blank line here.

[Brian Burg]

Comment on attachment 245350 [details]
Patch

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

> Source/WebCore/ChangeLog:17
> +        ReadableStream implements the core functionality,

Nit; most ChangeLog entries hard-wrap to 100 columns or so, this looks weirdly narrow and inconsistent.

> Source/WebCore/ChangeLog:45
> +        (WebCore::ReadableStream::create): Creates a ReadableStream.

Most of these comments are too low-level, explaining the how or repeating function name. Save comments for explaining the 'why'.

> Source/WebKit2/ChangeLog:9
> +        * Configurations/FeatureDefines.xcconfig: Added ENABLE_STREAMS_API flag.

Comments unnecessary.

> Source/JavaScriptCore/runtime/JSPromiseDeferred.h:75
> +JS_EXPORT_PRIVATE bool resolvePromise(ExecState*, JSValue promise, JSValue fullfilFunction, JSValue rejectFunction);

'fulfillFunction'

> Source/WebCore/Modules/streams/ReadableStream.cpp:63
> +    , m_isDraining(false)

For primitives, use default member initializers in the header file, like so:

bool m_isDraining {false};

> Source/WebCore/Modules/streams/ReadableStream.cpp:90
> +    case Waiting:

I agree it is awkward to see bare enum names. If you use an enum class, these will be properly scoped.

> Source/WebCore/Modules/streams/ReadableStream.cpp:99
> +    return ASCIILiteral("");

Should this be an assertion?

> Source/WebCore/Modules/streams/ReadableStream.cpp:108
> +    callOnMainThread([callback]() {

Can we use std::bind(WTF::move(m_readyCallback), nullptr) or some variation like that?

> Source/WebCore/Modules/streams/ReadableStream.cpp:141
> +void ReadableStream::cancel(const String& reason, SuccessCallback successCallback, ErrorCallback errorCallback)

It bothers me a bit that I cannot tell by looking at the signature whether the callbacks are being passed by copy, by reference, pointer, or whatever.

> Source/WebCore/Modules/streams/ReadableStream.cpp:235
> +PassRefPtr<JSC::ArrayBuffer> ReadableStream::read(ExceptionCode& exceptionCode)

Return RefPtr.

> Source/WebCore/Modules/streams/ReadableStream.cpp:266
> +    // FIXME: We should optimize the array buffer pipeline.

File an empty bug and add the link to the fixme.

> Source/WebCore/Modules/streams/ReadableStream.h:14
> + * 3.  Neither the name of Canon Inc. nor the names of

Not sure this is the default WebKit license template these days, i.e., http://trac.webkit.org/browser/trunk/Source/WebKit/LICENSE

> Source/WebCore/Modules/streams/ReadableStream.h:53
> +// It handles in particular the backpressure according the queue size.

This is a useful comment. A link to the specification would be great too.

> Source/WebCore/Modules/streams/ReadableStream.h:65
> +    static PassRefPtr<ReadableStream> create(ScriptExecutionContext&, PassRefPtr<ReadableStreamSource>);

Use Ref<> and RefPtr&& with move semantics.

> Source/WebCore/Modules/streams/ReadableStream.h:95
> +    bool canEnqueue(String &);

Nit: String&

[Xabier Rodríguez Calvar]

(In reply to comment #56)
> > Source/WTF/wtf/FeatureDefines.h:657
> > +#define ENABLE_STREAMS_API 0
> 
> Disabled by default? :(

It is enabled by default in build-webkit, but having a 1 there caused --no-streams-api to be ignored when building for Mac. Any idea of how to do it without having a 0 there?

[Benjamin Poulain]

(In reply to comment #58)
> (In reply to comment #56)
> > > Source/WTF/wtf/FeatureDefines.h:657
> > > +#define ENABLE_STREAMS_API 0
> > 
> > Disabled by default? :(
> 
> It is enabled by default in build-webkit, but having a 1 there caused
> --no-streams-api to be ignored when building for Mac. Any idea of how to do
> it without having a 0 there?

Nope sorry. I gave up on understanding xcode a long time ago :)

[Carlos Garcia Campos]

Comment on attachment 245350 [details]
Patch

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

Added a few more comments

> Source/WebCore/Modules/streams/ReadableStream.cpp:33
> +#if ENABLE(STREAMS_API)
> +#include "ReadableStream.h"

ReadableStream.h already has a #if ENABLE(STREAMS_API), so you can move it outside the #if and leave the WebKit coding style with the header it implemented right after config.h

> Source/WebCore/Modules/streams/ReadableStream.cpp:52
> +PassRefPtr<ReadableStream> ReadableStream::create(ScriptExecutionContext& scriptExecutionContext, PassRefPtr<ReadableStreamSource> source)

This never returns nullptr, you could use Ref<ReadableStream> instead of PassRefPtr<> and Ref<ReadableStreamSource>&& for the source parameter

> Source/WebCore/Modules/streams/ReadableStream.cpp:55
> +    readableStream->suspendIfNeeded();

Could we call this in the constructor and leave this function as a single line?

>> Source/WebCore/Modules/streams/ReadableStream.cpp:99
>> +    return ASCIILiteral("");
> 
> Should this be an assertion?

Yes, I think we should assert here, but the compiler will complain, so we need to return something, I would use String::emptyString() instead of ASCIILiteral("") or just String().

> Source/WebCore/Modules/streams/ReadableStream.cpp:195
> +unsigned ReadableStream::queueSize()

This should be const

> Source/WebCore/Modules/streams/ReadableStream.cpp:200
> +bool ReadableStream::shouldApplyBackpressure()

This should be const

> Source/WebCore/Modules/streams/ReadableStream.cpp:202
> +    bool shouldApplyBackpressureAsBool = m_source->shouldApplyBackpressure(this->queueSize());

You don't need to use this->, actually, the queue size is a member, use just m_totalQueueSize

> Source/WebCore/Modules/streams/ReadableStream.cpp:217
> +bool ReadableStream::canPull()

This should be const

> Source/WebCore/Modules/streams/ReadableStream.cpp:227
> +    if (this->shouldApplyBackpressure())

You don't need the this-> here.

> Source/WebCore/Modules/streams/ReadableStream.cpp:272
> +    (m_state == Errored) ? createReadableStreamErrorFunction() :  createReadableStreamCloseFunction();

I find a bit weird the ternary operator used here.

> Source/WebCore/Modules/streams/ReadableStream.cpp:275
> +void ReadableStream::createReadableStreamCloseFunction()

This is very confusing, becase this doesn't create anything, it doesn't even return anything. I guess this is the implementation of the Close function created. I would call this close() or am I misunderstanding this?

> Source/WebCore/Modules/streams/ReadableStream.cpp:285
> +void ReadableStream::createReadableStreamErrorFunction()

Same here

> Source/WebCore/Modules/streams/ReadableStream.cpp:297
> +bool ReadableStream::canEnqueue(String &error)

String& error

> Source/WebCore/Modules/streams/ReadableStream.cpp:316
> +bool ReadableStream::enqueue(const char* chunkValue, unsigned valueLength, unsigned chunkSize)

This should probably receive a SharedBuffer&&

> Source/WebCore/Modules/streams/ReadableStream.h:89
> +    ReadableStreamSource* source() { return m_source.get(); }

This could return a reference instead of a pointer.

[youenn fablet]

(In reply to comment #60)
> Comment on attachment 245350 [details]
> Patch
> 
Thanks for the review.

> View in context:
> https://bugs.webkit.org/attachment.cgi?id=245350&action=review
> 
> Added a few more comments
> 
> > Source/WebCore/Modules/streams/ReadableStream.cpp:33
> > +#if ENABLE(STREAMS_API)> > +#include "ReadableStream.h"

> 
> ReadableStream.h already has a #if ENABLE(STREAMS_API), so you can move it
> outside the #if and leave the WebKit coding style with the header it
> implemented right after config.h
> 

Done as part of bug 141045

> > Source/WebCore/Modules/streams/ReadableStream.cpp:52
> > +PassRefPtr<ReadableStream> ReadableStream::create(ScriptExecutionContext& scriptExecutionContext, PassRefPtr<ReadableStreamSource> source)
> 
> This never returns nullptr, you could use Ref<ReadableStream> instead of
> PassRefPtr<> and Ref<ReadableStreamSource>&& for the source parameter

Done as part of bug 141045

> > Source/WebCore/Modules/streams/ReadableStream.cpp:55
> > +    readableStream->suspendIfNeeded();
> 
> Could we call this in the constructor and leave this function as a single
> line?

We could but on most cases I see, suspendIfNeeded is called in the create function, not the constructor.

> 
> >> Source/WebCore/Modules/streams/ReadableStream.cpp:99
> >> +    return ASCIILiteral("");
> > 
> > Should this be an assertion?
> 
> Yes, I think we should assert here, but the compiler will complain, so we
> need to return something, I would use String::emptyString() instead of
> ASCIILiteral("") or just String().

Partially done in bug 141045
I will replace ASCIILiteral("") by String() in a further patch.

> 
> > Source/WebCore/Modules/streams/ReadableStream.cpp:195
> > +unsigned ReadableStream::queueSize()
> 
> This should be const

OK.
I will update this on the corresponding subpatch.

> 
> > Source/WebCore/Modules/streams/ReadableStream.cpp:200
> > +bool ReadableStream::shouldApplyBackpressure()
> 
> This should be const
> 
> > Source/WebCore/Modules/streams/ReadableStream.cpp:202
> > +    bool shouldApplyBackpressureAsBool = m_source->shouldApplyBackpressure(this->queueSize());
> 
> You don't need to use this->, actually, the queue size is a member, use just
> m_totalQueueSize
> 
> > Source/WebCore/Modules/streams/ReadableStream.cpp:217
> > +bool ReadableStream::canPull()
> 
> This should be const

OK.
I will update this on the corresponding subpatch.

> 
> > Source/WebCore/Modules/streams/ReadableStream.cpp:227
> > +    if (this->shouldApplyBackpressure())
> 
> You don't need the this-> here.
> 
> > Source/WebCore/Modules/streams/ReadableStream.cpp:272
> > +    (m_state == Errored) ? createReadableStreamErrorFunction() :  createReadableStreamCloseFunction();
> 
> I find a bit weird the ternary operator used here.

It was suggested in a previous review.

> 
> > Source/WebCore/Modules/streams/ReadableStream.cpp:275
> > +void ReadableStream::createReadableStreamCloseFunction()
> 
> This is very confusing, becase this doesn't create anything, it doesn't even
> return anything. I guess this is the implementation of the Close function
> created. I would call this close() or am I misunderstanding this?

The name was chosen from the spec (https://streams.spec.whatwg.org/#create-readable-stream-close-function), but I agree it is somehow misleading. 
I will find a better name in bug 141160 patch and add the link to the spec before the function body.


> 
> > Source/WebCore/Modules/streams/ReadableStream.cpp:285
> > +void ReadableStream::createReadableStreamErrorFunction()
> 
> Same here

Ditto.

> 
> > Source/WebCore/Modules/streams/ReadableStream.cpp:297
> > +bool ReadableStream::canEnqueue(String &error)
> 
> String& error

OK

> 
> > Source/WebCore/Modules/streams/ReadableStream.cpp:316
> > +bool ReadableStream::enqueue(const char* chunkValue, unsigned valueLength, unsigned chunkSize)
> 
> This should probably receive a SharedBuffer&&

OK

> 
> > Source/WebCore/Modules/streams/ReadableStream.h:89
> > +    ReadableStreamSource* source() { return m_source.get(); }
> 
> This could return a reference instead of a pointer.

Correct, this is already changed in subsequent patches.

[Anne van Kesteren]

I think we can consider this done.

[Radar WebKit Bug Importer]

<rdar://problem/107268572>