Add unknown file size handling in ComplexFormDataBytesConsumer

Add unknown file size handling in ComplexFormDataBytesConsumer

...to read data from a form data containing a file chosen via the file picker.

BUG=688100
Review-Url: https://codereview.chromium.org/2710033003
Cr-Commit-Position: refs/heads/master@{#459043}
Committed: https://chromium.googlesource.com/chromium/src/+/3dca0cfff95b9b9d8806184490be8b87f76af378

[yhirano, 2017-02-23 12:32:18]

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

[commit-bot: I haz the power, 2017-02-23 12:32:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-23 14:13:18]

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

[commit-bot: I haz the power, 2017-02-23 14:13:18]

Dry run: This issue passed the CQ dry run.

[yhirano, 2017-02-24 03:30:33]

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

[commit-bot: I haz the power, 2017-02-24 03:31:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2017-02-24 03:34:49]

yhirano@chromium.org changed reviewers:
+ dmurph@chromium.org, hiroshige@chromium.org, kinuko@chromium.org

[yhirano, 2017-02-24 03:34:49]

PTAL

+hiroshige@ for the overall change
+dmurph@, kinuko@ for BlobDataHandle creation in
ComplexFormDataBytesConsumer::beginRead.

[commit-bot: I haz the power, 2017-02-24 04:55:21]

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

[commit-bot: I haz the power, 2017-02-24 04:55:22]

Dry run: This issue passed the CQ dry run.

[kinuko, 2017-02-24 07:04:39]

On 2017/02/24 03:34:49, yhirano wrote:
> PTAL
> 
> +hiroshige@ for the overall change
> +dmurph@, kinuko@ for BlobDataHandle creation in
> ComplexFormDataBytesConsumer::beginRead.

lgtm

[dmurph, 2017-02-24 19:49:24]

https://codereview.chromium.org/2710033003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/fetch/FormDataBytesConsumer.cpp (left):

https://codereview.chromium.org/2710033003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FormDataBytesConsumer.cpp:123:
element.m_fileLength,
Why not just check if the file size is -1, and if so, call blink::getFileSize
for the size?

Pro: simpler code
Con: can slow down this operation, but cost would happen anyways if you expect a
'slice' call in the new version.

[dmurph, 2017-02-27 19:21:31]

On 2017/02/24 19:49:24, dmurph wrote:
>
https://codereview.chromium.org/2710033003/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/fetch/FormDataBytesConsumer.cpp (left):
> 
>
https://codereview.chromium.org/2710033003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/fetch/FormDataBytesConsumer.cpp:123:
> element.m_fileLength,
> Why not just check if the file size is -1, and if so, call blink::getFileSize
> for the size?
> 
> Pro: simpler code
> Con: can slow down this operation, but cost would happen anyways if you expect
a
> 'slice' call in the new version.

If you don't like this then that's fine, I just thought it might simplify things
a bit :)

[yhirano, 2017-03-02 03:43:50]

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

[commit-bot: I haz the power, 2017-03-02 03:44:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2017-03-02 04:01:50]

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

[commit-bot: I haz the power, 2017-03-02 04:03:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2017-03-02 04:04:09]

Description was changed from

==========
ComplexFormDataBytesConsumer should read blobs and compose them into one

...instead of reading a blob which contains all elements in the given form.

BUG=688100
==========

to

==========
Add unknown file size handling in ComplexFormDataBytesConsumer

...to read data from a form data containing a file chosen via the file picker.

BUG=688100
==========

[yhirano, 2017-03-02 04:05:04]

Thank you for the review and sorry for the late response. I reverted most of
changes and added getFileSize.

https://codereview.chromium.org/2710033003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/fetch/FormDataBytesConsumer.cpp (left):

https://codereview.chromium.org/2710033003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FormDataBytesConsumer.cpp:123:
element.m_fileLength,
On 2017/02/24 19:49:24, dmurph wrote:
> Why not just check if the file size is -1, and if so, call blink::getFileSize
> for the size?
> 
> Pro: simpler code
> Con: can slow down this operation, but cost would happen anyways if you expect
a
> 'slice' call in the new version.

Fixed.

[commit-bot: I haz the power, 2017-03-02 06:00:49]

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

[commit-bot: I haz the power, 2017-03-02 06:00:50]

Dry run: This issue passed the CQ dry run.

[dmurph, 2017-03-02 18:52:31]

On 2017/03/02 04:05:04, yhirano (slow) wrote:
> Thank you for the review and sorry for the late response. I reverted most of
> changes and added getFileSize.
> 
>
https://codereview.chromium.org/2710033003/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/fetch/FormDataBytesConsumer.cpp (left):
> 
>
https://codereview.chromium.org/2710033003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/fetch/FormDataBytesConsumer.cpp:123:
> element.m_fileLength,
> On 2017/02/24 19:49:24, dmurph wrote:
> > Why not just check if the file size is -1, and if so, call
blink::getFileSize
> > for the size?
> > 
> > Pro: simpler code
> > Con: can slow down this operation, but cost would happen anyways if you
expect
> a
> > 'slice' call in the new version.
> 
> Fixed.

