Use downloadToFile option when XHR downloads a Blob

Use downloadToFile option when XHR downloads a Blob

Leveraging downloadToFile when XHR downloads a Blob. As a result, a Blob
is downloaded in the browser. It eliminates unnecessary memory copies
between the renderer and the browser.

When downloadToFile options is specified, didDownloadData is called
instead of didReceiveData. So not to break the Inspector, we add the
inspector instrumentations to it.

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

[yusukesuzuki, 2013-09-19 04:23:36]

https://codereview.chromium.org/23444058/diff/29001/LayoutTests/http/tests/in...
File
LayoutTests/http/tests/inspector/network/network-xhr-async-response-type-blob-expected.txt
(right):

https://codereview.chromium.org/23444058/diff/29001/LayoutTests/http/tests/in...
LayoutTests/http/tests/inspector/network/network-xhr-async-response-type-blob-expected.txt:8:
resource.content after requesting content: null
The requestContent result for XHR with responseType="blob" becomes null.
It is discussed at https://codereview.chromium.org/23658039/.

https://codereview.chromium.org/23444058/diff/29001/LayoutTests/platform/win/...
File
LayoutTests/platform/win/http/tests/xmlhttprequest/xmlhttprequest-no-content-length-onProgress-response-type-blob-expected.txt
(right):

https://codereview.chromium.org/23444058/diff/29001/LayoutTests/platform/win/...
LayoutTests/platform/win/http/tests/xmlhttprequest/xmlhttprequest-no-content-length-onProgress-response-type-blob-expected.txt:7:
FAILED
"xmlhttprequest/xmlhttprequest-no-content-length-noProgress-response-type-blob.html"
test is a specialized version of
"xmlhttprequest/xmlhttprequest-no-content-length-noProgress.html" to check
response type blob case.
And the original test also has a platform dependent result, it is filed as
"LayoutTests/platform/win/http/tests/xmlhttprequest/xmlhttprequest-no-content-length-onProgress-expected.txt".

https://codereview.chromium.org/23444058/diff/29001/Source/core/loader/FrameF...
File Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/23444058/diff/29001/Source/core/loader/FrameF...
Source/core/loader/FrameFetchContext.cpp:142:
InspectorInstrumentation::didReceiveDataWithoutData(m_frame, identifier,
dataLength, encodedDataLength);
Since it is implementation details that XHR uses the downloadToFile option when
response type is 'blob', we use `InspectorInstrumentation::didReceiveData*`
interfaces.

[eustas, 2013-09-19 04:52:37]

https://codereview.chromium.org/23444058/diff/29001/Source/core/inspector/Ins...
File Source/core/inspector/InspectorInstrumentation.idl (right):

https://codereview.chromium.org/23444058/diff/29001/Source/core/inspector/Ins...
Source/core/inspector/InspectorInstrumentation.idl:286: void
didReceiveDataWithoutData(Frame*, unsigned long identifier, int dataLength, int
encodedDataLength);
The method name doesn't sound good.

Why don't reuse "didReceiveData" and clarify that data could be null?

[yusukesuzuki, 2013-09-19 05:13:58]

https://codereview.chromium.org/23444058/diff/29001/Source/core/inspector/Ins...
File Source/core/inspector/InspectorInstrumentation.idl (right):

https://codereview.chromium.org/23444058/diff/29001/Source/core/inspector/Ins...
Source/core/inspector/InspectorInstrumentation.idl:286: void
didReceiveDataWithoutData(Frame*, unsigned long identifier, int dataLength, int
encodedDataLength);
On 2013/09/19 04:52:37, eustas.ru wrote:
> The method name doesn't sound good.
> 
> Why don't reuse "didReceiveData" and clarify that data could be null?

Since didDownloadData always has no data, I think that clarifying it by function
signature is better.
But reusing "didReceiveData" and adding comment about that is also reasonable.

[yusukesuzuki, 2013-09-19 05:33:06]

https://codereview.chromium.org/23444058/diff/29001/Source/core/inspector/Ins...
File Source/core/inspector/InspectorInstrumentation.idl (right):

