Add FetchDataConsumerHandle and utility functions/classes

Add FetchDataConsumerHandle and utility functions/classes

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

[hiroshige, 2015-06-16 14:17:23]

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

[hiroshige, 2015-06-16 18:59:32]

Patchset #6 (id:100001) has been deleted

[hiroshige, 2015-06-16 18:59:42]

Patchset #6 (id:120001) has been deleted

[hiroshige, 2015-06-16 19:00:10]

Patchset #6 (id:140001) has been deleted

[hiroshige, 2015-06-16 19:28:54]

PTAL.

[yhirano, 2015-06-17 04:13:35]

https://codereview.chromium.org/1176243004/diff/180001/Source/modules/fetch/C...
File Source/modules/fetch/CompositeDataConsumerHandle.cpp (right):

https://codereview.chromium.org/1176243004/diff/180001/Source/modules/fetch/C...
Source/modules/fetch/CompositeDataConsumerHandle.cpp:13: #include
"wtf/Functional.h"
You don't need this since bind is gone.

https://codereview.chromium.org/1176243004/diff/180001/Source/modules/fetch/D...
File Source/modules/fetch/DataConsumerHandleUtil.cpp (right):

https://codereview.chromium.org/1176243004/diff/180001/Source/modules/fetch/D...
Source/modules/fetch/DataConsumerHandleUtil.cpp:44: class DoneHandle final :
public WebDataConsumerHandle {
[opt] You can unify DoneHandle and UnexpectedErrorHandle.

https://codereview.chromium.org/1176243004/diff/180001/Source/modules/fetch/D...
File Source/modules/fetch/DataConsumerHandleUtil.h (right):

https://codereview.chromium.org/1176243004/diff/180001/Source/modules/fetch/D...
Source/modules/fetch/DataConsumerHandleUtil.h:17:
PassOwnPtr<WebDataConsumerHandle> createWaitingHandle();
I'm afraid blink::createWaitingHandle and others are too generic. Can you put
them in a class, or name them longer?

https://codereview.chromium.org/1176243004/diff/180001/Source/modules/fetch/D...
Source/modules/fetch/DataConsumerHandleUtil.h:32: class
NotifyOnReaderCreationHelper {
+final

https://codereview.chromium.org/1176243004/diff/180001/Source/modules/fetch/D...
Source/modules/fetch/DataConsumerHandleUtil.h:32: class
NotifyOnReaderCreationHelper {
Do you want to expose this class? Is it a problem to move it into the
implementation?

https://codereview.chromium.org/1176243004/diff/180001/Source/modules/fetch/D...
File Source/modules/fetch/DataConsumerHandleUtilTest.h (right):

https://codereview.chromium.org/1176243004/diff/180001/Source/modules/fetch/D...
Source/modules/fetch/DataConsumerHandleUtilTest.h:3: // found in the LICENSE
file.
[optional] How about naming this class as *TestUtil.h?

https://codereview.chromium.org/1176243004/diff/180001/Source/modules/fetch/D...
Source/modules/fetch/DataConsumerHandleUtilTest.h:22: using Result =
WebDataConsumerHandle::Result;
I don't like placing many (blink:: global) using directives in a header file.
Can you move them to other places such as individual classes or cpp files?

https://codereview.chromium.org/1176243004/diff/180001/Source/modules/fetch/D...
Source/modules/fetch/DataConsumerHandleUtilTest.h:29: const Result kShouldWait =
WebDataConsumerHandle::ShouldWait;
ditto

https://codereview.chromium.org/1176243004/diff/180001/Source/modules/fetch/D...
Source/modules/fetch/DataConsumerHandleUtilTest.h:35: class NoopClient final :
public WebDataConsumerHandle::Client {
I generally like short names in unnamed namespaces, but we are in blink
namespace now. NoopClient is too generic and may conflict with other class
names. Ditto for classes below.

https://codereview.chromium.org/1176243004/diff/180001/Source/modules/fetch/F...
File Source/modules/fetch/FetchDataConsumerHandle.h (right):

https://codereview.chromium.org/1176243004/diff/180001/Source/modules/fetch/F...
Source/modules/fetch/FetchDataConsumerHandle.h:32: //
throughWebDataConsumerHandle APIs without calling this function.
space after "through"

https://codereview.chromium.org/1176243004/diff/180001/Source/modules/fetch/F...
Source/modules/fetch/FetchDataConsumerHandle.h:33: virtual
PassRefPtr<BlobDataHandle> blobDataHandle() = 0;
As this function is not a getter, a name that doesn't sound like a getter is
preferable, I think?

https://codereview.chromium.org/1176243004/diff/180001/Source/modules/fetch/F...
Source/modules/fetch/FetchDataConsumerHandle.h:35: PassOwnPtr<Reader>
obtainReader(Client* client) { return adoptPtr(obtainReaderInternal(client)); }
Please say that obtainReader is non-virtual overriding, which is OK in this
case.
(I will make it virtual when we can use scoped_ptr / unique_ptr in both blink
and chromium).

[hiroshige, 2015-06-17 08:30:24]

https://codereview.chromium.org/1176243004/diff/180001/Source/modules/fetch/C...
File Source/modules/fetch/CompositeDataConsumerHandle.cpp (right):

https://codereview.chromium.org/1176243004/diff/180001/Source/modules/fetch/C...
Source/modules/fetch/CompositeDataConsumerHandle.cpp:13: #include
"wtf/Functional.h"
On 2015/06/17 04:13:34, yhirano wrote:
> You don't need this since bind is gone.

Done.

https://codereview.chromium.org/1176243004/diff/180001/Source/modules/fetch/D...
File Source/modules/fetch/DataConsumerHandleUtil.cpp (right):

https://codereview.chromium.org/1176243004/diff/180001/Source/modules/fetch/D...
Source/modules/fetch/DataConsumerHandleUtil.cpp:44: class DoneHandle final :
public WebDataConsumerHandle {
On 2015/06/17 04:13:34, yhirano wrote:
> [opt] You can unify DoneHandle and UnexpectedErrorHandle.

Done.

https://codereview.chromium.org/1176243004/diff/180001/Source/modules/fetch/D...
File Source/modules/fetch/DataConsumerHandleUtil.h (right):

https://codereview.chromium.org/1176243004/diff/180001/Source/modules/fetch/D...
Source/modules/fetch/DataConsumerHandleUtil.h:17:
PassOwnPtr<WebDataConsumerHandle> createWaitingHandle();
On 2015/06/17 04:13:34, yhirano wrote:
> I'm afraid blink::createWaitingHandle and others are too generic. Can you put
> them in a class, or name them longer?
Made names longer.

https://codereview.chromium.org/1176243004/diff/180001/Source/modules/fetch/D...
Source/modules/fetch/DataConsumerHandleUtil.h:32: class
NotifyOnReaderCreationHelper {
On 2015/06/17 04:13:34, yhirano wrote:
> Do you want to expose this class? Is it a problem to move it into the
> implementation?

I'd like to use this in FetchBlobConsumerHandle and thus expose this.

https://codereview.chromium.org/1176243004/diff/180001/Source/modules/fetch/D...
Source/modules/fetch/DataConsumerHandleUtil.h:32: class
NotifyOnReaderCreationHelper {
On 2015/06/17 04:13:34, yhirano wrote:
> +final

Done.

https://codereview.chromium.org/1176243004/diff/180001/Source/modules/fetch/D...
File Source/modules/fetch/DataConsumerHandleUtilTest.h (right):

https://codereview.chromium.org/1176243004/diff/180001/Source/modules/fetch/D...
Source/modules/fetch/DataConsumerHandleUtilTest.h:3: // found in the LICENSE
file.
On 2015/06/17 04:13:34, yhirano wrote:
> [optional] How about naming this class as *TestUtil.h?

Done.

https://codereview.chromium.org/1176243004/diff/180001/Source/modules/fetch/D...
Source/modules/fetch/DataConsumerHandleUtilTest.h:22: using Result =
WebDataConsumerHandle::Result;
On 2015/06/17 04:13:34, yhirano wrote:
> I don't like placing many (blink:: global) using directives in a header file.
> Can you move them to other places such as individual classes or cpp files?

Done.

https://codereview.chromium.org/1176243004/diff/180001/Source/modules/fetch/D...
Source/modules/fetch/DataConsumerHandleUtilTest.h:29: const Result kShouldWait =
WebDataConsumerHandle::ShouldWait;
On 2015/06/17 04:13:34, yhirano wrote:
> ditto

Done.

https://codereview.chromium.org/1176243004/diff/180001/Source/modules/fetch/D...
Source/modules/fetch/DataConsumerHandleUtilTest.h:35: class NoopClient final :
public WebDataConsumerHandle::Client {
On 2015/06/17 04:13:34, yhirano wrote:
> I generally like short names in unnamed namespaces, but we are in blink
> namespace now. NoopClient is too generic and may conflict with other class
> names. Ditto for classes below.

Moved back some classes to CompositeDataConsumerHandleTest.cpp, and wrapped
others inside DataConsumerHandleTestUtil class.

https://codereview.chromium.org/1176243004/diff/180001/Source/modules/fetch/F...
File Source/modules/fetch/FetchDataConsumerHandle.h (right):

https://codereview.chromium.org/1176243004/diff/180001/Source/modules/fetch/F...
Source/modules/fetch/FetchDataConsumerHandle.h:32: //
throughWebDataConsumerHandle APIs without calling this function.
On 2015/06/17 04:13:34, yhirano wrote:
> space after "through"

Done.

https://codereview.chromium.org/1176243004/diff/180001/Source/modules/fetch/F...
Source/modules/fetch/FetchDataConsumerHandle.h:33: virtual
PassRefPtr<BlobDataHandle> blobDataHandle() = 0;
On 2015/06/17 04:13:35, yhirano wrote:
> As this function is not a getter, a name that doesn't sound like a getter is
> preferable, I think?

Hmm. Do you have suggestions for the name?
(|obtainBlobDataHandle|?)

https://codereview.chromium.org/1176243004/diff/180001/Source/modules/fetch/F...
Source/modules/fetch/FetchDataConsumerHandle.h:35: PassOwnPtr<Reader>
obtainReader(Client* client) { return adoptPtr(obtainReaderInternal(client)); }
On 2015/06/17 04:13:35, yhirano wrote:
> Please say that obtainReader is non-virtual overriding, which is OK in this
> case.
> (I will make it virtual when we can use scoped_ptr / unique_ptr in both blink
> and chromium).

Done.

[yhirano, 2015-06-17 09:10:16]

https://codereview.chromium.org/1176243004/diff/180001/Source/modules/fetch/F...
File Source/modules/fetch/FetchDataConsumerHandle.h (right):

https://codereview.chromium.org/1176243004/diff/180001/Source/modules/fetch/F...
Source/modules/fetch/FetchDataConsumerHandle.h:33: virtual
PassRefPtr<BlobDataHandle> blobDataHandle() = 0;
On 2015/06/17 08:30:23, hiroshige wrote:
> On 2015/06/17 04:13:35, yhirano wrote:
> > As this function is not a getter, a name that doesn't sound like a getter is
> > preferable, I think?
> 
> Hmm. Do you have suggestions for the name?
> (|obtainBlobDataHandle|?)

take / obtain / acquire?

https://codereview.chromium.org/1176243004/diff/200001/Source/modules/fetch/D...
File Source/modules/fetch/DataConsumerHandleTestUtil.h (right):

https://codereview.chromium.org/1176243004/diff/200001/Source/modules/fetch/D...
Source/modules/fetch/DataConsumerHandleTestUtil.h:4: 
+ include guard

https://codereview.chromium.org/1176243004/diff/200001/Source/modules/fetch/D...
File Source/modules/fetch/DataConsumerHandleUtil.cpp (right):

https://codereview.chromium.org/1176243004/diff/200001/Source/modules/fetch/D...
Source/modules/fetch/DataConsumerHandleUtil.cpp:145: {
Please say that |client| dereference is safe here because this object is owned
by a reader (is it better to say this restriction in the header?) and thus
|client| must be valid as long as this object is alive.

[hiroshige, 2015-06-17 09:25:27]

https://codereview.chromium.org/1176243004/diff/180001/Source/modules/fetch/F...
File Source/modules/fetch/FetchDataConsumerHandle.h (right):

https://codereview.chromium.org/1176243004/diff/180001/Source/modules/fetch/F...
Source/modules/fetch/FetchDataConsumerHandle.h:33: virtual
PassRefPtr<BlobDataHandle> blobDataHandle() = 0;
On 2015/06/17 09:10:16, yhirano wrote:
> On 2015/06/17 08:30:23, hiroshige wrote:
> > On 2015/06/17 04:13:35, yhirano wrote:
> > > As this function is not a getter, a name that doesn't sound like a getter
is
> > > preferable, I think?
> > 
> > Hmm. Do you have suggestions for the name?
> > (|obtainBlobDataHandle|?)
> 
> take / obtain / acquire?

Done in Patch Set 9.

https://codereview.chromium.org/1176243004/diff/200001/Source/modules/fetch/D...
File Source/modules/fetch/DataConsumerHandleTestUtil.h (right):

https://codereview.chromium.org/1176243004/diff/200001/Source/modules/fetch/D...
Source/modules/fetch/DataConsumerHandleTestUtil.h:4: 
On 2015/06/17 09:10:16, yhirano wrote:
> + include guard

Done.

https://codereview.chromium.org/1176243004/diff/200001/Source/modules/fetch/D...
File Source/modules/fetch/DataConsumerHandleUtil.cpp (right):

https://codereview.chromium.org/1176243004/diff/200001/Source/modules/fetch/D...
Source/modules/fetch/DataConsumerHandleUtil.cpp:145: {
On 2015/06/17 09:10:16, yhirano wrote:
> Please say that |client| dereference is safe here because this object is owned
> by a reader (is it better to say this restriction in the header?) and thus
> |client| must be valid as long as this object is alive.

Done.

[yhirano, 2015-06-17 09:36:35]

Looks good, but I have a question about FetchDataConsumerHandle.h interface at
[1].
One possible fix is to state that obtainBlobDataConsumerHandle will not return a
valid handle with size unset.

1:
https://codereview.chromium.org/1179393007/diff/80001/Source/modules/fetch/Fe...

[hiroshige, 2015-06-17 10:55:31]

On 2015/06/17 09:36:35, yhirano wrote:
> Looks good, but I have a question about FetchDataConsumerHandle.h interface at
> [1].
> One possible fix is to state that obtainBlobDataConsumerHandle will not return
a
> valid handle with size unset.
> 
> 1:
>
https://codereview.chromium.org/1179393007/diff/80001/Source/modules/fetch/Fe...

Oh, I misunderstood the semantics of obtainBlobDataHandle();
I thought the data is still available via reader after obtainBlobDataHandle(),
but the reader should return Done after obtainBlobDataHandle(), right?

If so, name such as drainAsBlobDataHandle() might be better.
Also, maybe drainAsBlobDataHandle() should be a member of Handle (not reader)
and require that the handle is not locked when called, to avoid situation like
drainAsBlobDataHandle() is called during two-phase.

[yhirano, 2015-06-17 11:11:47]

On 2015/06/17 10:55:31, hiroshige wrote:
> On 2015/06/17 09:36:35, yhirano wrote:
> > Looks good, but I have a question about FetchDataConsumerHandle.h interface
at
> > [1].
> > One possible fix is to state that obtainBlobDataConsumerHandle will not
return
> a
> > valid handle with size unset.
> > 
> > 1:
> >
>
https://codereview.chromium.org/1179393007/diff/80001/Source/modules/fetch/Fe...
> 
> Oh, I misunderstood the semantics of obtainBlobDataHandle();
> I thought the data is still available via reader after obtainBlobDataHandle(),
> but the reader should return Done after obtainBlobDataHandle(), right?
I thought so.

> If so, name such as drainAsBlobDataHandle() might be better.
Agreed.

> Also, maybe drainAsBlobDataHandle() should be a member of Handle (not reader)
> and require that the handle is not locked when called, to avoid situation like
> drainAsBlobDataHandle() is called during two-phase.
After the first read / beginRead, drainAsBlobDataHandle() will return nullptr. I
think it should be no problem.

Placing it in Reader means it works as reading all data synchronously from the
reader and returning the handle.
Placing it in Handle means it works as taking an internal reader, reading all
data synchronously from the reader, releasing the reader and returning the
handle.

Both look reasonable to me.

[hiroshige, 2015-06-17 16:23:02]

On 2015/06/17 11:11:47, yhirano wrote:
> On 2015/06/17 10:55:31, hiroshige wrote:
> > On 2015/06/17 09:36:35, yhirano wrote:
> > > Looks good, but I have a question about FetchDataConsumerHandle.h
interface
> at
> > > [1].
> > > One possible fix is to state that obtainBlobDataConsumerHandle will not
> return
> > a
> > > valid handle with size unset.
> > > 
> > > 1:
> > >
> >
>
https://codereview.chromium.org/1179393007/diff/80001/Source/modules/fetch/Fe...
> > 
> > Oh, I misunderstood the semantics of obtainBlobDataHandle();
> > I thought the data is still available via reader after
obtainBlobDataHandle(),
> > but the reader should return Done after obtainBlobDataHandle(), right?
> I thought so.
> 
> > If so, name such as drainAsBlobDataHandle() might be better.
> Agreed.
> 
> > Also, maybe drainAsBlobDataHandle() should be a member of Handle (not
reader)
> > and require that the handle is not locked when called, to avoid situation
like
> > drainAsBlobDataHandle() is called during two-phase.
> After the first read / beginRead, drainAsBlobDataHandle() will return nullptr.
I
> think it should be no problem.
> 
> Placing it in Reader means it works as reading all data synchronously from the
> reader and returning the handle.
> Placing it in Handle means it works as taking an internal reader, reading all
> data synchronously from the reader, releasing the reader and returning the
> handle.
> 
> Both look reasonable to me.

Done: I renamed obtainBlobDataHandle to drainAsBlobDataHandle, and updated the
comments.

[hiroshige, 2015-06-17 16:24:55]

> After the first read / beginRead, drainAsBlobDataHandle() will return nullptr.
> think it should be no problem.
I see. So I kept drainAsBlobDataHandle() in Reader.

[yhirano, 2015-06-18 06:46:43]

lgtm

[hiroshige, 2015-06-18 07:02:04]

The CQ bit was checked by hiroshige@chromium.org

[commit-bot: I haz the power, 2015-06-18 07:02:21]

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

[commit-bot: I haz the power, 2015-06-18 09:12:34]

Committed patchset #12 (id:280001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197333