[2a] Implement FetchDataLoader

Implement FetchDataLoader

This class is intended to replace BlobCreator and FileReader currently used in fetch/modules for data loading.

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

[hiroshige, 2015-06-16 11:59:19]

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

[hiroshige, 2015-06-16 12:00:00]

PTAL.

[hiroshige, 2015-06-16 12:53:53]

Depending on FetchDataConsumerHandle in
https://codereview.chromium.org/1176243004/

[yhirano, 2015-06-17 05:18:54]

Can we have unit tests?

https://codereview.chromium.org/1179393007/diff/60001/Source/modules/fetch/Fe...
File Source/modules/fetch/FetchDataLoader.cpp (right):

https://codereview.chromium.org/1179393007/diff/60001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchDataLoader.cpp:6: #include
"modules/fetch/FetchDataLoader.h"
+wtf/text/WTFString.h

https://codereview.chromium.org/1179393007/diff/60001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchDataLoader.cpp:16: FetchDataLoaderAsBlobHandle(const
String& mimeType)
explicit

https://codereview.chromium.org/1179393007/diff/60001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchDataLoader.cpp:26: m_reader =
handle->obtainReader(this);
Please say that passing |this| is safe because |this| owns |m_reader|.

https://codereview.chromium.org/1179393007/diff/60001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchDataLoader.cpp:34:
m_client->didFetchDataLoadedBlobHandle(BlobDataHandle::create(blobHandle->uuid(),
m_mimeType, blobHandle->size()));
[optional] for m_client->call() pattern, I think
{
    auto client = m_client;
    m_client = nullptr;
    client->call();
}
is more robust.

ditto for below.

https://codereview.chromium.org/1179393007/diff/60001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchDataLoader.cpp:131: m_reader->endRead(available);
You can move this endRead call out of this block and remove the else block.

https://codereview.chromium.org/1179393007/diff/60001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchDataLoader.cpp:146: default:
You don't need this default clause (compiler checks the type).

https://codereview.chromium.org/1179393007/diff/60001/Source/modules/fetch/Fe...
File Source/modules/fetch/FetchDataLoader.h (right):

https://codereview.chromium.org/1179393007/diff/60001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchDataLoader.h:11: #include "wtf/ArrayBufferBuilder.h"
+wtf/Forward.h

https://codereview.chromium.org/1179393007/diff/60001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchDataLoader.h:11: #include "wtf/ArrayBufferBuilder.h"
Why do we need ArrayBufferBuilder.h?

https://codereview.chromium.org/1179393007/diff/60001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchDataLoader.h:36: class FetchDataLoader {
It is possible to make this class GarbageCollectedFinalized and make Client
GarbageCollectedMixin. Then we can simplify the lifetime comments. What do you
think about it?

https://codereview.chromium.org/1179393007/diff/60001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchDataLoader.h:58: // start() do not take the ownership
of |handle|.
+"|handle| must not be locked when called".

https://codereview.chromium.org/1179393007/diff/60001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchDataLoader.h:62: FetchDataLoader();
You can make this constructor public (or delete it if you remove members): In
any case this class cannot be instantiated because it has a pure virtual
function.

https://codereview.chromium.org/1179393007/diff/60001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchDataLoader.h:64:
OwnPtr<FetchDataConsumerHandle::Reader> m_reader;
Can you move these members to derived classes? Then we can remove this protected
section.

[hiroshige, 2015-06-17 08:48:55]

I'll respond about unit tests and GC-related things in a later CL.

https://codereview.chromium.org/1179393007/diff/60001/Source/modules/fetch/Fe...
File Source/modules/fetch/FetchDataLoader.cpp (right):

https://codereview.chromium.org/1179393007/diff/60001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchDataLoader.cpp:6: #include
"modules/fetch/FetchDataLoader.h"
On 2015/06/17 05:18:53, yhirano wrote:
> +wtf/text/WTFString.h

Done.

