[Fetch API] Remove DrainingBuffer.

[Fetch API] Remove DrainingBuffer.

This is a Fetch API related refactoring CL. Its main focus is to remove
"draining buffer". Instead, BodyStreamBuffer has lock / unlock methods. A
user can call |lock| to get its underlying handle and call |unlock| to
release the handle.

Now |body| property is held in FetchRequestData and FetchResponseData
whereas previously it was held in Body implementation. This change
simplifies the property update (e.g. clone) and removes several
functions such as |refreshBody|.

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

[yhirano, 2015-07-13 04:12:29]

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

[yhirano, 2015-07-13 04:17:27]

Patchset #6 (id:100001) has been deleted

[yhirano, 2015-07-13 04:17:41]

Patchset #2 (id:20001) has been deleted

[yhirano, 2015-07-13 04:17:44]

Patchset #1 (id:1) has been deleted

[yhirano, 2015-07-13 04:17:54]

Patchset #3 (id:80001) has been deleted

[yhirano, 2015-07-13 04:17:57]

Patchset #2 (id:60001) has been deleted

[yhirano, 2015-07-13 04:18:01]

Patchset #1 (id:40001) has been deleted

[yhirano, 2015-07-21 05:12:58]

ping

[hiroshige, 2015-07-21 12:14:01]

https://codereview.chromium.org/1233573002/diff/140001/Source/modules/fetch/B...
File Source/modules/fetch/BodyStreamBuffer.h (right):

https://codereview.chromium.org/1233573002/diff/140001/Source/modules/fetch/B...
Source/modules/fetch/BodyStreamBuffer.h:40: // Note: There is a case that
calling |lock| doesn't make the buffer
The comments L40--L42, L44--L45 are complicated, and this is because the
inconsistency between the names lock()/unlock() and the name
isLocked()/ReadableByteStream::isLocked().
Probably changing the names lock()/unlock() to better names will resolve the
problem; use the word "locked" for ReadableByteStream, and another word for
BodyStreamBuffer. (currently no idea for the new name though...)

https://codereview.chromium.org/1233573002/diff/140001/Source/modules/fetch/B...
Source/modules/fetch/BodyStreamBuffer.h:43: PassOwnPtr<FetchDataConsumerHandle>
lock(ExecutionContext*);
In my understanding of this CL:
- lock()/unlock() should be carefully used (because human cannot book-keep
lock()-unlock() correspondence in general), and should be used inside
BodyStreamBuffer internally.
- But lock() is public due to two call sites to lock() outside BodyStreamBuffer,
which do not have corresponding unlock() (and we are not sure when to call
unlock()) and are to be removed (have such TODO comments). These call sites of
lock() corresponds to previous BodyStreamBuffer::releaseBuffer() calls (which
were also not so clean interface).
If this is correct, please add TODO comments to lock() (and possibly to
unlock()) that lock()/unlock() should be encapsulated in BodyStreamBuffer and
made private, and external users should not use lock()/unlock().

https://codereview.chromium.org/1233573002/diff/140001/Source/modules/fetch/B...
Source/modules/fetch/BodyStreamBuffer.h:47: void
unlock(PassOwnPtr<FetchDataConsumerHandle> /* handle */);
Please add a comment for what is |handle|.

https://codereview.chromium.org/1233573002/diff/140001/Source/modules/fetch/F...
File Source/modules/fetch/FetchManager.cpp (right):

https://codereview.chromium.org/1233573002/diff/140001/Source/modules/fetch/F...
Source/modules/fetch/FetchManager.cpp:332: buffer->unlock(handle.release());
Can we move L330-L332 to BodyStreamBuffer::drainAsBlobDataHandle()?
By doing so, we can remove all unlock() calls from outside BodyStreamBuffer and
make unlock() private.

https://codereview.chromium.org/1233573002/diff/140001/Source/modules/service...
File Source/modules/serviceworkers/RespondWithObserver.cpp (right):

https://codereview.chromium.org/1233573002/diff/140001/Source/modules/service...
Source/modules/serviceworkers/RespondWithObserver.cpp:174:
buffer->unlock(handle.release());
Same for the comment in FetchManager.cpp.

[yhirano, 2015-07-22 09:42:45]

