Add initial code to do filename munging in the FileSystem....

Add initial code to do filename munging in the FileSystem.
To start off with, it's all turned off and only part of it is implemented.
We can't turn it on until it can detect and migrate older data.

BUG=71635
TEST=none yet.

[ericu, 2011-02-09 00:11:17]

Don't panic.  This isn't going to go in as a single changelist.

This is my overall CL for filename obfuscation.  I'd like you to take a look at
it and see if you like the overall organization before I fill in all the
details.  I've only finished directory creation so far, but that's enough to
prove out a lot of the infrastructure, so I'd like to get feedback before I go
too much further.

You won't want to look at all the files; here are some notes on where the
important stuff is and why.

Thanks!


file_util_proxy_base.h: Factored out a common base class for FileUtilProxy and
ObfuscatedFileUtilProxy.  It defines the interface that callers use. 
ObfuscatedFileUtilProxy happens to extend FileUtilProxy, but nobody has to care
about that.  Instead of calling into static methods on FileUtilProxy, now
callers will use a singleton object.  That lets us store just a pointer to the
object, so that we can swap in the obfuscated version where we want it.  This
change caused *most* of the files in this changelist to be included, but they're
almost all trivial changes.  I still have to go back in and adjust a bunch of
header file includes [to file_util_proxy_base.h instead of file_util_proxy.h].

FileSystemPathManager::CrackFileSystemPath now returns the canonical form of the
root of the filesystem [including the random directory and temporary or
persistent] as well as the virtual component.

obfuscated_file_util.h/cc supply the file_util methods that require obfuscation.
 Basically that's anything that's used by the FileSystem that uses a path.  If
you just need to close an open PlatformFile, you can use file_util directly.  So
far I've only got a complete implementation of CreateDirectory, although the
lowest-level code is there for a bit more, and CreateOrOpen will come next.

