Implement ReadableStreamBytesConsumer

Implement ReadableStreamBytesConsumer

This CL implements ReadableStreamBytesConsumer and replaces
ReadableStreamDataConsumerHandle with it.

BUG=610195
Committed: https://crrev.com/bf54066c4ef19c4815dc5433d4152a322686a8c0
Cr-Commit-Position: refs/heads/master@{#422369}

[yhirano, 2016-09-23 10:41:15]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-23 10:41:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 10:52:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-23 10:52:41]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[yhirano, 2016-09-23 10:56:03]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-23 10:56:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 11:02:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-23 11:02:53]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[yhirano, 2016-09-26 05:17:14]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-26 05:17:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-26 06:42:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-26 06:42:37]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[yhirano, 2016-09-28 02:04:48]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 02:05:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-09-28 02:15:00]

yhirano@chromium.org changed reviewers:
+ hiroshige@chromium.org

[commit-bot: I haz the power, 2016-09-28 03:20:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-28 03:20:07]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-09-28 04:40:27]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 04:40:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 04:50:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-28 04:50:08]

Dry run: Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[yhirano, 2016-09-28 04:57:59]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 04:58:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 06:16:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-28 06:16:10]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2016-09-30 06:35:15]

code basically looks good: just moving code as-is, except for removing
thread-related things and ReadingContext.

https://codereview.chromium.org/2365853002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/ReadableStreamBytesConsumer.cpp
(right):

https://codereview.chromium.org/2365853002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/ReadableStreamBytesConsumer.cpp:108:
DCHECK_LE(m_pendingOffset, m_pendingBuffer->length());
This CL seems to allow 0-byte |m_pendingBuffer| while the previous impl didn't.
(e.g. we have DCHECK_LE here instead of DCHECK_LT, and we lack size check around
Line 192)
What is the motivation for that?
(The change is fine with me, just to check)

https://codereview.chromium.org/2365853002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/ReadableStreamBytesConsumer.cpp:132: if
(m_pendingOffset == m_pendingBuffer->length()) {
Using '>=' instead of '==' is more robust for buffer overflow.

https://codereview.chromium.org/2365853002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/ReadableStreamBytesConsumer.h
(right):

https://codereview.chromium.org/2365853002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/ReadableStreamBytesConsumer.h:59: //
outside of ReadableStreamDataConsumerHandle.
nit: ReadableStreamBytesConsumer?

https://codereview.chromium.org/2365853002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/ReadableStreamBytesConsumerTest.cpp
(right):

https://codereview.chromium.org/2365853002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/ReadableStreamBytesConsumerTest.cpp:31:
class MockClient : public GarbageCollectedFinalized<MockClient>, public
BytesConsumer::Client {
BytesConsumerForDataConsumerHandleTest.cpp also has MockClient.
(Also there are two NoopClient and two TestClient)
Please merge duplicated BytesConsumer::Client subclasses later.

https://codereview.chromium.org/2365853002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/ReadableStreamBytesConsumerTest.cpp:128:
testing::runPendingTasks();
Could you add checkpoint.Call() before runPendingTasks() to ensure that
onStateChange() is not called synchronously in beginRead()?

https://codereview.chromium.org/2365853002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/ReadableStreamBytesConsumerTest.cpp:158:
testing::runPendingTasks();
ditto.

https://codereview.chromium.org/2365853002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/ReadableStreamBytesConsumerTest.cpp:205:
testing::runPendingTasks();
ditto.

https://codereview.chromium.org/2365853002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/ReadableStreamBytesConsumerTest.cpp:229:
testing::runPendingTasks();
ditto.

https://codereview.chromium.org/2365853002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/ReadableStreamBytesConsumerTest.cpp:240:
testing::runPendingTasks();
ditto.

https://codereview.chromium.org/2365853002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/ReadableStreamBytesConsumerTest.cpp:274:
testing::runPendingTasks();
ditto.

https://codereview.chromium.org/2365853002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/ReadableStreamBytesConsumerTest.cpp:309:
testing::runPendingTasks();
ditto.

https://codereview.chromium.org/2365853002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/ReadableStreamBytesConsumerTest.cpp:343:
testing::runPendingTasks();
ditto.

https://codereview.chromium.org/2365853002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/ReadableStreamBytesConsumerTest.cpp:348:

StreamReaderShouldBeWeak and StreamReaderShouldBeWeakWhenReading are removed.
Could you explain why we can remove them?

https://codereview.chromium.org/2365853002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/Response.cpp (right):

https://codereview.chromium.org/2365853002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/Response.cpp:27: #include
"modules/fetch/ReadableStreamBytesConsumer.h"
nit: is this include needed? (no references to ReadableStreamBytesConsumer in
this cpp)

[yhirano, 2016-09-30 06:48:00]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 06:48:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-09-30 08:57:26]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 08:57:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-09-30 09:00:35]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 09:00:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-09-30 09:01:02]

