Implement BlobBytesConsumer

Implement BlobBytesConsumer

This CL implements BlobBytesConsumer which will replace
FetchBlobDataConsumerHandle.

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

[yhirano, 2016-08-29 11:44:25]

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

[commit-bot: I haz the power, 2016-08-29 11:44:46]

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-08-29 13:00:09]

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

[commit-bot: I haz the power, 2016-08-29 13:00:10]

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-08-30 01:42:55]

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

[yhirano, 2016-08-30 02:04:08]

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

[commit-bot: I haz the power, 2016-08-30 02:46:56]

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

[yhirano, 2016-08-30 03:56:28]

Patchset #2 (id:20001) has been deleted

[yhirano, 2016-08-30 03:56:33]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-08-30 03:59:18]

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

[commit-bot: I haz the power, 2016-08-30 03:59:19]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-08-30 04:13:40]

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

[yhirano, 2016-08-30 04:14:29]

Description was changed from

==========
Implement BlobBytesConsumer

wip

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

to

==========
Implement BlobBytesConsumer

This CL implements BlobBytesConsumer which will replace
FetchBlobDataConsumerHandle.

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

[commit-bot: I haz the power, 2016-08-30 04:17:42]

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

[yhirano, 2016-08-30 05:54:20]

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

[commit-bot: I haz the power, 2016-08-30 05:57:47]

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

[commit-bot: I haz the power, 2016-08-30 05:57:48]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-08-30 07:30:13]

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

[commit-bot: I haz the power, 2016-08-30 07:30:29]

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-08-30 08:26:15]

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

[commit-bot: I haz the power, 2016-08-30 08:26:16]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yhirano, 2016-09-08 01:52:51]

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

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

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-08 02:04:57]

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

[commit-bot: I haz the power, 2016-09-08 02:04:57]

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

[yhirano, 2016-09-08 02:09:20]

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

[commit-bot: I haz the power, 2016-09-08 02:09:40]

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-08 03:37:04]

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

[commit-bot: I haz the power, 2016-09-08 03:37:05]

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_...)

[yhirano, 2016-09-08 12:08:14]

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

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

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-08 14:00:48]

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

[commit-bot: I haz the power, 2016-09-08 14:00:49]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-09-12 08:09:47]

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

[commit-bot: I haz the power, 2016-09-12 08:09:54]

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

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

[commit-bot: I haz the power, 2016-09-12 08:21:55]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yhirano, 2016-09-12 08:32:42]

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

[commit-bot: I haz the power, 2016-09-12 08:32:53]

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-12 10:02:16]

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

[commit-bot: I haz the power, 2016-09-12 10:02:17]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-09-13 03:25:25]

hiroshige@, can you take a look?

[hiroshige, 2016-09-13 03:57:23]

https://codereview.chromium.org/2287323002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp (right):

https://codereview.chromium.org/2287323002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp:53:
SecurityOrigin* origin = getExecutionContext()->getSecurityOrigin();
SecurityOrigin is RefCounted and thus using a raw pointer looks unsafe.

https://codereview.chromium.org/2287323002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp:68: //
'setExternalRequestStateFromRequestorAddressSpace', as 'data:'
'blob:' instead of 'data:'? (not sure)

https://codereview.chromium.org/2287323002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp:146: void
BlobBytesConsumer::cancel()
Is it safe to cancel ThreadableLoader in a prefinalizer?

https://codereview.chromium.org/2287323002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp:225: // This
function is called synchronously in ThreadableLoader::start.
According to beginRead(), ThreadableLoader::start() is called after
|m_hasStarted| is set to true and thus |m_hasStarted| is true even the callbacks
are called synchronously?

