Cleanup FileSystemOperation for preparing for adding FSO-factory method

Cleanup FileSystemOperation for preparing for adding FSO-factory method

1. Move OpenFileSystem() to FileSystemContext
2. Change Cancel() not to take another FileSystemOperation
 These two changes are made so that all the operations that require FileSystemOperation take target path URL.

3. Did some related code cleanups in FileSystemMountPointProvider:
 - Renamed ValidateFileSystemRootAndGetURL() to ValidateFileSystemRoot() as we no longer need to return RootURL
 - Renamed ValidateFileSystemRootAndGetPathOnFileThread() to GetFileSystemRootPathOnFileThread() for the sake of simplicity
 - Reimplemented SandboxMPP::GetRootPathTask using PostTaskAndReply

Patch from http://codereview.chromium.org/9004019/

BUG=none
TEST=existing tests should pass

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=117197

[tzik, 2011-12-22 06:49:20]

http://codereview.chromium.org/9016020/diff/3001/webkit/fileapi/sandbox_mount...
File webkit/fileapi/sandbox_mount_point_provider.cc (right):

http://codereview.chromium.org/9016020/diff/3001/webkit/fileapi/sandbox_mount...
webkit/fileapi/sandbox_mount_point_provider.cc:371: origin_url, type,
old_base_path(), create),
Could we add sandbox_file_util_ as parameter and omit it in
ValidateFileSystemRootHelper?

[kinuko, 2011-12-22 07:04:33]

http://codereview.chromium.org/9016020/diff/3001/webkit/fileapi/sandbox_mount...
File webkit/fileapi/sandbox_mount_point_provider.cc (right):

http://codereview.chromium.org/9016020/diff/3001/webkit/fileapi/sandbox_mount...
webkit/fileapi/sandbox_mount_point_provider.cc:371: origin_url, type,
old_base_path(), create),
On 2011/12/22 06:49:20, tzik wrote:
> Could we add sandbox_file_util_ as parameter and omit it in
> ValidateFileSystemRootHelper?

Good point. Done.

[satorux1, 2011-12-27 18:56:37]

Looks like nice refactoring, but I'm not familiar enough with the code to
approve the changes. Just some minor comments:

http://codereview.chromium.org/9016020/diff/7001/webkit/fileapi/file_system_c...
File webkit/fileapi/file_system_context.h (right):

http://codereview.chromium.org/9016020/diff/7001/webkit/fileapi/file_system_c...
webkit/fileapi/file_system_context.h:90: // Opens the filesystem for the given
origin and type and dispatches
"and type and dispatches" is a bit confusing. adding a comma may be helpful.

"and type, and dispatches"

http://codereview.chromium.org/9016020/diff/7001/webkit/fileapi/file_system_o...
File webkit/fileapi/file_system_operation_interface.h (right):

http://codereview.chromium.org/9016020/diff/7001/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_operation_interface.h:107: // The ownership of
|cancel_dispatcher| is transfered to and will deleted
will deleted -> will be deleted?

http://codereview.chromium.org/9016020/diff/7001/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_operation_interface.h:112: //  
abort_current_operation();
what's this? Please add some explanation.

Besides, function should look like AbortCurrentOperation(). :)

http://codereview.chromium.org/9016020/diff/7001/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_operation_interface.h:113: //  
dispatcher_->DidFail(base::PLATFORM_FILE_ERROR_ABORT);
dispatcher here is the dispather attached to the operation being canceled,
right? would be nice to make it clearer.

http://codereview.chromium.org/9016020/diff/7001/webkit/fileapi/file_system_u...
File webkit/fileapi/file_system_util.h (right):

http://codereview.chromium.org/9016020/diff/7001/webkit/fileapi/file_system_u...
webkit/fileapi/file_system_util.h:41: // |origin_url| and |type|.
Would be nice to add some example so developers can tell how file system names
look like.

http://codereview.chromium.org/9016020/diff/7001/webkit/fileapi/sandbox_mount...
File webkit/fileapi/sandbox_mount_point_provider.cc (left):

http://codereview.chromium.org/9016020/diff/7001/webkit/fileapi/sandbox_mount...
webkit/fileapi/sandbox_mount_point_provider.cc:351: };
Wow, a lot of code is removed. Cool!

http://codereview.chromium.org/9016020/diff/7001/webkit/fileapi/sandbox_mount...
File webkit/fileapi/sandbox_mount_point_provider.cc (right):

