Bug 144387 - [Streams API] Refactor ReadableJSStream and ReadableStreamJSSource
Summary: [Streams API] Refactor ReadableJSStream and ReadableStreamJSSource
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-04-29 06:02 PDT by youenn fablet
Modified: 2015-05-04 06:31 PDT (History)
3 users (show)

See Also:


Attachments
Patch (9.08 KB, patch)
2015-04-29 09:41 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (9.13 KB, patch)
2015-04-29 09:55 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (9.70 KB, patch)
2015-05-04 03:40 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (9.79 KB, patch)
2015-05-04 05: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-29 06:02:33 PDT
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
Comment 1 youenn fablet 2015-04-29 09:41:50 PDT
Created attachment 251959 [details]
Patch
Comment 2 youenn fablet 2015-04-29 09:55:58 PDT
Created attachment 251960 [details]
Patch
Comment 3 youenn fablet 2015-04-29 11:17:39 PDT
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
Comment 4 Darin Adler 2015-05-01 16:27:49 PDT
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.
Comment 5 youenn fablet 2015-05-04 03:40:23 PDT
Created attachment 252303 [details]
Patch for landing
Comment 6 youenn fablet 2015-05-04 03:48:51 PDT
Comment on attachment 252303 [details]
Patch for landing

Waiting for ews to finish testing 144455 patch before cq+
Comment 7 Xabier Rodríguez Calvar 2015-05-04 03:59:16 PDT
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
> +2015-05-03  Youenn Fablet  <youenn.fablet@crf.canon.fr>

Di you forget me again?
Comment 8 youenn fablet 2015-05-04 05:36:23 PDT
Created attachment 252306 [details]
Patch for landing
Comment 9 youenn fablet 2015-05-04 05:40:10 PDT
(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.
Comment 10 WebKit Commit Bot 2015-05-04 06:31:37 PDT
Comment on attachment 252306 [details]
Patch for landing

Clearing flags on attachment: 252306

Committed r183744: <http://trac.webkit.org/changeset/183744>
Comment 11 WebKit Commit Bot 2015-05-04 06:31:40 PDT
All reviewed patches have been landed.  Closing bug.