https://codereview.chromium.org/23444058/diff/29001/Source/core/inspector/Ins...
Source/core/inspector/InspectorInstrumentation.idl:286: void
didReceiveDataWithoutData(Frame*, unsigned long identifier, int dataLength, int
encodedDataLength);
On 2013/09/19 04:52:37, eustas.ru wrote:
> The method name doesn't sound good.
> 
> Why don't reuse "didReceiveData" and clarify that data could be null?

Is it more preferable reusing "didReceiveData"?

https://codereview.chromium.org/23444058/diff/38001/LayoutTests/platform/linu...
File
LayoutTests/platform/linux/http/tests/xmlhttprequest/xmlhttprequest-no-content-length-onProgress-response-type-blob-expected.txt
(right):

https://codereview.chromium.org/23444058/diff/38001/LayoutTests/platform/linu...
LayoutTests/platform/linux/http/tests/xmlhttprequest/xmlhttprequest-no-content-length-onProgress-response-type-blob-expected.txt:7:
PASSED
Since test baseline is `linux -> win -> generic`, I've added linux expected
file.

[tyoshino (SeeGerritForStatus), 2013-09-19 06:58:45]

https://codereview.chromium.org/23444058/diff/38001/Source/core/loader/FrameF...
File Source/core/loader/FrameFetchContext.h (right):

https://codereview.chromium.org/23444058/diff/38001/Source/core/loader/FrameF...
Source/core/loader/FrameFetchContext.h:60: virtual void
dispatchDidDownloadData(DocumentLoader*, unsigned long identifier, int
dataLength, int encodedDataLength)  OVERRIDE;
double spaces

https://codereview.chromium.org/23444058/diff/38001/Source/core/xml/XMLHttpRe...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/23444058/diff/38001/Source/core/xml/XMLHttpRe...
Source/core/xml/XMLHttpRequest.cpp:287: // we redirect the downloaded data to
the file-handle directly
the file-handle -> a file-handle

https://codereview.chromium.org/23444058/diff/38001/Source/core/xml/XMLHttpRe...
Source/core/xml/XMLHttpRequest.cpp:1262: if (!m_error) {
factor out L1232-1245 and reuse it

https://codereview.chromium.org/23444058/diff/38001/Source/core/xml/XMLHttpRe...
File Source/core/xml/XMLHttpRequest.h (right):

https://codereview.chromium.org/23444058/diff/38001/Source/core/xml/XMLHttpRe...
Source/core/xml/XMLHttpRequest.h:161: 
no need to have a blank line here. let's keep these ThreadableLoaderClient
methods grouped.

[yusukesuzuki, 2013-09-19 07:19:06]

https://codereview.chromium.org/23444058/diff/38001/Source/core/loader/FrameF...
File Source/core/loader/FrameFetchContext.h (right):

https://codereview.chromium.org/23444058/diff/38001/Source/core/loader/FrameF...
Source/core/loader/FrameFetchContext.h:60: virtual void
dispatchDidDownloadData(DocumentLoader*, unsigned long identifier, int
dataLength, int encodedDataLength)  OVERRIDE;
On 2013/09/19 06:58:46, tyoshino wrote:
> double spaces

Done.

https://codereview.chromium.org/23444058/diff/38001/Source/core/xml/XMLHttpRe...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/23444058/diff/38001/Source/core/xml/XMLHttpRe...
Source/core/xml/XMLHttpRequest.cpp:287: // we redirect the downloaded data to
the file-handle directly
On 2013/09/19 06:58:46, tyoshino wrote:
> the file-handle -> a file-handle

Done.

https://codereview.chromium.org/23444058/diff/38001/Source/core/xml/XMLHttpRe...
Source/core/xml/XMLHttpRequest.cpp:1262: if (!m_error) {
On 2013/09/19 06:58:46, tyoshino wrote:
> factor out L1232-1245 and reuse it

Done. Seeing the m_receivedLength comment, named `trackProgress`.

https://codereview.chromium.org/23444058/diff/38001/Source/core/xml/XMLHttpRe...
File Source/core/xml/XMLHttpRequest.h (right):

https://codereview.chromium.org/23444058/diff/38001/Source/core/xml/XMLHttpRe...
Source/core/xml/XMLHttpRequest.h:161: 
On 2013/09/19 06:58:46, tyoshino wrote:
> no need to have a blank line here. let's keep these ThreadableLoaderClient
> methods grouped.

Done.

[yusukesuzuki, 2013-09-19 07:34:33]

https://codereview.chromium.org/23444058/diff/45001/Source/core/xml/XMLHttpRe...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/23444058/diff/45001/Source/core/xml/XMLHttpRe...
Source/core/xml/XMLHttpRequest.cpp:770: // we redirect the downloaded data to a
file-handle directly
Fixed comment.

https://codereview.chromium.org/23444058/diff/45001/Source/core/xml/XMLHttpRe...
Source/core/xml/XMLHttpRequest.cpp:799: // Since we redirect the downloaded data
to a file-handle directly
Fixed comment.

[eustas, 2013-09-19 08:28:41]

> Since didDownloadData always has no data, I think that clarifying it by
function
> signature is better.
> But reusing "didReceiveData" and adding comment about that is also reasonable.

As I see, didReceiveData already checks if data is null. So, the new method
looks redundant.

I think that calling this method with null clearly states that we intentionally
do not send data =)

[yusukesuzuki, 2013-09-19 15:24:44]

On 2013/09/19 08:28:41, eustas.ru wrote:
> I think that calling this method with null clearly states that we
intentionally
> do not send data =)