https://codereview.chromium.org/2287323002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp:237: if
(!m_hasStarted) {
ditto.

https://codereview.chromium.org/2287323002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp:306: if
(!m_hasStarted) {
ditto.

[yhirano, 2016-09-13 05:11:57]

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

[commit-bot: I haz the power, 2016-09-13 05:12:12]

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-13 05:13:51]

https://codereview.chromium.org/2287323002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp (right):

https://codereview.chromium.org/2287323002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp:53:
SecurityOrigin* origin = getExecutionContext()->getSecurityOrigin();
On 2016/09/13 03:57:23, hiroshige wrote:
> SecurityOrigin is RefCounted and thus using a raw pointer looks unsafe.

Done.

https://codereview.chromium.org/2287323002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp:68: //
'setExternalRequestStateFromRequestorAddressSpace', as 'data:'
On 2016/09/13 03:57:23, hiroshige wrote:
> 'blob:' instead of 'data:'? (not sure)

Done.

https://codereview.chromium.org/2287323002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp:146: void
BlobBytesConsumer::cancel()
On 2016/09/13 03:57:23, hiroshige wrote:
> Is it safe to cancel ThreadableLoader in a prefinalizer?

I think so.

https://codereview.chromium.org/2287323002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp:225: // This
function is called synchronously in ThreadableLoader::start.
On 2016/09/13 03:57:23, hiroshige wrote:
> According to beginRead(), ThreadableLoader::start() is called after
> |m_hasStarted| is set to true and thus |m_hasStarted| is true even the
callbacks
> are called synchronously?

Done.

https://codereview.chromium.org/2287323002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp:237: if
(!m_hasStarted) {
On 2016/09/13 03:57:23, hiroshige wrote:
> ditto.

Done.

https://codereview.chromium.org/2287323002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp:306: if
(!m_hasStarted) {
On 2016/09/13 03:57:23, hiroshige wrote:
> ditto.

Done.

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

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

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

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

[hiroshige, 2016-09-13 08:35:44]

We have bunch of booleans and pointers that can be null or non-null:
m_blobDataHandle
m_body
m_client
m_loader
m_hasStarted
m_hasReceivedResponse
m_hasSeenEndOfData
m_hasFinishedLoading
m_hasFailedToLoad
I think there should also be invariants to hold, but I feel this state control
is too complex.
Can we simplify the state control e.g. by turning booleans to a single state
enum?

How about adding tests for the cases where something
(didFail()/didFinishLoading()/didReceiveResponse()) occurs during two-phase
read?

Currently don't we have tests where ThreadableLoaderClient's methods are called
synchronously?
(I'm not sure whether we can add such tests though.)

https://codereview.chromium.org/2287323002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp (right):

https://codereview.chromium.org/2287323002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp:265:
didFailInternal();
nit: Can we place NOTREACHED()?
(FetchBlobDCH's BlobLoaderContext has ASSERT_NOT_REACHED())

https://codereview.chromium.org/2287323002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/BlobBytesConsumerTest.cpp (right):

https://codereview.chromium.org/2287323002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumerTest.cpp:309:
EXPECT_EQ(Result::Ok, consumer->read(buffer, sizeof(buffer), &read));
How about testing |read| and the contents of |buffer|?

https://codereview.chromium.org/2287323002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumerTest.cpp:352:
EXPECT_EQ(Result::Ok, consumer->read(buffer, sizeof(buffer), &read));
How about testing |read| and the contents of |buffer|?

[yhirano, 2016-09-14 07:13:45]

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

[commit-bot: I haz the power, 2016-09-14 07:13:58]

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-14 07:28:39]

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

[commit-bot: I haz the power, 2016-09-14 07:28:55]

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-14 07:33:25]

https://codereview.chromium.org/2287323002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp (right):

https://codereview.chromium.org/2287323002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp:265:
didFailInternal();
On 2016/09/13 08:35:44, hiroshige wrote:
> nit: Can we place NOTREACHED()?
> (FetchBlobDCH's BlobLoaderContext has ASSERT_NOT_REACHED())

Done.

https://codereview.chromium.org/2287323002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/BlobBytesConsumerTest.cpp (right):

https://codereview.chromium.org/2287323002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumerTest.cpp:309:
EXPECT_EQ(Result::Ok, consumer->read(buffer, sizeof(buffer), &read));
On 2016/09/13 08:35:44, hiroshige wrote:
> How about testing |read| and the contents of |buffer|?

Done.

https://codereview.chromium.org/2287323002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumerTest.cpp:352:
EXPECT_EQ(Result::Ok, consumer->read(buffer, sizeof(buffer), &read));
On 2016/09/13 08:35:44, hiroshige wrote:
> How about testing |read| and the contents of |buffer|?

Done.

[yhirano, 2016-09-14 07:35:01]

> Can we simplify the state control e.g. by turning booleans to a single state
enum?

PS11 uses

 - m_state (a BytesConsumer::PublicState),
 - m_hasSeenEndOfData,
 - m_hasFinishedLoading,
 - isClean()

.

[commit-bot: I haz the power, 2016-09-14 08:29:00]

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

[commit-bot: I haz the power, 2016-09-14 08:29:02]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yhirano, 2016-09-14 08:50:56]

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

[commit-bot: I haz the power, 2016-09-14 08:51:07]

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-14 08:57:08]

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

[commit-bot: I haz the power, 2016-09-14 08:57: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-09-14 10:47:08]

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

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

Dry run: This issue passed the CQ dry run.

[hiroshige, 2016-09-15 05:54:51]

https://codereview.chromium.org/2287323002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp (right):

https://codereview.chromium.org/2287323002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp:75: if
(m_hasSeenEndOfData && m_hasFinishedLoading) {
This if statement is removed in Patch Set 12.
Do we need |if (m_state == PublicState::Closed) { return Done; }|?

https://codereview.chromium.org/2287323002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp (right):

https://codereview.chromium.org/2287323002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp:169: if (m_loader)
{
Do we need this block?
when |m_state| is set to Errored/Closed, |m_loader| seems cleared.

https://codereview.chromium.org/2287323002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp:177: m_state =
PublicState::Errored;
Can we make a method like

void BlobBytesConsumer::setState(PublicState newState, bool shouldNotifyClient)
{
    m_state = newState;
    BytesConsumer::Client* client = m_client;
    clearIfNecessary();
    if (client && shouldNotifyClient)
        client->onStateChange();
}

and use it when changing state?

- By calling clearIfNecessary() before onStateChange(), we can make the
invariant that |m_loader| is null when m_state is Done/Errored stronger, and
- There are multiple occurrences of "set m_state, onStateChange(), and
clearIfNecessary()". Merging them into a single method is helpful to make code
clearer.

[yhirano, 2016-09-15 08:11:09]

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:11:26]

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:13:32]

https://codereview.chromium.org/2287323002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp (right):

https://codereview.chromium.org/2287323002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp:75: if
(m_hasSeenEndOfData && m_hasFinishedLoading) {
On 2016/09/15 05:54:51, hiroshige wrote:
> This if statement is removed in Patch Set 12.
> Do we need |if (m_state == PublicState::Closed) { return Done; }|?

I found that this should not happen because m_hasSeenEndOfData cannot be true
until we read data (or cancel the consumer).

https://codereview.chromium.org/2287323002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp (right):

https://codereview.chromium.org/2287323002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp:169: if (m_loader)
{
On 2016/09/15 05:54:51, hiroshige wrote:
> Do we need this block?
> when |m_state| is set to Errored/Closed, |m_loader| seems cleared.

Done.

https://codereview.chromium.org/2287323002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp:177: m_state =
PublicState::Errored;
On 2016/09/15 05:54:51, hiroshige wrote:
> Can we make a method like
> 
> void BlobBytesConsumer::setState(PublicState newState, bool
shouldNotifyClient)
> {
>     m_state = newState;
>     BytesConsumer::Client* client = m_client;
>     clearIfNecessary();
>     if (client && shouldNotifyClient)
>         client->onStateChange();
> }
> 
> and use it when changing state?
> 
> - By calling clearIfNecessary() before onStateChange(), we can make the
> invariant that |m_loader| is null when m_state is Done/Errored stronger, and
> - There are multiple occurrences of "set m_state, onStateChange(), and
> clearIfNecessary()". Merging them into a single method is helpful to make code
> clearer.

I'm more comfortable with saving m_client manually. Is this acceptable to you?

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

Manually saving m_client also looks good.
Can we always call error()/close() when setting state to Errored/Closed for
consistency and to make it clearer that loader and client are null after
Errored/Closed?

https://codereview.chromium.org/2287323002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp (right):

https://codereview.chromium.org/2287323002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp:53: m_state =
PublicState::Errored;
Can we call error() instead of setting |m_state|?

https://codereview.chromium.org/2287323002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp:111: m_state =
PublicState::Closed;
Can we call close() instead of setting |m_state|?

https://codereview.chromium.org/2287323002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp:222: m_state =
PublicState::Closed;
Can we call close() instead of setting |m_state|?
(Needs to save |m_client|)

https://codereview.chromium.org/2287323002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp:236: m_state =
PublicState::Errored;
Can we call error() instead of setting |m_state|?
(Set |m_loader = nullptr;| and then error(). Needs to save |m_client|)

[yhirano, 2016-09-15 09:09:20]

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

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

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 09:11:22]

https://codereview.chromium.org/2287323002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp (right):

https://codereview.chromium.org/2287323002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp:53: m_state =
PublicState::Errored;
On 2016/09/15 09:04:34, hiroshige wrote:
> Can we call error() instead of setting |m_state|?

Done.

https://codereview.chromium.org/2287323002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp:111: m_state =
PublicState::Closed;
On 2016/09/15 09:04:34, hiroshige wrote:
> Can we call close() instead of setting |m_state|?

Done.

https://codereview.chromium.org/2287323002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp:222: m_state =
PublicState::Closed;
On 2016/09/15 09:04:34, hiroshige wrote:
> Can we call close() instead of setting |m_state|?
> (Needs to save |m_client|)

Done.

https://codereview.chromium.org/2287323002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp:236: m_state =
PublicState::Errored;
On 2016/09/15 09:04:34, hiroshige wrote:
> Can we call error() instead of setting |m_state|?
> (Set |m_loader = nullptr;| and then error(). Needs to save |m_client|)

Done.

[commit-bot: I haz the power, 2016-09-15 12:44:28]

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

[commit-bot: I haz the power, 2016-09-15 12:44:29]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-09-21 01:47:24]

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

[commit-bot: I haz the power, 2016-09-21 01:47:57]

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 02:13:42]

I rebased this CL on https://codereview.chromium.org/2351643004. PTAL.

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

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

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

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-21 03:59:13]

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

