Implement SharedBuffer::createWithContentsOfFile with standard IO functions.
Created attachment 52760 [details] Patch
> + 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
(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.
Created attachment 54268 [details] Patch Remove the semicolon after the namespace block.
(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.
Created attachment 55002 [details] Alternative patch This patch only modifies the exisiting SharedBufferWin.cpp.
Comment on attachment 54268 [details] Patch Cancel my review request in favor of Patrick's patch.
Adam Roben is probably the right reviewer here.
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.
(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.
(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.
Created attachment 55554 [details] Alternative patch part 2 Adressed Adams points.
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();
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 on attachment 55569 [details] Alternative patch part 3 r=me!
Comment on attachment 55569 [details] Alternative patch part 3 Clearing flags on attachment: 55569 Committed r59154: <http://trac.webkit.org/changeset/59154>
All reviewed patches have been landed. Closing bug.