Bug 37222 – [WINCE] Port SharedBuffer

• Component: Platform
• Status: RESOLVED FIXED

[Kwang Yul Seo]

Implement SharedBuffer::createWithContentsOfFile with standard IO functions.

[Kwang Yul Seo]

Created attachment 52760 [details]
Patch

[Patrick R. Gansterer]

> +    FILE* fileDescriptor = 0;
> +    fileDescriptor = _wfopen(nullifiedPath.charactersWithNullTermination(), TEXT("rb"));
> +    if (!fileDescriptor) {
Isn't it possible to use CreateFileW and CloseHandle? They would be more native.

> +}; // namespace WebCore
no semicolon please

[Kwang Yul Seo]

(In reply to comment #2)
> > +    FILE* fileDescriptor = 0;
> > +    fileDescriptor = _wfopen(nullifiedPath.charactersWithNullTermination(), TEXT("rb"));
> > +    if (!fileDescriptor) {
> Isn't it possible to use CreateFileW and CloseHandle? They would be more
> native.
> 

It is possible, but I tried to follow the convention used in SharedBufferWin.cpp

> > +}; // namespace WebCore
> no semicolon please

Oops. I found the same mistake in SharedBufferWin.cpp. I will fix it.

[Kwang Yul Seo]

Created attachment 54268 [details]
Patch

Remove the semicolon after the namespace block.

[Patrick R. Gansterer]

(In reply to comment #3)
> It is possible, but I tried to follow the convention used in
> SharedBufferWin.cpp
I'd like to see the WinCE port directly in the Win32 port (where possible). Maybe you can post a patch which only changes the Win32 version and remove the ugly goto in this step by using somthing like:

fopen();
do {

  ftell()
  if (error)
    break;

  fread()

} while (0);

fclose();
return;


I think that it is more WebKit Style than your FileDescriptorHolder class.

[Patrick R. Gansterer]

Created attachment 55002 [details]
Alternative patch

This patch only modifies the exisiting SharedBufferWin.cpp.

[Kwang Yul Seo]

Comment on attachment 54268 [details]
Patch

Cancel my review request in favor of Patrick's patch.

[Eric Seidel (no email)]

Adam Roben is probably the right reviewer here.

[Adam Roben (:aroben)]

Comment on attachment 55002 [details]
Alternative patch

> -    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.

[Patrick R. Gansterer]

(In reply to comment #9)
> 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?
I don't see the additional value for calling GetFileSizeEx. ReadFile can only read 0xffffffff in one call. That's the maximum returned by GetFileSize already.

[Adam Roben (:aroben)]

(In reply to comment #10)
> (In reply to comment #9)
> > 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?
> I don't see the additional value for calling GetFileSizeEx. ReadFile can only read 0xffffffff in one call. That's the maximum returned by GetFileSize already.

GetFileSizeEx is just a nicer API (returns a BOOL indicating success/failure instead of returning a DWORD with a special value that might indicate failure). But switching to it isn't critical.

[Patrick R. Gansterer]

Created attachment 55554 [details]
Alternative patch part 2

Adressed Adams points.

[Adam Roben (:aroben)]

Comment on attachment 55554 [details]
Alternative patch part 2

> +    Vector<char> buffer;
> +    DWORD bytesToRead = GetFileSize(fileHandle, 0);
> +    DWORD lastError = GetLastError();
>  
> -    // Stat the file to get its size
> -    struct _stat64 fileStat;
> -    if (_fstat64(_fileno(fileDescriptor), &fileStat))
> -        goto exit;
> +    if (bytesToRead != INVALID_FILE_SIZE || lastError == NO_ERROR) {
> +        DWORD bytesRead;
> +        if (!ReadFile(fileHandle, buffer.data(), bytesToRead, &bytesRead, 0) || bytesToRead != bytesRead)

You're using buffer.data() here without setting buffer's size.

I'd suggest structuring things like this:

RefPtr<SharedBuffer> result;
DWORD bytesToRead = GetFileSize(fileHandle, 0);

if (bytesToRead != INVALID_FILE_SIZE || lastError == NO_ERROR) {
    Vector<char> buffer(bytesToRead);
    if (!ReadFile(...) || bytesToRead != bytesRead)
        LOG_ERROR(...);
    else
        result = SharedBuffer::adoptVector(buffer);
} else
    LOG_ERROR(...);

CloseHandle(fileHandle);
return result.release();

[Patrick R. Gansterer]

Created attachment 55569 [details]
Alternative patch part 3

> You're using buffer.data() here without setting buffer's size.
Sorry! I've corrected that.

[Adam Roben (:aroben)]

Comment on attachment 55569 [details]
Alternative patch part 3

r=me!

[WebKit Commit Bot]

Comment on attachment 55569 [details]
Alternative patch part 3

Clearing flags on attachment: 55569

Committed r59154: <http://trac.webkit.org/changeset/59154>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.