Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8907014:
Implement async verision of FileSystemFileUtil and in-memory file system for testing purposes. (Closed)
|
Created:
9 years ago by satorux1 Modified:
9 years ago CC:
chromium-reviews, pam+watch_chromium.org, kinuko+watch, darin-cc_chromium.org, Paweł Hajdan Jr. Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement async verision of FileSystemFileUtil and in-memory file system for testing purposes.
patch from issue 8574007
BUG=chromium-os:23316
TEST=out/Debug/test_shell_tests --gtest_filter='MemoryFileUtilTest.*'
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114923
Patch Set 1 #Patch Set 2 : Fix the build #Patch Set 3 : Address comments #
Total comments: 41
Patch Set 4 : Address comments #
Total comments: 6
Patch Set 5 : Remove Read() and Write() from FileUtilAsync #
Total comments: 6
Patch Set 6 : Move Open() implementation out #Patch Set 7 : Remove file_util_async.cc #
Total comments: 19
Patch Set 8 : Address comments #Patch Set 9 : Address comments #Patch Set 10 : minor cleanup #Patch Set 11 : RemoveMode -> bool #Patch Set 12 : rebase #Patch Set 13 : fix clang #Patch Set 14 : fix the build... #Patch Set 15 : add webkit/chromeos/OWNERS #
Total comments: 4
Patch Set 16 : fix nits #
Messages
Total messages: 25 (0 generated)
Thanks. Some more comments http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/asyn... File webkit/chromeos/fileapi/async_file_stream.h (right): http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/asyn... webkit/chromeos/fileapi/async_file_stream.h:6: #define WEBKIT_FILEAPI_ASYNC_FILE_STREAM_H_ nit: needs to be updated as well (to add CHROMEOS_) http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/file... File webkit/chromeos/fileapi/file_util_async.cc (right): http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/file... webkit/chromeos/fileapi/file_util_async.cc:131: // ... or should if be _FAILED? Not really sure if we want this error check. (For example FileStream.Seek() doesn't return error in this case) http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/file... webkit/chromeos/fileapi/file_util_async.cc:234: base::PLATFORM_FILE_ERROR_INVALID_OPERATION, callback)); I think this should work: Bind(callback, INVALID_OPERATION, static_cast<AsyncFileStream*>(NULL)); http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/file... webkit/chromeos/fileapi/file_util_async.cc:287: base::Unretained(this), file_path, flags, 0, callback)); need to return here? http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/file... File webkit/chromeos/fileapi/file_util_async.h (right): http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/file... webkit/chromeos/fileapi/file_util_async.h:48: )> ReadDirectoryCallback; nit: weird indentation http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/memo... File webkit/chromeos/fileapi/memory_file_util.cc (right): http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/memo... webkit/chromeos/fileapi/memory_file_util.cc:8: nit: extra empty line http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/memo... webkit/chromeos/fileapi/memory_file_util.cc:109: const ReadWriteCallback& callback) { DCHECK(data) ? This code assumes the data pointer must be valid until the write ends-- maybe we should note that in the AsyncFileUtil interface header file? http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/memo... webkit/chromeos/fileapi/memory_file_util.cc:122: const ReadWriteCallback& callback) { DCHECK(buffer) ? http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/memo... webkit/chromeos/fileapi/memory_file_util.cc:136: const GetFileInfoCallback& callback) { weird indentation http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/memo... webkit/chromeos/fileapi/memory_file_util.cc:194: callback.Run(base::PLATFORM_FILE_ERROR_FAILED); I think this should NOT result in error? http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/memo... webkit/chromeos/fileapi/memory_file_util.cc:262: FilePath file_path_with_separator = file_path.Append(FilePath::StringType()); This looks inconsistent with line 229-- is this to append the separator at the end? The comment in file_path says it doesn't append anything if the given component is empty. http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/memo... File webkit/chromeos/fileapi/memory_file_util.h (right): http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/memo... webkit/chromeos/fileapi/memory_file_util.h:114: // from the beginning. Please surround the param names with | |. (from -> |from|) http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/memo... webkit/chromeos/fileapi/memory_file_util.h:117: FilePath from, const FilePath& http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/memo... webkit/chromeos/fileapi/memory_file_util.h:135: void set_read_directory_buffer_size(int size) { int -> size_t ? http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/memo... File webkit/chromeos/fileapi/memory_file_util_unittest.cc (right): http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/memo... webkit/chromeos/fileapi/memory_file_util_unittest.cc:60: if (request_id > max_request_id_) { Please always omit (or use) the curly braces for single-line body. (In chrome common style seems to be omitting { }) (Or we can just use std::max as satoru suggested offline) http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/memo... webkit/chromeos/fileapi/memory_file_util_unittest.cc:236: // until the callback is called. Is this really a TODO comment or just a note for other fs implementors? http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/memo... webkit/chromeos/fileapi/memory_file_util_unittest.cc:245: file_util()->Create(FilePath("/mnt/memory/test.txt"), I prefer using (local) const variables if we refer these values repeatedly. http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/memo... webkit/chromeos/fileapi/memory_file_util_unittest.cc:371: GetOpenCallback(8)); This code would make it hard to add other test snippets in the mid of the test sequence-- can't we simply use GetNextRequestId everywhere? http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/memo... webkit/chromeos/fileapi/memory_file_util_unittest.cc:502: // /mnt/memory/longer_file_name.txt kTestStringLength Can we define these test constants somewhere and make the test code a bit more organized so that we can easily add/change test conditions? (Not really mandatory since the memory_file_util itself is for testing (for now) but would be nice to make it cleaner)
http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/asyn... File webkit/chromeos/fileapi/async_file_stream.h (right): http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/asyn... webkit/chromeos/fileapi/async_file_stream.h:6: #define WEBKIT_FILEAPI_ASYNC_FILE_STREAM_H_ On 2011/12/14 03:39:22, kinuko wrote: > nit: needs to be updated as well (to add CHROMEOS_) Done. http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/file... File webkit/chromeos/fileapi/file_util_async.cc (right): http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/file... webkit/chromeos/fileapi/file_util_async.cc:131: // ... or should if be _FAILED? On 2011/12/14 03:39:22, kinuko wrote: > Not really sure if we want this error check. (For example FileStream.Seek() > doesn't return error in this case) I'm not sure too. lseek() allows a larger offset and creates a "hole" in a file, but I'm not sure we want to follow the behavior. Added a TODO comment instead. http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/file... webkit/chromeos/fileapi/file_util_async.cc:234: base::PLATFORM_FILE_ERROR_INVALID_OPERATION, callback)); On 2011/12/14 03:39:22, kinuko wrote: > I think this should work: > Bind(callback, INVALID_OPERATION, static_cast<AsyncFileStream*>(NULL)); Good idea. Done. http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/file... webkit/chromeos/fileapi/file_util_async.cc:287: base::Unretained(this), file_path, flags, 0, callback)); On 2011/12/14 03:39:22, kinuko wrote: > need to return here? Done. http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/file... File webkit/chromeos/fileapi/file_util_async.h (right): http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/file... webkit/chromeos/fileapi/file_util_async.h:48: )> ReadDirectoryCallback; On 2011/12/14 03:39:22, kinuko wrote: > nit: weird indentation Done. http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/memo... File webkit/chromeos/fileapi/memory_file_util.cc (right): http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/memo... webkit/chromeos/fileapi/memory_file_util.cc:8: On 2011/12/14 03:39:22, kinuko wrote: > nit: extra empty line Done. http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/memo... webkit/chromeos/fileapi/memory_file_util.cc:109: const ReadWriteCallback& callback) { On 2011/12/14 03:39:22, kinuko wrote: > DCHECK(data) ? > > This code assumes the data pointer must be valid until the write ends-- maybe we > should note that in the AsyncFileUtil interface header file? Added a DCHECK. I totally agree with you that this should be documented in the header file. Let me think about this a bit more. We may be able to define better API for Read and Write. http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/memo... webkit/chromeos/fileapi/memory_file_util.cc:122: const ReadWriteCallback& callback) { On 2011/12/14 03:39:22, kinuko wrote: > DCHECK(buffer) ? Done. http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/memo... webkit/chromeos/fileapi/memory_file_util.cc:136: const GetFileInfoCallback& callback) { On 2011/12/14 03:39:22, kinuko wrote: > weird indentation Done. http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/memo... webkit/chromeos/fileapi/memory_file_util.cc:194: callback.Run(base::PLATFORM_FILE_ERROR_FAILED); On 2011/12/14 03:39:22, kinuko wrote: > I think this should NOT result in error? Good point. truncate(2) allows extending to a larger size with null bytes. Changed to do so. http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/memo... webkit/chromeos/fileapi/memory_file_util.cc:262: FilePath file_path_with_separator = file_path.Append(FilePath::StringType()); On 2011/12/14 03:39:22, kinuko wrote: > This looks inconsistent with line 229-- is this to append the separator at the > end? The comment in file_path says it doesn't append anything if the given > component is empty. Good catch. Fixed. http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/memo... File webkit/chromeos/fileapi/memory_file_util.h (right): http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/memo... webkit/chromeos/fileapi/memory_file_util.h:114: // from the beginning. On 2011/12/14 03:39:22, kinuko wrote: > Please surround the param names with | |. (from -> |from|) Done. http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/memo... webkit/chromeos/fileapi/memory_file_util.h:117: FilePath from, On 2011/12/14 03:39:22, kinuko wrote: > const FilePath& Done. http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/memo... webkit/chromeos/fileapi/memory_file_util.h:135: void set_read_directory_buffer_size(int size) { On 2011/12/14 03:39:22, kinuko wrote: > int -> size_t ? Done. http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/memo... File webkit/chromeos/fileapi/memory_file_util_unittest.cc (right): http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/memo... webkit/chromeos/fileapi/memory_file_util_unittest.cc:60: if (request_id > max_request_id_) { On 2011/12/14 03:39:22, kinuko wrote: > Please always omit (or use) the curly braces for single-line body. > (In chrome common style seems to be omitting { }) > > (Or we can just use std::max as satoru suggested offline) Done. http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/memo... webkit/chromeos/fileapi/memory_file_util_unittest.cc:236: // until the callback is called. On 2011/12/14 03:39:22, kinuko wrote: > Is this really a TODO comment or just a note for other fs implementors? Looks like a note. Removed TODO, and added some more comment. http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/memo... webkit/chromeos/fileapi/memory_file_util_unittest.cc:245: file_util()->Create(FilePath("/mnt/memory/test.txt"), On 2011/12/14 03:39:22, kinuko wrote: > I prefer using (local) const variables if we refer these values repeatedly. Let's keep it as is. file_util() is inlined anyway. http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/memo... webkit/chromeos/fileapi/memory_file_util_unittest.cc:371: GetOpenCallback(8)); On 2011/12/14 03:39:22, kinuko wrote: > This code would make it hard to add other test snippets in the mid of the test > sequence-- can't we simply use GetNextRequestId everywhere? Done. http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/memo... webkit/chromeos/fileapi/memory_file_util_unittest.cc:502: // /mnt/memory/longer_file_name.txt kTestStringLength On 2011/12/14 03:39:22, kinuko wrote: > Can we define these test constants somewhere and make the test code a bit more > organized so that we can easily add/change test conditions? > (Not really mandatory since the memory_file_util itself is for testing (for now) > but would be nice to make it cleaner) I thought about it but constants may make the code less readable as we cannot use / in file names. Let's revisit this when needed.
http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/memo... File webkit/chromeos/fileapi/memory_file_util_unittest.cc (right): http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/memo... webkit/chromeos/fileapi/memory_file_util_unittest.cc:502: // /mnt/memory/longer_file_name.txt kTestStringLength On 2011/12/14 04:43:26, satorux1 wrote: > On 2011/12/14 03:39:22, kinuko wrote: > > Can we define these test constants somewhere and make the test code a bit more > > organized so that we can easily add/change test conditions? > > (Not really mandatory since the memory_file_util itself is for testing (for > now) > > but would be nice to make it cleaner) > > I thought about it but constants may make the code less readable as we cannot > use / in file names. Let's revisit this when needed. I meant variable names.
Had discussion with kinuko about the design. http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/asyn... File webkit/chromeos/fileapi/async_file_stream.h (right): http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/asyn... webkit/chromeos/fileapi/async_file_stream.h:26: const ReadWriteCallback& callback) = 0; The API looks error-prone. The caller needs to maintain |buffer| to remain valid until |callback| is called. FileStream in 'net' is designed this way, as it's used both synchronously and asynchronously. Here, we only need to support asynchronous reads, so we could provide a better API for this. Let me give it a try. http://codereview.chromium.org/8907014/diff/8002/webkit/chromeos/fileapi/file... File webkit/chromeos/fileapi/file_util_async.h (right): http://codereview.chromium.org/8907014/diff/8002/webkit/chromeos/fileapi/file... webkit/chromeos/fileapi/file_util_async.h:28: // operation when it is possible to batch several checks into one query. Discussed this with Kinuko. She suggested providing three options here was not a good idea, and I agreed per the philosophy of YAGNI (see wikipedia). Providing options here will bloat the code, and make it hard to maintain and test code. We can add options at a later time when really needed. Let me simplify the code a bit. http://codereview.chromium.org/8907014/diff/8002/webkit/chromeos/fileapi/file... webkit/chromeos/fileapi/file_util_async.h:119: } Discussed this with kinuko. Providing Read() and Write() here looks wrong. These operations can be done using AsyncFileStream object. Providing two ways to do the same thing is not great for the same reason described above.
http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/asyn... File webkit/chromeos/fileapi/async_file_stream.h (right): http://codereview.chromium.org/8907014/diff/4002/webkit/chromeos/fileapi/asyn... webkit/chromeos/fileapi/async_file_stream.h:26: const ReadWriteCallback& callback) = 0; On 2011/12/14 05:19:55, satorux1 wrote: > The API looks error-prone. The caller needs to maintain |buffer| to remain valid > until |callback| is called. FileStream in 'net' is designed this way, as it's > used both synchronously and asynchronously. Here, we only need to support > asynchronous reads, so we could provide a better API for this. Let me give it a > try. I take it back. We could design an API that does not require the caller to keep the pointer valid, but it'll likely require an extra data copy, which is not desirable here.
Kinuko, per our discussion, I've removed Read() and Write() from FileUtilAsync, as well as FileUtilAsyncFileStream class. Now the code is 200 lines shorter. The test still passes.
http://codereview.chromium.org/8907014/diff/14001/webkit/chromeos/fileapi/fil... File webkit/chromeos/fileapi/file_util_async.h (right): http://codereview.chromium.org/8907014/diff/14001/webkit/chromeos/fileapi/fil... webkit/chromeos/fileapi/file_util_async.h:62: bool recursive, Use of boolean parameter like this is discouraged per chromium coding style, as it makes callers less readable: http://www.chromium.org/developers/coding-style#Code_Formatting
http://codereview.chromium.org/8907014/diff/14001/webkit/chromeos/fileapi/fil... File webkit/chromeos/fileapi/file_util_async.cc (right): http://codereview.chromium.org/8907014/diff/14001/webkit/chromeos/fileapi/fil... webkit/chromeos/fileapi/file_util_async.cc:48: void FileUtilAsync::Open( She was doubtful of having the default implementation here. I think she's right. The implementation assumes that the logic is reusable for file system implementations but the assumption doesn't seem to be sound.
Also added a bunch of comments. The test still passes. http://codereview.chromium.org/8907014/diff/14001/webkit/chromeos/fileapi/fil... File webkit/chromeos/fileapi/file_util_async.cc (right): http://codereview.chromium.org/8907014/diff/14001/webkit/chromeos/fileapi/fil... webkit/chromeos/fileapi/file_util_async.cc:48: void FileUtilAsync::Open( On 2011/12/14 08:42:29, satorux1 wrote: > She was doubtful of having the default implementation here. I think she's right. > The implementation assumes that the logic is reusable for file system > implementations but the assumption doesn't seem to be sound. Moved to MemoryFileUtil. http://codereview.chromium.org/8907014/diff/14001/webkit/chromeos/fileapi/fil... File webkit/chromeos/fileapi/file_util_async.h (right): http://codereview.chromium.org/8907014/diff/14001/webkit/chromeos/fileapi/fil... webkit/chromeos/fileapi/file_util_async.h:62: bool recursive, On 2011/12/14 08:27:03, satorux1 wrote: > Use of boolean parameter like this is discouraged per chromium coding style, as > it makes callers less readable: > > http://www.chromium.org/developers/coding-style#Code_Formatting Introduced RemoveMode.
I like this no default impl design! (We could bring some of the code back if we find ourselves writing the same code again) This almost looks good to me mod a few more nits. http://codereview.chromium.org/8907014/diff/13003/webkit/chromeos/fileapi/asy... File webkit/chromeos/fileapi/async_file_stream.h (right): http://codereview.chromium.org/8907014/diff/13003/webkit/chromeos/fileapi/asy... webkit/chromeos/fileapi/async_file_stream.h:15: // This class an interface used for implementing OpenVerifiedFile() in This class *is* an interface ... ? http://codereview.chromium.org/8907014/diff/13003/webkit/chromeos/fileapi/fil... File webkit/chromeos/fileapi/file_util_async.h (right): http://codereview.chromium.org/8907014/diff/13003/webkit/chromeos/fileapi/fil... webkit/chromeos/fileapi/file_util_async.h:92: // option is set, remove the contents of the given directory nit: remove -> removes http://codereview.chromium.org/8907014/diff/13003/webkit/chromeos/fileapi/mem... File webkit/chromeos/fileapi/memory_file_util.cc (right): http://codereview.chromium.org/8907014/diff/13003/webkit/chromeos/fileapi/mem... webkit/chromeos/fileapi/memory_file_util.cc:18: // |file_entry| is owened by MemoryFileUtil. nit: s/owened/owned/ http://codereview.chromium.org/8907014/diff/13003/webkit/chromeos/fileapi/mem... webkit/chromeos/fileapi/memory_file_util.cc:20: MemoryFileUtil::FileEntry* file_entry, int flags) style-nit: MemoryFileUtilAsyncFileStream(MemoryFileUtil::FileEntry* file_entry, int flags) http://codereview.chromium.org/8907014/diff/13003/webkit/chromeos/fileapi/mem... webkit/chromeos/fileapi/memory_file_util.cc:29: const ReadWriteCallback& callback) OVERRIDE { Just to make sure, we're dispatching callbacks synchronously in Stream while we do asynchronously in AsyncFileUtil. If we really want to test the async nature of these interfaces we may want to run them in async tasks. http://codereview.chromium.org/8907014/diff/13003/webkit/chromeos/fileapi/mem... webkit/chromeos/fileapi/memory_file_util.cc:236: if (mode == RECURSIVE) { changing this into switch/case and having default with NOTREACHED check might be better? http://codereview.chromium.org/8907014/diff/13003/webkit/chromeos/fileapi/mem... webkit/chromeos/fileapi/memory_file_util.cc:364: FilePath path_with_separator = file_it->first.Append("/"); Does this (path.Append("/")) work? This looks strange to me since Append() is basically for appending path components without worrying about separator itself. (You'll need to call AppendASCII() or Append(FILE_PATH_LITERAL("/")) anyway on Windows) I think we should explicitly append FILE_PATH_LITERAL("/") to the path.value() if we really want to do that manually. (maybe we could make it an inline method and call it like EnsureEndsWithSeparator()) http://codereview.chromium.org/8907014/diff/13003/webkit/chromeos/fileapi/mem... File webkit/chromeos/fileapi/memory_file_util.h (right): http://codereview.chromium.org/8907014/diff/13003/webkit/chromeos/fileapi/mem... webkit/chromeos/fileapi/memory_file_util.h:64: bool FileExists(const FilePath& file_path) { could this method be const? http://codereview.chromium.org/8907014/diff/13003/webkit/chromeos/fileapi/mem... webkit/chromeos/fileapi/memory_file_util.h:69: bool IsDirectory(const FilePath& file_path) { const?
http://codereview.chromium.org/8907014/diff/8002/webkit/chromeos/fileapi/file... File webkit/chromeos/fileapi/file_util_async.h (right): http://codereview.chromium.org/8907014/diff/8002/webkit/chromeos/fileapi/file... webkit/chromeos/fileapi/file_util_async.h:28: // operation when it is possible to batch several checks into one query. On 2011/12/14 05:19:55, satorux1 wrote: > Discussed this with Kinuko. She suggested providing three options here was not a > good idea, and I agreed per the philosophy of YAGNI (see wikipedia). Providing > options here will bloat the code, and make it hard to maintain and test code. We > can add options at a later time when really needed. Let me simplify the code a > bit. In that case I suggest to move OpenVerifiedFile from protected to private and make Open non-virtual. I suggest leaving only stateless protected Read & Write to be overridden by the implementations (as it is done in MemoryFileUtil). http://codereview.chromium.org/8907014/diff/8002/webkit/chromeos/fileapi/file... webkit/chromeos/fileapi/file_util_async.h:119: } On 2011/12/14 05:19:55, satorux1 wrote: > Discussed this with kinuko. Providing Read() and Write() here looks wrong. These > operations can be done using AsyncFileStream object. Providing two ways to do > the same thing is not great for the same reason described above. These Read() and Write() allow the implementation not to create their versions of AsyncFileStream, but to perform these operations statelesly. It allows reusing quite a bit of code (AsyncFileStream takes more than 150 lines). http://codereview.chromium.org/8907014/diff/14001/webkit/chromeos/fileapi/fil... File webkit/chromeos/fileapi/file_util_async.cc (right): http://codereview.chromium.org/8907014/diff/14001/webkit/chromeos/fileapi/fil... webkit/chromeos/fileapi/file_util_async.cc:48: void FileUtilAsync::Open( On 2011/12/14 08:55:32, satorux1 wrote: > On 2011/12/14 08:42:29, satorux1 wrote: > > She was doubtful of having the default implementation here. I think she's > right. > > The implementation assumes that the logic is reusable for file system > > implementations but the assumption doesn't seem to be sound. > > Moved to MemoryFileUtil. To be honest, I'm not sure about this. Having the Open() code in the base class guarantees that different implementations will behave in the same way with regard to open modes. Also much the same code should be present in almost any implementation of FileUtilAsync, so moving it to just one instance looks wasteful to me. On the other hand, it's never too late to move this code back once it proves to be useful in another FU implementation. Plainly, I'm too anxious to commit this code ASAP to argue about about the best place for this code. :) http://codereview.chromium.org/8907014/diff/14001/webkit/chromeos/fileapi/fil... File webkit/chromeos/fileapi/file_util_async.h (right): http://codereview.chromium.org/8907014/diff/14001/webkit/chromeos/fileapi/fil... webkit/chromeos/fileapi/file_util_async.h:62: bool recursive, On 2011/12/14 08:27:03, satorux1 wrote: > Use of boolean parameter like this is discouraged per chromium coding style, as > it makes callers less readable: > > http://www.chromium.org/developers/coding-style#Code_Formatting Just want to note that boolean here is consistent with base::file_util, FileSystemOperation and FileSystemFileUtil. I'd prefer to leave it boolean. http://codereview.chromium.org/8907014/diff/13003/webkit/chromeos/fileapi/mem... File webkit/chromeos/fileapi/memory_file_util.cc (right): http://codereview.chromium.org/8907014/diff/13003/webkit/chromeos/fileapi/mem... webkit/chromeos/fileapi/memory_file_util.cc:29: const ReadWriteCallback& callback) OVERRIDE { On 2011/12/14 11:19:13, kinuko wrote: > Just to make sure, we're dispatching callbacks synchronously in Stream while we > do asynchronously in AsyncFileUtil. If we really want to test the async nature > of these interfaces we may want to run them in async tasks. I believe the callback must be called _after_ the call itself, so these callback.Run's looks wrong to me. They should probably be replaced by MessageLoop::current()->PostTask().
http://codereview.chromium.org/8907014/diff/8002/webkit/chromeos/fileapi/file... File webkit/chromeos/fileapi/file_util_async.h (right): http://codereview.chromium.org/8907014/diff/8002/webkit/chromeos/fileapi/file... webkit/chromeos/fileapi/file_util_async.h:28: // operation when it is possible to batch several checks into one query. On 2011/12/14 15:20:59, Oleg wrote: > On 2011/12/14 05:19:55, satorux1 wrote: > > Discussed this with Kinuko. She suggested providing three options here was not > a > > good idea, and I agreed per the philosophy of YAGNI (see wikipedia). Providing > > options here will bloat the code, and make it hard to maintain and test code. > We > > can add options at a later time when really needed. Let me simplify the code a > > bit. > > In that case I suggest to move OpenVerifiedFile from protected to private and > make Open non-virtual. I suggest leaving only stateless protected Read & Write > to be overridden by the implementations (as it is done in MemoryFileUtil). Kinuko and I found it awkward to implement Read() and Write() in the class. That's why we chose to remove them. See below. http://codereview.chromium.org/8907014/diff/8002/webkit/chromeos/fileapi/file... webkit/chromeos/fileapi/file_util_async.h:119: } On 2011/12/14 15:20:59, Oleg wrote: > On 2011/12/14 05:19:55, satorux1 wrote: > > Discussed this with kinuko. Providing Read() and Write() here looks wrong. > These > > operations can be done using AsyncFileStream object. Providing two ways to do > > the same thing is not great for the same reason described above. > > These Read() and Write() allow the implementation not to create their versions > of AsyncFileStream, but to perform these operations statelesly. It allows > reusing quite a bit of code (AsyncFileStream takes more than 150 lines). Read and write operations are inherently associated with file streams. Implementing them statelessly here looked awkward. Besides, Kinuko and I agreed that interaction between FileUtilAsyncFileStream and FileUtilAsync in the original patch was the most convoluted part in the orignal patch, and we were able to remove FileUtilAsyncFileStream by removing Read() and Write() from this class.
http://codereview.chromium.org/8907014/diff/13003/webkit/chromeos/fileapi/asy... File webkit/chromeos/fileapi/async_file_stream.h (right): http://codereview.chromium.org/8907014/diff/13003/webkit/chromeos/fileapi/asy... webkit/chromeos/fileapi/async_file_stream.h:15: // This class an interface used for implementing OpenVerifiedFile() in On 2011/12/14 11:19:13, kinuko wrote: > This class *is* an interface ... ? Done. http://codereview.chromium.org/8907014/diff/13003/webkit/chromeos/fileapi/fil... File webkit/chromeos/fileapi/file_util_async.h (right): http://codereview.chromium.org/8907014/diff/13003/webkit/chromeos/fileapi/fil... webkit/chromeos/fileapi/file_util_async.h:92: // option is set, remove the contents of the given directory On 2011/12/14 11:19:13, kinuko wrote: > nit: remove -> removes Done. http://codereview.chromium.org/8907014/diff/13003/webkit/chromeos/fileapi/mem... File webkit/chromeos/fileapi/memory_file_util.cc (right): http://codereview.chromium.org/8907014/diff/13003/webkit/chromeos/fileapi/mem... webkit/chromeos/fileapi/memory_file_util.cc:18: // |file_entry| is owened by MemoryFileUtil. On 2011/12/14 11:19:13, kinuko wrote: > nit: s/owened/owned/ Done. http://codereview.chromium.org/8907014/diff/13003/webkit/chromeos/fileapi/mem... webkit/chromeos/fileapi/memory_file_util.cc:20: MemoryFileUtil::FileEntry* file_entry, int flags) On 2011/12/14 11:19:13, kinuko wrote: > style-nit: > > MemoryFileUtilAsyncFileStream(MemoryFileUtil::FileEntry* file_entry, > int flags) Done. http://codereview.chromium.org/8907014/diff/13003/webkit/chromeos/fileapi/mem... webkit/chromeos/fileapi/memory_file_util.cc:29: const ReadWriteCallback& callback) OVERRIDE { On 2011/12/14 15:20:59, Oleg wrote: > On 2011/12/14 11:19:13, kinuko wrote: > > Just to make sure, we're dispatching callbacks synchronously in Stream while > we > > do asynchronously in AsyncFileUtil. If we really want to test the async > nature > > of these interfaces we may want to run them in async tasks. > > I believe the callback must be called _after_ the call itself, so these > callback.Run's looks wrong to me. They should probably be replaced by > MessageLoop::current()->PostTask(). Done. http://codereview.chromium.org/8907014/diff/13003/webkit/chromeos/fileapi/mem... File webkit/chromeos/fileapi/memory_file_util.h (right): http://codereview.chromium.org/8907014/diff/13003/webkit/chromeos/fileapi/mem... webkit/chromeos/fileapi/memory_file_util.h:64: bool FileExists(const FilePath& file_path) { On 2011/12/14 11:19:13, kinuko wrote: > could this method be const? Done. http://codereview.chromium.org/8907014/diff/13003/webkit/chromeos/fileapi/mem... webkit/chromeos/fileapi/memory_file_util.h:69: bool IsDirectory(const FilePath& file_path) { On 2011/12/14 11:19:13, kinuko wrote: > const? Done.
http://codereview.chromium.org/8907014/diff/13003/webkit/chromeos/fileapi/mem... File webkit/chromeos/fileapi/memory_file_util.cc (right): http://codereview.chromium.org/8907014/diff/13003/webkit/chromeos/fileapi/mem... webkit/chromeos/fileapi/memory_file_util.cc:236: if (mode == RECURSIVE) { On 2011/12/14 11:19:13, kinuko wrote: > changing this into switch/case and having default with NOTREACHED check might be > better? Done. http://codereview.chromium.org/8907014/diff/13003/webkit/chromeos/fileapi/mem... webkit/chromeos/fileapi/memory_file_util.cc:364: FilePath path_with_separator = file_it->first.Append("/"); On 2011/12/14 11:19:13, kinuko wrote: > Does this (path.Append("/")) work? > This looks strange to me since Append() is basically for appending path > components without worrying about separator itself. > (You'll need to call AppendASCII() or Append(FILE_PATH_LITERAL("/")) anyway on > Windows) > > I think we should explicitly append FILE_PATH_LITERAL("/") to the path.value() > if we really want to do that manually. > (maybe we could make it an inline method and call it like > EnsureEndsWithSeparator()) My bad, you are absolutely right. Besides, relying on '/' is not desirable as Windows can use both / and \ as path separators. Removed code to rely on '/'.
LGTM On 2011/12/15 07:04:31, satorux1 wrote: > http://codereview.chromium.org/8907014/diff/13003/webkit/chromeos/fileapi/mem... > File webkit/chromeos/fileapi/memory_file_util.cc (right): > > http://codereview.chromium.org/8907014/diff/13003/webkit/chromeos/fileapi/mem... > webkit/chromeos/fileapi/memory_file_util.cc:236: if (mode == RECURSIVE) { > On 2011/12/14 11:19:13, kinuko wrote: > > changing this into switch/case and having default with NOTREACHED check might > be > > better? > > Done. > > http://codereview.chromium.org/8907014/diff/13003/webkit/chromeos/fileapi/mem... > webkit/chromeos/fileapi/memory_file_util.cc:364: FilePath path_with_separator = > file_it->first.Append("/"); > On 2011/12/14 11:19:13, kinuko wrote: > > Does this (path.Append("/")) work? > > This looks strange to me since Append() is basically for appending path > > components without worrying about separator itself. > > (You'll need to call AppendASCII() or Append(FILE_PATH_LITERAL("/")) anyway on > > Windows) > > > > I think we should explicitly append FILE_PATH_LITERAL("/") to the path.value() > > if we really want to do that manually. > > (maybe we could make it an inline method and call it like > > EnsureEndsWithSeparator()) > > My bad, you are absolutely right. Besides, relying on '/' is not desirable as > Windows can use both / and \ as path separators. Removed code to rely on '/'.
Changed RemoveMode back to bool per Oleg's strong request out of band...
On 2011/12/15 14:30:55, satorux1 wrote: > Changed RemoveMode back to bool per Oleg's strong request out of band... LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/satorux@chromium.org/8907014/34001
Presubmit check for 8907014-34001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit ERRORS ** Missing LGTM from an OWNER for: webkit/tools/test_shell/test_shell.gypi,webkit/chromeos/fileapi/file_util_async.h,webkit/chromeos/fileapi/memory_file_util.cc,webkit/chromeos/fileapi/memory_file_util_unittest.cc,webkit/chromeos/fileapi/async_file_stream.h,webkit/chromeos/fileapi/memory_file_util.h Presubmit checks took 1.7s to calculate.
Hmm, I seem to need LGTM from darin or tony. I've just added webkit/chromeos/OWNERS.
LGTM http://codereview.chromium.org/8907014/diff/28013/webkit/chromeos/fileapi/asy... File webkit/chromeos/fileapi/async_file_stream.h (right): http://codereview.chromium.org/8907014/diff/28013/webkit/chromeos/fileapi/asy... webkit/chromeos/fileapi/async_file_stream.h:9: #include "base/callback.h" Nit: Sort includes. http://codereview.chromium.org/8907014/diff/28013/webkit/fileapi/webkit_filea... File webkit/fileapi/webkit_fileapi.gypi (right): http://codereview.chromium.org/8907014/diff/28013/webkit/fileapi/webkit_filea... webkit/fileapi/webkit_fileapi.gypi:76: '../chromeos/fileapi/file_util_async.h', Nit: Sort these.
http://codereview.chromium.org/8907014/diff/28013/webkit/chromeos/fileapi/asy... File webkit/chromeos/fileapi/async_file_stream.h (right): http://codereview.chromium.org/8907014/diff/28013/webkit/chromeos/fileapi/asy... webkit/chromeos/fileapi/async_file_stream.h:9: #include "base/callback.h" On 2011/12/16 18:13:25, tony wrote: > Nit: Sort includes. Done. http://codereview.chromium.org/8907014/diff/28013/webkit/fileapi/webkit_filea... File webkit/fileapi/webkit_fileapi.gypi (right): http://codereview.chromium.org/8907014/diff/28013/webkit/fileapi/webkit_filea... webkit/fileapi/webkit_fileapi.gypi:76: '../chromeos/fileapi/file_util_async.h', On 2011/12/16 18:13:25, tony wrote: > Nit: Sort these. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/satorux@chromium.org/8907014/35001
Change committed as 114923 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
