Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3212002:
Changes for browser-side implementation for file api.... (Closed)
|
Created:
10 years, 3 months ago by Kavita Kanetkar Modified:
9 years, 7 months ago CC:
chromium-reviews, ben+cc_chromium.org, brettw-cc_chromium.org, michaeln, Eric U. Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionChanges for browser-side implementation for file api.
BUG=32277
TEST=Separate CL for unit test.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=58312
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #
Total comments: 97
Patch Set 4 : '' #Patch Set 5 : '' #
Total comments: 20
Patch Set 6 : '' #
Total comments: 6
Patch Set 7 : '' #
Total comments: 18
Patch Set 8 : '' #Patch Set 9 : '' #Patch Set 10 : '' #Patch Set 11 : '' #
Total comments: 4
Messages
Total messages: 20 (0 generated)
Dumi please see the base/ Kinuko please see base/file_util_proxy for correct return codes, and the rest of browser. Thanks!
Thanks for working on this - writing generic file operations and defining error code is tough. http://codereview.chromium.org/3212002/diff/16001/7002 File base/file_util_proxy.cc (right): http://codereview.chromium.org/3212002/diff/16001/7002#newcode78 base/file_util_proxy.cc:78: bool exclusive, this flag won't be necessary (see below) http://codereview.chromium.org/3212002/diff/16001/7002#newcode102 base/file_util_proxy.cc:102: if (exclusive_ && file_util::PathExists(file_path_)) { No need to check this here - seems like if you call CreatePlatformFile with flags=PLATFORM_FILE_CREATE it always fails if the entry already exists. http://codereview.chromium.org/3212002/diff/16001/7002#newcode108 base/file_util_proxy.cc:108: set_error_code(base::PLATFORM_FILE_MODIFICATION_DISALLOWED_ERROR); How about changing this error check statement to: if (file_handle == base::kInvalidPlatformFileValue) { if ((flags & base::PLATFORM_FILE_CREATE) && file_util::PathExists(file_path)) set_error_code(base::PLATFORM_FILE_ALREADY_EXISTS); else set_error_code(base::PLATFORM_FILE_ERROR); } For generic file errors like this I don't think we should replace it with another error code like MODIFICATION_DISALLOWED_ERROR - CreatePlatformFile can fail here for several different reasons while 'MODIFICATION_DISALLOWED' sounds more specific error type. Maybe we should just continue to use PLATFORM_FILE_ERROR in such cases. http://codereview.chromium.org/3212002/diff/16001/7002#newcode158 base/file_util_proxy.cc:158: set_error_code(base::PLATFORM_FILE_MODIFICATION_DISALLOWED_ERROR); same here; do we need to change this error code? http://codereview.chromium.org/3212002/diff/16001/7002#newcode207 base/file_util_proxy.cc:207: set_error_code(base::PLATFORM_FILE_MODIFICATION_DISALLOWED_ERROR); ditto. http://codereview.chromium.org/3212002/diff/16001/7002#newcode229 base/file_util_proxy.cc:229: set_error_code(base::PLATFORM_FILE_INVALID_STATE_ERROR); How about defining an error code like PLATFORM_INVALID_ENTRY or PLATFORM_NOT_A_FILE? I'm afraid 'invalid state' is too specific to the API - may not make sense as a generic error code. http://codereview.chromium.org/3212002/diff/16001/7002#newcode272 base/file_util_proxy.cc:272: if (!file_util::IsDirectoryEmpty(file_path_)) { if (!recursive && !file_util::IsDirectoryEmpty(file_path_)) { http://codereview.chromium.org/3212002/diff/16001/7002#newcode276 base/file_util_proxy.cc:276: set_error_code(base::PLATFORM_FILE_MODIFICATION_DISALLOWED_ERROR); again PLATFORM_FILE_ERROR would be better here and we can translate the error code into NoModificationAllowedError in our error translation code. http://codereview.chromium.org/3212002/diff/16001/7002#newcode297 base/file_util_proxy.cc:297: if (!DirectoryExists(dest_file_path_)) { how about something like following that would be more generic & enough for our purpose: if (!file_util::PathExists(dest_file_path_) && !file_util::DirectoryExists(dest_file_path_.DirName()) http://codereview.chromium.org/3212002/diff/16001/7002#newcode301 base/file_util_proxy.cc:301: if (src_file_path_.value() == dest_file_path_.value() || I think we can remove this check (the former one) now that we made sure that we have this check in the renderer. (thanks for adding though) http://codereview.chromium.org/3212002/diff/16001/7002#newcode308 base/file_util_proxy.cc:308: set_error_code(base::PLATFORM_FILE_MODIFICATION_DISALLOWED_ERROR); PLATFORM_FILE_ERROR http://codereview.chromium.org/3212002/diff/16001/7002#newcode328 base/file_util_proxy.cc:328: if (!DirectoryExists(dest_file_path_)) { ditto. (if (!file_util::PathExists(dest_file_path_) && !file_util::DirectoryExists(dest_file_path_.DirName())) http://codereview.chromium.org/3212002/diff/16001/7002#newcode332 base/file_util_proxy.cc:332: if (src_file_path_.value() == dest_file_path_.value() || ditto. http://codereview.chromium.org/3212002/diff/16001/7002#newcode338 base/file_util_proxy.cc:338: set_error_code(base::PLATFORM_FILE_MODIFICATION_DISALLOWED_ERROR); PLATFORM_FILE_ERROR http://codereview.chromium.org/3212002/diff/16001/7002#newcode360 base/file_util_proxy.cc:360: if (!file_util::PathExists(file_path_.DirName())) { PathExists -> DirectoryExists? http://codereview.chromium.org/3212002/diff/16001/7002#newcode365 base/file_util_proxy.cc:365: set_error_code(base:: PLATFORM_FILE_INVALID_MODIFICATION_ERROR); How about returning more informative error code like PLATFORM_FILE_ALREADY_EXISTS (as in CreateFile) http://codereview.chromium.org/3212002/diff/16001/7002#newcode369 base/file_util_proxy.cc:369: set_error_code(base::PLATFORM_FILE_MODIFICATION_DISALLOWED_ERROR); PLATFORM_FILE_ERROR http://codereview.chromium.org/3212002/diff/16001/7002#newcode431 base/file_util_proxy.cc:431: if (!file_util::DirectoryExists(file_path_)) { would be good to add comments in the header file telling that this doesn't work for directory http://codereview.chromium.org/3212002/diff/16001/7003 File base/file_util_proxy.h (right): http://codereview.chromium.org/3212002/diff/16001/7003#newcode72 base/file_util_proxy.h:72: // Copies a file or a directory from src_file_path to dest_file_path. nits: || around the param names http://codereview.chromium.org/3212002/diff/16001/7003#newcode72 base/file_util_proxy.h:72: // Copies a file or a directory from src_file_path to dest_file_path. It'd be great to add some comments about what condition could cause an error (e.g. 'it is an error to copy a file to... bula bula'). http://codereview.chromium.org/3212002/diff/16001/7004 File base/platform_file.h (right): http://codereview.chromium.org/3212002/diff/16001/7004#newcode45 base/platform_file.h:45: PLATFORM_FILE_INVALID_STATE_ERROR = -1, Let's remove this one (see my other comment) http://codereview.chromium.org/3212002/diff/16001/7004#newcode49 base/platform_file.h:49: PLATFORM_FILE_NOT_READABLE_ERROR = -5 I think it'd be better to keep PLATFORM_FILE_ERROR as a generic error code. There can be tons of different error reasons and we're adding only a few of them here. http://codereview.chromium.org/3212002/diff/16001/7005 File chrome/browser/file_system/file_system_backend.cc (right): http://codereview.chromium.org/3212002/diff/16001/7005#newcode14 chrome/browser/file_system/file_system_backend.cc:14: WebKit::WebFileError PlatformToWebkitError(int rv) { indent http://codereview.chromium.org/3212002/diff/16001/7007 File chrome/browser/file_system/file_system_backend_client.h (right): http://codereview.chromium.org/3212002/diff/16001/7007#newcode11 chrome/browser/file_system/file_system_backend_client.h:11: #include "third_party/WebKit/WebKit/chromium/public/WebFileEntry.h" this include doesn't seem to be necessary. (And it doesn't exist)
http://codereview.chromium.org/3212002/diff/16001/7005 File chrome/browser/file_system/file_system_backend.cc (right): http://codereview.chromium.org/3212002/diff/16001/7005#newcode124 chrome/browser/file_system/file_system_backend.cc:124: int rv, base::PassPlatformFile file, bool created) { Hmm... if we could pass through the exclusive flag value to this callback (as we may need to do for request_id) this callback could be written like: if (rv == PLATFORM_FILE_OK || (rv == PLATFORM_FILE_ALREADY_EXISTS && !exclusive)) // success else // failure otherwise we may need to have another RelayCreate something in file_util_proxy (we don't have a good flag for non-exclusive cases - PLATFORM_FILE_CREATE_ALWAYS truncates the length to 0 and that's not what we want).
http://codereview.chromium.org/3212002/diff/16001/7002 File base/file_util_proxy.cc (right): http://codereview.chromium.org/3212002/diff/16001/7002#newcode445 base/file_util_proxy.cc:445: modification_time_ = file_info.last_modified.ToDoubleT(); I think it'd make more sense to return the entire FileInfo here rather than only returning modification_time. (In the fs_backend we can extract what we need.) Then we can simply name this RelayGetFileInfo.
i'm assuming kinuko is reviewing the code in chrome/browser/file_system, so i have only a few style comments for those classes. btw, you might want to talk to darin or brett about adding a sub-directory to chrome/browser, if they haven't approved that already. http://codereview.chromium.org/3212002/diff/16001/7002 File base/file_util_proxy.cc (right): http://codereview.chromium.org/3212002/diff/16001/7002#newcode13 base/file_util_proxy.cc:13: bool IsDirectory(const FilePath& path) { i think we should move this to file_path.h. http://codereview.chromium.org/3212002/diff/16001/7002#newcode19 base/file_util_proxy.cc:19: bool DirectoryExists(const FilePath& path) { use DirectoryExists() in file_util.h. http://codereview.chromium.org/3212002/diff/16001/7002#newcode102 base/file_util_proxy.cc:102: if (exclusive_ && file_util::PathExists(file_path_)) { i agree with kinuko. http://codereview.chromium.org/3212002/diff/16001/7002#newcode108 base/file_util_proxy.cc:108: set_error_code(base::PLATFORM_FILE_MODIFICATION_DISALLOWED_ERROR); i just talked to darin about this, and we think the best way to do this is to add a 'int* error' parameter to CreatePlatformFile. i'll add your error codes from platform_file.h and make that change. http://codereview.chromium.org/3212002/diff/16001/7002#newcode214 base/file_util_proxy.cc:214: class RelayFileExists: public RelayWithStatusCallback { could probably combine RelayFileExists and RelayDirectoryExists into one class. http://codereview.chromium.org/3212002/diff/16001/7002#newcode229 base/file_util_proxy.cc:229: set_error_code(base::PLATFORM_FILE_INVALID_STATE_ERROR); so it looks like the error code in linux is ENOTDIR, and if you do 'cd some_file', you get the 'Not a directory' error message. so i propose we add a PLATFORM_FILE_NOT_A_DIRECTORY_ERROR error code. http://codereview.chromium.org/3212002/diff/16001/7002#newcode268 base/file_util_proxy.cc:268: if (!file_util::PathExists(file_path_)) { if we know of a reason why Delete() might fail, we should check for it before we try to delete anything. and if Delete() fails for some unknown reason, then i think we should return the generic PLATFORM_FILE_ERROR error code. basically, so i'm thinking of something like this: // does the path exist? if (!file_util::PathExists(file_path_)) { set_error_code(PLATFORM_FILE_NOT_FOUND_ERROR); return; } // should recursive_ be true only for directories? if (recursive_ && !IsDirectory(file_path_)) { set_error_code(PLATFORM_FILE_NOT_A_DIRECTORY_ERROR); return; } // other reasons Delete() might fail? if (!file_util::Delete(file_path_, recursive_)) set_error_code(base::PLATFORM_FILE_ERROR); http://codereview.chromium.org/3212002/diff/16001/7002#newcode297 base/file_util_proxy.cc:297: if (!DirectoryExists(dest_file_path_)) { i agree with kinuko that we need something more generic here, but i don't agree with her proposal (it fails if dest_file_path == "/a/b/" and "/a" exists, but "/b" doesn't). i think this method needs to check for the following conditions (and maybe more): 1. src_file_path exists. 2. dest_file_path is either a directory that exists, or is a file in a directory that exists. 3. if src_file_path is a directory, then dest_file_path is a directory too. 4. if src_file_path == dest_file_path, return without doing any work and without setting an error code. 5. !ContainsPath(src_file_path, dest_file_path). 6. ??? http://codereview.chromium.org/3212002/diff/16001/7002#newcode301 base/file_util_proxy.cc:301: if (src_file_path_.value() == dest_file_path_.value() || not all users will have this check in their code, so i think we should keep it here. however, if src_file_path_ == dest_file_path_, then i think we should just return without doing any work or setting any error code. http://codereview.chromium.org/3212002/diff/16001/7002#newcode308 base/file_util_proxy.cc:308: set_error_code(base::PLATFORM_FILE_MODIFICATION_DISALLOWED_ERROR); On 2010/08/27 05:19:03, Kinuko Yasuda wrote: > PLATFORM_FILE_ERROR agree. http://codereview.chromium.org/3212002/diff/16001/7002#newcode327 base/file_util_proxy.cc:327: virtual void RunWork() { same checks as RelayCopy. http://codereview.chromium.org/3212002/diff/16001/7002#newcode365 base/file_util_proxy.cc:365: set_error_code(base:: PLATFORM_FILE_INVALID_MODIFICATION_ERROR); On 2010/08/27 05:19:03, Kinuko Yasuda wrote: > How about returning more informative error code like > PLATFORM_FILE_ALREADY_EXISTS (as in CreateFile) PLATFORM_FILE_ALREADY_EXISTS is used as a flag to CreatePlatformFile. we cannot use it as an error code, but we could add a PLATFORM_FILE_EXISTS_ERROR (the name of the error code on windows is ERROR_FILE_EXISTS, and on linux it's EEXIST). http://codereview.chromium.org/3212002/diff/16001/7002#newcode431 base/file_util_proxy.cc:431: if (!file_util::DirectoryExists(file_path_)) { On 2010/08/27 05:19:03, Kinuko Yasuda wrote: > would be good to add comments in the header file telling that this doesn't work > for directory did you mean if not PathExists()? the way it's coded now, this method will work on directories only. http://codereview.chromium.org/3212002/diff/16001/7002#newcode437 base/file_util_proxy.cc:437: if (!file_util::PathExists(file_path_)) { do we need this check here if we change the one above to PathExists()? http://codereview.chromium.org/3212002/diff/16001/7002#newcode441 base/file_util_proxy.cc:441: set_error_code(base::PLATFORM_FILE_NOT_READABLE_ERROR); PLATFORM_FILE_ERROR. same thing: if we know why GetFileInfo() failed, let's check for that before calling GetFileInfo(). if we don't know why it fails, then we should return PLATFORM_FILE_ERROR. http://codereview.chromium.org/3212002/diff/16001/7002#newcode445 base/file_util_proxy.cc:445: modification_time_ = file_info.last_modified.ToDoubleT(); On 2010/08/27 06:15:44, Kinuko Yasuda wrote: > I think it'd make more sense to return the entire FileInfo here rather than only > returning modification_time. (In the fs_backend we can extract what we need.) > > Then we can simply name this RelayGetFileInfo. i agree. http://codereview.chromium.org/3212002/diff/16001/7002#newcode510 base/file_util_proxy.cc:510: callback)); indentation. 'callback' should align with 'FROM_HERE' (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls). http://codereview.chromium.org/3212002/diff/16001/7002#newcode519 base/file_util_proxy.cc:519: new RelayDirectoryExists(file_path, callback)); indentation. http://codereview.chromium.org/3212002/diff/16001/7002#newcode536 base/file_util_proxy.cc:536: new RelayCopy(src_file_path, dest_file_path, callback)); indentation. http://codereview.chromium.org/3212002/diff/16001/7002#newcode545 base/file_util_proxy.cc:545: new RelayMove(src_file_path, dest_file_path, callback)); indentation. http://codereview.chromium.org/3212002/diff/16001/7002#newcode554 base/file_util_proxy.cc:554: file_path, callback)); indentation. http://codereview.chromium.org/3212002/diff/16001/7002#newcode563 base/file_util_proxy.cc:563: file_path, callback)); indentation. http://codereview.chromium.org/3212002/diff/16001/7003 File base/file_util_proxy.h (right): http://codereview.chromium.org/3212002/diff/16001/7003#newcode5 base/file_util_proxy.h:5: #ifndef BASE_FILE_UTIL_PROXY_H_ thanks for fixing this! http://codereview.chromium.org/3212002/diff/16001/7003#newcode18 base/file_util_proxy.h:18: namespace file_util_proxy { i think it might be better to drop this namespace, and rename Entry to FileSystemEntry, or something like that. http://codereview.chromium.org/3212002/diff/16001/7003#newcode62 base/file_util_proxy.h:62: static bool GetMetadata(scoped_refptr<MessageLoopProxy> message_loop_proxy, how about QueryInfo? i'm not sure 'metadata' is a very popular term on windows, and the corresponding method in the pepper API is called Query. http://codereview.chromium.org/3212002/diff/16001/7003#newcode68 base/file_util_proxy.h:68: static bool ReadDirectory(scoped_refptr<MessageLoopProxy> message_loop_proxy, maybe ListDirectory? hmm, not sure, i'll leave it up to you. http://codereview.chromium.org/3212002/diff/16001/7003#newcode91 base/file_util_proxy.h:91: // Checks whether or not file exists. s/file/directory/ http://codereview.chromium.org/3212002/diff/16001/7003#newcode98 base/file_util_proxy.h:98: static bool FileExists(scoped_refptr<MessageLoopProxy> message_loop_proxy, can we combine these 2 functions? maybe something like: static bool FilePathExists(scoped_refptr<MessageLoopProxy>, const FilePath&, bool isDirectory, StatusCallback*); or: FileSystemEntryExists(scoped_refptr<MLP>, const FileSystemEntry&, StatusCallback*); ? http://codereview.chromium.org/3212002/diff/16001/7004 File base/platform_file.h (right): http://codereview.chromium.org/3212002/diff/16001/7004#newcode49 base/platform_file.h:49: PLATFORM_FILE_NOT_READABLE_ERROR = -5 On 2010/08/27 05:19:03, Kinuko Yasuda wrote: > I think it'd be better to keep PLATFORM_FILE_ERROR as a generic error code. > There can be tons of different error reasons and we're adding only a few of them > here. > agree. http://codereview.chromium.org/3212002/diff/16001/7005 File chrome/browser/file_system/file_system_backend.cc (right): http://codereview.chromium.org/3212002/diff/16001/7005#newcode29 chrome/browser/file_system/file_system_backend.cc:29: DCHECK(false); s/DCHECK(false)/NOTREACHED()/ http://codereview.chromium.org/3212002/diff/16001/7006 File chrome/browser/file_system/file_system_backend.h (right): http://codereview.chromium.org/3212002/diff/16001/7006#newcode25 chrome/browser/file_system/file_system_backend.h:25: void CreateDirectory( if you can't fit the entire function declaration on one line, then i think the following style is preferred in chromium: void CreateDirectory(const FilePath& path, bool exclusive, int request_id);
http://codereview.chromium.org/3212002/diff/16001/7002 File base/file_util_proxy.cc (right): http://codereview.chromium.org/3212002/diff/16001/7002#newcode297 base/file_util_proxy.cc:297: if (!DirectoryExists(dest_file_path_)) { > 4. if src_file_path == dest_file_path, return without doing sorry, i was wrong about this. linux returns an error if src and dst are the same in a copy or move operation, so we should do the same.
http://codereview.chromium.org/3212002/diff/16001/7004 File base/platform_file.h (right): http://codereview.chromium.org/3212002/diff/16001/7004#newcode47 base/platform_file.h:47: PLATFORM_FILE_MODIFICATION_DISALLOWED_ERROR = -3, one more minor comment: what do you think about renaming PLATFORM_FILE_INVALID_MODIFICATION_ERROR to PLATFORM_FILE_INVALID_OPERATION_ERROR? i think it might be useful for other operations (like Read), then don't need to modify anything. and how about PLATFORM_FILE_ACCESS_DENIED_ERROR instead of PLATFORM_FILE_MODIFICATION_DISALLOWED_ERROR? seems like a more generic error type that could be used for other purposes too.
http://codereview.chromium.org/3212002/diff/16001/7002 File base/file_util_proxy.cc (right): http://codereview.chromium.org/3212002/diff/16001/7002#newcode108 base/file_util_proxy.cc:108: set_error_code(base::PLATFORM_FILE_MODIFICATION_DISALLOWED_ERROR); On 2010/08/27 19:30:31, dumi wrote: > i just talked to darin about this, and we think the best way to do this is to > add a 'int* error' parameter to CreatePlatformFile. i'll add your error codes > from platform_file.h and make that change. Great! http://codereview.chromium.org/3212002/diff/16001/7002#newcode214 base/file_util_proxy.cc:214: class RelayFileExists: public RelayWithStatusCallback { On 2010/08/27 19:30:31, dumi wrote: > could probably combine RelayFileExists and RelayDirectoryExists into one class. I talked with kavita and maybe we can integrate GetMetadata, FileExists and DirectoryExists into GetFileInfo. http://codereview.chromium.org/3212002/diff/16001/7002#newcode268 base/file_util_proxy.cc:268: if (!file_util::PathExists(file_path_)) { On 2010/08/27 19:30:31, dumi wrote: > if we know of a reason why Delete() might fail, we should check for it before we > try to delete anything. and if Delete() fails for some unknown reason, then i > think we should return the generic PLATFORM_FILE_ERROR error code. basically, so > i'm thinking of something like this: > > // does the path exist? > if (!file_util::PathExists(file_path_)) { > set_error_code(PLATFORM_FILE_NOT_FOUND_ERROR); > return; > } > > // should recursive_ be true only for directories? > if (recursive_ && !IsDirectory(file_path_)) { > set_error_code(PLATFORM_FILE_NOT_A_DIRECTORY_ERROR); > return; > } > > // other reasons Delete() might fail? > > if (!file_util::Delete(file_path_, recursive_)) > set_error_code(base::PLATFORM_FILE_ERROR); Doesn't that add some cost? I basically agree pre-check would be generally better but I'm concerned with the performance. I don't think it's necessary to do the same checks beforehand if Delete does the same. http://codereview.chromium.org/3212002/diff/16001/7002#newcode297 base/file_util_proxy.cc:297: if (!DirectoryExists(dest_file_path_)) { On 2010/08/27 19:30:31, dumi wrote: > i agree with kinuko that we need something more generic here, but i don't agree > with her proposal (it fails if dest_file_path == "/a/b/" and "/a" exists, but > "/b" doesn't). i think this method needs to check for the following conditions > (and maybe more): I may not understand your example - do you mean if we call RelayCopy("/dir", "/a/newdir/") and "/a" exists but "/a/newdir" doesn't it should NOT fail? What I thought was: if we're given RelayCopy("/dir", "/a/newdir/") this must be interpreted as: "Copy /dir to /a/newdir/dir". And if "/a/newdir" doesn't exist it should return NOT_FOUND error. (On the contrary if we call RelayCopy("/dir", "/a/newdir") I think "/dir" must be copied to "/a/newdir" if "/a" exists. Or if you're saying we should create the "/a/newdir/" if the dest_path ends with "/", I think we'd better add a flag to indicate that (e.g. always_create_dir or something). > 1. src_file_path exists. This must fail anyway in CopyDirectory. > 2. dest_file_path is either a directory that exists, or is a file in a directory > that exists. > 3. if src_file_path is a directory, then dest_file_path is a directory too. If src_file_path is a directory and dest_file_path exists AND is a file, doesn't CopyDirectory fail? (I need to check) > 4. if src_file_path == dest_file_path, return without doing any work and without > setting an error code. This must fail in CopyDirectory. > 5. !ContainsPath(src_file_path, dest_file_path). > 6. ??? http://codereview.chromium.org/3212002/diff/16001/7002#newcode431 base/file_util_proxy.cc:431: if (!file_util::DirectoryExists(file_path_)) { On 2010/08/27 19:30:31, dumi wrote: > On 2010/08/27 05:19:03, Kinuko Yasuda wrote: > > would be good to add comments in the header file telling that this doesn't > work > > for directory > > did you mean if not PathExists()? the way it's coded now, this method will work > on directories only. Agreed and I think we're going to replace this with more generic GetFileInfo.
On 2010/08/27 20:17:59, dumi wrote: > http://codereview.chromium.org/3212002/diff/16001/7004 > File base/platform_file.h (right): > > http://codereview.chromium.org/3212002/diff/16001/7004#newcode47 > base/platform_file.h:47: PLATFORM_FILE_MODIFICATION_DISALLOWED_ERROR = -3, > one more minor comment: what do you think about renaming > PLATFORM_FILE_INVALID_MODIFICATION_ERROR to > PLATFORM_FILE_INVALID_OPERATION_ERROR? i think it might be useful for other > operations (like Read), then don't need to modify anything. > > and how about PLATFORM_FILE_ACCESS_DENIED_ERROR instead of > PLATFORM_FILE_MODIFICATION_DISALLOWED_ERROR? seems like a more generic error > type that could be used for other purposes too. I agree, sounds good to me.
Dumi - I have patched your CL and will wait to submit - I have just 1 extra error code for platform_file.h. I have reverted that file from this patch. Kinuko, please take a look. Thanks, Kavita. http://codereview.chromium.org/3212002/diff/16001/7002 File base/file_util_proxy.cc (right): http://codereview.chromium.org/3212002/diff/16001/7002#newcode13 base/file_util_proxy.cc:13: bool IsDirectory(const FilePath& path) { Removed all helpers :) On 2010/08/27 19:30:31, dumi wrote: > i think we should move this to file_path.h. http://codereview.chromium.org/3212002/diff/16001/7002#newcode19 base/file_util_proxy.cc:19: bool DirectoryExists(const FilePath& path) { On 2010/08/27 19:30:31, dumi wrote: > use DirectoryExists() in file_util.h. Done. http://codereview.chromium.org/3212002/diff/16001/7002#newcode78 base/file_util_proxy.cc:78: bool exclusive, On 2010/08/27 05:19:03, Kinuko Yasuda wrote: > this flag won't be necessary (see below) Done. http://codereview.chromium.org/3212002/diff/16001/7002#newcode102 base/file_util_proxy.cc:102: if (exclusive_ && file_util::PathExists(file_path_)) { On 2010/08/27 05:19:03, Kinuko Yasuda wrote: > No need to check this here - seems like if you call CreatePlatformFile with > flags=PLATFORM_FILE_CREATE it always fails if the entry already exists. Done. http://codereview.chromium.org/3212002/diff/16001/7002#newcode108 base/file_util_proxy.cc:108: set_error_code(base::PLATFORM_FILE_MODIFICATION_DISALLOWED_ERROR); Will wait for ur CL to be submitted. I will just check the int error then. On 2010/08/27 20:20:35, Kinuko Yasuda wrote: > On 2010/08/27 19:30:31, dumi wrote: > > i just talked to darin about this, and we think the best way to do this is to > > add a 'int* error' parameter to CreatePlatformFile. i'll add your error codes > > from platform_file.h and make that change. > > Great! > http://codereview.chromium.org/3212002/diff/16001/7002#newcode108 base/file_util_proxy.cc:108: set_error_code(base::PLATFORM_FILE_MODIFICATION_DISALLOWED_ERROR); On 2010/08/27 05:19:03, Kinuko Yasuda wrote: > How about changing this error check statement to: > > if (file_handle == base::kInvalidPlatformFileValue) { > if ((flags & base::PLATFORM_FILE_CREATE) && file_util::PathExists(file_path)) > set_error_code(base::PLATFORM_FILE_ALREADY_EXISTS); > else > set_error_code(base::PLATFORM_FILE_ERROR); > } > > For generic file errors like this I don't think we should replace it with > another error code like MODIFICATION_DISALLOWED_ERROR - CreatePlatformFile can > fail here for several different reasons while 'MODIFICATION_DISALLOWED' sounds > more specific error type. Maybe we should just continue to use > PLATFORM_FILE_ERROR in such cases. > Done. http://codereview.chromium.org/3212002/diff/16001/7002#newcode158 base/file_util_proxy.cc:158: set_error_code(base::PLATFORM_FILE_MODIFICATION_DISALLOWED_ERROR); On 2010/08/27 05:19:03, Kinuko Yasuda wrote: > same here; do we need to change this error code? Done. http://codereview.chromium.org/3212002/diff/16001/7002#newcode207 base/file_util_proxy.cc:207: set_error_code(base::PLATFORM_FILE_MODIFICATION_DISALLOWED_ERROR); On 2010/08/27 05:19:03, Kinuko Yasuda wrote: > ditto. Done. http://codereview.chromium.org/3212002/diff/16001/7002#newcode214 base/file_util_proxy.cc:214: class RelayFileExists: public RelayWithStatusCallback { On 2010/08/27 20:20:35, Kinuko Yasuda wrote: > On 2010/08/27 19:30:31, dumi wrote: > > could probably combine RelayFileExists and RelayDirectoryExists into one > class. > > I talked with kavita and maybe we can integrate GetMetadata, FileExists and > DirectoryExists into GetFileInfo. > Done. http://codereview.chromium.org/3212002/diff/16001/7002#newcode229 base/file_util_proxy.cc:229: set_error_code(base::PLATFORM_FILE_INVALID_STATE_ERROR); Consolidated all to GetFileInfo On 2010/08/27 19:30:31, dumi wrote: > so it looks like the error code in linux is ENOTDIR, and if you do 'cd > some_file', you get the 'Not a directory' error message. so i propose we add a > PLATFORM_FILE_NOT_A_DIRECTORY_ERROR error code. http://codereview.chromium.org/3212002/diff/16001/7002#newcode268 base/file_util_proxy.cc:268: if (!file_util::PathExists(file_path_)) { Yeah. Slowness was the reason I put the check inside. On 2010/08/27 20:20:35, Kinuko Yasuda wrote: > On 2010/08/27 19:30:31, dumi wrote: > > if we know of a reason why Delete() might fail, we should check for it before > we > > try to delete anything. and if Delete() fails for some unknown reason, then i > > think we should return the generic PLATFORM_FILE_ERROR error code. basically, > so > > i'm thinking of something like this: > > > > // does the path exist? > > if (!file_util::PathExists(file_path_)) { > > set_error_code(PLATFORM_FILE_NOT_FOUND_ERROR); > > return; > > } > > > > // should recursive_ be true only for directories? > > if (recursive_ && !IsDirectory(file_path_)) { > > set_error_code(PLATFORM_FILE_NOT_A_DIRECTORY_ERROR); > > return; > > } > > > > // other reasons Delete() might fail? > > > > if (!file_util::Delete(file_path_, recursive_)) > > set_error_code(base::PLATFORM_FILE_ERROR); > > Doesn't that add some cost? I basically agree pre-check would be generally > better but I'm concerned with the performance. I don't think it's necessary to > do the same checks beforehand if Delete does the same. http://codereview.chromium.org/3212002/diff/16001/7002#newcode272 base/file_util_proxy.cc:272: if (!file_util::IsDirectoryEmpty(file_path_)) { On 2010/08/27 05:19:03, Kinuko Yasuda wrote: > if (!recursive && !file_util::IsDirectoryEmpty(file_path_)) { Done. http://codereview.chromium.org/3212002/diff/16001/7002#newcode276 base/file_util_proxy.cc:276: set_error_code(base::PLATFORM_FILE_MODIFICATION_DISALLOWED_ERROR); On 2010/08/27 05:19:03, Kinuko Yasuda wrote: > again PLATFORM_FILE_ERROR would be better here and we can translate the error > code into NoModificationAllowedError in our error translation code. Done. http://codereview.chromium.org/3212002/diff/16001/7002#newcode297 base/file_util_proxy.cc:297: if (!DirectoryExists(dest_file_path_)) { Done! On 2010/08/27 20:20:35, Kinuko Yasuda wrote: > On 2010/08/27 19:30:31, dumi wrote: > > i agree with kinuko that we need something more generic here, but i don't > agree > > with her proposal (it fails if dest_file_path == "/a/b/" and "/a" exists, but > > "/b" doesn't). i think this method needs to check for the following conditions > > (and maybe more): > > I may not understand your example - do you mean if we call RelayCopy("/dir", > "/a/newdir/") and "/a" exists but "/a/newdir" doesn't it should NOT fail? > > What I thought was: if we're given RelayCopy("/dir", "/a/newdir/") this must be > interpreted as: "Copy /dir to /a/newdir/dir". And if "/a/newdir" doesn't exist > it should return NOT_FOUND error. (On the contrary if we call RelayCopy("/dir", > "/a/newdir") I think "/dir" must be copied to "/a/newdir" if "/a" exists. > > Or if you're saying we should create the "/a/newdir/" if the dest_path ends with > "/", I think we'd better add a flag to indicate that (e.g. always_create_dir or > something). > > > 1. src_file_path exists. > > This must fail anyway in CopyDirectory. > > > 2. dest_file_path is either a directory that exists, or is a file in a > directory > > that exists. > > 3. if src_file_path is a directory, then dest_file_path is a directory too. > > If src_file_path is a directory and dest_file_path exists AND is a file, doesn't > CopyDirectory fail? (I need to check) > > > 4. if src_file_path == dest_file_path, return without doing any work and > without > > setting an error code. > > This must fail in CopyDirectory. > > > 5. !ContainsPath(src_file_path, dest_file_path). > > 6. ??? > > http://codereview.chromium.org/3212002/diff/16001/7002#newcode301 base/file_util_proxy.cc:301: if (src_file_path_.value() == dest_file_path_.value() || On 2010/08/27 05:19:03, Kinuko Yasuda wrote: > I think we can remove this check (the former one) now that we made sure that we > have this check in the renderer. (thanks for adding though) Done. http://codereview.chromium.org/3212002/diff/16001/7002#newcode308 base/file_util_proxy.cc:308: set_error_code(base::PLATFORM_FILE_MODIFICATION_DISALLOWED_ERROR); On 2010/08/27 05:19:03, Kinuko Yasuda wrote: > PLATFORM_FILE_ERROR Done. http://codereview.chromium.org/3212002/diff/16001/7002#newcode327 base/file_util_proxy.cc:327: virtual void RunWork() { On 2010/08/27 19:30:31, dumi wrote: > same checks as RelayCopy. Done. http://codereview.chromium.org/3212002/diff/16001/7002#newcode328 base/file_util_proxy.cc:328: if (!DirectoryExists(dest_file_path_)) { On 2010/08/27 05:19:03, Kinuko Yasuda wrote: > ditto. (if (!file_util::PathExists(dest_file_path_) && > !file_util::DirectoryExists(dest_file_path_.DirName())) Done. http://codereview.chromium.org/3212002/diff/16001/7002#newcode332 base/file_util_proxy.cc:332: if (src_file_path_.value() == dest_file_path_.value() || On 2010/08/27 05:19:03, Kinuko Yasuda wrote: > ditto. Done. http://codereview.chromium.org/3212002/diff/16001/7002#newcode338 base/file_util_proxy.cc:338: set_error_code(base::PLATFORM_FILE_MODIFICATION_DISALLOWED_ERROR); On 2010/08/27 05:19:03, Kinuko Yasuda wrote: > PLATFORM_FILE_ERROR Done. http://codereview.chromium.org/3212002/diff/16001/7002#newcode360 base/file_util_proxy.cc:360: if (!file_util::PathExists(file_path_.DirName())) { I think what I had was as per the specs. Let me know! On 2010/08/27 05:19:03, Kinuko Yasuda wrote: > PathExists -> DirectoryExists? http://codereview.chromium.org/3212002/diff/16001/7002#newcode365 base/file_util_proxy.cc:365: set_error_code(base:: PLATFORM_FILE_INVALID_MODIFICATION_ERROR); On 2010/08/27 05:19:03, Kinuko Yasuda wrote: > How about returning more informative error code like > PLATFORM_FILE_ALREADY_EXISTS (as in CreateFile) Done. http://codereview.chromium.org/3212002/diff/16001/7002#newcode369 base/file_util_proxy.cc:369: set_error_code(base::PLATFORM_FILE_MODIFICATION_DISALLOWED_ERROR); On 2010/08/27 05:19:03, Kinuko Yasuda wrote: > PLATFORM_FILE_ERROR Done. http://codereview.chromium.org/3212002/diff/16001/7002#newcode563 base/file_util_proxy.cc:563: file_path, callback)); On 2010/08/27 19:30:31, dumi wrote: > indentation. Done. http://codereview.chromium.org/3212002/diff/16001/7003 File base/file_util_proxy.h (right): http://codereview.chromium.org/3212002/diff/16001/7003#newcode18 base/file_util_proxy.h:18: namespace file_util_proxy { base::FileSystemEntry seems too generic. base::file_util_proxy::Entry looked ok to me. Let me know if you feel strongly about this renaming. On 2010/08/27 19:30:31, dumi wrote: > i think it might be better to drop this namespace, and rename Entry to > FileSystemEntry, or something like that. http://codereview.chromium.org/3212002/diff/16001/7003#newcode62 base/file_util_proxy.h:62: static bool GetMetadata(scoped_refptr<MessageLoopProxy> message_loop_proxy, On 2010/08/27 19:30:31, dumi wrote: > how about QueryInfo? i'm not sure 'metadata' is a very popular term on windows, > and the corresponding method in the pepper API is called Query. Done. http://codereview.chromium.org/3212002/diff/16001/7003#newcode72 base/file_util_proxy.h:72: // Copies a file or a directory from src_file_path to dest_file_path. On 2010/08/27 05:19:03, Kinuko Yasuda wrote: > nits: || around the param names Done. http://codereview.chromium.org/3212002/diff/16001/7003#newcode91 base/file_util_proxy.h:91: // Checks whether or not file exists. On 2010/08/27 19:30:31, dumi wrote: > s/file/directory/ Done. http://codereview.chromium.org/3212002/diff/16001/7003#newcode98 base/file_util_proxy.h:98: static bool FileExists(scoped_refptr<MessageLoopProxy> message_loop_proxy, On 2010/08/27 19:30:31, dumi wrote: > can we combine these 2 functions? maybe something like: > > static bool FilePathExists(scoped_refptr<MessageLoopProxy>, > const FilePath&, > bool isDirectory, > StatusCallback*); > > or: > > FileSystemEntryExists(scoped_refptr<MLP>, > const FileSystemEntry&, > StatusCallback*); > > ? Done. http://codereview.chromium.org/3212002/diff/16001/7005 File chrome/browser/file_system/file_system_backend.cc (right): http://codereview.chromium.org/3212002/diff/16001/7005#newcode14 chrome/browser/file_system/file_system_backend.cc:14: WebKit::WebFileError PlatformToWebkitError(int rv) { On 2010/08/27 05:19:03, Kinuko Yasuda wrote: > indent Done. http://codereview.chromium.org/3212002/diff/16001/7005#newcode29 chrome/browser/file_system/file_system_backend.cc:29: DCHECK(false); On 2010/08/27 19:30:31, dumi wrote: > s/DCHECK(false)/NOTREACHED()/ Done. http://codereview.chromium.org/3212002/diff/16001/7005#newcode124 chrome/browser/file_system/file_system_backend.cc:124: int rv, base::PassPlatformFile file, bool created) { One trick is to pass exclusive to Relay function to simply be passed back to callback :) This will be a TODO until I find a way to pass req id. On 2010/08/27 05:33:48, Kinuko Yasuda wrote: > Hmm... if we could pass through the exclusive flag value to this callback (as we > may need to do for request_id) this callback could be written like: > > if (rv == PLATFORM_FILE_OK || (rv == PLATFORM_FILE_ALREADY_EXISTS && > !exclusive)) > // success > else > // failure > > otherwise we may need to have another RelayCreate something in file_util_proxy > (we don't have a good flag for non-exclusive cases - PLATFORM_FILE_CREATE_ALWAYS > truncates the length to 0 and that's not what we want).
looking good; some more comments. http://codereview.chromium.org/3212002/diff/16001/7006 File chrome/browser/file_system/file_system_backend.h (right): http://codereview.chromium.org/3212002/diff/16001/7006#newcode25 chrome/browser/file_system/file_system_backend.h:25: void CreateDirectory( On 2010/08/27 19:30:31, dumi wrote: > if you can't fit the entire function declaration on one line, then i think the > following style is preferred in chromium: > > void CreateDirectory(const FilePath& path, > bool exclusive, > int request_id); nits: if you don't have strong preference it'd be good to follow this. http://codereview.chromium.org/3212002/diff/21002/26002 File base/file_util_proxy.cc (right): http://codereview.chromium.org/3212002/diff/21002/26002#newcode16 base/file_util_proxy.cc:16: base::MessageLoopProxy::CreateForCurrentThread()), nit: indent: 2 more spaces? http://codereview.chromium.org/3212002/diff/21002/26002#newcode230 base/file_util_proxy.cc:230: if (!file_util::PathExists(dest_file_path_) && nits: can we cache the result of PathExists(dest_file_path_) for later checks? http://codereview.chromium.org/3212002/diff/21002/26002#newcode235 base/file_util_proxy.cc:235: // src exists and is a directory. Dest exists and is a file. nits: src -> |src_file_path|, Dest -> |dest_file_path| http://codereview.chromium.org/3212002/diff/21002/26002#newcode236 base/file_util_proxy.cc:236: if (!(file_util::DirectoryExists(src_file_path_) && do we need to negate the conditions here? http://codereview.chromium.org/3212002/diff/21002/26002#newcode252 base/file_util_proxy.cc:252: if (!file_util::PathExists(src_file_path_)) { Probably this should be checked before the above check (src==dest). http://codereview.chromium.org/3212002/diff/21002/26002#newcode278 base/file_util_proxy.cc:278: if (!file_util::PathExists(dest_file_path_) && ditto. (here and others; as in Copy) http://codereview.chromium.org/3212002/diff/21002/26002#newcode477 base/file_util_proxy.cc:477: new RelayCopy(src_file_path, dest_file_path, callback)); nits: indentation. http://codereview.chromium.org/3212002/diff/21002/26002#newcode486 base/file_util_proxy.cc:486: new RelayMove(src_file_path, dest_file_path, callback)); nits: indentation. http://codereview.chromium.org/3212002/diff/21002/26004 File chrome/browser/file_system/file_system_backend.cc (right): http://codereview.chromium.org/3212002/diff/21002/26004#newcode183 chrome/browser/file_system/file_system_backend.cc:183: client_->DidReadMetadata(file_info.last_modified.ToDoubleT(), request_id_); I told you I'll want DidReadMetadata(double), but could you just return FileInfo here? We may be extending the ReadMetadata call to include other info (e.g. size). http://codereview.chromium.org/3212002/diff/21002/26006 File chrome/browser/file_system/file_system_backend_client.h (right): http://codereview.chromium.org/3212002/diff/21002/26006#newcode11 chrome/browser/file_system/file_system_backend_client.h:11: #include "third_party/WebKit/WebKit/chromium/public/WebFileEntry.h" On 2010/08/27 05:19:03, Kinuko Yasuda wrote: > this include doesn't seem to be necessary. (And it doesn't exist) this line needs to be deleted. http://codereview.chromium.org/3212002/diff/21002/26006#newcode23 chrome/browser/file_system/file_system_backend_client.h:23: virtual void DidReadMetadata(double modification_time, int request_id) = 0; Could you change this to DidReadMetadata(const FileInfo& fileInfo, int request_id)? It would make things simpler.
Thanks. Changed the metadata to now give you the entire file info instead of just double modification date. Please take a look. http://codereview.chromium.org/3212002/diff/21002/26002 File base/file_util_proxy.cc (right): http://codereview.chromium.org/3212002/diff/21002/26002#newcode16 base/file_util_proxy.cc:16: base::MessageLoopProxy::CreateForCurrentThread()), On 2010/08/30 21:51:36, Kinuko Yasuda wrote: > nit: indent: 2 more spaces? Done. http://codereview.chromium.org/3212002/diff/21002/26002#newcode230 base/file_util_proxy.cc:230: if (!file_util::PathExists(dest_file_path_) && On 2010/08/30 21:51:36, Kinuko Yasuda wrote: > nits: can we cache the result of PathExists(dest_file_path_) for later checks? Done. http://codereview.chromium.org/3212002/diff/21002/26002#newcode235 base/file_util_proxy.cc:235: // src exists and is a directory. Dest exists and is a file. On 2010/08/30 21:51:36, Kinuko Yasuda wrote: > nits: src -> |src_file_path|, Dest -> |dest_file_path| Done. http://codereview.chromium.org/3212002/diff/21002/26002#newcode236 base/file_util_proxy.cc:236: if (!(file_util::DirectoryExists(src_file_path_) && On 2010/08/30 21:51:36, Kinuko Yasuda wrote: > do we need to negate the conditions here? Done. http://codereview.chromium.org/3212002/diff/21002/26002#newcode252 base/file_util_proxy.cc:252: if (!file_util::PathExists(src_file_path_)) { On 2010/08/30 21:51:36, Kinuko Yasuda wrote: > Probably this should be checked before the above check (src==dest). Done. http://codereview.chromium.org/3212002/diff/21002/26002#newcode278 base/file_util_proxy.cc:278: if (!file_util::PathExists(dest_file_path_) && On 2010/08/30 21:51:36, Kinuko Yasuda wrote: > ditto. (here and others; as in Copy) Done. http://codereview.chromium.org/3212002/diff/21002/26002#newcode477 base/file_util_proxy.cc:477: new RelayCopy(src_file_path, dest_file_path, callback)); On 2010/08/30 21:51:36, Kinuko Yasuda wrote: > nits: indentation. Done. http://codereview.chromium.org/3212002/diff/21002/26002#newcode486 base/file_util_proxy.cc:486: new RelayMove(src_file_path, dest_file_path, callback)); On 2010/08/30 21:51:36, Kinuko Yasuda wrote: > nits: indentation. Done. http://codereview.chromium.org/3212002/diff/21002/26004 File chrome/browser/file_system/file_system_backend.cc (right): http://codereview.chromium.org/3212002/diff/21002/26004#newcode183 chrome/browser/file_system/file_system_backend.cc:183: client_->DidReadMetadata(file_info.last_modified.ToDoubleT(), request_id_); On 2010/08/30 21:51:36, Kinuko Yasuda wrote: > I told you I'll want DidReadMetadata(double), but could you just return FileInfo > here? We may be extending the ReadMetadata call to include other info (e.g. > size). Done.
LGTM for error codes & browser/file_system/ part. http://codereview.chromium.org/3212002/diff/37001/38001 File base/file_util_proxy.cc (right): http://codereview.chromium.org/3212002/diff/37001/38001#newcode236 base/file_util_proxy.cc:236: // |src_file_path| exists and is a directory. Dest exists and is a file. nits: Dest -> |dest_file_path| http://codereview.chromium.org/3212002/diff/37001/38001#newcode238 base/file_util_proxy.cc:238: dest_path_exists && !file_util::DirectoryExists(dest_file_path_)) { nits: indentation http://codereview.chromium.org/3212002/diff/37001/38001#newcode289 base/file_util_proxy.cc:289: !file_util::DirectoryExists(dest_file_path_)) { nits: indentation http://codereview.chromium.org/3212002/diff/37001/38002 File base/file_util_proxy.h (right): http://codereview.chromium.org/3212002/diff/37001/38002#newcode20 base/file_util_proxy.h:20: // Holds metadata for file entry. nits: indent It'd be better to have more descriptive comments - how about "Holds metadata for directory entries. Used for ReadDirectory's callbacks." or something like that? http://codereview.chromium.org/3212002/diff/37001/38004 File chrome/browser/file_system/file_system_backend.h (right): http://codereview.chromium.org/3212002/diff/37001/38004#newcode41 chrome/browser/file_system/file_system_backend.h:41: void Move(const FilePath& src_path, super nits: it may look a bit prettier if we put Copy and Move next to each other (same for .cc). http://codereview.chromium.org/3212002/diff/37001/38005 File chrome/browser/file_system/file_system_backend_client.h (right): http://codereview.chromium.org/3212002/diff/37001/38005#newcode11 chrome/browser/file_system/file_system_backend_client.h:11: #include "third_party/WebKit/WebKit/chromium/public/WebFileEntry.h" Please remove this line. (it was renamed to WebFileSystemEntry.h and we don't need the file here)
Dumi, I have your changes now. I will wait for your LGTM as well. Thanks for reviews!
i'm _really_ hoping to submit my patch tonight... sorry it's taking so long... http://codereview.chromium.org/3212002/diff/17003/16002 File base/file_util_proxy.cc (right): http://codereview.chromium.org/3212002/diff/17003/16002#newcode201 base/file_util_proxy.cc:201: if (!file_util::PathExists(file_path_)) { i think we should be checking for this (and the next if-block), before we call file_util::Delete(). http://codereview.chromium.org/3212002/diff/17003/16002#newcode240 base/file_util_proxy.cc:240: set_error_code(base::PLATFORM_FILE_ERROR_EXISTS); i think we should return PLATFORM_FILE_ERROR_NOT_A_DIRECTORY here. http://codereview.chromium.org/3212002/diff/17003/16002#newcode249 base/file_util_proxy.cc:249: if (!file_util::PathExists(src_file_path_)) { i think we should check for these 2 conditions before calling CopyDirectory(). http://codereview.chromium.org/3212002/diff/17003/16002#newcode298 base/file_util_proxy.cc:298: if (!file_util::PathExists(src_file_path_)) { check for these 2 conditions before calling Move(). http://codereview.chromium.org/3212002/diff/17003/16002#newcode328 base/file_util_proxy.cc:328: virtual void RunWork() { i think we need 1 more check here: file_path_ exists, and it's a file. in that case i think we should return PLATFORM_FILE_ERROR_EXISTS (or NOT_A_DIRECTORY?). http://codereview.chromium.org/3212002/diff/17003/16002#newcode368 base/file_util_proxy.cc:368: while (!(current = file_enum.Next()).value().empty()) { no need for .value(): FilePath has an empty() method too. http://codereview.chromium.org/3212002/diff/17003/16002#newcode373 base/file_util_proxy.cc:373: entry.name = file_util::FileEnumerator::GetFilename(info).value(); entry.name = current.value(); http://codereview.chromium.org/3212002/diff/17003/16002#newcode389 base/file_util_proxy.cc:389: class RelayGetFileInfo : public MessageLoopRelay { looks like jianli added a GetFileInfo function at http://codereview.chromium.org/3282003. should be easy for you to merge the changes though. http://codereview.chromium.org/3212002/diff/17003/16003 File base/file_util_proxy.h (right): http://codereview.chromium.org/3212002/diff/17003/16003#newcode22 base/file_util_proxy.h:22: FilePath::StringType name; why FilePath::StringType and not just FilePath? http://codereview.chromium.org/3212002/diff/17003/16003#newcode59 base/file_util_proxy.h:59: // Reads FileInfo at given |file_path|. It s invalid to pass NULL for typo: "It s" http://codereview.chromium.org/3212002/diff/17003/16004 File base/platform_file.h (right): http://codereview.chromium.org/3212002/diff/17003/16004#newcode53 base/platform_file.h:53: PLATFORM_FILE_ERROR_NO_SPACE = -7, one of these 2 is -8. http://codereview.chromium.org/3212002/diff/17003/16004#newcode55 base/platform_file.h:55: PLATFORM_FILE_ERROR_INVALID_MODIFICATION = -10 s/MODIFICATION/OPERATION/ ? http://codereview.chromium.org/3212002/diff/17003/16005 File chrome/browser/file_system/file_system_backend.cc (right): http://codereview.chromium.org/3212002/diff/17003/16005#newcode16 chrome/browser/file_system/file_system_backend.cc:16: static_cast<base::PlatformFileError>(rv); if you change rv's type from int to base::PlatformFileError, then you won't need this static_cast. http://codereview.chromium.org/3212002/diff/17003/16005#newcode29 chrome/browser/file_system/file_system_backend.cc:29: } // Anonymous namespace i think the ending comment for anonymous namespaces usually is just "// namespace". http://codereview.chromium.org/3212002/diff/17003/16005#newcode42 chrome/browser/file_system/file_system_backend.cc:42: if (exclusive) you definitely need {} around these if-else blocks. :) or how about replacing the whole thing with: base::FileUtilProxy::CreateOrOpen( ChromeThread::GetMessageLoopProxyForThread(ChromeThread::FILE), path, base::PLATFORM_FILE_CREATE, reinterpret_cast<base::FileUtilProxy::CreateOrOpenCallback*>( callback_factory_.NewCallback( exclusive ? &FileSystemBackend::DidCreateFileExclusive : &FileSystemBackend::DidCreateFileNonExclusive))); ? http://codereview.chromium.org/3212002/diff/17003/16005#newcode92 chrome/browser/file_system/file_system_backend.cc:92: path, reinterpret_cast<base::FileUtilProxy::GetFileInfoCallback*>( i'm changing all callbacks to take a base::PlatformFileError as the first argument, so you shouldn't need these reinterpret_casts. http://codereview.chromium.org/3212002/diff/17003/16006 File chrome/browser/file_system/file_system_backend.h (right): http://codereview.chromium.org/3212002/diff/17003/16006#newcode52 chrome/browser/file_system/file_system_backend.h:52: int rv, base::PassPlatformFile file, bool created); please change 'int rv' to 'base::PlatformFileError rv' everywhere in this file.
http://codereview.chromium.org/3212002/diff/17003/16002 File base/file_util_proxy.cc (right): http://codereview.chromium.org/3212002/diff/17003/16002#newcode298 base/file_util_proxy.cc:298: if (!file_util::PathExists(src_file_path_)) { On 2010/08/31 04:19:34, dumi wrote: > check for these 2 conditions before calling Move(). nevermind, kinuko just told me that Move() already checks for these conditions. just ignore this comment and the other similar ones.
I tried replying through the codereview.chromium it didn't work. Dumi please take a look - I merged your int => PlatformFileError change in platform_file.h Thanks. On Mon, Aug 30, 2010 at 9:26 PM, <dumi@chromium.org> wrote: > > http://codereview.chromium.org/3212002/diff/17003/16002 > File base/file_util_proxy.cc (right): > > http://codereview.chromium.org/3212002/diff/17003/16002#newcode298 > base/file_util_proxy.cc:298: if (!file_util::PathExists(src_file_path_)) > { > On 2010/08/31 04:19:34, dumi wrote: >> >> check for these 2 conditions before calling Move(). > > nevermind, kinuko just told me that Move() already checks for these > conditions. just ignore this comment and the other similar ones. > > http://codereview.chromium.org/3212002/show >
LGTM!!! http://codereview.chromium.org/3212002/diff/60001/61007 File chrome/chrome_browser.gypi (right): http://codereview.chromium.org/3212002/diff/60001/61007#newcode1511 chrome/chrome_browser.gypi:1511: 'browser/file_system/file_system_backend.h', remove.
http://codereview.chromium.org/3212002/diff/60001/61002 File base/file_util_proxy.h (right): http://codereview.chromium.org/3212002/diff/60001/61002#newcode23 base/file_util_proxy.h:23: namespace file_util_proxy { introducing the file_util_proxy namespace like this is kind of awkward. why not just make Entry be defined within the FileUtilProxy class? were you trying to support forward declaring Entry without including this header file? http://codereview.chromium.org/3212002/diff/60001/61002#newcode76 base/file_util_proxy.h:76: typedef Callback2<base::PlatformFileError /* error code */, nit: no need to mention "base::" when you are inside namespace base { ... }
http://codereview.chromium.org/3212002/diff/60001/61002 File base/file_util_proxy.h (right): http://codereview.chromium.org/3212002/diff/60001/61002#newcode23 base/file_util_proxy.h:23: namespace file_util_proxy { Sending a follow-up CL to move this within the FileUtilProxy class. On 2010/11/01 17:28:38, darin wrote: > introducing the file_util_proxy namespace like this is kind of awkward. why not > just make Entry be defined within the FileUtilProxy class? were you trying to > support forward declaring Entry without including this header file? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