OK. So I'll follow your suggestion. Thanks :)

[yusukesuzuki, 2013-09-19 15:41:20]

https://chromiumcodereview.appspot.com/23444058/diff/29001/Source/core/inspec...
File Source/core/inspector/InspectorInstrumentation.idl (right):

https://chromiumcodereview.appspot.com/23444058/diff/29001/Source/core/inspec...
Source/core/inspector/InspectorInstrumentation.idl:286: void
didReceiveDataWithoutData(Frame*, unsigned long identifier, int dataLength, int
encodedDataLength);
On 2013/09/19 04:52:37, eustas.ru wrote:
> The method name doesn't sound good.
> 
> Why don't reuse "didReceiveData" and clarify that data could be null?

Done. Thanks for your suggestion.

[tyoshino (SeeGerritForStatus), 2013-09-24 10:28:30]

lgtm

https://codereview.chromium.org/23444058/diff/64001/LayoutTests/http/tests/xm...
File
LayoutTests/http/tests/xmlhttprequest/infoOnProgressEvent-response-type-blob.html
(right):

https://codereview.chromium.org/23444058/diff/64001/LayoutTests/http/tests/xm...
LayoutTests/http/tests/xmlhttprequest/infoOnProgressEvent-response-type-blob.html:32:
}
this is not used (original file doesn't use it, too).

https://codereview.chromium.org/23444058/diff/64001/LayoutTests/http/tests/xm...
LayoutTests/http/tests/xmlhttprequest/infoOnProgressEvent-response-type-blob.html:34:
var shouldNotify = false;
ditto

https://codereview.chromium.org/23444058/diff/64001/LayoutTests/http/tests/xm...
File
LayoutTests/http/tests/xmlhttprequest/resources/xmlhttprequest-response-type-blob.js
(right):

https://codereview.chromium.org/23444058/diff/64001/LayoutTests/http/tests/xm...
LayoutTests/http/tests/xmlhttprequest/resources/xmlhttprequest-response-type-blob.js:1:
var global = window;
is this to make it clear that variables are set to window explicitly to make
shouldBe work?

https://codereview.chromium.org/23444058/diff/64001/LayoutTests/http/tests/xm...
File
LayoutTests/http/tests/xmlhttprequest/workers/shared-worker-response-type-blob-sync.html
(right):

https://codereview.chromium.org/23444058/diff/64001/LayoutTests/http/tests/xm...
LayoutTests/http/tests/xmlhttprequest/workers/shared-worker-response-type-blob-sync.html:7:
description('Request a file as the Blob type in synchronous XMLHttpRequest in a
shared worker.');
in -> with

https://codereview.chromium.org/23444058/diff/64001/LayoutTests/http/tests/xm...
File
LayoutTests/http/tests/xmlhttprequest/workers/shared-worker-response-type-blob.html
(right):

https://codereview.chromium.org/23444058/diff/64001/LayoutTests/http/tests/xm...
LayoutTests/http/tests/xmlhttprequest/workers/shared-worker-response-type-blob.html:7:
description('Request a file as the Blob type in asynchronous XMLHttpRequest in a
shared worker.');
in -> with

https://codereview.chromium.org/23444058/diff/64001/LayoutTests/http/tests/xm...
File
LayoutTests/http/tests/xmlhttprequest/workers/xmlhttprequest-response-type-blob-sync.html
(right):

https://codereview.chromium.org/23444058/diff/64001/LayoutTests/http/tests/xm...
LayoutTests/http/tests/xmlhttprequest/workers/xmlhttprequest-response-type-blob-sync.html:7:
description('Request a file as the Blob type in synchronous XMLHttpRequest in a
worker.');
in -> with

https://codereview.chromium.org/23444058/diff/64001/LayoutTests/http/tests/xm...
File
LayoutTests/http/tests/xmlhttprequest/workers/xmlhttprequest-response-type-blob.html
(right):

https://codereview.chromium.org/23444058/diff/64001/LayoutTests/http/tests/xm...
LayoutTests/http/tests/xmlhttprequest/workers/xmlhttprequest-response-type-blob.html:7:
description('Request a file as the Blob type in asynchronous XMLHttpRequest in a
worker.');
in -> with

https://codereview.chromium.org/23444058/diff/64001/LayoutTests/http/tests/xm...
File
LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-no-content-length-onProgress-response-type-blob.html
(right):

https://codereview.chromium.org/23444058/diff/64001/LayoutTests/http/tests/xm...
LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-no-content-length-onProgress-response-type-blob.html:5:
<p> This test verify that file with content type does trigger onProgress event
when response type 'blob' is specified. </p>
Not your mistake but
verify -> verifies

https://codereview.chromium.org/23444058/diff/64001/LayoutTests/http/tests/xm...
File
LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-response-type-blob.html
(right):

https://codereview.chromium.org/23444058/diff/64001/LayoutTests/http/tests/xm...
LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-response-type-blob.html:7:
description('Request a file as the Blob type in asynchronous XMLHttpRequest in a
document context.');
in -> with

[yusukesuzuki, 2013-09-24 11:13:35]

https://codereview.chromium.org/23444058/diff/64001/LayoutTests/http/tests/xm...
File
LayoutTests/http/tests/xmlhttprequest/infoOnProgressEvent-response-type-blob.html
(right):

https://codereview.chromium.org/23444058/diff/64001/LayoutTests/http/tests/xm...
LayoutTests/http/tests/xmlhttprequest/infoOnProgressEvent-response-type-blob.html:32:
}
On 2013/09/24 10:28:31, tyoshino wrote:
> this is not used (original file doesn't use it, too).

Done.

https://codereview.chromium.org/23444058/diff/64001/LayoutTests/http/tests/xm...
LayoutTests/http/tests/xmlhttprequest/infoOnProgressEvent-response-type-blob.html:34:
var shouldNotify = false;
On 2013/09/24 10:28:31, tyoshino wrote:
> ditto

Done.

https://codereview.chromium.org/23444058/diff/64001/LayoutTests/http/tests/xm...
File
LayoutTests/http/tests/xmlhttprequest/resources/xmlhttprequest-response-type-blob.js
(right):

https://codereview.chromium.org/23444058/diff/64001/LayoutTests/http/tests/xm...
LayoutTests/http/tests/xmlhttprequest/resources/xmlhttprequest-response-type-blob.js:1:
var global = window;
On 2013/09/24 10:28:31, tyoshino wrote:
> is this to make it clear that variables are set to window explicitly to make
> shouldBe work?

It is because of reusing the following code of
"workers/resources/xmlhttprequest-response-type-blob.js".

Discussed offline with tyoshino,
we leave this as is and don't merge them into one file since we'd like to keep
worker related js resources under worker/resources directory.

https://codereview.chromium.org/23444058/diff/64001/LayoutTests/http/tests/xm...
File
LayoutTests/http/tests/xmlhttprequest/workers/shared-worker-response-type-blob-sync.html
(right):

https://codereview.chromium.org/23444058/diff/64001/LayoutTests/http/tests/xm...
LayoutTests/http/tests/xmlhttprequest/workers/shared-worker-response-type-blob-sync.html:7:
description('Request a file as the Blob type in synchronous XMLHttpRequest in a
shared worker.');
On 2013/09/24 10:28:31, tyoshino wrote:
> in -> with

Done.

https://codereview.chromium.org/23444058/diff/64001/LayoutTests/http/tests/xm...
File
LayoutTests/http/tests/xmlhttprequest/workers/shared-worker-response-type-blob.html
(right):

https://codereview.chromium.org/23444058/diff/64001/LayoutTests/http/tests/xm...
LayoutTests/http/tests/xmlhttprequest/workers/shared-worker-response-type-blob.html:7:
description('Request a file as the Blob type in asynchronous XMLHttpRequest in a
shared worker.');
On 2013/09/24 10:28:31, tyoshino wrote:
> in -> with

Done.

https://codereview.chromium.org/23444058/diff/64001/LayoutTests/http/tests/xm...
File
LayoutTests/http/tests/xmlhttprequest/workers/xmlhttprequest-response-type-blob-sync.html
(right):

https://codereview.chromium.org/23444058/diff/64001/LayoutTests/http/tests/xm...
LayoutTests/http/tests/xmlhttprequest/workers/xmlhttprequest-response-type-blob-sync.html:7:
description('Request a file as the Blob type in synchronous XMLHttpRequest in a
worker.');
On 2013/09/24 10:28:31, tyoshino wrote:
> in -> with

Done.

https://codereview.chromium.org/23444058/diff/64001/LayoutTests/http/tests/xm...
File
LayoutTests/http/tests/xmlhttprequest/workers/xmlhttprequest-response-type-blob.html
(right):

https://codereview.chromium.org/23444058/diff/64001/LayoutTests/http/tests/xm...
LayoutTests/http/tests/xmlhttprequest/workers/xmlhttprequest-response-type-blob.html:7:
description('Request a file as the Blob type in asynchronous XMLHttpRequest in a
worker.');
On 2013/09/24 10:28:31, tyoshino wrote:
> in -> with

Done.

https://codereview.chromium.org/23444058/diff/64001/LayoutTests/http/tests/xm...
File
LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-no-content-length-onProgress-response-type-blob.html
(right):

https://codereview.chromium.org/23444058/diff/64001/LayoutTests/http/tests/xm...
LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-no-content-length-onProgress-response-type-blob.html:5:
<p> This test verify that file with content type does trigger onProgress event
when response type 'blob' is specified. </p>
On 2013/09/24 10:28:31, tyoshino wrote:
> Not your mistake but
> verify -> verifies

Done.

https://codereview.chromium.org/23444058/diff/64001/LayoutTests/http/tests/xm...
File
LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-response-type-blob.html
(right):

https://codereview.chromium.org/23444058/diff/64001/LayoutTests/http/tests/xm...
LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-response-type-blob.html:7:
description('Request a file as the Blob type in asynchronous XMLHttpRequest in a
document context.');
On 2013/09/24 10:28:31, tyoshino wrote:
> in -> with

Done.

[vsevik, 2013-09-24 13:43:17]

Adding caseq@ for inspector/timeline tests.

https://codereview.chromium.org/23444058/diff/99001/LayoutTests/http/tests/in...
File
LayoutTests/http/tests/inspector/network/network-xhr-data-received-async-response-type-blob.html
(right):

https://codereview.chromium.org/23444058/diff/99001/LayoutTests/http/tests/in...
LayoutTests/http/tests/inspector/network/network-xhr-data-received-async-response-type-blob.html:1:
<html>
I would remove this test. I don't think there is anything specific enough about
blob xhrs that makes it necessary.

https://codereview.chromium.org/23444058/diff/99001/LayoutTests/http/tests/in...
File
LayoutTests/http/tests/inspector/network/network-xhr-response-received-async-response-type-blob.html
(right):

https://codereview.chromium.org/23444058/diff/99001/LayoutTests/http/tests/in...
LayoutTests/http/tests/inspector/network/network-xhr-response-received-async-response-type-blob.html:1:
<html>
Same here.

https://codereview.chromium.org/23444058/diff/99001/Source/core/inspector/Ins...
File Source/core/inspector/InspectorPageAgent.cpp (right):

https://codereview.chromium.org/23444058/diff/99001/Source/core/inspector/Ins...
Source/core/inspector/InspectorPageAgent.cpp:132: return false;
I don't think this code is ever reached. Why do you need it?

[yusukesuzuki, 2013-09-25 01:43:04]

Thanks for your review.

https://chromiumcodereview.appspot.com/23444058/diff/99001/LayoutTests/http/t...
File
LayoutTests/http/tests/inspector/network/network-xhr-data-received-async-response-type-blob.html
(right):

https://chromiumcodereview.appspot.com/23444058/diff/99001/LayoutTests/http/t...
LayoutTests/http/tests/inspector/network/network-xhr-data-received-async-response-type-blob.html:1:
<html>
On 2013/09/24 13:43:17, vsevik wrote:
> I would remove this test. I don't think there is anything specific enough
about
> blob xhrs that makes it necessary.

Since didDownloadData is called instead of didReceiveData in XHR Blob
implementation, XHR uses newly added path to instrument its progress and this
test guarantees that the progress instrumentation works fine with XHR Blob.

But if you think it is preferable to remove this test, I'll follow.

https://chromiumcodereview.appspot.com/23444058/diff/99001/LayoutTests/http/t...
File
LayoutTests/http/tests/inspector/network/network-xhr-response-received-async-response-type-blob.html
(right):

https://chromiumcodereview.appspot.com/23444058/diff/99001/LayoutTests/http/t...
LayoutTests/http/tests/inspector/network/network-xhr-response-received-async-response-type-blob.html:1:
<html>
On 2013/09/24 13:43:17, vsevik wrote:
> Same here.

We can remove this test since the same didReceiveResponse path is used in both
normal XHR and XHR Blob.

https://chromiumcodereview.appspot.com/23444058/diff/99001/Source/core/inspec...
File Source/core/inspector/InspectorPageAgent.cpp (right):

https://chromiumcodereview.appspot.com/23444058/diff/99001/Source/core/inspec...
Source/core/inspector/InspectorPageAgent.cpp:132: return false;
On 2013/09/24 13:43:17, vsevik wrote:
> I don't think this code is ever reached. Why do you need it?

When requestContent for XHR Blob is called, this path is used for returning null
as the content of XHR Blob.

Since XHR Blob's encodedSize is 0 (because DoNotBufferData is set),
|hasZeroSize| becomes true and XHR Blob's content becomes empty string "", not
null.

[vsevik, 2013-09-25 15:26:02]

On 2013/09/25 01:43:04, yusukesuzuki wrote:
> Thanks for your review.
> 
>
https://chromiumcodereview.appspot.com/23444058/diff/99001/LayoutTests/http/t...
> File
>
LayoutTests/http/tests/inspector/network/network-xhr-data-received-async-response-type-blob.html
> (right):
> 
>
https://chromiumcodereview.appspot.com/23444058/diff/99001/LayoutTests/http/t...
>
LayoutTests/http/tests/inspector/network/network-xhr-data-received-async-response-type-blob.html:1:
> <html>
> On 2013/09/24 13:43:17, vsevik wrote:
> > I would remove this test. I don't think there is anything specific enough
> about
> > blob xhrs that makes it necessary.
> 
> Since didDownloadData is called instead of didReceiveData in XHR Blob
> implementation, XHR uses newly added path to instrument its progress and this
> test guarantees that the progress instrumentation works fine with XHR Blob.
> 
> But if you think it is preferable to remove this test, I'll follow.
Let's keep it then.

> 
>
https://chromiumcodereview.appspot.com/23444058/diff/99001/LayoutTests/http/t...
> File
>
LayoutTests/http/tests/inspector/network/network-xhr-response-received-async-response-type-blob.html
> (right):
> 
>
https://chromiumcodereview.appspot.com/23444058/diff/99001/LayoutTests/http/t...
>
LayoutTests/http/tests/inspector/network/network-xhr-response-received-async-response-type-blob.html:1:
> <html>
> On 2013/09/24 13:43:17, vsevik wrote:
> > Same here.
> 
> We can remove this test since the same didReceiveResponse path is used in both
> normal XHR and XHR Blob.
Let's remove it then! :)

> 
>
https://chromiumcodereview.appspot.com/23444058/diff/99001/Source/core/inspec...
> File Source/core/inspector/InspectorPageAgent.cpp (right):
> 
>
https://chromiumcodereview.appspot.com/23444058/diff/99001/Source/core/inspec...
> Source/core/inspector/InspectorPageAgent.cpp:132: return false;
> On 2013/09/24 13:43:17, vsevik wrote:
> > I don't think this code is ever reached. Why do you need it?
> 
> When requestContent for XHR Blob is called, this path is used for returning
null
> as the content of XHR Blob.
> 
> Since XHR Blob's encodedSize is 0 (because DoNotBufferData is set),
> |hasZeroSize| becomes true and XHR Blob's content becomes empty string "", not
> null.
Makes sense.

http/tests/inspector/network and core/inspector changes lgtm.

[caseq, 2013-09-25 15:33:30]

https://chromiumcodereview.appspot.com/23444058/diff/104001/LayoutTests/http/...
File LayoutTests/http/tests/inspector/timeline-xhr-response-type-blob-event.html
(right):

https://chromiumcodereview.appspot.com/23444058/diff/104001/LayoutTests/http/...
LayoutTests/http/tests/inspector/timeline-xhr-response-type-blob-event.html:11:
xhr.responseType = 'blob';
nit: we use double quotes on strings by default.

[yusukesuzuki, 2013-09-25 16:26:15]

https://chromiumcodereview.appspot.com/23444058/diff/99001/LayoutTests/http/t...
File
LayoutTests/http/tests/inspector/network/network-xhr-response-received-async-response-type-blob.html
(right):

https://chromiumcodereview.appspot.com/23444058/diff/99001/LayoutTests/http/t...
LayoutTests/http/tests/inspector/network/network-xhr-response-received-async-response-type-blob.html:1:
<html>
On 2013/09/25 01:43:05, yusukesuzuki wrote:
> On 2013/09/24 13:43:17, vsevik wrote:
> > Same here.
> 
> We can remove this test since the same didReceiveResponse path is used in both
> normal XHR and XHR Blob.

Done.

https://chromiumcodereview.appspot.com/23444058/diff/104001/LayoutTests/http/...
File LayoutTests/http/tests/inspector/timeline-xhr-response-type-blob-event.html
(right):

https://chromiumcodereview.appspot.com/23444058/diff/104001/LayoutTests/http/...
LayoutTests/http/tests/inspector/timeline-xhr-response-type-blob-event.html:11:
xhr.responseType = 'blob';
On 2013/09/25 15:33:31, caseq wrote:
> nit: we use double quotes on strings by default.

Done.

[vsevik, 2013-10-01 15:00:30]

inspector lgtm

[yusukesuzuki, 2013-10-12 06:52:43]

Hi abarth@, can you take a look at this CL?

This patch introduces didDownloadData(int) path through loader and fetch layers.
And by using this, we leverage downloadToFile option to download XHR Blob
response in the browser side.

[abarth-chromium, 2013-10-13 05:20:29]

japhet should really review this CL instead of me.

https://codereview.chromium.org/23444058/diff/131001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/23444058/diff/131001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:409: void XMLHttpRequest::trackProgress(int
len)
len -> length

https://codereview.chromium.org/23444058/diff/131001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:423: // Firefox calls readyStateChanged every
time it receives data, 4449442
4449442 <-- What is this number?  Can we make it a hyperlink?

[tyoshino (SeeGerritForStatus), 2013-10-16 07:57:55]

https://codereview.chromium.org/23444058/diff/131001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/23444058/diff/131001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:423: // Firefox calls readyStateChanged every
time it receives data, 4449442
On 2013/10/13 05:20:30, abarth wrote:
> 4449442 <-- What is this number?  Can we make it a hyperlink?

This is Apple Radar ID. I've replaced this with explanation.
http://src.chromium.org/viewvc/blink/trunk/Source/core/xml/XMLHttpRequest.cpp...

So, please just rebase.

[pfeldman, 2013-10-24 13:58:07]

Is this forgotten?

[yusukesuzuki, 2013-10-24 13:59:21]

Sorry for my late reply.
I'll rebase it and fix the code pointed by reviewers.

[yusukesuzuki, 2013-10-24 15:22:40]

Sorry for my very late fix.

Now I've rebased the patch and fixed the issues pointed by reviewers abarth@ and
tyoshino@.

And since `m_receivedLength` is now cleared when XHR finishes loading, I
introduced new variable to track a size of a downloaded blob.

PTAL.

https://codereview.chromium.org/23444058/diff/131001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/23444058/diff/131001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:409: void XMLHttpRequest::trackProgress(int
len)
On 2013/10/13 05:20:30, abarth wrote:
> len -> length

Done.

https://codereview.chromium.org/23444058/diff/131001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:423: // Firefox calls readyStateChanged every
time it receives data, 4449442
On 2013/10/16 07:57:56, tyoshino wrote:
> On 2013/10/13 05:20:30, abarth wrote:
> > 4449442 <-- What is this number?  Can we make it a hyperlink?
> 
> This is Apple Radar ID. I've replaced this with explanation.
>
http://src.chromium.org/viewvc/blink/trunk/Source/core/xml/XMLHttpRequest.cpp...
> 
> So, please just rebase.

Right. I've rebased and this issue is fixed by that.

https://codereview.chromium.org/23444058/diff/209001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/23444058/diff/209001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:295: if (!filePath.isEmpty() &&
m_downloadedBlobLength) {
In the previous patch, I used m_receivedLength as Blob data length.
But now, m_receivedLength is cleared when XHR finishes loading (didFinishLoading
& clearVariablesForLoading). So I introduced new member variable
`m_downloadedBlobLength` to track a size of a downloaded Blob.

[yusukesuzuki, 2013-11-05 03:10:04]

Hi abarth@ and japhet@, could you take a look?

[abarth-chromium, 2013-11-05 07:01:50]

LGTM

https://codereview.chromium.org/23444058/diff/209001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/23444058/diff/209001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:293: const String& filePath =
m_response.downloadedFilePath();
const String& -> String

String is a value type with an internally refcounted buffer.  There's no need to
use const String&

https://codereview.chromium.org/23444058/diff/209001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:1356: return;
Can changeState set m_error?  We just checked it above.

[yusukesuzuki, 2013-11-05 11:03:59]

https://codereview.chromium.org/23444058/diff/209001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/23444058/diff/209001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:293: const String& filePath =
m_response.downloadedFilePath();
On 2013/11/05 07:01:51, abarth wrote:
> const String& -> String
> 
> String is a value type with an internally refcounted buffer.  There's no need
to
> use const String&

That's reasonable. Done.

https://codereview.chromium.org/23444058/diff/209001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:1356: return;
On 2013/11/05 07:01:51, abarth wrote:
> Can changeState set m_error?  We just checked it above.

Yeah, changeState can set m_error. changeState causes "onreadystatechange"
event.
If we call xhr.abort in the handler, m_error becomes true.

[michaeln, 2013-11-05 19:37:55]

wooohoooo :)

[yusukesuzuki, 2013-11-06 01:57:00]

Hi japhet@, could you take a look?
This change contains loader / fetch changes. So your review is needed.

[vsevik, 2013-12-03 19:59:54]

On 2013/11/06 01:57:00, yusukesuzuki wrote:
> Hi japhet@, could you take a look?
> This change contains loader / fetch changes. So your review is needed.

ping

[Nate Chapin, 2013-12-03 20:05:53]

loader and fetch changes LGTM

In the future, feel free to ping me on IRC (or corp chat if you're a googler) if
I sit on a review for longer than a day like this. I'm not very good about
reading my bugmail and tend to miss things like review pings. :(

[yusukesuzuki, 2013-12-03 20:08:56]

On 2013/12/03 20:05:53, Nate Chapin wrote:
> loader and fetch changes LGTM
> 
> In the future, feel free to ping me on IRC (or corp chat if you're a googler)
if
> I sit on a review for longer than a day like this. I'm not very good about
> reading my bugmail and tend to miss things like review pings. :(

OK. Thank you for your review :)

[commit-bot: I haz the power, 2013-12-04 01:33:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yusukesuzuki@chromium.org/23444058/519001

[commit-bot: I haz the power, 2013-12-04 03:21:35]

Change committed as 163141