Introduce FormDataBytesConsumer

Introduce FormDataBytesConsumer

This CL introduces FormDataBytesConsumer which will replace
FetchFormDataConsumerHandle.

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

[yhirano, 2016-09-15 07:54:07]

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

[commit-bot: I haz the power, 2016-09-15 07:54:22]

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-15 08:52:17]

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

[commit-bot: I haz the power, 2016-09-15 08:52:28]

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-15 08:52:54]

Description was changed from

==========
Introduce FormDataBytesConsumer


wip


wip wip


wip

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

to

==========
Introduce FormDataBytesConsumer

This CL introduces FormDataBytesConsumer which will replace
FetchFormDataConsumerHandle.

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

[yhirano, 2016-09-15 08:52:54]

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

[hiroshige, 2016-09-15 09:47:34]

Basically looks good.

https://codereview.chromium.org/2342023002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/fetch/FormDataBytesConsumer.cpp (right):

https://codereview.chromium.org/2342023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FormDataBytesConsumer.cpp:21: bool
isSimple(const EncodedFormData* formData)
Memo: this is dup of isSimple() in FetchFormDataConsumerHandle.cpp.
This isn't a problem provided FetchFormDataConsumerHandle.cpp will be deleted
soon.

https://codereview.chromium.org/2342023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FormDataBytesConsumer.cpp:153: return
m_blobBytesConsumer->beginRead(buffer, available);
IIUC this code returns Done after drainAsFormData() returns non-null, because
|m_blobBytesConsumer| is cancelled. Leaving a short comment might be helpful to
understand the behavior.

https://codereview.chromium.org/2342023002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/fetch/FormDataBytesConsumerTest.cpp
(right):

https://codereview.chromium.org/2342023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FormDataBytesConsumerTest.cpp:54: class
StubBytesConsumer final : public BytesConsumer {
optional: How about creating a class with gmock and testing by EXPECT_CALL()s?

https://codereview.chromium.org/2342023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FormDataBytesConsumerTest.cpp:166:
TEST_F(FormDataBytesConsumerTest, ReadFromSimplFormData)
nit: s/Simpl/Simple/

https://codereview.chromium.org/2342023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FormDataBytesConsumerTest.cpp:222:
EXPECT_EQ(Result::Done, consumer->read(&c, 1, &readSize));
Could you also test getPublicState()?

https://codereview.chromium.org/2342023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FormDataBytesConsumerTest.cpp:236:
EXPECT_EQ(Result::Done, consumer->read(&c, 1, &readSize));
ditto.

https://codereview.chromium.org/2342023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FormDataBytesConsumerTest.cpp:255:
EXPECT_EQ(Result::Done, consumer->read(&c, 1, &readSize));
ditto.

https://codereview.chromium.org/2342023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FormDataBytesConsumerTest.cpp:269:
EXPECT_EQ(Result::Done, consumer->read(&c, 1, &readSize));
ditto.

https://codereview.chromium.org/2342023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FormDataBytesConsumerTest.cpp:283:
EXPECT_EQ(Result::Done, consumer->beginRead(&buffer, &size));
ditto.

https://codereview.chromium.org/2342023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FormDataBytesConsumerTest.cpp:298:
EXPECT_EQ(Result::Done, consumer->beginRead(&buffer, &size));
ditto.

https://codereview.chromium.org/2342023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FormDataBytesConsumerTest.cpp:314:
EXPECT_EQ(Result::Done, consumer->beginRead(&buffer, &size));
ditto.

https://codereview.chromium.org/2342023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FormDataBytesConsumerTest.cpp:327:
EXPECT_EQ(Result::Done, consumer->beginRead(&buffer, &size));
ditto.

https://codereview.chromium.org/2342023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FormDataBytesConsumerTest.cpp:337:
EXPECT_FALSE(consumer->drainAsFormData());
Could you also test drainAsBlobDataHandle()?

https://codereview.chromium.org/2342023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FormDataBytesConsumerTest.cpp:349:
EXPECT_FALSE(consumer->drainAsFormData());
ditto.

https://codereview.chromium.org/2342023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FormDataBytesConsumerTest.cpp:351: 
Could you also test ReadAffectsDraining/BeginReadAffectsDraining for complex
form data?

[commit-bot: I haz the power, 2016-09-15 10:40:49]

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

[commit-bot: I haz the power, 2016-09-15 10:40:49]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-09-16 01:55:11]

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

