Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1173)

Issue 246293011: Mount MTP devices in Chrome OS Files.app. (Closed)

Created:
6 years, 8 months ago by kinaba
Modified:
6 years, 8 months ago
CC:
chromium-reviews, nkostylev+watch_chromium.org, tzik, yoshiki+watch_chromium.org, nhiroki, rginda+watch_chromium.org, darin-cc_chromium.org, oshima+watch_chromium.org, kinuko+watch, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, Lei Zhang, tommycli
Visibility:
Public.

Description

Mount MTP devices in Chrome OS Files.app. This CL * Adds new filesystem type kFileSystemTypeDeviceMediaAsFileStorage, which is handled by chromeos::FileSystemBackend. It uses DeviceMediaAsyncFileUtil, so the implementation is shared with MTP support in mediaGalleries API (MediaFileSystemBackend). We need different types since we need different handlings. (Permission management for file handler extensions, unlimited CopyOrMoveValidator for allowing non-media files, etc.) * Adds monitoring by storage_monitor::RemovableStorageObserver in file_manager::VolumeManager so that MTP device is detected and mounted to the file manager. BUG=363960 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266458

Patch Set 1 : #

Total comments: 14

Patch Set 2 : Review fix (almost) #

Patch Set 3 : Split delegate class #

Total comments: 2

Patch Set 4 : DCHECK type. #

Total comments: 1

Patch Set 5 : Rebase. #

Patch Set 6 : Rebase + fix test (handle null StorageMonitor in some tests) #