https://codereview.chromium.org/1179393007/diff/60001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchDataLoader.cpp:16: FetchDataLoaderAsBlobHandle(const
String& mimeType)
On 2015/06/17 05:18:53, yhirano wrote:
> explicit

Done.

https://codereview.chromium.org/1179393007/diff/60001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchDataLoader.cpp:26: m_reader =
handle->obtainReader(this);
On 2015/06/17 05:18:53, yhirano wrote:
> Please say that passing |this| is safe because |this| owns |m_reader|.

Done.

https://codereview.chromium.org/1179393007/diff/60001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchDataLoader.cpp:34:
m_client->didFetchDataLoadedBlobHandle(BlobDataHandle::create(blobHandle->uuid(),
m_mimeType, blobHandle->size()));
On 2015/06/17 05:18:53, yhirano wrote:
> [optional] for m_client->call() pattern, I think
> {
>     auto client = m_client;
>     m_client = nullptr;
>     client->call();
> }
> is more robust.
> 
> ditto for below.

Done.

https://codereview.chromium.org/1179393007/diff/60001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchDataLoader.cpp:131: m_reader->endRead(available);
On 2015/06/17 05:18:53, yhirano wrote:
> You can move this endRead call out of this block and remove the else block.

Done.

https://codereview.chromium.org/1179393007/diff/60001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchDataLoader.cpp:146: default:
On 2015/06/17 05:18:53, yhirano wrote:
> You don't need this default clause (compiler checks the type).

Done.

https://codereview.chromium.org/1179393007/diff/60001/Source/modules/fetch/Fe...
File Source/modules/fetch/FetchDataLoader.h (right):

https://codereview.chromium.org/1179393007/diff/60001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchDataLoader.h:11: #include "wtf/ArrayBufferBuilder.h"
On 2015/06/17 05:18:54, yhirano wrote:
> +wtf/Forward.h

Done.

https://codereview.chromium.org/1179393007/diff/60001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchDataLoader.h:11: #include "wtf/ArrayBufferBuilder.h"
On 2015/06/17 05:18:54, yhirano wrote:
> Why do we need ArrayBufferBuilder.h?

Moved to .cpp.

https://codereview.chromium.org/1179393007/diff/60001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchDataLoader.h:58: // start() do not take the ownership
of |handle|.
On 2015/06/17 05:18:54, yhirano wrote:
> +"|handle| must not be locked when called".

Done.

https://codereview.chromium.org/1179393007/diff/60001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchDataLoader.h:62: FetchDataLoader();
On 2015/06/17 05:18:54, yhirano wrote:
> You can make this constructor public (or delete it if you remove members): In
> any case this class cannot be instantiated because it has a pure virtual
> function.

Done.

https://codereview.chromium.org/1179393007/diff/60001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchDataLoader.h:64:
OwnPtr<FetchDataConsumerHandle::Reader> m_reader;
On 2015/06/17 05:18:54, yhirano wrote:
> Can you move these members to derived classes? Then we can remove this
protected
> section.

Done.

[yhirano, 2015-06-17 09:27:58]

https://codereview.chromium.org/1179393007/diff/80001/Source/modules/fetch/Fe...
File Source/modules/fetch/FetchDataLoader.cpp (right):