[commit-bot: I haz the power, 2016-09-16 01:55: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-16 02:02:58]

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

[commit-bot: I haz the power, 2016-09-16 02:02:59]

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

[yhirano, 2016-09-16 02:47:33]

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

[commit-bot: I haz the power, 2016-09-16 02:47:47]

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-16 02:53:08]

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

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

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-16 02:57:03]

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

[commit-bot: I haz the power, 2016-09-16 02:57:25]

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-16 03:08:19]

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

[commit-bot: I haz the power, 2016-09-16 03:08:33]

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-16 03:10:32]

I failed the statement management in SimpleFormDataBytesConsumer, so I fixed it.

https://codereview.chromium.org/2342023002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/fetch/FormDataBytesConsumer.cpp (right):

https://codereview.chromium.org/2342023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FormDataBytesConsumer.cpp:21: bool
isSimple(const EncodedFormData* formData)
On 2016/09/15 09:47:33, hiroshige wrote:
> Memo: this is dup of isSimple() in FetchFormDataConsumerHandle.cpp.
> This isn't a problem provided FetchFormDataConsumerHandle.cpp will be deleted
> soon.

Acknowledged.

https://codereview.chromium.org/2342023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FormDataBytesConsumer.cpp:153: return
m_blobBytesConsumer->beginRead(buffer, available);
On 2016/09/15 09:47:33, hiroshige wrote:
> IIUC this code returns Done after drainAsFormData() returns non-null, because
> |m_blobBytesConsumer| is cancelled. Leaving a short comment might be helpful
to
> understand the behavior.

Done.

https://codereview.chromium.org/2342023002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/fetch/FormDataBytesConsumerTest.cpp
(right):

