ReadableStream initial implementation

ReadableStream initial implementation

ReadableStream is part of the Streams API:
https://github.com/whatwg/streams

This CL also adds GC instructions to suppress false mock leakage reports.

BUG=393911
R=tyoshino

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179210

[tyoshino (SeeGerritForStatus), 2014-07-18 04:31:40]

https://codereview.chromium.org/407453002/diff/20001/Source/core/streams/Read...
File Source/core/streams/ReadableStream.h (right):

https://codereview.chromium.org/407453002/diff/20001/Source/core/streams/Read...
Source/core/streams/ReadableStream.h:72: #endif // #ifndef ReadableStream_h
uncommon in blink
just as follows?

// ReadableStream_h

https://codereview.chromium.org/407453002/diff/20001/Source/core/streams/Unde...
File Source/core/streams/UnderlyingSource.h (right):

https://codereview.chromium.org/407453002/diff/20001/Source/core/streams/Unde...
Source/core/streams/UnderlyingSource.h:17: virtual void
cancelStream(ExceptionState*) = 0;
are these suffixes needed? it looks like Stream is the object of the preceding
verb but it's not. they are invoked by the corresponding stream.

[yhirano, 2014-07-18 04:38:54]

https://codereview.chromium.org/407453002/diff/20001/Source/core/streams/Read...
File Source/core/streams/ReadableStream.h (right):

https://codereview.chromium.org/407453002/diff/20001/Source/core/streams/Read...
Source/core/streams/ReadableStream.h:72: #endif // #ifndef ReadableStream_h
On 2014/07/18 04:31:40, tyoshino wrote:
> uncommon in blink
> just as follows?
> 
> // ReadableStream_h

Done.

https://codereview.chromium.org/407453002/diff/20001/Source/core/streams/Unde...
File Source/core/streams/UnderlyingSource.h (right):

https://codereview.chromium.org/407453002/diff/20001/Source/core/streams/Unde...
Source/core/streams/UnderlyingSource.h:17: virtual void
cancelStream(ExceptionState*) = 0;
On 2014/07/18 04:31:40, tyoshino wrote:
> are these suffixes needed? it looks like Stream is the object of the preceding
> verb but it's not. they are invoked by the corresponding stream.

Yes, they are called by ReadableStream objects. But they can be implemented in a
fat class such as XMLHttpRequest, I think. In such a case cancel is too simple.

[tyoshino (SeeGerritForStatus), 2014-07-18 04:49:54]

https://codereview.chromium.org/407453002/diff/20001/Source/core/streams/Unde...
File Source/core/streams/UnderlyingSource.h (right):

https://codereview.chromium.org/407453002/diff/20001/Source/core/streams/Unde...
Source/core/streams/UnderlyingSource.h:17: virtual void
cancelStream(ExceptionState*) = 0;
On 2014/07/18 04:38:54, yhirano wrote:
> On 2014/07/18 04:31:40, tyoshino wrote:
> > are these suffixes needed? it looks like Stream is the object of the
preceding
> > verb but it's not. they are invoked by the corresponding stream.
> 
> Yes, they are called by ReadableStream objects. But they can be implemented in
a
> fat class such as XMLHttpRequest, I think. In such a case cancel is too
simple.

Oh, I see. How about did prefix? Hmm, still too simple?
- didStreamStart
- didStreamPull
- didStreamCancel

- streamDidStart
- streamDidPull
- streamDidCancel

or

- readableStreamDidStart
- readableStreamDidPull
- readableStreamDidCancel

are the safest, but maybe just stream is fine for now.

[yhirano, 2014-07-18 06:33:37]

https://codereview.chromium.org/407453002/diff/20001/Source/core/streams/Unde...
File Source/core/streams/UnderlyingSource.h (right):

https://codereview.chromium.org/407453002/diff/20001/Source/core/streams/Unde...
Source/core/streams/UnderlyingSource.h:17: virtual void
cancelStream(ExceptionState*) = 0;
On 2014/07/18 04:49:54, tyoshino wrote:
> On 2014/07/18 04:38:54, yhirano wrote:
> > On 2014/07/18 04:31:40, tyoshino wrote:
> > > are these suffixes needed? it looks like Stream is the object of the
> preceding
> > > verb but it's not. they are invoked by the corresponding stream.
> > 
> > Yes, they are called by ReadableStream objects. But they can be implemented
in
> a
> > fat class such as XMLHttpRequest, I think. In such a case cancel is too
> simple.
> 
> Oh, I see. How about did prefix? Hmm, still too simple?
> - didStreamStart
> - didStreamPull
> - didStreamCancel
> 
> - streamDidStart
> - streamDidPull
> - streamDidCancel
> 
> or
> 
> - readableStreamDidStart
> - readableStreamDidPull
> - readableStreamDidCancel
> 
> are the safest, but maybe just stream is fine for now.
How about suffixing Source?

