Launch packaged app with files on Drive on Chrome OS.

Launch packaged app with files on Drive on Chrome OS.

BUG=156277
TEST=Install a packaged app associated to some file type, and execute
the app for a file on "Google Drive" folder in Chrome OS file manager.


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

[kinaba, 2013-03-19 13:17:31]

+tbarzic, +kinuko,

Based on the patch sets of https://codereview.chromium.org/12258021/,
I've created a version that I think it is ready to submit.
Could you take a look? The idea is, to minimize field addition
to FileSystemURL, by not using virtual_path() from implementation side.

+benwells

Could you take a look around platform_app_launcher.cc?
Toni has addressed your comments in the previous CL.
I'm also preparing a CL for api/file_system/file_system_api.cc,
but I'd like to proceed incrementally.

[kinuko, 2013-03-19 20:34:50]

https://codereview.chromium.org/12717014/diff/13002/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/drive_file_system_proxy.cc (right):

https://codereview.chromium.org/12717014/diff/13002/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_file_system_proxy.cc:698: }
nit: can you drop { } for this one line body while you're there?

https://codereview.chromium.org/12717014/diff/13002/webkit/fileapi/file_syste...
File webkit/fileapi/file_system_context.cc (right):

https://codereview.chromium.org/12717014/diff/13002/webkit/fileapi/file_syste...
webkit/fileapi/file_system_context.cc:368: if (!crack_happened)
nit: if (cracked == current) may be enough

https://codereview.chromium.org/12717014/diff/13002/webkit/fileapi/file_syste...
File webkit/fileapi/file_system_types.h (right):

https://codereview.chromium.org/12717014/diff/13002/webkit/fileapi/file_syste...
webkit/fileapi/file_system_types.h:81: kFileSystemTypeExternalFullPath,
This filesystem type is being added not based on underlying implementation but
for higher-level mount point view.  Could we come up with a better name (and
better comment) to indicate the fact?  'ExternalFullPath' sounds too biased to
the chromeos usage to me.  How one can guess this could mean NativeLocal in
non-chromeos world?

The term 'Native' may feel better than 'External', since in ChromeOS 'native
path' might just mean the external filesystem path (I could be wrong). 
kFileSystemTypeNativeForPlatformApp?  May sound too restrictive?

...oh but it's handled in ExternalMountPoints, that's why you prefixed External.
 Maybe should we just use this type only for ChromeOS and remove the special
cracking using NativeLocal for non-chromeos?

https://codereview.chromium.org/12717014/diff/13002/webkit/fileapi/mount_poin...
File webkit/fileapi/mount_points.h (right):

https://codereview.chromium.org/12717014/diff/13002/webkit/fileapi/mount_poin...
webkit/fileapi/mount_points.h:60: virtual FileSystemURL CrackFileSystemURL(const
FileSystemURL& url) const = 0;
Assuming that nested cracking only happens in FileSystemContext, does it make
sense if we make this protected, friend FileSystemContext here and implement
this as private method in each subclass.

It's not clean but having this exposed as a public method worries me as it
appears very confusing.

[benwells, 2013-03-20 21:49:44]

chrome/browser/extensions lgtm

[kinaba, 2013-03-21 07:35:13]

Updated. Please take another look. Thanks!

https://codereview.chromium.org/12717014/diff/13002/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/drive_file_system_proxy.cc (right):

https://codereview.chromium.org/12717014/diff/13002/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_file_system_proxy.cc:698: }
On 2013/03/19 20:34:50, kinuko wrote:
> nit: can you drop { } for this one line body while you're there?

Done.

https://codereview.chromium.org/12717014/diff/13002/webkit/fileapi/file_syste...
File webkit/fileapi/file_system_context.cc (right):

https://codereview.chromium.org/12717014/diff/13002/webkit/fileapi/file_syste...
webkit/fileapi/file_system_context.cc:368: if (!crack_happened)
On 2013/03/19 20:34:50, kinuko wrote:
> nit: if (cracked == current) may be enough

Done.

https://codereview.chromium.org/12717014/diff/13002/webkit/fileapi/file_syste...
File webkit/fileapi/file_system_types.h (right):

https://codereview.chromium.org/12717014/diff/13002/webkit/fileapi/file_syste...
webkit/fileapi/file_system_types.h:81: kFileSystemTypeExternalFullPath,
Changed to kFileSystemTypeNativeForPlatformApp.

Using the type only for ChromeOS is possible, but it means
the uses of RegisterFileSystemForPath in apps must be wrapped
with #ifdef OS_CHROMEOS, which doesn't seem clean and maintainable.

On 2013/03/19 20:34:50, kinuko wrote:
> This filesystem type is being added not based on underlying implementation but
> for higher-level mount point view.  Could we come up with a better name (and
> better comment) to indicate the fact?  'ExternalFullPath' sounds too biased to
> the chromeos usage to me.  How one can guess this could mean NativeLocal in
> non-chromeos world?
> 
> The term 'Native' may feel better than 'External', since in ChromeOS 'native
> path' might just mean the external filesystem path (I could be wrong). 
> kFileSystemTypeNativeForPlatformApp?  May sound too restrictive?
> 
> ...oh but it's handled in ExternalMountPoints, that's why you prefixed
External.
>  Maybe should we just use this type only for ChromeOS and remove the special
> cracking using NativeLocal for non-chromeos?

https://codereview.chromium.org/12717014/diff/13002/webkit/fileapi/mount_poin...
File webkit/fileapi/mount_points.h (right):

