Implement CopyFileFromLocal of MTPDeviceAsyncDelegate.

Implement CopyFileFromLocal of MTPDeviceAsyncDelegate.

CopyFileFromLocal is a method to copy a local file to the device. This method will be used in CopyInForeingFile of AsyncFileUtil.
A CL to connect CopyFileFromLocal to CopyInForeingFile will come later when all other write operations (e.g. delete) are implemented.

BUG=413541
TEST=none; manually tested with changing write_supported variable in volume_manager.cc to true.

Committed: https://crrev.com/8cd28e349ab9605da7dbf97c8c85f8c541b3b59b
Cr-Commit-Position: refs/heads/master@{#319205}

[yawano, 2015-02-23 12:43:54]

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

[yawano, 2015-02-23 12:43:55]

PTAL. Thank you!

[Lei Zhang, 2015-02-24 23:35:48]

We need to open the MTP storage in read-write mode, and invalidate the relevant
part of the cache in MTPDeviceDelegateImplLinux when writing. (See |root_node_|
and below.

https://codereview.chromium.org/947943002/diff/40001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/mtp_device_async_delegate.h (right):

https://codereview.chromium.org/947943002/diff/40001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/mtp_device_async_delegate.h:67: typedef
base::Callback<void()> CopyFileFromLocalSuccessCallback;
Isn't base::Callback<void()> just base::Closure, like elsewhere?

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

https://codereview.chromium.org/947943002/diff/40001/chrome/browser/media_gal...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:629:
open(source_file_path.value().c_str(), O_RDONLY);
open() can fail.

https://codereview.chromium.org/947943002/diff/40001/chrome/browser/media_gal...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:640:
base::Closure closure = base::Bind(base::Bind(
I think you copied a typo from elsewhere in the file. This should just be
base::Bind() right?

https://codereview.chromium.org/947943002/diff/40001/chrome/browser/media_gal...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:932:
close(source_file_descriptor);
Wrap this in IGNORE_EINTR()

[yawano, 2015-02-26 07:27:54]

Thank you for the review. I added read only flag to async delegate. Sorry for a
large CL. PTAL.

For invalidating caching, async delegate does not cache content itself. It only
caches paths and object ids. I think there is no need to invalidate cache for
copying a file to a device. WDYT?

Thank you!

Note: This CL fails to compile until a CL to define kReadWriteMode is landed (
https://chromium-review.googlesource.com/#/c/253255/ ).

https://codereview.chromium.org/947943002/diff/40001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/mtp_device_async_delegate.h (right):

https://codereview.chromium.org/947943002/diff/40001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/mtp_device_async_delegate.h:67: typedef
base::Callback<void()> CopyFileFromLocalSuccessCallback;
On 2015/02/24 23:35:48, Lei Zhang wrote:
> Isn't base::Callback<void()> just base::Closure, like elsewhere?

Done.

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

https://codereview.chromium.org/947943002/diff/40001/chrome/browser/media_gal...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:629:
open(source_file_path.value().c_str(), O_RDONLY);
On 2015/02/24 23:35:48, Lei Zhang wrote:
> open() can fail.

Done.

https://codereview.chromium.org/947943002/diff/40001/chrome/browser/media_gal...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:640:
base::Closure closure = base::Bind(base::Bind(
On 2015/02/24 23:35:48, Lei Zhang wrote:
> I think you copied a typo from elsewhere in the file. This should just be
> base::Bind() right?

Done.

https://codereview.chromium.org/947943002/diff/40001/chrome/browser/media_gal...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:932:
close(source_file_descriptor);
On 2015/02/24 23:35:48, Lei Zhang wrote:
> Wrap this in IGNORE_EINTR()

Done.

[Lei Zhang, 2015-02-27 00:22:48]

On 2015/02/26 07:27:54, yawano wrote:
> Thank you for the review. I added read only flag to async delegate. Sorry for
a
> large CL. PTAL.

Sure, but unfortunately the CL failed many trybots and needs a bit more work.

> For invalidating caching, async delegate does not cache content itself. It
only
> caches paths and object ids. I think there is no need to invalidate cache for
> copying a file to a device. WDYT?

I guess I'm thinking of the next CL when we implement deletion. Once we delete a
file, it should no longer be in the cached path to id mapping. Having stale info
in the cache means we will return an invalid id in CachedPathToId().

For file creation, we may need to add it to the cache, otherwise the cache
lookup may fail. It's been almost a year since I wrote the caching code, so I
need to jog my memory to see if this is actually the case.

[yawano, 2015-02-27 00:36:40]

On 2015/02/27 00:22:48, Lei Zhang wrote:
> On 2015/02/26 07:27:54, yawano wrote:
> > Thank you for the review. I added read only flag to async delegate. Sorry
for
> a
> > large CL. PTAL.
> 
> Sure, but unfortunately the CL failed many trybots and needs a bit more work.
> 
> > For invalidating caching, async delegate does not cache content itself. It
> only
> > caches paths and object ids. I think there is no need to invalidate cache
for
> > copying a file to a device. WDYT?
> 
> I guess I'm thinking of the next CL when we implement deletion. Once we delete
a
> file, it should no longer be in the cached path to id mapping. Having stale
info
> in the cache means we will return an invalid id in CachedPathToId().
> 
> For file creation, we may need to add it to the cache, otherwise the cache
> lookup may fail. It's been almost a year since I wrote the caching code, so I
> need to jog my memory to see if this is actually the case.

The reason many try bots fail is that a CL which defines kReadWriteMode is not
landed yet (https://chromium-review.googlesource.com/#/c/253255/). We need to
wait the CL is landed and synced.

[yawano, 2015-02-27 01:11:03]

> > For invalidating caching, async delegate does not cache content itself. It
> only
> > caches paths and object ids. I think there is no need to invalidate cache
for
> > copying a file to a device. WDYT?
> 
> I guess I'm thinking of the next CL when we implement deletion. Once we delete
a
> file, it should no longer be in the cached path to id mapping. Having stale
info
> in the cache means we will return an invalid id in CachedPathToId().
> 
> For file creation, we may need to add it to the cache, otherwise the cache
> lookup may fail. It's been almost a year since I wrote the caching code, so I
> need to jog my memory to see if this is actually the case.

Yes, we need to invalidate cache for delete and rename operation.

The cache is always checked and filled before a task run in
MTPDeviceDelegateImplLinux (
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/med...
).
So we don't need to fill a cache manually for adding a new file.
While we can manually add new object_id to the cache by obtaining it from
libmtp, I think it makes the cache more complex.
I want to keep the cache as simple as possible.

[Lei Zhang, 2015-02-27 03:54:15]

On 2015/02/27 01:11:03, yawano wrote:
> Yes, we need to invalidate cache for delete and rename operation.
> 
> The cache is always checked and filled before a task run in
> MTPDeviceDelegateImplLinux (
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/med...
> ).
> So we don't need to fill a cache manually for adding a new file.
> While we can manually add new object_id to the cache by obtaining it from
> libmtp, I think it makes the cache more complex.
> I want to keep the cache as simple as possible.

Yes, I need a bit more time to convince myself that the code I wrote for caching
handles this case correctly. In the mean time, can we get the code building on
all platforms?

I'll get back to you and let you know if (a) the caching works and I have
forgotten why it works, or (b) I wrote some bad code.

[yawano, 2015-02-27 04:03:11]

On 2015/02/27 03:54:15, Lei Zhang wrote:
> On 2015/02/27 01:11:03, yawano wrote:
> > Yes, we need to invalidate cache for delete and rename operation.
> > 
> > The cache is always checked and filled before a task run in
> > MTPDeviceDelegateImplLinux (
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/med...
> > ).
> > So we don't need to fill a cache manually for adding a new file.
> > While we can manually add new object_id to the cache by obtaining it from
> > libmtp, I think it makes the cache more complex.
> > I want to keep the cache as simple as possible.
> 
> Yes, I need a bit more time to convince myself that the code I wrote for
caching
> handles this case correctly. In the mean time, can we get the code building on
> all platforms?
> 
> I'll get back to you and let you know if (a) the caching works and I have
> forgotten why it works, or (b) I wrote some bad code.

Sorry, I forgot to implement new methods also on other platforms. I'll create a
patch set for it. Thank you!

[yawano, 2015-02-27 10:05:55]

Added a patch set to fix compile errors on other platforms. PTAL. Thank you!

[Lei Zhang, 2015-03-02 23:41:39]

Tryjob failures look like you might need a cros_system_api DEPS roll.

https://codereview.chromium.org/947943002/diff/220001/chrome/browser/media_ga...
File chrome/browser/media_galleries/fileapi/mtp_device_map_service.cc (left):

https://codereview.chromium.org/947943002/diff/220001/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/mtp_device_map_service.cc:88: if
(!storage::ExternalMountPoints::GetSystemInstance()->GetRegisteredPath(
I can't remember why we do this. I'll need to dig into the history and see how
we ended up with this code.

https://codereview.chromium.org/947943002/diff/220001/chrome/browser/media_ga...
File chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc
(right):

https://codereview.chromium.org/947943002/diff/220001/chrome/browser/media_ga...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:635:
open(source_file_path.value().c_str(), O_RDONLY);
Actually, we should open() and close() this on a FILE thread or in a blocking
pool, rather than blocking the IO thread. Perhaps we should kick it off in
CopyFileFromLocal(), and then CopyFileFromLocalInternal() can take an int in
place of |source_file_path|.

https://codereview.chromium.org/947943002/diff/220001/chrome/browser/media_ga...
File chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.h
(right):

https://codereview.chromium.org/947943002/diff/220001/chrome/browser/media_ga...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.h:70:
explicit MTPDeviceDelegateImplLinux(const std::string& device_location,
no longer explicit

https://codereview.chromium.org/947943002/diff/220001/chrome/browser/media_ga...
File chrome/browser/media_galleries/linux/mtp_device_task_helper.h (right):

https://codereview.chromium.org/947943002/diff/220001/chrome/browser/media_ga...
chrome/browser/media_galleries/linux/mtp_device_task_helper.h:174: // Called
when CopyFileFromLocal completed either succeeded or failed.
nit: grammar.

[Lei Zhang, 2015-03-03 00:16:34]

https://codereview.chromium.org/947943002/diff/220001/chrome/browser/media_ga...
File chrome/browser/media_galleries/fileapi/mtp_device_map_service.cc (left):

https://codereview.chromium.org/947943002/diff/220001/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/mtp_device_map_service.cc:88: if
(!storage::ExternalMountPoints::GetSystemInstance()->GetRegisteredPath(
On 2015/03/02 23:41:38, Lei Zhang wrote:
> I can't remember why we do this. I'll need to dig into the history and see how
> we ended up with this code.

I suspect the reason for this is:

MediaFileSystemRegistry::MediaFileSystemContextImpl::RevokeFileSystem() first
calls [A] ExternalMountPoints::GetSystemInstance()->RevokeFileSystem(), and then
it uses PostTask() to call [B] MTPDeviceMapService::RevokeMTPFileSystem(). If
MTPDeviceMapService::GetMTPDeviceAsyncDelegate() happens after [A] but before
[B], then we would return a delegate for file system that's already revoked from
ExternalMountPoints's perspective. Thus we should probably leave this in.

[yawano, 2015-03-03 09:16:54]

We also needed to fix MTPDeviceTaskHelperMapService to support storage mode. I
added a patch set to fix it. PTAL. Thank you!

https://codereview.chromium.org/947943002/diff/220001/chrome/browser/media_ga...
File chrome/browser/media_galleries/fileapi/mtp_device_map_service.cc (left):

https://codereview.chromium.org/947943002/diff/220001/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/mtp_device_map_service.cc:88: if
(!storage::ExternalMountPoints::GetSystemInstance()->GetRegisteredPath(
On 2015/03/03 00:16:34, Lei Zhang wrote:
> On 2015/03/02 23:41:38, Lei Zhang wrote:
> > I can't remember why we do this. I'll need to dig into the history and see
how
> > we ended up with this code.
> 
> I suspect the reason for this is:
> 
> MediaFileSystemRegistry::MediaFileSystemContextImpl::RevokeFileSystem() first
> calls [A] ExternalMountPoints::GetSystemInstance()->RevokeFileSystem(), and
then
> it uses PostTask() to call [B] MTPDeviceMapService::RevokeMTPFileSystem(). If
> MTPDeviceMapService::GetMTPDeviceAsyncDelegate() happens after [A] but before
> [B], then we would return a delegate for file system that's already revoked
from
> ExternalMountPoints's perspective. Thus we should probably leave this in.

Done. Yes, we needed the check. I added it. Thank you!

https://codereview.chromium.org/947943002/diff/220001/chrome/browser/media_ga...
File chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc
(right):

https://codereview.chromium.org/947943002/diff/220001/chrome/browser/media_ga...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:635:
open(source_file_path.value().c_str(), O_RDONLY);
On 2015/03/02 23:41:39, Lei Zhang wrote:
> Actually, we should open() and close() this on a FILE thread or in a blocking
> pool, rather than blocking the IO thread. Perhaps we should kick it off in
> CopyFileFromLocal(), and then CopyFileFromLocalInternal() can take an int in
> place of |source_file_path|.

Done.

https://codereview.chromium.org/947943002/diff/220001/chrome/browser/media_ga...
File chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.h
(right):

https://codereview.chromium.org/947943002/diff/220001/chrome/browser/media_ga...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.h:70:
explicit MTPDeviceDelegateImplLinux(const std::string& device_location,
On 2015/03/02 23:41:39, Lei Zhang wrote:
> no longer explicit

Done.

https://codereview.chromium.org/947943002/diff/220001/chrome/browser/media_ga...
File chrome/browser/media_galleries/linux/mtp_device_task_helper.h (right):

https://codereview.chromium.org/947943002/diff/220001/chrome/browser/media_ga...
chrome/browser/media_galleries/linux/mtp_device_task_helper.h:174: // Called
when CopyFileFromLocal completed either succeeded or failed.
On 2015/03/02 23:41:39, Lei Zhang wrote:
> nit: grammar.

Done. I wish this change fixes the error.

[Lei Zhang, 2015-03-03 10:03:18]

https://codereview.chromium.org/947943002/diff/280001/chrome/browser/media_ga...
File chrome/browser/media_galleries/fileapi/mtp_device_map_service.cc (right):

https://codereview.chromium.org/947943002/diff/280001/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/mtp_device_map_service.cc:103:
MTPDeviceMapService::AsyncDelegateKey MTPDeviceMapService::GetAsyncDelegateKey(
Also static?

https://codereview.chromium.org/947943002/diff/280001/chrome/browser/media_ga...
File chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc
(right):

https://codereview.chromium.org/947943002/diff/280001/chrome/browser/media_ga...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:667: if
(source_file_descriptor != -1) {
nit: just write this as >= 0. Although open() should never return any other
negative value, they are all also invalid.

https://codereview.chromium.org/947943002/diff/280001/chrome/browser/media_ga...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:681:
PendingRequestDone();
This is being called on the wrong thread. Should be IO thread.

https://codereview.chromium.org/947943002/diff/280001/chrome/browser/media_ga...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:1026:
void MTPDeviceDelegateImplLinux::CloseFileDescriptor(
nit: This can just be a function in the anonymous namespace, rather than a
MTPDeviceDelegateImplLinux method.

[yawano, 2015-03-03 10:33:15]

Thank you for the review! PTAL.

https://codereview.chromium.org/947943002/diff/280001/chrome/browser/media_ga...
File chrome/browser/media_galleries/fileapi/mtp_device_map_service.cc (right):

https://codereview.chromium.org/947943002/diff/280001/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/mtp_device_map_service.cc:103:
MTPDeviceMapService::AsyncDelegateKey MTPDeviceMapService::GetAsyncDelegateKey(
On 2015/03/03 10:03:17, Lei Zhang wrote:
> Also static?

Done.

https://codereview.chromium.org/947943002/diff/280001/chrome/browser/media_ga...
File chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc
(right):

https://codereview.chromium.org/947943002/diff/280001/chrome/browser/media_ga...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:667: if
(source_file_descriptor != -1) {
On 2015/03/03 10:03:18, Lei Zhang wrote:
> nit: just write this as >= 0. Although open() should never return any other
> negative value, they are all also invalid.

Done.

https://codereview.chromium.org/947943002/diff/280001/chrome/browser/media_ga...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:681:
PendingRequestDone();
On 2015/03/03 10:03:18, Lei Zhang wrote:
> This is being called on the wrong thread. Should be IO thread.

Done.

https://codereview.chromium.org/947943002/diff/280001/chrome/browser/media_ga...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:1026:
void MTPDeviceDelegateImplLinux::CloseFileDescriptor(
On 2015/03/03 10:03:17, Lei Zhang wrote:
> nit: This can just be a function in the anonymous namespace, rather than a
> MTPDeviceDelegateImplLinux method.

Done.

[Lei Zhang, 2015-03-03 19:17:31]

https://codereview.chromium.org/947943002/diff/300001/chrome/browser/media_ga...
File chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc
(right):

https://codereview.chromium.org/947943002/diff/300001/chrome/browser/media_ga...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:679:
EnsureInitAndRunTask(PendingTaskInfo(
Sorry I didn't point this out earlier, but this is also on the wrong thread.
It's probably easier to do just the open() on the FILE thread, and then reply
back to CopyFileFromLocalInternal() on the IO thread.

I highly recommend running the code to make sure it actually works.

[yawano, 2015-03-04 02:29:07]

Fixed to call open() without using PendingTaskInfo. It could make the code much
simpler. I also tested that it actually works with changing write_supported flag
to true. PTAL. Thank you!

https://codereview.chromium.org/947943002/diff/300001/chrome/browser/media_ga...
File chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc
(right):

https://codereview.chromium.org/947943002/diff/300001/chrome/browser/media_ga...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:679:
EnsureInitAndRunTask(PendingTaskInfo(
On 2015/03/03 19:17:31, Lei Zhang wrote:
> Sorry I didn't point this out earlier, but this is also on the wrong thread.
> It's probably easier to do just the open() on the FILE thread, and then reply
> back to CopyFileFromLocalInternal() on the IO thread.
> 
> I highly recommend running the code to make sure it actually works.

Done. Thank you, we could make the code much simpler!

https://codereview.chromium.org/947943002/diff/320001/chrome/browser/media_ga...
File chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc
(right):

https://codereview.chromium.org/947943002/diff/320001/chrome/browser/media_ga...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:237:
return open(file_path, flags);
Since open() takes variable number of arguments, we needed to create a wrapper
for it.

[Lei Zhang, 2015-03-04 02:37:00]

lgtm

https://codereview.chromium.org/947943002/diff/320001/chrome/browser/media_ga...
File chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc
(right):

https://codereview.chromium.org/947943002/diff/320001/chrome/browser/media_ga...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:237:
return open(file_path, flags);
Can you add DCHECK_CURRENTLY_ON(content::BrowserThread::FILE); just like CloseFD
below?

[yawano, 2015-03-04 04:11:27]

Thank you for the review! Added DCHECK_CURRENTLY_ON. After dependent CL is
landed, I'm going to roll it out, rebase and commit this CL.

https://codereview.chromium.org/947943002/diff/320001/chrome/browser/media_ga...
File chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc
(right):

https://codereview.chromium.org/947943002/diff/320001/chrome/browser/media_ga...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:237:
return open(file_path, flags);
On 2015/03/04 02:36:59, Lei Zhang wrote:
> Can you add DCHECK_CURRENTLY_ON(content::BrowserThread::FILE); just like
CloseFD
> below?

Done.

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

The CQ bit was checked by yawano@chromium.org

[yawano, 2015-03-05 01:58:29]

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/947943002/#ps360001
(title: "Rebase.")

[commit-bot: I haz the power, 2015-03-05 01:59:43]

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

[commit-bot: I haz the power, 2015-03-05 03:43:23]

Committed patchset #19 (id:360001)

[commit-bot: I haz the power, 2015-03-05 03:44:00]

Patchset 19 (id:??) landed as
https://crrev.com/8cd28e349ab9605da7dbf97c8c85f8c541b3b59b
Cr-Commit-Position: refs/heads/master@{#319205}