[Webkit-unassigned] [Bug 37222] [WINCE] Port SharedBuffer

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=37222


Adam Roben (aroben) <aroben at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #55002|review?, commit-queue?      |review+
               Flag|                            |




--- Comment #9 from Adam Roben (aroben) <aroben at apple.com>  2010-05-10 06:47:14 PST ---
(From update of attachment 55002)
> -    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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.