Fix filesystem API file_handlers to work for drive on ChromeOS.

Fix filesystem API file_handlers to work for drive on ChromeOS.

BUG=156277
TEST=tbd

START new filesystemextensionapi tests

[tbarzic, 2013-02-13 22:04:09]

hey guys,
what do you think about doing something like this?

[benwells, 2013-02-14 01:20:05]

On 2013/02/13 22:04:09, tbarzic wrote:
> hey guys,
> what do you think about doing something like this?

Is there work underway to make isolated file systems on drive Just Work? (sorry
if that is a dumb question).

[tbarzic, 2013-02-14 02:25:30]

On 2013/02/14 01:20:05, benwells wrote:
> On 2013/02/13 22:04:09, tbarzic wrote:
> > hey guys,
> > what do you think about doing something like this?
> 
> Is there work underway to make isolated file systems on drive Just Work?
(sorry
> if that is a dumb question).

Yeah, I think kinaba is working on this (cc-ed on the cl). I was playing with
cros_mount_point_provider part of it this morning, and figured this could be
useful :)

Still, I'm not sure we'll be able to avoid the changes in platform_app_launcher,
since I don't think we can get the file MIME type directly from its drive cache
path...

[kinaba, 2013-02-14 02:27:53]

On 2013/02/14 02:25:30, tbarzic wrote:
> > Is there work underway to make isolated file systems on drive Just Work?
> Yeah, I think kinaba is working on this (cc-ed on the cl).

Yes, that's what I'm going to do (targetting M27).
#will take a look at this CL later,

[kinuko, 2013-02-14 17:09:48]

https://codereview.chromium.org/12258021/diff/4001/webkit/chromeos/fileapi/cr...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

https://codereview.chromium.org/12258021/diff/4001/webkit/chromeos/fileapi/cr...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:287: const
fileapi::FileSystemURL& original) const {
Do we need to specifically crack the isolated URL as 'external FS' URL?
Is it possible to just crack this URL at once in FSC::CrackFileSystemURL by
recursively apply URL crackers?  It'll need some tweaks in several modules but
if it's possible it sounds like lower modules can just get the final cracked URL
and do not need to worry about nested mount details.  (I might be missing some
devils in detail though)

[tbarzic, 2013-02-14 20:22:31]

https://codereview.chromium.org/12258021/diff/4001/webkit/chromeos/fileapi/cr...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

https://codereview.chromium.org/12258021/diff/4001/webkit/chromeos/fileapi/cr...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:287: const
fileapi::FileSystemURL& original) const {
On 2013/02/14 17:09:48, kinuko wrote:
> Do we need to specifically crack the isolated URL as 'external FS' URL?
> Is it possible to just crack this URL at once in FSC::CrackFileSystemURL by
> recursively apply URL crackers?  It'll need some tweaks in several modules but
> if it's possible it sounds like lower modules can just get the final cracked
URL
> and do not need to worry about nested mount details.  (I might be missing some
> devils in detail though)

this could be problematic because we use different policy for isolated and
external mount points when checking access permissions.

Hm, maybe we could add some AsExternalFileSystemURL method to FileSystemURL, but
that would probably mean more members in FileSystemURL (external_type_,
external_virtual_path_, external_mount_name_)

I'll think about this over the weekend..

[kinuko, 2013-02-15 02:18:19]

On 2013/02/14 20:22:31, tbarzic wrote:
>
https://codereview.chromium.org/12258021/diff/4001/webkit/chromeos/fileapi/cr...
> File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):
> 
>
https://codereview.chromium.org/12258021/diff/4001/webkit/chromeos/fileapi/cr...
> webkit/chromeos/fileapi/cros_mount_point_provider.cc:287: const
> fileapi::FileSystemURL& original) const {
> On 2013/02/14 17:09:48, kinuko wrote:
> > Do we need to specifically crack the isolated URL as 'external FS' URL?
> > Is it possible to just crack this URL at once in FSC::CrackFileSystemURL by
> > recursively apply URL crackers?  It'll need some tweaks in several modules
but
> > if it's possible it sounds like lower modules can just get the final cracked
> URL
> > and do not need to worry about nested mount details.  (I might be missing
some
> > devils in detail though)
> 
> this could be problematic because we use different policy for isolated and
> external mount points when checking access permissions.

I thought about that but in other IsolatedFS usage in general creating an
isolated FS on top of another FS and giving permission to the isolated FS means
we give the permission to the particular part scoped by the isolated FS only
when it's accessed via isolated FS. (Assuming that creating an isolated FS is
usually only done by an explicit user action and/or via special permission given
to the app)
Does this policy work for your case too or do we need to enforce different
security policy?

