[2c] Replace Body::readAsyncFromBlob with readAsyncFromFetchDataConsumerHandle

Replace Body::readAsyncFromBlob with readAsyncFromFetchDataConsumerHandle

Replace readAsyncFromBlob (that loads data from BlobDataHandle by FileReader) with readAsyncFromFetchDataConsumerHandle (that loads data from FetchDataConsumerHandle by FetchDataLoader) + FetchBlobDataConsumerHandle.

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

[hiroshige, 2015-06-19 10:02:45]

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

[hiroshige, 2015-06-19 10:05:55]

PTAL.

https://codereview.chromium.org/1173173006/diff/20001/Source/modules/fetch/Bo...
File Source/modules/fetch/Body.cpp (left):

https://codereview.chromium.org/1173173006/diff/20001/Source/modules/fetch/Bo...
Source/modules/fetch/Body.cpp:479: // Canceling the load will call didFail which
will remove the resolver.
Currently Patch Set doesn't remove the resolver, because I thought, despite of
this comment, didFail() does nothing after stop() due to:
    if (!executionContext() || executionContext()->activeDOMObjectsAreStopped())
        return;

https://codereview.chromium.org/1173173006/diff/20001/Source/modules/fetch/Bo...
Source/modules/fetch/Body.cpp:583: m_resolver->resolve("");
I changed this resolve() to reject() to pass layout tests.
Was there any dependencies that left this resolve() despite of the FIXME
comment?

[yhirano, 2015-06-19 13:39:15]

https://codereview.chromium.org/1173173006/diff/20001/Source/modules/fetch/Bo...
File Source/modules/fetch/Body.cpp (left):

https://codereview.chromium.org/1173173006/diff/20001/Source/modules/fetch/Bo...
Source/modules/fetch/Body.cpp:479: // Canceling the load will call didFail which
will remove the resolver.
On 2015/06/19 10:05:55, hiroshige wrote:
> Currently Patch Set doesn't remove the resolver, because I thought, despite of
> this comment, didFail() does nothing after stop() due to:
>     if (!executionContext() ||
executionContext()->activeDOMObjectsAreStopped())
>         return;
> 

I think this comment says that m_resolver will be cleared in didFail. By the
way, we don't have didFail any more. You can update the comment, or delete it -
the comment doesn't look so helpful.

https://codereview.chromium.org/1173173006/diff/20001/Source/modules/fetch/Bo...
Source/modules/fetch/Body.cpp:583: m_resolver->resolve("");
On 2015/06/19 10:05:55, hiroshige wrote:
> I changed this resolve() to reject() to pass layout tests.
> Was there any dependencies that left this resolve() despite of the FIXME
> comment?

This was added at
https://codereview.chromium.org/745143002/diff2/20001:80001/Source/modules/se...
I don't remember why.

[hiroshige, 2015-06-22 11:22:20]

https://codereview.chromium.org/1173173006/diff/20001/Source/modules/fetch/Bo...
File Source/modules/fetch/Body.cpp (left):

https://codereview.chromium.org/1173173006/diff/20001/Source/modules/fetch/Bo...
Source/modules/fetch/Body.cpp:479: // Canceling the load will call didFail which
will remove the resolver.
On 2015/06/19 13:39:15, yhirano wrote:
> On 2015/06/19 10:05:55, hiroshige wrote:
> > Currently Patch Set doesn't remove the resolver, because I thought, despite
of
> > this comment, didFail() does nothing after stop() due to:
> >     if (!executionContext() ||
> executionContext()->activeDOMObjectsAreStopped())
> >         return;
> > 
> 
> I think this comment says that m_resolver will be cleared in didFail. By the
> way, we don't have didFail any more. You can update the comment, or delete it
-
> the comment doesn't look so helpful.

Done.

[hiroshige, 2015-06-23 13:14:26]

https://codereview.chromium.org/1173173006/diff/60001/Source/modules/fetch/Bo...
File Source/modules/fetch/Body.cpp (right):

