[XHR] Accept data on didReceiveData call even if responseType is set to Blob

[XHR] Accept data on didReceiveData call even if responseType is set to Blob

Since Blink 175446
https://src.chromium.org/viewvc/blink?revision=175446&view=revision
we're loading data into a file. The downloadToFile flag is determined in
createRequest() i.e. inside xhr.send() call. But responseType is allowed
to be changed even after xhr.send() call.

Accumulate data into m_binaryResponseBuilder and create a Blob from it
anyway if m_downloadingToFile is not set.

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

[tyoshino (SeeGerritForStatus), 2014-09-10 09:31:14]

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

[yhirano, 2014-09-10 10:56:38]

https://codereview.chromium.org/562563003/diff/40001/LayoutTests/http/tests/x...
File
LayoutTests/http/tests/xmlhttprequest/resources/xmlhttprequest-response-type-blob.js
(right):

https://codereview.chromium.org/562563003/diff/40001/LayoutTests/http/tests/x...
LayoutTests/http/tests/xmlhttprequest/resources/xmlhttprequest-response-type-blob.js:35:
}, function (r) {
You can simply pass reject as the second argument.
Moreover, you can write like this (if you prefer):
readBlobAsArrayBuffer(req.response).then(function(value) {
...
}).then(resolve, reject);

https://codereview.chromium.org/562563003/diff/40001/LayoutTests/http/tests/x...
LayoutTests/http/tests/xmlhttprequest/resources/xmlhttprequest-response-type-blob.js:54:
readBlobAsArrayBuffer(req.response).then(function (value) {
ditto

https://codereview.chromium.org/562563003/diff/40001/LayoutTests/http/tests/x...
LayoutTests/http/tests/xmlhttprequest/resources/xmlhttprequest-response-type-blob.js:88:
}, function (r) {
ditto

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

https://codereview.chromium.org/562563003/diff/40001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:275: // When responseType is set to "blob",
we redirect the downloaded data
Please wrap comments in 80 columns.

https://codereview.chromium.org/562563003/diff/40001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:1490: if (m_error)
I'm not sure when the condition holds, but Shouldn't we omit the above assertion
when m_error is true?

[tyoshino (SeeGerritForStatus), 2014-09-10 12:19:29]

https://codereview.chromium.org/562563003/diff/40001/LayoutTests/http/tests/x...
File
LayoutTests/http/tests/xmlhttprequest/resources/xmlhttprequest-response-type-blob.js
(right):

https://codereview.chromium.org/562563003/diff/40001/LayoutTests/http/tests/x...
LayoutTests/http/tests/xmlhttprequest/resources/xmlhttprequest-response-type-blob.js:35:
}, function (r) {
On 2014/09/10 10:56:37, yhirano wrote:
> You can simply pass reject as the second argument.
> Moreover, you can write like this (if you prefer):
> readBlobAsArrayBuffer(req.response).then(function(value) {
> ...
> }).then(resolve, reject);

Done.

https://codereview.chromium.org/562563003/diff/40001/LayoutTests/http/tests/x...
LayoutTests/http/tests/xmlhttprequest/resources/xmlhttprequest-response-type-blob.js:54:
readBlobAsArrayBuffer(req.response).then(function (value) {
On 2014/09/10 10:56:38, yhirano wrote:
> ditto

Done.

https://codereview.chromium.org/562563003/diff/40001/LayoutTests/http/tests/x...
LayoutTests/http/tests/xmlhttprequest/resources/xmlhttprequest-response-type-blob.js:88:
}, function (r) {
On 2014/09/10 10:56:38, yhirano wrote:
> ditto

Done.

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

https://codereview.chromium.org/562563003/diff/40001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:275: // When responseType is set to "blob",
we redirect the downloaded data
On 2014/09/10 10:56:38, yhirano wrote:
> Please wrap comments in 80 columns.

Done.

https://codereview.chromium.org/562563003/diff/40001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:1490: if (m_error)
On 2014/09/10 10:56:38, yhirano wrote:
> I'm not sure when the condition holds, but Shouldn't we omit the above
assertion
> when m_error is true?

m_responseTypeCode check was working. So m_downloadingToFile should also work.

RedirectToFileResourceHandler is set up only when downloadToFile is specified.
There's no path (even error handling) where didDownloadData can be called
regardless of downloadToFile option.

[yhirano, 2014-09-10 12:29:26]

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

https://codereview.chromium.org/562563003/diff/40001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:1490: if (m_error)
On 2014/09/10 12:19:29, tyoshino wrote:
> On 2014/09/10 10:56:38, yhirano wrote:
> > I'm not sure when the condition holds, but Shouldn't we omit the above
> assertion
> > when m_error is true?
> 
> m_responseTypeCode check was working. So m_downloadingToFile should also work.
> 
> RedirectToFileResourceHandler is set up only when downloadToFile is specified.
> There's no path (even error handling) where didDownloadData can be called
> regardless of downloadToFile option.
m_downloadingToFile is cleared in clearResponse while m_responseTypeCode isn't.
Hence the first statement is not generally true.

[tyoshino (SeeGerritForStatus), 2014-09-10 12:48:29]

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

https://codereview.chromium.org/562563003/diff/40001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:1490: if (m_error)
On 2014/09/10 12:29:26, yhirano wrote:
> On 2014/09/10 12:19:29, tyoshino wrote:
> > On 2014/09/10 10:56:38, yhirano wrote:
> > > I'm not sure when the condition holds, but Shouldn't we omit the above
> > assertion
> > > when m_error is true?
> > 
> > m_responseTypeCode check was working. So m_downloadingToFile should also
work.
> > 
> > RedirectToFileResourceHandler is set up only when downloadToFile is
specified.
> > There's no path (even error handling) where didDownloadData can be called
> > regardless of downloadToFile option.
> m_downloadingToFile is cleared in clearResponse while m_responseTypeCode
isn't.
> Hence the first statement is not generally true.

Oh, good point! clearResponse() is basically preceded by internalAbort(). In
handleDidCancel() it isn't but in this case the loader is already stopped. So I
think we don't need to be worried about didDownloadData being called after we
reset m_downloadingToFile.

But I'm fine with putting this after m_error check. Done.

[yhirano, 2014-09-10 12:56:08]

lgtm

[tyoshino (SeeGerritForStatus), 2014-09-10 14:07:44]

tyoshino@chromium.org changed reviewers:
+ sigbjornf@opera.com

[tyoshino (SeeGerritForStatus), 2014-09-10 14:07:52]

+sof

[sof, 2014-09-10 14:21:40]

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

https://codereview.chromium.org/562563003/diff/80001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:292: if (m_binaryResponseBuilder.get() &&
m_binaryResponseBuilder->size() > 0) {
Can be shortened to 

  if (m_binaryResponseBuilder && m_binaryResponseBuilder->size())

https://codereview.chromium.org/562563003/diff/80001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:294: size = m_binaryResponseBuilder->size();
What happens if size_t is signed and MSB of the returned size here is set?

https://codereview.chromium.org/562563003/diff/80001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:872: m_downloadingToFile = true;
Move out of "if" as,

  m_downloadingToFile = responseTypeCode() == ResponseTypeBlob;
  if (m_downloadingToFile) { ... }

Goes better with the comment above.

https://codereview.chromium.org/562563003/diff/80001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequest.h (right):

https://codereview.chromium.org/562563003/diff/80001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.h:267: bool m_downloadingToFile;
Move this down into the bool block at the end.

[sof, 2014-09-10 20:24:11]

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

https://codereview.chromium.org/562563003/diff/80001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:294: size = m_binaryResponseBuilder->size();
On 2014/09/10 14:21:39, sof wrote:
> What happens if size_t is signed and MSB of the returned size here is set?

Please disregard, a non-issue. But couldn't this buffer append be simplified to

  blobData->appendBytes(m_binaryResponseBuilder->data(),
m_binaryResponseBuilder->size());

i.e., there's now a bunch of manual uses of appendData(PassRefPtr<RawData>) in
the codebase. I think we should make appendBytes() public and use it instead.

Totally fine to handle that separately & as a follow-up, though.

[tyoshino (SeeGerritForStatus), 2014-09-11 04:30:58]

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

https://codereview.chromium.org/562563003/diff/80001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:292: if (m_binaryResponseBuilder.get() &&
m_binaryResponseBuilder->size() > 0) {
On 2014/09/10 14:21:39, sof wrote:
> Can be shortened to 
> 
>   if (m_binaryResponseBuilder && m_binaryResponseBuilder->size())

Done.

https://codereview.chromium.org/562563003/diff/80001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:294: size = m_binaryResponseBuilder->size();
On 2014/09/10 20:24:10, sof wrote:
> On 2014/09/10 14:21:39, sof wrote:
> > What happens if size_t is signed and MSB of the returned size here is set?
> 
> Please disregard, a non-issue. But couldn't this buffer append be simplified
to
> 
>   blobData->appendBytes(m_binaryResponseBuilder->data(),
> m_binaryResponseBuilder->size());
> 
> i.e., there's now a bunch of manual uses of appendData(PassRefPtr<RawData>) in
> the codebase. I think we should make appendBytes() public and use it instead.
> 
> Totally fine to handle that separately & as a follow-up, though.

Sounds good. I'll do it

https://codereview.chromium.org/562563003/diff/80001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:872: m_downloadingToFile = true;
On 2014/09/10 14:21:39, sof wrote:
> Move out of "if" as,
> 
>   m_downloadingToFile = responseTypeCode() == ResponseTypeBlob;
>   if (m_downloadingToFile) { ... }
> 
> Goes better with the comment above.

Nice! Done

https://codereview.chromium.org/562563003/diff/80001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequest.h (right):

https://codereview.chromium.org/562563003/diff/80001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.h:267: bool m_downloadingToFile;
On 2014/09/10 14:21:39, sof wrote:
> Move this down into the bool block at the end.

Done.

[sof, 2014-09-11 07:04:28]

lgtm

[tyoshino (SeeGerritForStatus), 2014-09-11 07:20:09]

The CQ bit was checked by tyoshino@chromium.org

[commit-bot: I haz the power, 2014-09-11 07:20:57]

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

[commit-bot: I haz the power, 2014-09-11 08:27:22]

Committed patchset #7 (id:120001) as 181802