[webkit-reviews] review denied: [Bug 36938] Add basic FileSystem operations for FileReader/FileWriter support : [Attachment 52666] 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 52666: Patch
https://bugs.webkit.org/attachment.cgi?id=52666&action=review

------- Additional Comments from Jian Li <jianli at chromium.org>
> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index fe8d9a4..dfe25d7 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,22 @@
> +2010-04-06  Kinuko Yasuda  <kinuko at chromium.org>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Add basic FileSystem operations for FileReader/FileWriter support
> +	   https://bugs.webkit.org/show_bug.cgi?id=36938
> +
> +	   No new tests; tests will be added after we implement upper layers.
> +
> +	   * platform/FileSystem.h:
> +	   (WebCore::):
> +	   * platform/posix/FileSystemPOSIX.cpp:
> +	   (WebCore::openFile):
> +	   (WebCore::closeFile):
> +	   (WebCore::seekFile):
> +	   (WebCore::truncateFile):
> +	   (WebCore::writeToFile):
> +	   (WebCore::readFromFile):
> +
>  2010-04-02  Laszlo Gombos  <laszlo.1.gombos at nokia.com>
>  
>	   Unreviewed build fix when building --no-svg.
> diff --git a/WebCore/platform/FileSystem.h b/WebCore/platform/FileSystem.h
> index 80023d4..663458f 100644
> --- a/WebCore/platform/FileSystem.h
> +++ b/WebCore/platform/FileSystem.h
> @@ -26,7 +26,7 @@
>   * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
>   * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>   */
> - 
> +
>  #ifndef FileSystem_h
>  #define FileSystem_h
>  
> @@ -122,6 +122,17 @@ typedef int PlatformFileHandle;
>  const PlatformFileHandle invalidPlatformFileHandle = -1;
>  #endif
>  
> +enum FileOpenMode {
> +    OpenForRead = 0,
> +    OpenForWrite
> +};
> +
> +enum FileSeekOrigin {
> +    SeekFromBeginning = 0,
> +    SeekFromCurrent,
> +    SeekFromEnd
> +};
> +
>  bool fileExists(const String&);
>  bool deleteFile(const String&);
>  bool deleteEmptyDirectory(const String&);
> @@ -141,8 +152,15 @@ 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&);
> +// Returns seeked offset if successful, -1 otherwise.
> +long long seekFile(PlatformFileHandle, long long offset, FileSeekOrigin);
> +bool truncateFile(PlatformFileHandle, long long offset);
> +// Returns number of bytes actually read if successful, -1 otherwise.
>  int writeToFile(PlatformFileHandle, const char* data, int length);
> +// Returns number of bytes actually written if successful, -1 otherwise.
> +int readFromFile(PlatformFileHandle, char* data, int length);
>  
>  // 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..5427948 100644
> --- a/WebCore/platform/posix/FileSystemPOSIX.cpp
> +++ b/WebCore/platform/posix/FileSystemPOSIX.cpp
> @@ -30,11 +30,11 @@
>  #include "FileSystem.h"
>  
>  #include "PlatformString.h"
> -#include <wtf/text/CString.h>
> -
> -#include <sys/stat.h>
> +#include <errno.h>
>  #include <libgen.h>
> +#include <sys/stat.h>
>  #include <unistd.h>
> +#include <wtf/text/CString.h>
>  
>  namespace WebCore {
>  
> @@ -65,6 +65,77 @@ bool deleteFile(const String& path)
>      return !unlink(fsRep.data());
>  }
>  
> +PlatformFileHandle openFile(const String& path, FileOpenMode mode)
> +{
> +    int platformFlag = 0;
> +    if (mode == OpenForRead)
> +	   platformFlag |= O_RDONLY;
> +    else if (mode == OpenForWrite)
> +	   platformFlag |= (O_WRONLY | O_CREAT | O_TRUNC);
> +    return open(path.utf8().data(), platformFlag, 0666);
> +}
> +
> +void closeFile(PlatformFileHandle& handle)
> +{
> +    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;
> +    case SeekFromCurrent:
> +	   whence = SEEK_CUR;
> +	   break;
> +    case SeekFromEnd:
> +	   whence = SEEK_END;
> +	   break;
> +    default:
> +	   ASSERT_NOT_REACHED();
> +    }
> +    return static_cast<long long>(lseek(handle, offset, whence));
> +}
> +
> +bool truncateFile(PlatformFileHandle handle, long long offset)
Why do we need to introduce truncateFile? I am not seeing any reference in your
FileWriter patch.
> +{
> +    // A return value 0 indicates the call succeeds.
The comment is a little bit confusing because it might also mean the return
value of truncateFile. How about something like: 
       // ftruncate returns 0 to indicates the success.
> +    return !ftruncate(handle, offset);
> +}
> +
> +int writeToFile(PlatformFileHandle handle, const char* data, int length)
> +{
> +    int totalBytesWritten = 0;
> +    while (totalBytesWritten < length) {
> +	   int bytesWritten = write(handle, data + totalBytesWritten,
static_cast<size_t>(length - totalBytesWritten));
> +	   if (bytesWritten < 0 && errno != EINTR)
> +	       return -1;
> +	   if (errno != EINTR)
It would be safe to say:
	   if (bytesWritten > 0)
> +	       totalBytesWritten += bytesWritten;
> +    }
> +
> +    return totalBytesWritten;
> +}
> +
> +int readFromFile(PlatformFileHandle handle, char* data, int length)
> +{
> +    int totalBytesRead = 0;
> +    while (totalBytesRead < length) {
> +	   int bytesRead = read(handle, data + totalBytesRead,
static_cast<size_t>(length - totalBytesRead));
> +	   if (bytesRead <= 0 && errno != EINTR)
> +	       break;
> +	   if (errno != EINTR)
ditto.
> +	       totalBytesRead += bytesRead;
> +    }
> +
> +    return totalBytesRead;
> +}
> +
>  bool deleteEmptyDirectory(const String& path)
>  {
>      CString fsRep = fileSystemRepresentation(path);