https://codereview.chromium.org/1233573002/diff/140001/Source/modules/fetch/B...
File Source/modules/fetch/BodyStreamBuffer.h (right):

https://codereview.chromium.org/1233573002/diff/140001/Source/modules/fetch/B...
Source/modules/fetch/BodyStreamBuffer.h:40: // Note: There is a case that
calling |lock| doesn't make the buffer
On 2015/07/21 12:14:01, hiroshige wrote:
> The comments L40--L42, L44--L45 are complicated, and this is because the
> inconsistency between the names lock()/unlock() and the name
> isLocked()/ReadableByteStream::isLocked().
> Probably changing the names lock()/unlock() to better names will resolve the
> problem; use the word "locked" for ReadableByteStream, and another word for
> BodyStreamBuffer. (currently no idea for the new name though...)

As we discussed, I moved |unlock| to the private section and added
drainAsBlobDataHandle.

https://codereview.chromium.org/1233573002/diff/140001/Source/modules/fetch/B...
Source/modules/fetch/BodyStreamBuffer.h:43: PassOwnPtr<FetchDataConsumerHandle>
lock(ExecutionContext*);
On 2015/07/21 12:14:01, hiroshige wrote:
> In my understanding of this CL:
> - lock()/unlock() should be carefully used (because human cannot book-keep
> lock()-unlock() correspondence in general), and should be used inside
> BodyStreamBuffer internally.
> - But lock() is public due to two call sites to lock() outside
BodyStreamBuffer,
> which do not have corresponding unlock() (and we are not sure when to call
> unlock()) and are to be removed (have such TODO comments). These call sites of
> lock() corresponds to previous BodyStreamBuffer::releaseBuffer() calls (which
> were also not so clean interface).
> If this is correct, please add TODO comments to lock() (and possibly to
> unlock()) that lock()/unlock() should be encapsulated in BodyStreamBuffer and
> made private, and external users should not use lock()/unlock().

ditto

https://codereview.chromium.org/1233573002/diff/140001/Source/modules/fetch/B...
Source/modules/fetch/BodyStreamBuffer.h:47: void
unlock(PassOwnPtr<FetchDataConsumerHandle> /* handle */);
On 2015/07/21 12:14:01, hiroshige wrote:
> Please add a comment for what is |handle|.

Removed.

https://codereview.chromium.org/1233573002/diff/140001/Source/modules/fetch/F...
File Source/modules/fetch/FetchManager.cpp (right):

https://codereview.chromium.org/1233573002/diff/140001/Source/modules/fetch/F...
Source/modules/fetch/FetchManager.cpp:332: buffer->unlock(handle.release());
On 2015/07/21 12:14:01, hiroshige wrote:
> Can we move L330-L332 to BodyStreamBuffer::drainAsBlobDataHandle()?
> By doing so, we can remove all unlock() calls from outside BodyStreamBuffer
and
> make unlock() private.

Done.

https://codereview.chromium.org/1233573002/diff/140001/Source/modules/service...
File Source/modules/serviceworkers/RespondWithObserver.cpp (right):

https://codereview.chromium.org/1233573002/diff/140001/Source/modules/service...
Source/modules/serviceworkers/RespondWithObserver.cpp:174:
buffer->unlock(handle.release());
On 2015/07/21 12:14:01, hiroshige wrote:
> Same for the comment in FetchManager.cpp.

Done.

[hiroshige, 2015-07-24 12:47:04]

https://codereview.chromium.org/1233573002/diff/160001/Source/modules/fetch/F...
File Source/modules/fetch/FetchRequestData.cpp (right):

https://codereview.chromium.org/1233573002/diff/160001/Source/modules/fetch/F...
Source/modules/fetch/FetchRequestData.cpp:58: request->m_buffer = new
BodyStreamBuffer;
same as the FetchResponseData::clone()'s comment.

https://codereview.chromium.org/1233573002/diff/160001/Source/modules/fetch/F...
File Source/modules/fetch/FetchResponseData.cpp (right):

https://codereview.chromium.org/1233573002/diff/160001/Source/modules/fetch/F...
Source/modules/fetch/FetchResponseData.cpp:178: newResponse->m_buffer = new
BodyStreamBuffer;
Do we need these two lines? (I'm not sure about this though; IIUC |m_buffer| is
initialized to |new BodyStreamBuffer|, so setting |newResponse->m_buffer| has no
effect? Also this block is else block of m_buffer->hasBody(), so replaceing
|m_buffer|  of no body to new BodyStreamBuffer has also no effect?)

