Modify test case to support streaming operation in sandbox file system.

Modify test case to support streaming operation in sandbox file system.

BUG=360088
TEST=content_unittests passed

Committed: https://crrev.com/0df386658195ba87c94db713c06e91bba7b1fe9c
Cr-Commit-Position: refs/heads/master@{#293327}

[iseki, 2014-08-29 02:23:07]

iseki@chromium.org changed reviewers:
+ tzik@chromium.org

[iseki, 2014-08-29 02:23:07]

This cl is related to https://codereview.chromium.org/491813002/.

Sorry, please take a look again.

[iseki, 2014-09-02 02:11:47]

tzik@

I use RunLoop::Run and pass the test.
Please take a look.

[tzik, 2014-09-02 03:25:03]

On 2014/09/02 02:11:47, iseki wrote:
> tzik@
> 
> I use RunLoop::Run and pass the test.
> Please take a look.

I think we need to replace all RunUntilIdle with Run for Copy operation, since
any of them yields empty message loop on the task.

[tzik, 2014-09-02 03:25:11]

https://codereview.chromium.org/522543002/diff/140001/content/browser/fileapi...
File content/browser/fileapi/file_system_operation_impl_unittest.cc (left):

https://codereview.chromium.org/522543002/diff/140001/content/browser/fileapi...
content/browser/fileapi/file_system_operation_impl_unittest.cc:609: EXPECT_EQ(1,
change_observer()->get_and_reset_modify_file_count());
We should get notified for the file modification.

https://codereview.chromium.org/522543002/diff/140001/content/browser/fileapi...
File content/browser/fileapi/file_system_operation_impl_unittest.cc (right):

https://codereview.chromium.org/522543002/diff/140001/content/browser/fileapi...
content/browser/fileapi/file_system_operation_impl_unittest.cc:567: EXPECT_EQ(1,
change_observer()->get_and_reset_remove_directory_count());
This looks strange to me. Why was a directory created and removed?
Does "dest" exists at this point?

https://codereview.chromium.org/522543002/diff/140001/content/browser/fileapi...
content/browser/fileapi/file_system_operation_impl_unittest.cc:1291:
base::RunLoop run_loop_for_dest2;
How about making a block scope for these RunLoops?

[iseki, 2014-09-03 09:34:16]

Thank you for your review.

https://codereview.chromium.org/522543002/diff/140001/content/browser/fileapi...
File content/browser/fileapi/file_system_operation_impl_unittest.cc (left):

https://codereview.chromium.org/522543002/diff/140001/content/browser/fileapi...
content/browser/fileapi/file_system_operation_impl_unittest.cc:609: EXPECT_EQ(1,
change_observer()->get_and_reset_modify_file_count());
On 2014/09/02 03:25:10, tzik wrote:
> We should get notified for the file modification.

Done.

https://codereview.chromium.org/522543002/diff/140001/content/browser/fileapi...
File content/browser/fileapi/file_system_operation_impl_unittest.cc (right):

https://codereview.chromium.org/522543002/diff/140001/content/browser/fileapi...
content/browser/fileapi/file_system_operation_impl_unittest.cc:567: EXPECT_EQ(1,
change_observer()->get_and_reset_remove_directory_count());
On 2014/09/02 03:25:11, tzik wrote:
> This looks strange to me. Why was a directory created and removed?
> Does "dest" exists at this point?

Done.

https://codereview.chromium.org/522543002/diff/140001/content/browser/fileapi...
content/browser/fileapi/file_system_operation_impl_unittest.cc:1291:
base::RunLoop run_loop_for_dest2;
On 2014/09/02 03:25:10, tzik wrote:
> How about making a block scope for these RunLoops?

Done.

[tzik, 2014-09-04 03:32:13]

https://codereview.chromium.org/522543002/diff/200001/webkit/browser/fileapi/...
File webkit/browser/fileapi/copy_or_move_operation_delegate.cc (right):

https://codereview.chromium.org/522543002/diff/200001/webkit/browser/fileapi/...
webkit/browser/fileapi/copy_or_move_operation_delegate.cc:503:
NotifyOnStartUpdate(src_url_);
Could you remove this since the src is not updated for copy operation.

https://codereview.chromium.org/522543002/diff/200001/webkit/browser/fileapi/...
webkit/browser/fileapi/copy_or_move_operation_delegate.cc:524:
NotifyOnEndUpdate(src_url_);
ditto

https://codereview.chromium.org/522543002/diff/200001/webkit/browser/fileapi/...
webkit/browser/fileapi/copy_or_move_operation_delegate.cc:579:
FileSystemContext* file_system_context_;
scoped_refptr?

[tzik, 2014-09-04 04:05:44]

https://codereview.chromium.org/522543002/diff/200001/webkit/browser/fileapi/...
File webkit/browser/fileapi/copy_or_move_operation_delegate.cc (right):

https://codereview.chromium.org/522543002/diff/200001/webkit/browser/fileapi/...
webkit/browser/fileapi/copy_or_move_operation_delegate.cc:464: // This
conversion it to avoid redundant creation and deletion.
"is to return the consistent status code to FileSystemFileUtil::Copy"?

[iseki, 2014-09-04 04:12:17]

Thank you for your review :)

