Bug 143333

Created attachment 250055 [details] Patch

Comment on attachment 250055 [details] Patch Attachment 250055 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6551989915746304 New failing tests: streams/readablestreamreader-constructor.html

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

Created attachment 250059 [details] Patch

Comment on attachment 250059 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250059&action=review > Source/WebCore/ChangeLog:11 > + Alternative to storing a boolean would be to make ReadableStreamReader CustomIsReachable and return true if it has a stream. But I guess the stream and reader would not be collectable anymore for the whole session, hence the current approach.

Comment on attachment 250059 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250059&action=review > Source/WebCore/Modules/streams/ReadableStream.h:63 > + void release(bool keepLocking = false) { m_reader = nullptr; m_isLocked = keepLocking; } This should be two different named functions rather than a single function with a boolean argument. Nobody is calling this and passing a boolean expression, so treating this as an argument just makes it confusing at the call site for no reason. Maybe the new one could be named releaseAndLeaveLocked() or releaseButKeepLocked()?

Comment on attachment 250059 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250059&action=review Good patch. Nice to see coverage for the edge cases. Anything interesting if the Stream is collected with a live reader? >> Source/WebCore/ChangeLog:11 >> + > > Alternative to storing a boolean would be to make ReadableStreamReader CustomIsReachable and return true if it has a stream. > But I guess the stream and reader would not be collectable anymore for the whole session, hence the current approach. Yep, that looks like a good approach. No reason to keep both objects alive. > Source/WebCore/Modules/streams/ReadableStream.h:61 > + bool isLocked() { return m_isLocked; } This should be const. >> Source/WebCore/Modules/streams/ReadableStream.h:63 >> + void release(bool keepLocking = false) { m_reader = nullptr; m_isLocked = keepLocking; } > > This should be two different named functions rather than a single function with a boolean argument. Nobody is calling this and passing a boolean expression, so treating this as an argument just makes it confusing at the call site for no reason. > > Maybe the new one could be named releaseAndLeaveLocked() or releaseButKeepLocked()? +1 for having 2 names. Can you please also move the inline implementation after the definition of the class? It will be a little cleaner than having 2 assignment directly in the class declaration.

(In reply to comment #7) > Comment on attachment 250059 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=250059&action=review > > Good patch. Nice to see coverage for the edge cases. > > Anything interesting if the Stream is collected with a live reader? That may be a usual case. ReadableStream destructor will be called once reader will be destroyed. > >> Source/WebCore/ChangeLog:11 > >> + > > > > Alternative to storing a boolean would be to make ReadableStreamReader CustomIsReachable and return true if it has a stream. > > But I guess the stream and reader would not be collectable anymore for the whole session, hence the current approach. > > Yep, that looks like a good approach. No reason to keep both objects alive. > > > Source/WebCore/Modules/streams/ReadableStream.h:61 > > + bool isLocked() { return m_isLocked; } > > This should be const. OK > >> Source/WebCore/Modules/streams/ReadableStream.h:63 > >> + void release(bool keepLocking = false) { m_reader = nullptr; m_isLocked = keepLocking; } > > > > This should be two different named functions rather than a single function with a boolean argument. Nobody is calling this and passing a boolean expression, so treating this as an argument just makes it confusing at the call site for no reason. > > > > Maybe the new one could be named releaseAndLeaveLocked() or releaseButKeepLocked()? > > +1 for having 2 names. OK > Can you please also move the inline implementation after the definition of > the class? It will be a little cleaner than having 2 assignment directly in > the class declaration. OK

Created attachment 250128 [details] Patch for landing

The commit-queue encountered the following flaky tests while processing attachment 250128 [details]: http/tests/xmlhttprequest/workers/methods-async.html bug 143403 (authors: atwilson@chromium.org and levin@chromium.org) The commit-queue is continuing to process your patch.

Comment on attachment 250128 [details] Patch for landing Clearing flags on attachment: 250128 Committed r182344: <http://trac.webkit.org/changeset/182344>

All reviewed patches have been landed. Closing bug.