Bug 37222 - [WINCE] Port SharedBuffer
Summary: [WINCE] Port SharedBuffer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-07 11:31 PDT by Kwang Yul Seo
Modified: 2010-05-11 09:38 PDT (History)
7 users (show)

See Also:


Attachments
Patch (3.98 KB, patch)
2010-04-07 11:35 PDT, Kwang Yul Seo
no flags Details | Formatted Diff | Diff
Patch (3.98 KB, patch)
2010-04-26 02:34 PDT, Kwang Yul Seo
no flags Details | Formatted Diff | Diff
Alternative patch (3.29 KB, patch)
2010-05-04 01:39 PDT, Patrick R. Gansterer
aroben: review+
Details | Formatted Diff | Diff
Alternative patch part 2 (3.49 KB, patch)
2010-05-10 08:11 PDT, Patrick R. Gansterer
aroben: review-
Details | Formatted Diff | Diff
Alternative patch part 3 (3.53 KB, patch)
2010-05-10 10:50 PDT, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kwang Yul Seo 2010-04-07 11:31:07 PDT
Implement SharedBuffer::createWithContentsOfFile with standard IO functions.
Comment 1 Kwang Yul Seo 2010-04-07 11:35:14 PDT
Created attachment 52760 [details]
Patch
Comment 2 Patrick R. Gansterer 2010-04-25 02:38:34 PDT
> +    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
Comment 3 Kwang Yul Seo 2010-04-26 02:31:15 PDT
(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.
Comment 4 Kwang Yul Seo 2010-04-26 02:34:03 PDT
Created attachment 54268 [details]
Patch

Remove the semicolon after the namespace block.
Comment 5 Patrick R. Gansterer 2010-04-26 02:44:38 PDT
(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.
Comment 6 Patrick R. Gansterer 2010-05-04 01:39:08 PDT
Created attachment 55002 [details]
Alternative patch

This patch only modifies the exisiting SharedBufferWin.cpp.
Comment 7 Kwang Yul Seo 2010-05-04 19:17:37 PDT
Comment on attachment 54268 [details]
Patch

Cancel my review request in favor of Patrick's patch.
Comment 8 Eric Seidel (no email) 2010-05-09 14:34:01 PDT
Adam Roben is probably the right reviewer here.
Comment 9 Adam Roben (:aroben) 2010-05-10 06:47:14 PDT
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.
Comment 10 Patrick R. Gansterer 2010-05-10 07:36:54 PDT
(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.
Comment 11 Adam Roben (:aroben) 2010-05-10 07:52:32 PDT
(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.
Comment 12 Patrick R. Gansterer 2010-05-10 08:11:02 PDT
Created attachment 55554 [details]
Alternative patch part 2

Adressed Adams points.
Comment 13 Adam Roben (:aroben) 2010-05-10 08:56:22 PDT
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();
Comment 14 Patrick R. Gansterer 2010-05-10 10:50:55 PDT
Created attachment 55569 [details]
Alternative patch part 3

> You're using buffer.data() here without setting buffer's size.
Sorry! I've corrected that.
Comment 15 Adam Roben (:aroben) 2010-05-10 12:05:11 PDT
Comment on attachment 55569 [details]
Alternative patch part 3

r=me!
Comment 16 WebKit Commit Bot 2010-05-11 09:38:31 PDT
Comment on attachment 55569 [details]
Alternative patch part 3

Clearing flags on attachment: 55569

Committed r59154: <http://trac.webkit.org/changeset/59154>
Comment 17 WebKit Commit Bot 2010-05-11 09:38:38 PDT
All reviewed patches have been landed.  Closing bug.