Make FetchDataLoader accept BytesConsumer

Make FetchDataLoader accept BytesConsumer

This CL makes FetchDataLoader accept a BytesConsumer instead of
FetchDataConsumerHandle.

This CL removes many statements clearing pointers, because

- As onStateChange will not be called after the consumer is
  closed or errored, clearing |m_client| is not necessary.
- Unlike FetchDataConsumerHandle::Reader, BytesConsumer doesn't
  rely on the destructor. Instead, we need to cancel it appropriately.
- Clearing |m_decoder| is not necessary and they will be
  destructed when the loader is destructed.

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

[yhirano, 2016-08-10 07:23:33]

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

[commit-bot: I haz the power, 2016-08-10 07:23:50]

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-10 09:19:38]

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

[commit-bot: I haz the power, 2016-08-10 09:19:39]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-08-10 10:59:00]

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

[commit-bot: I haz the power, 2016-08-10 10:59:24]

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

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

[commit-bot: I haz the power, 2016-08-10 11:05: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-10 11:11:25]

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

[commit-bot: I haz the power, 2016-08-10 11:11:45]

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-10 11:14:20]

Patchset #3 (id:40001) has been deleted

[yhirano, 2016-08-10 11:14:22]

Patchset #2 (id:20001) has been deleted

[yhirano, 2016-08-10 11:14:29]

Patchset #1 (id:1) has been deleted

[yhirano, 2016-08-10 11:15:35]

Description was changed from

==========
Make FetchDataLoader take ownership of FetchDataConsumerHandle

This is a preparation for making FetchDataLoader use BytesConsumer instead
of FetchDataConsumerHandle. This CL doesn't change the behavior.

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

to

==========
Make FetchDataLoader accept BytesConsumer

This CL makes FetchDataLoader accept a BytesConsumer instead of
FetchDataConsumerHandle.

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

[yhirano, 2016-08-10 11:15:35]

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

[yhirano, 2016-08-10 11:20:11]

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

[commit-bot: I haz the power, 2016-08-10 11:20:27]

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-10 11:53:56]

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

[commit-bot: I haz the power, 2016-08-10 11:53:57]

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

[hiroshige, 2016-08-12 06:02:31]

Unit tests are failing.

[yhirano, 2016-08-12 06:26:57]

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

[commit-bot: I haz the power, 2016-08-12 06:27:03]

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-12 07:44:11]

On 2016/08/12 06:02:31, hiroshige wrote:
> Unit tests are failing.

Sorry, fixed.

[commit-bot: I haz the power, 2016-08-12 08:18:45]

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

[commit-bot: I haz the power, 2016-08-12 08:18:46]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2016-08-12 08:21:17]

(From offline discussion)
This CL makes FetchDataLoader not to clear when done/errored/cancelled:
- |m_client| because BytesConsumer must ensure no notifications are done to its
client (i.e. FetchDataLoader)
- |m_consumer| because, unlike FetchDataConsumerHandle::Reader, |m_consumer|
don't rely on its destructor much and we don't need destruct after the consumer
becomes done/errored or is cancelled.
- |m_decoder| etc. Clearing |m_decoder| etc. would still have effect on
destructing the pointed object, so this CL delays |*m_decoder| destruction from
the point of done/errored/cancelled to the FetchDataLoader's destruction. Is
this OK? (perhaps OK because FetchDataLoader is not expected to be alive long
after its completion)

Could you briefly mention these changes in the CL description?

https://codereview.chromium.org/2229313002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp (left):

https://codereview.chromium.org/2229313002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp:352: // process.
Is this issue fixed?

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

https://codereview.chromium.org/2229313002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp:122:
DCHECK(m_consumer);
This CL removes DCHECK(m_client) and DCHECK(m_consumer) in
FetchDataLoaderAsBlobHandle because we no longer explicitly clears these
pointers, while keeps the DCHECKs here. Can we make them consistent?

https://codereview.chromium.org/2229313002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp:187:
DCHECK(m_consumer);
ditto.

https://codereview.chromium.org/2229313002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp:253:
DCHECK(m_consumer);
ditto.

[yhirano, 2016-08-12 09:10:53]

Description was changed from

==========
Make FetchDataLoader accept BytesConsumer

This CL makes FetchDataLoader accept a BytesConsumer instead of
FetchDataConsumerHandle.

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

to

==========
Make FetchDataLoader accept BytesConsumer

This CL makes FetchDataLoader accept a BytesConsumer instead of
FetchDataConsumerHandle.

This CL removes many statements clearing pointers, because

- As onStateChange will not be called after the consumer is
  closed or errored, clearing |m_client| is not necessary.
- Unlike FetchDataConsumerHandle::Reader, BytesConsumer doesn't
  rely on the destructor. Instead, we need to cancel it appropriately.
- Clearing |m_decoder| is not necessary and they will be
  destructed when the loader is destructed.

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

[yhirano, 2016-08-12 09:18:33]

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

[commit-bot: I haz the power, 2016-08-12 09:18: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-08-12 12:36:55]

Updated the CL description.

