Implement stream based cross file system copy.

Implement stream based cross file system copy.

This CL implements the FileStreamReader/Writer based cross file system copy
operation.

BUG=279278
TEST=Ran content_unittests and unit_tests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=225378

[hidehiko, 2013-09-20 13:16:41]

Thank you for your review in advance,
- hidehiko

[kinuko, 2013-09-21 10:44:35]

Looking good.

https://codereview.chromium.org/23463048/diff/1/webkit/browser/fileapi/copy_o...
File webkit/browser/fileapi/copy_or_move_operation_delegate.cc (right):

https://codereview.chromium.org/23463048/diff/1/webkit/browser/fileapi/copy_o...
webkit/browser/fileapi/copy_or_move_operation_delegate.cc:322: // Reader can be
created even if the entry is not exists or the entry is
nit: is not exists -> does not exist

https://codereview.chromium.org/23463048/diff/1/webkit/browser/fileapi/copy_o...
webkit/browser/fileapi/copy_or_move_operation_delegate.cc:418:
base::PlatformFileError NetErrorToPlatformFileError(int error) {
Hmm.. could this be moved to webkit/common/fileapi/file_system_util.h and
deprecate the other similar private routine in
webkit/browser/fileapi/file_writer_delegate.cc?

https://codereview.chromium.org/23463048/diff/1/webkit/browser/fileapi/copy_o...
File webkit/browser/fileapi/copy_or_move_operation_delegate_unittest.cc (right):

https://codereview.chromium.org/23463048/diff/1/webkit/browser/fileapi/copy_o...
webkit/browser/fileapi/copy_or_move_operation_delegate_unittest.cc:5: #include
"webkit/browser/fileapi/copy_or_move_operation_delegate.h"
afaik having this header at the top of unittest file is not mandatory nor very
common in chromium.

https://codereview.chromium.org/23463048/diff/1/webkit/browser/fileapi/copy_o...
webkit/browser/fileapi/copy_or_move_operation_delegate_unittest.cc:144:
base::RunLoop().RunUntilIdle();
Should we post one more task to the thread/runner and wait until it finishes to
make sure?

https://codereview.chromium.org/23463048/diff/1/webkit/browser/fileapi/copy_o...
webkit/browser/fileapi/copy_or_move_operation_delegate_unittest.cc:710: //
LocalFileWriter requires the file is exists. So create an empty file here.
nit: is exists -> exists

[hidehiko, 2013-09-23 08:18:10]

Thank you for your review. PTAL?

https://codereview.chromium.org/23463048/diff/1/webkit/browser/fileapi/copy_o...
File webkit/browser/fileapi/copy_or_move_operation_delegate.cc (right):

https://codereview.chromium.org/23463048/diff/1/webkit/browser/fileapi/copy_o...
webkit/browser/fileapi/copy_or_move_operation_delegate.cc:322: // Reader can be
created even if the entry is not exists or the entry is
On 2013/09/21 10:44:36, kinuko wrote:
> nit: is not exists -> does not exist

Oops Done.

https://codereview.chromium.org/23463048/diff/1/webkit/browser/fileapi/copy_o...
webkit/browser/fileapi/copy_or_move_operation_delegate.cc:418:
base::PlatformFileError NetErrorToPlatformFileError(int error) {
On 2013/09/21 10:44:36, kinuko wrote:
> Hmm.. could this be moved to webkit/common/fileapi/file_system_util.h and
> deprecate the other similar private routine in
> webkit/browser/fileapi/file_writer_delegate.cc?

Done.

https://codereview.chromium.org/23463048/diff/1/webkit/browser/fileapi/copy_o...
File webkit/browser/fileapi/copy_or_move_operation_delegate_unittest.cc (right):

https://codereview.chromium.org/23463048/diff/1/webkit/browser/fileapi/copy_o...
webkit/browser/fileapi/copy_or_move_operation_delegate_unittest.cc:5: #include
"webkit/browser/fileapi/copy_or_move_operation_delegate.h"
On 2013/09/21 10:44:36, kinuko wrote:
> afaik having this header at the top of unittest file is not mandatory nor very
> common in chromium.

Ok, Done.
# But about half of unittest files in this directory have this kind of include
directive at top?

https://codereview.chromium.org/23463048/diff/1/webkit/browser/fileapi/copy_o...
webkit/browser/fileapi/copy_or_move_operation_delegate_unittest.cc:144:
base::RunLoop().RunUntilIdle();
On 2013/09/21 10:44:36, kinuko wrote:
> Should we post one more task to the thread/runner and wait until it finishes
to
> make sure?

Done.

https://codereview.chromium.org/23463048/diff/1/webkit/browser/fileapi/copy_o...
webkit/browser/fileapi/copy_or_move_operation_delegate_unittest.cc:710: //
LocalFileWriter requires the file is exists. So create an empty file here.
On 2013/09/21 10:44:36, kinuko wrote:
> nit: is exists -> exists

Done.

[kinuko, 2013-09-23 22:10:20]

lgtm

https://codereview.chromium.org/23463048/diff/1/webkit/browser/fileapi/copy_o...
File webkit/browser/fileapi/copy_or_move_operation_delegate_unittest.cc (right):

https://codereview.chromium.org/23463048/diff/1/webkit/browser/fileapi/copy_o...
webkit/browser/fileapi/copy_or_move_operation_delegate_unittest.cc:5: #include
"webkit/browser/fileapi/copy_or_move_operation_delegate.h"
On 2013/09/23 08:18:10, hidehiko wrote:
> On 2013/09/21 10:44:36, kinuko wrote:
> > afaik having this header at the top of unittest file is not mandatory nor
very
> > common in chromium.
> 
> Ok, Done.
> # But about half of unittest files in this directory have this kind of include
> directive at top?

Yup, there was an era when we were told to do so, but we've changed our custom
at some point (but didn't go through all existing files to fix that as it's
pretty minor issue)

https://codereview.chromium.org/23463048/diff/9001/webkit/browser/fileapi/cop...
File webkit/browser/fileapi/copy_or_move_operation_delegate.cc (right):

https://codereview.chromium.org/23463048/diff/9001/webkit/browser/fileapi/cop...
webkit/browser/fileapi/copy_or_move_operation_delegate.cc:322: // Reader can be
created even if the entry does not exists or the entry is
nit: exists -> exist

https://codereview.chromium.org/23463048/diff/9001/webkit/browser/fileapi/cop...
File webkit/browser/fileapi/copy_or_move_operation_delegate_unittest.cc (right):

https://codereview.chromium.org/23463048/diff/9001/webkit/browser/fileapi/cop...
webkit/browser/fileapi/copy_or_move_operation_delegate_unittest.cc:136:
DCHECK(thread);
nit: ASSERT is preferred over DCHECK in tests (so that assert failure doesn't
force entire tests to halt)

https://codereview.chromium.org/23463048/diff/9001/webkit/common/fileapi/file...
File webkit/common/fileapi/file_system_util.cc (right):

https://codereview.chromium.org/23463048/diff/9001/webkit/common/fileapi/file...
webkit/common/fileapi/file_system_util.cc:400: base::PlatformFileError
NetErrorToPlatformFileError(int error) {
It's really great if you could split this CL.  (You can ask tzik@ to stamp it
quickly while I'm in US)

[hidehiko, 2013-09-24 02:06:14]

Thank you for your review.

https://codereview.chromium.org/23463048/diff/9001/webkit/browser/fileapi/cop...
File webkit/browser/fileapi/copy_or_move_operation_delegate.cc (right):

https://codereview.chromium.org/23463048/diff/9001/webkit/browser/fileapi/cop...
webkit/browser/fileapi/copy_or_move_operation_delegate.cc:322: // Reader can be
created even if the entry does not exists or the entry is
On 2013/09/23 22:10:20, kinuko wrote:
> nit: exists -> exist

Oops... Done.

https://codereview.chromium.org/23463048/diff/9001/webkit/browser/fileapi/cop...
File webkit/browser/fileapi/copy_or_move_operation_delegate_unittest.cc (right):

https://codereview.chromium.org/23463048/diff/9001/webkit/browser/fileapi/cop...
webkit/browser/fileapi/copy_or_move_operation_delegate_unittest.cc:136:
DCHECK(thread);
On 2013/09/23 22:10:20, kinuko wrote:
> nit: ASSERT is preferred over DCHECK in tests (so that assert failure doesn't
> force entire tests to halt)

Done.
Note that ASSERT family doesn't work here, because it is just a macro of return
with recording an error.

https://codereview.chromium.org/23463048/diff/9001/webkit/common/fileapi/file...
File webkit/common/fileapi/file_system_util.cc (right):

https://codereview.chromium.org/23463048/diff/9001/webkit/common/fileapi/file...
webkit/common/fileapi/file_system_util.cc:400: base::PlatformFileError
NetErrorToPlatformFileError(int error) {
On 2013/09/23 22:10:20, kinuko wrote:
> It's really great if you could split this CL.  (You can ask tzik@ to stamp it
> quickly while I'm in US)

Done. crrev.com/24300003
I'll rebase this onto it after the CL is landed.

[commit-bot: I haz the power, 2013-09-25 13:21:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hidehiko@chromium.org/23463048/41001

[commit-bot: I haz the power, 2013-09-25 13:56:16]

Retried try job too often on linux_rel for step(s) remoting_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-09-25 13:56:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hidehiko@chromium.org/23463048/41001

[commit-bot: I haz the power, 2013-09-25 16:29:14]

Retried try job too often on win7_aura for step(s) content_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[hidehiko, 2013-09-25 17:45:12]

On 2013/09/25 16:29:14, I haz the power (commit-bot) wrote:
> Retried try job too often on win7_aura for step(s) content_unittests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

It seems that on windows, even for testing, the StreamReader needs to run on IO
thread.
Fixed it. Kinuko, could you take another look?

- hidehiko

[kinuko, 2013-09-25 18:00:37]

On 2013/09/25 17:45:12, hidehiko wrote:
> On 2013/09/25 16:29:14, I haz the power (commit-bot) wrote:
> > Retried try job too often on win7_aura for step(s) content_unittests
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
> 
> It seems that on windows, even for testing, the StreamReader needs to run on
IO
> thread.
> Fixed it. Kinuko, could you take another look?
> 
> - hidehiko

the last diff lgtm

[commit-bot: I haz the power, 2013-09-25 18:05:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hidehiko@chromium.org/23463048/78001

[hidehiko, 2013-09-25 19:03:38]

On 2013/09/25 18:05:29, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/hidehiko%40chromium.org/23463048/78001

Kinuko, I'm really sorry, but I overlooked some other tests are also failed, and
needed to fix one more line...
PTAL? (I should have Win testing environment...)

- hidehiko

[hidehiko, 2013-09-26 03:28:16]

Talked offline with tzik@, and he kindly told me he'd take a look the test
fixing.

- hidehiko

[tzik, 2013-09-26 03:43:03]

Patchset 5 vs 7 diff LGTM.

[hidehiko, 2013-09-26 03:43:30]

On 2013/09/26 03:43:03, tzik wrote:
> Patchset 5 vs 7 diff LGTM.

Thanks! sending to CQ.
- hidehiko

[commit-bot: I haz the power, 2013-09-26 03:49:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hidehiko@chromium.org/23463048/92001

[commit-bot: I haz the power, 2013-09-26 07:37:44]

Change committed as 225378