Bug 55336 – Use Win32 API to get file information

• Component: Platform
• Status: RESOLVED FIXED

[Patrick R. Gansterer]

see patch

[Patrick R. Gansterer]

Created attachment 84001 [details]
Patch

[Patrick R. Gansterer]

This patch will fail on the Win EWS, since it depends on Win32Handle.h from bug 55334.

[Build Bot]

Attachment 84001 [details] did not build on win:
Build output: http://queues.webkit.org/results/8074142

[Build Bot]

Attachment 84001 [details] did not build on win:
Build output: http://queues.webkit.org/results/8070238

[Adam Roben (:aroben)]

Comment on attachment 84001 [details]
Patch

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.

[Patrick R. Gansterer]

Created attachment 84603 [details]
Patch

> +    result = fileSize.QuadPart / 10000000 - 11644473600;
This line comes from FileSystemWinCE. I didn't find a good name for the constants.

[Adam Roben (:aroben)]

Comment on attachment 84603 [details]
Patch

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

[Patrick R. Gansterer]

Committed r81551: <http://trac.webkit.org/changeset/81551>

[WebKit Review Bot]

http://trac.webkit.org/changeset/81551 might have broken Windows 7 Release (Tests)
The following tests are not passing:
fast/workers/storage/multiple-databases-garbage-collection.html
fast/workers/storage/open-database-inputs-sync.html
storage/multiple-databases-garbage-collection.html
storage/open-database-creation-callback-isolated-world.html
storage/open-database-creation-callback.html
storage/quota-tracking.html
storage/statement-error-callback-isolated-world.html
storage/statement-success-callback-isolated-world.html
storage/transaction-callback-isolated-world.html
storage/transaction-error-callback-isolated-world.html
storage/transaction-success-callback-isolated-world.html

[Patrick R. Gansterer]

Reverted r81551 for reason:

Broke

Committed r81555: <http://trac.webkit.org/changeset/81555>

[Patrick R. Gansterer]

Created attachment 127782 [details]
Patch

[Early Warning System Bot]

Comment on attachment 127782 [details]
Patch

Attachment 127782 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11548226

[Patrick R. Gansterer]

Comment on attachment 127782 [details]
Patch

Clearing flags on attachment: 127782

Committed r108321: <http://trac.webkit.org/changeset/108321>

[Patrick R. Gansterer]

All reviewed patches have been landed.  Closing bug.