> Hm, maybe we could add some AsExternalFileSystemURL method to FileSystemURL,
but
> that would probably mean more members in FileSystemURL (external_type_,
> external_virtual_path_, external_mount_name_)
> 
> I'll think about this over the weekend..

[tbarzic, 2013-02-15 02:51:04]

On 2013/02/15 02:18:19, kinuko wrote:
> On 2013/02/14 20:22:31, tbarzic wrote:
> >
>
https://codereview.chromium.org/12258021/diff/4001/webkit/chromeos/fileapi/cr...
> > File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):
> > 
> >
>
https://codereview.chromium.org/12258021/diff/4001/webkit/chromeos/fileapi/cr...
> > webkit/chromeos/fileapi/cros_mount_point_provider.cc:287: const
> > fileapi::FileSystemURL& original) const {
> > On 2013/02/14 17:09:48, kinuko wrote:
> > > Do we need to specifically crack the isolated URL as 'external FS' URL?
> > > Is it possible to just crack this URL at once in FSC::CrackFileSystemURL
by
> > > recursively apply URL crackers?  It'll need some tweaks in several modules
> but
> > > if it's possible it sounds like lower modules can just get the final
cracked
> > URL
> > > and do not need to worry about nested mount details.  (I might be missing
> some
> > > devils in detail though)
> > 
> > this could be problematic because we use different policy for isolated and
> > external mount points when checking access permissions.
> 
> I thought about that but in other IsolatedFS usage in general creating an
> isolated FS on top of another FS and giving permission to the isolated FS
means
> we give the permission to the particular part scoped by the isolated FS only
> when it's accessed via isolated FS. (Assuming that creating an isolated FS is
> usually only done by an explicit user action and/or via special permission
given
> to the app)
> Does this policy work for your case too or do we need to enforce different
> security policy?
> 

I don't think we should change security policy. I'm just saying that (I probably
had misunderstood your initial point a bit) if FSC::CrackFileSystemContextURL
returned an external file system url in this case, then security check would
fail for the url, because the permissions for the path are granted through
IsolatedFS, so we need IsolatedFSURL to verify access permission...

So what I'm thinking about now is expanding FSC::CrackFileSystemURL to add some
additional information about underlying external file system (and use it later
when getting remote proxy).. but this probably nedds some more thought.

> > Hm, maybe we could add some AsExternalFileSystemURL method to FileSystemURL,
> but
> > that would probably mean more members in FileSystemURL (external_type_,
> > external_virtual_path_, external_mount_name_)
> > 
> > I'll think about this over the weekend..

[kinuko, 2013-02-18 09:55:07]

On 2013/02/15 02:51:04, tbarzic wrote:
> On 2013/02/15 02:18:19, kinuko wrote:
> > On 2013/02/14 20:22:31, tbarzic wrote:
> > >
> >
>
https://codereview.chromium.org/12258021/diff/4001/webkit/chromeos/fileapi/cr...
> > > File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/12258021/diff/4001/webkit/chromeos/fileapi/cr...
> > > webkit/chromeos/fileapi/cros_mount_point_provider.cc:287: const
> > > fileapi::FileSystemURL& original) const {
> > > On 2013/02/14 17:09:48, kinuko wrote:
> > > > Do we need to specifically crack the isolated URL as 'external FS' URL?
> > > > Is it possible to just crack this URL at once in FSC::CrackFileSystemURL
> by
> > > > recursively apply URL crackers?  It'll need some tweaks in several
modules
> > but
> > > > if it's possible it sounds like lower modules can just get the final
> cracked
> > > URL
> > > > and do not need to worry about nested mount details.  (I might be
missing
> > some
> > > > devils in detail though)
> > > 
> > > this could be problematic because we use different policy for isolated and
> > > external mount points when checking access permissions.
> > 
> > I thought about that but in other IsolatedFS usage in general creating an
> > isolated FS on top of another FS and giving permission to the isolated FS
> means
> > we give the permission to the particular part scoped by the isolated FS only
> > when it's accessed via isolated FS. (Assuming that creating an isolated FS
is
> > usually only done by an explicit user action and/or via special permission
> given
> > to the app)
> > Does this policy work for your case too or do we need to enforce different
> > security policy?
> > 
> 
> I don't think we should change security policy. I'm just saying that (I
probably
> had misunderstood your initial point a bit) if FSC::CrackFileSystemContextURL
> returned an external file system url in this case, then security check would
> fail for the url, because the permissions for the path are granted through
> IsolatedFS, so we need IsolatedFSURL to verify access permission...