https://codereview.chromium.org/2365853002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/ReadableStreamBytesConsumer.cpp
(right):

https://codereview.chromium.org/2365853002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/ReadableStreamBytesConsumer.cpp:108:
DCHECK_LE(m_pendingOffset, m_pendingBuffer->length());
On 2016/09/30 06:35:14, hiroshige wrote:
> This CL seems to allow 0-byte |m_pendingBuffer| while the previous impl
didn't.
> (e.g. we have DCHECK_LE here instead of DCHECK_LT, and we lack size check
around
> Line 192)
> What is the motivation for that?
> (The change is fine with me, just to check)

Because I think it's simpler.

https://codereview.chromium.org/2365853002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/ReadableStreamBytesConsumer.cpp:132: if
(m_pendingOffset == m_pendingBuffer->length()) {
On 2016/09/30 06:35:14, hiroshige wrote:
> Using '>=' instead of '==' is more robust for buffer overflow.

Done.

https://codereview.chromium.org/2365853002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/ReadableStreamBytesConsumer.h
(right):

https://codereview.chromium.org/2365853002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/ReadableStreamBytesConsumer.h:59: //
outside of ReadableStreamDataConsumerHandle.
On 2016/09/30 06:35:14, hiroshige wrote:
> nit: ReadableStreamBytesConsumer?

Done.

https://codereview.chromium.org/2365853002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/ReadableStreamBytesConsumerTest.cpp
(right):

https://codereview.chromium.org/2365853002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/ReadableStreamBytesConsumerTest.cpp:31:
class MockClient : public GarbageCollectedFinalized<MockClient>, public
BytesConsumer::Client {
On 2016/09/30 06:35:14, hiroshige wrote:
> BytesConsumerForDataConsumerHandleTest.cpp also has MockClient.
> (Also there are two NoopClient and two TestClient)
> Please merge duplicated BytesConsumer::Client subclasses later.

Acknowledged.

https://codereview.chromium.org/2365853002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/ReadableStreamBytesConsumerTest.cpp:128:
testing::runPendingTasks();
On 2016/09/30 06:35:15, hiroshige wrote:
> Could you add checkpoint.Call() before runPendingTasks() to ensure that
> onStateChange() is not called synchronously in beginRead()?

Done.

https://codereview.chromium.org/2365853002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/ReadableStreamBytesConsumerTest.cpp:158:
testing::runPendingTasks();
On 2016/09/30 06:35:14, hiroshige wrote:
> ditto.

Done.

https://codereview.chromium.org/2365853002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/ReadableStreamBytesConsumerTest.cpp:205:
testing::runPendingTasks();
On 2016/09/30 06:35:14, hiroshige wrote:
> ditto.

Done.

https://codereview.chromium.org/2365853002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/ReadableStreamBytesConsumerTest.cpp:229:
testing::runPendingTasks();
On 2016/09/30 06:35:15, hiroshige wrote:
> ditto.

Done.

https://codereview.chromium.org/2365853002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/ReadableStreamBytesConsumerTest.cpp:240:
testing::runPendingTasks();
On 2016/09/30 06:35:14, hiroshige wrote:
> ditto.

Done.

https://codereview.chromium.org/2365853002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/ReadableStreamBytesConsumerTest.cpp:274:
testing::runPendingTasks();
On 2016/09/30 06:35:14, hiroshige wrote:
> ditto.

Done.

https://codereview.chromium.org/2365853002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/ReadableStreamBytesConsumerTest.cpp:309:
testing::runPendingTasks();
On 2016/09/30 06:35:15, hiroshige wrote:
> ditto.

Done.

https://codereview.chromium.org/2365853002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/ReadableStreamBytesConsumerTest.cpp:343:
testing::runPendingTasks();
On 2016/09/30 06:35:14, hiroshige wrote:
> ditto.

Done.

https://codereview.chromium.org/2365853002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/ReadableStreamBytesConsumerTest.cpp:348:

On 2016/09/30 06:35:14, hiroshige wrote:
> StreamReaderShouldBeWeak and StreamReaderShouldBeWeakWhenReading are removed.
> Could you explain why we can remove them?

Because ReadableStreamBytesConsumer expects that someone holds the stream
reference appropriately. It has DCHECKs for that and they will fail with such
tests.

https://codereview.chromium.org/2365853002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/Response.cpp (right):

https://codereview.chromium.org/2365853002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/Response.cpp:27: #include
"modules/fetch/ReadableStreamBytesConsumer.h"
On 2016/09/30 06:35:15, hiroshige wrote:
> nit: is this include needed? (no references to ReadableStreamBytesConsumer in
> this cpp)

Done.

[commit-bot: I haz the power, 2016-09-30 10:14:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-30 10:14:27]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2016-09-30 11:46:25]

lgtm.

https://codereview.chromium.org/2365853002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/ReadableStreamBytesConsumer.cpp
(right):

https://codereview.chromium.org/2365853002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/ReadableStreamBytesConsumer.cpp:108:
DCHECK_LE(m_pendingOffset, m_pendingBuffer->length());
On 2016/09/30 09:01:02, yhirano wrote:
> On 2016/09/30 06:35:14, hiroshige wrote:
> > This CL seems to allow 0-byte |m_pendingBuffer| while the previous impl
> didn't.
> > (e.g. we have DCHECK_LE here instead of DCHECK_LT, and we lack size check
> around
> > Line 192)
> > What is the motivation for that?
> > (The change is fine with me, just to check)
> 
> Because I think it's simpler.

Acknowledged.

https://codereview.chromium.org/2365853002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/ReadableStreamBytesConsumerTest.cpp
(right):

https://codereview.chromium.org/2365853002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/ReadableStreamBytesConsumerTest.cpp:348:

On 2016/09/30 09:01:02, yhirano wrote:
> On 2016/09/30 06:35:14, hiroshige wrote:
> > StreamReaderShouldBeWeak and StreamReaderShouldBeWeakWhenReading are
removed.
> > Could you explain why we can remove them?
> 
> Because ReadableStreamBytesConsumer expects that someone holds the stream
> reference appropriately. It has DCHECKs for that and they will fail with such
> tests.

Acknowledged.

[yhirano, 2016-09-30 15:00:17]

yhirano@chromium.org changed reviewers:
+ haraken@chromium.org

[yhirano, 2016-09-30 15:00:17]

+haraken@ for modules/BUILD.gn.

[haraken, 2016-10-03 01:35:33]

LGTM

[yhirano, 2016-10-03 01:56:07]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-03 01:56:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-03 02:39:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-03 02:39:43]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[yhirano, 2016-10-03 03:37:45]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2016-10-03 03:37:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
hiroshige@chromium.org, haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2365853002/#ps180001
(title: "git cl format")

[commit-bot: I haz the power, 2016-10-03 03:37:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-03 04:11:42]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-10-03 04:13:45]

Description was changed from

==========
Implement ReadableStreamBytesConsumer

This CL implements ReadableStreamBytesConsumer and replaces
ReadableStreamDataConsumerHandle with it.

BUG=610195
==========

to

==========
Implement ReadableStreamBytesConsumer

This CL implements ReadableStreamBytesConsumer and replaces
ReadableStreamDataConsumerHandle with it.

BUG=610195

Committed: https://crrev.com/bf54066c4ef19c4815dc5433d4152a322686a8c0
Cr-Commit-Position: refs/heads/master@{#422369}
==========

[commit-bot: I haz the power, 2016-10-03 04:13:46]

Patchset 10 (id:??) landed as
https://crrev.com/bf54066c4ef19c4815dc5433d4152a322686a8c0
Cr-Commit-Position: refs/heads/master@{#422369}