[tyoshino (SeeGerritForStatus), 2014-07-18 07:14:48]

lgtm

https://codereview.chromium.org/407453002/diff/20001/Source/core/streams/Unde...
File Source/core/streams/UnderlyingSource.h (right):

https://codereview.chromium.org/407453002/diff/20001/Source/core/streams/Unde...
Source/core/streams/UnderlyingSource.h:17: virtual void
cancelStream(ExceptionState*) = 0;
On 2014/07/18 06:33:37, yhirano wrote:
> On 2014/07/18 04:49:54, tyoshino wrote:
> > On 2014/07/18 04:38:54, yhirano wrote:
> > > On 2014/07/18 04:31:40, tyoshino wrote:
> > > > are these suffixes needed? it looks like Stream is the object of the
> > preceding
> > > > verb but it's not. they are invoked by the corresponding stream.
> > > 
> > > Yes, they are called by ReadableStream objects. But they can be
implemented
> in
> > a
> > > fat class such as XMLHttpRequest, I think. In such a case cancel is too
> > simple.
> > 
> > Oh, I see. How about did prefix? Hmm, still too simple?
> > - didStreamStart
> > - didStreamPull
> > - didStreamCancel
> > 
> > - streamDidStart
> > - streamDidPull
> > - streamDidCancel
> > 
> > or
> > 
> > - readableStreamDidStart
> > - readableStreamDidPull
> > - readableStreamDidCancel
> > 
> > are the safest, but maybe just stream is fine for now.
> How about suffixing Source?

Sounds good

https://codereview.chromium.org/407453002/diff/50001/Source/core/streams/Read...
File Source/core/streams/ReadableStream.h (right):

https://codereview.chromium.org/407453002/diff/50001/Source/core/streams/Read...
Source/core/streams/ReadableStream.h:37:
ScriptState::Scope(m_scriptState.get());
ScriptState::Scope scope(...);
?

https://codereview.chromium.org/407453002/diff/50001/Source/platform/testing/...
File Source/platform/testing/TestWithGC.h (right):

https://codereview.chromium.org/407453002/diff/50001/Source/platform/testing/...
Source/platform/testing/TestWithGC.h:18: WebCore::Heap::collectAllGarbage();
WebCore:: needed?

[yhirano, 2014-07-18 07:44:36]

https://codereview.chromium.org/407453002/diff/50001/Source/core/streams/Read...
File Source/core/streams/ReadableStream.h (right):

https://codereview.chromium.org/407453002/diff/50001/Source/core/streams/Read...
Source/core/streams/ReadableStream.h:37:
ScriptState::Scope(m_scriptState.get());
On 2014/07/18 07:14:47, tyoshino wrote:
> ScriptState::Scope scope(...);
> ?
Thanks, done.

https://codereview.chromium.org/407453002/diff/50001/Source/platform/testing/...
File Source/platform/testing/TestWithGC.h (right):

https://codereview.chromium.org/407453002/diff/50001/Source/platform/testing/...
Source/platform/testing/TestWithGC.h:18: WebCore::Heap::collectAllGarbage();
On 2014/07/18 07:14:47, tyoshino wrote:
> WebCore:: needed?

Done.

[yhirano, 2014-07-18 08:53:26]

+japhet for OWNER review

[yhirano, 2014-07-23 01:39:26]

On 2014/07/18 08:53:26, yhirano wrote:
> +japhet for OWNER review

[yhirano, 2014-07-24 13:40:04]

+abarth for Source/core and Source/platform

[dcheng, 2014-07-29 05:30:59]

https://codereview.chromium.org/407453002/diff/90001/Source/platform/testing/...
File Source/platform/testing/TestWithGC.h (right):

https://codereview.chromium.org/407453002/diff/90001/Source/platform/testing/...
Source/platform/testing/TestWithGC.h:18: Heap::collectAllGarbage();
Does every test that might allocate objects on the Oilpan heap need to do this?

When does the leak detector run? My impression was that it only ran when the
test suite exited; if that's the case, I think it'd be better to put this logic
elsewhere.

[yhirano, 2014-07-29 06:36:24]

Thank you for the comment.

https://codereview.chromium.org/407453002/diff/90001/Source/platform/testing/...
File Source/platform/testing/TestWithGC.h (right):

https://codereview.chromium.org/407453002/diff/90001/Source/platform/testing/...
Source/platform/testing/TestWithGC.h:18: Heap::collectAllGarbage();
On 2014/07/29 05:30:59, dcheng (OOO) wrote:
> Does every test that might allocate objects on the Oilpan heap need to do
this?
> 
> When does the leak detector run? My impression was that it only ran when the
> test suite exited; if that's the case, I think it'd be better to put this
logic
> elsewhere.