https://codereview.chromium.org/1233573002/diff/160001/Source/modules/fetch/R...
File Source/modules/fetch/Request.cpp (right):

https://codereview.chromium.org/1233573002/diff/160001/Source/modules/fetch/R...
Source/modules/fetch/Request.cpp:53: BodyStreamBuffer* temporaryBodyBuffer =
nullptr;
Now we remove DrainingBodyStreamBuffer, can we merge these two into
BodyStreamBuffer* temporaryBody = null; again?

https://codereview.chromium.org/1233573002/diff/160001/Source/modules/fetch/R...
File Source/modules/fetch/ResponseTest.cpp (right):

https://codereview.chromium.org/1233573002/diff/160001/Source/modules/fetch/R...
Source/modules/fetch/ResponseTest.cpp:150: void* buffer =
response->internalBodyBuffer();
optional: should we add ASSERT_TRUE(response->bodyBuffer()) here and other
locations where e.g. we call bodyBuffer()->hasBody() without asserting
bodyBuffer() is non-null?

https://codereview.chromium.org/1233573002/diff/160001/Source/modules/fetch/R...
Source/modules/fetch/ResponseTest.cpp:162:
EXPECT_TRUE(clonedResponse->internalBodyBuffer());
Should we also add
EXPECT_TRUE(response|clonedResponse->internalBodyBuffer()->hasBody());?
(EXPECT_TRUE(internalBufferForTest()) corresponds to
EXPECT_TRUE(internalBodyBuffer()->hasBody()))

https://codereview.chromium.org/1233573002/diff/160001/Source/modules/service...
File Source/modules/serviceworkers/RespondWithObserver.cpp (right):