[commit-bot: I haz the power, 2016-09-21 03:59: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-21 05:20:51]

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

[commit-bot: I haz the power, 2016-09-21 05:20:52]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-09-27 05:42:54]

On 2016/09/21 02:13:42, yhirano wrote:
> I rebased this CL on https://codereview.chromium.org/2351643004. PTAL.

ping

[hiroshige, 2016-09-27 07:26:24]

lgtm.

https://codereview.chromium.org/2287323002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp (right):

https://codereview.chromium.org/2287323002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp:28: if (m_loader)
{
nit: Could you add a comment that says this if block is only for tests? (because
cancelling ThreadableLoader in a constructor looks unusual in general)

[yhirano, 2016-09-27 07:29:40]

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

[commit-bot: I haz the power, 2016-09-27 07:29:58]

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-27 07:30:06]

https://codereview.chromium.org/2287323002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp (right):

https://codereview.chromium.org/2287323002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp:28: if (m_loader)
{
On 2016/09/27 07:26:23, hiroshige wrote:
> nit: Could you add a comment that says this if block is only for tests?
(because
> cancelling ThreadableLoader in a constructor looks unusual in general)

Done.

[yhirano, 2016-09-27 07:30:46]

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

[yhirano, 2016-09-27 07:30:48]

+haraken@ for modules/BUILD.gn.

[hiroshige, 2016-09-27 07:31:36]

One additional comment:

https://codereview.chromium.org/2287323002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/BlobBytesConsumerTest.cpp (right):

https://codereview.chromium.org/2287323002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumerTest.cpp:404:
EXPECT_EQ(12345u, result->size());
Could you check |EXPECT_FALSE(loader->isStarted());| here?

BodyStreamBufferTest checks that ThreadableLoader is not created for draining
cases, but https://codereview.chromium.org/2342233002/ will remove the checks
(i.e. FakeLoaderFactory).
Checking the condition here will keep the coverage.

https://codereview.chromium.org/2287323002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumerTest.cpp:420:
EXPECT_EQ(UINT64_MAX, result->size());
ditto.

https://codereview.chromium.org/2287323002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumerTest.cpp:432:
EXPECT_FALSE(consumer->drainAsBlobDataHandle(BytesConsumer::BlobSizePolicy::DisallowBlobWithInvalidSize));
ditto.

https://codereview.chromium.org/2287323002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumerTest.cpp:450:
EXPECT_EQ(blobDataHandle->uuid(), result->elements()[0].m_blobUUID);
ditto.

[yhirano, 2016-09-27 07:43:51]

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

[commit-bot: I haz the power, 2016-09-27 07:44:09]

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-27 07:47:06]

