[webkit-reviews] review granted: [Bug 37222] [WINCE] Port SharedBuffer : [Attachment 55002] Alternative patch

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has granted Patrick R. Gansterer
<paroga at paroga.com>'s request for review:
Bug 37222: [WINCE] Port SharedBuffer
https://bugs.webkit.org/show_bug.cgi?id=37222

Attachment 55002: Alternative patch
https://bugs.webkit.org/attachment.cgi?id=55002&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
> -    FILE* fileDescriptor = 0;
> -    if (_wfopen_s(&fileDescriptor,
nullifiedPath.charactersWithNullTermination(), TEXT("rb")) || !fileDescriptor)
{
> -	   LOG_ERROR("Failed to open file %s to create shared buffer,
errno(%i)", filePath.ascii().data(), errno);
> +    HANDLE fileHandle =
CreateFileW(nullifiedPath.charactersWithNullTermination(), GENERIC_READ,
FILE_SHARE_READ, 0, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, 0);
> +    if (fileHandle == INVALID_HANDLE_VALUE) {
> +	   LOG_ERROR("Failed to open file %s to create shared buffer,
errno(%u)", filePath.ascii().data(), GetLastError());
>	   return 0;
>      }

The "errno" in the log message isn't really accurate anymore. Maybe you could
change it to "GetLastError() = %u"

> +    do {
> +	   DWORD bytesRead;
> +	   DWORD bytesToRead;

No need to declare these here. You can just declare them where you first use
them.

> +	   bytesToRead = GetFileSize(fileHandle, 0);
> +	   if (bytesToRead == 0xffffffff)
> +	       break;

I think it would be nicer to put 0xffffffff in a constant. Apparently on non-CE
versions of Windows this is called INVALID_FILE_SIZE. So maybe a static const
DWORD invalidFileSize constant would be best.

MSDN says that when 0xffffffff is returned you need to check that
GetLastError() is not NO_ERROR before assuming that an error has occurred.
Otherwise the file size might actually be 0xffffffff. (Of course, the rest of
the code might not be able to handle that situation.)

It would be nice to use GetFileSizeEx on non-CE Windows. Maybe we should have a
wrapper function that calls GetFileSize on CE and GetFileSizeEx on everything
else?

> +	   result = SharedBuffer::create();
> +	   result->m_buffer.resize(bytesToRead);
> +	   if (result->m_buffer.size() != bytesToRead) {
> +	       result = 0;
> +	       break;
> +	   }

I don't think it's possible for Vector::resize to fail in this way.

> +
> +	   result->m_size = result->m_buffer.size();
> +
> +	   if (!ReadFile(fileHandle, result->m_buffer.data(), bytesToRead,
&bytesRead, 0) || bytesToRead != bytesRead)
> +	       LOG_ERROR("Failed to fully read contents of file %s -
errno(%u)", filePath.ascii().data(), GetLastError());
> +    } while (0);

It would be more efficient to do:

Vector<char> buffer(bytesToRead);
// Read into buffer.data();
return SharedBuffer::adoptVector(buffer);

do{}while(0) isn't super-common in WebCore. A more common way to accomplish
this would be to add a helper function, like

static void getDataFromFile(HANDLE fileHandle, Vector<char>& buffer)

These comments are mostly stylistic, so I'll r+ the patch. But I think it would
be nice to clean it up a little more before landing it, so I won't cq+ it just
yet.