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
Created attachment 251959 [details] Patch
Created attachment 251960 [details] Patch
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 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.
Created attachment 252303 [details] Patch for landing
Comment on attachment 252303 [details] Patch for landing Waiting for ews to finish testing 144455 patch before cq+
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?
Created attachment 252306 [details] Patch for landing
(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 on attachment 252306 [details] Patch for landing Clearing flags on attachment: 252306 Committed r183744: <http://trac.webkit.org/changeset/183744>
All reviewed patches have been landed. Closing bug.