Bug 55336 - Use Win32 API to get file information
Summary: Use Win32 API to get file information
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Patrick R. Gansterer
Depends on: 55334
  Show dependency treegraph
Reported: 2011-02-27 16:03 PST by Patrick R. Gansterer
Modified: 2012-02-21 00:43 PST (History)
5 users (show)

See Also:

Patch (3.04 KB, patch)
2011-02-27 16:10 PST, Patrick R. Gansterer
paroga: commit-queue-
Details | Formatted Diff | Diff
Patch (3.23 KB, patch)
2011-03-03 12:03 PST, Patrick R. Gansterer
aroben: commit-queue-
Details | Formatted Diff | Diff
Patch (3.06 KB, patch)
2012-02-20 01:47 PST, 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 Patrick R. Gansterer 2011-02-27 16:03:13 PST
see patch
Comment 1 Patrick R. Gansterer 2011-02-27 16:10:01 PST
Created attachment 84001 [details]
Comment 2 Patrick R. Gansterer 2011-02-27 16:11:14 PST
This patch will fail on the Win EWS, since it depends on Win32Handle.h from bug 55334.
Comment 3 Build Bot 2011-02-27 17:33:23 PST
Attachment 84001 [details] did not build on win:
Build output: http://queues.webkit.org/results/8074142
Comment 4 Build Bot 2011-02-27 18:41:54 PST
Attachment 84001 [details] did not build on win:
Build output: http://queues.webkit.org/results/8070238
Comment 5 Adam Roben (:aroben) 2011-02-28 05:28:41 PST
Comment on attachment 84001 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=84001&action=review

No r+ yet because I have some questions.

> Source/WebCore/platform/win/FileSystemWin.cpp:46
> +// UNIX epoch (1970-01-01 00:00:00) expressed in Windows NT FILETIME
> +static const uint64_t unixEpoch = 0x019DB1DED53E8000ULL;

Did you get this constant from somewhere? It would be nice to document it if so.

> Source/WebCore/platform/win/FileSystemWin.cpp:51
> +    return CreateFileW(filename.charactersWithNullTermination(), GENERIC_READ, FILE_SHARE_READ, 0, OPEN_EXISTING, FILE_FLAG_NO_BUFFERING, 0);

In WebKit2 we've started prefixing Win32 API calls with ::. I know that can be a problem for certain Win32 API that on WinCE are implemented as macros. But maybe we should do it wherever possible?

Why are you passing FILE_FLAG_NO_BUFFERING? That makes it much harder to use the file correctly.

> Source/WebCore/platform/win/FileSystemWin.cpp:75
> +    if (!GetFileInformationByHandle(fileHandle, &fileInformation))

You could use GetFileAttributesEx instead to avoid needing a handle.

> Source/WebCore/platform/win/FileSystemWin.cpp:79
> +    uint64_t lastWriteTime = fileInformation.ftLastWriteTime.dwHighDateTime;
> +    lastWriteTime = lastWriteTime << 32 | fileInformation.ftLastWriteTime.dwLowDateTime;

I think using ULARGE_INTEGER to get this into a uint64_t would be clearer.
Comment 6 Patrick R. Gansterer 2011-03-03 12:03:48 PST
Created attachment 84603 [details]

> +    result = fileSize.QuadPart / 10000000 - 11644473600;
This line comes from FileSystemWinCE. I didn't find a good name for the constants.
Comment 7 Adam Roben (:aroben) 2011-03-07 12:45:26 PST
Comment on attachment 84603 [details]

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
Comment 8 Patrick R. Gansterer 2011-03-19 10:55:38 PDT
Committed r81551: <http://trac.webkit.org/changeset/81551>
Comment 9 WebKit Review Bot 2011-03-19 12:57:20 PDT
http://trac.webkit.org/changeset/81551 might have broken Windows 7 Release (Tests)
The following tests are not passing:
Comment 10 Patrick R. Gansterer 2011-03-19 13:10:26 PDT
Reverted r81551 for reason:


Committed r81555: <http://trac.webkit.org/changeset/81555>
Comment 11 Patrick R. Gansterer 2012-02-20 01:47:21 PST
Created attachment 127782 [details]
Comment 12 Early Warning System Bot 2012-02-20 01:59:02 PST
Comment on attachment 127782 [details]

Attachment 127782 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11548226
Comment 13 Patrick R. Gansterer 2012-02-21 00:43:34 PST
Comment on attachment 127782 [details]

Clearing flags on attachment: 127782

Committed r108321: <http://trac.webkit.org/changeset/108321>
Comment 14 Patrick R. Gansterer 2012-02-21 00:43:47 PST
All reviewed patches have been landed.  Closing bug.