https://codereview.chromium.org/2342023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FormDataBytesConsumerTest.cpp:54: class
StubBytesConsumer final : public BytesConsumer {
On 2016/09/15 09:47:33, hiroshige wrote:
> optional: How about creating a class with gmock and testing by EXPECT_CALL()s?

Done.

https://codereview.chromium.org/2342023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FormDataBytesConsumerTest.cpp:166:
TEST_F(FormDataBytesConsumerTest, ReadFromSimplFormData)
On 2016/09/15 09:47:33, hiroshige wrote:
> nit: s/Simpl/Simple/

Done.

https://codereview.chromium.org/2342023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FormDataBytesConsumerTest.cpp:222:
EXPECT_EQ(Result::Done, consumer->read(&c, 1, &readSize));
On 2016/09/15 09:47:33, hiroshige wrote:
> Could you also test getPublicState()?

Done.

https://codereview.chromium.org/2342023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FormDataBytesConsumerTest.cpp:236:
EXPECT_EQ(Result::Done, consumer->read(&c, 1, &readSize));
On 2016/09/15 09:47:33, hiroshige wrote:
> ditto.

Done.

https://codereview.chromium.org/2342023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FormDataBytesConsumerTest.cpp:255:
EXPECT_EQ(Result::Done, consumer->read(&c, 1, &readSize));
On 2016/09/15 09:47:33, hiroshige wrote:
> ditto.

Done.

https://codereview.chromium.org/2342023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FormDataBytesConsumerTest.cpp:269:
EXPECT_EQ(Result::Done, consumer->read(&c, 1, &readSize));
On 2016/09/15 09:47:33, hiroshige wrote:
> ditto.

Done.

https://codereview.chromium.org/2342023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FormDataBytesConsumerTest.cpp:283:
EXPECT_EQ(Result::Done, consumer->beginRead(&buffer, &size));
On 2016/09/15 09:47:33, hiroshige wrote:
> ditto.

Done.

https://codereview.chromium.org/2342023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FormDataBytesConsumerTest.cpp:298:
EXPECT_EQ(Result::Done, consumer->beginRead(&buffer, &size));
On 2016/09/15 09:47:33, hiroshige wrote:
> ditto.

Done.

https://codereview.chromium.org/2342023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FormDataBytesConsumerTest.cpp:314:
EXPECT_EQ(Result::Done, consumer->beginRead(&buffer, &size));
On 2016/09/15 09:47:33, hiroshige wrote:
> ditto.

Done.

https://codereview.chromium.org/2342023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FormDataBytesConsumerTest.cpp:327:
EXPECT_EQ(Result::Done, consumer->beginRead(&buffer, &size));
On 2016/09/15 09:47:33, hiroshige wrote:
> ditto.

Done.

https://codereview.chromium.org/2342023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FormDataBytesConsumerTest.cpp:337:
EXPECT_FALSE(consumer->drainAsFormData());
On 2016/09/15 09:47:33, hiroshige wrote:
> Could you also test drainAsBlobDataHandle()?

Done.

https://codereview.chromium.org/2342023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FormDataBytesConsumerTest.cpp:349:
EXPECT_FALSE(consumer->drainAsFormData());
On 2016/09/15 09:47:33, hiroshige wrote:
> ditto.

Done.

https://codereview.chromium.org/2342023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FormDataBytesConsumerTest.cpp:351: 
On 2016/09/15 09:47:33, hiroshige wrote:
> Could you also test ReadAffectsDraining/BeginReadAffectsDraining for complex
> form data?

Done.

[commit-bot: I haz the power, 2016-09-16 04:44:50]

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

[commit-bot: I haz the power, 2016-09-16 04:44:51]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2016-09-16 07:24:34]

https://codereview.chromium.org/2342023002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/FormDataBytesConsumer.cpp (right):

https://codereview.chromium.org/2342023002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/FormDataBytesConsumer.cpp:59: m_state =
PublicState::Closed;
If we change the state to Closed here, shouldn't we notify the client?

https://codereview.chromium.org/2342023002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/FormDataBytesConsumerTest.cpp
(right):

https://codereview.chromium.org/2342023002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/FormDataBytesConsumerTest.cpp:367:
checkpoint.Call(3);
Could you test getPublicState() here?

[yhirano, 2016-09-21 02:05:48]

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

[commit-bot: I haz the power, 2016-09-21 02:06:22]

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-21 03:16:17]

https://codereview.chromium.org/2342023002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/FormDataBytesConsumer.cpp (right):

https://codereview.chromium.org/2342023002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/FormDataBytesConsumer.cpp:59: m_state =
PublicState::Closed;
On 2016/09/16 07:24:34, hiroshige wrote:
> If we change the state to Closed here, shouldn't we notify the client?

You're right, but as talked offline I changed the design so that endRead can
return Result::Done.

https://codereview.chromium.org/2342023002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/FormDataBytesConsumerTest.cpp
(right):

https://codereview.chromium.org/2342023002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/FormDataBytesConsumerTest.cpp:367:
checkpoint.Call(3);
On 2016/09/16 07:24:34, hiroshige wrote:
> Could you test getPublicState() here?

In this case consumer->getPublicState() is simply redirected to
underlying->getPublicState and we need to provide its return value manually. Do
you think it's valuable to test that here?

[commit-bot: I haz the power, 2016-09-21 03:42:45]

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

[commit-bot: I haz the power, 2016-09-21 03:42:45]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-09-21 04:05:31]

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

[commit-bot: I haz the power, 2016-09-21 04:05:47]

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-21 07:15:20]

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

[commit-bot: I haz the power, 2016-09-21 07:15:21]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2016-09-21 07:42:19]

https://codereview.chromium.org/2342023002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/FormDataBytesConsumer.cpp (right):