https://codereview.chromium.org/1233573002/diff/160001/Source/modules/service...
Source/modules/serviceworkers/RespondWithObserver.cpp:64: namespace {
nit: can we merge two anonymous namespaces (i.e. remove L62--65)?

https://codereview.chromium.org/1233573002/diff/160001/Source/modules/service...
Source/modules/serviceworkers/RespondWithObserver.cpp:66: class LoaderClient
final : public GarbageCollectedFinalized<LoaderClient>, public
FetchDataLoader::Client {
nit optional: how about a name like NoopLoaderClient, DoNothingLoaderClient or
so?

[hiroshige, 2015-07-25 05:27:35]

https://codereview.chromium.org/1233573002/diff/160001/Source/modules/fetch/B...
File Source/modules/fetch/BodyStreamBufferTest.cpp (right):

https://codereview.chromium.org/1233573002/diff/160001/Source/modules/fetch/B...
Source/modules/fetch/BodyStreamBufferTest.cpp:110: handle =
buffer->lock(executionContext());
nit: Reusing |handle| here is somehow confusing.

https://codereview.chromium.org/1233573002/diff/160001/Source/modules/fetch/B...
Source/modules/fetch/BodyStreamBufferTest.cpp:118: TEST_F(BodyStreamBufferTest,
LoadBodyStreamBufferAsArrayBuffer)
optional: do we need LoadBodyStreamBufferAsArrayBuffer/AsBlob tests? I think
these are covered by FetchDataLoader unit tests and we need only one test for
loading data type here.

https://codereview.chromium.org/1233573002/diff/160001/Source/modules/fetch/B...
Source/modules/fetch/BodyStreamBufferTest.cpp:222: EXPECT_TRUE(handle);
nit optional: looks a little nicer if this is moved after L218.

https://codereview.chromium.org/1233573002/diff/160001/Source/modules/fetch/B...
Source/modules/fetch/BodyStreamBufferTest.cpp:274: EXPECT_TRUE(handle);
ditto.

https://codereview.chromium.org/1233573002/diff/160001/Source/modules/fetch/B...
Source/modules/fetch/BodyStreamBufferTest.cpp:304: 
If we call collectGarbase() here, can we test that multiple FetchDataLoaders are
kept alive?
(If so, please apply the same to other tests with startLoading() above)

[yhirano, 2015-07-29 08:43:50]

Patchset #4 (id:180001) has been deleted

[yhirano, 2015-07-29 08:45:03]

https://codereview.chromium.org/1233573002/diff/160001/Source/modules/fetch/B...
File Source/modules/fetch/BodyStreamBufferTest.cpp (right):

https://codereview.chromium.org/1233573002/diff/160001/Source/modules/fetch/B...
Source/modules/fetch/BodyStreamBufferTest.cpp:110: handle =
buffer->lock(executionContext());
On 2015/07/25 05:27:35, hiroshige wrote:
> nit: Reusing |handle| here is somehow confusing.

Done.

https://codereview.chromium.org/1233573002/diff/160001/Source/modules/fetch/B...
Source/modules/fetch/BodyStreamBufferTest.cpp:118: TEST_F(BodyStreamBufferTest,
LoadBodyStreamBufferAsArrayBuffer)
On 2015/07/25 05:27:35, hiroshige wrote:
> optional: do we need LoadBodyStreamBufferAsArrayBuffer/AsBlob tests? I think
> these are covered by FetchDataLoader unit tests and we need only one test for
> loading data type here.

I don't have a strong opinion, but what I wanted to ensure was
BodyStreamBuffer::LoaderHolder works for each type.

https://codereview.chromium.org/1233573002/diff/160001/Source/modules/fetch/B...
Source/modules/fetch/BodyStreamBufferTest.cpp:222: EXPECT_TRUE(handle);
On 2015/07/25 05:27:35, hiroshige wrote:
> nit optional: looks a little nicer if this is moved after L218.

Done.

https://codereview.chromium.org/1233573002/diff/160001/Source/modules/fetch/B...
Source/modules/fetch/BodyStreamBufferTest.cpp:274: EXPECT_TRUE(handle);
On 2015/07/25 05:27:35, hiroshige wrote:
> ditto.

Done.

https://codereview.chromium.org/1233573002/diff/160001/Source/modules/fetch/B...
Source/modules/fetch/BodyStreamBufferTest.cpp:304: 
On 2015/07/25 05:27:35, hiroshige wrote:
> If we call collectGarbase() here, can we test that multiple FetchDataLoaders
are
> kept alive?
> (If so, please apply the same to other tests with startLoading() above)

Yes, but I'm not sure if it should be tested. Generally we don't test x is held
by y.
Anyway, I think even if it should be tested, it should be tested separately. I
added LoaderShouldBeKeptAliveByBodyStreamBuffer.

https://codereview.chromium.org/1233573002/diff/160001/Source/modules/fetch/F...
File Source/modules/fetch/FetchRequestData.cpp (right):

https://codereview.chromium.org/1233573002/diff/160001/Source/modules/fetch/F...
Source/modules/fetch/FetchRequestData.cpp:58: request->m_buffer = new
BodyStreamBuffer;
On 2015/07/24 12:47:03, hiroshige wrote:
> same as the FetchResponseData::clone()'s comment.

Done.

https://codereview.chromium.org/1233573002/diff/160001/Source/modules/fetch/F...
File Source/modules/fetch/FetchResponseData.cpp (right):

https://codereview.chromium.org/1233573002/diff/160001/Source/modules/fetch/F...
Source/modules/fetch/FetchResponseData.cpp:178: newResponse->m_buffer = new
BodyStreamBuffer;
On 2015/07/24 12:47:03, hiroshige wrote:
> Do we need these two lines? (I'm not sure about this though; IIUC |m_buffer|
is
> initialized to |new BodyStreamBuffer|, so setting |newResponse->m_buffer| has
no
> effect? Also this block is else block of m_buffer->hasBody(), so replaceing
> |m_buffer|  of no body to new BodyStreamBuffer has also no effect?)

Done.

https://codereview.chromium.org/1233573002/diff/160001/Source/modules/fetch/R...
File Source/modules/fetch/Request.cpp (right):

https://codereview.chromium.org/1233573002/diff/160001/Source/modules/fetch/R...
Source/modules/fetch/Request.cpp:53: BodyStreamBuffer* temporaryBodyBuffer =
nullptr;
On 2015/07/24 12:47:03, hiroshige wrote:
> Now we remove DrainingBodyStreamBuffer, can we merge these two into
> BodyStreamBuffer* temporaryBody = null; again?

Done.

https://codereview.chromium.org/1233573002/diff/160001/Source/modules/fetch/R...
File Source/modules/fetch/ResponseTest.cpp (right):

https://codereview.chromium.org/1233573002/diff/160001/Source/modules/fetch/R...
Source/modules/fetch/ResponseTest.cpp:150: void* buffer =
response->internalBodyBuffer();
On 2015/07/24 12:47:03, hiroshige wrote:
> optional: should we add ASSERT_TRUE(response->bodyBuffer()) here and other
> locations where e.g. we call bodyBuffer()->hasBody() without asserting
> bodyBuffer() is non-null?

I think we cannot place ASSERT_* here.

https://codereview.chromium.org/1233573002/diff/160001/Source/modules/fetch/R...
Source/modules/fetch/ResponseTest.cpp:162:
EXPECT_TRUE(clonedResponse->internalBodyBuffer());
On 2015/07/24 12:47:03, hiroshige wrote:
> Should we also add
> EXPECT_TRUE(response|clonedResponse->internalBodyBuffer()->hasBody());?
> (EXPECT_TRUE(internalBufferForTest()) corresponds to
> EXPECT_TRUE(internalBodyBuffer()->hasBody()))

Done.

https://codereview.chromium.org/1233573002/diff/160001/Source/modules/service...
File Source/modules/serviceworkers/RespondWithObserver.cpp (right):

https://codereview.chromium.org/1233573002/diff/160001/Source/modules/service...
Source/modules/serviceworkers/RespondWithObserver.cpp:64: namespace {
On 2015/07/24 12:47:03, hiroshige wrote:
> nit: can we merge two anonymous namespaces (i.e. remove L62--65)?

Done.

https://codereview.chromium.org/1233573002/diff/160001/Source/modules/service...
Source/modules/serviceworkers/RespondWithObserver.cpp:66: class LoaderClient
final : public GarbageCollectedFinalized<LoaderClient>, public
FetchDataLoader::Client {
On 2015/07/24 12:47:03, hiroshige wrote:
> nit optional: how about a name like NoopLoaderClient, DoNothingLoaderClient or
> so?

Done.

[hiroshige, 2015-07-29 09:23:09]

lgtm. Thanks for refactoring!

https://codereview.chromium.org/1233573002/diff/200001/Source/modules/fetch/F...
File Source/modules/fetch/FetchRequestData.cpp (right):

https://codereview.chromium.org/1233573002/diff/200001/Source/modules/fetch/F...
Source/modules/fetch/FetchRequestData.cpp:87: return request;
optional: pass() and clone() have duplicated code that copies fields other than
|m_buffer|. Merging those two code instances to one would be better for
maintainance.

[yhirano, 2015-07-29 10:52:05]

https://codereview.chromium.org/1233573002/diff/200001/Source/modules/fetch/F...
File Source/modules/fetch/FetchRequestData.cpp (right):

https://codereview.chromium.org/1233573002/diff/200001/Source/modules/fetch/F...
Source/modules/fetch/FetchRequestData.cpp:87: return request;
On 2015/07/29 09:23:09, hiroshige wrote:
> optional: pass() and clone() have duplicated code that copies fields other
than
> |m_buffer|. Merging those two code instances to one would be better for
> maintainance.

Done.

[yhirano, 2015-07-29 10:53:57]

yhirano@chromium.org changed reviewers:
+ horo@chromium.org, mkwst@chromium.org, tyoshino@chromium.org

[yhirano, 2015-07-29 10:53:58]

+horo@ for modules/serviceworkers, modules/cachestorage.
+tyoshino@ for core/streams.
+mkwst@ for public/platform.

Thanks,

[horo, 2015-07-30 09:53:27]

> +horo@ for modules/serviceworkers, modules/cachestorage.
lgtm

[Mike West, 2015-07-31 07:17:21]

public/ LGTM.

[tyoshino (SeeGerritForStatus), 2015-07-31 07:26:40]

lgtm

[yhirano, 2015-07-31 07:29:14]

Thanks!

[yhirano, 2015-07-31 07:29:18]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2015-07-31 07:29:18]

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/1233573002/#ps220001
(title: " ")

[commit-bot: I haz the power, 2015-07-31 07:29:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1233573002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1233573002/220001

[commit-bot: I haz the power, 2015-07-31 08:54:12]

Committed patchset #5 (id:220001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199808