I moved these statements to Source/web/tests/RunAllTests.cpp.
Done.

[abarth-chromium, 2014-07-29 17:13:29]

I don't have the bandwidth to review these CLs.  Can you find another reivewer?

[yhirano, 2014-07-30 01:23:26]

-abarth
tkent, can you take a look at this CL?

[tkent, 2014-07-30 01:52:26]

lgtm

https://codereview.chromium.org/407453002/diff/130001/Source/core/streams/Rea...
File Source/core/streams/ReadableStream.cpp (right):

https://codereview.chromium.org/407453002/diff/130001/Source/core/streams/Rea...
Source/core/streams/ReadableStream.cpp:13: namespace WebCore {
namespace blink

https://codereview.chromium.org/407453002/diff/130001/Source/core/streams/Rea...
Source/core/streams/ReadableStream.cpp:83: } // namespace WebCore
namespace blink

https://codereview.chromium.org/407453002/diff/130001/Source/core/streams/Rea...
File Source/core/streams/ReadableStream.h (right):

https://codereview.chromium.org/407453002/diff/130001/Source/core/streams/Rea...
Source/core/streams/ReadableStream.h:15: namespace WebCore {
namespace blink

https://codereview.chromium.org/407453002/diff/130001/Source/core/streams/Rea...
Source/core/streams/ReadableStream.h:20: class ReadableStream FINAL : public
GarbageCollectedFinalized<ReadableStream> {
Oilpan-by-default.  Cool!

https://codereview.chromium.org/407453002/diff/130001/Source/core/streams/Rea...
Source/core/streams/ReadableStream.h:70: } // namespace WebCore
namespace blink

https://codereview.chromium.org/407453002/diff/130001/Source/core/streams/Rea...
File Source/core/streams/ReadableStreamTest.cpp (right):

https://codereview.chromium.org/407453002/diff/130001/Source/core/streams/Rea...
Source/core/streams/ReadableStreamTest.cpp:17: namespace WebCore {
namespace blink

https://codereview.chromium.org/407453002/diff/130001/Source/core/streams/Rea...
Source/core/streams/ReadableStreamTest.cpp:19: namespace {
We don't need to wrap with anonymous namespace.

https://codereview.chromium.org/407453002/diff/130001/Source/core/streams/Rea...
Source/core/streams/ReadableStreamTest.cpp:44: private:
nit: add a blank line before |private:|.

https://codereview.chromium.org/407453002/diff/130001/Source/core/streams/Rea...
Source/core/streams/ReadableStreamTest.cpp:69: TEST_F(ReadableStreamTest,
construct)
construct -> Construct
We usually apply TitleCase for test names.

https://codereview.chromium.org/407453002/diff/130001/Source/core/streams/Rea...
Source/core/streams/ReadableStreamTest.cpp:97: TEST_F(ReadableStreamTest,
constructError)
Ditto.

https://codereview.chromium.org/407453002/diff/130001/Source/core/streams/Rea...
Source/core/streams/ReadableStreamTest.cpp:141: } // namespace WebCore
namespace blink

https://codereview.chromium.org/407453002/diff/130001/Source/core/streams/Und...
File Source/core/streams/UnderlyingSource.h (right):

https://codereview.chromium.org/407453002/diff/130001/Source/core/streams/Und...
Source/core/streams/UnderlyingSource.h:10: namespace WebCore {
namespace blink

https://codereview.chromium.org/407453002/diff/130001/Source/core/streams/Und...
Source/core/streams/UnderlyingSource.h:23: } // namespace WebCore
namespace blink

https://codereview.chromium.org/407453002/diff/130001/Source/web/tests/RunAll...
File Source/web/tests/RunAllTests.cpp (right):

https://codereview.chromium.org/407453002/diff/130001/Source/web/tests/RunAll...
Source/web/tests/RunAllTests.cpp:53:
v8::Isolate::GetCurrent()->RequestGarbageCollectionForTesting(v8::Isolate::kFullGarbageCollection);
Why do you need GC?  Please mention this change in the CL description.

[yhirano, 2014-07-30 03:27:24]

https://codereview.chromium.org/407453002/diff/130001/Source/core/streams/Rea...
File Source/core/streams/ReadableStream.cpp (right):

https://codereview.chromium.org/407453002/diff/130001/Source/core/streams/Rea...
Source/core/streams/ReadableStream.cpp:13: namespace WebCore {
On 2014/07/30 01:52:25, tkent wrote:
> namespace blink

Done.

https://codereview.chromium.org/407453002/diff/130001/Source/core/streams/Rea...
Source/core/streams/ReadableStream.cpp:83: } // namespace WebCore
On 2014/07/30 01:52:24, tkent wrote:
> namespace blink

Done.

https://codereview.chromium.org/407453002/diff/130001/Source/core/streams/Rea...
File Source/core/streams/ReadableStream.h (right):

https://codereview.chromium.org/407453002/diff/130001/Source/core/streams/Rea...
Source/core/streams/ReadableStream.h:15: namespace WebCore {
On 2014/07/30 01:52:25, tkent wrote:
> namespace blink

Done.

https://codereview.chromium.org/407453002/diff/130001/Source/core/streams/Rea...
Source/core/streams/ReadableStream.h:70: } // namespace WebCore
On 2014/07/30 01:52:25, tkent wrote:
> namespace blink

Done.

https://codereview.chromium.org/407453002/diff/130001/Source/core/streams/Rea...
File Source/core/streams/ReadableStreamTest.cpp (right):

https://codereview.chromium.org/407453002/diff/130001/Source/core/streams/Rea...
Source/core/streams/ReadableStreamTest.cpp:17: namespace WebCore {
On 2014/07/30 01:52:25, tkent wrote:
> namespace blink

Done.

https://codereview.chromium.org/407453002/diff/130001/Source/core/streams/Rea...
Source/core/streams/ReadableStreamTest.cpp:19: namespace {
On 2014/07/30 01:52:25, tkent wrote:
> We don't need to wrap with anonymous namespace.

Done.

https://codereview.chromium.org/407453002/diff/130001/Source/core/streams/Rea...
Source/core/streams/ReadableStreamTest.cpp:44: private:
On 2014/07/30 01:52:25, tkent wrote:
> nit: add a blank line before |private:|.

Done.

https://codereview.chromium.org/407453002/diff/130001/Source/core/streams/Rea...
Source/core/streams/ReadableStreamTest.cpp:69: TEST_F(ReadableStreamTest,
construct)
On 2014/07/30 01:52:25, tkent wrote:
> construct -> Construct
> We usually apply TitleCase for test names.

Done.

https://codereview.chromium.org/407453002/diff/130001/Source/core/streams/Rea...
Source/core/streams/ReadableStreamTest.cpp:97: TEST_F(ReadableStreamTest,
constructError)
On 2014/07/30 01:52:25, tkent wrote:
> Ditto.

Done.

https://codereview.chromium.org/407453002/diff/130001/Source/core/streams/Rea...
Source/core/streams/ReadableStreamTest.cpp:141: } // namespace WebCore
On 2014/07/30 01:52:25, tkent wrote:
> namespace blink

Done.

https://codereview.chromium.org/407453002/diff/130001/Source/core/streams/Und...
File Source/core/streams/UnderlyingSource.h (right):

https://codereview.chromium.org/407453002/diff/130001/Source/core/streams/Und...
Source/core/streams/UnderlyingSource.h:10: namespace WebCore {
On 2014/07/30 01:52:25, tkent wrote:
> namespace blink

Done.

https://codereview.chromium.org/407453002/diff/130001/Source/core/streams/Und...
Source/core/streams/UnderlyingSource.h:23: } // namespace WebCore
On 2014/07/30 01:52:25, tkent wrote:
> namespace blink

Done.

https://codereview.chromium.org/407453002/diff/130001/Source/web/tests/RunAll...
File Source/web/tests/RunAllTests.cpp (right):

https://codereview.chromium.org/407453002/diff/130001/Source/web/tests/RunAll...
Source/web/tests/RunAllTests.cpp:53:
v8::Isolate::GetCurrent()->RequestGarbageCollectionForTesting(v8::Isolate::kFullGarbageCollection);
On 2014/07/30 01:52:25, tkent wrote:
> Why do you need GC?  Please mention this change in the CL description.

Done.

[yhirano, 2014-07-30 04:57:39]

Kent-san, I found BLINK_IMPLEMENTATION could be 0 in
Source/web/tests/RunAllTests.cpp (see [1]).
I moved the changes to Source/web/tests/WebUnitTests.cpp . Can you take another
look?
Thanks,

1: https://bugs.webkit.org/show_bug.cgi?id=90094

[tkent, 2014-07-30 05:02:39]

On 2014/07/30 04:57:39, yhirano wrote:
> Kent-san, I found BLINK_IMPLEMENTATION could be 0 in
> Source/web/tests/RunAllTests.cpp (see [1]).
> I moved the changes to Source/web/tests/WebUnitTests.cpp . Can you take
another
> look?


lgtm

[yhirano, 2014-07-30 07:31:40]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2014-07-30 07:32:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yhirano@chromium.org/407453002/170001

[commit-bot: I haz the power, 2014-07-30 08:34:48]

Change committed as 179210