https://codereview.chromium.org/12717014/diff/13002/webkit/fileapi/mount_poin...
webkit/fileapi/mount_points.h:60: virtual FileSystemURL CrackFileSystemURL(const
FileSystemURL& url) const = 0;
On 2013/03/19 20:34:50, kinuko wrote:
> Assuming that nested cracking only happens in FileSystemContext, does it make
> sense if we make this protected, friend FileSystemContext here and implement
> this as private method in each subclass.
> 
> It's not clean but having this exposed as a public method worries me as it
> appears very confusing.

Done.

[kinuko, 2013-03-21 16:47:50]

Ok, the new fs type concerns me a bit but I think this could work.

https://codereview.chromium.org/12717014/diff/44001/webkit/fileapi/external_m...
File webkit/fileapi/external_mount_points.cc (right):

https://codereview.chromium.org/12717014/diff/44001/webkit/fileapi/external_m...
webkit/fileapi/external_mount_points.cc:210: FileSystemURL
ExternalMountPoints::CrackFileSystemURL(
can you match the method order in .cc and in .h?

https://codereview.chromium.org/12717014/diff/44001/webkit/fileapi/file_syste...
File webkit/fileapi/file_system_url.h (right):

https://codereview.chromium.org/12717014/diff/44001/webkit/fileapi/file_syste...
webkit/fileapi/file_system_url.h:62: //   filesystem_id() returns 'mount_name'.
can you add a brief explanation about mount_filesystem_id() and which one caller
should use here?

https://codereview.chromium.org/12717014/diff/44001/webkit/fileapi/isolated_c...
File webkit/fileapi/isolated_context.cc (right):

https://codereview.chromium.org/12717014/diff/44001/webkit/fileapi/isolated_c...
webkit/fileapi/isolated_context.cc:307: FileSystemURL
IsolatedContext::CrackFileSystemURL(
nit: can you match the method order in .cc and .h?

[kinuko, 2013-03-21 16:49:01]

On 2013/03/21 16:47:50, kinuko wrote:
> Ok, the new fs type concerns me a bit but I think this could work.
> 
>
https://codereview.chromium.org/12717014/diff/44001/webkit/fileapi/external_m...
> File webkit/fileapi/external_mount_points.cc (right):
> 
>
https://codereview.chromium.org/12717014/diff/44001/webkit/fileapi/external_m...
> webkit/fileapi/external_mount_points.cc:210: FileSystemURL
> ExternalMountPoints::CrackFileSystemURL(
> can you match the method order in .cc and in .h?
> 
>
https://codereview.chromium.org/12717014/diff/44001/webkit/fileapi/file_syste...
> File webkit/fileapi/file_system_url.h (right):
> 
>
https://codereview.chromium.org/12717014/diff/44001/webkit/fileapi/file_syste...
> webkit/fileapi/file_system_url.h:62: //   filesystem_id() returns
'mount_name'.
> can you add a brief explanation about mount_filesystem_id() and which one
caller
> should use here?

I want to see this comment improved but stamping lgtm in advance

>
https://codereview.chromium.org/12717014/diff/44001/webkit/fileapi/isolated_c...
> File webkit/fileapi/isolated_context.cc (right):
> 
>
https://codereview.chromium.org/12717014/diff/44001/webkit/fileapi/isolated_c...
> webkit/fileapi/isolated_context.cc:307: FileSystemURL
> IsolatedContext::CrackFileSystemURL(
> nit: can you match the method order in .cc and .h?

[kinaba, 2013-03-22 04:58:33]

> Ok, the new fs type concerns me a bit but I think this could work.
Ideally I'm thinking about merging it to the simple, existing
kFileSystemTypeExternal, by changing
Chrome OS file chooser and app invocation process, but it seems to require
a fair amount of changes everywhere. I'll revisit here if I could come up with
the cleanest solution.

Thanks for the review again. Let me go ahead with this one.

https://codereview.chromium.org/12717014/diff/44001/webkit/fileapi/external_m...
File webkit/fileapi/external_mount_points.cc (right):

https://codereview.chromium.org/12717014/diff/44001/webkit/fileapi/external_m...
webkit/fileapi/external_mount_points.cc:210: FileSystemURL
ExternalMountPoints::CrackFileSystemURL(
On 2013/03/21 16:47:51, kinuko wrote:
> can you match the method order in .cc and in .h?

Done.

https://codereview.chromium.org/12717014/diff/44001/webkit/fileapi/file_syste...
File webkit/fileapi/file_system_url.h (right):

https://codereview.chromium.org/12717014/diff/44001/webkit/fileapi/file_syste...
webkit/fileapi/file_system_url.h:62: //   filesystem_id() returns 'mount_name'.
On 2013/03/21 16:47:51, kinuko wrote:
> can you add a brief explanation about mount_filesystem_id() and which one
caller
> should use here?

Done.

https://codereview.chromium.org/12717014/diff/44001/webkit/fileapi/isolated_c...
File webkit/fileapi/isolated_context.cc (right):

https://codereview.chromium.org/12717014/diff/44001/webkit/fileapi/isolated_c...
webkit/fileapi/isolated_context.cc:307: FileSystemURL
IsolatedContext::CrackFileSystemURL(
On 2013/03/21 16:47:51, kinuko wrote:
> nit: can you match the method order in .cc and .h?

Done.

[commit-bot: I haz the power, 2013-03-22 05:00:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinaba@chromium.org/12717014/53001

[commit-bot: I haz the power, 2013-03-22 18:35:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinaba@chromium.org/12717014/53001

[commit-bot: I haz the power, 2013-03-23 15:21:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinaba@chromium.org/12717014/53001

[commit-bot: I haz the power, 2013-03-24 01:37:15]

Change committed as 190144