I see, I was mistaken your point.  I think CrackFileSystemContextURL should
always set the given 'type' field to mount_type (so it should always be Isolated
and we should apply permission checks for isolated).  Current code doesn't
handle this case well as you say.

> So what I'm thinking about now is expanding FSC::CrackFileSystemURL to add
some
> additional information about underlying external file system (and use it later
> when getting remote proxy).. but this probably nedds some more thought.

This seems to be the point I had overlooked... Ok now we store the remote proxy
info in the external mount point info so we need the intermediate mount point
info. Does it make sense if we make the base MountPoints code be able to store
RemoteProxy so one could also register a proxy in Isolated case... or could we
amove the proxy info out of mount points again... Hmm I think I need to think
over it too.

> > > Hm, maybe we could add some AsExternalFileSystemURL method to
FileSystemURL,
> > but
> > > that would probably mean more members in FileSystemURL (external_type_,
> > > external_virtual_path_, external_mount_name_)
> > > 
> > > I'll think about this over the weekend..

[kinaba, 2013-03-01 06:12:29]

I've been taking long time to correctly understand the whole view of
the FileSystemURL and permission handling, etc. Now I'm gradually getting the
points.
Let me dump my current understanding, questions, and suggestions.

* Currently, context->CrackURL(url) constructs a FileSystemURL by {
origin : obtained by parse(url),
mount_type : parse(url),
virtual_path : parse(url),
filesystem_id : crack(virtual_path),
type : crack(virtual_path),
path : crack(virtual_path)
};

* FileAPIMessageFilter::HasPermissionsForFile gets a responsible
MountPointProvoder
  by type(), and obtains permission policy from it.

?[question] Cannot this be done by mount_type()? For instance, if
ExternalFilesystem is wrapped by
  a IsolatedFileSystem, the permission is checked inside
ExternalMountPointProvider
  that has special rules for the case mount_type()==Isolated. Looks weird to me.

* After passing the permission check, FileSystemOperation is created by
  MountPointProvider selected by type(). The FileSystemOperation basically
  takes action based on type(), path(), and filesystem_id().