https://codereview.chromium.org/522543002/diff/200001/webkit/browser/fileapi/...
File webkit/browser/fileapi/copy_or_move_operation_delegate.cc (right):

https://codereview.chromium.org/522543002/diff/200001/webkit/browser/fileapi/...
webkit/browser/fileapi/copy_or_move_operation_delegate.cc:464: // This
conversion it to avoid redundant creation and deletion.
On 2014/09/04 04:05:43, tzik wrote:
> "is to return the consistent status code to FileSystemFileUtil::Copy"?

Yes!

https://codereview.chromium.org/522543002/diff/200001/webkit/browser/fileapi/...
webkit/browser/fileapi/copy_or_move_operation_delegate.cc:503:
NotifyOnStartUpdate(src_url_);
On 2014/09/04 03:32:12, tzik wrote:
> Could you remove this since the src is not updated for copy operation.

Done.

https://codereview.chromium.org/522543002/diff/200001/webkit/browser/fileapi/...
webkit/browser/fileapi/copy_or_move_operation_delegate.cc:524:
NotifyOnEndUpdate(src_url_);
On 2014/09/04 03:32:12, tzik wrote:
> ditto

Done.

https://codereview.chromium.org/522543002/diff/200001/webkit/browser/fileapi/...
webkit/browser/fileapi/copy_or_move_operation_delegate.cc:579:
FileSystemContext* file_system_context_;
On 2014/09/04 03:32:12, tzik wrote:
> scoped_refptr?

Done.

[iseki, 2014-09-04 06:00:08]

@tzik
This patch pass the test.

[tzik, 2014-09-04 06:05:23]

https://codereview.chromium.org/522543002/diff/220001/webkit/browser/fileapi/...
File webkit/browser/fileapi/copy_or_move_operation_delegate.cc (right):

https://codereview.chromium.org/522543002/diff/220001/webkit/browser/fileapi/...
webkit/browser/fileapi/copy_or_move_operation_delegate.cc:464: // This
conversion it to avoid redundant creation and deletion and
s/it/is/?
Plus, the extra creation/deletion is the bug that is fixed by this CL.
So that part of comment doesn't help the reader to understand the behavior.

Could you remove "to avoid" part?

[iseki, 2014-09-04 06:09:09]

Thanks :)

https://codereview.chromium.org/522543002/diff/220001/webkit/browser/fileapi/...
File webkit/browser/fileapi/copy_or_move_operation_delegate.cc (right):

https://codereview.chromium.org/522543002/diff/220001/webkit/browser/fileapi/...
webkit/browser/fileapi/copy_or_move_operation_delegate.cc:464: // This
conversion it to avoid redundant creation and deletion and
On 2014/09/04 06:05:23, tzik wrote:
> s/it/is/?
> Plus, the extra creation/deletion is the bug that is fixed by this CL.
> So that part of comment doesn't help the reader to understand the behavior.
> 
> Could you remove "to avoid" part?

Done.

[tzik, 2014-09-04 06:09:34]

LGTM!

[iseki, 2014-09-04 06:09:52]

The CQ bit was checked by iseki@chromium.org

[commit-bot: I haz the power, 2014-09-04 06:11:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/iseki@chromium.org/522543002/240001

[iseki, 2014-09-04 11:38:49]

The CQ bit was checked by iseki@chromium.org

[commit-bot: I haz the power, 2014-09-04 11:39:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/iseki@chromium.org/522543002/260001

[commit-bot: I haz the power, 2014-09-04 19:57:22]

Committed patchset #14 (id:260001) as 6785b53bcbd5e9b94c7cc8138562ae75d856b3fe

[commit-bot: I haz the power, 2014-09-10 03:32:22]

Patchset 14 (id:??) landed as
https://crrev.com/0df386658195ba87c94db713c06e91bba7b1fe9c
Cr-Commit-Position: refs/heads/master@{#293327}