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

------- Additional Comments from Jian Li <jianli at chromium.org>
Noticed another problem in read file logic plus a couple minor comment
suggestions. Almost there.

> +// Returns seeked offset if successful, -1 otherwise.
Probably need to rephrase a little bit, like:
    // Returns the resulting offset from the beginning of the file if
successful, -1 otherwise.
> +long long seekFile(PlatformFileHandle, long long offset, FileSeekOrigin);

> +bool truncateFile(PlatformFileHandle handle, long long offset)
> +{
> +    // ftruncate return 0 to indicate the success.
r/return/returns
> +    return !ftruncate(handle, offset);
> +}
> +

> +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 we encounter non-EINTR error and break out of the loop, we will return
totalBytesRead that is not -1.
In addition, we should not continue the reading if bytesRead >= 0.
I think the block can be reorganized like the following:
	   if (bytesRead >= 0) {
	       totalBytesRead += bytesRead;
	       break;
	   }
	   if (errno != EINTR)
	       return -1;

writeToFile could also do the similar thing for better readerability.

> +	   if (bytesRead > 0)
> +	       totalBytesRead += bytesRead;
> +    }
> +
> +    return totalBytesRead;
> +}