https://codereview.chromium.org/2342023002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/FormDataBytesConsumer.cpp:59: m_state =
PublicState::Closed;
On 2016/09/21 03:16:16, yhirano (slow) wrote:
> On 2016/09/16 07:24:34, hiroshige wrote:
> > If we change the state to Closed here, shouldn't we notify the client?
> 
> You're right, but as talked offline I changed the design so that endRead can
> return Result::Done.

Acknowledged.

https://codereview.chromium.org/2342023002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/FormDataBytesConsumerTest.cpp
(right):

https://codereview.chromium.org/2342023002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/FormDataBytesConsumerTest.cpp:367:
checkpoint.Call(3);
On 2016/09/21 03:16:16, yhirano (slow) wrote:
> On 2016/09/16 07:24:34, hiroshige wrote:
> > Could you test getPublicState() here?
> 
> In this case consumer->getPublicState() is simply redirected to
> underlying->getPublicState and we need to provide its return value manually.
Do
> you think it's valuable to test that here?

I think at least we should test getPublicState() does not return Done  unless we
completely read the data (in other words, |consumer| shouldn't change the state
to Done for failed drainAs*()).

[hiroshige, 2016-09-21 07:48:45]

lgtm if my previous comment for tests is addressed.

[yhirano, 2016-09-21 08:01:21]

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

[commit-bot: I haz the power, 2016-09-21 08:01:53]

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-21 08:02:12]

https://codereview.chromium.org/2342023002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/FormDataBytesConsumerTest.cpp
(right):

https://codereview.chromium.org/2342023002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/FormDataBytesConsumerTest.cpp:367:
checkpoint.Call(3);
On 2016/09/21 07:42:19, hiroshige wrote:
> On 2016/09/21 03:16:16, yhirano (slow) wrote:
> > On 2016/09/16 07:24:34, hiroshige wrote:
> > > Could you test getPublicState() here?
> > 
> > In this case consumer->getPublicState() is simply redirected to
> > underlying->getPublicState and we need to provide its return value manually.
> Do
> > you think it's valuable to test that here?
> 
> I think at least we should test getPublicState() does not return Done  unless
we
> completely read the data (in other words, |consumer| shouldn't change the
state
> to Done for failed drainAs*()).

Done.

[yhirano, 2016-09-21 08:02:31]

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

[yhirano, 2016-09-21 08:02:31]

+haraken@ for modules/BUILD.gn.

[haraken, 2016-09-21 08:05:25]

LGTM

[commit-bot: I haz the power, 2016-09-21 11:12:27]

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

[commit-bot: I haz the power, 2016-09-21 11:12:28]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-09-27 13:32:56]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2016-09-27 13:32:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
hiroshige@chromium.org
Link to the patchset: https://codereview.chromium.org/2342023002/#ps180001
(title: "fix")

[commit-bot: I haz the power, 2016-09-27 13:33:09]

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-27 15:38:25]

Description was changed from

==========
Introduce FormDataBytesConsumer

This CL introduces FormDataBytesConsumer which will replace
FetchFormDataConsumerHandle.

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

to

==========
Introduce FormDataBytesConsumer

This CL introduces FormDataBytesConsumer which will replace
FetchFormDataConsumerHandle.

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

[commit-bot: I haz the power, 2016-09-27 15:38:26]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-09-27 15:39:50]

Description was changed from

==========
Introduce FormDataBytesConsumer

This CL introduces FormDataBytesConsumer which will replace
FetchFormDataConsumerHandle.

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

to

==========
Introduce FormDataBytesConsumer

This CL introduces FormDataBytesConsumer which will replace
FetchFormDataConsumerHandle.

BUG=610195

Committed: https://crrev.com/1ceb0ca40370a0e0f40d8a9c379e6396672f4923
Cr-Commit-Position: refs/heads/master@{#421219}
==========

[commit-bot: I haz the power, 2016-09-27 15:39:51]

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