http://codereview.chromium.org/9016020/diff/7001/webkit/fileapi/sandbox_mount...
webkit/fileapi/sandbox_mount_point_provider.cc:264: void
PassPointerByValue(const base::Callback<void(A)>& callback, A* a) {
This looks a bit tricky (or I'm just not good at reading templated code). The
function seems to be only used from one place, so I guess you can make it a
non-template function, by replacing A with base::PlatformFileError.

[kinuko, 2011-12-28 10:59:34]

Thanks, addressed comments.

http://codereview.chromium.org/9016020/diff/7001/webkit/fileapi/file_system_c...
File webkit/fileapi/file_system_context.h (right):

http://codereview.chromium.org/9016020/diff/7001/webkit/fileapi/file_system_c...
webkit/fileapi/file_system_context.h:90: // Opens the filesystem for the given
origin and type and dispatches
On 2011/12/27 18:56:37, satorux1 wrote:
> "and type and dispatches" is a bit confusing. adding a comma may be helpful.
> 
> "and type, and dispatches"

Done.

http://codereview.chromium.org/9016020/diff/7001/webkit/fileapi/file_system_o...
File webkit/fileapi/file_system_operation_interface.h (right):

http://codereview.chromium.org/9016020/diff/7001/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_operation_interface.h:107: // The ownership of
|cancel_dispatcher| is transfered to and will deleted
On 2011/12/27 18:56:37, satorux1 wrote:
> will deleted -> will be deleted?

Done.

http://codereview.chromium.org/9016020/diff/7001/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_operation_interface.h:112: //  
abort_current_operation();
On 2011/12/27 18:56:37, satorux1 wrote:
> what's this? Please add some explanation.
> 
> Besides, function should look like AbortCurrentOperation(). :)

Done.

http://codereview.chromium.org/9016020/diff/7001/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_operation_interface.h:113: //  
dispatcher_->DidFail(base::PLATFORM_FILE_ERROR_ABORT);
On 2011/12/27 18:56:37, satorux1 wrote:
> dispatcher here is the dispather attached to the operation being canceled,
> right? would be nice to make it clearer.

Done.

http://codereview.chromium.org/9016020/diff/7001/webkit/fileapi/file_system_u...
File webkit/fileapi/file_system_util.h (right):

http://codereview.chromium.org/9016020/diff/7001/webkit/fileapi/file_system_u...
webkit/fileapi/file_system_util.h:41: // |origin_url| and |type|.
On 2011/12/27 18:56:37, satorux1 wrote:
> Would be nice to add some example so developers can tell how file system names
> look like.

Done.

http://codereview.chromium.org/9016020/diff/7001/webkit/fileapi/sandbox_mount...
File webkit/fileapi/sandbox_mount_point_provider.cc (right):

http://codereview.chromium.org/9016020/diff/7001/webkit/fileapi/sandbox_mount...
webkit/fileapi/sandbox_mount_point_provider.cc:264: void
PassPointerByValue(const base::Callback<void(A)>& callback, A* a) {
On 2011/12/27 18:56:37, satorux1 wrote:
> This looks a bit tricky (or I'm just not good at reading templated code). The
> function seems to be only used from one place, so I guess you can make it a
> non-template function, by replacing A with base::PlatformFileError.

Done.

[kinuko, 2012-01-05 12:01:19]

(Added the correct eric as a reviewer)

[kinuko, 2012-01-06 12:44:23]

(Re-publishing this one as I haven't explicitly done that yet)

This change is for preparing for this next one:
http://codereview.chromium.org/8999017/

[ericu, 2012-01-09 18:44:44]

LGTM.

Thanks for doing this--much of it's been needed for a really long time!

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

http://codereview.chromium.org/9016020/diff/24029/webkit/fileapi/file_system_...
webkit/fileapi/file_system_context.h:10: #include "base/platform_file.h"
Why include platform_file.h?

http://codereview.chromium.org/9016020/diff/24029/webkit/fileapi/file_system_...
webkit/fileapi/file_system_context.h:91: // If |create| is true this may
actually setup a filesystem instance
s/setup/set up/

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

http://codereview.chromium.org/9016020/diff/24029/webkit/fileapi/file_system_...
webkit/fileapi/file_system_dir_url_request_job.cc:45: // (so that we do not leak
ownerships).
s/ownerships/ownership/ here and elsewhere.

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

http://codereview.chromium.org/9016020/diff/24029/webkit/fileapi/file_system_...
webkit/fileapi/file_system_util.h:43: // but can be referred as .name field of
the returned FileSystem object
s/referred as/read as the/

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

http://codereview.chromium.org/9016020/diff/24029/webkit/fileapi/sandbox_moun...
webkit/fileapi/sandbox_mount_point_provider.cc:32: namespace fileapi {
Please remove all the "fileapi::" that are now unnecessary inside the namespace.

Hmm...also, we don't really need them before
kFileSystemTypeTemporary/Persistent/Unknown throughout, do we?  Only
FileSystemType is ambiguous.

[tzik, 2012-01-10 08:31:13]

lgtm

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

http://codereview.chromium.org/9016020/diff/24029/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:707:
scoped_refptr<const Extension> extension,
can be const&?

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

http://codereview.chromium.org/9016020/diff/24029/webkit/fileapi/file_system_...
webkit/fileapi/file_system_mount_point_provider.h:12: #include
"base/platform_file.h"
sort?

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

http://codereview.chromium.org/9016020/diff/24029/webkit/fileapi/file_system_...
webkit/fileapi/file_system_operation.cc:520:
cancel_dispatcher_.swap(cancel_dispatcher);
can be cancel_dispatcher_ = cancel_dispatcher.Pass();?

[kinuko, 2012-01-10 09:48:53]

Thanks, I updated the code.  Will be submitting if I see any other issues.

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

http://codereview.chromium.org/9016020/diff/24029/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:707:
scoped_refptr<const Extension> extension,
On 2012/01/10 08:31:13, tzik wrote:
> can be const&?

Done.

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

http://codereview.chromium.org/9016020/diff/24029/webkit/fileapi/file_system_...
webkit/fileapi/file_system_context.h:10: #include "base/platform_file.h"
On 2012/01/09 18:44:44, ericu wrote:
> Why include platform_file.h?

Removed.

http://codereview.chromium.org/9016020/diff/24029/webkit/fileapi/file_system_...
webkit/fileapi/file_system_context.h:91: // If |create| is true this may
actually setup a filesystem instance
On 2012/01/09 18:44:44, ericu wrote:
> s/setup/set up/

Done.

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

http://codereview.chromium.org/9016020/diff/24029/webkit/fileapi/file_system_...
webkit/fileapi/file_system_dir_url_request_job.cc:45: // (so that we do not leak
ownerships).
On 2012/01/09 18:44:44, ericu wrote:
> s/ownerships/ownership/ here and elsewhere.

Done.

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

http://codereview.chromium.org/9016020/diff/24029/webkit/fileapi/file_system_...
webkit/fileapi/file_system_mount_point_provider.h:12: #include
"base/platform_file.h"
On 2012/01/10 08:31:13, tzik wrote:
> sort?

Done.

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

http://codereview.chromium.org/9016020/diff/24029/webkit/fileapi/file_system_...
webkit/fileapi/file_system_operation.cc:520:
cancel_dispatcher_.swap(cancel_dispatcher);
On 2012/01/10 08:31:13, tzik wrote:
> can be cancel_dispatcher_ = cancel_dispatcher.Pass();?

Done.

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

http://codereview.chromium.org/9016020/diff/24029/webkit/fileapi/file_system_...
webkit/fileapi/file_system_util.h:43: // but can be referred as .name field of
the returned FileSystem object
On 2012/01/09 18:44:44, ericu wrote:
> s/referred as/read as the/

Done.

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

http://codereview.chromium.org/9016020/diff/24029/webkit/fileapi/sandbox_moun...
webkit/fileapi/sandbox_mount_point_provider.cc:32: namespace fileapi {
On 2012/01/09 18:44:44, ericu wrote:
> Please remove all the "fileapi::" that are now unnecessary inside the
namespace.
> 
> Hmm...also, we don't really need them before
> kFileSystemTypeTemporary/Persistent/Unknown throughout, do we?  Only
> FileSystemType is ambiguous.

Done.

[commit-bot: I haz the power, 2012-01-11 03:45:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/9016020/40005

[commit-bot: I haz the power, 2012-01-11 03:45:28]

Presubmit check for 9016020-40005 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
webkit/tools/test_shell/simple_file_system.cc,webkit/tools/test_shell/simple_file_writer.cc

Presubmit checks took 2.9s to calculate.