Remove CloseFile from FileSystem.

Remove CloseFile from FileSystem.

CloseFile has a potential issue, which may close a file different from opened
one. To avoid such a case, by this CL, OpenFile starts to return a closure,
which should be called on the file closing.
This is the same technique used in fileapi implementation.

BUG=259184
TEST=Ran unit_tests and tested manually.

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

[hidehiko, 2013-07-23 07:26:40]

Thank you for your review in advance,

# Sorry for a large CL, but I couldn't come up with a good idea to split this
into smaller pieces. Please let me know if you have a good idea.

- hidehiko

[kinaba, 2013-07-23 10:58:52]

https://codereview.chromium.org/19596003/diff/1/chrome/browser/chromeos/drive...
File chrome/browser/chromeos/drive/file_system/open_file_operation.cc (right):

https://codereview.chromium.org/19596003/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/file_system/open_file_operation.cc:164:
weak_ptr_factory_.GetWeakPtr(), resource_id));
I think the callback in the case error != FILE_ERROR_OK should be a null
callback.

https://codereview.chromium.org/19596003/diff/1/chrome/browser/chromeos/drive...
File chrome/browser/chromeos/drive/file_system/open_file_operation.h (right):

https://codereview.chromium.org/19596003/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/file_system/open_file_operation.h:86: // The map
from resource id for an opened file to the number how many the
nit: "...how many <times> the file is..."?

[hashimoto, 2013-07-23 11:24:58]

nit: Please remove the "WIP" line from the change description.

https://codereview.chromium.org/19596003/diff/1/chrome/browser/chromeos/drive...
File chrome/browser/chromeos/drive/file_system/open_file_operation_unittest.cc
(right):

https://codereview.chromium.org/19596003/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/file_system/open_file_operation_unittest.cc:55:
EXPECT_EQ(file_size, local_file_size);
How about having EXPECT_FALSE(on_close_callback.is_null()) for all appropriate
cases?
Optionally, actually calling the callback and check the state of
observer()->upload_needed_resource_ids().

https://codereview.chromium.org/19596003/diff/1/chrome/browser/chromeos/drive...
File chrome/browser/chromeos/drive/file_system_interface.h (right):

https://codereview.chromium.org/19596003/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/file_system_interface.h:205: // pick up the file
for uploading.
nit: This sentence should better be moved near the typedef of OpenFileCallback?
It's not obvious what |on_close_callback| is.
Also, it might be worth noting that the callback must not be called more than
once.

https://codereview.chromium.org/19596003/diff/1/chrome/browser/chromeos/drive...
File chrome/browser/chromeos/drive/file_system_unittest.cc (right):

https://codereview.chromium.org/19596003/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/file_system_unittest.cc:784: // Close kFileInRoot
("drive/root/File 1.txt").
nit: How about ASSERT_FALSE(on_close_callback.is_null())?

https://codereview.chromium.org/19596003/diff/1/chrome/browser/chromeos/drive...
File chrome/browser/chromeos/drive/webkit_file_stream_writer_impl.cc (right):

https://codereview.chromium.org/19596003/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/webkit_file_stream_writer_impl.cc:62:
local_file_writer_.reset();
nit: How about DCHECK(on_close_file_callback_on_ui_thread_)?

[hidehiko, 2013-07-23 14:32:24]

Thank you for your review. PTAL?

Done to remove WIP.

https://codereview.chromium.org/19596003/diff/1/chrome/browser/chromeos/drive...
File chrome/browser/chromeos/drive/file_system/open_file_operation.cc (right):

https://codereview.chromium.org/19596003/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/file_system/open_file_operation.cc:164:
weak_ptr_factory_.GetWeakPtr(), resource_id));
On 2013/07/23 10:58:53, kinaba wrote:
> I think the callback in the case error != FILE_ERROR_OK should be a null
> callback.

Done.

https://codereview.chromium.org/19596003/diff/1/chrome/browser/chromeos/drive...
File chrome/browser/chromeos/drive/file_system/open_file_operation.h (right):

https://codereview.chromium.org/19596003/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/file_system/open_file_operation.h:86: // The map
from resource id for an opened file to the number how many the
On 2013/07/23 10:58:53, kinaba wrote:
> nit: "...how many <times> the file is..."?

Done.

https://codereview.chromium.org/19596003/diff/1/chrome/browser/chromeos/drive...
File chrome/browser/chromeos/drive/file_system/open_file_operation_unittest.cc
(right):

https://codereview.chromium.org/19596003/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/file_system/open_file_operation_unittest.cc:55:
EXPECT_EQ(file_size, local_file_size);
On 2013/07/23 11:24:58, hashimoto wrote:
> How about having EXPECT_FALSE(on_close_callback.is_null()) for all appropriate
> cases?
> Optionally, actually calling the callback and check the state of
> observer()->upload_needed_resource_ids().

Done.

https://codereview.chromium.org/19596003/diff/1/chrome/browser/chromeos/drive...
File chrome/browser/chromeos/drive/file_system_interface.h (right):

https://codereview.chromium.org/19596003/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/file_system_interface.h:205: // pick up the file
for uploading.
On 2013/07/23 11:24:58, hashimoto wrote:
> nit: This sentence should better be moved near the typedef of
OpenFileCallback?
> It's not obvious what |on_close_callback| is.
> Also, it might be worth noting that the callback must not be called more than
> once.

Done.

https://codereview.chromium.org/19596003/diff/1/chrome/browser/chromeos/drive...
File chrome/browser/chromeos/drive/file_system_unittest.cc (right):

https://codereview.chromium.org/19596003/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/file_system_unittest.cc:784: // Close kFileInRoot
("drive/root/File 1.txt").
On 2013/07/23 11:24:58, hashimoto wrote:
> nit: How about ASSERT_FALSE(on_close_callback.is_null())?

Done.

https://codereview.chromium.org/19596003/diff/1/chrome/browser/chromeos/drive...
File chrome/browser/chromeos/drive/webkit_file_stream_writer_impl.cc (right):

https://codereview.chromium.org/19596003/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/webkit_file_stream_writer_impl.cc:62:
local_file_writer_.reset();
On 2013/07/23 11:24:58, hashimoto wrote:
> nit: How about DCHECK(on_close_file_callback_on_ui_thread_)?

Done.

[kinaba, 2013-07-23 22:46:25]

lgtm

[hashimoto, 2013-07-24 03:43:56]

lgtm with a nit

https://codereview.chromium.org/19596003/diff/10001/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/file_system_interface.h (right):

https://codereview.chromium.org/19596003/diff/10001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/file_system_interface.h:107: const base::Closure&
on_close_callback)>
nit: Why is this callback called "on close callback"?  I feel "close callback"
sounds better because this callback is not called after the file gets closed,
but when the user code wants to close the file.

[hidehiko, 2013-07-24 04:49:45]

Thank you for your review. Sending to CQ.

https://codereview.chromium.org/19596003/diff/10001/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/file_system_interface.h (right):

https://codereview.chromium.org/19596003/diff/10001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/file_system_interface.h:107: const base::Closure&
on_close_callback)>
On 2013/07/24 03:43:57, hashimoto wrote:
> nit: Why is this callback called "on close callback"?  I feel "close callback"
> sounds better because this callback is not called after the file gets closed,
> but when the user code wants to close the file.

Done.

[commit-bot: I haz the power, 2013-07-24 04:50:11]

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

[commit-bot: I haz the power, 2013-07-24 07:29:34]

Change committed as 213366