https://codereview.chromium.org/2287323002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/BlobBytesConsumerTest.cpp (right):

https://codereview.chromium.org/2287323002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumerTest.cpp:404:
EXPECT_EQ(12345u, result->size());
On 2016/09/27 07:31:35, hiroshige wrote:
> Could you check |EXPECT_FALSE(loader->isStarted());| here?
> 
> BodyStreamBufferTest checks that ThreadableLoader is not created for draining
> cases, but https://codereview.chromium.org/2342233002/ will remove the checks
> (i.e. FakeLoaderFactory).
> Checking the condition here will keep the coverage.

Done.

https://codereview.chromium.org/2287323002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumerTest.cpp:420:
EXPECT_EQ(UINT64_MAX, result->size());
On 2016/09/27 07:31:35, hiroshige wrote:
> ditto.

Done.

https://codereview.chromium.org/2287323002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumerTest.cpp:432:
EXPECT_FALSE(consumer->drainAsBlobDataHandle(BytesConsumer::BlobSizePolicy::DisallowBlobWithInvalidSize));
On 2016/09/27 07:31:35, hiroshige wrote:
> ditto.

Done.

https://codereview.chromium.org/2287323002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumerTest.cpp:450:
EXPECT_EQ(blobDataHandle->uuid(), result->elements()[0].m_blobUUID);
On 2016/09/27 07:31:35, hiroshige wrote:
> ditto.