https://codereview.chromium.org/1173173006/diff/60001/Source/modules/fetch/Bo...
Source/modules/fetch/Body.cpp:551:
m_resolver->reject(V8ThrowException::createTypeError(state->isolate(), "Failed
to fetch"));
didFetchDataLoadFailed() can be called e.g. in
http/tests/serviceworker/resources/invalid-chunked-encoding.php.
Previously, didFail() of ThreadableLoaderClient was called and so I copied the
reject() code from FetchManager::Loader::failed().

However, in terms of the spec, there seem no conditions where we can reject in
text().

[yhirano, 2015-06-25 04:37:08]

https://codereview.chromium.org/1173173006/diff/60001/Source/modules/fetch/Bo...
File Source/modules/fetch/Body.cpp (right):

https://codereview.chromium.org/1173173006/diff/60001/Source/modules/fetch/Bo...
Source/modules/fetch/Body.cpp:385:
m_fetchDataLoader->start(createFetchDataConsumerHandleFromWebHandle(createDoneDataConsumerHandle()).get(),
this);
|handle| will never be null.

[hiroshige, 2015-06-25 05:39:17]

https://codereview.chromium.org/1173173006/diff/60001/Source/modules/fetch/Bo...
File Source/modules/fetch/Body.cpp (right):

https://codereview.chromium.org/1173173006/diff/60001/Source/modules/fetch/Bo...
Source/modules/fetch/Body.cpp:385:
m_fetchDataLoader->start(createFetchDataConsumerHandleFromWebHandle(createDoneDataConsumerHandle()).get(),
this);
On 2015/06/25 04:37:08, yhirano wrote:
> |handle| will never be null.

Done.

[yhirano, 2015-06-25 05:42:19]

lgtm

[hiroshige, 2015-06-25 06:33:19]

The CQ bit was checked by hiroshige@chromium.org

[hiroshige, 2015-06-25 06:35:39]

The CQ bit was unchecked by hiroshige@chromium.org

[hiroshige, 2015-07-02 15:45:28]

The CQ bit was checked by hiroshige@chromium.org

[commit-bot: I haz the power, 2015-07-02 15:45:47]

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

[hiroshige, 2015-07-02 15:45:53]

The CQ bit was unchecked by hiroshige@chromium.org

[hiroshige, 2015-07-02 15:49:12]

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

[hiroshige, 2015-07-02 15:49:13]

+horo@ as a modules/cachestorage/ and modules/serviceworker owner.
Could you take a look at the changes (added #include due to removal of #include
in modules/fetch/) to the files under those directories?

[hiroshige, 2015-07-02 16:17:34]

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

[hiroshige, 2015-07-02 16:17:34]

The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/1173173006/#ps120001
(title: "Add #include to ResponseTest.cpp")

[commit-bot: I haz the power, 2015-07-02 16:17:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1173173006/120001

[commit-bot: I haz the power, 2015-07-02 16:53:16]

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

[commit-bot: I haz the power, 2015-07-02 16:53:17]

Dry run: Try jobs failed on following builders:
  win_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)

[horo, 2015-07-03 02:23:02]

On 2015/07/02 15:49:13, hiroshige wrote:
> +horo@ as a modules/cachestorage/ and modules/serviceworker owner.
> Could you take a look at the changes (added #include due to removal of
#include
> in modules/fetch/) to the files under those directories?

lgtm

[yhirano, 2015-07-03 02:23:38]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2015-07-03 02:23:46]

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

[commit-bot: I haz the power, 2015-07-03 02:52:52]

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

[commit-bot: I haz the power, 2015-07-03 02:52:54]

Try jobs failed on following builders:
  win_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)

[yhirano, 2015-07-03 04:31:00]

https://codereview.chromium.org/1173173006/diff/120001/Source/modules/fetch/B...
File Source/modules/fetch/Body.h (right):

https://codereview.chromium.org/1173173006/diff/120001/Source/modules/fetch/B...
Source/modules/fetch/Body.h:32: , public FetchDataLoader::Client {
Maybe you need to add MODULES_EXPORT to FetchDataLoader::Client. In chromium
there is NON_EXPORTED_BASE, but I cannot find the corresponding one in Blink.

[yhirano, 2015-07-03 05:16:32]

PS8 LGTM

[yhirano, 2015-07-03 05:40:06]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2015-07-03 05:40:07]

The patchset sent to the CQ was uploaded after l-g-t-m from horo@chromium.org
Link to the patchset: https://codereview.chromium.org/1173173006/#ps140001
(title: "MODULES_EXPORT")

[commit-bot: I haz the power, 2015-07-03 05:40:11]

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

[hiroshige, 2015-07-03 06:45:19]

https://codereview.chromium.org/1173173006/diff/120001/Source/modules/fetch/B...
File Source/modules/fetch/Body.h (right):

https://codereview.chromium.org/1173173006/diff/120001/Source/modules/fetch/B...
Source/modules/fetch/Body.h:32: , public FetchDataLoader::Client {
On 2015/07/03 04:31:00, yhirano wrote:
> Maybe you need to add MODULES_EXPORT to FetchDataLoader::Client. In chromium
> there is NON_EXPORTED_BASE, but I cannot find the corresponding one in Blink.

Done.

[commit-bot: I haz the power, 2015-07-03 07:02:29]

Committed patchset #8 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=198263