Unified diffs Side-by-side diffs Delta from patch set Stats (+251 lines, -3 lines) Patch
M chrome/browser/chrome_content_browser_client.cc View 1 2 3 4 2 chunks +2 lines, -0 lines 0 comments Download
M chrome/browser/chromeos/file_manager/volume_manager.h View 3 chunks +10 lines, -1 line 0 comments Download
M chrome/browser/chromeos/file_manager/volume_manager.cc View 1 2 3 4 5 7 chunks +87 lines, -0 lines 0 comments Download
M chrome/browser/chromeos/fileapi/file_system_backend.h View 1 2 2 chunks +4 lines, -0 lines 0 comments Download
M chrome/browser/chromeos/fileapi/file_system_backend.cc View 1 2 3 4 8 chunks +21 lines, -1 line 0 comments Download
M chrome/browser/chromeos/fileapi/file_system_backend_unittest.cc View 1 2 4 chunks +4 lines, -0 lines 0 comments Download
A chrome/browser/chromeos/fileapi/mtp_file_system_backend_delegate.h View 1 2 1 chunk +48 lines, -0 lines 0 comments Download
A chrome/browser/chromeos/fileapi/mtp_file_system_backend_delegate.cc View 1 2 3 1 chunk +56 lines, -0 lines 0 comments Download
M chrome/chrome_browser_chromeos.gypi View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M components/storage_monitor/storage_info.h View 1 1 chunk +2 lines, -0 lines 0 comments Download
M components/storage_monitor/storage_info.cc View 1 1 chunk +6 lines, -0 lines 0 comments Download
M webkit/browser/fileapi/external_mount_points.cc View 1 chunk +2 lines, -1 line 0 comments Download
M webkit/browser/fileapi/file_system_context.cc View 1 chunk +1 line, -0 lines 0 comments Download
M webkit/common/fileapi/file_system_types.h View 1 chunk +4 lines, -0 lines 0 comments Download
M webkit/common/fileapi/file_system_util.cc View 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 24 (0 generated)
kinaba
@vandebo: on the usage of components from media file system implementation. @mtomasz: on VolumeManager changes. ...
6 years, 8 months ago (2014-04-23 08:11:20 UTC) #1
tzik
fileapi/ change lgtm.
6 years, 8 months ago (2014-04-23 11:28:45 UTC) #2
mtomasz
https://codereview.chromium.org/246293011/diff/60001/chrome/browser/chromeos/fileapi/file_system_backend_unittest.cc File chrome/browser/chromeos/fileapi/file_system_backend_unittest.cc (right): https://codereview.chromium.org/246293011/diff/60001/chrome/browser/chromeos/fileapi/file_system_backend_unittest.cc#newcode45 chrome/browser/chromeos/fileapi/file_system_backend_unittest.cc:45: NULL, // media_task_runner, There seem to be no media_task_runner ...
6 years, 8 months ago (2014-04-23 12:17:18 UTC) #3
vandebo (ex-Chrome)
https://codereview.chromium.org/246293011/diff/60001/chrome/browser/chromeos/file_manager/volume_manager.cc File chrome/browser/chromeos/file_manager/volume_manager.cc (right): https://codereview.chromium.org/246293011/diff/60001/chrome/browser/chromeos/file_manager/volume_manager.cc#newcode45 chrome/browser/chromeos/file_manager/volume_manager.cc:45: const char kFileManagerMountNamePrefix[] = "fileman"; nit: maybe make this ...
6 years, 8 months ago (2014-04-23 16:24:44 UTC) #4
kinaba
https://codereview.chromium.org/246293011/diff/60001/chrome/browser/chromeos/file_manager/volume_manager.cc File chrome/browser/chromeos/file_manager/volume_manager.cc (right): https://codereview.chromium.org/246293011/diff/60001/chrome/browser/chromeos/file_manager/volume_manager.cc#newcode45 chrome/browser/chromeos/file_manager/volume_manager.cc:45: const char kFileManagerMountNamePrefix[] = "fileman"; On 2014/04/23 16:24:44, vandebo ...
6 years, 8 months ago (2014-04-24 01:33:30 UTC) #5
kinaba
+ericu as the webkit/common/fileapi OWNER (I didn't realize webkit/(browser|common)/fileapi has different set of owners!)
6 years, 8 months ago (2014-04-24 01:35:08 UTC) #6
mtomasz
lgtm with a nit https://codereview.chromium.org/246293011/diff/100001/chrome/browser/chromeos/fileapi/mtp_file_system_backend_delegate.cc File chrome/browser/chromeos/fileapi/mtp_file_system_backend_delegate.cc (right): https://codereview.chromium.org/246293011/diff/100001/chrome/browser/chromeos/fileapi/mtp_file_system_backend_delegate.cc#newcode23 chrome/browser/chromeos/fileapi/mtp_file_system_backend_delegate.cc:23: return device_media_async_file_util_.get(); nit: How about ...
6 years, 8 months ago (2014-04-24 01:41:41 UTC) #7
kinaba
https://codereview.chromium.org/246293011/diff/100001/chrome/browser/chromeos/fileapi/mtp_file_system_backend_delegate.cc File chrome/browser/chromeos/fileapi/mtp_file_system_backend_delegate.cc (right): https://codereview.chromium.org/246293011/diff/100001/chrome/browser/chromeos/fileapi/mtp_file_system_backend_delegate.cc#newcode23 chrome/browser/chromeos/fileapi/mtp_file_system_backend_delegate.cc:23: return device_media_async_file_util_.get(); On 2014/04/24 01:41:42, mtomasz wrote: > nit: ...
6 years, 8 months ago (2014-04-24 01:52:34 UTC) #8
kinuko
On 2014/04/24 01:35:08, kinaba wrote: > (I didn't realize > webkit/(browser|common)/fileapi has different set of ...
6 years, 8 months ago (2014-04-24 02:22:30 UTC) #9
Lei Zhang
chrome/browser/chrome_content_browser_client.cc and components/storage_monitor/ lgtm https://codereview.chromium.org/246293011/diff/120001/chrome/browser/chromeos/fileapi/mtp_file_system_backend_delegate.cc File chrome/browser/chromeos/fileapi/mtp_file_system_backend_delegate.cc (right): https://codereview.chromium.org/246293011/diff/120001/chrome/browser/chromeos/fileapi/mtp_file_system_backend_delegate.cc#newcode52 chrome/browser/chromeos/fileapi/mtp_file_system_backend_delegate.cc:52: // TODO(kinaba): support writing. We ...
6 years, 8 months ago (2014-04-24 02:42:02 UTC) #10
ericu
webkit/common/fileapi LGTM.
6 years, 8 months ago (2014-04-24 03:46:20 UTC) #11
kinaba
On 2014/04/24 03:46:20, ericu wrote: > webkit/common/fileapi LGTM. Thank you, all! @vandebo, could you take ...
6 years, 8 months ago (2014-04-24 22:48:31 UTC) #12
vandebo (ex-Chrome)
sorry for the delay, LGTM
6 years, 8 months ago (2014-04-25 15:54:04 UTC) #13
kinaba
The CQ bit was checked by kinaba@chromium.org
6 years, 8 months ago (2014-04-26 01:13:37 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kinaba@chromium.org/246293011/140001
6 years, 8 months ago (2014-04-26 02:05:16 UTC) #15
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 8 months ago (2014-04-26 03:24:56 UTC) #16
commit-bot: I haz the power
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
6 years, 8 months ago (2014-04-26 03:24:57 UTC) #17
Lei Zhang
The CQ bit was checked by thestig@chromium.org
6 years, 8 months ago (2014-04-26 03:48:02 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kinaba@chromium.org/246293011/140001
6 years, 8 months ago (2014-04-26 03:51:17 UTC) #19
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 8 months ago (2014-04-26 04:57:55 UTC) #20
commit-bot: I haz the power
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
6 years, 8 months ago (2014-04-26 04:57:56 UTC) #21
kinaba
The CQ bit was checked by kinaba@chromium.org
6 years, 8 months ago (2014-04-28 00:37:38 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kinaba@chromium.org/246293011/150001
6 years, 8 months ago (2014-04-28 00:38:53 UTC) #23
commit-bot: I haz the power
6 years, 8 months ago (2014-04-28 07:00:02 UTC) #24
Message was sent while issue was closed.
Change committed as 266458

Powered by Google App Engine
This is Rietveld 408576698