OpenFileSystem should accept not mount_type but lower filesystem type

OpenFileSystem should accept not mount_type but lower filesystem type

FileSystemContext::ResolveURL which will be enabled by the Blink side change[1]
passes not a mount_type (ex. kFileSystemTypeExternal) but a lower filesystem
type (ex. kFileSystemTypeNativeLocal) into FileSystemBackend::OpenFileSystem.

[1] https://codereview.chromium.org/24376002/


BUG=177137, 297412
TEST=browser_tests --gtest_filter=FileDisplay/FileManagerBrowserTest.* (with Blink patch)
TEST=unit_tests --gtest_filter=ChromeOSFileSystemBackendTest.*
NOTRY=true

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

[nhiroki, 2013-09-23 17:49:46]

Hi satoru-san, can you review this? Thanks.

[kinuko, 2013-09-23 23:57:34]

https://codereview.chromium.org/24373002/diff/3001/chrome/browser/chromeos/fi...
File chrome/browser/chromeos/fileapi/file_system_backend.cc (right):

https://codereview.chromium.org/24373002/diff/3001/chrome/browser/chromeos/fi...
chrome/browser/chromeos/fileapi/file_system_backend.cc:116:
GetFileSystemName(origin_url, mount_type),
Hmm... this looks to be a very unfortunate change, I think at least we need a
TODO comment here.

Here's why: if cros backend's going to migrate to multiple filesystems this
should return a root URL that includes mount-name path for the given fs type
(like we do for syncfs).  It doesn't look correct to always assume kExternal
type for all filesystem types either.

Some more thoughts:
- OpenFileSystem for non-sandboxed filesystems should be probably just
deprecated as it has no points.  Existing callsites (there're three, [1]) should
be revised either to use ResolveURL or to directly use GetFileSystemRootURI /
GetExternalFileSystemRootURIString
- When crbug.com/284963 is done ResolveURI should start to return the correct
root URI which includes the mount name for isolated/external types (and the part
must be able to be generalized so that we don't need to call per-backend
OpenFileSystem for non-sandboxed filesystems just to get the rootURI & name)

[1]
https://code.google.com/p/chromium/codesearch#search/&q=%5C-%3EOpenFileSystem...

[satorux1, 2013-09-24 04:12:50]

https://codereview.chromium.org/24373002/diff/3001/chrome/browser/chromeos/fi...
File chrome/browser/chromeos/fileapi/file_system_backend.cc (right):

https://codereview.chromium.org/24373002/diff/3001/chrome/browser/chromeos/fi...
chrome/browser/chromeos/fileapi/file_system_backend.cc:116:
GetFileSystemName(origin_url, mount_type),
On 2013/09/23 23:57:34, kinuko wrote:
> Hmm... this looks to be a very unfortunate change, I think at least we need a
> TODO comment here.
> 
> Here's why: if cros backend's going to migrate to multiple filesystems this
> should return a root URL that includes mount-name path for the given fs type
> (like we do for syncfs).  It doesn't look correct to always assume kExternal
> type for all filesystem types either.
> 
> Some more thoughts:
> - OpenFileSystem for non-sandboxed filesystems should be probably just
> deprecated as it has no points.  Existing callsites (there're three, [1])
should
> be revised either to use ResolveURL or to directly use GetFileSystemRootURI /
> GetExternalFileSystemRootURIString
> - When crbug.com/284963 is done ResolveURI should start to return the correct
> root URI which includes the mount name for isolated/external types (and the
part
> must be able to be generalized so that we don't need to call per-backend
> OpenFileSystem for non-sandboxed filesystems just to get the rootURI & name)
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#search/&q=%255C-%253EOpenFileSy...

Agreed. TODO comment is a crbug.com URL would be nice here.

[nhiroki, 2013-09-24 06:14:28]

Updated. Can you take another look? Thanks!

https://codereview.chromium.org/24373002/diff/3001/chrome/browser/chromeos/fi...
File chrome/browser/chromeos/fileapi/file_system_backend.cc (right):

https://codereview.chromium.org/24373002/diff/3001/chrome/browser/chromeos/fi...
chrome/browser/chromeos/fileapi/file_system_backend.cc:116:
GetFileSystemName(origin_url, mount_type),
On 2013/09/24 04:12:51, satorux1 wrote:
> On 2013/09/23 23:57:34, kinuko wrote:
> > Hmm... this looks to be a very unfortunate change, I think at least we need
a
> > TODO comment here.
> > 
> > Here's why: if cros backend's going to migrate to multiple filesystems this
> > should return a root URL that includes mount-name path for the given fs type
> > (like we do for syncfs).  It doesn't look correct to always assume kExternal
> > type for all filesystem types either.
> > 
> > Some more thoughts:
> > - OpenFileSystem for non-sandboxed filesystems should be probably just
> > deprecated as it has no points.  Existing callsites (there're three, [1])
> should
> > be revised either to use ResolveURL or to directly use GetFileSystemRootURI
/
> > GetExternalFileSystemRootURIString
> > - When crbug.com/284963 is done ResolveURI should start to return the
correct
> > root URI which includes the mount name for isolated/external types (and the
> part
> > must be able to be generalized so that we don't need to call per-backend
> > OpenFileSystem for non-sandboxed filesystems just to get the rootURI & name)
> > 
> > [1]
> >
>
https://code.google.com/p/chromium/codesearch#search/&q=%25255C-%25253EOpenFi...
> 
> Agreed. TODO comment is a http://crbug.com URL would be nice here.

Filed (http://crbug.com/297412) and added a comment.

[satorux1, 2013-09-24 06:19:20]

lgtm

[commit-bot: I haz the power, 2013-09-24 06:21:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nhiroki@chromium.org/24373002/14001

[commit-bot: I haz the power, 2013-09-24 12:00:57]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-09-24 13:24:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nhiroki@chromium.org/24373002/14001

[commit-bot: I haz the power, 2013-09-24 13:25:19]

Change committed as 224991