obfuscated_file_util_proxy.h/cc are like FileUtilProxy, but with obfuscation. 
All work is done on the file thread, in a single operation, so this class will
be very thin.  By putting all the real work in ObfuscatedFileUtil, we make that
available to anybody else who wants to call it from the file thread [possibly
Pepper, or Adam's URI code].

obfuscated_name_map.h/cc represents the on-disk dictionary that maps between
hashes and real names.  It only deals with names, not paths, as it understands
only a single directory.  It handles the journal-style updates and crash
recovery.

path_obfuscator.h/cc is a higher-level interface that handles paths, rather than
names.

The SandboxedFileSystemContext will now hold on to a FileUtilProxyBase, which
we'll swap in [as FileUtilProxy or ObfuscatedFileUtilProxy] based on a flag for
now.  During the transitional period we'll first have all this code turned off
by compile flag and runtime flag, then just have it behind a runtime flag, then
we'll take out the flag and have migration code that might need both versions [I
haven't specced that out yet], and then eventually drop the use of the
nonobfuscated version for sandboxed filesystems, but use it for other mount
points.

Given that FileUtilProxy is a singleton and ObfuscatedFileUtilProxy isn't, we
currently need a flag to decide whether to delete the object held by the
SandboxedFileSystemContext or not.  I'm not really happy with this solution, but
I haven't been able to come up with a better one yet.

[kinuko, 2011-02-09 02:58:38]

I have only take a really quick glance of some of files, but here's my initial
comment:
as for obfuscated_file_util/obfuscated_file_util_proxy, we (quota side) were
also be going to make some another version of file_util{,_proxy} code to track
usages.

Just an idea, since you're trying to make this FileUtilProxy stuff switchable,
can we also make these operations-on-filethread stuff somehow stackable (like we
did in FileSystemOperation in the caller side) so that each change can be made
independently?
This can be done later, or changes can be staged, depending on your and our
target date.  (Especially our side of changes are not ready yet)
Wdyt?


On 2011/02/09 00:11:17, ericu wrote:
> Don't panic.  This isn't going to go in as a single changelist.
> 
> This is my overall CL for filename obfuscation.  I'd like you to take a look
at
> it and see if you like the overall organization before I fill in all the
> details.  I've only finished directory creation so far, but that's enough to
> prove out a lot of the infrastructure, so I'd like to get feedback before I go
> too much further.
> 
> You won't want to look at all the files; here are some notes on where the
> important stuff is and why.
> 
> Thanks!
> 
> 
> file_util_proxy_base.h: Factored out a common base class for FileUtilProxy and
> ObfuscatedFileUtilProxy.  It defines the interface that callers use. 
> ObfuscatedFileUtilProxy happens to extend FileUtilProxy, but nobody has to
care
> about that.  Instead of calling into static methods on FileUtilProxy, now
> callers will use a singleton object.  That lets us store just a pointer to the
> object, so that we can swap in the obfuscated version where we want it.  This
> change caused *most* of the files in this changelist to be included, but
they're
> almost all trivial changes.  I still have to go back in and adjust a bunch of
> header file includes [to file_util_proxy_base.h instead of file_util_proxy.h].
> 
> FileSystemPathManager::CrackFileSystemPath now returns the canonical form of
the
> root of the filesystem [including the random directory and temporary or
> persistent] as well as the virtual component.
> 
> obfuscated_file_util.h/cc supply the file_util methods that require
obfuscation.
>  Basically that's anything that's used by the FileSystem that uses a path.  If
> you just need to close an open PlatformFile, you can use file_util directly. 
So
> far I've only got a complete implementation of CreateDirectory, although the
> lowest-level code is there for a bit more, and CreateOrOpen will come next.
> 
> obfuscated_file_util_proxy.h/cc are like FileUtilProxy, but with obfuscation. 
> All work is done on the file thread, in a single operation, so this class will
> be very thin.  By putting all the real work in ObfuscatedFileUtil, we make
that
> available to anybody else who wants to call it from the file thread [possibly
> Pepper, or Adam's URI code].
> 
> obfuscated_name_map.h/cc represents the on-disk dictionary that maps between
> hashes and real names.  It only deals with names, not paths, as it understands
> only a single directory.  It handles the journal-style updates and crash
> recovery.
> 
> path_obfuscator.h/cc is a higher-level interface that handles paths, rather
than
> names.
> 
> The SandboxedFileSystemContext will now hold on to a FileUtilProxyBase, which
> we'll swap in [as FileUtilProxy or ObfuscatedFileUtilProxy] based on a flag
for
> now.  During the transitional period we'll first have all this code turned off
> by compile flag and runtime flag, then just have it behind a runtime flag,
then
> we'll take out the flag and have migration code that might need both versions
[I
> haven't specced that out yet], and then eventually drop the use of the
> nonobfuscated version for sandboxed filesystems, but use it for other mount
> points.
> 
> Given that FileUtilProxy is a singleton and ObfuscatedFileUtilProxy isn't, we
> currently need a flag to decide whether to delete the object held by the
> SandboxedFileSystemContext or not.  I'm not really happy with this solution,
but
> I haven't been able to come up with a better one yet.

[michaeln, 2011-02-11 00:03:34]

Definitely want some unittests for the the Obfuscated[Path|Name] classes.

http://codereview.chromium.org/6286038/diff/83001/webkit/fileapi/obfuscated_f...
File webkit/fileapi/obfuscated_file_util.h (right):

http://codereview.chromium.org/6286038/diff/83001/webkit/fileapi/obfuscated_f...
webkit/fileapi/obfuscated_file_util.h:15: namespace obfuscated_file_util {
i don't think we generally use namespaces for this type of scoping, i think
these should all be in the fileapi namespace.

http://codereview.chromium.org/6286038/diff/83001/webkit/fileapi/obfuscated_n...
File webkit/fileapi/obfuscated_name_map.cc (right):

http://codereview.chromium.org/6286038/diff/83001/webkit/fileapi/obfuscated_n...
webkit/fileapi/obfuscated_name_map.cc:21: , map_next_valid_(false) {
Massage the style here to chrome-land style :) Or have we adopted this style for
init lists too?

http://codereview.chromium.org/6286038/diff/83001/webkit/fileapi/obfuscated_n...
webkit/fileapi/obfuscated_name_map.cc:37: {
is the extra scope here intentional?

http://codereview.chromium.org/6286038/diff/83001/webkit/fileapi/obfuscated_n...
webkit/fileapi/obfuscated_name_map.cc:119: map_ = map_next_;
i wonder if you can use the swap method here?
  map_.clear();
  map_.swap(map_next_);

http://codereview.chromium.org/6286038/diff/83001/webkit/fileapi/obfuscated_n...
webkit/fileapi/obfuscated_name_map.cc:159: PlatformFileError
ObfuscatedNameMap::LoadFromFile() {
maybe rename the method to make it more clear that if the file is already
loaded, this is a noop... LazyLoadFromFile()?

http://codereview.chromium.org/6286038/diff/83001/webkit/fileapi/obfuscated_n...
webkit/fileapi/obfuscated_name_map.cc:242: PlatformFileError
ObfuscatedNameMap::InitDictNext() {
InitNextMap()? This is the only place the term 'dict' shows up in this class
although i see that term is used elsewhere in the CL.

http://codereview.chromium.org/6286038/diff/83001/webkit/fileapi/obfuscated_n...
webkit/fileapi/obfuscated_name_map.cc:244: if (!map_valid_) {
Given how LoadFromFile works, i don't think you need to test map_valid_ here.
(maybe that's where the extra scope above came from :)

http://codereview.chromium.org/6286038/diff/83001/webkit/fileapi/obfuscated_n...
webkit/fileapi/obfuscated_name_map.cc:280: data = contents;
another potential swap() callsite?

http://codereview.chromium.org/6286038/diff/83001/webkit/fileapi/obfuscated_n...
webkit/fileapi/obfuscated_name_map.cc:289: error = wcstombs_s(&converted,
buffer, BUFFER_SIZE, contents.c_str() +
is there's no util that can do this conversion for you? would it make any sense
to put this somewhere in 'base' as a utility in case this particular conversion
is needed elsewhere?

also, some comments about these char set conversions would be helpful.

http://codereview.chromium.org/6286038/diff/83001/webkit/fileapi/obfuscated_n...
webkit/fileapi/obfuscated_name_map.cc:316: const StringType
ObfuscatedNameMap::DICTIONARY_NAME =
Since StringType is a class with a constructor, i don't think we want these as
globals. I think we have to construct/init these lazily.

http://codereview.chromium.org/6286038/diff/83001/webkit/fileapi/obfuscated_n...
File webkit/fileapi/obfuscated_name_map.h (right):

http://codereview.chromium.org/6286038/diff/83001/webkit/fileapi/obfuscated_n...
webkit/fileapi/obfuscated_name_map.h:22: // object.
class comments about what this class does might be better here, these comments
about Rollback/Commit usage might be better in next to those methods down below?

http://codereview.chromium.org/6286038/diff/83001/webkit/fileapi/obfuscated_n...
webkit/fileapi/obfuscated_name_map.h:53: void RollBack();
There's precedence in the project for 'Rollback' w/o the capital B, see
transaction.h for example.

http://codereview.chromium.org/6286038/diff/83001/webkit/fileapi/path_obfusca...
File webkit/fileapi/path_obfuscator.cc (right):

http://codereview.chromium.org/6286038/diff/83001/webkit/fileapi/path_obfusca...
webkit/fileapi/path_obfuscator.cc:10: namespace obfuscated_file_util {
not sure we want a new namespace for this, can we just put this stuff in the
fileapi namespace

http://codereview.chromium.org/6286038/diff/83001/webkit/fileapi/path_obfusca...
webkit/fileapi/path_obfuscator.cc:63: {
extra scope?

http://codereview.chromium.org/6286038/diff/83001/webkit/fileapi/path_obfusca...
webkit/fileapi/path_obfuscator.cc:155: to_map_.RollBack();
do you want to reset the flags here?

http://codereview.chromium.org/6286038/diff/83001/webkit/fileapi/path_obfusca...
File webkit/fileapi/path_obfuscator.h (right):

http://codereview.chromium.org/6286038/diff/83001/webkit/fileapi/path_obfusca...
webkit/fileapi/path_obfuscator.h:60: ObfuscatedNameMap from_map_, to_map_;
we usually declare one data member per line, but i'm not sure if that's required
by the style guide?

[ericu, 2011-02-11 01:57:47]

http://codereview.chromium.org/6286038/diff/83001/webkit/fileapi/obfuscated_f...
File webkit/fileapi/obfuscated_file_util.h (right):

http://codereview.chromium.org/6286038/diff/83001/webkit/fileapi/obfuscated_f...
webkit/fileapi/obfuscated_file_util.h:15: namespace obfuscated_file_util {
On 2011/02/11 00:03:34, michaeln wrote:
> i don't think we generally use namespaces for this type of scoping, i think
> these should all be in the fileapi namespace.

This matches base::file_util.  If I dump this all into the fileapi namespace, we
end up with file_util::CreateOrOpen vs. either just bare calls to CreateOrOpen
from inside fileapi, which might be confusing, or fully-qualified calls to
fileapi::CreateOrOpen, which isn't obviously something that obfuscates.

However, I think the cleaner solution is to go with something like what Dai
wrote up: we always access the obfuscated calls through an object of some sort,
and we make an object with similar interface to call out to file_util, so we get
obvious qualification on every call.  How does that sound?

http://codereview.chromium.org/6286038/diff/83001/webkit/fileapi/obfuscated_n...
File webkit/fileapi/obfuscated_name_map.cc (right):

http://codereview.chromium.org/6286038/diff/83001/webkit/fileapi/obfuscated_n...
webkit/fileapi/obfuscated_name_map.cc:21: , map_next_valid_(false) {
On 2011/02/11 00:03:34, michaeln wrote:
> Massage the style here to chrome-land style :) Or have we adopted this style
for
> init lists too?

No, that's an accidental webkit-style leak.

http://codereview.chromium.org/6286038/diff/83001/webkit/fileapi/obfuscated_n...
webkit/fileapi/obfuscated_name_map.cc:37: {
On 2011/02/11 00:03:34, michaeln wrote:
> is the extra scope here intentional?

It was just to limit the scope of error, but that seems a bit heavy-handed now. 
I think that came from an earlier attempt that was more complicated.  I'll take
it out.

http://codereview.chromium.org/6286038/diff/83001/webkit/fileapi/obfuscated_n...
webkit/fileapi/obfuscated_name_map.cc:119: map_ = map_next_;
On 2011/02/11 00:03:34, michaeln wrote:
> i wonder if you can use the swap method here?
>   map_.clear();
>   map_.swap(map_next_);

Yup.

http://codereview.chromium.org/6286038/diff/83001/webkit/fileapi/obfuscated_n...
webkit/fileapi/obfuscated_name_map.cc:159: PlatformFileError
ObfuscatedNameMap::LoadFromFile() {
On 2011/02/11 00:03:34, michaeln wrote:
> maybe rename the method to make it more clear that if the file is already
> loaded, this is a noop... LazyLoadFromFile()?

Will do.

http://codereview.chromium.org/6286038/diff/83001/webkit/fileapi/obfuscated_n...
webkit/fileapi/obfuscated_name_map.cc:242: PlatformFileError
ObfuscatedNameMap::InitDictNext() {
On 2011/02/11 00:03:34, michaeln wrote:
> InitNextMap()? This is the only place the term 'dict' shows up in this class
> although i see that term is used elsewhere in the CL.

I'll try to clean up the map vs. dict terminology.  Maybe I should just drop
dict entirely, but I liked having it refer to the on-disk dictionary, vs. the
in-memory map.  This method isn't named properly anyway, though.

http://codereview.chromium.org/6286038/diff/83001/webkit/fileapi/obfuscated_n...
webkit/fileapi/obfuscated_name_map.cc:244: if (!map_valid_) {
On 2011/02/11 00:03:34, michaeln wrote:
> Given how LoadFromFile works, i don't think you need to test map_valid_ here.
> (maybe that's where the extra scope above came from :)

You mean just always call it, since it's lazy?  Yeah, that works.

http://codereview.chromium.org/6286038/diff/83001/webkit/fileapi/obfuscated_n...
webkit/fileapi/obfuscated_name_map.cc:280: data = contents;
On 2011/02/11 00:03:34, michaeln wrote:
> another potential swap() callsite?

Yup.

http://codereview.chromium.org/6286038/diff/83001/webkit/fileapi/obfuscated_n...
webkit/fileapi/obfuscated_name_map.cc:289: error = wcstombs_s(&converted,
buffer, BUFFER_SIZE, contents.c_str() +
On 2011/02/11 00:03:34, michaeln wrote:
> is there's no util that can do this conversion for you? would it make any
sense
> to put this somewhere in 'base' as a utility in case this particular
conversion
> is needed elsewhere?

I'm not sure that base is the place, given that I'm converting from StringType,
which is a FilePath thing.  Possibly in FilePath, though?  Hmm...actually, this
only works because I know that StringType is a Windows wstring, so I guess it
could go in base with that signature.

> also, some comments about these char set conversions would be helpful.

Will add some.

http://codereview.chromium.org/6286038/diff/83001/webkit/fileapi/obfuscated_n...
webkit/fileapi/obfuscated_name_map.cc:316: const StringType
ObfuscatedNameMap::DICTIONARY_NAME =
On 2011/02/11 00:03:34, michaeln wrote:
> Since StringType is a class with a constructor, i don't think we want these as
> globals. I think we have to construct/init these lazily.

Good point.  Converted to StringType::value_type.

http://codereview.chromium.org/6286038/diff/83001/webkit/fileapi/obfuscated_n...
File webkit/fileapi/obfuscated_name_map.h (right):

http://codereview.chromium.org/6286038/diff/83001/webkit/fileapi/obfuscated_n...
webkit/fileapi/obfuscated_name_map.h:22: // object.
On 2011/02/11 00:03:34, michaeln wrote:
> class comments about what this class does might be better here, these comments
> about Rollback/Commit usage might be better in next to those methods down
below?
> 

Hmm...I thought it best to put "how to reuse this class" type stuff up here,
since it concerns object lifetime.  I can clean it up a bit and add some stuff
near RollBack and Commit as well, but I don't want to have redundant comments.

http://codereview.chromium.org/6286038/diff/83001/webkit/fileapi/obfuscated_n...
webkit/fileapi/obfuscated_name_map.h:53: void RollBack();
On 2011/02/11 00:03:34, michaeln wrote:
> There's precedence in the project for 'Rollback' w/o the capital B, see
> transaction.h for example.

OK, will change.

http://codereview.chromium.org/6286038/diff/83001/webkit/fileapi/path_obfusca...
File webkit/fileapi/path_obfuscator.cc (right):

http://codereview.chromium.org/6286038/diff/83001/webkit/fileapi/path_obfusca...
webkit/fileapi/path_obfuscator.cc:10: namespace obfuscated_file_util {
On 2011/02/11 00:03:34, michaeln wrote:
> not sure we want a new namespace for this, can we just put this stuff in the
> fileapi namespace

Can do, but see the other comment about encapsulation for file_util vs.
obfuscated_file_util.

http://codereview.chromium.org/6286038/diff/83001/webkit/fileapi/path_obfusca...
webkit/fileapi/path_obfuscator.cc:63: {
On 2011/02/11 00:03:34, michaeln wrote:
> extra scope?

Will remove.

http://codereview.chromium.org/6286038/diff/83001/webkit/fileapi/path_obfusca...
webkit/fileapi/path_obfuscator.cc:155: to_map_.RollBack();
On 2011/02/11 00:03:34, michaeln wrote:
> do you want to reset the flags here?

Yes--thanks.

http://codereview.chromium.org/6286038/diff/83001/webkit/fileapi/path_obfusca...
File webkit/fileapi/path_obfuscator.h (right):

http://codereview.chromium.org/6286038/diff/83001/webkit/fileapi/path_obfusca...
webkit/fileapi/path_obfuscator.h:60: ObfuscatedNameMap from_map_, to_map_;
On 2011/02/11 00:03:34, michaeln wrote:
> we usually declare one data member per line, but i'm not sure if that's
required
> by the style guide?

An easy fix, anyway.

[darin (slow to review), 2011-02-11 18:16:10]

http://codereview.chromium.org/6286038/diff/83001/base/file_util_proxy_base.h
File base/file_util_proxy_base.h (right):

http://codereview.chromium.org/6286038/diff/83001/base/file_util_proxy_base.h...
base/file_util_proxy_base.h:17: class FileUtilProxyBase {
what's the point of this base class?  if the goal is to do file name munging...
couldn't that be better expressed with a filter interface passed to a
FileUtilProxy object?

[ericu, 2011-02-12 00:05:48]

On Fri, Feb 11, 2011 at 10:16 AM,  <darin@chromium.org> wrote:
>
> http://codereview.chromium.org/6286038/diff/83001/base/file_util_proxy_base.h
> File base/file_util_proxy_base.h (right):
>
>
http://codereview.chromium.org/6286038/diff/83001/base/file_util_proxy_base.h...
> base/file_util_proxy_base.h:17: class FileUtilProxyBase {
> what's the point of this base class?  if the goal is to do file name
> munging... couldn't that be better expressed with a filter interface
> passed to a FileUtilProxy object?
>
> http://codereview.chromium.org/6286038/

Darin and I discussed this offline; the removal of all the changes to
base remove the need for the question.

[ericu, 2011-02-19 02:19:22]

I've uploaded a new version.  Kinuko+Dai:
I'd appreciate it if you could take a look at how I've used
FileSystemOperationContext, to see if it fits what you're thinking of.

[Dai Mikurube (google.com), 2011-02-21 07:10:40]

On 2011/02/19 02:19:22, ericu wrote:
> I've uploaded a new version.  Kinuko+Dai:
> I'd appreciate it if you could take a look at how I've used
> FileSystemOperationContext, to see if it fits what you're thinking of.

The stacking LGTM. Thank you reflecting the comments.

[Dai Mikurube (google.com), 2011-02-21 07:33:25]

On 2011/02/21 07:10:40, Dai Mikurube wrote:
> On 2011/02/19 02:19:22, ericu wrote:
> > I've uploaded a new version.  Kinuko+Dai:
> > I'd appreciate it if you could take a look at how I've used
> > FileSystemOperationContext, to see if it fits what you're thinking of.
> 
> The stacking LGTM. Thank you reflecting the comments.

In addition, some changes to FileSystemDirURLRequestJob and
FileSystemURLRequestJob might be required such as
http://codereview.chromium.org/6471019/ . They use FileUtilProxy::ReadDirectory.

[Dai Mikurube (google.com), 2011-02-22 02:57:34]

Hi Eric,

Just 2 small changes when I tried compiling.

http://codereview.chromium.org/6286038/diff/105041/webkit/fileapi/obfuscated_...
File webkit/fileapi/obfuscated_name_map.cc (right):

http://codereview.chromium.org/6286038/diff/105041/webkit/fileapi/obfuscated_...
webkit/fileapi/obfuscated_name_map.cc:141: struct __gnu__cxx::hash<FilePath>
gcc_hash;
s/__gnu__cxx/__gnu_cxx/

http://codereview.chromium.org/6286038/diff/105041/webkit/fileapi/obfuscated_...
File webkit/fileapi/obfuscated_name_map.h (right):

http://codereview.chromium.org/6286038/diff/105041/webkit/fileapi/obfuscated_...
webkit/fileapi/obfuscated_name_map.h:80: FilePath directory_;
Move this declaration before "bool map_valid_;"

[Dai Mikurube (google.com), 2011-02-22 07:41:26]

I found that the current stacking fails (segmentation fault) with
obfuscate=false and the two fixes above.

Drag-and-dropping files into
http://www.html5rocks.com/tutorials/file/filesystem/ often causes segmentation
fault.

I'm investigating...

[Dai Mikurube (google.com), 2011-02-22 07:45:21]

On 2011/02/22 07:41:26, Dai Mikurube wrote:
> I found that the current stacking fails (segmentation fault) with
> obfuscate=false and the two fixes above.
> 
> Drag-and-dropping files into
> http://www.html5rocks.com/tutorials/file/filesystem/ often causes segmentation
> fault.
> 
> I'm investigating...

The failure occurs only in "Write" operation in my observation. Copy and Move
operations work well.

[Dai Mikurube (google.com), 2011-02-22 08:18:04]

The following change looks fixing the failure.

http://codereview.chromium.org/6286038/diff/105041/webkit/fileapi/file_system...
File webkit/fileapi/file_system_file_util_proxy.cc (right):

http://codereview.chromium.org/6286038/diff/105041/webkit/fileapi/file_system...
webkit/fileapi/file_system_file_util_proxy.cc:90:
base::FileUtilProxy::Close(message_loop_proxy_, file_handle_, NULL);
It should be :
fileapi::FileSystemFileUtilProxy::Close(context(), message_loop_proxy_,
file_handle_, NULL);

[Dai Mikurube (google.com), 2011-02-22 08:32:15]

On 2011/02/22 08:18:04, Dai Mikurube wrote:
> The following change looks fixing the failure.
> 
>
http://codereview.chromium.org/6286038/diff/105041/webkit/fileapi/file_system...
> File webkit/fileapi/file_system_file_util_proxy.cc (right):
> 
>
http://codereview.chromium.org/6286038/diff/105041/webkit/fileapi/file_system...
> webkit/fileapi/file_system_file_util_proxy.cc:90:
> base::FileUtilProxy::Close(message_loop_proxy_, file_handle_, NULL);
> It should be :
> fileapi::FileSystemFileUtilProxy::Close(context(), message_loop_proxy_,
> file_handle_, NULL);

Sorry, I found the failure reproducing...

[Dai Mikurube (google.com), 2011-02-22 10:13:53]

Hi Eric,

Found the point. The following change fixes the failure though I'm not sure
whether it's good to call FileUtilProxy functions from FileSystem functions.

http://codereview.chromium.org/6286038/diff/105041/webkit/fileapi/file_system...
File webkit/fileapi/file_system_file_util_proxy.cc (right):

http://codereview.chromium.org/6286038/diff/105041/webkit/fileapi/file_system...
webkit/fileapi/file_system_file_util_proxy.cc:90:
base::FileUtilProxy::Close(message_loop_proxy_, file_handle_, NULL);
On 2011/02/22 08:18:04, Dai Mikurube wrote:
> It should be :
> fileapi::FileSystemFileUtilProxy::Close(context(), message_loop_proxy_,
> file_handle_, NULL);

It was opposite to the correct way. Revert it.
FileSystemOperation::~FileSystemOperation is to be fixed.

http://codereview.chromium.org/6286038/diff/105041/webkit/fileapi/file_system...
File webkit/fileapi/file_system_operation.cc (right):

http://codereview.chromium.org/6286038/diff/105041/webkit/fileapi/file_system...
webkit/fileapi/file_system_operation.cc:39: proxy_,
file_writer_delegate_->file(), NULL);
It's to be fixed as follows :
base::FileUtilProxy::Close(
    _proxy, file_writer_delegate_->file(), NULL);

[kinuko, 2011-02-22 11:13:39]

http://codereview.chromium.org/6286038/diff/105041/webkit/fileapi/file_system...
File webkit/fileapi/file_system_file_util_proxy.cc (right):

http://codereview.chromium.org/6286038/diff/105041/webkit/fileapi/file_system...
webkit/fileapi/file_system_file_util_proxy.cc:90:
base::FileUtilProxy::Close(message_loop_proxy_, file_handle_, NULL);
On 2011/02/22 10:13:53, Dai Mikurube wrote:
> On 2011/02/22 08:18:04, Dai Mikurube wrote:
> > It should be :
> > fileapi::FileSystemFileUtilProxy::Close(context(), message_loop_proxy_,
> > file_handle_, NULL);
> 
> It was opposite to the correct way. Revert it.
> FileSystemOperation::~FileSystemOperation is to be fixed.

I think the Close here should be fixed too.

[Dai Mikurube (google.com), 2011-02-22 11:25:16]

http://codereview.chromium.org/6286038/diff/105041/webkit/fileapi/file_system...
File webkit/fileapi/file_system_file_util_proxy.cc (right):

http://codereview.chromium.org/6286038/diff/105041/webkit/fileapi/file_system...
webkit/fileapi/file_system_file_util_proxy.cc:90:
base::FileUtilProxy::Close(message_loop_proxy_, file_handle_, NULL);
On 2011/02/22 11:13:39, kinuko wrote:
> On 2011/02/22 10:13:53, Dai Mikurube wrote:
> > On 2011/02/22 08:18:04, Dai Mikurube wrote:
> > > It should be :
> > > fileapi::FileSystemFileUtilProxy::Close(context(), message_loop_proxy_,
> > > file_handle_, NULL);
> > 
> > It was opposite to the correct way. Revert it.
> > FileSystemOperation::~FileSystemOperation is to be fixed.
> 
> I think the Close here should be fixed too.

We should not use fileapi::FileSystemFileUtilProxy:: functions here since we
cannot access some members which are already destructed.

In FileSystemOperation, my change suggests base::FileUtilProxy::Close instead of
FileSystemFielUtilProxy::Close.

But I know using base::FileUtilProxy is not good. We need to think out another
way.

[kinuko, 2011-02-22 12:20:58]

http://codereview.chromium.org/6286038/diff/105041/webkit/fileapi/file_system...
File webkit/fileapi/file_system_file_util_proxy.cc (right):

http://codereview.chromium.org/6286038/diff/105041/webkit/fileapi/file_system...
webkit/fileapi/file_system_file_util_proxy.cc:17: explicit
MessageLoopRelay(fileapi::FileSystemOperationContext* context)
Can we add a comment about who owns the context?
(Looks like FSO is going to own it?)

http://codereview.chromium.org/6286038/diff/105041/webkit/fileapi/file_system...
webkit/fileapi/file_system_file_util_proxy.cc:90:
base::FileUtilProxy::Close(message_loop_proxy_, file_handle_, NULL);
On 2011/02/22 11:25:16, Dai Mikurube wrote:
> On 2011/02/22 11:13:39, kinuko wrote:
> > On 2011/02/22 10:13:53, Dai Mikurube wrote:
> > > On 2011/02/22 08:18:04, Dai Mikurube wrote:
> > > > It should be :
> > > > fileapi::FileSystemFileUtilProxy::Close(context(), message_loop_proxy_,
> > > > file_handle_, NULL);
> > > 
> > > It was opposite to the correct way. Revert it.
> > > FileSystemOperation::~FileSystemOperation is to be fixed.
> > 
> > I think the Close here should be fixed too.
> 
> We should not use fileapi::FileSystemFileUtilProxy:: functions here since we
> cannot access some members which are already destructed.
> 
> In FileSystemOperation, my change suggests base::FileUtilProxy::Close instead
of
> FileSystemFielUtilProxy::Close.
> 
> But I know using base::FileUtilProxy is not good. We need to think out another
> way.

Ah I see (here and your other comment).  But calling FileUtilProxy here alone
looks weird and might cause some problems if anything is added to
FileSystemFileUtilProxy::Close.

As for file_system_file_util(), at least can we work around by holding the
FileSystemFileUtil* pointer directly in the MessageLoopRelay instance?  (I mean,
rather than obtaining the pointer via context() each time it's called.)  The
FileSystemFileUtil instance is singleton and there should be no lifetime problem
around the FSFU pointer right?

Another question: do we need to make the context keep the pointer?
(Can't we simply do something like file_system_file_util_(...::GetInstance()) in
the constructor?)
The operation context is created by IO thread but used by FILE thread, and I
think we should keep the context as simple as possible.

Also I strongly think we should add more explicit comments about the context
owner and lifetime -- it's not very intuitive right now.