Fixed file/directory url resolution for external mount point provider.

Fixed file/directory url resolution for external mount point provider.

Per Eric's request, refactored FileSystemDirURLRequestJob and FileSystemURLRequestJob classes to resolve local file system through a new operation.

BUG=chromium-os:14225
TEST=added new test cases to FileSystemPathManagerTest.*, added FileSystemOperationTest.TestGetLocalFilePathSuccess
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=82266

[ericu, 2011-04-18 20:35:56]

Just a quick once-over, as I'm trying to figure out what you're doing.

http://codereview.chromium.org/6864040/diff/6001/webkit/chromeos/fileapi/cros...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

http://codereview.chromium.org/6864040/diff/6001/webkit/chromeos/fileapi/cros...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:60: // On the other hand,
the root path of a file/dir from the real file system
I don't understand this comment.
Also, the start "on the other hand" implies there's some background I'm missing.

http://codereview.chromium.org/6864040/diff/6001/webkit/chromeos/fileapi/cros...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:83: bool is_file_system,
is_file_system seems poorly named.  If this flag is intended to mean "this path
isn't actually in the filesystem at all", then it seems like you shouldn't
really be calling this method at all.  Is the whole point of this change just to
add a hack so that you can test some other class?  Is there another way to do
that?

http://codereview.chromium.org/6864040/diff/6001/webkit/chromeos/fileapi/cros...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:90: // File system root
path is a non-existent path from the file system...
Why are we looking up a nonexistent path?

http://codereview.chromium.org/6864040/diff/6001/webkit/chromeos/fileapi/cros...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:93: root_path =
FilePath(fileapi::kExternalDir);
This is kind of weird.  '/external/' is in the namespace of FileSystem URLs, and
now you're using it in a FilePath that's intended to be in the namespace of real
paths.  Why do this?

http://codereview.chromium.org/6864040/diff/6001/webkit/fileapi/file_system_m...
File webkit/fileapi/file_system_mount_point_provider.h (right):

http://codereview.chromium.org/6864040/diff/6001/webkit/fileapi/file_system_m...
webkit/fileapi/file_system_mount_point_provider.h:74: // Removed a mount point.
s/Removed/Removes/

http://codereview.chromium.org/6864040/diff/6001/webkit/fileapi/file_system_p...
File webkit/fileapi/file_system_path_manager_unittest.cc (right):

