[ServiceWorker] Use BodyStreamBuffer as the body data of the Response object.

[ServiceWorker] Use BodyStreamBuffer as the body data of the Response object.

Currently we use BlobDataHandle as the body data of Request and Response object.
BlobDataHandle can handle only static data. It can't handle progressive data.
We will use BodyStreamBuffer for Response objects which will be created in FetchManager to support progressive loading.

In current implementation when Response.clone() is called, FetchResponseData object will be shared between the responses.
But this cl changes this behavior to "tee" the stream.
Response.clone() will create two FetchResponseData with new BodyStreamBuffers, and StreamTeePump will pump up the data from original BodyStreamBuffer to the two new BodyStreamBuffers.

This change depends on https://codereview.chromium.org/787793002/.

BUG=436424
TEST=webkit_unit_tests --gtest_filter=ServiceWorkerResponseTest.*

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

[horo, 2014-12-09 14:55:02]

Patchset #1 (id:1) has been deleted

[horo, 2014-12-09 14:55:08]

Patchset #1 (id:20001) has been deleted

[horo, 2014-12-09 14:55:13]

Patchset #1 (id:40001) has been deleted

[horo, 2014-12-09 15:07:42]

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

[horo, 2014-12-09 15:07:42]

yhirano@
Could you please review this?

[horo, 2014-12-09 15:09:16]

Patchset #1 (id:60001) has been deleted

[yhirano, 2014-12-11 03:55:47]

https://codereview.chromium.org/786893004/diff/80001/Source/modules/servicewo...
File Source/modules/serviceworkers/Body.h (right):

https://codereview.chromium.org/786893004/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/Body.h:84: virtual PassRefPtr<BlobDataHandle>
blobDataHandle(bool /* internal */) const = 0;
What does internal mean?

https://codereview.chromium.org/786893004/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/Body.h:85: virtual BodyStreamBuffer* buffer(bool
/* internal */) const = 0;
ditto

[horo, 2014-12-11 05:43:08]

https://codereview.chromium.org/786893004/diff/80001/Source/modules/servicewo...
File Source/modules/serviceworkers/Body.h (right):

https://codereview.chromium.org/786893004/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/Body.h:84: virtual PassRefPtr<BlobDataHandle>
blobDataHandle(bool /* internal */) const = 0;
On 2014/12/11 03:55:47, yhirano wrote:
> What does internal mean?

I wanted to use this flag to access the internal response of the Response
object.
But it is not good solution to add the internal argument to blobDataHandle(),
and buffer().

Instead I introduced internalBlobDataHandle(), internalBuffer() and
internalContentType() to Response and FetchResponseData.

[yhirano, 2014-12-12 04:31:16]

Do you plan to maintain both two interfaces (Blob and BodyStreamBuffer) in Body?
I think it is not sustainable.

https://codereview.chromium.org/786893004/diff/100001/Source/modules/servicew...
File Source/modules/serviceworkers/Body.cpp (right):

