Add FileUtileHelper

Add FileUtileHelper and split out FileSystemFileUtil::Delete

BUG=114732
TEST='existing tests'


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

[tzik, 2012-03-02 02:37:01]

Moving FileSystemFileUtil::Delete out (not completed).

Could you take a look?

[kinuko, 2012-03-02 05:25:16]

Can we get rid of FileSystemFileUtil::Delete?  Maybe we could have a convenient
static entry method in which it creates an instance (if it makes the code
simpler) to do the rest of job so that we can easily call it from outside? 
Wdyt?
(CrossFileUtilHelper could be revised in the same way if it feels better)

http://codereview.chromium.org/9564047/diff/1/webkit/fileapi/file_util_delete...
File webkit/fileapi/file_util_delete_helper.cc (right):

http://codereview.chromium.org/9564047/diff/1/webkit/fileapi/file_util_delete...
webkit/fileapi/file_util_delete_helper.cc:50: return
base::PLATFORM_FILE_ERROR_FAILED;
It makes me wonder we should just let it go here...?  (Does any test require
this?)

[tzik, 2012-03-02 19:09:09]

Done for FileSystemFileUtil::Delete.

The static method sounds good, but after that, its instance seems not to be
needed.
How about let them be just functions, like this?

http://codereview.chromium.org/9564047/diff/1/webkit/fileapi/file_util_delete...
File webkit/fileapi/file_util_delete_helper.cc (right):

http://codereview.chromium.org/9564047/diff/1/webkit/fileapi/file_util_delete...
webkit/fileapi/file_util_delete_helper.cc:50: return
base::PLATFORM_FILE_ERROR_FAILED;
On 2012/03/02 05:25:16, kinuko wrote:
> It makes me wonder we should just let it go here...?  (Does any test require
> this?)

Seems right. No test require it.

[kinuko, 2012-03-02 19:29:22]

http://codereview.chromium.org/9564047/diff/7022/webkit/fileapi/file_system_f...
File webkit/fileapi/file_system_file_util.h (right):

http://codereview.chromium.org/9564047/diff/7022/webkit/fileapi/file_system_f...
webkit/fileapi/file_system_file_util.h:190: PlatformFileError
DeleteDirectoryRecursive(
Should this be also removed?

http://codereview.chromium.org/9564047/diff/7022/webkit/fileapi/file_util_del...
File webkit/fileapi/file_util_delete_helper.h (right):

http://codereview.chromium.org/9564047/diff/7022/webkit/fileapi/file_util_del...
webkit/fileapi/file_util_delete_helper.h:19: bool recursive);
Simple function looks good.

This changeset itself looks good now but could/should we maybe do the same thing
for CrossFileUtil?

We could add a single new file, file_util_helper.{h,cc}, and there could expose
following methods:

DeletePath
MovePath
CopyPath

or maybe like this?

// A collection of static methods that are usually called by
FileSystemFileUtilProxy and run on the FILE thread.
class FileUtilHelper {
  static PlatformFileError Delete(...);
  static PlatformFileError Move(...);
  static PlatformFileError Copy(...);
};

[tzik, 2012-03-02 21:25:47]

http://codereview.chromium.org/9564047/diff/7022/webkit/fileapi/file_system_f...
File webkit/fileapi/file_system_file_util.h (right):

http://codereview.chromium.org/9564047/diff/7022/webkit/fileapi/file_system_f...
webkit/fileapi/file_system_file_util.h:190: PlatformFileError
DeleteDirectoryRecursive(
On 2012/03/02 19:29:22, kinuko wrote:
> Should this be also removed?

Done.

http://codereview.chromium.org/9564047/diff/7022/webkit/fileapi/file_util_del...
File webkit/fileapi/file_util_delete_helper.h (right):

http://codereview.chromium.org/9564047/diff/7022/webkit/fileapi/file_util_del...
webkit/fileapi/file_util_delete_helper.h:19: bool recursive);
On 2012/03/02 19:29:22, kinuko wrote:
> Simple function looks good.
> 
> This changeset itself looks good now but could/should we maybe do the same
thing
> for CrossFileUtil?
> 
> We could add a single new file, file_util_helper.{h,cc}, and there could
expose
> following methods:
> 
> DeletePath
> MovePath
> CopyPath
> 
> or maybe like this?
> 
> // A collection of static methods that are usually called by
> FileSystemFileUtilProxy and run on the FILE thread.
> class FileUtilHelper {
>   static PlatformFileError Delete(...);
>   static PlatformFileError Move(...);
>   static PlatformFileError Copy(...);
> };

Done with wrapping function and to rename file_util_{delete_,}helper.
Same thing for CrossFileUtil sounds good. Yet, may I do it in another CL?

[kinuko, 2012-03-02 22:46:29]

LGTM except for the naming nits.

http://codereview.chromium.org/9564047/diff/5016/webkit/fileapi/cross_file_ut...
File webkit/fileapi/cross_file_util_helper.cc (right):

http://codereview.chromium.org/9564047/diff/5016/webkit/fileapi/cross_file_ut...
webkit/fileapi/cross_file_util_helper.cc:147:
FileUtilHelper::DeletePath(context_, src_util_,
Now we introduced the new class we can rename DeletePath to Delete?

[tzik, 2012-03-02 22:59:30]

Thanks. I'll put it in CQ after try job.

http://codereview.chromium.org/9564047/diff/5016/webkit/fileapi/cross_file_ut...
File webkit/fileapi/cross_file_util_helper.cc (right):

http://codereview.chromium.org/9564047/diff/5016/webkit/fileapi/cross_file_ut...
webkit/fileapi/cross_file_util_helper.cc:147:
FileUtilHelper::DeletePath(context_, src_util_,
On 2012/03/02 22:46:29, kinuko wrote:
> Now we introduced the new class we can rename DeletePath to Delete?

Done.

[commit-bot: I haz the power, 2012-03-03 00:14:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tzik@chromium.org/9564047/9003

[commit-bot: I haz the power, 2012-03-03 02:09:14]

Change committed as 124824