?[question] However, DriveFileSystemProxy uses virtual_path() instead of path().
Why?
  I think the Drive files can correctly identified by using path().
  (Eh, this is where I'm supposed to knowledgable :p. Now investigating...)

If the two questions above are resolved, I think we can clean up the
FileSystemURL
creation by CrackURL as follows: {
// USED FOR PERMISSION CHECKING
origin : obtained by parse(url),
mount_type : parse(url),
virtual_path : parse(url),
mount_filesystem_id : crack(virtual_path),
// USED FOR REAL FILE OPERATIONS
filesystem_id : recursive_crack(virtual_path),
type : recursive_crack(virtual_path),
path : recursive_crack(virtual_path)
};
clarifying the roles of each field, and smoothly supporting nested mounting by
Isolated and External
file systems without maintaining three sets of similar fields.

WDYT?

[kinuko, 2013-03-01 06:56:23]

Thanks for reviving this thread!

On 2013/03/01 06:12:29, kinaba wrote:
> I've been taking long time to correctly understand the whole view of
> the FileSystemURL and permission handling, etc. Now I'm gradually getting the
> points.
> Let me dump my current understanding, questions, and suggestions.
> 
> * Currently, context->CrackURL(url) constructs a FileSystemURL by {
> origin : obtained by parse(url),
> mount_type : parse(url),
> virtual_path : parse(url),
> filesystem_id : crack(virtual_path),
> type : crack(virtual_path),
> path : crack(virtual_path)
> };
> 
> * FileAPIMessageFilter::HasPermissionsForFile gets a responsible
> MountPointProvoder
>   by type(), and obtains permission policy from it.
> 
> ?[question] Cannot this be done by mount_type()? For instance, if
> ExternalFilesystem is wrapped by
>   a IsolatedFileSystem, the permission is checked inside
> ExternalMountPointProvider
>   that has special rules for the case mount_type()==Isolated. Looks weird to
me.
> * After passing the permission check, FileSystemOperation is created by
>   MountPointProvider selected by type(). The FileSystemOperation basically
>   takes action based on type(), path(), and filesystem_id().

Using different mount point providers for security checks and implementation
hasn't come up to me, but I share the point that security check should be always
done based on the top-most 'virtual' part.

> ?[question] However, DriveFileSystemProxy uses virtual_path() instead of
path().
> Why?
>   I think the Drive files can correctly identified by using path().
>   (Eh, this is where I'm supposed to knowledgable :p. Now investigating...)

path() should have implementation-specific path information so in general yes,
and in order to make the nested mounting work I imagine it needs to migrate to
the direction.
(But as far as virtual_path() always reflects the absolute virtual location in
JS filesystem namespace I can imagine some implementation may also want to refer
virtual_path(), say when resolving parent etc)

> If the two questions above are resolved, I think we can clean up the
> FileSystemURL
> creation by CrackURL as follows: {
> // USED FOR PERMISSION CHECKING
> origin : obtained by parse(url),
> mount_type : parse(url),
> virtual_path : parse(url),
> mount_filesystem_id : crack(virtual_path),
> // USED FOR REAL FILE OPERATIONS
> filesystem_id : recursive_crack(virtual_path),
> type : recursive_crack(virtual_path),
> path : recursive_crack(virtual_path)
> };
> clarifying the roles of each field, and smoothly supporting nested mounting by
> Isolated and External
> file systems without maintaining three sets of similar fields.
> WDYT?

I didn't like adding more fields to FileSystemURL but this sounds good!
So we're going to have two filesystem_id fields, one for upper-layer process and
the other for implementation, as well as type and path fields do.

[tonibarzic, 2013-03-01 09:45:54]

On 2013/03/01 06:56:23, kinuko wrote:
> Thanks for reviving this thread!
> 
> On 2013/03/01 06:12:29, kinaba wrote:
> > I've been taking long time to correctly understand the whole view of
> > the FileSystemURL and permission handling, etc. Now I'm gradually getting
the
> > points.
> > Let me dump my current understanding, questions, and suggestions.
> > 
> > * Currently, context->CrackURL(url) constructs a FileSystemURL by {
> > origin : obtained by parse(url),
> > mount_type : parse(url),
> > virtual_path : parse(url),
> > filesystem_id : crack(virtual_path),
> > type : crack(virtual_path),
> > path : crack(virtual_path)
> > };
> > 
> > * FileAPIMessageFilter::HasPermissionsForFile gets a responsible
> > MountPointProvoder
> >   by type(), and obtains permission policy from it.
> > 
> > ?[question] Cannot this be done by mount_type()? For instance, if
> > ExternalFilesystem is wrapped by
> >   a IsolatedFileSystem, the permission is checked inside
> > ExternalMountPointProvider
> >   that has special rules for the case mount_type()==Isolated. Looks weird to
> me.
> > * After passing the permission check, FileSystemOperation is created by
> >   MountPointProvider selected by type(). The FileSystemOperation basically
> >   takes action based on type(), path(), and filesystem_id().
> 
> Using different mount point providers for security checks and implementation
> hasn't come up to me, but I share the point that security check should be
always
> done based on the top-most 'virtual' part.
> 
> > ?[question] However, DriveFileSystemProxy uses virtual_path() instead of
> path().
> > Why?
> >   I think the Drive files can correctly identified by using path().
> >   (Eh, this is where I'm supposed to knowledgable :p. Now investigating...)
> 
> path() should have implementation-specific path information so in general yes,
> and in order to make the nested mounting work I imagine it needs to migrate to
> the direction.
> (But as far as virtual_path() always reflects the absolute virtual location in
> JS filesystem namespace I can imagine some implementation may also want to
refer
> virtual_path(), say when resolving parent etc)
> 

I see this as drive file system internally using paths that are (incidentally)
the same as virtual_paths of external file system urls :) Basically,
DriveFileSystemProxy should take FileSystemURL and translate it to something
DriveFileSystem understands, and FileSystemURL::virtual_path works for external
urls. Obviously, this is not good enough for isolated urls, so I agree
FileSystemURL::path() should be used to get a path usable by  drive file system.


> > If the two questions above are resolved, I think we can clean up the
> > FileSystemURL
> > creation by CrackURL as follows: {
> > // USED FOR PERMISSION CHECKING
> > origin : obtained by parse(url),
> > mount_type : parse(url),
> > virtual_path : parse(url),
> > mount_filesystem_id : crack(virtual_path),
> > // USED FOR REAL FILE OPERATIONS
> > filesystem_id : recursive_crack(virtual_path),
> > type : recursive_crack(virtual_path),
> > path : recursive_crack(virtual_path)
> > };
> > clarifying the roles of each field, and smoothly supporting nested mounting
by
> > Isolated and External
> > file systems without maintaining three sets of similar fields.
> > WDYT?
> 
> I didn't like adding more fields to FileSystemURL but this sounds good!
> So we're going to have two filesystem_id fields, one for upper-layer process
and
> the other for implementation, as well as type and path fields do.

hmm.. we may be over-thinking this.. Essentially, what we want is to be able to
get right remote file system proxy for an isolated filesystem url.. what do you
think about instead of  ExternalMountPoints::GetRemoteFSProxy(mount_point)
introducing ExternalMountPoints::GetRemoteFSProxyForPath(full path).. see the
latest patch

[benwells, 2013-03-04 01:27:39]

https://codereview.chromium.org/12258021/diff/13002/chrome/browser/extensions...
File chrome/browser/extensions/platform_app_launcher.cc (right):

https://codereview.chromium.org/12258021/diff/13002/chrome/browser/extensions...
chrome/browser/extensions/platform_app_launcher.cc:161: void
GetMimeTypeAndLaunchForDriveFile() {
Can this run on any thread? It is currently on UI, if that's where it needs to
be it should have a DCHECK for that. If it can run anywhere maybe a comment
saying it is on the UI thread is warranted.

https://codereview.chromium.org/12258021/diff/13002/chrome/browser/extensions...
chrome/browser/extensions/platform_app_launcher.cc:174: void
OnGotDriveFile(drive::DriveFileError error,
Same question here about threads. This file is pretty confusing with all the
thread bouncing, so anything to help with that is good.

https://codereview.chromium.org/12258021/diff/13002/chrome/browser/extensions...
chrome/browser/extensions/platform_app_launcher.cc:263: file_system_type_,
file_path_, &registered_name);
There will need to be a similar type of addition to the file_system_api code.

[tbarzic, 2013-03-06 01:45:10]

https://codereview.chromium.org/12258021/diff/13002/chrome/browser/extensions...
File chrome/browser/extensions/platform_app_launcher.cc (right):

https://codereview.chromium.org/12258021/diff/13002/chrome/browser/extensions...
chrome/browser/extensions/platform_app_launcher.cc:161: void
GetMimeTypeAndLaunchForDriveFile() {
On 2013/03/04 01:27:39, benwells wrote:
> Can this run on any thread? It is currently on UI, if that's where it needs to
> be it should have a DCHECK for that. If it can run anywhere maybe a comment
> saying it is on the UI thread is warranted.

It _must_ run on UI thread, added DCHECK.

https://codereview.chromium.org/12258021/diff/13002/chrome/browser/extensions...
chrome/browser/extensions/platform_app_launcher.cc:174: void
OnGotDriveFile(drive::DriveFileError error,
On 2013/03/04 01:27:39, benwells wrote:
> Same question here about threads. This file is pretty confusing with all the
> thread bouncing, so anything to help with that is good.

Done.

https://codereview.chromium.org/12258021/diff/13002/chrome/browser/extensions...
chrome/browser/extensions/platform_app_launcher.cc:263: file_system_type_,
file_path_, &registered_name);
On 2013/03/04 01:27:39, benwells wrote:
> There will need to be a similar type of addition to the file_system_api code.

yes, I know.. or fileapi layer should be changed so using
kFileSystemTypeNativeLocal works for all isolated fs' registered here (and
determining the file system type on cros file system is done before/while
executing filesystem operations)

[kinaba, 2013-03-06 02:00:41]

Sorry for belated response (I'm on vacation and will be back on work from
tomorrow)

> hmm.. we may be over-thinking this.. Essentially, what we want is to be able
to
> get right remote file system proxy for an isolated filesystem url..

Beside, we're passing the filesystem url to the remote proxy.
If we don't adjust the fields of the FileSystemURL to the "right" values
and just end up drawing right proxy, the proxy will have to
receive and url object with incorrect .filesystem_id() etc.
Currently the only .path() field is effectively used so there
may be no problem, but it scares me.

https://codereview.chromium.org/12258021/diff/13002/chrome/browser/extensions...
File chrome/browser/extensions/platform_app_launcher.cc (right):

https://codereview.chromium.org/12258021/diff/13002/chrome/browser/extensions...
chrome/browser/extensions/platform_app_launcher.cc:263: file_system_type_,
file_path_, &registered_name);
On 2013/03/06 01:45:10, tbarzic wrote:
> On 2013/03/04 01:27:39, benwells wrote:
> > There will need to be a similar type of addition to the file_system_api
code.
> 
> yes, I know.. or fileapi layer should be changed so using
> kFileSystemTypeNativeLocal works for all isolated fs' registered here (and
> determining the file system type on cros file system is done before/while
> executing filesystem operations)

I feel it a little strange that the client code of IsolatedContext needs to
differentiate Drive and Native paths by itself.
(Or if it needs to specify kFileSystemTypeNativeLocal for Drive paths even.)

Can't we refactor the stuffs to allow fileapi::kFileSystemTypeExternal to be
specified here?
I'm guessing we can if we employ recursive cracking of URL.

[tbarzic, 2013-03-06 02:13:00]

https://codereview.chromium.org/12258021/diff/13002/chrome/browser/extensions...
File chrome/browser/extensions/platform_app_launcher.cc (right):

https://codereview.chromium.org/12258021/diff/13002/chrome/browser/extensions...
chrome/browser/extensions/platform_app_launcher.cc:263: file_system_type_,
file_path_, &registered_name);
On 2013/03/06 02:00:42, kinaba wrote:
> On 2013/03/06 01:45:10, tbarzic wrote:
> > On 2013/03/04 01:27:39, benwells wrote:
> > > There will need to be a similar type of addition to the file_system_api
> code.
> > 
> > yes, I know.. or fileapi layer should be changed so using
> > kFileSystemTypeNativeLocal works for all isolated fs' registered here (and
> > determining the file system type on cros file system is done before/while
> > executing filesystem operations)
> 
> I feel it a little strange that the client code of IsolatedContext needs to
> differentiate Drive and Native paths by itself.
> (Or if it needs to specify kFileSystemTypeNativeLocal for Drive paths even.)
> 
> Can't we refactor the stuffs to allow fileapi::kFileSystemTypeExternal to be
> specified here?
> I'm guessing we can if we employ recursive cracking of URL.