https://codereview.chromium.org/2229313002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp (left):

https://codereview.chromium.org/2229313002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp:352: // process.
On 2016/08/12 08:21:17, hiroshige wrote:
> Is this issue fixed?

Oops, no.

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

https://codereview.chromium.org/2229313002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp:122:
DCHECK(m_consumer);
On 2016/08/12 08:21:17, hiroshige wrote:
> This CL removes DCHECK(m_client) and DCHECK(m_consumer) in
> FetchDataLoaderAsBlobHandle because we no longer explicitly clears these
> pointers, while keeps the DCHECKs here. Can we make them consistent?

Done.

https://codereview.chromium.org/2229313002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp:187:
DCHECK(m_consumer);
On 2016/08/12 08:21:17, hiroshige wrote:
> ditto.

Done.

https://codereview.chromium.org/2229313002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp:253:
DCHECK(m_consumer);
On 2016/08/12 08:21:17, hiroshige wrote:
> ditto.

Done.

[commit-bot: I haz the power, 2016-08-12 12:46:47]

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

[commit-bot: I haz the power, 2016-08-12 12:46:47]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2016-08-15 07:09:57]

lgtm.

https://codereview.chromium.org/2229313002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/BytesConsumer.h (right):

https://codereview.chromium.org/2229313002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BytesConsumer.h:62: // function cannot
be called when the state is closed or errored before
nit: how about "... but it is guaranteed that this function cannot be called
after the state is closed or errored"?

[yhirano, 2016-08-16 09:01:54]

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

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

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-16 09:02:21]

https://codereview.chromium.org/2229313002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/BytesConsumer.h (right):

https://codereview.chromium.org/2229313002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BytesConsumer.h:62: // function cannot
be called when the state is closed or errored before
On 2016/08/15 07:09:57, hiroshige wrote:
> nit: how about "... but it is guaranteed that this function cannot be called
> after the state is closed or errored"?

Done.

[commit-bot: I haz the power, 2016-08-16 10:23:40]

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

[commit-bot: I haz the power, 2016-08-16 10:23:41]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-08-17 04:11:43]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2016-08-17 04:11:44]

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

[commit-bot: I haz the power, 2016-08-17 04:11:59]

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-17 04:15:11]

Description was changed from

==========
Make FetchDataLoader accept BytesConsumer

This CL makes FetchDataLoader accept a BytesConsumer instead of
FetchDataConsumerHandle.

This CL removes many statements clearing pointers, because

- As onStateChange will not be called after the consumer is
  closed or errored, clearing |m_client| is not necessary.
- Unlike FetchDataConsumerHandle::Reader, BytesConsumer doesn't
  rely on the destructor. Instead, we need to cancel it appropriately.
- Clearing |m_decoder| is not necessary and they will be
  destructed when the loader is destructed.

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

to

==========
Make FetchDataLoader accept BytesConsumer

This CL makes FetchDataLoader accept a BytesConsumer instead of
FetchDataConsumerHandle.

This CL removes many statements clearing pointers, because

- As onStateChange will not be called after the consumer is
  closed or errored, clearing |m_client| is not necessary.
- Unlike FetchDataConsumerHandle::Reader, BytesConsumer doesn't
  rely on the destructor. Instead, we need to cancel it appropriately.
- Clearing |m_decoder| is not necessary and they will be
  destructed when the loader is destructed.

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

[commit-bot: I haz the power, 2016-08-17 04:15:12]

Committed patchset #5 (id:140001)

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

Description was changed from

==========
Make FetchDataLoader accept BytesConsumer

This CL makes FetchDataLoader accept a BytesConsumer instead of
FetchDataConsumerHandle.

This CL removes many statements clearing pointers, because

- As onStateChange will not be called after the consumer is
  closed or errored, clearing |m_client| is not necessary.
- Unlike FetchDataConsumerHandle::Reader, BytesConsumer doesn't
  rely on the destructor. Instead, we need to cancel it appropriately.
- Clearing |m_decoder| is not necessary and they will be
  destructed when the loader is destructed.

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

to

==========
Make FetchDataLoader accept BytesConsumer

This CL makes FetchDataLoader accept a BytesConsumer instead of
FetchDataConsumerHandle.

This CL removes many statements clearing pointers, because

- As onStateChange will not be called after the consumer is
  closed or errored, clearing |m_client| is not necessary.
- Unlike FetchDataConsumerHandle::Reader, BytesConsumer doesn't
  rely on the destructor. Instead, we need to cancel it appropriately.
- Clearing |m_decoder| is not necessary and they will be
  destructed when the loader is destructed.

BUG=610195

Committed: https://crrev.com/6963b6b43faef1611ba0c380c3de7aa4ec22a1ed
Cr-Commit-Position: refs/heads/master@{#412447}
==========

[commit-bot: I haz the power, 2016-08-17 04:16:31]

Patchset 5 (id:??) landed as
https://crrev.com/6963b6b43faef1611ba0c380c3de7aa4ec22a1ed
Cr-Commit-Position: refs/heads/master@{#412447}