RESOLVED FIXED Bug 55336
Use Win32 API to get file information
https://bugs.webkit.org/show_bug.cgi?id=55336
Summary Use Win32 API to get file information
Patrick R. Gansterer
Reported 2011-02-27 16:03:13 PST
see patch
Attachments
Patch (3.04 KB, patch)
2011-02-27 16:10 PST, Patrick R. Gansterer
paroga: commit-queue-
Patch (3.23 KB, patch)
2011-03-03 12:03 PST, Patrick R. Gansterer
aroben: commit-queue-
Patch (3.06 KB, patch)
2012-02-20 01:47 PST, Patrick R. Gansterer
no flags
Patrick R. Gansterer
Comment 1 2011-02-27 16:10:01 PST
Patrick R. Gansterer
Comment 2 2011-02-27 16:11:14 PST
This patch will fail on the Win EWS, since it depends on Win32Handle.h from bug 55334.
Build Bot
Comment 3 2011-02-27 17:33:23 PST
Build Bot
Comment 4 2011-02-27 18:41:54 PST
Adam Roben (:aroben)
Comment 5 2011-02-28 05:28:41 PST
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
Comment 6 2011-03-03 12:03:48 PST
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 7 2011-03-07 12:45:26 PST
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
Comment 8 2011-03-19 10:55:38 PDT
WebKit Review Bot
Comment 9 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: 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
Comment 10 2011-03-19 13:10:26 PDT
Reverted r81551 for reason: Broke Committed r81555: <http://trac.webkit.org/changeset/81555>
Patrick R. Gansterer
Comment 11 2012-02-20 01:47:21 PST
Early Warning System Bot
Comment 12 2012-02-20 01:59:02 PST
Patrick R. Gansterer
Comment 13 2012-02-21 00:43:34 PST
Comment on attachment 127782 [details] Patch Clearing flags on attachment: 127782 Committed r108321: <http://trac.webkit.org/changeset/108321>
Patrick R. Gansterer
Comment 14 2012-02-21 00:43:47 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.