Stop returning the true root path of each filesystem from openFileSystem....

Stop returning the true root path of each filesystem from openFileSystem.
Instead, return the FileSystem URI of the root.  This will make it easier
to swap in different filesystem implementations.

BUG=71635
TEST=Just a couple in FileSystemUtilTests, but a bunch of existing ones [this doesn't add much new functionality].
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=79228

[ericu, 2011-03-05 01:42:40]

Hey all--as you can see, there are a lot of TODO(ericu) marks in this, but I
think they can be taken care of in separate changelists.  Everything seems to
work, including unit tests [with a bit of hacking], and I think this should move
us toward being able to slot in other filesystems soon.

Please let me know what you think.

[ericu, 2011-03-05 01:54:47]

I was missing a file; it's there now.

On Fri, Mar 4, 2011 at 5:42 PM,  <ericu@chromium.org> wrote:
> Reviewers: kinuko, Dai Mikurube,
>
> Message:
> Hey all--as you can see, there are a lot of TODO(ericu) marks in this, but I
> think they can be taken care of in separate changelists.  Everything seems
> to
> work, including unit tests [with a bit of hacking], and I think this should
> move
> us toward being able to slot in other filesystems soon.
>
> Please let me know what you think.
>
> Description:
> Stop returning the true root path of each filesystem from openFileSystem.
> Instead, return the FileSystem URI of the root.  This will make it easier
> to swap in different filesystem implementations.
>
> BUG=71635
> TEST=Just a couple in FileSystemUtilTests, but a bunch of existing ones
> [this
> doesn't add much new functionality].
>
>
> Please review this at http://codereview.chromium.org/6603034/
>
> SVN Base: svn://chrome-svn/chrome/trunk/src/
>
> Affected files:
>  M     webkit/fileapi/file_system_file_util.h
>  M     webkit/fileapi/file_system_file_util.cc
>  M     webkit/fileapi/file_system_operation.h
>  M     webkit/fileapi/file_system_operation.cc
>  M     webkit/fileapi/file_system_operation_context.h
>  M     webkit/fileapi/file_system_path_manager.h
>  M     webkit/fileapi/file_system_path_manager.cc
>  M     webkit/fileapi/file_system_path_manager_unittest.cc
>  M     webkit/fileapi/file_system_util.h
>  M     webkit/fileapi/file_system_util.cc
>  M     webkit/fileapi/file_system_util_unittest.cc
>  M     webkit/fileapi/webkit_fileapi.gypi
>
>
>

[kinuko, 2011-03-05 02:22:34]

At a quick glance it looks neat; but I'm not sure if we have strong reasons
to switch the place where we perform simple/generic path translation.  We
are doing it before calling into FILE thread.  Do we need to change that?

The reasons I stick to the current architecture (virtual path or URL crack
-> resolve type and path -> calling FSFU operations on FILE thread):

As in your comment we need to crack the path (URL) first to get a fs type,
origin and appropriate FileSystemFileUtil instance before calling to the
FILE method.  For filesystems (e.g. local fs) for which we don't need any
intricate path obfuscation, we can resolve the paths there (using a
mount-point path provider for "local" filesystem) and continue to use the
base FSFU operations as is.

For our transitional temporary/persistent filesystems, we could use a
transitional mount-point path provider, which returns our familiar long root
path + virtual path, and again we can continue to use the same base FSFU
operations.

When we switch the current fs to DB-based one, the path
translation/obfuscation would need to be more integrated into each
operation, and it would be realized as one of the FSFU implementations -- so
I assume we won't use this GetLocalPath for our purpose.  When we enable the
feature we could switch the FSFU operations we use to the new DB-based one,
and also switch the transitional mount-point path provider to a new one,
which does almost nothing for path resolution.

Overall I think there would be less changes and also we could utilize zel's
provider idea almost as is.

Wdyt?


On Fri, Mar 4, 2011 at 5:42 PM, <ericu@chromium.org> wrote:

> Reviewers: kinuko, Dai Mikurube,
>
> Message:
> Hey all--as you can see, there are a lot of TODO(ericu) marks in this, but
> I
> think they can be taken care of in separate changelists.  Everything seems
> to
> work, including unit tests [with a bit of hacking], and I think this should
> move
> us toward being able to slot in other filesystems soon.
>
> Please let me know what you think.
>
> Description:
> Stop returning the true root path of each filesystem from openFileSystem.
> Instead, return the FileSystem URI of the root.  This will make it easier
> to swap in different filesystem implementations.
>
> BUG=71635
> TEST=Just a couple in FileSystemUtilTests, but a bunch of existing ones
> [this
> doesn't add much new functionality].
>
>
> Please review this at http://codereview.chromium.org/6603034/
>
> SVN Base: svn://chrome-svn/chrome/trunk/src/
>
> Affected files:
>  M     webkit/fileapi/file_system_file_util.h
>  M     webkit/fileapi/file_system_file_util.cc
>  M     webkit/fileapi/file_system_operation.h
>  M     webkit/fileapi/file_system_operation.cc
>  M     webkit/fileapi/file_system_operation_context.h
>  M     webkit/fileapi/file_system_path_manager.h
>  M     webkit/fileapi/file_system_path_manager.cc
>  M     webkit/fileapi/file_system_path_manager_unittest.cc
>  M     webkit/fileapi/file_system_util.h
>  M     webkit/fileapi/file_system_util.cc
>  M     webkit/fileapi/file_system_util_unittest.cc
>  M     webkit/fileapi/webkit_fileapi.gypi
>
>
>

[ericu, 2011-03-07 02:50:29]

On Fri, Mar 4, 2011 at 6:22 PM, Kinuko Yasuda <kinuko@chromium.org> wrote:
> At a quick glance it looks neat; but I'm not sure if we have strong reasons
> to switch the place where we perform simple/generic path translation.  We
> are doing it before calling into FILE thread.  Do we need to change that?

Well, we'll need to for the DB, since it will require a database read.
 And if we're going to need to push it down there for one filesystem
type, it seems a lot simpler and cleaner to me to do it for every
filesystem type.  That way there's no special-casing, everybody just
uses the same interfaces, and we only really need to do that kind of
thing in a single place.  It also means that in between the top layer
[where I think you're suggesting doing the local filesystem
translation] and the FSFU, we're always passing the same data type.
Otherwise we'd have a variable that would hold a real local path some
of the time, and a URL some of the time, and I think that might be
error-prone.

> The reasons I stick to the current architecture (virtual path or URL crack
> -> resolve type and path -> calling FSFU operations on FILE thread):
> As in your comment we need to crack the path (URL) first to get a fs type,
> origin and appropriate FileSystemFileUtil instance before calling to the
> FILE method.  For filesystems (e.g. local fs) for which we don't need any
> intricate path obfuscation, we can resolve the paths there (using a
> mount-point path provider for "local" filesystem) and continue to use the
> base FSFU operations as is.

The code where I made that comment could also be cleaned up just by
passing the type through along with the context, I think.  But it's
also not a really big deal--I just wanted to remember to revisit it
because I think I can do better.

> For our transitional temporary/persistent filesystems, we could use a
> transitional mount-point path provider, which returns our familiar long root
> path + virtual path, and again we can continue to use the same base FSFU
> operations.
> When we switch the current fs to DB-based one, the path
> translation/obfuscation would need to be more integrated into each
> operation, and it would be realized as one of the FSFU implementations -- so
> I assume we won't use this GetLocalPath for our purpose.  When we enable the
> feature we could switch the FSFU operations we use to the new DB-based one,
> and also switch the transitional mount-point path provider to a new one,
> which does almost nothing for path resolution.
> Overall I think there would be less changes and also we could utilize zel's
> provider idea almost as is.
> Wdyt?

There would certainly be less in the way of changes for the local
filesystem, but I think we'd need just as many for the DB filesystem.
And then, since we'd have to maintain both the translate-at-the-top
code and the translate-for-DB code, I think it would end up being more
code overall.  My goal is to get all the hard plumbing out of the way
here, so that adding additionaly filesystem layers [quota +DB] and
instances [extensions/ChromeOS, <input type=file directory>, etc.] is
as simple as possible.  Ideally, when we get around to things like
<input type=file directory>, it would only require adding an object
with a method to determine access yes/no, and another to supply the
URL->real-path translation.

> On Fri, Mar 4, 2011 at 5:42 PM, <ericu@chromium.org> wrote:
>>
>> Reviewers: kinuko, Dai Mikurube,
>>
>> Message:
>> Hey all--as you can see, there are a lot of TODO(ericu) marks in this, but
>> I
>> think they can be taken care of in separate changelists.  Everything seems
>> to
>> work, including unit tests [with a bit of hacking], and I think this
>> should move
>> us toward being able to slot in other filesystems soon.
>>
>> Please let me know what you think.
>>
>> Description:
>> Stop returning the true root path of each filesystem from openFileSystem.
>> Instead, return the FileSystem URI of the root.  This will make it easier
>> to swap in different filesystem implementations.
>>
>> BUG=71635
>> TEST=Just a couple in FileSystemUtilTests, but a bunch of existing ones
>> [this
>> doesn't add much new functionality].
>>
>>
>> Please review this at http://codereview.chromium.org/6603034/
>>
>> SVN Base: svn://chrome-svn/chrome/trunk/src/
>>
>> Affected files:
>>  M     webkit/fileapi/file_system_file_util.h
>>  M     webkit/fileapi/file_system_file_util.cc
>>  M     webkit/fileapi/file_system_operation.h
>>  M     webkit/fileapi/file_system_operation.cc
>>  M     webkit/fileapi/file_system_operation_context.h
>>  M     webkit/fileapi/file_system_path_manager.h
>>  M     webkit/fileapi/file_system_path_manager.cc
>>  M     webkit/fileapi/file_system_path_manager_unittest.cc
>>  M     webkit/fileapi/file_system_util.h
>>  M     webkit/fileapi/file_system_util.cc
>>  M     webkit/fileapi/file_system_util_unittest.cc
>>  M     webkit/fileapi/webkit_fileapi.gypi
>>
>>
>
>

[kinuko, 2011-03-07 07:46:30]

All I'd like to see is a simple architecture with less changes, and I
believe we share the same goal for those parts.  So let's just focus on what
we want to achieve.

All we want to for each operation is:

1. crack the given input URL
2. get the type, origin and virtual_path (the local 'path' part of the given
URL)
3. pass the tuple information to each FSFU implementation

Where:
a. each FSFU implementation may have its own namespace where an entry can be
identified by a local part of the URL (some implementation may also need
'type' and 'origin' to resolve the local path).
b. some FSFU implementation just maps a file path to another native file
path, but resolving the path may need DB or some local file access.

Correct?  I added b. from your response.

Then I would imagine something like this:
At step 2. we can make up an operation context that includes 'type' and
'origin'.
We could add a thin FSFU layer, let's say we call it LocalFSFU, that just
prepends its own 'root path' to the 'local path' and call into the base FSFU
operations (probably the operation context could contain additional 'root
path' entry for the FSFU), and could add the layer to any filesystems which
needs such prefix translation.  (Please see below for more discussion on
this)

On Sun, Mar 6, 2011 at 6:50 PM, Eric Uhrhane <ericu@chromium.org> wrote:

> On Fri, Mar 4, 2011 at 6:22 PM, Kinuko Yasuda <kinuko@chromium.org> wrote:
> > At a quick glance it looks neat; but I'm not sure if we have strong
> reasons
> > to switch the place where we perform simple/generic path translation.  We
> > are doing it before calling into FILE thread.  Do we need to change that?
>
> Well, we'll need to for the DB, since it will require a database read.
>  And if we're going to need to push it down there for one filesystem
> type, it seems a lot simpler and cleaner to me to do it for every
> filesystem type.  That way there's no special-casing, everybody just
> uses the same interfaces, and we only really need to do that kind of
> thing in a single place.

 It also means that in between the top layer
> [where I think you're suggesting doing the local filesystem
> translation] and the FSFU, we're always passing the same data type.
> Otherwise we'd have a variable that would hold a real local path some
> of the time, and a URL some of the time, and I think that might be
> error-prone.


Once we crack the URL we could simply pass a tuple of { 'type', 'origin',
'virtual_path' } couldn't we?
If we change all FSFU operations to take URL (GURL?) instead of FilePath, we
need the GURL -> FilePath conversion at each layer, where some or most of
layers may just want to focus on their local operations.

> The reasons I stick to the current architecture (virtual path or URL crack
> > -> resolve type and path -> calling FSFU operations on FILE thread):
> > As in your comment we need to crack the path (URL) first to get a fs
> type,
> > origin and appropriate FileSystemFileUtil instance before calling to the
> > FILE method.  For filesystems (e.g. local fs) for which we don't need any
> > intricate path obfuscation, we can resolve the paths there (using a
> > mount-point path provider for "local" filesystem) and continue to use the
> > base FSFU operations as is.
>
> The code where I made that comment could also be cleaned up just by
> passing the type through along with the context, I think.  But it's
> also not a really big deal--I just wanted to remember to revisit it
> because I think I can do better.


It's not a big deal, that's true.  The point I thought was if we have some
hierarchy / layered structure in a namespace, in general we should be able
to strip outer/upper layer information before we pass the name to the down
path.   Of course this cannot be applied all the time in the real coding
world, and sometime we just need to take some compromised alternatives.


> > For our transitional temporary/persistent filesystems, we could use a
> > transitional mount-point path provider, which returns our familiar long
> root
> > path + virtual path, and again we can continue to use the same base FSFU
> > operations.
> > When we switch the current fs to DB-based one, the path
> > translation/obfuscation would need to be more integrated into each
> > operation, and it would be realized as one of the FSFU implementations --
> so
> > I assume we won't use this GetLocalPath for our purpose.  When we enable
> the
> > feature we could switch the FSFU operations we use to the new DB-based
> one,
> > and also switch the transitional mount-point path provider to a new one,
> > which does almost nothing for path resolution.
> > Overall I think there would be less changes and also we could utilize
> zel's
> > provider idea almost as is.
> > Wdyt?
>
> There would certainly be less in the way of changes for the local
> filesystem, but I think we'd need just as many for the DB filesystem.
> And then, since we'd have to maintain both the translate-at-the-top
> code and the translate-for-DB code, I think it would end up being more
> code overall.


By calling GetLocalPath() in the current way all we can do is a prefix
conversion, i.e. prepending each filesystem's own root path.
And in the sense I am not fully convinced if we could merge
'translate-at-top' and 'translate-for-DB' into a single clean location.
They are doing very different level of path name translations imho, and
should be handled separately.

Also having a unified conversion point in the base FSFU implementation makes
me worry how we could make sure how and when the conversion is made in each
FSFU wiring/implementation.
Some FSFU implementation may or may not override some of the methods, and
may or may not call into underlying FSFU layer.  Some upper layer FSFU
implementation may decide to directly call file_util operations.

If we really need to do the conversion in FILE thread, I would introduce
another thin layer that performs the 'local path' to 'real path'
translation, just only for local filesystem or other filesystems where every
entry name of the local path are completely preserved in their namespace,
and would not use the layer for DB based implementation.


>  My goal is to get all the hard plumbing out of the way

here, so that adding additionaly filesystem layers [quota +DB] and
> instances [extensions/ChromeOS, <input type=file directory>, etc.] is
> as simple as possible.  Ideally, when we get around to things like
> <input type=file directory>, it would only require adding an object
> with a method to determine access yes/no, and another to supply the
> URL->real-path translation.
>
> > On Fri, Mar 4, 2011 at 5:42 PM, <ericu@chromium.org> wrote:
> >>
> >> Reviewers: kinuko, Dai Mikurube,
> >>
> >> Message:
> >> Hey all--as you can see, there are a lot of TODO(ericu) marks in this,
> but
> >> I
> >> think they can be taken care of in separate changelists.  Everything
> seems
> >> to
> >> work, including unit tests [with a bit of hacking], and I think this
> >> should move
> >> us toward being able to slot in other filesystems soon.
> >>
> >> Please let me know what you think.
> >>
> >> Description:
> >> Stop returning the true root path of each filesystem from
> openFileSystem.
> >> Instead, return the FileSystem URI of the root.  This will make it
> easier
> >> to swap in different filesystem implementations.
> >>
> >> BUG=71635
> >> TEST=Just a couple in FileSystemUtilTests, but a bunch of existing ones
> >> [this
> >> doesn't add much new functionality].
> >>
> >>
> >> Please review this at http://codereview.chromium.org/6603034/
> >>
> >> SVN Base: svn://chrome-svn/chrome/trunk/src/
> >>
> >> Affected files:
> >>  M     webkit/fileapi/file_system_file_util.h
> >>  M     webkit/fileapi/file_system_file_util.cc
> >>  M     webkit/fileapi/file_system_operation.h
> >>  M     webkit/fileapi/file_system_operation.cc
> >>  M     webkit/fileapi/file_system_operation_context.h
> >>  M     webkit/fileapi/file_system_path_manager.h
> >>  M     webkit/fileapi/file_system_path_manager.cc
> >>  M     webkit/fileapi/file_system_path_manager_unittest.cc
> >>  M     webkit/fileapi/file_system_util.h
> >>  M     webkit/fileapi/file_system_util.cc
> >>  M     webkit/fileapi/file_system_util_unittest.cc
> >>  M     webkit/fileapi/webkit_fileapi.gypi
> >>
> >>
> >
> >
>

[ericu, 2011-03-07 20:17:54]

On Sun, Mar 6, 2011 at 11:46 PM, Kinuko Yasuda <kinuko@chromium.org> wrote:
> All I'd like to see is a simple architecture with less changes, and I
> believe we share the same goal for those parts.  So let's just focus on what
> we want to achieve.

Sounds good.  However, I think there may be a bit of a tradeoff
between "less changes" and "simple architecture".  Still, where
there's a tradeoff, there are often multiple solutions that make
sense, and we can just figure out which one seems best.

> All we want to for each operation is:
> 1. crack the given input URL
> 2. get the type, origin and virtual_path (the local 'path' part of the given
> URL)
> 3. pass the tuple information to each FSFU implementation
> Where:
> a. each FSFU implementation may have its own namespace where an entry can be
> identified by a local part of the URL (some implementation may also need
> 'type' and 'origin' to resolve the local path).
> b. some FSFU implementation just maps a file path to another native file
> path, but resolving the path may need DB or some local file access.
> Correct?  I added b. from your response.

Right.

> Then I would imagine something like this:
> At step 2. we can make up an operation context that includes 'type' and
> 'origin'.
> We could add a thin FSFU layer, let's say we call it LocalFSFU, that just
> prepends its own 'root path' to the 'local path' and call into the base FSFU
> operations (probably the operation context could contain additional 'root
> path' entry for the FSFU), and could add the layer to any filesystems which
> needs such prefix translation.  (Please see below for more discussion on
> this)

Is FSFU pretty much LocalFSFU already?  I guess it's still going to
have to call out to get the mapping of the root path.  There's going
to have to be some object that handles that part, and it can't be
LFSFU if that's a singleton.

> On Sun, Mar 6, 2011 at 6:50 PM, Eric Uhrhane <ericu@chromium.org> wrote:
>>
>> On Fri, Mar 4, 2011 at 6:22 PM, Kinuko Yasuda <kinuko@chromium.org> wrote:
>> > At a quick glance it looks neat; but I'm not sure if we have strong
>> > reasons
>> > to switch the place where we perform simple/generic path translation.
>> >  We
>> > are doing it before calling into FILE thread.  Do we need to change
>> > that?
>>
>> Well, we'll need to for the DB, since it will require a database read.
>>  And if we're going to need to push it down there for one filesystem
>> type, it seems a lot simpler and cleaner to me to do it for every
>> filesystem type.  That way there's no special-casing, everybody just
>> uses the same interfaces, and we only really need to do that kind of
>> thing in a single place.
>>
>>  It also means that in between the top layer
>> [where I think you're suggesting doing the local filesystem
>> translation] and the FSFU, we're always passing the same data type.
>> Otherwise we'd have a variable that would hold a real local path some
>> of the time, and a URL some of the time, and I think that might be
>> error-prone.
>
> Once we crack the URL we could simply pass a tuple of { 'type', 'origin',
> 'virtual_path' } couldn't we?

Yes, absolutely.  We can just pass that all the way through instead of
cracking multiple times.  The key point is that the type is needed to
look up the proper FSFU subclass, but is also needed in the sandbox to
determine the full root.  I was cracking once for each need, but this
would enable us to crack just once.

> If we change all FSFU operations to take URL (GURL?) instead of FilePath, we
> need the GURL -> FilePath conversion at each layer, where some or most of
> layers may just want to focus on their local operations.

I think we have 2 choices here that would eliminate having to do two
conversions.  Both ways we'd just do it at a low level, inside the
FSFU subclass.  Either we can pass the path tuple you suggested all
the way in (and create the path inside the FSFU), or we can generate
the root path [but not the full path] at the top level, and pass
{root, type, virtual_path} in, and use that in each FSFU.  Either way,
we only process the URL once.  We know that for any anticipated
filesystem type, we're going to need a root path of some sort, so we
don't have to do anything more general than this.

>> > The reasons I stick to the current architecture (virtual path or URL
>> > crack
>> > -> resolve type and path -> calling FSFU operations on FILE thread):
>> > As in your comment we need to crack the path (URL) first to get a fs
>> > type,
>> > origin and appropriate FileSystemFileUtil instance before calling to the
>> > FILE method.  For filesystems (e.g. local fs) for which we don't need
>> > any
>> > intricate path obfuscation, we can resolve the paths there (using a
>> > mount-point path provider for "local" filesystem) and continue to use
>> > the
>> > base FSFU operations as is.
>>
>> The code where I made that comment could also be cleaned up just by
>> passing the type through along with the context, I think.  But it's
>> also not a really big deal--I just wanted to remember to revisit it
>> because I think I can do better.
>
> It's not a big deal, that's true.  The point I thought was if we have some
> hierarchy / layered structure in a namespace, in general we should be able
> to strip outer/upper layer information before we pass the name to the down
> path.   Of course this cannot be applied all the time in the real coding
> world, and sometime we just need to take some compromised alternatives.

Agreed.  I think creating the tuple is a clean way to do this, as it
would still work identically for each filesystem type.  I really want
to avoid stripping of hierarchy only for some subtypes at one level,
and then handling the others at a different level.  It feels like we'd
be adding a bunch of special cases and cluttering up what could be
simpler code.

>> > For our transitional temporary/persistent filesystems, we could use a
>> > transitional mount-point path provider, which returns our familiar long
>> > root
>> > path + virtual path, and again we can continue to use the same base FSFU
>> > operations.
>> > When we switch the current fs to DB-based one, the path
>> > translation/obfuscation would need to be more integrated into each
>> > operation, and it would be realized as one of the FSFU implementations
>> > -- so
>> > I assume we won't use this GetLocalPath for our purpose.  When we enable
>> > the
>> > feature we could switch the FSFU operations we use to the new DB-based
>> > one,
>> > and also switch the transitional mount-point path provider to a new one,
>> > which does almost nothing for path resolution.
>> > Overall I think there would be less changes and also we could utilize
>> > zel's
>> > provider idea almost as is.
>> > Wdyt?
>>
>> There would certainly be less in the way of changes for the local
>> filesystem, but I think we'd need just as many for the DB filesystem.
>> And then, since we'd have to maintain both the translate-at-the-top
>> code and the translate-for-DB code, I think it would end up being more
>> code overall.
>
> By calling GetLocalPath() in the current way all we can do is a prefix
> conversion, i.e. prepending each filesystem's own root path.
> And in the sense I am not fully convinced if we could merge
> 'translate-at-top' and 'translate-for-DB' into a single clean location.
> They are doing very different level of path name translations imho, and
> should be handled separately.

We agree on that...

> Also having a unified conversion point in the base FSFU implementation makes
> me worry how we could make sure how and when the conversion is made in each
> FSFU wiring/implementation.

Here I'm less sure what you mean.  I expect that the base FSFU will be
used only for the current sandbox implementation and the Local
filesystem.  A derived class will handle the DB filesystem, and will
override or not use the base class's conversion.

> Some FSFU implementation may or may not override some of the methods, and
> may or may not call into underlying FSFU layer.  Some upper layer FSFU
> implementation may decide to directly call file_util operations.

Agreed.  The current implementation does that, and I don't expect to change it.

> If we really need to do the conversion in FILE thread, I would introduce
> another thin layer that performs the 'local path' to 'real path'
> translation, just only for local filesystem or other filesystems where every
> entry name of the local path are completely preserved in their namespace,
> and would not use the layer for DB based implementation.

Here's where I get confused.  The local->real path translation, that's
only needed by local filesystems, should be hidden away in code that's
only used by local filesystems.  The easy way to hide code that's only
needed by one filesystem type is to put it in the FSFU subclass used
by that type.  In this case it's the base FSFU, which is designed to
be used by local filesystems.  Why would we add another layer?

Is it possible that we're saying the same thing in different words?
We seem to agree on quite a bit.

Aha!  OK, on rereading this, I think I may be understanding you now.
Tell me if this is it:

I'm suggesting that FSFU call out to some kind of a delegate that will
do path translation to get the root, and you're suggesting that we put
a wrapper around FSFU that will do path translation, then call into
it.  Have I got that right?

>>  My goal is to get all the hard plumbing out of the way
>>
>> here, so that adding additionaly filesystem layers [quota +DB] and
>> instances [extensions/ChromeOS, <input type=file directory>, etc.] is
>> as simple as possible.  Ideally, when we get around to things like
>> <input type=file directory>, it would only require adding an object
>> with a method to determine access yes/no, and another to supply the
>> URL->real-path translation.
>>
>> > On Fri, Mar 4, 2011 at 5:42 PM, <ericu@chromium.org> wrote:
>> >>
>> >> Reviewers: kinuko, Dai Mikurube,
>> >>
>> >> Message:
>> >> Hey all--as you can see, there are a lot of TODO(ericu) marks in this,
>> >> but
>> >> I
>> >> think they can be taken care of in separate changelists.  Everything
>> >> seems
>> >> to
>> >> work, including unit tests [with a bit of hacking], and I think this
>> >> should move
>> >> us toward being able to slot in other filesystems soon.
>> >>
>> >> Please let me know what you think.
>> >>
>> >> Description:
>> >> Stop returning the true root path of each filesystem from
>> >> openFileSystem.
>> >> Instead, return the FileSystem URI of the root.  This will make it
>> >> easier
>> >> to swap in different filesystem implementations.
>> >>
>> >> BUG=71635
>> >> TEST=Just a couple in FileSystemUtilTests, but a bunch of existing ones
>> >> [this
>> >> doesn't add much new functionality].
>> >>
>> >>
>> >> Please review this at http://codereview.chromium.org/6603034/
>> >>
>> >> SVN Base: svn://chrome-svn/chrome/trunk/src/
>> >>
>> >> Affected files:
>> >>  M     webkit/fileapi/file_system_file_util.h
>> >>  M     webkit/fileapi/file_system_file_util.cc
>> >>  M     webkit/fileapi/file_system_operation.h
>> >>  M     webkit/fileapi/file_system_operation.cc
>> >>  M     webkit/fileapi/file_system_operation_context.h
>> >>  M     webkit/fileapi/file_system_path_manager.h
>> >>  M     webkit/fileapi/file_system_path_manager.cc
>> >>  M     webkit/fileapi/file_system_path_manager_unittest.cc
>> >>  M     webkit/fileapi/file_system_util.h
>> >>  M     webkit/fileapi/file_system_util.cc
>> >>  M     webkit/fileapi/file_system_util_unittest.cc
>> >>  M     webkit/fileapi/webkit_fileapi.gypi
>> >>
>> >>
>> >
>> >
>
>

[kinuko, 2011-03-08 01:28:35]

(Sorry for my late response, I lost my half-written response due to
XSRF token timeout...)

On Mon, Mar 7, 2011 at 12:17 PM, Eric Uhrhane <ericu@chromium.org> wrote:
> On Sun, Mar 6, 2011 at 11:46 PM, Kinuko Yasuda <kinuko@chromium.org> wrote:
>> All I'd like to see is a simple architecture with less changes, and I
>> believe we share the same goal for those parts.  So let's just focus on what
>> we want to achieve.
>
> Sounds good.  However, I think there may be a bit of a tradeoff
> between "less changes" and "simple architecture".  Still, where
> there's a tradeoff, there are often multiple solutions that make
> sense, and we can just figure out which one seems best.
>
>> All we want to for each operation is:
>> 1. crack the given input URL
>> 2. get the type, origin and virtual_path (the local 'path' part of the given
>> URL)
>> 3. pass the tuple information to each FSFU implementation
>> Where:
>> a. each FSFU implementation may have its own namespace where an entry can be
>> identified by a local part of the URL (some implementation may also need
>> 'type' and 'origin' to resolve the local path).
>> b. some FSFU implementation just maps a file path to another native file
>> path, but resolving the path may need DB or some local file access.
>> Correct?  I added b. from your response.
>
> Right.
>
>> Then I would imagine something like this:
>> At step 2. we can make up an operation context that includes 'type' and
>> 'origin'.
>> We could add a thin FSFU layer, let's say we call it LocalFSFU, that just
>> prepends its own 'root path' to the 'local path' and call into the base FSFU
>> operations (probably the operation context could contain additional 'root
>> path' entry for the FSFU), and could add the layer to any filesystems which
>> needs such prefix translation.  (Please see below for more discussion on
>> this)
>
> Is FSFU pretty much LocalFSFU already?  I guess it's still going to
> have to call out to get the mapping of the root path.  There's going
> to have to be some object that handles that part, and it can't be
> LFSFU if that's a singleton.
>
>> On Sun, Mar 6, 2011 at 6:50 PM, Eric Uhrhane <ericu@chromium.org> wrote:
>>>
>>> On Fri, Mar 4, 2011 at 6:22 PM, Kinuko Yasuda <kinuko@chromium.org> wrote:
>>> > At a quick glance it looks neat; but I'm not sure if we have strong
>>> > reasons
>>> > to switch the place where we perform simple/generic path translation.
>>> >  We
>>> > are doing it before calling into FILE thread.  Do we need to change
>>> > that?
>>>
>>> Well, we'll need to for the DB, since it will require a database read.
>>>  And if we're going to need to push it down there for one filesystem
>>> type, it seems a lot simpler and cleaner to me to do it for every
>>> filesystem type.  That way there's no special-casing, everybody just
>>> uses the same interfaces, and we only really need to do that kind of
>>> thing in a single place.
>>>
>>>  It also means that in between the top layer
>>> [where I think you're suggesting doing the local filesystem
>>> translation] and the FSFU, we're always passing the same data type.
>>> Otherwise we'd have a variable that would hold a real local path some
>>> of the time, and a URL some of the time, and I think that might be
>>> error-prone.
>>
>> Once we crack the URL we could simply pass a tuple of { 'type', 'origin',
>> 'virtual_path' } couldn't we?
>
> Yes, absolutely.  We can just pass that all the way through instead of
> cracking multiple times.  The key point is that the type is needed to
> look up the proper FSFU subclass, but is also needed in the sandbox to
> determine the full root.  I was cracking once for each need, but this
> would enable us to crack just once.
>
>> If we change all FSFU operations to take URL (GURL?) instead of FilePath, we
>> need the GURL -> FilePath conversion at each layer, where some or most of
>> layers may just want to focus on their local operations.
>
> I think we have 2 choices here that would eliminate having to do two
> conversions.  Both ways we'd just do it at a low level, inside the
> FSFU subclass.  Either we can pass the path tuple you suggested all
> the way in (and create the path inside the FSFU), or we can generate
> the root path [but not the full path] at the top level, and pass
> {root, type, virtual_path} in, and use that in each FSFU.  Either way,
> we only process the URL once.  We know that for any anticipated
> filesystem type, we're going to need a root path of some sort, so we
> don't have to do anything more general than this.

Either one would work... to be sure, I assumed the 'virtual_path' part
might vary when an operation goes across layers, e.g. for the FSFU
implementations placed below the DB-based layer the original
'virtual_path' will have no meanings, and instead they will need to be
given a new virtual_path which is an actual relative path to the given
root path.

>>> > The reasons I stick to the current architecture (virtual path or URL
>>> > crack
>>> > -> resolve type and path -> calling FSFU operations on FILE thread):
>>> > As in your comment we need to crack the path (URL) first to get a fs
>>> > type,
>>> > origin and appropriate FileSystemFileUtil instance before calling to the
>>> > FILE method.  For filesystems (e.g. local fs) for which we don't need
>>> > any
>>> > intricate path obfuscation, we can resolve the paths there (using a
>>> > mount-point path provider for "local" filesystem) and continue to use
>>> > the
>>> > base FSFU operations as is.
>>>
>>> The code where I made that comment could also be cleaned up just by
>>> passing the type through along with the context, I think.  But it's
>>> also not a really big deal--I just wanted to remember to revisit it
>>> because I think I can do better.
>>
>> It's not a big deal, that's true.  The point I thought was if we have some
>> hierarchy / layered structure in a namespace, in general we should be able
>> to strip outer/upper layer information before we pass the name to the down
>> path.   Of course this cannot be applied all the time in the real coding
>> world, and sometime we just need to take some compromised alternatives.
>
> Agreed.  I think creating the tuple is a clean way to do this, as it
> would still work identically for each filesystem type.  I really want
> to avoid stripping of hierarchy only for some subtypes at one level,
> and then handling the others at a different level.  It feels like we'd
> be adding a bunch of special cases and cluttering up what could be
> simpler code.
>
>>> > For our transitional temporary/persistent filesystems, we could use a
>>> > transitional mount-point path provider, which returns our familiar long
>>> > root
>>> > path + virtual path, and again we can continue to use the same base FSFU
>>> > operations.
>>> > When we switch the current fs to DB-based one, the path
>>> > translation/obfuscation would need to be more integrated into each
>>> > operation, and it would be realized as one of the FSFU implementations
>>> > -- so
>>> > I assume we won't use this GetLocalPath for our purpose.  When we enable
>>> > the
>>> > feature we could switch the FSFU operations we use to the new DB-based
>>> > one,
>>> > and also switch the transitional mount-point path provider to a new one,
>>> > which does almost nothing for path resolution.
>>> > Overall I think there would be less changes and also we could utilize
>>> > zel's
>>> > provider idea almost as is.
>>> > Wdyt?
>>>
>>> There would certainly be less in the way of changes for the local
>>> filesystem, but I think we'd need just as many for the DB filesystem.
>>> And then, since we'd have to maintain both the translate-at-the-top
>>> code and the translate-for-DB code, I think it would end up being more
>>> code overall.
>>
>> By calling GetLocalPath() in the current way all we can do is a prefix
>> conversion, i.e. prepending each filesystem's own root path.
>> And in the sense I am not fully convinced if we could merge
>> 'translate-at-top' and 'translate-for-DB' into a single clean location.
>> They are doing very different level of path name translations imho, and
>> should be handled separately.
>
> We agree on that...
>
>> Also having a unified conversion point in the base FSFU implementation makes
>> me worry how we could make sure how and when the conversion is made in each
>> FSFU wiring/implementation.
>
> Here I'm less sure what you mean.  I expect that the base FSFU will be
> used only for the current sandbox implementation and the Local
> filesystem.  A derived class will handle the DB filesystem, and will
> override or not use the base class's conversion.
>
>> Some FSFU implementation may or may not override some of the methods, and
>> may or may not call into underlying FSFU layer.  Some upper layer FSFU
>> implementation may decide to directly call file_util operations.
>
> Agreed.  The current implementation does that, and I don't expect to change
it.
>
>> If we really need to do the conversion in FILE thread, I would introduce
>> another thin layer that performs the 'local path' to 'real path'
>> translation, just only for local filesystem or other filesystems where every
>> entry name of the local path are completely preserved in their namespace,
>> and would not use the layer for DB based implementation.
>
> Here's where I get confused.  The local->real path translation, that's
> only needed by local filesystems, should be hidden away in code that's
> only used by local filesystems.  The easy way to hide code that's only
> needed by one filesystem type is to put it in the FSFU subclass used
> by that type.  In this case it's the base FSFU, which is designed to
> be used by local filesystems.  Why would we add another layer?
>
> Is it possible that we're saying the same thing in different words?
> We seem to agree on quite a bit.

We seem to do.

Having a subclass that does path translation for local filesystems
makes sense to me too.
What worries me is, I think, having the translation part in the base
FSFU implementation.  It could be confusing to subclass implementors,
or at least it confused me.
Some FSFU subclasses may call base FSFU's methods only for a few
operations but may not for others, and in that case I was afraid we
might end up calling GetRootPath only for the few operations.
Also, for example, I'd assume the quota FSFU would be given an already
translated path from the upper layer (otherwise it needs to talk to
the DB which should be invisible to the lower layer), but it'd be also
calling some of the methods in the base FSFU class, and consequently
we would be calling GetRootPath on those methods while it's not
necessary.  Might I be missing something?

> Aha!  OK, on rereading this, I think I may be understanding you now.
> Tell me if this is it:
>
> I'm suggesting that FSFU call out to some kind of a delegate that will
> do path translation to get the root, and you're suggesting that we put
> a wrapper around FSFU that will do path translation, then call into
> it.  Have I got that right?

Sounds like... or I think I'm just not convinced to have the path
translation logic in the base FSFU class.
If the logic is only used by local filesystems, we could have a
particular subclass that does the translation and applies it only for
those who need it-- that's what I thought.

And I think from the viewpoint of quota FSFU having a thin layer on
top of it makes more sense-- the quota FSFU could just assume the
given path (or given root path + relative path) is pointing to an
actual file entry in the local filesystem, regardless of which
filesystem the upper layer would be, and could just call file_util
methods whenever needed.

>>>  My goal is to get all the hard plumbing out of the way
>>>
>>> here, so that adding additionaly filesystem layers [quota +DB] and
>>> instances [extensions/ChromeOS, <input type=file directory>, etc.] is
>>> as simple as possible.  Ideally, when we get around to things like
>>> <input type=file directory>, it would only require adding an object
>>> with a method to determine access yes/no, and another to supply the
>>> URL->real-path translation.
>>>
>>> > On Fri, Mar 4, 2011 at 5:42 PM, <ericu@chromium.org> wrote:
>>> >>
>>> >> Reviewers: kinuko, Dai Mikurube,
>>> >>
>>> >> Message:
>>> >> Hey all--as you can see, there are a lot of TODO(ericu) marks in this,
>>> >> but
>>> >> I
>>> >> think they can be taken care of in separate changelists.  Everything
>>> >> seems
>>> >> to
>>> >> work, including unit tests [with a bit of hacking], and I think this
>>> >> should move
>>> >> us toward being able to slot in other filesystems soon.
>>> >>
>>> >> Please let me know what you think.
>>> >>
>>> >> Description:
>>> >> Stop returning the true root path of each filesystem from
>>> >> openFileSystem.
>>> >> Instead, return the FileSystem URI of the root.  This will make it
>>> >> easier
>>> >> to swap in different filesystem implementations.
>>> >>
>>> >> BUG=71635
>>> >> TEST=Just a couple in FileSystemUtilTests, but a bunch of existing ones
>>> >> [this
>>> >> doesn't add much new functionality].
>>> >>
>>> >>
>>> >> Please review this at http://codereview.chromium.org/6603034/
>>> >>
>>> >> SVN Base: svn://chrome-svn/chrome/trunk/src/
>>> >>
>>> >> Affected files:
>>> >>  M     webkit/fileapi/file_system_file_util.h
>>> >>  M     webkit/fileapi/file_system_file_util.cc
>>> >>  M     webkit/fileapi/file_system_operation.h
>>> >>  M     webkit/fileapi/file_system_operation.cc
>>> >>  M     webkit/fileapi/file_system_operation_context.h
>>> >>  M     webkit/fileapi/file_system_path_manager.h
>>> >>  M     webkit/fileapi/file_system_path_manager.cc
>>> >>  M     webkit/fileapi/file_system_path_manager_unittest.cc
>>> >>  M     webkit/fileapi/file_system_util.h
>>> >>  M     webkit/fileapi/file_system_util.cc
>>> >>  M     webkit/fileapi/file_system_util_unittest.cc
>>> >>  M     webkit/fileapi/webkit_fileapi.gypi
>>> >>
>>> >>
>>> >
>>> >
>>
>>
>

[ericu, 2011-03-08 02:56:20]

On Mon, Mar 7, 2011 at 5:28 PM, Kinuko Yasuda <kinuko@chromium.org> wrote:
> (Sorry for my late response, I lost my half-written response due to
> XSRF token timeout...)

I definitely need to add local filesystem storage to jV, so that you
can save drafts manually with :w ;'>.

> On Mon, Mar 7, 2011 at 12:17 PM, Eric Uhrhane <ericu@chromium.org> wrote:
>> On Sun, Mar 6, 2011 at 11:46 PM, Kinuko Yasuda <kinuko@chromium.org> wrote:
>>> All I'd like to see is a simple architecture with less changes, and I
>>> believe we share the same goal for those parts.  So let's just focus on what
>>> we want to achieve.
>>
>> Sounds good.  However, I think there may be a bit of a tradeoff
>> between "less changes" and "simple architecture".  Still, where
>> there's a tradeoff, there are often multiple solutions that make
>> sense, and we can just figure out which one seems best.
>>
>>> All we want to for each operation is:
>>> 1. crack the given input URL
>>> 2. get the type, origin and virtual_path (the local 'path' part of the given
>>> URL)
>>> 3. pass the tuple information to each FSFU implementation
>>> Where:
>>> a. each FSFU implementation may have its own namespace where an entry can be
>>> identified by a local part of the URL (some implementation may also need
>>> 'type' and 'origin' to resolve the local path).
>>> b. some FSFU implementation just maps a file path to another native file
>>> path, but resolving the path may need DB or some local file access.
>>> Correct?  I added b. from your response.
>>
>> Right.
>>
>>> Then I would imagine something like this:
>>> At step 2. we can make up an operation context that includes 'type' and
>>> 'origin'.
>>> We could add a thin FSFU layer, let's say we call it LocalFSFU, that just
>>> prepends its own 'root path' to the 'local path' and call into the base FSFU
>>> operations (probably the operation context could contain additional 'root
>>> path' entry for the FSFU), and could add the layer to any filesystems which
>>> needs such prefix translation.  (Please see below for more discussion on
>>> this)
>>
>> Is FSFU pretty much LocalFSFU already?  I guess it's still going to
>> have to call out to get the mapping of the root path.  There's going
>> to have to be some object that handles that part, and it can't be
>> LFSFU if that's a singleton.
>>
>>> On Sun, Mar 6, 2011 at 6:50 PM, Eric Uhrhane <ericu@chromium.org> wrote:
>>>>
>>>> On Fri, Mar 4, 2011 at 6:22 PM, Kinuko Yasuda <kinuko@chromium.org> wrote:
>>>> > At a quick glance it looks neat; but I'm not sure if we have strong
>>>> > reasons
>>>> > to switch the place where we perform simple/generic path translation.
>>>> >  We
>>>> > are doing it before calling into FILE thread.  Do we need to change
>>>> > that?
>>>>
>>>> Well, we'll need to for the DB, since it will require a database read.
>>>>  And if we're going to need to push it down there for one filesystem
>>>> type, it seems a lot simpler and cleaner to me to do it for every
>>>> filesystem type.  That way there's no special-casing, everybody just
>>>> uses the same interfaces, and we only really need to do that kind of
>>>> thing in a single place.
>>>>
>>>>  It also means that in between the top layer
>>>> [where I think you're suggesting doing the local filesystem
>>>> translation] and the FSFU, we're always passing the same data type.
>>>> Otherwise we'd have a variable that would hold a real local path some
>>>> of the time, and a URL some of the time, and I think that might be
>>>> error-prone.
>>>
>>> Once we crack the URL we could simply pass a tuple of { 'type', 'origin',
>>> 'virtual_path' } couldn't we?
>>
>> Yes, absolutely.  We can just pass that all the way through instead of
>> cracking multiple times.  The key point is that the type is needed to
>> look up the proper FSFU subclass, but is also needed in the sandbox to
>> determine the full root.  I was cracking once for each need, but this
>> would enable us to crack just once.
>>
>>> If we change all FSFU operations to take URL (GURL?) instead of FilePath, we
>>> need the GURL -> FilePath conversion at each layer, where some or most of
>>> layers may just want to focus on their local operations.
>>
>> I think we have 2 choices here that would eliminate having to do two
>> conversions.  Both ways we'd just do it at a low level, inside the
>> FSFU subclass.  Either we can pass the path tuple you suggested all
>> the way in (and create the path inside the FSFU), or we can generate
>> the root path [but not the full path] at the top level, and pass
>> {root, type, virtual_path} in, and use that in each FSFU.  Either way,
>> we only process the URL once.  We know that for any anticipated
>> filesystem type, we're going to need a root path of some sort, so we
>> don't have to do anything more general than this.
>
> Either one would work... to be sure, I assumed the 'virtual_path' part
> might vary when an operation goes across layers, e.g. for the FSFU
> implementations placed below the DB-based layer the original
> 'virtual_path' will have no meanings, and instead they will need to be
> given a new virtual_path which is an actual relative path to the given
> root path.

Ah, I see.  I'd always meant to have virtual_path mean the same thing
on any layer, but I see that I was depending on a bunch of assumptions
that I haven't explained.  Also, you've pointed out a piece that I was
missing anyway, so let me try to talk it all out and see if it all
works.  I guess we should keep calling the parameter file_path, and
let it mean different things as needed.  The rest of the tuple [root
URL, type] can be accessed from the context as needed.

>>>> > The reasons I stick to the current architecture (virtual path or URL
>>>> > crack
>>>> > -> resolve type and path -> calling FSFU operations on FILE thread):
>>>> > As in your comment we need to crack the path (URL) first to get a fs
>>>> > type,
>>>> > origin and appropriate FileSystemFileUtil instance before calling to the
>>>> > FILE method.  For filesystems (e.g. local fs) for which we don't need
>>>> > any
>>>> > intricate path obfuscation, we can resolve the paths there (using a
>>>> > mount-point path provider for "local" filesystem) and continue to use
>>>> > the
>>>> > base FSFU operations as is.
>>>>
>>>> The code where I made that comment could also be cleaned up just by
>>>> passing the type through along with the context, I think.  But it's
>>>> also not a really big deal--I just wanted to remember to revisit it
>>>> because I think I can do better.
>>>
>>> It's not a big deal, that's true.  The point I thought was if we have some
>>> hierarchy / layered structure in a namespace, in general we should be able
>>> to strip outer/upper layer information before we pass the name to the down
>>> path.   Of course this cannot be applied all the time in the real coding
>>> world, and sometime we just need to take some compromised alternatives.
>>
>> Agreed.  I think creating the tuple is a clean way to do this, as it
>> would still work identically for each filesystem type.  I really want
>> to avoid stripping of hierarchy only for some subtypes at one level,
>> and then handling the others at a different level.  It feels like we'd
>> be adding a bunch of special cases and cluttering up what could be
>> simpler code.
>>
>>>> > For our transitional temporary/persistent filesystems, we could use a
>>>> > transitional mount-point path provider, which returns our familiar long
>>>> > root
>>>> > path + virtual path, and again we can continue to use the same base FSFU
>>>> > operations.
>>>> > When we switch the current fs to DB-based one, the path
>>>> > translation/obfuscation would need to be more integrated into each
>>>> > operation, and it would be realized as one of the FSFU implementations
>>>> > -- so
>>>> > I assume we won't use this GetLocalPath for our purpose.  When we enable
>>>> > the
>>>> > feature we could switch the FSFU operations we use to the new DB-based
>>>> > one,
>>>> > and also switch the transitional mount-point path provider to a new one,
>>>> > which does almost nothing for path resolution.
>>>> > Overall I think there would be less changes and also we could utilize
>>>> > zel's
>>>> > provider idea almost as is.
>>>> > Wdyt?
>>>>
>>>> There would certainly be less in the way of changes for the local
>>>> filesystem, but I think we'd need just as many for the DB filesystem.
>>>> And then, since we'd have to maintain both the translate-at-the-top
>>>> code and the translate-for-DB code, I think it would end up being more
>>>> code overall.
>>>
>>> By calling GetLocalPath() in the current way all we can do is a prefix
>>> conversion, i.e. prepending each filesystem's own root path.
>>> And in the sense I am not fully convinced if we could merge
>>> 'translate-at-top' and 'translate-for-DB' into a single clean location.
>>> They are doing very different level of path name translations imho, and
>>> should be handled separately.
>>
>> We agree on that...
>>
>>> Also having a unified conversion point in the base FSFU implementation makes
>>> me worry how we could make sure how and when the conversion is made in each
>>> FSFU wiring/implementation.
>>
>> Here I'm less sure what you mean.  I expect that the base FSFU will be
>> used only for the current sandbox implementation and the Local
>> filesystem.  A derived class will handle the DB filesystem, and will
>> override or not use the base class's conversion.
>>
>>> Some FSFU implementation may or may not override some of the methods, and
>>> may or may not call into underlying FSFU layer.  Some upper layer FSFU
>>> implementation may decide to directly call file_util operations.
>>
>> Agreed.  The current implementation does that, and I don't expect to change
it.
>>
>>> If we really need to do the conversion in FILE thread, I would introduce
>>> another thin layer that performs the 'local path' to 'real path'
>>> translation, just only for local filesystem or other filesystems where every
>>> entry name of the local path are completely preserved in their namespace,
>>> and would not use the layer for DB based implementation.
>>
>> Here's where I get confused.  The local->real path translation, that's
>> only needed by local filesystems, should be hidden away in code that's
>> only used by local filesystems.  The easy way to hide code that's only
>> needed by one filesystem type is to put it in the FSFU subclass used
>> by that type.  In this case it's the base FSFU, which is designed to
>> be used by local filesystems.  Why would we add another layer?
>>
>> Is it possible that we're saying the same thing in different words?
>> We seem to agree on quite a bit.
>
> We seem to do.
>
> Having a subclass that does path translation for local filesystems
> makes sense to me too.
> What worries me is, I think, having the translation part in the base
> FSFU implementation.  It could be confusing to subclass implementors,
> or at least it confused me.

You're absolutely right--we should pull out a subclass for that.

How does this look for the eventual stack options?

DB calls into Quota calls into FileSystemFileUtil calls into file_util
 [for the sandbox]
OR
Local calls into Quota calls into FileSystemFileUtil calls into
file_util  [for the old sandbox or mount points with quota]
OR
Local calls into FileSystemFileUtil calls into file_util  [for mount
points without quota]

DB and Local will each hold a pointer to an FSFU or QFSFU singleton,
passed in via context [so that a single Local singleton can handle
filesystems with and without quota].
The QFSFU->FSFU calls could either be done via singleton or via
parent-class "FSFU::method" call.
We look up the appropriate FSFU stack when we crack the URL and get the type.

In this model, FSFU does no path translation whatsoever, so the
file_path it's passed is an absolute one.
DB and Local take virtual paths, and translate them as appropriate
using the information in the FileSystemOperationContext before calling
into the next lower layer.  Quota probably doesn't need to do any
translation, only accounting.

> Some FSFU subclasses may call base FSFU's methods only for a few
> operations but may not for others, and in that case I was afraid we
> might end up calling GetRootPath only for the few operations.

You're right--that was messy in my original model.

> Also, for example, I'd assume the quota FSFU would be given an already
> translated path from the upper layer (otherwise it needs to talk to
> the DB which should be invisible to the lower layer), but it'd be also
> calling some of the methods in the base FSFU class, and consequently
> we would be calling GetRootPath on those methods while it's not
> necessary.  Might I be missing something?

No, you were right.

>> Aha!  OK, on rereading this, I think I may be understanding you now.
>> Tell me if this is it:
>>
>> I'm suggesting that FSFU call out to some kind of a delegate that will
>> do path translation to get the root, and you're suggesting that we put
>> a wrapper around FSFU that will do path translation, then call into
>> it.  Have I got that right?
>
> Sounds like... or I think I'm just not convinced to have the path
> translation logic in the base FSFU class.
> If the logic is only used by local filesystems, we could have a
> particular subclass that does the translation and applies it only for
> those who need it-- that's what I thought.
>
> And I think from the viewpoint of quota FSFU having a thin layer on
> top of it makes more sense-- the quota FSFU could just assume the
> given path (or given root path + relative path) is pointing to an
> actual file entry in the local filesystem, regardless of which
> filesystem the upper layer would be, and could just call file_util
> methods whenever needed.

It could call directly, but I think it might be cleaner to have
QuotaFSFU call the base FSFU, and have that call file_util.  That way
the QFSFU methods will be really small [single FSFU calls, after doing
the quota accounting] and the FSFU calls can have multiple file_util
calls where needed.

>>>>  My goal is to get all the hard plumbing out of the way
>>>>
>>>> here, so that adding additionaly filesystem layers [quota +DB] and
>>>> instances [extensions/ChromeOS, <input type=file directory>, etc.] is
>>>> as simple as possible.  Ideally, when we get around to things like
>>>> <input type=file directory>, it would only require adding an object
>>>> with a method to determine access yes/no, and another to supply the
>>>> URL->real-path translation.
>>>>
>>>> > On Fri, Mar 4, 2011 at 5:42 PM, <ericu@chromium.org> wrote:
>>>> >>
>>>> >> Reviewers: kinuko, Dai Mikurube,
>>>> >>
>>>> >> Message:
>>>> >> Hey all--as you can see, there are a lot of TODO(ericu) marks in this,
>>>> >> but
>>>> >> I
>>>> >> think they can be taken care of in separate changelists.  Everything
>>>> >> seems
>>>> >> to
>>>> >> work, including unit tests [with a bit of hacking], and I think this
>>>> >> should move
>>>> >> us toward being able to slot in other filesystems soon.
>>>> >>
>>>> >> Please let me know what you think.
>>>> >>
>>>> >> Description:
>>>> >> Stop returning the true root path of each filesystem from
>>>> >> openFileSystem.
>>>> >> Instead, return the FileSystem URI of the root.  This will make it
>>>> >> easier
>>>> >> to swap in different filesystem implementations.
>>>> >>
>>>> >> BUG=71635
>>>> >> TEST=Just a couple in FileSystemUtilTests, but a bunch of existing ones
>>>> >> [this
>>>> >> doesn't add much new functionality].
>>>> >>
>>>> >>
>>>> >> Please review this at http://codereview.chromium.org/6603034/
>>>> >>
>>>> >> SVN Base: svn://chrome-svn/chrome/trunk/src/
>>>> >>
>>>> >> Affected files:
>>>> >>  M     webkit/fileapi/file_system_file_util.h
>>>> >>  M     webkit/fileapi/file_system_file_util.cc
>>>> >>  M     webkit/fileapi/file_system_operation.h
>>>> >>  M     webkit/fileapi/file_system_operation.cc
>>>> >>  M     webkit/fileapi/file_system_operation_context.h
>>>> >>  M     webkit/fileapi/file_system_path_manager.h
>>>> >>  M     webkit/fileapi/file_system_path_manager.cc
>>>> >>  M     webkit/fileapi/file_system_path_manager_unittest.cc
>>>> >>  M     webkit/fileapi/file_system_util.h
>>>> >>  M     webkit/fileapi/file_system_util.cc
>>>> >>  M     webkit/fileapi/file_system_util_unittest.cc
>>>> >>  M     webkit/fileapi/webkit_fileapi.gypi
>>>> >>
>>>> >>
>>>> >
>>>> >
>>>
>>>
>>
>

[kinuko, 2011-03-08 20:26:50]

On Mon, Mar 7, 2011 at 6:55 PM, Eric Uhrhane <ericu@chromium.org> wrote:
>> (Sorry for my late response, I lost my half-written response due to
>> XSRF token timeout...)
>
> I definitely need to add local filesystem storage to jV, so that you
> can save drafts manually with :w ;'>.

Seems like it turned out to be very important for our productivity ;)

>>> I think we have 2 choices here that would eliminate having to do two
>>> conversions.  Both ways we'd just do it at a low level, inside the
>>> FSFU subclass.  Either we can pass the path tuple you suggested all
>>> the way in (and create the path inside the FSFU), or we can generate
>>> the root path [but not the full path] at the top level, and pass
>>> {root, type, virtual_path} in, and use that in each FSFU.  Either way,
>>> we only process the URL once.  We know that for any anticipated
>>> filesystem type, we're going to need a root path of some sort, so we
>>> don't have to do anything more general than this.
>>
>> Either one would work... to be sure, I assumed the 'virtual_path' part
>> might vary when an operation goes across layers, e.g. for the FSFU
>> implementations placed below the DB-based layer the original
>> 'virtual_path' will have no meanings, and instead they will need to be
>> given a new virtual_path which is an actual relative path to the given
>> root path.
>
> Ah, I see.  I'd always meant to have virtual_path mean the same thing
> on any layer, but I see that I was depending on a bunch of assumptions
> that I haven't explained.  Also, you've pointed out a piece that I was
> missing anyway, so let me try to talk it all out and see if it all
> works.  I guess we should keep calling the parameter file_path, and
> let it mean different things as needed.  The rest of the tuple [root
> URL, type] can be accessed from the context as needed.
>
>>>>> > The reasons I stick to the current architecture (virtual path or URL
>>>>> > crack
>>>>> > -> resolve type and path -> calling FSFU operations on FILE thread):
>>>>> > As in your comment we need to crack the path (URL) first to get a fs
>>>>> > type,
>>>>> > origin and appropriate FileSystemFileUtil instance before calling to the
>>>>> > FILE method.  For filesystems (e.g. local fs) for which we don't need
>>>>> > any
>>>>> > intricate path obfuscation, we can resolve the paths there (using a
>>>>> > mount-point path provider for "local" filesystem) and continue to use
>>>>> > the
>>>>> > base FSFU operations as is.
>>>>>
>>>>> The code where I made that comment could also be cleaned up just by
>>>>> passing the type through along with the context, I think.  But it's
>>>>> also not a really big deal--I just wanted to remember to revisit it
>>>>> because I think I can do better.
>>>>
>>>> It's not a big deal, that's true.  The point I thought was if we have some
>>>> hierarchy / layered structure in a namespace, in general we should be able
>>>> to strip outer/upper layer information before we pass the name to the down
>>>> path.   Of course this cannot be applied all the time in the real coding
>>>> world, and sometime we just need to take some compromised alternatives.
>>>
>>> Agreed.  I think creating the tuple is a clean way to do this, as it
>>> would still work identically for each filesystem type.  I really want
>>> to avoid stripping of hierarchy only for some subtypes at one level,
>>> and then handling the others at a different level.  It feels like we'd
>>> be adding a bunch of special cases and cluttering up what could be
>>> simpler code.
>>>
>>>>> > For our transitional temporary/persistent filesystems, we could use a
>>>>> > transitional mount-point path provider, which returns our familiar long
>>>>> > root
>>>>> > path + virtual path, and again we can continue to use the same base FSFU
>>>>> > operations.
>>>>> > When we switch the current fs to DB-based one, the path
>>>>> > translation/obfuscation would need to be more integrated into each
>>>>> > operation, and it would be realized as one of the FSFU implementations
>>>>> > -- so
>>>>> > I assume we won't use this GetLocalPath for our purpose.  When we enable
>>>>> > the
>>>>> > feature we could switch the FSFU operations we use to the new DB-based
>>>>> > one,
>>>>> > and also switch the transitional mount-point path provider to a new one,
>>>>> > which does almost nothing for path resolution.
>>>>> > Overall I think there would be less changes and also we could utilize
>>>>> > zel's
>>>>> > provider idea almost as is.
>>>>> > Wdyt?
>>>>>
>>>>> There would certainly be less in the way of changes for the local
>>>>> filesystem, but I think we'd need just as many for the DB filesystem.
>>>>> And then, since we'd have to maintain both the translate-at-the-top
>>>>> code and the translate-for-DB code, I think it would end up being more
>>>>> code overall.
>>>>
>>>> By calling GetLocalPath() in the current way all we can do is a prefix
>>>> conversion, i.e. prepending each filesystem's own root path.
>>>> And in the sense I am not fully convinced if we could merge
>>>> 'translate-at-top' and 'translate-for-DB' into a single clean location.
>>>> They are doing very different level of path name translations imho, and
>>>> should be handled separately.
>>>
>>> We agree on that...
>>>
>>>> Also having a unified conversion point in the base FSFU implementation
makes
>>>> me worry how we could make sure how and when the conversion is made in each
>>>> FSFU wiring/implementation.
>>>
>>> Here I'm less sure what you mean.  I expect that the base FSFU will be
>>> used only for the current sandbox implementation and the Local
>>> filesystem.  A derived class will handle the DB filesystem, and will
>>> override or not use the base class's conversion.
>>>
>>>> Some FSFU implementation may or may not override some of the methods, and
>>>> may or may not call into underlying FSFU layer.  Some upper layer FSFU
>>>> implementation may decide to directly call file_util operations.
>>>
>>> Agreed.  The current implementation does that, and I don't expect to change
it.
>>>
>>>> If we really need to do the conversion in FILE thread, I would introduce
>>>> another thin layer that performs the 'local path' to 'real path'
>>>> translation, just only for local filesystem or other filesystems where
every
>>>> entry name of the local path are completely preserved in their namespace,
>>>> and would not use the layer for DB based implementation.
>>>
>>> Here's where I get confused.  The local->real path translation, that's
>>> only needed by local filesystems, should be hidden away in code that's
>>> only used by local filesystems.  The easy way to hide code that's only
>>> needed by one filesystem type is to put it in the FSFU subclass used
>>> by that type.  In this case it's the base FSFU, which is designed to
>>> be used by local filesystems.  Why would we add another layer?
>>>
>>> Is it possible that we're saying the same thing in different words?
>>> We seem to agree on quite a bit.
>>
>> We seem to do.
>>
>> Having a subclass that does path translation for local filesystems
>> makes sense to me too.
>> What worries me is, I think, having the translation part in the base
>> FSFU implementation.  It could be confusing to subclass implementors,
>> or at least it confused me.
>
> You're absolutely right--we should pull out a subclass for that.
>
> How does this look for the eventual stack options?
>
> DB calls into Quota calls into FileSystemFileUtil calls into file_util
>  [for the sandbox]
> OR
> Local calls into Quota calls into FileSystemFileUtil calls into
> file_util  [for the old sandbox or mount points with quota]
> OR
> Local calls into FileSystemFileUtil calls into file_util  [for mount
> points without quota]
>
> DB and Local will each hold a pointer to an FSFU or QFSFU singleton,
> passed in via context [so that a single Local singleton can handle
> filesystems with and without quota].
> The QFSFU->FSFU calls could either be done via singleton or via
> parent-class "FSFU::method" call.
> We look up the appropriate FSFU stack when we crack the URL and get the type.
>
> In this model, FSFU does no path translation whatsoever, so the
> file_path it's passed is an absolute one.
> DB and Local take virtual paths, and translate them as appropriate
> using the information in the FileSystemOperationContext before calling
> into the next lower layer.  Quota probably doesn't need to do any
> translation, only accounting.

This sounds good to me, thanks for sorting various patterns out!
(Dai, could you also take a look at the models if it seems to work for
our/your implementation and possible future plan (if any)?)

>> Some FSFU subclasses may call base FSFU's methods only for a few
>> operations but may not for others, and in that case I was afraid we
>> might end up calling GetRootPath only for the few operations.
>
> You're right--that was messy in my original model.
>
>> Also, for example, I'd assume the quota FSFU would be given an already
>> translated path from the upper layer (otherwise it needs to talk to
>> the DB which should be invisible to the lower layer), but it'd be also
>> calling some of the methods in the base FSFU class, and consequently
>> we would be calling GetRootPath on those methods while it's not
>> necessary.  Might I be missing something?
>
> No, you were right.
>
>>> Aha!  OK, on rereading this, I think I may be understanding you now.
>>> Tell me if this is it:
>>>
>>> I'm suggesting that FSFU call out to some kind of a delegate that will
>>> do path translation to get the root, and you're suggesting that we put
>>> a wrapper around FSFU that will do path translation, then call into
>>> it.  Have I got that right?
>>
>> Sounds like... or I think I'm just not convinced to have the path
>> translation logic in the base FSFU class.
>> If the logic is only used by local filesystems, we could have a
>> particular subclass that does the translation and applies it only for
>> those who need it-- that's what I thought.
>>
>> And I think from the viewpoint of quota FSFU having a thin layer on
>> top of it makes more sense-- the quota FSFU could just assume the
>> given path (or given root path + relative path) is pointing to an
>> actual file entry in the local filesystem, regardless of which
>> filesystem the upper layer would be, and could just call file_util
>> methods whenever needed.
>
> It could call directly, but I think it might be cleaner to have
> QuotaFSFU call the base FSFU, and have that call file_util.  That way
> the QFSFU methods will be really small [single FSFU calls, after doing
> the quota accounting] and the FSFU calls can have multiple file_util
> calls where needed.

Yeah, agreed.

>>>>>  My goal is to get all the hard plumbing out of the way
>>>>>
>>>>> here, so that adding additionaly filesystem layers [quota +DB] and
>>>>> instances [extensions/ChromeOS, <input type=file directory>, etc.] is
>>>>> as simple as possible.  Ideally, when we get around to things like
>>>>> <input type=file directory>, it would only require adding an object
>>>>> with a method to determine access yes/no, and another to supply the
>>>>> URL->real-path translation.
>>>>>
>>>>> > On Fri, Mar 4, 2011 at 5:42 PM, <ericu@chromium.org> wrote:
>>>>> >>
>>>>> >> Reviewers: kinuko, Dai Mikurube,
>>>>> >>
>>>>> >> Message:
>>>>> >> Hey all--as you can see, there are a lot of TODO(ericu) marks in this,
>>>>> >> but
>>>>> >> I
>>>>> >> think they can be taken care of in separate changelists.  Everything
>>>>> >> seems
>>>>> >> to
>>>>> >> work, including unit tests [with a bit of hacking], and I think this
>>>>> >> should move
>>>>> >> us toward being able to slot in other filesystems soon.
>>>>> >>
>>>>> >> Please let me know what you think.
>>>>> >>
>>>>> >> Description:
>>>>> >> Stop returning the true root path of each filesystem from
>>>>> >> openFileSystem.
>>>>> >> Instead, return the FileSystem URI of the root.  This will make it
>>>>> >> easier
>>>>> >> to swap in different filesystem implementations.
>>>>> >>
>>>>> >> BUG=71635
>>>>> >> TEST=Just a couple in FileSystemUtilTests, but a bunch of existing ones
>>>>> >> [this
>>>>> >> doesn't add much new functionality].
>>>>> >>
>>>>> >>
>>>>> >> Please review this at http://codereview.chromium.org/6603034/
>>>>> >>
>>>>> >> SVN Base: svn://chrome-svn/chrome/trunk/src/
>>>>> >>
>>>>> >> Affected files:
>>>>> >>  M     webkit/fileapi/file_system_file_util.h
>>>>> >>  M     webkit/fileapi/file_system_file_util.cc
>>>>> >>  M     webkit/fileapi/file_system_operation.h
>>>>> >>  M     webkit/fileapi/file_system_operation.cc
>>>>> >>  M     webkit/fileapi/file_system_operation_context.h
>>>>> >>  M     webkit/fileapi/file_system_path_manager.h
>>>>> >>  M     webkit/fileapi/file_system_path_manager.cc
>>>>> >>  M     webkit/fileapi/file_system_path_manager_unittest.cc
>>>>> >>  M     webkit/fileapi/file_system_util.h
>>>>> >>  M     webkit/fileapi/file_system_util.cc
>>>>> >>  M     webkit/fileapi/file_system_util_unittest.cc
>>>>> >>  M     webkit/fileapi/webkit_fileapi.gypi
>>>>> >>
>>>>> >>
>>>>> >
>>>>> >
>>>>
>>>>
>>>
>>
>

[Dai Mikurube (google.com), 2011-03-08 20:46:53]

On 2011/03/08 20:26:50, kinuko wrote:
> On Mon, Mar 7, 2011 at 6:55 PM, Eric Uhrhane <mailto:ericu@chromium.org>
wrote:
> >> (Sorry for my late response, I lost my half-written response due to
> >> XSRF token timeout...)
> >
> > I definitely need to add local filesystem storage to jV, so that you
> > can save drafts manually with :w ;'>.
> 
> Seems like it turned out to be very important for our productivity ;)
> 
> >>> I think we have 2 choices here that would eliminate having to do two
> >>> conversions. &nbsp;Both ways we'd just do it at a low level, inside the
> >>> FSFU subclass. &nbsp;Either we can pass the path tuple you suggested all
> >>> the way in (and create the path inside the FSFU), or we can generate
> >>> the root path [but not the full path] at the top level, and pass
> >>> {root, type, virtual_path} in, and use that in each FSFU. &nbsp;Either
way,
> >>> we only process the URL once. &nbsp;We know that for any anticipated
> >>> filesystem type, we're going to need a root path of some sort, so we
> >>> don't have to do anything more general than this.
> >>
> >> Either one would work... to be sure, I assumed the 'virtual_path' part
> >> might vary when an operation goes across layers, e.g. for the FSFU
> >> implementations placed below the DB-based layer the original
> >> 'virtual_path' will have no meanings, and instead they will need to be
> >> given a new virtual_path which is an actual relative path to the given
> >> root path.
> >
> > Ah, I see. &nbsp;I'd always meant to have virtual_path mean the same thing
> > on any layer, but I see that I was depending on a bunch of assumptions
> > that I haven't explained. &nbsp;Also, you've pointed out a piece that I was
> > missing anyway, so let me try to talk it all out and see if it all
> > works. &nbsp;I guess we should keep calling the parameter file_path, and
> > let it mean different things as needed. &nbsp;The rest of the tuple [root
> > URL, type] can be accessed from the context as needed.
> >
> >>>>> > The reasons I stick to&nbsp;the current architecture (virtual path or
URL
> >>>>> > crack
> >>>>> > -> resolve type and path -> calling FSFU operations on FILE thread):
> >>>>> > As in your comment we need to crack the path (URL) first to get a fs
> >>>>> > type,
> >>>>> > origin and appropriate FileSystemFileUtil instance before calling to
the
> >>>>> > FILE method. &nbsp;For filesystems (e.g. local fs) for which we don't
need
> >>>>> > any
> >>>>> > intricate path obfuscation, we can resolve the paths there (using a
> >>>>> > mount-point path provider for "local" filesystem) and continue to use
> >>>>> > the
> >>>>> > base FSFU operations as is.
> >>>>>
> >>>>> The code where I made that comment could also be cleaned up just by
> >>>>> passing the type through along with the context, I think. &nbsp;But it's
> >>>>> also not a really big deal--I just wanted to remember to revisit it
> >>>>> because I think I can do better.
> >>>>
> >>>> It's not a big deal, that's true. &nbsp;The point I thought was if we
have some
> >>>> hierarchy / layered structure in a namespace, in general we should be
able
> >>>> to strip outer/upper layer information before we pass the name to the
down
> >>>> path. &nbsp; Of course this cannot be applied all the time in the real
coding
> >>>> world, and sometime we just need to take some compromised alternatives.
> >>>
> >>> Agreed. &nbsp;I think creating the tuple is a clean way to do this, as it
> >>> would still work identically for each filesystem type. &nbsp;I really want
> >>> to avoid stripping of hierarchy only for some subtypes at one level,
> >>> and then handling the others at a different level. &nbsp;It feels like
we'd
> >>> be adding a bunch of special cases and cluttering up what could be
> >>> simpler code.
> >>>
> >>>>> > For our transitional temporary/persistent filesystems, we could use a
> >>>>> > transitional mount-point path provider, which returns our familiar
long
> >>>>> > root
> >>>>> > path + virtual path, and again we can continue to use the same base
FSFU
> >>>>> > operations.
> >>>>> > When we switch the current fs to DB-based one, the path
> >>>>> > translation/obfuscation would need to be more integrated into each
> >>>>> > operation, and it would be realized as one of the FSFU implementations
> >>>>> > -- so
> >>>>> > I assume we won't use this GetLocalPath for our purpose. &nbsp;When we
enable
> >>>>> > the
> >>>>> > feature we could switch the FSFU operations we use to the new DB-based
> >>>>> > one,
> >>>>> > and also&nbsp;switch the transitional mount-point path provider to a
new one,
> >>>>> > which does almost nothing for path resolution.
> >>>>> > Overall I think there would be less changes and also we could utilize
> >>>>> > zel's
> >>>>> > provider idea almost as is.
> >>>>> > Wdyt?
> >>>>>
> >>>>> There would certainly be less in the way of changes for the local
> >>>>> filesystem, but I think we'd need just as many for the DB filesystem.
> >>>>> And then, since we'd have to maintain both the translate-at-the-top
> >>>>> code and the translate-for-DB code, I think it would end up being more
> >>>>> code overall.
> >>>>
> >>>> By calling GetLocalPath() in the current way all we can do is a prefix
> >>>> conversion, i.e. prepending each filesystem's own root path.
> >>>> And in the sense I am not fully convinced if we could merge
> >>>> 'translate-at-top' and 'translate-for-DB' into a single clean location.
> >>>> They are doing very different level of path name translations imho, and
> >>>> should be handled separately.
> >>>
> >>> We agree on that...
> >>>
> >>>> Also having a unified conversion point in the base FSFU implementation
> makes
> >>>> me worry how we could make sure how and when the conversion is made in
each
> >>>> FSFU wiring/implementation.
> >>>
> >>> Here I'm less sure what you mean. &nbsp;I expect that the base FSFU will
be
> >>> used only for the current sandbox implementation and the Local
> >>> filesystem. &nbsp;A derived class will handle the DB filesystem, and will
> >>> override or not use the base class's conversion.
> >>>
> >>>> Some FSFU implementation may or may not override some of the methods, and
> >>>> may or may not call into underlying FSFU layer. &nbsp;Some upper layer
FSFU
> >>>> implementation may decide to directly call file_util operations.
> >>>
> >>> Agreed. &nbsp;The current implementation does that, and I don't expect to
change
> it.
> >>>
> >>>> If we really need to do the conversion in FILE thread, I would introduce
> >>>> another thin layer that performs the 'local path' to 'real path'
> >>>> translation, just only for local filesystem or other filesystems where
> every
> >>>> entry name of the local path are completely preserved in their namespace,
> >>>> and would not use the layer for DB based implementation.
> >>>
> >>> Here's where I get confused. &nbsp;The local->real path translation,
that's
> >>> only needed by local filesystems, should be hidden away in code that's
> >>> only used by local filesystems. &nbsp;The easy way to hide code that's
only
> >>> needed by one filesystem type is to put it in the FSFU subclass used
> >>> by that type. &nbsp;In this case it's the base FSFU, which is designed to
> >>> be used by local filesystems. &nbsp;Why would we add another layer?
> >>>
> >>> Is it possible that we're saying the same thing in different words?
> >>> We seem to agree on quite a bit.
> >>
> >> We seem to do.
> >>
> >> Having a subclass that does path translation for local filesystems
> >> makes sense to me too.
> >> What worries me is, I think, having the translation part in the base
> >> FSFU implementation. &nbsp;It could be confusing to subclass implementors,
> >> or at least it confused me.
> >
> > You're absolutely right--we should pull out a subclass for that.
> >
> > How does this look for the eventual stack options?
> >
> > DB calls into Quota calls into FileSystemFileUtil calls into file_util
> > &nbsp;[for the sandbox]
> > OR
> > Local calls into Quota calls into FileSystemFileUtil calls into
> > file_util &nbsp;[for the old sandbox or mount points with quota]
> > OR
> > Local calls into FileSystemFileUtil calls into file_util &nbsp;[for mount
> > points without quota]
> >
> > DB and Local will each hold a pointer to an FSFU or QFSFU singleton,
> > passed in via context [so that a single Local singleton can handle
> > filesystems with and without quota].
> > The QFSFU->FSFU calls could either be done via singleton or via
> > parent-class "FSFU::method" call.
> > We look up the appropriate FSFU stack when we crack the URL and get the
type.
> >
> > In this model, FSFU does no path translation whatsoever, so the
> > file_path it's passed is an absolute one.
> > DB and Local take virtual paths, and translate them as appropriate
> > using the information in the FileSystemOperationContext before calling
> > into the next lower layer. &nbsp;Quota probably doesn't need to do any
> > translation, only accounting.
> 
> This sounds good to me, thanks for sorting various patterns out!
> (Dai, could you also take a look at the models if it seems to work for
> our/your implementation and possible future plan (if any)?)

Just a short comment. It looks good to me.
Reducing dependency, and non-touching the base class, helps us to work on it
with time differences. :)

[ericu, 2011-03-10 03:39:46]

OK, I've rolled in Kinuko's feedback.  There's quite a bit of churn there, and
it's not the cleanest change in the world, but I think it's close to ready. 
I've marked a bunch of TODOs for cleanup in subsequent CLs, but I don't want
this CL to get too big, and I'd like to get it in to free up folks that are
waiting on it.

Zelidrag, you can see that I absorbed your MountPointProvider interface
[mostly].  Hopefully your stuff can slot in to a few switch statements and just
work.

[ericu, 2011-03-10 22:48:14]

Incidentally, I'm working on the trybot's test failures now.  I'd
passed all the tests, but then made some changes before I posted the
last version, and now some tests fail in ways that just have to do
with the falseness of their setup.

On Wed, Mar 9, 2011 at 7:39 PM,  <ericu@chromium.org> wrote:
> OK, I've rolled in Kinuko's feedback.  There's quite a bit of churn there,
> and
> it's not the cleanest change in the world, but I think it's close to ready.
> I've marked a bunch of TODOs for cleanup in subsequent CLs, but I don't want
> this CL to get too big, and I'd like to get it in to free up folks that are
> waiting on it.
>
> Zelidrag, you can see that I absorbed your MountPointProvider interface
> [mostly].  Hopefully your stuff can slot in to a few switch statements and
> just
> work.
>
> http://codereview.chromium.org/6603034/
>

[ericu, 2011-03-10 23:28:14]

The [small] fix is up, and the trybots are running again.

On Thu, Mar 10, 2011 at 2:47 PM, Eric Uhrhane <ericu@chromium.org> wrote:
> Incidentally, I'm working on the trybot's test failures now.  I'd
> passed all the tests, but then made some changes before I posted the
> last version, and now some tests fail in ways that just have to do
> with the falseness of their setup.
>
> On Wed, Mar 9, 2011 at 7:39 PM,  <ericu@chromium.org> wrote:
>> OK, I've rolled in Kinuko's feedback.  There's quite a bit of churn there,
>> and
>> it's not the cleanest change in the world, but I think it's close to ready.
>> I've marked a bunch of TODOs for cleanup in subsequent CLs, but I don't want
>> this CL to get too big, and I'd like to get it in to free up folks that are
>> waiting on it.
>>
>> Zelidrag, you can see that I absorbed your MountPointProvider interface
>> [mostly].  Hopefully your stuff can slot in to a few switch statements and
>> just
>> work.
>>
>> http://codereview.chromium.org/6603034/
>>
>

[Dai Mikurube (NOT FULLTIME), 2011-03-11 18:39:29]

I just looked down briefly at the architecture, your approach looks basically
good. I'll take a finer look later.

[kinuko, 2011-03-14 11:03:57]

Really sorry for my very late review.  Overall its direction looks good, but
since this change is relatively big I have some comments/questions/concerns.

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_mount_point_provider.h (right):

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_mount_point_provider.h:40: virtual bool
IsRestrictedFileName(const FilePath& filename) = 0;
Can this be a const method?

Can we add a comment about which thread it is supposed to be called on (or
whether it can be called on any threads etc)?

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_operation.cc (right):

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_operation.cc:166: &dest_origin_url, &dest_type,
&virtual_path_1)) {
I think we should send back an error (security error or something?) before
deleting this & returning.

dispatcher_->DidFail(base::PLATFORM_FILE_ERROR_SECURITY);

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_operation_context.h (right):

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_operation_context.h:39: GURL src_root_url() const;
src_root_url() and get_root_url() are not single getters and I think they should
be in CamelCase.  Actually, I doubt if we need those methods in this class--
they can be created by calling a single function with public values of this
method, and having both {src,dest}_origin_url() and {src,dest}_root_url() is
confusing.
Do we really need them here?

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_path_manager.cc (right):

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_path_manager.cc:19: #include
"third_party/WebKit/Source/WebKit/chromium/public/WebSecurityOrigin.h"
line 17,19 not needed

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_path_manager.cc:28: using WebKit::WebString;
line 27,28 not needed

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_path_manager.cc:121:
DCHECK(!local_path.ReferencesParent());
Can't we get rid of these checks (line 111 to 121) as we seem to be doing the
same check in CrackFileSystemURL?

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_path_manager.h (right):

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_path_manager.h:97:
scoped_ptr<SandboxMountPointProvider> sandbox_provider_;
Is it supposed to be a pointer to FSMountPointProvider or SandBoxMountProvider? 
The path manager dispatches most of its methods to this instance, and all of
those methods called by the path manager are defined in FSMountPointProvider as
virtual.

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_util.cc (right):

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_util.cc:29: std::string temp = url.path();
Can't we create a new FilePath (e.g. by FilePath().AppendASCII(url.path()) and
call NormalizeWindowsPathSeparators() and ReferencesParent()?

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_util.cc:47: GURL bare_url(temp);
It might be great to have a brief comment to explain how url and bare_url
differ. (Simple example urls for each would be enough)

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_util.h (right):

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_util.h:21: GURL GetFileSystemRootURI(const GURL&
origin_url, fileapi::FileSystemType type);
When we use URL but not URI (or vice versa)?

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_util_unittest.cc (right):

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_util_unittest.cc:83: fileapi::FileSystemType type =
fileapi::kFileSystemTypePersistent;
A test for kFileSystemTypeUnknown?

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/local_file_s...
File webkit/fileapi/local_file_system_file_util.cc (right):

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_file_util.cc:111: GetLocalPath(context,
context->dest_root_url(), context->dest_type(),
Sometimes we call xxx_origin_url() but also call xxx_root_url() for other times.
From a quick glance I think we need to call xxx_origin_url() all the time don't
we?

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_file_util.cc:125: GetLocalPath(context,
context->src_root_url(), context->src_type(),
src_origin_url()?

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_file_util.cc:130: GetLocalPath(context,
context->dest_root_url(), context->dest_type(),
dest_origin_url()?

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_file_util.cc:187: }
I think we want to minimize the code that performs this context check
(!context->file_system_context()) since it is clearly an exceptional case.
Can we DCHECK(context->file_system_context()) here and directly instantiate
FileSystemFileUtil in FileSystemOperation's constructor if raw filesystem access
is required (i.e. no context is given)?

(Also I think we'll want another TODO to have a  SimpleFileSystemContext and
SimpleMountPointProvider for SimpleFileSystem to let each of its operation go
through LocalFSFU)

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_file_util.cc:190: return root;
If an intention is returning an empty path, return FilePath() might be easier to
follow.

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/local_file_s...
File webkit/fileapi/local_file_system_file_util.h (right):

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_file_util.h:36: // routines. It includes
dropping FileAPI-specific routines from FileUtilProxy.
I think you can remove this comment (line 34-36) :)

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/sandbox_moun...
File webkit/fileapi/sandbox_mount_point_provider.cc (right):

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/sandbox_moun...
webkit/fileapi/sandbox_mount_point_provider.cc:18: #include
"third_party/WebKit/Source/WebKit/chromium/public/WebFileSystem.h"
not needed?

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/sandbox_moun...
webkit/fileapi/sandbox_mount_point_provider.cc:181: // static
please remove this comment

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/sandbox_moun...
File webkit/fileapi/sandbox_mount_point_provider.h (right):

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/sandbox_moun...
webkit/fileapi/sandbox_mount_point_provider.h:38: callback);
nits: if we format these lines like following the long line (37, 38) could be
place in one line?  (Here and in file_system_mount_provider.h)

void GetFileSystemRootPath(
    const GURL& origin_url,
    FileSystemType type,
    ...

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/sandbox_moun...
webkit/fileapi/sandbox_mount_point_provider.h:103: FileSystemPathManager*
path_manager_;
Let's add a comment to note that it's not owned by this instance (but this
instance is owned by the path_manager and its lifetime is identical to that of
the manager).

[ericu, 2011-03-15 02:43:11]

I still need to fix the DRT tests, but here's most of it.

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_mount_point_provider.h (right):

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_mount_point_provider.h:40: virtual bool
IsRestrictedFileName(const FilePath& filename) = 0;
On 2011/03/14 11:03:57, kinuko wrote:
> Can this be a const method?

Fixed.

> Can we add a comment about which thread it is supposed to be called on (or
> whether it can be called on any threads etc)?

Done.  Should be callable on any thread, until we come up with a good reason to
restrict it.  It's just meant to be a quick string inspection.

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_operation.cc (right):

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_operation.cc:166: &dest_origin_url, &dest_type,
&virtual_path_1)) {
On 2011/03/14 11:03:57, kinuko wrote:
> I think we should send back an error (security error or something?) before
> deleting this & returning.
> 
> dispatcher_->DidFail(base::PLATFORM_FILE_ERROR_SECURITY);

That's already done inside the VerifyFileSystemPath* methods.

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_operation_context.h (right):

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_operation_context.h:39: GURL src_root_url() const;
On 2011/03/14 11:03:57, kinuko wrote:
> src_root_url() and get_root_url() are not single getters and I think they
should
> be in CamelCase.  Actually, I doubt if we need those methods in this class--
> they can be created by calling a single function with public values of this
> method, and having both {src,dest}_origin_url() and {src,dest}_root_url() is
> confusing.
> Do we really need them here?

Removed.  After fixing a bug, there was only one call left anyway.

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_path_manager.cc (right):

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_path_manager.cc:19: #include
"third_party/WebKit/Source/WebKit/chromium/public/WebSecurityOrigin.h"
On 2011/03/14 11:03:57, kinuko wrote:
> line 17,19 not needed

Done.

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_path_manager.cc:28: using WebKit::WebString;
On 2011/03/14 11:03:57, kinuko wrote:
> line 27,28 not needed

Done.

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_path_manager.cc:121:
DCHECK(!local_path.ReferencesParent());
On 2011/03/14 11:03:57, kinuko wrote:
> Can't we get rid of these checks (line 111 to 121) as we seem to be doing the
> same check in CrackFileSystemURL?

111->117 fixes up separators that are the wrong way [for Windows] when they come
out of CrackFSURL.

119->121 just makes sure that the work /was/ done.  I'd like to leave that in
for now, since I'm going to go in and clean up the underlying call [as
documented in the TODOs], and I want to have lots of checks in to make sure I
didn't break anything.

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_path_manager.h (right):

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_path_manager.h:97:
scoped_ptr<SandboxMountPointProvider> sandbox_provider_;
On 2011/03/14 11:03:57, kinuko wrote:
> Is it supposed to be a pointer to FSMountPointProvider or
SandBoxMountProvider? 
> The path manager dispatches most of its methods to this instance, and all of
> those methods called by the path manager are defined in FSMountPointProvider
as
> virtual.

Currently it's important that this be a SandboxMountPointProvider so that in
worker_process_host.cc we can grab its base_path to grant sandbox exceptions for
its access.  That's sandbox-only code.  I could push the base_path accessor up
to FSMPP, but I'm not sure whether or not it really belongs there, as it's sort
of a sandbox concept [the FileSystem directory, above temporary or persistent]. 
What do you think?

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_util.cc (right):

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_util.cc:29: std::string temp = url.path();
On 2011/03/14 11:03:57, kinuko wrote:
> Can't we create a new FilePath (e.g. by FilePath().AppendASCII(url.path()) and
> call NormalizeWindowsPathSeparators() and ReferencesParent()?

url.path() is something like "file:///temporary/\testdir; it's not yet clean
enough to use FilePath methods, which can't handle mixed separators.

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_util.cc:47: GURL bare_url(temp);
On 2011/03/14 11:03:57, kinuko wrote:
> It might be great to have a brief comment to explain how url and bare_url
> differ. (Simple example urls for each would be enough)

Good point.  Done.

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_util.h (right):

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_util.h:21: GURL GetFileSystemRootURI(const GURL&
origin_url, fileapi::FileSystemType type);
On 2011/03/14 11:03:57, kinuko wrote:
> When we use URL but not URI (or vice versa)?

Ian Hickson suggested that the whole spec switch to URL to match the rest of the
web platform better.  Once I bring that up on-list and put it into the spec, we
should change everything.  I'll try to do that this week, so that we can fix the
API before Chrome 12.

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_util_unittest.cc (right):

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_util_unittest.cc:83: fileapi::FileSystemType type =
fileapi::kFileSystemTypePersistent;
On 2011/03/14 11:03:57, kinuko wrote:
> A test for kFileSystemTypeUnknown?

That will DCHECK(false).  I think it's better to let it go untested, given that
nobody should ever call it, than to take the DCHECK out.

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/local_file_s...
File webkit/fileapi/local_file_system_file_util.cc (right):

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_file_util.cc:111: GetLocalPath(context,
context->dest_root_url(), context->dest_type(),
On 2011/03/14 11:03:57, kinuko wrote:
> Sometimes we call xxx_origin_url() but also call xxx_root_url() for other
times.
> From a quick glance I think we need to call xxx_origin_url() all the time
don't
> we?

You're right--this should be *_origin_url not *_root_url.  This version produces
the right outfit, but wastes cycles.

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_file_util.cc:125: GetLocalPath(context,
context->src_root_url(), context->src_type(),
On 2011/03/14 11:03:57, kinuko wrote:
> src_origin_url()?

Done.

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_file_util.cc:130: GetLocalPath(context,
context->dest_root_url(), context->dest_type(),
On 2011/03/14 11:03:57, kinuko wrote:
> dest_origin_url()?

Done.

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_file_util.cc:187: }
On 2011/03/14 11:03:57, kinuko wrote:
> I think we want to minimize the code that performs this context check
> (!context->file_system_context()) since it is clearly an exceptional case.
> Can we DCHECK(context->file_system_context()) here and directly instantiate
> FileSystemFileUtil in FileSystemOperation's constructor if raw filesystem
access
> is required (i.e. no context is given)?

I like that idea, but I've altered it slightly; those that call the
FileSystemOperation constructor can now force it to accept the
FileSystemFileUtil of their choice.  That way I can get rid of all this
dependence on context here.

However, we still need something in FileSystemOperation.cc inside
VerifyFileSystemPathForRead/Write.  How about I derive a new subclass of
FileSystemOperation, just for use in the tests, that overrides those two
methods?  That way all the test-only code is over in the tests.  If you like
that idea, I'd like to do it in a subsequent CL, to keep this from getting any
bigger.

> (Also I think we'll want another TODO to have a  SimpleFileSystemContext and
> SimpleMountPointProvider for SimpleFileSystem to let each of its operation go
> through LocalFSFU)

Good point.  Let me run the layout tests and see if I broke anything there.
Ah, looks like I broke most of them.  I'll get on that, but am going to send
this revision for review in the mean time.

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_file_util.cc:190: return root;
On 2011/03/14 11:03:57, kinuko wrote:
> If an intention is returning an empty path, return FilePath() might be easier
to
> follow.

Done.

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/local_file_s...
File webkit/fileapi/local_file_system_file_util.h (right):

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_file_util.h:36: // routines. It includes
dropping FileAPI-specific routines from FileUtilProxy.
On 2011/03/14 11:03:57, kinuko wrote:
> I think you can remove this comment (line 34-36) :)

Done.

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/sandbox_moun...
File webkit/fileapi/sandbox_mount_point_provider.cc (right):

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/sandbox_moun...
webkit/fileapi/sandbox_mount_point_provider.cc:18: #include
"third_party/WebKit/Source/WebKit/chromium/public/WebFileSystem.h"
On 2011/03/14 11:03:57, kinuko wrote:
> not needed?

Done.

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/sandbox_moun...
webkit/fileapi/sandbox_mount_point_provider.cc:181: // static
On 2011/03/14 11:03:57, kinuko wrote:
> please remove this comment

Done.

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/sandbox_moun...
File webkit/fileapi/sandbox_mount_point_provider.h (right):

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/sandbox_moun...
webkit/fileapi/sandbox_mount_point_provider.h:38: callback);
On 2011/03/14 11:03:57, kinuko wrote:
> nits: if we format these lines like following the long line (37, 38) could be
> place in one line?  (Here and in file_system_mount_provider.h)
> 
> void GetFileSystemRootPath(
>     const GURL& origin_url,
>     FileSystemType type,
>     ...

Done.

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/sandbox_moun...
webkit/fileapi/sandbox_mount_point_provider.h:103: FileSystemPathManager*
path_manager_;
On 2011/03/14 11:03:57, kinuko wrote:
> Let's add a comment to note that it's not owned by this instance (but this
> instance is owned by the path_manager and its lifetime is identical to that of
> the manager).

Done.

[kinuko, 2011-03-16 19:33:38]

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_operation.cc (right):

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_operation.cc:166: &dest_origin_url, &dest_type,
&virtual_path_1)) {
On 2011/03/15 02:43:11, ericu wrote:
> On 2011/03/14 11:03:57, kinuko wrote:
> > I think we should send back an error (security error or something?) before
> > deleting this & returning.
> > 
> > dispatcher_->DidFail(base::PLATFORM_FILE_ERROR_SECURITY);
> 
> That's already done inside the VerifyFileSystemPath* methods.

Oh, you're right..

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_path_manager.cc (right):

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_path_manager.cc:121:
DCHECK(!local_path.ReferencesParent());
On 2011/03/15 02:43:11, ericu wrote:
> On 2011/03/14 11:03:57, kinuko wrote:
> > Can't we get rid of these checks (line 111 to 121) as we seem to be doing
the
> > same check in CrackFileSystemURL?
> 
> 111->117 fixes up separators that are the wrong way [for Windows] when they
come
> out of CrackFSURL.
> 
> 119->121 just makes sure that the work /was/ done.  I'd like to leave that in
> for now, since I'm going to go in and clean up the underlying call [as
> documented in the TODOs], and I want to have lots of checks in to make sure I
> didn't break anything.

Ok... can we add some comments about that?  (A short one is enough, just to
avoid confusion why we have similar code in multiple places)

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_path_manager.h (right):

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_path_manager.h:97:
scoped_ptr<SandboxMountPointProvider> sandbox_provider_;
On 2011/03/15 02:43:11, ericu wrote:
> On 2011/03/14 11:03:57, kinuko wrote:
> > Is it supposed to be a pointer to FSMountPointProvider or
> SandBoxMountProvider? 
> > The path manager dispatches most of its methods to this instance, and all of
> > those methods called by the path manager are defined in FSMountPointProvider
> as
> > virtual.
> 
> Currently it's important that this be a SandboxMountPointProvider so that in
> worker_process_host.cc we can grab its base_path to grant sandbox exceptions
for
> its access.  That's sandbox-only code.  I could push the base_path accessor up
> to FSMPP, but I'm not sure whether or not it really belongs there, as it's
sort
> of a sandbox concept [the FileSystem directory, above temporary or
persistent]. 
> What do you think?

It's a bit blur how we should separate the roles between the two, but for now
having sandbox_provider as SandboxMountProvider and base_path only in SandboxMPP
lgtm.

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_util.cc (right):

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_util.cc:29: std::string temp = url.path();
On 2011/03/15 02:43:11, ericu wrote:
> On 2011/03/14 11:03:57, kinuko wrote:
> > Can't we create a new FilePath (e.g. by FilePath().AppendASCII(url.path())
and
> > call NormalizeWindowsPathSeparators() and ReferencesParent()?
> 
> url.path() is something like "file:///temporary/\testdir; it's not yet clean
> enough to use FilePath methods, which can't handle mixed separators.

I see, it's a bit sad though.  How about using FilePath for the parent reference
check then?  I just want to get rid of duplicated code, even it may be not super
efficient (I believe it is not significant).

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_util.h (right):

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_util.h:21: GURL GetFileSystemRootURI(const GURL&
origin_url, fileapi::FileSystemType type);
On 2011/03/15 02:43:11, ericu wrote:
> On 2011/03/14 11:03:57, kinuko wrote:
> > When we use URL but not URI (or vice versa)?
> 
> Ian Hickson suggested that the whole spec switch to URL to match the rest of
the
> web platform better.  Once I bring that up on-list and put it into the spec,
we
> should change everything.  I'll try to do that this week, so that we can fix
the
> API before Chrome 12.

Thanks, sgtm.

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/local_file_s...
File webkit/fileapi/local_file_system_file_util.cc (right):

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_file_util.cc:187: }
On 2011/03/15 02:43:11, ericu wrote:
> On 2011/03/14 11:03:57, kinuko wrote:
> > I think we want to minimize the code that performs this context check
> > (!context->file_system_context()) since it is clearly an exceptional case.
> > Can we DCHECK(context->file_system_context()) here and directly instantiate
> > FileSystemFileUtil in FileSystemOperation's constructor if raw filesystem
> access
> > is required (i.e. no context is given)?
> 
> I like that idea, but I've altered it slightly; those that call the
> FileSystemOperation constructor can now force it to accept the
> FileSystemFileUtil of their choice.  That way I can get rid of all this
> dependence on context here.
> 
> However, we still need something in FileSystemOperation.cc inside
> VerifyFileSystemPathForRead/Write.  How about I derive a new subclass of
> FileSystemOperation, just for use in the tests, that overrides those two
> methods?  That way all the test-only code is over in the tests.  If you like
> that idea, I'd like to do it in a subsequent CL, to keep this from getting any
> bigger.

sgtm.

> > (Also I think we'll want another TODO to have a  SimpleFileSystemContext and
> > SimpleMountPointProvider for SimpleFileSystem to let each of its operation
go
> > through LocalFSFU)
> 
> Good point.  Let me run the layout tests and see if I broke anything there.
> Ah, looks like I broke most of them.  I'll get on that, but am going to send
> this revision for review in the mean time.

[ericu, 2011-03-16 23:47:31]

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_path_manager.cc (right):

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_path_manager.cc:121:
DCHECK(!local_path.ReferencesParent());
On 2011/03/16 19:33:38, kinuko wrote:
> On 2011/03/15 02:43:11, ericu wrote:
> > On 2011/03/14 11:03:57, kinuko wrote:
> > > Can't we get rid of these checks (line 111 to 121) as we seem to be doing
> the
> > > same check in CrackFileSystemURL?
> > 
> > 111->117 fixes up separators that are the wrong way [for Windows] when they
> come
> > out of CrackFSURL.
> > 
> > 119->121 just makes sure that the work /was/ done.  I'd like to leave that
in
> > for now, since I'm going to go in and clean up the underlying call [as
> > documented in the TODOs], and I want to have lots of checks in to make sure
I
> > didn't break anything.
> 
> Ok... can we add some comments about that?  (A short one is enough, just to
> avoid confusion why we have similar code in multiple places) 

Added a small addendum to the upper comment in this function.  All the todos in
both functions say that the code is going away anyway; I hope that's clear
enough for now.

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_util.cc (right):

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_util.cc:29: std::string temp = url.path();
On 2011/03/16 19:33:38, kinuko wrote:
> On 2011/03/15 02:43:11, ericu wrote:
> > On 2011/03/14 11:03:57, kinuko wrote:
> > > Can't we create a new FilePath (e.g. by FilePath().AppendASCII(url.path())
> and
> > > call NormalizeWindowsPathSeparators() and ReferencesParent()?
> > 
> > url.path() is something like "file:///temporary/\testdir; it's not yet clean
> > enough to use FilePath methods, which can't handle mixed separators.
> 
> I see, it's a bit sad though.  How about using FilePath for the parent
reference
> check then?  I just want to get rid of duplicated code, even it may be not
super
> efficient (I believe it is not significant).

Using FilePath wouldn't shrink the code much.  First we'd have to cut off the
part that the FilePath could handle, since "file:///temporary/\testdir" isn't
really valid for it.  This whole body of code is going away soon, so I'd really
prefer not to spend too much time optimizing it.  I certainly agree that it's
sad, though.

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_util.h (right):

http://codereview.chromium.org/6603034/diff/25001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_util.h:21: GURL GetFileSystemRootURI(const GURL&
origin_url, fileapi::FileSystemType type);
On 2011/03/15 02:43:11, ericu wrote:
> On 2011/03/14 11:03:57, kinuko wrote:
> > When we use URL but not URI (or vice versa)?
> 
> Ian Hickson suggested that the whole spec switch to URL to match the rest of
the
> web platform better.  Once I bring that up on-list and put it into the spec,
we
> should change everything.  I'll try to do that this week, so that we can fix
the
> API before Chrome 12.

I've now made and announced this spec change, and logged a webkit bug for the
interface change.

[ericu, 2011-03-20 00:20:22]

Please take another look; I've put the webkit changes up at
https://bugs.webkit.org/show_bug.cgi?id=56704, and this CL now depends on them
[so trybots can't run], but everything works on my machine.

[ericu, 2011-03-21 21:33:58]

The webkit patch has landed; this should be ready to go as of the next
webkit roll.

On Sat, Mar 19, 2011 at 5:20 PM,  <ericu@chromium.org> wrote:
> Please take another look; I've put the webkit changes up at
> https://bugs.webkit.org/show_bug.cgi?id=56704, and this CL now depends on
> them
> [so trybots can't run], but everything works on my machine.
>
> http://codereview.chromium.org/6603034/
>

[kinuko, 2011-03-22 00:16:24]

I would make a separate changeset for changes in platform_file and
webfilesystem_callback_dispatcher, and there're still some concerns but I
believe/hope they will go away soon.

LGTM

http://codereview.chromium.org/6603034/diff/47012/content/common/file_system/...
File content/common/file_system/webfilesystem_impl.h (right):

http://codereview.chromium.org/6603034/diff/47012/content/common/file_system/...
content/common/file_system/webfilesystem_impl.h:67: 
No need to change this file?

http://codereview.chromium.org/6603034/diff/47012/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_usage_tracker.h (right):

http://codereview.chromium.org/6603034/diff/47012/webkit/fileapi/file_system_...
webkit/fileapi/file_system_usage_tracker.h:32: // FileSystemContext.
This class will need to be overhauled anyway, I think you can get rid of this
TODO.

http://codereview.chromium.org/6603034/diff/47012/webkit/fileapi/local_file_s...
File webkit/fileapi/local_file_system_file_util.cc (right):

http://codereview.chromium.org/6603034/diff/47012/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_file_util.cc:17: FilePath GetRootPath(
Why do we need a separate local function just to call
GetFileSystemRootPathOnFileThread()?

http://codereview.chromium.org/6603034/diff/47012/webkit/fileapi/local_file_s...
File webkit/fileapi/local_file_system_file_util.h (right):

http://codereview.chromium.org/6603034/diff/47012/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_file_util.h:43: //
PLATFORM_FILE_ERROR_FILE_EXISTS if the |file_path| already exists.
Should be ok to remove all the comments for the methods derived from the base
class.

http://codereview.chromium.org/6603034/diff/47012/webkit/tools/test_shell/sim...
File webkit/tools/test_shell/simple_file_writer.cc (right):

http://codereview.chromium.org/6603034/diff/47012/webkit/tools/test_shell/sim...
webkit/tools/test_shell/simple_file_writer.cc:128: io_thread_,
file_system_context_.get(),
Yeah, I was thinking we should have done this.

[ericu, 2011-03-22 00:40:31]

I also did a few lint cleanups.
I'll wait for the next webkit roll, then run a try job before submitting.

I'll take a look at splitting out those two files, but I think there may be some
bits connected to those, so I'm not sure whether it's that easy or not.

http://codereview.chromium.org/6603034/diff/47012/content/common/file_system/...
File content/common/file_system/webfilesystem_impl.h (right):

http://codereview.chromium.org/6603034/diff/47012/content/common/file_system/...
content/common/file_system/webfilesystem_impl.h:67: 
On 2011/03/22 00:16:24, kinuko wrote:
> No need to change this file?

Reverted.

http://codereview.chromium.org/6603034/diff/47012/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_usage_tracker.h (right):

http://codereview.chromium.org/6603034/diff/47012/webkit/fileapi/file_system_...
webkit/fileapi/file_system_usage_tracker.h:32: // FileSystemContext.
On 2011/03/22 00:16:24, kinuko wrote:
> This class will need to be overhauled anyway, I think you can get rid of this
> TODO.

Done.

http://codereview.chromium.org/6603034/diff/47012/webkit/fileapi/local_file_s...
File webkit/fileapi/local_file_system_file_util.cc (right):

http://codereview.chromium.org/6603034/diff/47012/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_file_util.cc:17: FilePath GetRootPath(
On 2011/03/22 00:16:24, kinuko wrote:
> Why do we need a separate local function just to call
> GetFileSystemRootPathOnFileThread()?

Good point.  In an earlier version it was called more than once.  Removed.

http://codereview.chromium.org/6603034/diff/47012/webkit/fileapi/local_file_s...
File webkit/fileapi/local_file_system_file_util.h (right):

http://codereview.chromium.org/6603034/diff/47012/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_file_util.h:43: //
PLATFORM_FILE_ERROR_FILE_EXISTS if the |file_path| already exists.
On 2011/03/22 00:16:24, kinuko wrote:
> Should be ok to remove all the comments for the methods derived from the base
> class.

Done.

[ericu, 2011-03-22 21:46:43]

On Mon, Mar 21, 2011 at 5:16 PM,  <kinuko@chromium.org> wrote:
> I would make a separate changeset for changes in platform_file and
> webfilesystem_callback_dispatcher, and there're still some concerns but I
> believe/hope they will go away soon.

If you think it would be better, I could create a new
filesystem-specific fileinfo object to replace our use of the
PlatformFileInfo.  Then we could remove the new path field from
PlatformFileInfo, since nobody else is using it.  I'd prefer to do
this in a separate CL, though, in the interest of speed.

	Eric

[kinuko, 2011-03-22 22:11:53]

On 2011/03/22 21:46:43, ericu wrote:
> On Mon, Mar 21, 2011 at 5:16 PM,  <mailto:kinuko@chromium.org> wrote:
> > I would make a separate changeset for changes in platform_file and
> > webfilesystem_callback_dispatcher, and there're still some concerns but I
> > believe/hope they will go away soon.
> 
> If you think it would be better, I could create a new
> filesystem-specific fileinfo object to replace our use of the
> PlatformFileInfo.  Then we could remove the new path field from
> PlatformFileInfo, since nobody else is using it.  I'd prefer to do
> this in a separate CL, though, in the interest of speed.

Ok, let's run try jobs and land this.
LGTM

[zel, 2011-03-22 22:12:13]

Yes, I am also voting for separate CL on all remaining issues for my own
selfish reasons :)


On Tue, Mar 22, 2011 at 2:46 PM, Eric Uhrhane <ericu@chromium.org> wrote:

> On Mon, Mar 21, 2011 at 5:16 PM,  <kinuko@chromium.org> wrote:
> > I would make a separate changeset for changes in platform_file and
> > webfilesystem_callback_dispatcher, and there're still some concerns but I
> > believe/hope they will go away soon.
>
> If you think it would be better, I could create a new
> filesystem-specific fileinfo object to replace our use of the
> PlatformFileInfo.  Then we could remove the new path field from
> PlatformFileInfo, since nobody else is using it.  I'd prefer to do
> this in a separate CL, though, in the interest of speed.
>
>        Eric
>

[ericu, 2011-03-22 22:13:01]

On Tue, Mar 22, 2011 at 3:11 PM,  <kinuko@chromium.org> wrote:
> On 2011/03/22 21:46:43, ericu wrote:
>>
>> On Mon, Mar 21, 2011 at 5:16 PM,  <mailto:kinuko@chromium.org> wrote:
>> > I would make a separate changeset for changes in platform_file and
>> > webfilesystem_callback_dispatcher, and there're still some concerns but
>> > I
>> > believe/hope they will go away soon.
>
>> If you think it would be better, I could create a new
>> filesystem-specific fileinfo object to replace our use of the
>> PlatformFileInfo.  Then we could remove the new path field from
>> PlatformFileInfo, since nobody else is using it.  I'd prefer to do
>> this in a separate CL, though, in the interest of speed.
>
> Ok, let's run try jobs and land this.

Great!  Michael's running the try job for me, since I'm at home today.

> LGTM
>
> http://codereview.chromium.org/6603034/
>

[ericu, 2011-03-23 18:20:19]

I need a review from someone in content/OWNERS, too.
Could one of you please take a look?

Thanks,

	Eric

[brettw, 2011-03-26 17:55:36]

I only gave a superficial review for content and it LGTM (if you still need
that). Sorry for the delay.