https://codereview.chromium.org/786893004/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/Body.cpp:22: class Body::BlobHandleResolver final
: public BodyStreamBuffer::BlobHandleCreatorClient {
I don't understand why this class is named as "Resolver". Can you explain?

https://codereview.chromium.org/786893004/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/Body.cpp:96: FileReaderLoader::ReadType readType =
FileReaderLoader::ReadAsArrayBuffer;
You can call readAsyncFromBlob here.

https://codereview.chromium.org/786893004/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/Body.cpp:275: if (m_resolver && m_loader)
Just |if (m_loader)| is fine, I think.

[horo, 2014-12-12 05:30:47]

> Do you plan to maintain both two interfaces (Blob and BodyStreamBuffer) in
Body?
> I think it is not sustainable.

Yes. We should do so.
When we get the response from the Cache and send back it to the page, we don't
need to read the data in the ServiceWorker.
In such case we should handle the body as a blob for the performance.
I think this use case will be commonly used.

self.onfetch = function(event) {
  event.respondWith(
    caches.match(event.request).then(function(response) {
      return response || fetch(event.request);
    })
  );
};

https://codereview.chromium.org/786893004/diff/100001/Source/modules/servicew...
File Source/modules/serviceworkers/Body.cpp (right):

https://codereview.chromium.org/786893004/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/Body.cpp:22: class Body::BlobHandleResolver final
: public BodyStreamBuffer::BlobHandleCreatorClient {
On 2014/12/12 04:31:16, yhirano wrote:
> I don't understand why this class is named as "Resolver". Can you explain?

I think BlobHandle"Receiver" is a better name.
Changed the name.

https://codereview.chromium.org/786893004/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/Body.cpp:96: FileReaderLoader::ReadType readType =
FileReaderLoader::ReadAsArrayBuffer;
On 2014/12/12 04:31:16, yhirano wrote:
> You can call readAsyncFromBlob here.

Done.

https://codereview.chromium.org/786893004/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/Body.cpp:275: if (m_resolver && m_loader)
On 2014/12/12 04:31:16, yhirano wrote:
> Just |if (m_loader)| is fine, I think.

Done.

[yhirano, 2014-12-12 12:42:33]

I still think interacting with both two interfaces is not good. We should seek a
cleaner way; You don't need to fix it now, but can you add FIXME?

https://codereview.chromium.org/786893004/diff/120001/Source/modules/servicew...
File Source/modules/serviceworkers/FetchRequestData.cpp (right):

https://codereview.chromium.org/786893004/diff/120001/Source/modules/servicew...
Source/modules/serviceworkers/FetchRequestData.cpp:71: String
FetchRequestData::contentType() const
Is this function's meaning clear?
This function is called only in Body, when m_blobDataHandle is null. It means
that this function always return an empty string and that seems wrong.

https://codereview.chromium.org/786893004/diff/120001/Source/modules/servicew...
Source/modules/serviceworkers/FetchRequestData.cpp:75: return "";
return String() is more efficient.

https://codereview.chromium.org/786893004/diff/120001/Source/modules/servicew...
File Source/modules/serviceworkers/FetchResponseData.cpp (right):

https://codereview.chromium.org/786893004/diff/120001/Source/modules/servicew...
Source/modules/serviceworkers/FetchResponseData.cpp:235: 
ASSERT(!m_buffer || m_buffer == m_internalResponse->m_buffer)

https://codereview.chromium.org/786893004/diff/120001/Source/modules/servicew...
File Source/modules/serviceworkers/ResponseTest.cpp (right):

https://codereview.chromium.org/786893004/diff/120001/Source/modules/servicew...
Source/modules/serviceworkers/ResponseTest.cpp:44: class BlobHandleCallback
final : public BodyStreamBuffer::BlobHandleCreatorClient {
How about BlobHandleCreatorClient?

https://codereview.chromium.org/786893004/diff/120001/Source/modules/servicew...
Source/modules/serviceworkers/ResponseTest.cpp:261: void
checkResponseStream(Response* response, BodyStreamBuffer* buffer, bool
hasNonInternalStream)
checkResponseBodyStreamBuffer

https://codereview.chromium.org/786893004/diff/120001/Source/modules/servicew...
Source/modules/serviceworkers/ResponseTest.cpp:261: void
checkResponseStream(Response* response, BodyStreamBuffer* buffer, bool
hasNonInternalStream)
hasNonInternalBodyStreamBuffer

https://codereview.chromium.org/786893004/diff/120001/Source/modules/servicew...
Source/modules/serviceworkers/ResponseTest.cpp:261: void
checkResponseStream(Response* response, BodyStreamBuffer* buffer, bool
hasNonInternalStream)
Do you need to pass buffer? Isn't just using response->internalBuffer() enough?

https://codereview.chromium.org/786893004/diff/120001/Source/modules/servicew...
Source/modules/serviceworkers/ResponseTest.cpp:307:
TEST_F(ServiceWorkerResponseTest, StreamCloneDefault)
s/Stream/BodyStreamBuffer/
Same for the below tests.

[horo, 2014-12-15 02:12:24]

https://codereview.chromium.org/786893004/diff/120001/Source/modules/servicew...
File Source/modules/serviceworkers/FetchRequestData.cpp (right):

https://codereview.chromium.org/786893004/diff/120001/Source/modules/servicew...
Source/modules/serviceworkers/FetchRequestData.cpp:71: String
FetchRequestData::contentType() const
On 2014/12/12 12:42:33, yhirano wrote:
> Is this function's meaning clear?
> This function is called only in Body, when m_blobDataHandle is null. It means
> that this function always return an empty string and that seems wrong.

Removed and I introduced contentTypeForBuffer to Body.

https://codereview.chromium.org/786893004/diff/120001/Source/modules/servicew...
Source/modules/serviceworkers/FetchRequestData.cpp:75: return "";
On 2014/12/12 12:42:32, yhirano wrote:
> return String() is more efficient.

Done in Request::contentTypeForBuffer().

https://codereview.chromium.org/786893004/diff/120001/Source/modules/servicew...
File Source/modules/serviceworkers/FetchResponseData.cpp (right):

https://codereview.chromium.org/786893004/diff/120001/Source/modules/servicew...
Source/modules/serviceworkers/FetchResponseData.cpp:235: 
On 2014/12/12 12:42:33, yhirano wrote:
> ASSERT(!m_buffer || m_buffer == m_internalResponse->m_buffer)

Done.

https://codereview.chromium.org/786893004/diff/120001/Source/modules/servicew...
File Source/modules/serviceworkers/ResponseTest.cpp (right):

https://codereview.chromium.org/786893004/diff/120001/Source/modules/servicew...
Source/modules/serviceworkers/ResponseTest.cpp:44: class BlobHandleCallback
final : public BodyStreamBuffer::BlobHandleCreatorClient {
On 2014/12/12 12:42:33, yhirano wrote:
> How about BlobHandleCreatorClient?

Done.

https://codereview.chromium.org/786893004/diff/120001/Source/modules/servicew...
Source/modules/serviceworkers/ResponseTest.cpp:261: void
checkResponseStream(Response* response, BodyStreamBuffer* buffer, bool
hasNonInternalStream)
On 2014/12/12 12:42:33, yhirano wrote:
> Do you need to pass buffer? Isn't just using response->internalBuffer()
enough?

We need |buffer| to write the data (l.296) on it and close it (l.300).

https://codereview.chromium.org/786893004/diff/120001/Source/modules/servicew...
Source/modules/serviceworkers/ResponseTest.cpp:261: void
checkResponseStream(Response* response, BodyStreamBuffer* buffer, bool
hasNonInternalStream)
On 2014/12/12 12:42:33, yhirano wrote:
> checkResponseBodyStreamBuffer

Done.

https://codereview.chromium.org/786893004/diff/120001/Source/modules/servicew...
Source/modules/serviceworkers/ResponseTest.cpp:307:
TEST_F(ServiceWorkerResponseTest, StreamCloneDefault)
On 2014/12/12 12:42:33, yhirano wrote:
> s/Stream/BodyStreamBuffer/
> Same for the below tests.

Done.

[horo, 2014-12-15 02:27:21]

On 2014/12/12 12:42:33, yhirano wrote:
> I still think interacting with both two interfaces is not good. We should seek
a
> cleaner way; You don't need to fix it now, but can you add FIXME?

Done in Body.h.

[yhirano, 2014-12-15 04:03:06]

https://codereview.chromium.org/786893004/diff/120001/Source/modules/servicew...
File Source/modules/serviceworkers/ResponseTest.cpp (right):

https://codereview.chromium.org/786893004/diff/120001/Source/modules/servicew...
Source/modules/serviceworkers/ResponseTest.cpp:261: void
checkResponseStream(Response* response, BodyStreamBuffer* buffer, bool
hasNonInternalStream)
On 2014/12/15 02:12:24, horo wrote:
> On 2014/12/12 12:42:33, yhirano wrote:
> > Do you need to pass buffer? Isn't just using response->internalBuffer()
> enough?
> 
> We need |buffer| to write the data (l.296) on it and close it (l.300).

Yes, we need the buffer object, but it always equals to
|response->internalBuffer()|, as expected in L265. My suggestion is to stop
passing |buffer| and to use |response->internalBuffer()| instead.

https://codereview.chromium.org/786893004/diff/160001/Source/modules/servicew...
File Source/modules/serviceworkers/ResponseTest.cpp (right):

https://codereview.chromium.org/786893004/diff/160001/Source/modules/servicew...
Source/modules/serviceworkers/ResponseTest.cpp:292: BlobHandleCreatorClient*
callback1 = new BlobHandleCreatorClient();
client1

https://codereview.chromium.org/786893004/diff/160001/Source/modules/servicew...
Source/modules/serviceworkers/ResponseTest.cpp:293: BlobHandleCreatorClient*
callback2 = new BlobHandleCreatorClient();
client2

https://codereview.chromium.org/786893004/diff/160001/Source/modules/servicew...
Source/modules/serviceworkers/ResponseTest.cpp:351: BlobHandleCreatorClient*
callback1 = new BlobHandleCreatorClient();
client1

https://codereview.chromium.org/786893004/diff/160001/Source/modules/servicew...
Source/modules/serviceworkers/ResponseTest.cpp:352: BlobHandleCreatorClient*
callback2 = new BlobHandleCreatorClient();
client2

[horo, 2014-12-15 04:26:08]

https://codereview.chromium.org/786893004/diff/120001/Source/modules/servicew...
File Source/modules/serviceworkers/ResponseTest.cpp (right):

https://codereview.chromium.org/786893004/diff/120001/Source/modules/servicew...
Source/modules/serviceworkers/ResponseTest.cpp:261: void
checkResponseStream(Response* response, BodyStreamBuffer* buffer, bool
hasNonInternalStream)
On 2014/12/15 04:03:06, yhirano wrote:
> On 2014/12/15 02:12:24, horo wrote:
> > On 2014/12/12 12:42:33, yhirano wrote:
> > > Do you need to pass buffer? Isn't just using response->internalBuffer()
> > enough?
> > 
> > We need |buffer| to write the data (l.296) on it and close it (l.300).
> 
> Yes, we need the buffer object, but it always equals to
> |response->internalBuffer()|, as expected in L265. My suggestion is to stop
> passing |buffer| and to use |response->internalBuffer()| instead.

I see.
Done.

https://codereview.chromium.org/786893004/diff/160001/Source/modules/servicew...
File Source/modules/serviceworkers/ResponseTest.cpp (right):

https://codereview.chromium.org/786893004/diff/160001/Source/modules/servicew...
Source/modules/serviceworkers/ResponseTest.cpp:292: BlobHandleCreatorClient*
callback1 = new BlobHandleCreatorClient();
On 2014/12/15 04:03:06, yhirano wrote:
> client1

Done.

https://codereview.chromium.org/786893004/diff/160001/Source/modules/servicew...
Source/modules/serviceworkers/ResponseTest.cpp:293: BlobHandleCreatorClient*
callback2 = new BlobHandleCreatorClient();
On 2014/12/15 04:03:06, yhirano wrote:
> client2

Done.

https://codereview.chromium.org/786893004/diff/160001/Source/modules/servicew...
Source/modules/serviceworkers/ResponseTest.cpp:351: BlobHandleCreatorClient*
callback1 = new BlobHandleCreatorClient();
On 2014/12/15 04:03:06, yhirano wrote:
> client1

Done.

https://codereview.chromium.org/786893004/diff/160001/Source/modules/servicew...
Source/modules/serviceworkers/ResponseTest.cpp:352: BlobHandleCreatorClient*
callback2 = new BlobHandleCreatorClient();
On 2014/12/15 04:03:06, yhirano wrote:
> client2

Done.

[yhirano, 2014-12-15 04:28:17]

lgtm

[horo, 2014-12-15 04:44:00]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-12-15 04:44:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/786893004/180001

[commit-bot: I haz the power, 2014-12-15 05:52:34]

Committed patchset #6 (id:180001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=187114