Implement DeleteFile and DeleteDirectory.

Implement DeleteFile and DeleteDirectory.

BUG=413541
TEST=manually tested by chaning write_supported in volume_manager.cc to true.

Committed: https://crrev.com/e27c27c27edf0b7f0c898f97a966e77a98a41a6d
Cr-Commit-Position: refs/heads/master@{#319853}

[yawano, 2015-03-06 11:00:11]

yawano@chromium.org changed reviewers:
+ thestig@chromium.org

[yawano, 2015-03-06 11:00:11]

Sorry for a large CL again. PTAL. Thank you!

[yawano, 2015-03-06 11:07:35]

ReadDirectoryEntryIds is added to check the directory is empty. DeleteDirectory
of AsyncFileUtil requires to return FILE_ERROR_NOT_EMPTY if the target directory
is not empty. -
https://code.google.com/p/chromium/codesearch#chromium/src/storage/browser/fi...

While we can return error code from mtpd, I wanted to keep mtpd simple and
separated from the design (requirement) of AsyncFileUtil. I decided to check it
at the chrome side.

[Lei Zhang, 2015-03-07 01:17:24]

https://codereview.chromium.org/982283002/diff/20001/chrome/browser/media_gal...
File chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc
(right):

https://codereview.chromium.org/982283002/diff/20001/chrome/browser/media_gal...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:127: void
ReadDirectoryEntryIdsOnUIThread(
What if we:
- got rid of this
- make ReadDirectoryOnUIThread() take an optional chunk size. (0 = default)
-- plumb that parameter down to MediaTransferProtocolManagerImpl
-- get rid the newly added ReadDirectoryEntryIds code
- Call ReadDirectoryOnUIThread(chunk_size = 1) instead of
ReadDirectoryEntryIdsOnUIThread()

Would that save us some code?

https://codereview.chromium.org/982283002/diff/20001/chrome/browser/media_gal...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:839:
const MTPDeviceTaskHelper::ReadDirectoryEntryIdsSuccessCallback
Before we issue the ReadDirectory call, can we check the cache first? We'll need
to add a HasChildren() method to MTPDeviceDelegateImplLinux::MTPFileNode. If
HasChildren() return true, we know it's not empty, else we stil need to call
ReadDirectory, since the cache could just be empty.

https://codereview.chromium.org/982283002/diff/20001/chrome/browser/media_gal...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:877: void
MTPDeviceDelegateImplLinux::RunDeleteObjectOnUIThread(
There's only one caller for this. I would just put get rid of this function and
put the code in DeleteFileInternal().

https://codereview.chromium.org/982283002/diff/20001/chrome/browser/media_gal...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:905:
EnsureInitAndRunTask(PendingTaskInfo(
You can just call content::BrowserThread::PostTask() directly at this point. The
first argument to PendingTaskInfo() is empty, and initialization must have
already taken place.

https://codereview.chromium.org/982283002/diff/20001/chrome/browser/media_gal...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:1317:
MTPFileNode* parent = it->second->parent();
Assuming we add MTPNode::HasChildren(), I would
DCHECK(!it->second->HasChildren()); here as well.

[yawano, 2015-03-09 03:46:49]

Thank you for the review!

https://codereview.chromium.org/982283002/diff/20001/chrome/browser/media_gal...
File chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc
(right):

https://codereview.chromium.org/982283002/diff/20001/chrome/browser/media_gal...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:127: void
ReadDirectoryEntryIdsOnUIThread(
On 2015/03/07 01:17:24, Lei Zhang wrote:
> What if we:
> - got rid of this
> - make ReadDirectoryOnUIThread() take an optional chunk size. (0 = default)
> -- plumb that parameter down to MediaTransferProtocolManagerImpl
> -- get rid the newly added ReadDirectoryEntryIds code
> - Call ReadDirectoryOnUIThread(chunk_size = 1) instead of
> ReadDirectoryEntryIdsOnUIThread()
> 
> Would that save us some code?

Can I confirm that chunk_size is maximum_length? I think there is no way to
cancel read directory call now.

[yawano, 2015-03-09 04:37:49]

https://codereview.chromium.org/982283002/diff/20001/chrome/browser/media_gal...
File chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc
(right):

https://codereview.chromium.org/982283002/diff/20001/chrome/browser/media_gal...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:839:
const MTPDeviceTaskHelper::ReadDirectoryEntryIdsSuccessCallback
On 2015/03/07 01:17:24, Lei Zhang wrote:
> Before we issue the ReadDirectory call, can we check the cache first? We'll
need
> to add a HasChildren() method to MTPDeviceDelegateImplLinux::MTPFileNode. If
> HasChildren() return true, we know it's not empty, else we stil need to call
> ReadDirectory, since the cache could just be empty.

Done.

https://codereview.chromium.org/982283002/diff/20001/chrome/browser/media_gal...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:877: void
MTPDeviceDelegateImplLinux::RunDeleteObjectOnUIThread(
On 2015/03/07 01:17:24, Lei Zhang wrote:
> There's only one caller for this. I would just put get rid of this function
and
> put the code in DeleteFileInternal().

Done.

https://codereview.chromium.org/982283002/diff/20001/chrome/browser/media_gal...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:905:
EnsureInitAndRunTask(PendingTaskInfo(
On 2015/03/07 01:17:24, Lei Zhang wrote:
> You can just call content::BrowserThread::PostTask() directly at this point.
The
> first argument to PendingTaskInfo() is empty, and initialization must have
> already taken place.

Done.

https://codereview.chromium.org/982283002/diff/20001/chrome/browser/media_gal...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:1317:
MTPFileNode* parent = it->second->parent();
On 2015/03/07 01:17:24, Lei Zhang wrote:
> Assuming we add MTPNode::HasChildren(), I would
> DCHECK(!it->second->HasChildren()); here as well.

Done. Added also for DeleteChild.

[Lei Zhang, 2015-03-10 00:48:06]

https://codereview.chromium.org/982283002/diff/20001/chrome/browser/media_gal...
File chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc
(right):

https://codereview.chromium.org/982283002/diff/20001/chrome/browser/media_gal...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:127: void
ReadDirectoryEntryIdsOnUIThread(
On 2015/03/09 03:46:48, yawano wrote:
> On 2015/03/07 01:17:24, Lei Zhang wrote:
> > What if we:
> > - got rid of this
> > - make ReadDirectoryOnUIThread() take an optional chunk size. (0 = default)
> > -- plumb that parameter down to MediaTransferProtocolManagerImpl
> > -- get rid the newly added ReadDirectoryEntryIds code
> > - Call ReadDirectoryOnUIThread(chunk_size = 1) instead of
> > ReadDirectoryEntryIdsOnUIThread()
> > 
> > Would that save us some code?
> 
> Can I confirm that chunk_size is maximum_length? I think there is no way to
> cancel read directory call now.

Actually, my idea only works if we modify
MediaTransferProtocolManagerImpl::OnGotDirectoryEntries() to read at most 1
entry, not a chunk of 1 entry - as it, it would keep reading more directory
entries. Since we can't cancel, we need some way of telling the read directory
call to end early. WDYT?

https://codereview.chromium.org/982283002/diff/60001/chrome/browser/media_gal...
File chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc
(right):

https://codereview.chromium.org/982283002/diff/60001/chrome/browser/media_gal...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:883:
EnsureInitAndRunTask(PendingTaskInfo(
This can also just be PostTask().

[yawano, 2015-03-10 01:17:04]

https://codereview.chromium.org/982283002/diff/20001/chrome/browser/media_gal...
File chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc
(right):

https://codereview.chromium.org/982283002/diff/20001/chrome/browser/media_gal...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:127: void
ReadDirectoryEntryIdsOnUIThread(
On 2015/03/10 00:48:06, Lei Zhang wrote:
> On 2015/03/09 03:46:48, yawano wrote:
> > On 2015/03/07 01:17:24, Lei Zhang wrote:
> > > What if we:
> > > - got rid of this
> > > - make ReadDirectoryOnUIThread() take an optional chunk size. (0 =
default)
> > > -- plumb that parameter down to MediaTransferProtocolManagerImpl
> > > -- get rid the newly added ReadDirectoryEntryIds code
> > > - Call ReadDirectoryOnUIThread(chunk_size = 1) instead of
> > > ReadDirectoryEntryIdsOnUIThread()
> > > 
> > > Would that save us some code?
> > 
> > Can I confirm that chunk_size is maximum_length? I think there is no way to
> > cancel read directory call now.
> 
> Actually, my idea only works if we modify
> MediaTransferProtocolManagerImpl::OnGotDirectoryEntries() to read at most 1
> entry, not a chunk of 1 entry - as it, it would keep reading more directory
> entries. Since we can't cancel, we need some way of telling the read directory
> call to end early. WDYT?

Yes, I agree with implementing max_size. Adding a way to cancel read directory
call is a big task. I'm going to add max_size to ReadDirectory call, and remove
ReadDirectoryEntryIds.

[yawano, 2015-03-10 05:01:28]

I added max_size to ReadDirectory. PTAL. Thank you!

https://codereview.chromium.org/982283002/diff/60001/chrome/browser/media_gal...
File chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc
(right):

https://codereview.chromium.org/982283002/diff/60001/chrome/browser/media_gal...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:883:
EnsureInitAndRunTask(PendingTaskInfo(
On 2015/03/10 00:48:06, Lei Zhang wrote:
> This can also just be PostTask().

If we use PostTask instead of PendingTaskInfo with empty path
(base::FilePath()), we need to change how we call PendingRequestDone(). The
problem to call PendingRequestDone more often than RunTask is that we allow to
run other operation before a file is actually deleted.

We need to match the number of EnsureInitAndRunTask calls and that of
PendingRequestDone. As far as I see the other codes in this file, it seems to
use PendingTaskInfo with empty path for internal calls.

The current mtpd cannot run operations in parallel, so this should not become a
real problem at now. But to keep the flow simple, I prefer to block the other
operation until a current operation is completed.

Of course, we can use PostTask for internal calls, but it makes
PendingRequestDone far from corresponding EnsureInitAndRunTask. I prefer to keep
them close for readability.

WDYT?

[Lei Zhang, 2015-03-10 08:12:05]

lgtm

https://codereview.chromium.org/982283002/diff/60001/chrome/browser/media_gal...
File chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc
(right):

https://codereview.chromium.org/982283002/diff/60001/chrome/browser/media_gal...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:883:
EnsureInitAndRunTask(PendingTaskInfo(
On 2015/03/10 05:01:28, yawano wrote:
> On 2015/03/10 00:48:06, Lei Zhang wrote:
> > This can also just be PostTask().
> 
> If we use PostTask instead of PendingTaskInfo with empty path
> (base::FilePath()), we need to change how we call PendingRequestDone(). The
> problem to call PendingRequestDone more often than RunTask is that we allow to
> run other operation before a file is actually deleted.
> 
> We need to match the number of EnsureInitAndRunTask calls and that of
> PendingRequestDone. As far as I see the other codes in this file, it seems to
> use PendingTaskInfo with empty path for internal calls.
> 
> The current mtpd cannot run operations in parallel, so this should not become
a
> real problem at now. But to keep the flow simple, I prefer to block the other
> operation until a current operation is completed.
> 
> Of course, we can use PostTask for internal calls, but it makes
> PendingRequestDone far from corresponding EnsureInitAndRunTask. I prefer to
keep
> them close for readability.
> 
> WDYT?

If you were to use PostTask(), then you would not call PendingRequestDone(). The
task that runs on the UI thread would reply, and the callback would either
continue to PostTask() without calling PendingRequestDone(), or finally finish
the overall task at hand (delete in this case) and call PendingRequestDone().

Consistent usage of EnsureInitAndRunTask() and PendingRequestDone() is probably
easier to get correct, so let's go with that.

https://codereview.chromium.org/982283002/diff/100001/device/media_transfer_p...
File device/media_transfer_protocol/media_transfer_protocol_manager.cc (right):

https://codereview.chromium.org/982283002/diff/100001/device/media_transfer_p...
device/media_transfer_protocol/media_transfer_protocol_manager.cc:440:
next_offset + kFileInfoToFetchChunkSize > directory_size
This is just std::min(directory_size - next_offset, kFileInfoToFetchChunkSize);

[yawano, 2015-03-10 08:20:36]

Thank you for the review!

https://codereview.chromium.org/982283002/diff/60001/chrome/browser/media_gal...
File chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc
(right):

https://codereview.chromium.org/982283002/diff/60001/chrome/browser/media_gal...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:883:
EnsureInitAndRunTask(PendingTaskInfo(
On 2015/03/10 08:12:04, Lei Zhang wrote:
> On 2015/03/10 05:01:28, yawano wrote:
> > On 2015/03/10 00:48:06, Lei Zhang wrote:
> > > This can also just be PostTask().
> > 
> > If we use PostTask instead of PendingTaskInfo with empty path
> > (base::FilePath()), we need to change how we call PendingRequestDone(). The
> > problem to call PendingRequestDone more often than RunTask is that we allow
to
> > run other operation before a file is actually deleted.
> > 
> > We need to match the number of EnsureInitAndRunTask calls and that of
> > PendingRequestDone. As far as I see the other codes in this file, it seems
to
> > use PendingTaskInfo with empty path for internal calls.
> > 
> > The current mtpd cannot run operations in parallel, so this should not
become
> a
> > real problem at now. But to keep the flow simple, I prefer to block the
other
> > operation until a current operation is completed.
> > 
> > Of course, we can use PostTask for internal calls, but it makes
> > PendingRequestDone far from corresponding EnsureInitAndRunTask. I prefer to
> keep
> > them close for readability.
> > 
> > WDYT?
> 
> If you were to use PostTask(), then you would not call PendingRequestDone().
The
> task that runs on the UI thread would reply, and the callback would either
> continue to PostTask() without calling PendingRequestDone(), or finally finish
> the overall task at hand (delete in this case) and call PendingRequestDone().
> 
> Consistent usage of EnsureInitAndRunTask() and PendingRequestDone() is
probably
> easier to get correct, so let's go with that.

Acknowledged.

https://codereview.chromium.org/982283002/diff/100001/device/media_transfer_p...
File device/media_transfer_protocol/media_transfer_protocol_manager.cc (right):

https://codereview.chromium.org/982283002/diff/100001/device/media_transfer_p...
device/media_transfer_protocol/media_transfer_protocol_manager.cc:440:
next_offset + kFileInfoToFetchChunkSize > directory_size
On 2015/03/10 08:12:05, Lei Zhang wrote:
> This is just std::min(directory_size - next_offset,
kFileInfoToFetchChunkSize);

Done.

[yawano, 2015-03-10 08:20:41]

The CQ bit was checked by yawano@chromium.org

[yawano, 2015-03-10 08:20:42]

The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/982283002/#ps120001
(title: "Address comments.")

[commit-bot: I haz the power, 2015-03-10 08:20:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/982283002/120001

[commit-bot: I haz the power, 2015-03-10 09:21:53]

Committed patchset #7 (id:120001)

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

Patchset 7 (id:??) landed as
https://crrev.com/e27c27c27edf0b7f0c898f97a966e77a98a41a6d
Cr-Commit-Position: refs/heads/master@{#319853}