Use RunLoop::Run() instead of RunLoop::RunUntilIdle() in FileSystemOperationImplTest.

Use RunLoop::Run() instead of RunLoop::RunUntilIdle() in FileSystemOperationImplTest.

RunLoop::Run() is needed if any of the operation is implemented using
asynchronous I/O in OS layer, or performed in a separate thread.
Currently, only Copy needs RunLoop::Run().

This CL replace all RunUntilIdle() with Run() to keep all test consistent.

BUG=411153
TEST=content_unittest

Committed: https://crrev.com/9d84bde186bd2c504311257bcee92212a64cd57c
Cr-Commit-Position: refs/heads/master@{#294365}

[iseki, 2014-09-10 07:52:29]

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

[iseki, 2014-09-10 07:52:30]

Please take a look.

[tzik, 2014-09-10 08:45:14]

https://codereview.chromium.org/557273002/diff/1/content/browser/fileapi/file...
File content/browser/fileapi/file_system_operation_impl_unittest.cc (right):

https://codereview.chromium.org/557273002/diff/1/content/browser/fileapi/file...
content/browser/fileapi/file_system_operation_impl_unittest.cc:183:
RecordReadDirectoryCallback(base::Closure closure) {
const ref, here and elsewhere?

https://codereview.chromium.org/557273002/diff/1/content/browser/fileapi/file...
content/browser/fileapi/file_system_operation_impl_unittest.cc:258:
RemoveTest(url, false /* recursive */,
Can this be pushed into one line?

https://codereview.chromium.org/557273002/diff/1/content/browser/fileapi/file...
content/browser/fileapi/file_system_operation_impl_unittest.cc:290: void
MoveTest(
Could you change the return type from void to base::File::Error and check the
expectation outside of the function?
That will make failure log more readable.

https://codereview.chromium.org/557273002/diff/1/content/browser/fileapi/file...
content/browser/fileapi/file_system_operation_impl_unittest.cc:291: const
FileSystemURL& src,
Can you push these param to previous line, here and elsewhere?

[iseki, 2014-09-11 01:07:33]

Thank you for your review.

https://codereview.chromium.org/557273002/diff/1/content/browser/fileapi/file...
File content/browser/fileapi/file_system_operation_impl_unittest.cc (right):

https://codereview.chromium.org/557273002/diff/1/content/browser/fileapi/file...
content/browser/fileapi/file_system_operation_impl_unittest.cc:183:
RecordReadDirectoryCallback(base::Closure closure) {
On 2014/09/10 08:45:14, tzik wrote:
> const ref, here and elsewhere?

Done.

https://codereview.chromium.org/557273002/diff/1/content/browser/fileapi/file...
content/browser/fileapi/file_system_operation_impl_unittest.cc:258:
RemoveTest(url, false /* recursive */,
On 2014/09/10 08:45:14, tzik wrote:
> Can this be pushed into one line?

Done.

https://codereview.chromium.org/557273002/diff/1/content/browser/fileapi/file...
content/browser/fileapi/file_system_operation_impl_unittest.cc:290: void
MoveTest(
On 2014/09/10 08:45:14, tzik wrote:
> Could you change the return type from void to base::File::Error and check the
> expectation outside of the function?
> That will make failure log more readable.

Done.

https://codereview.chromium.org/557273002/diff/1/content/browser/fileapi/file...
content/browser/fileapi/file_system_operation_impl_unittest.cc:291: const
FileSystemURL& src,
On 2014/09/10 08:45:14, tzik wrote:
> Can you push these param to previous line, here and elsewhere?

Done.

[tzik, 2014-09-11 03:30:09]

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

https://codereview.chromium.org/557273002/diff/20001/content/browser/fileapi/...
content/browser/fileapi/file_system_operation_impl_unittest.cc:170: const
base::Closure& closure) {
These *Callback() functions seems simple enough.
Could you inline them into the caller?

https://codereview.chromium.org/557273002/diff/20001/content/browser/fileapi/...
content/browser/fileapi/file_system_operation_impl_unittest.cc:254:
EXPECT_EQ(RemoveTest(url, false /* recursive */), base::File::FILE_OK);
For better macro output, please swap RemoveTest() and FILE_OK.
The first parameter should be the expectation.

https://codereview.chromium.org/557273002/diff/20001/content/browser/fileapi/...
content/browser/fileapi/file_system_operation_impl_unittest.cc:285: int
MoveTest(const FileSystemURL& src,
s/int/base::File::Error/?
Could you remove "Test" for these function names?

https://codereview.chromium.org/557273002/diff/20001/content/browser/fileapi/...
content/browser/fileapi/file_system_operation_impl_unittest.cc:290: src, dest,
option, RecordStatusCallback(run_loop.QuitClosure()));
How about:
 - Define a local variable for |status|,
 - Pass it to the callback,
 - And, set the result to |status| in the callback.

[iseki, 2014-09-11 06:03:13]

Thank you for your reveiw:)

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

https://codereview.chromium.org/557273002/diff/20001/content/browser/fileapi/...
content/browser/fileapi/file_system_operation_impl_unittest.cc:170: const
base::Closure& closure) {
On 2014/09/11 03:30:09, tzik wrote:
> These *Callback() functions seems simple enough.
> Could you inline them into the caller?

Done.

https://codereview.chromium.org/557273002/diff/20001/content/browser/fileapi/...
content/browser/fileapi/file_system_operation_impl_unittest.cc:254:
EXPECT_EQ(RemoveTest(url, false /* recursive */), base::File::FILE_OK);
On 2014/09/11 03:30:09, tzik wrote:
> For better macro output, please swap RemoveTest() and FILE_OK.
> The first parameter should be the expectation.

Done.

https://codereview.chromium.org/557273002/diff/20001/content/browser/fileapi/...
content/browser/fileapi/file_system_operation_impl_unittest.cc:285: int
MoveTest(const FileSystemURL& src,
On 2014/09/11 03:30:09, tzik wrote:
> s/int/base::File::Error/?
> Could you remove "Test" for these function names?

Done.

https://codereview.chromium.org/557273002/diff/20001/content/browser/fileapi/...
content/browser/fileapi/file_system_operation_impl_unittest.cc:290: src, dest,
option, RecordStatusCallback(run_loop.QuitClosure()));
On 2014/09/11 03:30:09, tzik wrote:
> How about:
>  - Define a local variable for |status|,
>  - Pass it to the callback,
>  - And, set the result to |status| in the callback.

Done.

[tzik, 2014-09-11 06:19:54]

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

https://codereview.chromium.org/557273002/diff/40001/content/browser/fileapi/...
content/browser/fileapi/file_system_operation_impl_unittest.cc:258:
EXPECT_EQ(Remove(url, false /* recursive */), base::File::FILE_OK);
Please put the expectation first.

https://codereview.chromium.org/557273002/diff/40001/content/browser/fileapi/...
content/browser/fileapi/file_system_operation_impl_unittest.cc:290: const
FileSystemURL& src,
could you pack this to the previous line?

https://codereview.chromium.org/557273002/diff/40001/content/browser/fileapi/...
content/browser/fileapi/file_system_operation_impl_unittest.cc:305: const
FileSystemURL& src,
ditto

https://codereview.chromium.org/557273002/diff/40001/content/browser/fileapi/...
content/browser/fileapi/file_system_operation_impl_unittest.cc:321: const
FileSystemURL& dest) {
indent?

https://codereview.chromium.org/557273002/diff/40001/content/browser/fileapi/...
content/browser/fileapi/file_system_operation_impl_unittest.cc:358: bool
exclusive,
indent?

https://codereview.chromium.org/557273002/diff/40001/content/browser/fileapi/...
content/browser/fileapi/file_system_operation_impl_unittest.cc:417: const
base::Time& last_access_time,
indent?

https://codereview.chromium.org/557273002/diff/40001/content/browser/fileapi/...
content/browser/fileapi/file_system_operation_impl_unittest.cc:459:
URLForPath("a"), URLForPath("b"), FileSystemOperation::OPTION_NONE));
Just for my preference:
Move(URLForPath("a"), URLForPath("b"),
     FileSystemOperation::OPTION_NONE));?

https://codereview.chromium.org/557273002/diff/40001/content/browser/fileapi/...
content/browser/fileapi/file_system_operation_impl_unittest.cc:511:
URLForPath("nonexistent/deset"),
indent?

[iseki, 2014-09-11 06:36:55]

Thank you for your review.

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

https://codereview.chromium.org/557273002/diff/40001/content/browser/fileapi/...
content/browser/fileapi/file_system_operation_impl_unittest.cc:258:
EXPECT_EQ(Remove(url, false /* recursive */), base::File::FILE_OK);
On 2014/09/11 06:19:53, tzik wrote:
> Please put the expectation first.

Done.

https://codereview.chromium.org/557273002/diff/40001/content/browser/fileapi/...
content/browser/fileapi/file_system_operation_impl_unittest.cc:290: const
FileSystemURL& src,
On 2014/09/11 06:19:54, tzik wrote:
> could you pack this to the previous line?

If pack this to the previous line, option is over the character limitation in
one line.

https://codereview.chromium.org/557273002/diff/40001/content/browser/fileapi/...
content/browser/fileapi/file_system_operation_impl_unittest.cc:305: const
FileSystemURL& src,
On 2014/09/11 06:19:54, tzik wrote:
> ditto

The same reason.

https://codereview.chromium.org/557273002/diff/40001/content/browser/fileapi/...
content/browser/fileapi/file_system_operation_impl_unittest.cc:321: const
FileSystemURL& dest) {
On 2014/09/11 06:19:54, tzik wrote:
> indent?

Done.

https://codereview.chromium.org/557273002/diff/40001/content/browser/fileapi/...
content/browser/fileapi/file_system_operation_impl_unittest.cc:358: bool
exclusive,
On 2014/09/11 06:19:53, tzik wrote:
> indent?

Done.

https://codereview.chromium.org/557273002/diff/40001/content/browser/fileapi/...
content/browser/fileapi/file_system_operation_impl_unittest.cc:417: const
base::Time& last_access_time,
On 2014/09/11 06:19:54, tzik wrote:
> indent?

Done.

https://codereview.chromium.org/557273002/diff/40001/content/browser/fileapi/...
content/browser/fileapi/file_system_operation_impl_unittest.cc:459:
URLForPath("a"), URLForPath("b"), FileSystemOperation::OPTION_NONE));
On 2014/09/11 06:19:54, tzik wrote:
> Just for my preference:
> Move(URLForPath("a"), URLForPath("b"),
>      FileSystemOperation::OPTION_NONE));?

