[webkit-reviews] review denied: [Bug 36938] Add basic FileSystem operations for FileReader/FileWriter support : [Attachment 52460] Patch

[bugzilla-daemon at webkit.org]

Jian Li <jianli at chromium.org> has denied Kinuko Yasuda <kinuko at chromium.org>'s
request for review:
Bug 36938: Add basic FileSystem operations for FileReader/FileWriter support
https://bugs.webkit.org/show_bug.cgi?id=36938

Attachment 52460: Patch
https://bugs.webkit.org/attachment.cgi?id=52460&action=review

------- Additional Comments from Jian Li <jianli at chromium.org>
> --- a/WebCore/platform/FileSystem.h
> +++ b/WebCore/platform/FileSystem.h
> @@ -141,8 +152,12 @@ inline bool isHandleValid(const PlatformFileHandle&
handle) { return handle != i
>  
>  // Prefix is what the filename should be prefixed with, not the full path.
>  WTF::CString openTemporaryFile(const char* prefix, PlatformFileHandle&);
> +PlatformFileHandle openFile(const String& path, FileOpenMode);
>  void closeFile(PlatformFileHandle&);
> +long long seekFile(PlatformFileHandle, long long offset, FileSeekOrigin);
> +bool truncateFile(PlatformFileHandle, long long offset);
>  int writeToFile(PlatformFileHandle, const char* data, int length);
> +int readFromFile(PlatformFileHandle, char* data, int length);
Better to add comment saying that return value less than 0 indicates an error.
>  
>  // Methods for dealing with loadable modules
>  bool unloadModule(PlatformModule);
> diff --git a/WebCore/platform/posix/FileSystemPOSIX.cpp
b/WebCore/platform/posix/FileSystemPOSIX.cpp
> index d6804fb..57a36b8 100644
> --- a/WebCore/platform/posix/FileSystemPOSIX.cpp
> +++ b/WebCore/platform/posix/FileSystemPOSIX.cpp
> +void closeFile(PlatformFileHandle& handle)
> +{
> +    close(handle);
Since handle parameter is passed by reference, you need to set it to
invalidPlatformFileHandle if closing is successful, like the following:
    if (isHandleValid(handle)) {
	close(handle);
	handle = invalidPlatformFileHandle;
    }
> +}
> +
> +long long seekFile(PlatformFileHandle handle, long long offset,
FileSeekOrigin origin)
> +{
> +    int whence = SEEK_SET;
> +    switch (origin) {
> +    case SeekFromBeginning: whence = SEEK_SET; break;
Per WebKit coding style, each statement should get its own line.
> +    case SeekFromCurrent: whence = SEEK_CUR; break;
ditto.
> +    case SeekFromEnd: whence = SEEK_END; break;
ditto.
> +    default: ASSERT_NOT_REACHED();
ditto.
> +    }
> +    return static_cast<long long>(lseek(handle, offset, whence));
> +}
> +
> +bool truncateFile(PlatformFileHandle handle, long long offset)
> +{
> +    if (ftruncate(handle, offset) < 0)
> +	   return false;
> +    return true;
Simpler to say:
       return ftruncate(handle, offset) == 0;
> +}
> +
> +int writeToFile(PlatformFileHandle handle, const char* data, int length)
> +{
> +    int totalBytesWritten = 0;
> +    while (totalBytesWritten < length) {
> +	   int bytesWritten = HANDLE_EINTR(write(handle, data +
totalBytesWritten, length - totalBytesWritten));
Using HANDLE_EINTR might not be necessary and it causes the complexity to
understand the code.
How about something like the following:
	   int bytesWritten = write(handle, data + totalBytesWritten,
static_cast<size_t>(length - totalBytesWritten));
	   if (bytesWritten < 0 && errno != EINTR)
	       return -1;
In addition, better to do static_cast to size_t from int.
> +	   if (bytesWritten < 0)
> +	       return -1;
> +	   totalBytesWritten += bytesWritten;
> +    }
> +
> +    return totalBytesWritten;
> +}
> +
> +int readFromFile(PlatformFileHandle handle, char* data, int length)
> +{
> +    int totalBytesRead = 0;
> +    while (totalBytesRead < length) {
> +	   int bytesRead = HANDLE_EINTR(read(handle, data + totalBytesRead,
length - totalBytesRead));
ditto.
> +	   if (bytesRead <= 0)
> +	       break;
> +	   totalBytesRead += bytesRead;
> +    }
> +
> +    return totalBytesRead;
> +}
> +
>  bool deleteEmptyDirectory(const String& path)
>  {
>      CString fsRep = fileSystemRepresentation(path);