Done.

[haraken, 2016-09-27 08:36:20]

LGTM

https://codereview.chromium.org/2287323002/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp (right):

https://codereview.chromium.org/2287323002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp:140: void
BlobBytesConsumer::clearClient()

Where is this used?

[commit-bot: I haz the power, 2016-09-27 08:56:55]

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

[commit-bot: I haz the power, 2016-09-27 08:56:57]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-09-27 09:12:50]

https://codereview.chromium.org/2287323002/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp (right):

https://codereview.chromium.org/2287323002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp:140: void
BlobBytesConsumer::clearClient()
On 2016/09/27 08:36:20, haraken wrote:
> 
> Where is this used?

This is an override function and called via the BytesConsumer interface.

[yhirano, 2016-09-27 11:30:14]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2016-09-27 11:30:15]

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/2287323002/#ps400001
(title: "fix")

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

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 11:35:24]

Description was changed from

==========
Implement BlobBytesConsumer

This CL implements BlobBytesConsumer which will replace
FetchBlobDataConsumerHandle.

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

to

==========
Implement BlobBytesConsumer

This CL implements BlobBytesConsumer which will replace
FetchBlobDataConsumerHandle.

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

[commit-bot: I haz the power, 2016-09-27 11:35:25]

Committed patchset #19 (id:400001)

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

Description was changed from

==========
Implement BlobBytesConsumer

This CL implements BlobBytesConsumer which will replace
FetchBlobDataConsumerHandle.

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

to

==========
Implement BlobBytesConsumer

This CL implements BlobBytesConsumer which will replace
FetchBlobDataConsumerHandle.

BUG=610195

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

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

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