Bug 160242 – [Streams API] Replace ReadableStreamController by ReadableStreamDefaultController

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

[Romain Bellessort]

As was done for ReadableStreamReader/ReadableStreamDefaultReader, ReadableStreamController should be replaced by ReadableStreamDefaultController to align with Streams API spec.

[Romain Bellessort]

Created attachment 284693 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/bindings/js/ReadableStreamDefaultController.h:46:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 24 files


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

[youenn fablet]

Comment on attachment 284693 [details]
Patch

r=me.
It might be good to improve the controller code.
See below for all possible improvements.
This can be done now or in a later patch.
Wdyt romain?

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

> Source/WebCore/bindings/js/ReadableStreamDefaultController.cpp:36
> +#include "JSReadableStreamSource.h"

These two includes may no longer be needed

> Source/WebCore/bindings/js/ReadableStreamDefaultController.cpp:77
> +    auto isLocked = globalObject()->builtinInternalFunctions().readableStreamInternals().m_isReadableStreamLockedFunction.get();

Can we add an ASSERT(isLocked.isFunction())?

> Source/WebCore/bindings/js/ReadableStreamDefaultController.h:55
> +    void close() { invoke(*globalObject()->globalExec(), *m_jsController, "close", JSC::jsUndefined()); }

Can we add a jsConstroller() method helper that would do 
ASSERT(m_jsController);
Return m_jsController;

> Source/WebCore/bindings/js/ReadableStreamDefaultController.h:63
> +    JSDOMGlobalObject* globalObject() const;

Might be better to return a reference than a pointer.

> Source/WebCore/bindings/js/ReadableStreamDefaultController.h:72
> +    return static_cast<JSDOMGlobalObject*>(m_jsController->globalObject());

Might be better to add an ASSERT and return a JSDOMGlobalObject&

> Source/WebCore/bindings/js/ReadableStreamDefaultController.h:77
> +    JSC::ExecState& state = *globalObject()->globalExec();

Might be good to add an ASSERT(globalObject()->globalExec())

> Source/WebCore/bindings/js/ReadableStreamDefaultController.h:96
> +    JSC::JSLockHolder locker(state);

Ditto.

> Source/WebCore/bindings/js/WebCoreBuiltinNames.h:100
> +    macro(ReadableStreamDefaultController) \

Switch reader and controller lines?

[Romain Bellessort]

Created attachment 284781 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/bindings/js/ReadableStreamDefaultController.h:46:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 24 files


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

[Romain Bellessort]

(In reply to comment #3)
> Comment on attachment 284693 [details]
> Patch
> 
> r=me.
> It might be good to improve the controller code.
> See below for all possible improvements.
> This can be done now or in a later patch.
> Wdyt romain?

For this patch I was focusing on the JS part. I just uploaded a new patch that fixes alphabetical order in WebCoreBuiltinNames.h, but I let other remarks aiming at improving ReadableStreamDefaultController.h/cpp for another patch. I'm going to create a bug about that.

[WebKit Commit Bot]

Comment on attachment 284781 [details]
Patch

Clearing flags on attachment: 284781

Committed r203818: <http://trac.webkit.org/changeset/203818>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.