Done.

https://codereview.chromium.org/557273002/diff/40001/content/browser/fileapi/...
content/browser/fileapi/file_system_operation_impl_unittest.cc:511:
URLForPath("nonexistent/deset"),
On 2014/09/11 06:19:54, tzik wrote:
> indent?

Done.

[tzik, 2014-09-11 07:15:57]

LGTM.

Could you update the subject to specify affected area by adding "in
FileSystemOperationImplTest"?

Also, could you refine the description of the issue since the issue is not
windows-specific? Like:
"RunLoop::Run() is needed if any of the operation is implemented using
asynchronous I/O in OS layer, or performed in a separate thread.
Currently, only Copy needs RunLoop::Run().

This CL replace all RunUntilIdle() with Run() to keep all test consistent."

[iseki, 2014-09-11 07:19:46]

Thanks :)
I update the subject and description.

[iseki, 2014-09-11 07:19:52]

The CQ bit was checked by iseki@chromium.org

[commit-bot: I haz the power, 2014-09-11 07:22:50]

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

[tzik, 2014-09-11 08:12:40]

On 2014/09/11 07:19:46, iseki wrote:
> Thanks :)
> I update the subject and description.

The first line of the description is also used as the title of the commit.
Could you keep that to "Use RunLoop::Run() instead of RunLoop::RunUntilIdle() in
FileSystemOperationImplTest."

[iseki, 2014-09-11 08:14:44]

On 2014/09/11 08:12:40, tzik wrote:
> On 2014/09/11 07:19:46, iseki wrote:
> > Thanks :)
> > I update the subject and description.
> 
> The first line of the description is also used as the title of the commit.
> Could you keep that to "Use RunLoop::Run() instead of RunLoop::RunUntilIdle()
in
> FileSystemOperationImplTest."

Thank you for your information :)

[commit-bot: I haz the power, 2014-09-11 09:23:28]

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

[commit-bot: I haz the power, 2014-09-11 09:23:29]

Exceeded time limit waiting for builds to trigger.

[iseki, 2014-09-11 09:25:03]

The CQ bit was checked by iseki@chromium.org

[commit-bot: I haz the power, 2014-09-11 09:25:21]

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

[commit-bot: I haz the power, 2014-09-11 09:37:45]

Committed patchset #4 (id:60001) as 174e16c7a945660217d5df4859a154e9333e7def

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

Patchset 4 (id:??) landed as
https://crrev.com/9d84bde186bd2c504311257bcee92212a64cd57c
Cr-Commit-Position: refs/heads/master@{#294365}