http://codereview.chromium.org/6864040/diff/6001/webkit/fileapi/file_system_p...
webkit/fileapi/file_system_path_manager_unittest.cc:63: {
fileapi::kFileSystemTypeExternal, "chrome-extension://foo/",
Fix indent.

[zel, 2011-04-18 20:51:12]

http://codereview.chromium.org/6864040/diff/6001/webkit/chromeos/fileapi/cros...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

http://codereview.chromium.org/6864040/diff/6001/webkit/chromeos/fileapi/cros...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:60: // On the other hand,
the root path of a file/dir from the real file system
On 2011/04/18 20:35:56, ericu wrote:
> I don't understand this comment.
> Also, the start "on the other hand" implies there's some background I'm
missing.

sorry, this got removed from the context of other comments. Done

http://codereview.chromium.org/6864040/diff/6001/webkit/chromeos/fileapi/cros...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:83: bool is_file_system,
On 2011/04/18 20:35:56, ericu wrote:
> is_file_system seems poorly named.  If this flag is intended to mean "this
path
> isn't actually in the filesystem at all", then it seems like you shouldn't
> really be calling this method at all.  Is the whole point of this change just
to
> add a hack so that you can test some other class?  Is there another way to do
> that?

I agree about the name, any suggestion is welcomed.

It's actually not a test hack, it's little bit more complicated than that. The
external mount point provider exposes first segment of virtual path that does
not have a corresponding directory entry in the real file system - /external/.
When we want to get root path to construct DOMFileSystem, then /external/ is
acceptable, but when I need to map a file/dir URL to the real file path then I
need to strip /external/ out of the virtual path. This flag differentiates
between these two cases.

http://codereview.chromium.org/6864040/diff/6001/webkit/chromeos/fileapi/cros...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:90: // File system root
path is a non-existent path from the file system...
On 2011/04/18 20:35:56, ericu wrote:
> Why are we looking up a nonexistent path?

see above

http://codereview.chromium.org/6864040/diff/6001/webkit/chromeos/fileapi/cros...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:93: root_path =
FilePath(fileapi::kExternalDir);
On 2011/04/18 20:35:56, ericu wrote:
> This is kind of weird.  '/external/' is in the namespace of FileSystem URLs,
and
> now you're using it in a FilePath that's intended to be in the namespace of
real
> paths.  Why do this?

see my first comment on this one as well

http://codereview.chromium.org/6864040/diff/6001/webkit/fileapi/file_system_m...
File webkit/fileapi/file_system_mount_point_provider.h (right):

http://codereview.chromium.org/6864040/diff/6001/webkit/fileapi/file_system_m...
webkit/fileapi/file_system_mount_point_provider.h:74: // Removed a mount point.
On 2011/04/18 20:35:56, ericu wrote:
> s/Removed/Removes/

Done.

[zel, 2011-04-19 04:08:10]

PTAL

Eric, I haven't fully finished with all unit tests here, but you can see if this
is matching your implementation  expectations.

[ericu, 2011-04-19 20:30:49]

Looks good overall, but there's a bunch of small stuff left.

http://codereview.chromium.org/6864040/diff/10095/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_file_browser_private_api.cc (right):

http://codereview.chromium.org/6864040/diff/10095/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:403: // Callback
to report information for a file.
Comment is unnecessary.

http://codereview.chromium.org/6864040/diff/10095/webkit/chromeos/fileapi/cro...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

http://codereview.chromium.org/6864040/diff/10095/webkit/chromeos/fileapi/cro...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:78: void
CrosMountPointProvider::GetFileSystemRootPath(
Add a comment explaining that this is only called by openFileSystem, and that it
doesn't actually get a root path, but a filesystem root URL instead.

Actually, this is a bit of a mess, and it's my fault.  GetFileSystemRootPath
doesn't do the same thing as GetFileSystemRootPathOnFileThread, and that's just
wrong.  How about we change the name of this method to
ValidateFileSystemRootAndGetURL everywhere?  That'll clear up the confusing
names, and we can leave the stuff that actually gets the root path named as it
is.  Does that sound reasonable?

We could also change the stuff that gets the root path to
ValidateFileSystemRootAndGetPathOnFileThread, as the sandbox provider actually
does more than the extension provider, but I'm not sure it's necessary.

http://codereview.chromium.org/6864040/diff/10095/webkit/chromeos/fileapi/cro...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:89:
callback_ptr->Run(!root_path.empty(), root_path, name);
This code looks odd.  Why check if it's empty?
Also, you're not returning a full filesystem URL, which you should be.

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_callback_dispatcher.h (right):

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
webkit/fileapi/file_system_callback_dispatcher.h:29: virtual void
DidGetLocalPath(const FilePath& local_path) = 0;
Move this down below DidOpenFile, as it's not one of those that mirrors
WebFileSystemCallbacks [as mentioned in the above comment].  Given how specific
it is in purpose, I think it makes sense to give it a default implementation
here with a NOTREACHED, just like DidOpenFile.

Also, for the comment, please indicate that it's only to be used when absolutely
unavoidable, and won't work in some cases.  And "report information" is a bit
generic; how about "look up the real local platform path of the file, where
possible".

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_context.cc (right):

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
webkit/fileapi/file_system_context.cc:52: file_message_loop, profile_path,
is_incognito)) {
How about passing in an path_manager that may be null, and creating one if it's
not, rather than having another constructor?  We do that in some other
filesystem classes.

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
webkit/fileapi/file_system_context.cc:88: 
Extra line.

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_context.h (right):

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
webkit/fileapi/file_system_context.h:41: // Test seam only.
What does the comment mean?

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_dir_url_request_job.cc (right):

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
webkit/fileapi/file_system_dir_url_request_job.cc:70: relative_file_path_ =
local_path;
You're calling it relative_file_path here, but absolute in
FileSystemURLRequestJob.  I think absolute is correct.

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_file_util.cc (right):

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
webkit/fileapi/file_system_file_util.cc:66: const FilePath& virtual_file,
s/virtual_file/virtual_path/

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_file_util.h (right):

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
webkit/fileapi/file_system_file_util.h:68: // phyisical file location on the
host OS.
Add comment that this may not always make sense for all FSFU subclasses.

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
webkit/fileapi/file_system_file_util.h:71: const FilePath& virtual_file,
s/virtual_file/virtual_path/

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_file_util_proxy.cc (right):

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
webkit/fileapi/file_system_file_util_proxy.cc:471: // Retrieves the information
about a file local path.
Remove the comment, replace with "// static".
Would you do the same with GetFileInfo while you're in there?

Thanks.

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_file_util_proxy.h (right):

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
webkit/fileapi/file_system_file_util_proxy.h:45: >::Type GetGetLocalPath;
s/GetGetLocalPath/GetLocalPathCallback/

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
webkit/fileapi/file_system_file_util_proxy.h:80: // Retrieves the information
about a file. It is invalid to pass NULL for the
Same here--"Retrieves the information..." is way too vague.

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

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
webkit/fileapi/file_system_operation.cc:238: pending_operation_ =
kOperationGetMetadata;
kOperationGetLocalPath.

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
webkit/fileapi/file_system_operation.cc:657:
dispatcher_->DidFail(base::PLATFORM_FILE_ERROR_INVALID_URL);
Why this change?  In general, we view an invalid URL as an attempt to sniff for
resources to which you don't have access.

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_operation.h (right):

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
webkit/fileapi/file_system_operation.h:72: void GetLocalPath(const GURL& path);
Move this down with OpenFile.

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
webkit/fileapi/file_system_operation.h:127: void
DidGetLocalPath(base::PlatformFileError rv,
Move this down with DidOpenFile.  I'd like to keep the odd operations together.

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_operation_unittest.cc (right):

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
webkit/fileapi/file_system_operation_unittest.cc:741: 
Missing test for new operation.

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_path_manager_unittest.cc (right):

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
webkit/fileapi/file_system_path_manager_unittest.cc:82: "chrome-extension__0" PS
"External", "testing/test_file.txt" },
We don't need to stick on a file name here and pull it off below.  The reason
that the sandbox looks at DirName is that there's an unguessable directory name
created below "temporary" or "persistent" for security.

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
webkit/fileapi/file_system_path_manager_unittest.cc:247: const FilePath&
virtual_path,
Obsolete params.

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_url_request_job_base.h (right):

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
webkit/fileapi/file_system_url_request_job_base.h:18: // A request job that
handles reading filesystem: URLs for directories.
The comment refers to directories.

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

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_file_util.cc:57: false);    // create
Why is this not using GetLocalPath?

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

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_file_util.h:63: virtual FilePath GetLocalPath(
Why is this now virtual and public?  I think external calls are all going into
GetLocalFilePath.

[zel, 2011-04-19 23:14:00]

I am still working on file_system_operation_unittest.cc, other comments should
be addressed.

http://codereview.chromium.org/6864040/diff/10095/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_file_browser_private_api.cc (right):

http://codereview.chromium.org/6864040/diff/10095/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:403: // Callback
to report information for a file.
On 2011/04/19 20:30:49, ericu wrote:
> Comment is unnecessary.

Done.

http://codereview.chromium.org/6864040/diff/10095/webkit/chromeos/fileapi/cro...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

http://codereview.chromium.org/6864040/diff/10095/webkit/chromeos/fileapi/cro...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:78: void
CrosMountPointProvider::GetFileSystemRootPath(
On 2011/04/19 20:30:49, ericu wrote:
> Add a comment explaining that this is only called by openFileSystem, and that
it
> doesn't actually get a root path, but a filesystem root URL instead.
> 
> Actually, this is a bit of a mess, and it's my fault.  GetFileSystemRootPath
> doesn't do the same thing as GetFileSystemRootPathOnFileThread, and that's
just
> wrong.  How about we change the name of this method to
> ValidateFileSystemRootAndGetURL everywhere?  That'll clear up the confusing
> names, and we can leave the stuff that actually gets the root path named as it
> is.  Does that sound reasonable?
> 
> We could also change the stuff that gets the root path to
> ValidateFileSystemRootAndGetPathOnFileThread, as the sandbox provider actually
> does more than the extension provider, but I'm not sure it's necessary.

Done.

http://codereview.chromium.org/6864040/diff/10095/webkit/chromeos/fileapi/cro...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:89:
callback_ptr->Run(!root_path.empty(), root_path, name);
On 2011/04/19 20:30:49, ericu wrote:
> This code looks odd.  Why check if it's empty?
> Also, you're not returning a full filesystem URL, which you should be.

What do you mean URL? This one is accepting FilePath here:

typedef Callback3<bool /* success */,
                    const FilePath& /* root_path */,
                    const std::string& /* name */>::Type GetRootPathCallback;

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_callback_dispatcher.h (right):

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
webkit/fileapi/file_system_callback_dispatcher.h:29: virtual void
DidGetLocalPath(const FilePath& local_path) = 0;
On 2011/04/19 20:30:49, ericu wrote:
> Move this down below DidOpenFile, as it's not one of those that mirrors
> WebFileSystemCallbacks [as mentioned in the above comment].  Given how
specific
> it is in purpose, I think it makes sense to give it a default implementation
> here with a NOTREACHED, just like DidOpenFile.
> 
> Also, for the comment, please indicate that it's only to be used when
absolutely
> unavoidable, and won't work in some cases.  And "report information" is a bit
> generic; how about "look up the real local platform path of the file, where
> possible".

Done.

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_context.cc (right):

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
webkit/fileapi/file_system_context.cc:52: file_message_loop, profile_path,
is_incognito)) {
On 2011/04/19 20:30:49, ericu wrote:
> How about passing in an path_manager that may be null, and creating one if
it's
> not, rather than having another constructor?  We do that in some other
> filesystem classes.

Done.

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
webkit/fileapi/file_system_context.cc:88: 
On 2011/04/19 20:30:49, ericu wrote:
> Extra line.

Done.

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_context.h (right):

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
webkit/fileapi/file_system_context.h:41: // Test seam only.
Removed now. It meant that this constructor is meant to be used from unit tests
only.

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_dir_url_request_job.cc (right):

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
webkit/fileapi/file_system_dir_url_request_job.cc:70: relative_file_path_ =
local_path;
On 2011/04/19 20:30:49, ericu wrote:
> You're calling it relative_file_path here, but absolute in
> FileSystemURLRequestJob.  I think absolute is correct.

Yes, good catch. Done.

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_file_util.cc (right):

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
webkit/fileapi/file_system_file_util.cc:66: const FilePath& virtual_file,
On 2011/04/19 20:30:49, ericu wrote:
> s/virtual_file/virtual_path/

Done.

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_file_util.h (right):

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
webkit/fileapi/file_system_file_util.h:68: // phyisical file location on the
host OS.
On 2011/04/19 20:30:49, ericu wrote:
> Add comment that this may not always make sense for all FSFU subclasses.

Done.

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
webkit/fileapi/file_system_file_util.h:71: const FilePath& virtual_file,
On 2011/04/19 20:30:49, ericu wrote:
> s/virtual_file/virtual_path/

Done.

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_file_util_proxy.cc (right):

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
webkit/fileapi/file_system_file_util_proxy.cc:471: // Retrieves the information
about a file local path.
On 2011/04/19 20:30:49, ericu wrote:
> Remove the comment, replace with "// static".
> Would you do the same with GetFileInfo while you're in there?
> 
> Thanks.

Done.

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_file_util_proxy.h (right):

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
webkit/fileapi/file_system_file_util_proxy.h:45: >::Type GetGetLocalPath;
On 2011/04/19 20:30:49, ericu wrote:
> s/GetGetLocalPath/GetLocalPathCallback/

Done.

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
webkit/fileapi/file_system_file_util_proxy.h:80: // Retrieves the information
about a file. It is invalid to pass NULL for the
On 2011/04/19 20:30:49, ericu wrote:
> Same here--"Retrieves the information..." is way too vague.

Done.

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

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
webkit/fileapi/file_system_operation.cc:238: pending_operation_ =
kOperationGetMetadata;
On 2011/04/19 20:30:49, ericu wrote:
> kOperationGetLocalPath.

Done.

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
webkit/fileapi/file_system_operation.cc:657:
dispatcher_->DidFail(base::PLATFORM_FILE_ERROR_INVALID_URL);
On 2011/04/19 20:30:49, ericu wrote:
> Why this change?  In general, we view an invalid URL as an attempt to sniff
for
> resources to which you don't have access.

FileSystemURLRequestJob unit tests expected INVALID_URL error in such cases. Now
when things are going through FSO, we would mask the real reason why request
failed.

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_operation.h (right):

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
webkit/fileapi/file_system_operation.h:72: void GetLocalPath(const GURL& path);
On 2011/04/19 20:30:49, ericu wrote:
> Move this down with OpenFile.

Done.

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
webkit/fileapi/file_system_operation.h:127: void
DidGetLocalPath(base::PlatformFileError rv,
On 2011/04/19 20:30:49, ericu wrote:
> Move this down with DidOpenFile.  I'd like to keep the odd operations
together.

Done.

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_path_manager_unittest.cc (right):

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
webkit/fileapi/file_system_path_manager_unittest.cc:82: "chrome-extension__0" PS
"External", "testing/test_file.txt" },
On 2011/04/19 20:30:49, ericu wrote:
> We don't need to stick on a file name here and pull it off below.  The reason
> that the sandbox looks at DirName is that there's an unguessable directory
name
> created below "temporary" or "persistent" for security.

Done.

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
webkit/fileapi/file_system_path_manager_unittest.cc:247: const FilePath&
virtual_path,
On 2011/04/19 20:30:49, ericu wrote:
> Obsolete params.

Done.

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_url_request_job_base.h (right):

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
webkit/fileapi/file_system_url_request_job_base.h:18: // A request job that
handles reading filesystem: URLs for directories.
On 2011/04/19 20:30:49, ericu wrote:
> The comment refers to directories.

Done.

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

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_file_util.cc:57: false);    // create
On 2011/04/19 20:30:49, ericu wrote:
> Why is this not using GetLocalPath?