lgtm

[hiroshige, 2017-03-02 20:38:21]

https://codereview.chromium.org/2710033003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/fetch/FormDataBytesConsumer.cpp (right):

https://codereview.chromium.org/2710033003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FormDataBytesConsumer.cpp:144: if
(!getFileSize(element.m_filename, fileLength)) {
m_filename is not valid for encodedFileSystemURL.
(Thus test coverage not sufficient?)

https://codereview.chromium.org/2710033003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FormDataBytesConsumer.cpp:155: }
nit: +2 indent. (Perhaps this is git cl format bug?)

[yhirano, 2017-03-22 12:11:52]

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

[commit-bot: I haz the power, 2017-03-22 12:12:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 13:23:57]

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

[commit-bot: I haz the power, 2017-03-22 13:23:58]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yhirano, 2017-03-23 06:08:54]

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

[commit-bot: I haz the power, 2017-03-23 06:09:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2017-03-23 06:10:56]

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

[commit-bot: I haz the power, 2017-03-23 06:11:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2017-03-23 06:16:33]

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

[commit-bot: I haz the power, 2017-03-23 06:16:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2017-03-23 06:17:37]

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

[commit-bot: I haz the power, 2017-03-23 06:17:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2017-03-23 06:18:06]

https://codereview.chromium.org/2710033003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/fetch/FormDataBytesConsumer.cpp (right):

https://codereview.chromium.org/2710033003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FormDataBytesConsumer.cpp:144: if
(!getFileSize(element.m_filename, fileLength)) {
On 2017/03/02 20:38:21, hiroshige wrote:
> m_filename is not valid for encodedFileSystemURL.
> (Thus test coverage not sufficient?)

Thanks, you're right.
I talked with tzik@, and it seems we cannot test the feature. So I decided not
to support the feature for now.

https://codereview.chromium.org/2710033003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FormDataBytesConsumer.cpp:155: }
On 2017/03/02 20:38:21, hiroshige wrote:
> nit: +2 indent. (Perhaps this is git cl format bug?)

Removed

[commit-bot: I haz the power, 2017-03-23 06:33:17]

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

[commit-bot: I haz the power, 2017-03-23 06:33:18]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[hiroshige, 2017-03-23 09:28:12]

lgtm with an optional comment.

https://codereview.chromium.org/2710033003/diff/160001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/filesystem/form-reading-from-file.html
(right):

https://codereview.chromium.org/2710033003/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/filesystem/form-reading-from-file.html:41:
});
Short description of the test (e.g. testing Response.text() for FormData
containing a file?) might be preferable.

[yhirano, 2017-03-23 09:34:40]

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

[commit-bot: I haz the power, 2017-03-23 09:34:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2017-03-23 09:36:32]

https://codereview.chromium.org/2710033003/diff/160001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/filesystem/form-reading-from-file.html
(right):

https://codereview.chromium.org/2710033003/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/filesystem/form-reading-from-file.html:41:
});
On 2017/03/23 09:28:12, hiroshige wrote:
> Short description of the test (e.g. testing Response.text() for FormData
> containing a file?) might be preferable.

Done.

[yhirano, 2017-03-23 09:36:53]

The CQ bit was unchecked by yhirano@chromium.org

[yhirano, 2017-03-23 09:36:54]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2017-03-23 09:36:54]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
dmurph@chromium.org, hiroshige@chromium.org
Link to the patchset: https://codereview.chromium.org/2710033003/#ps180001
(title: "fix")

[commit-bot: I haz the power, 2017-03-23 09:37:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 11:07:56]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1490261814158760,
"parent_rev": "8ea295e153e8020017bb43081325a3be0e16e56a", "commit_rev":
"3dca0cfff95b9b9d8806184490be8b87f76af378"}

[commit-bot: I haz the power, 2017-03-23 11:08:39]

Description was changed from

==========
Add unknown file size handling in ComplexFormDataBytesConsumer

...to read data from a form data containing a file chosen via the file picker.

BUG=688100
==========

to

==========
Add unknown file size handling in ComplexFormDataBytesConsumer

...to read data from a form data containing a file chosen via the file picker.

BUG=688100

Review-Url: https://codereview.chromium.org/2710033003
Cr-Commit-Position: refs/heads/master@{#459043}
Committed:
https://chromium.googlesource.com/chromium/src/+/3dca0cfff95b9b9d8806184490be...
==========

[commit-bot: I haz the power, 2017-03-23 11:08:40]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/3dca0cfff95b9b9d8806184490be...