https://codereview.chromium.org/1179393007/diff/80001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchDataLoader.cpp:33: if (blobHandle &&
blobHandle->size() != kuint64max) {
Hmm, If taking a non-null BlobDataHandle drains all data from the reader, this
object creates an empty blob when blobHandle != null and blobHandle->size() ==
kuint64max at L33. Is my understanding correct?

https://codereview.chromium.org/1179393007/diff/80001/Source/modules/fetch/Fe...
File Source/modules/fetch/FetchDataLoader.h (right):

https://codereview.chromium.org/1179393007/diff/80001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchDataLoader.h:15: // FetchDataLoader subclasses
nit: "A FecthDataLoader subclass takes" or "FetchDataLoader subclasses take"

ditto for read / call

https://codereview.chromium.org/1179393007/diff/80001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchDataLoader.h:43: virtual void
didFetchDataLoadedBlobHandle(PassRefPtr<BlobDataHandle>)
[optional] Is it a good idea to put ASSERT_NOT_REACHED in each method?

[hiroshige, 2015-06-17 18:24:49]

Patchset #6 (id:100001) has been deleted

[hiroshige, 2015-06-17 18:27:29]

(except for unit tests)

https://codereview.chromium.org/1179393007/diff/60001/Source/modules/fetch/Fe...
File Source/modules/fetch/FetchDataLoader.h (right):

https://codereview.chromium.org/1179393007/diff/60001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchDataLoader.h:36: class FetchDataLoader {
On 2015/06/17 05:18:53, yhirano wrote:
> It is possible to make this class GarbageCollectedFinalized and make Client
> GarbageCollectedMixin. Then we can simplify the lifetime comments. What do you
> think about it?

Done.

https://codereview.chromium.org/1179393007/diff/80001/Source/modules/fetch/Fe...
File Source/modules/fetch/FetchDataLoader.cpp (right):

https://codereview.chromium.org/1179393007/diff/80001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchDataLoader.cpp:33: if (blobHandle &&
blobHandle->size() != kuint64max) {
On 2015/06/17 09:27:58, yhirano wrote:
> Hmm, If taking a non-null BlobDataHandle drains all data from the reader, this
> object creates an empty blob when blobHandle != null and blobHandle->size() ==
> kuint64max at L33. Is my understanding correct?

Agree; I add a restriction that drainAsBlobDataHandle()->size() is valid (if
non-null) https://codereview.chromium.org/1176243004/ and updated this CL.

https://codereview.chromium.org/1179393007/diff/80001/Source/modules/fetch/Fe...
File Source/modules/fetch/FetchDataLoader.h (right):

https://codereview.chromium.org/1179393007/diff/80001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchDataLoader.h:15: // FetchDataLoader subclasses
On 2015/06/17 09:27:58, yhirano wrote:
> nit: "A FecthDataLoader subclass takes" or "FetchDataLoader subclasses take"
> 
> ditto for read / call
> 

Done.

https://codereview.chromium.org/1179393007/diff/80001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchDataLoader.h:43: virtual void
didFetchDataLoadedBlobHandle(PassRefPtr<BlobDataHandle>)
On 2015/06/17 09:27:58, yhirano wrote:
> [optional] Is it a good idea to put ASSERT_NOT_REACHED in each method?

Done.

[yhirano, 2015-06-18 11:05:06]

https://codereview.chromium.org/1179393007/diff/120001/Source/modules/fetch/F...
File Source/modules/fetch/FetchDataLoader.h (right):

https://codereview.chromium.org/1179393007/diff/120001/Source/modules/fetch/F...
Source/modules/fetch/FetchDataLoader.h:60: virtual void
start(FetchDataConsumerHandle* /* handle */, Client*) = 0;
Just curious, is there any reason to prevent this function from taking handle's
ownership?

[hiroshige, 2015-06-18 11:48:02]

On 2015/06/18 11:05:06, yhirano wrote:
>
https://codereview.chromium.org/1179393007/diff/120001/Source/modules/fetch/F...
> File Source/modules/fetch/FetchDataLoader.h (right):
> 
>
https://codereview.chromium.org/1179393007/diff/120001/Source/modules/fetch/F...
> Source/modules/fetch/FetchDataLoader.h:60: virtual void
> start(FetchDataConsumerHandle* /* handle */, Client*) = 0;
> Just curious, is there any reason to prevent this function from taking
handle's
> ownership?

Currently taking ownership causes layout test crashes in cachestorage in my
latest CL.
I'd like to keep FetchDataLoader not to take the ownership at least for a while,
because
- FetchDataLoader itself works well without ownership, and
- Debugging will be easier.

[hiroshige, 2015-06-18 18:19:53]

Added unit tests.

[yhirano, 2015-06-19 06:19:30]

https://codereview.chromium.org/1179393007/diff/160001/Source/modules/fetch/F...
File Source/modules/fetch/FetchDataLoader.h (right):

https://codereview.chromium.org/1179393007/diff/160001/Source/modules/fetch/F...
Source/modules/fetch/FetchDataLoader.h:27: class FetchDataLoader : public
GarbageCollectedFinalized<FetchDataLoader> {
Is it worth mentioning that |start| can be called at most once? Is it OK to call
|start| after cancelling?

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

https://codereview.chromium.org/1179393007/diff/160001/Source/modules/fetch/F...
Source/modules/fetch/FetchDataLoaderTest.cpp:40: ~MockReader()
+ override

https://codereview.chromium.org/1179393007/diff/160001/Source/modules/fetch/F...
Source/modules/fetch/FetchDataLoaderTest.cpp:82: char buffer[16] = "Quick brown
fox";
+const?

https://codereview.chromium.org/1179393007/diff/160001/Source/modules/fetch/F...
Source/modules/fetch/FetchDataLoaderTest.cpp:82: char buffer[16] = "Quick brown
fox";
[optional] It would be good to decrease the magic number "16" and "15".

You can use strlen, or you can define
const char kQuickBrownFox[] = "Quick brown for";
and
const size_t kQuickBrownFoxLength = WTF_ARRAY_LENGTH(kQuickBrownFox) - 1;
possibly globally.

[hiroshige, 2015-06-19 07:27:57]

Also added a test for loading a string with null bytes.

https://codereview.chromium.org/1179393007/diff/160001/Source/modules/fetch/F...
File Source/modules/fetch/FetchDataLoader.h (right):

https://codereview.chromium.org/1179393007/diff/160001/Source/modules/fetch/F...
Source/modules/fetch/FetchDataLoader.h:27: class FetchDataLoader : public
GarbageCollectedFinalized<FetchDataLoader> {
On 2015/06/19 06:19:29, yhirano wrote:
> Is it worth mentioning that |start| can be called at most once? Is it OK to
call
> |start| after cancelling?

Added a comment in start().

In current implementation it is (probably) OK to call |start| after cancelling,
but it was not originally intended  and anyway I added restriction.

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

https://codereview.chromium.org/1179393007/diff/160001/Source/modules/fetch/F...
Source/modules/fetch/FetchDataLoaderTest.cpp:40: ~MockReader()
On 2015/06/19 06:19:30, yhirano wrote:
> + override

Done.

https://codereview.chromium.org/1179393007/diff/160001/Source/modules/fetch/F...
Source/modules/fetch/FetchDataLoaderTest.cpp:82: char buffer[16] = "Quick brown
fox";
On 2015/06/19 06:19:30, yhirano wrote:
> +const?

Done.

https://codereview.chromium.org/1179393007/diff/160001/Source/modules/fetch/F...
Source/modules/fetch/FetchDataLoaderTest.cpp:82: char buffer[16] = "Quick brown
fox";
On 2015/06/19 06:19:30, yhirano wrote:
> [optional] It would be good to decrease the magic number "16" and "15".
> 
> You can use strlen, or you can define
> const char kQuickBrownFox[] = "Quick brown for";
> and
> const size_t kQuickBrownFoxLength = WTF_ARRAY_LENGTH(kQuickBrownFox) - 1;
> possibly globally.

I introduced kQuickBrownFox, kQuickBrownFoxLength,
kQuickBrownFoxLengthWithTerminatingNull to remove duplication but prefer to keep
magic numbers to avoid logics (strlen or WTF_ARRAY_LENGTH) in the tests.

[yhirano, 2015-06-19 07:33:53]

LGTM!

[hiroshige, 2015-06-19 08:00:17]

The CQ bit was checked by hiroshige@chromium.org

[commit-bot: I haz the power, 2015-06-19 08:00:49]

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

[commit-bot: I haz the power, 2015-06-19 09:20:47]

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