yeah, I agree, that's the biggest issue I have with this.. it seems recursive
cracking on URL is probably the way to go.. I'm a bit busy with some more
pressing bugs, but I think I'll be able to return to this some time next week..

and yeah using kFileSystemNativeLocal here seems a bit confusing, we should
probably use another constant to declare this type (but I don't think
kFileSystemTypeExternal is the best choice either)

[kinaba, 2013-03-14 09:11:54]

On 2013/03/06 02:13:00, tbarzic wrote:
>
https://codereview.chromium.org/12258021/diff/13002/chrome/browser/extensions...
> File chrome/browser/extensions/platform_app_launcher.cc (right):
> 
>
https://codereview.chromium.org/12258021/diff/13002/chrome/browser/extensions...
> chrome/browser/extensions/platform_app_launcher.cc:263: file_system_type_,
> file_path_, &registered_name);
> On 2013/03/06 02:00:42, kinaba wrote:
> > On 2013/03/06 01:45:10, tbarzic wrote:
> > > On 2013/03/04 01:27:39, benwells wrote:
> > > > There will need to be a similar type of addition to the file_system_api
> > code.
> > > 
> > > yes, I know.. or fileapi layer should be changed so using
> > > kFileSystemTypeNativeLocal works for all isolated fs' registered here (and
> > > determining the file system type on cros file system is done before/while
> > > executing filesystem operations)
> > 
> > I feel it a little strange that the client code of IsolatedContext needs to
> > differentiate Drive and Native paths by itself.
> > (Or if it needs to specify kFileSystemTypeNativeLocal for Drive paths even.)
> > 
> > Can't we refactor the stuffs to allow fileapi::kFileSystemTypeExternal to be
> > specified here?
> > I'm guessing we can if we employ recursive cracking of URL.
> 
> yeah, I agree, that's the biggest issue I have with this.. it seems recursive
> cracking on URL is probably the way to go.. I'm a bit busy with some more
> pressing bugs, but I think I'll be able to return to this some time next
week..
> 
> and yeah using kFileSystemNativeLocal here seems a bit confusing, we should
> probably use another constant to declare this type (but I don't think
> kFileSystemTypeExternal is the best choice either)