Good point, didn't noticed that one. Done.

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

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_file_util.h:63: virtual FilePath GetLocalPath(
On 2011/04/19 20:30:49, ericu wrote:
> Why is this now virtual and public?  I think external calls are all going into
> GetLocalFilePath.

Done.

[ericu, 2011-04-19 23:36:19]

Very close.  Looking forward to the new unit test, and then we're good to go.

http://codereview.chromium.org/6864040/diff/10095/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_file_browser_private_api.cc (right):

http://codereview.chromium.org/6864040/diff/10095/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:403: // Callback
to report information for a file.
On 2011/04/19 23:14:00, zel wrote:
> On 2011/04/19 20:30:49, ericu wrote:
> > Comment is unnecessary.
> 
> Done.

Still there.

http://codereview.chromium.org/6864040/diff/10095/webkit/chromeos/fileapi/cro...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

http://codereview.chromium.org/6864040/diff/10095/webkit/chromeos/fileapi/cro...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:89:
callback_ptr->Run(!root_path.empty(), root_path, name);
On 2011/04/19 23:14:00, zel wrote:
> On 2011/04/19 20:30:49, ericu wrote:
> > This code looks odd.  Why check if it's empty?
> > Also, you're not returning a full filesystem URL, which you should be.
> 
> What do you mean URL? This one is accepting FilePath here:
> 
> typedef Callback3<bool /* success */,
>                     const FilePath& /* root_path */,
>                     const std::string& /* name */>::Type GetRootPathCallback;

You're right--I got myself confused.  Nevermind.

But perhaps we should return an empty path, instead of /External, since it
should never be used anyway, and /External looks like it could mean something?

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

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
webkit/fileapi/file_system_operation.cc:657:
dispatcher_->DidFail(base::PLATFORM_FILE_ERROR_INVALID_URL);
On 2011/04/19 23:14:00, zel wrote:
> On 2011/04/19 20:30:49, ericu wrote:
> > Why this change?  In general, we view an invalid URL as an attempt to sniff
> for
> > resources to which you don't have access.
> 
> FileSystemURLRequestJob unit tests expected INVALID_URL error in such cases.
Now
> when things are going through FSO, we would mask the real reason why request
> failed.

OK.

http://codereview.chromium.org/6864040/diff/6167/webkit/fileapi/file_system_c...
File webkit/fileapi/file_system_callback_dispatcher.h (right):

http://codereview.chromium.org/6864040/diff/6167/webkit/fileapi/file_system_c...
webkit/fileapi/file_system_callback_dispatcher.h:62: // Callback for the real
local platform path lookup, where possible.
Add: "This isn't in WebFileSystemCallbacks."

http://codereview.chromium.org/6864040/diff/6167/webkit/fileapi/file_system_f...
File webkit/fileapi/file_system_file_util_proxy.cc (right):

http://codereview.chromium.org/6864040/diff/6167/webkit/fileapi/file_system_f...
webkit/fileapi/file_system_file_util_proxy.cc:481: // static.
Unnecessary period.

http://codereview.chromium.org/6864040/diff/6167/webkit/fileapi/file_system_o...
File webkit/fileapi/file_system_operation.h (right):

http://codereview.chromium.org/6864040/diff/6167/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_operation.h:200: kOperationCancel,
Cancel should be last.

[zel, 2011-04-20 02:02:07]

http://codereview.chromium.org/6864040/diff/10095/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_file_browser_private_api.cc (right):

http://codereview.chromium.org/6864040/diff/10095/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:403: // Callback
to report information for a file.
On 2011/04/19 23:36:19, ericu wrote:
> On 2011/04/19 23:14:00, zel wrote:
> > On 2011/04/19 20:30:49, ericu wrote:
> > > Comment is unnecessary.
> > 
> > Done.
> 
> Still there.

Done.

http://codereview.chromium.org/6864040/diff/10095/webkit/chromeos/fileapi/cro...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

http://codereview.chromium.org/6864040/diff/10095/webkit/chromeos/fileapi/cro...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:89:
callback_ptr->Run(!root_path.empty(), root_path, name);
On 2011/04/19 23:36:19, ericu wrote:
> On 2011/04/19 23:14:00, zel wrote:
> > On 2011/04/19 20:30:49, ericu wrote:
> > > This code looks odd.  Why check if it's empty?
> > > Also, you're not returning a full filesystem URL, which you should be.
> > 
> > What do you mean URL? This one is accepting FilePath here:
> > 
> > typedef Callback3<bool /* success */,
> >                     const FilePath& /* root_path */,
> >                     const std::string& /* name */>::Type
GetRootPathCallback;
> 
> You're right--I got myself confused.  Nevermind.
> 
> But perhaps we should return an empty path, instead of /External, since it
> should never be used anyway, and /External looks like it could mean something?

I think there are tests on my side that expect /external/ here. I will remove
this is really nothing depends on it.

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_operation_unittest.cc (right):

http://codereview.chromium.org/6864040/diff/10095/webkit/fileapi/file_system_...
webkit/fileapi/file_system_operation_unittest.cc:741: 
On 2011/04/19 20:30:49, ericu wrote:
> Missing test for new operation.

Tests added. Done.

http://codereview.chromium.org/6864040/diff/6167/webkit/fileapi/file_system_c...
File webkit/fileapi/file_system_callback_dispatcher.h (right):

http://codereview.chromium.org/6864040/diff/6167/webkit/fileapi/file_system_c...
webkit/fileapi/file_system_callback_dispatcher.h:62: // Callback for the real
local platform path lookup, where possible.
On 2011/04/19 23:36:19, ericu wrote:
> Add: "This isn't in WebFileSystemCallbacks."

Done.

http://codereview.chromium.org/6864040/diff/6167/webkit/fileapi/file_system_f...
File webkit/fileapi/file_system_file_util_proxy.cc (right):

http://codereview.chromium.org/6864040/diff/6167/webkit/fileapi/file_system_f...
webkit/fileapi/file_system_file_util_proxy.cc:481: // static.
On 2011/04/19 23:36:19, ericu wrote:
> Unnecessary period.

Done.

http://codereview.chromium.org/6864040/diff/6167/webkit/fileapi/file_system_o...
File webkit/fileapi/file_system_operation.h (right):

http://codereview.chromium.org/6864040/diff/6167/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_operation.h:200: kOperationCancel,
On 2011/04/19 23:36:19, ericu wrote:
> Cancel should be last.

Done.

[ericu, 2011-04-20 02:23:46]

http://codereview.chromium.org/6864040/diff/9373/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_file_browser_private_api.cc (right):

http://codereview.chromium.org/6864040/diff/9373/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_file_browser_private_api.cc:192: 
This comment should have stayed.

http://codereview.chromium.org/6864040/diff/9373/webkit/fileapi/file_system_o...
File webkit/fileapi/file_system_operation_unittest.cc (right):

http://codereview.chromium.org/6864040/diff/9373/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_operation_unittest.cc:77: return
GURL(std::string("chrome://test") + path.value());
This looks unlikely to work on Windows, where path will start with "c:\".  Can
you just use URLForPath?

http://codereview.chromium.org/6864040/diff/9373/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_operation_unittest.cc:81: return
GURL(std::string("filesystem:") + TestLocalPath(path).spec());
Likewise here, can you use one of the above functions?

[zel, 2011-04-20 02:49:57]

http://codereview.chromium.org/6864040/diff/9373/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_file_browser_private_api.cc (right):

http://codereview.chromium.org/6864040/diff/9373/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_file_browser_private_api.cc:192: 
On 2011/04/20 02:23:46, ericu wrote:
> This comment should have stayed.

Done.

http://codereview.chromium.org/6864040/diff/9373/webkit/fileapi/file_system_o...
File webkit/fileapi/file_system_operation_unittest.cc (right):

http://codereview.chromium.org/6864040/diff/9373/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_operation_unittest.cc:77: return
GURL(std::string("chrome://test") + path.value());
On 2011/04/20 02:23:46, ericu wrote:
> This looks unlikely to work on Windows, where path will start with "c:\".  Can
> you just use URLForPath?

Done.

http://codereview.chromium.org/6864040/diff/9373/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_operation_unittest.cc:81: return
GURL(std::string("filesystem:") + TestLocalPath(path).spec());
On 2011/04/20 02:23:46, ericu wrote:
> Likewise here, can you use one of the above functions?

Yes, I've been already bitten by angry bbots on this one. Done.

[ericu, 2011-04-20 02:53:21]

LGTM, although if you find that that /extension path can go away, I'm happier
with it gone.

[kinuko, 2011-04-20 14:44:18]

(Drive-by comment...)

http://codereview.chromium.org/6864040/diff/10170/webkit/chromeos/fileapi/cro...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

http://codereview.chromium.org/6864040/diff/10170/webkit/chromeos/fileapi/cro...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:78: void
CrosMountPointProvider::ValidateFileSystemRootAndGetURL(
I'm afraid this new method name may confuse the readers of
SandboxMountPointProvider, where it actually returns a path but not URL.

Actually the url returned by this method is right now not used at the callsite. 
If we do not have a plan to use the returned URL could we simply name this
ValidateFileSystemRoot() and make it only return boolean value?  It shouldn't
contradict to the remaining TODO in the FileSystemOperation (i.e. we shouldn't
call this method when !create cases).
The FileThread version could probably be named
ValidateAndGetFilesystemRootPathOnFileThread then.

Wdyt?  I guess Eric may have some other future plan, but if it makes sense we
can make the followup patch.

[ericu, 2011-04-22 15:01:47]

On Wed, Apr 20, 2011 at 7:44 AM,  <kinuko@chromium.org> wrote:
> (Drive-by comment...)
>
>
>
http://codereview.chromium.org/6864040/diff/10170/webkit/chromeos/fileapi/cro...
> File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):
>
>
http://codereview.chromium.org/6864040/diff/10170/webkit/chromeos/fileapi/cro...
> webkit/chromeos/fileapi/cros_mount_point_provider.cc:78: void
> CrosMountPointProvider::ValidateFileSystemRootAndGetURL(
> I'm afraid this new method name may confuse the readers of
> SandboxMountPointProvider, where it actually returns a path but not URL.
>
> Actually the url returned by this method is right now not used at the
> callsite.  If we do not have a plan to use the returned URL could we
> simply name this ValidateFileSystemRoot() and make it only return
> boolean value?  It shouldn't contradict to the remaining TODO in the
> FileSystemOperation (i.e. we shouldn't call this method when !create
> cases).
> The FileThread version could probably be named
> ValidateAndGetFilesystemRootPathOnFileThread then.
>
> Wdyt?  I guess Eric may have some other future plan, but if it makes
> sense we can make the followup patch.
>
> http://codereview.chromium.org/6864040/

Kinuko--

	So sorry I didn't respond to this yesterday; I got swamped and lost
track.  The reason I had Zelidrag change the name of this method is
that, while it locally returns a path, overall its use in
openFileSystem is to get the URL.  Looking back, maybe that wasn't the
best decision, and I'll look at fixing it myself as I continue to
clean up the code.

	Eric

[kinuko, 2011-04-22 16:43:34]

On Sat, Apr 23, 2011 at 12:01 AM, Eric Uhrhane <ericu@chromium.org> wrote:

> On Wed, Apr 20, 2011 at 7:44 AM,  <kinuko@chromium.org> wrote:
> > (Drive-by comment...)
> >
> >
> >
>
http://codereview.chromium.org/6864040/diff/10170/webkit/chromeos/fileapi/cro...
> > File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):
> >
> >
>
http://codereview.chromium.org/6864040/diff/10170/webkit/chromeos/fileapi/cro...
> > webkit/chromeos/fileapi/cros_mount_point_provider.cc:78: void
> > CrosMountPointProvider::ValidateFileSystemRootAndGetURL(
> > I'm afraid this new method name may confuse the readers of
> > SandboxMountPointProvider, where it actually returns a path but not URL.
> >
> > Actually the url returned by this method is right now not used at the
> > callsite.  If we do not have a plan to use the returned URL could we
> > simply name this ValidateFileSystemRoot() and make it only return
> > boolean value?  It shouldn't contradict to the remaining TODO in the
> > FileSystemOperation (i.e. we shouldn't call this method when !create
> > cases).
> > The FileThread version could probably be named
> > ValidateAndGetFilesystemRootPathOnFileThread then.
> >
> > Wdyt?  I guess Eric may have some other future plan, but if it makes
> > sense we can make the followup patch.
> >
> > http://codereview.chromium.org/6864040/
>
> Kinuko--
>
>        So sorry I didn't respond to this yesterday; I got swamped and lost
> track.

 The reason I had Zelidrag change the name of this method is
> that, while it locally returns a path, overall its use in
> openFileSystem is to get the URL.  Looking back, maybe that wasn't the
> best decision, and I'll look at fixing it myself as I continue to
> clean up the code.


Sorry for nudging, this change just caught my eye while I was trying to make
other changes in the filesystem.

I had briefly read the review comment and somewhat know what motivated you
to make this change.  Just wanted to let you know the new method name may
not reflect what it does either.  And unfortunately now what the cros
implementation does is a bit diverted from other code-- but as you say
you'll want to take care of those remaining cleanups.

Thanks,


>        Eric
>