[webkit-reviews] review granted: [Bug 55336] Use Win32 API to get file information : [Attachment 84603] 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 55336: Use Win32 API to get file information
https://bugs.webkit.org/show_bug.cgi?id=55336

Attachment 84603: Patch
https://bugs.webkit.org/attachment.cgi?id=84603&action=review

------- Additional Comments from Adam Roben (:aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=84603&action=review

> Source/WebCore/platform/win/FileSystemWin.cpp:45
> +static inline HANDLE createFileHandle(const String& path)

It would be slightly better if we could come up with a name that indicated the
HANDLE is only valid for reading.

> Source/WebCore/platform/win/FileSystemWin.cpp:56
> +    if (!fileHandle.isValid())
> +	   return false;
> +    return !!::GetFileInformationByHandle(fileHandle.get(),
&fileInformation);

The !! isn't required in Apple's Windows port. We've disabled the warning that
omitting it would normally generate. You should probably disable that warning
in the WinCE port, too, and leave out the !! here. (Otherwise some unsuspecting
Windows developer is liable to come along and remove the !!, thus breaking the
WinCE build.)

You could also write this as: return fileHandle.isValid() &&
::GetFileInformationByHandle(...);

> Source/WebCore/platform/win/FileSystemWin.cpp:68
> +    ULARGE_INTEGER fileSize;
> +    fileSize.HighPart = fileInformation.nFileSizeHigh;
> +    fileSize.LowPart = fileInformation.nFileSizeLow;
> +    result = fileSize.QuadPart;

Seems like we need to check for overflow here. result is signed, but fileSize
is not.

> Source/WebCore/platform/win/FileSystemWin.cpp:81
> +    result = fileSize.QuadPart / 10000000 - 11644473600;

I think it would be better to put the magic number in a named constant.

Maybe you should reference this page in a comment:
http://msdn.microsoft.com/en-us/library/ms724228(v=vs.85).aspx