Toni, we've been asked to raise priority for this task.
https://code.google.com/p/chromium/issues/detail?id=161715#c13
Do you have time to spare now? I'd be happy to turn over (along the
line of adding recursive CrackURL).

[tbarzic, 2013-03-14 23:05:03]

On 2013/03/14 09:11:54, kinaba wrote:
>
>  ...
> 
> Toni, we've been asked to raise priority for this task.
> https://code.google.com/p/chromium/issues/detail?id=161715#c13
> Do you have time to spare now? I'd be happy to turn over (along the
> line of adding recursive CrackURL).

Hey, I've made some progress this week, but unfortunately, I'll have to drop
this for the next week or two (I meant to send you a note, but you beat me to
it), so feel free to take over.
I've uploaded the latest progress.. it still needs tests and some cleanup, but
in general, I think the approach should work..

[kinuko, 2013-03-15 06:54:39]

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

https://codereview.chromium.org/12258021/diff/36002/webkit/fileapi/file_syste...
webkit/fileapi/file_system_url.h:175: base::FilePath underlying_virtual_path_;
Hmm, do we need all these additional params? If so I feel something must be
wrong...

[tonibarzic, 2013-03-15 07:36:57]

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

https://codereview.chromium.org/12258021/diff/36002/webkit/fileapi/file_syste...
webkit/fileapi/file_system_url.h:175: base::FilePath underlying_virtual_path_;
On 2013/03/15 06:54:39, kinuko wrote:
> Hmm, do we need all these additional params? If so I feel something must be
> wrong...

