Bug 144387 – [Streams API] Refactor ReadableJSStream and ReadableStreamJSSource

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

[youenn fablet]

ReadableJSStream, ReadableJSStreamReader and ReadableStreamJSSource are all in Source/WebCore/bindings/js/ReadableStreamJSSource.h
This should be split in ReadableJSStream.h/.cpp (containing stream and related reader class definition) and ReadableJSStreamSoure.h/.cpp

[youenn fablet]

Created attachment 251959 [details]
Patch

[youenn fablet]

Created attachment 251960 [details]
Patch

[youenn fablet]

Comment on attachment 251960 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        [Streams API] Refactor ReadableJSStream and ReadableStreamJSSource

This is the first step of the refactoring.
Second step would be to move ReadableStreamJSSource as ReadableJSStream::Source and rename ReadableStreamJSSource.h/.cpp as ReadableJSStream.h/.cpp

[Darin Adler]

Comment on attachment 251960 [details]
Patch

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

> Source/WebCore/bindings/js/JSReadableStreamCustom.cpp:82
> +    RefPtr<ReadableJSStream> readableStream = ReadableJSStream::create(exec, *jsConstructor->scriptExecutionContext());

Should put this in a Ref. Not sure why it’s a RefPtr if it can’t be null.

> Source/WebCore/bindings/js/JSReadableStreamCustom.cpp:83
> +    return JSValue::encode(toJS(exec, jsCast<JSDOMGlobalObject*>(exec->callee()->globalObject()), readableStream.releaseNonNull()));

Should be WTF::move(readableStream), not readableStream.releaseNonNull();

> Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:107
> +    return jsDynamicCast<JSDOMGlobalObject*>(m_source.get()->globalObject());

Not sure you need the .get() here. Does m_source->globalObject() work?

> Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:152
> +    RefPtr<Reader> reader = Reader::create(*this);
>      return reader.releaseNonNull();

Should be written as a 1-liner. Surprised that Reader::create returns a RefPtr and not a Ref if it can’t return null.

> Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:168
> +    auto readableStreamReader = adoptRef(*new Reader(stream));
>      return readableStreamReader;

Should be a 1-liner. No need for a local variable.

> Source/WebCore/bindings/js/ReadableStreamJSSource.h:53
> +    void start(JSC::ExecState*, ReadableJSStream&);

Note that ExecState& is better than ExecState* too. Best to use it in functions we are modifying.

> Source/WebCore/bindings/js/ReadableStreamJSSource.h:66
> +    static Ref<ReadableJSStream> create(JSC::ExecState*, ScriptExecutionContext&);

Should be ExecState&.

> Source/WebCore/bindings/js/ReadableStreamJSSource.h:78
> +    class Reader: public ReadableStreamReader {
> +    public:
> +        static Ref<Reader> create(ReadableJSStream&);
> +    private:
> +        Reader(ReadableJSStream&);
> +    };

This class should not be indented.

The constructor should be marked explicit.

[youenn fablet]

Created attachment 252303 [details]
Patch for landing

[youenn fablet]

Comment on attachment 252303 [details]
Patch for landing

Waiting for ews to finish testing 144455 patch before cq+

[Xabier Rodríguez Calvar]

Comment on attachment 252303 [details]
Patch for landing

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

> Source/WebCore/ChangeLog:1
> [phone]  Youenn Fablet  <[e-mail]>

Di you forget me again?

[youenn fablet]

Created attachment 252306 [details]
Patch for landing

[youenn fablet]

(In reply to comment #4)
> Comment on attachment 251960 [details]
> Patch
> 

Thanks for the review.
I changed the patch accordingly...

> > Source/WebCore/bindings/js/ReadableStreamJSSource.h:78
> > +    class Reader: public ReadableStreamReader {
> > +    public:
> > +        static Ref<Reader> create(ReadableJSStream&);
> > +    private:
> > +        Reader(ReadableJSStream&);
> > +    };
> 
> This class should not be indented.

... except for that one.
It seems odd to not indent it as it is now inside ReadableJSStream definition.
I can change the indentation in the next refactoring patch (ReadableJSStreamSource -> ReadableJSStream::Source) if that is the preferred style.

[WebKit Commit Bot]

Comment on attachment 252306 [details]
Patch for landing

Clearing flags on attachment: 252306

Committed r183744: <http://trac.webkit.org/changeset/183744>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.