we could do without underlaying virtual path, and use virtual_path_ url
(actually, we probably should use that path, as it represents part of the
original url). but, file system operations should be changed to use only path()
when using the file.

also, instead of has_underlying_url_ == true we could use underlying_type_ !=
kFileSystemTypeUnknown

[kinuko, 2013-03-17 21:39:09]

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

https://codereview.chromium.org/12258021/diff/36002/webkit/fileapi/file_syste...
webkit/fileapi/file_system_url.h:175: base::FilePath underlying_virtual_path_;
On 2013/03/15 07:36:57, tonibarzic wrote:
> On 2013/03/15 06:54:39, kinuko wrote:
> > Hmm, do we need all these additional params? If so I feel something must be
> > wrong...
> 
> we could do without underlaying virtual path, and use virtual_path_ url
> (actually, we probably should use that path, as it represents part of the
> original url). but, file system operations should be changed to use only
path()
> when using the file.

So are these changes basically for not changing the existing implementation
(which relies on virtual_path) for now?   If so how about if we make a quick fix
like this:
https://codereview.chromium.org/12483010/

The fact that internal implementation requires its special virtual path is not
necessarily an issue of File API layer, and if that's the only problem a simple
implementation-specific conversion like above seems to work. (Pls correct me if
I'm still missing something)

> also, instead of has_underlying_url_ == true we could use underlying_type_ !=
> kFileSystemTypeUnknown
>

[kinaba, 2013-03-18 00:17:29]

On 2013/03/17 21:39:09, kinuko wrote:
> So are these changes basically for not changing the existing implementation
> (which relies on virtual_path) for now?   If so how about if we make a quick
fix
> like this:
> https://codereview.chromium.org/12483010/

I feel this is the right approach (internal file operations should use path(),
not virtual_path()).

[kinaba, 2013-03-19 10:14:59]

(Patch Set 10 : mmm, sorry it seems I've uploaded my local patched branch here
unexpectedly.
Please ignore it. I did't know even it is possible...)

[kinaba, 2013-03-27 01:59:01]

Took over and landed in https://codereview.chromium.org/12717014/.
Thanks for your work and investigation!