Cleanup: add a file system call which captures the file metadata at once. Current File.slice() (webkitSlice()) implementation calls two separate platform calls, getFileSize() and getFileModificationTime() [both are defined in platform/FileSystem.h], to capture the file metadata, but we should have a single file system call to get them at once for two reasons: 1. save additional system call costs, and 2. atomically obtain the file metadata.
Created attachment 142967 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment on attachment 142967 [details] Patch Attachment 142967 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12741192
Created attachment 142975 [details] Patch
Comment on attachment 142975 [details] Patch Attachment 142975 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12744151
Created attachment 142989 [details] Patch
Comment on attachment 142989 [details] Patch Attachment 142989 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12736521
Created attachment 143003 [details] Patch
Created attachment 143005 [details] Patch
Comment on attachment 143005 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143005&action=review > Source/Platform/chromium/public/WebFileUtilities.h:44 > +struct WebFileInfo; Looks like you forgot to svn add WebFileInfo.h > Source/Platform/chromium/public/WebFileUtilities.h:57 > virtual bool getFileSize(const WebString& path, long long& result) { return false; } > virtual bool getFileModificationTime(const WebString& path, double& result) { return false; } Should we mark these as deprecated and plan to delete them?
Actually, looks like it does exist at http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/public/WebFileInfo.h. Can you move it into Platform/chromium/public ? The Source/Platform shouldn't refer to things in Source/WebKit
Created attachment 143059 [details] Patch
Comment on attachment 143005 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143005&action=review >> Source/Platform/chromium/public/WebFileUtilities.h:44 >> +struct WebFileInfo; > > Looks like you forgot to svn add WebFileInfo.h Moved Source/WebKit/chromium/public/WebFileInfo.h into Platform/chromium/public. >> Source/Platform/chromium/public/WebFileUtilities.h:57 >> virtual bool getFileModificationTime(const WebString& path, double& result) { return false; } > > Should we mark these as deprecated and plan to delete them? Sounds good, added a comment.
Comment on attachment 143059 [details] Patch WebKit API changes LGTM
This isn't intended to change existing behavior but is intended to remove FIXME's in File and cleanup platform APIs. (I also would like to use the new platform API for other purpose) Jian, David could either of you take a look at the patch? Thanks!
Comment on attachment 143059 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143059&action=review r- see comments. I would do it in a dropdown but I have https://code.google.com/p/chromium/issues/detail?id=129444 to contend with :(. > Source/WebCore/platform/posix/FileSystemPOSIX.cpp:199 > + return true; It feels like this code should be exactly like the code you added in FileSystemGtk.cpp (or visa versa) except for the g_stat vs stat. Why are there are other differences? > Source/WebCore/platform/qt/FileSystemQt.cpp:80 > + result.modificationTime = info.lastModified().toTime_t(); What data is being access if the file doesn't exist? It feels sketchy to do this if !info.exists(). > Source/WebCore/platform/win/FileSystemWin.cpp:79 > + return true; Why return a bool at all if it always returns true? > Source/WebCore/platform/wince/FileSystemWinCE.cpp:72 > +static void getFileSizeFromFileInfo(const BY_HANDLE_FILE_INFORMATION& fileInfo, long long& result) result isn't very descriptive of what the variable contains. > Source/WebCore/platform/wince/FileSystemWinCE.cpp:84 > + result = t.QuadPart * 0.0000001 - 11644473600.0; ditto re result (and in the other changes in this file). > Source/WebCore/platform/wx/FileSystemWx.cpp:89 > +bool getFileMetadata(const String& path, FileMetadata& t) avoid abbreviations "t" > Source/WebKit/chromium/src/PlatformSupport.cpp:340 > + result.type = static_cast<FileMetadata::Type>(webFileInfo.type); Is there already a function to do this conversion (near some place with COMPILE_ASSERTS to verify that the values are the same)?
Created attachment 143768 [details] Patch
Comment on attachment 143059 [details] Patch Thanks!! Updated the patch. View in context: https://bugs.webkit.org/attachment.cgi?id=143059&action=review >> Source/WebCore/platform/posix/FileSystemPOSIX.cpp:199 >> + return true; > > It feels like this code should be exactly like the code you added in FileSystemGtk.cpp (or visa versa) except for the g_stat vs stat. Why are there are other differences? Basically I followed other code in the same file (e.g. posix's getFileMetadata() is implemented after posix's getFileSize and getFileModificationTime), and coding convention / preferences in each platform looks slightly different. >> Source/WebCore/platform/qt/FileSystemQt.cpp:80 >> + result.modificationTime = info.lastModified().toTime_t(); > > What data is being access if the file doesn't exist? > > It feels sketchy to do this if !info.exists(). Good catch, fixed to exit earlier. >> Source/WebCore/platform/win/FileSystemWin.cpp:79 >> + return true; > > Why return a bool at all if it always returns true? Fixed. >> Source/WebCore/platform/wince/FileSystemWinCE.cpp:72 >> +static void getFileSizeFromFileInfo(const BY_HANDLE_FILE_INFORMATION& fileInfo, long long& result) > > result isn't very descriptive of what the variable contains. Fixed. >> Source/WebCore/platform/wince/FileSystemWinCE.cpp:84 >> + result = t.QuadPart * 0.0000001 - 11644473600.0; > > ditto re result (and in the other changes in this file). Fixed. >> Source/WebCore/platform/wx/FileSystemWx.cpp:89 >> +bool getFileMetadata(const String& path, FileMetadata& t) > > avoid abbreviations "t" Fixed. >> Source/WebKit/chromium/src/PlatformSupport.cpp:340 >> + result.type = static_cast<FileMetadata::Type>(webFileInfo.type); > > Is there already a function to do this conversion (near some place with COMPILE_ASSERTS to verify that the values are the same)? Yes, we have assertions in WebKit/chromium/src/AssertMatchingEnums.cpp.
Comment on attachment 143768 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143768&action=review Only two comments. Feel free to address and submit. > Source/WebCore/platform/FileMetadata.h:57 > +#endif // ENABLE(FILE_SYSTEM) The end comment for the if isn't necessary and doesn't seem very useful for such a short area. > Source/WebCore/platform/chromium/FileSystemChromium.cpp:61 > +bool getFileMetadata(const String& path, FileMetadata& result) result isn't very descriptive. > Source/WebCore/platform/chromium/FileSystemChromium.cpp:70 > + result.modificationTime = modificationTime; Why doesn't this set type?
Thanks, will address them and submit. (In reply to comment #20) > (From update of attachment 143768 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=143768&action=review > > Only two comments. Feel free to address and submit. > > > Source/WebCore/platform/FileMetadata.h:57 > > +#endif // ENABLE(FILE_SYSTEM) > > The end comment for the if isn't necessary and doesn't seem very useful for such a short area. Will remove it. > > Source/WebCore/platform/chromium/FileSystemChromium.cpp:61 > > +bool getFileMetadata(const String& path, FileMetadata& result) > > result isn't very descriptive. Will fix. > > Source/WebCore/platform/chromium/FileSystemChromium.cpp:70 > > + result.modificationTime = modificationTime; > > Why doesn't this set type? The type info isn't available until the chromium port implementation is landed (which will come shortly). For now I I'll add more specific FIXME comment.
Committed r118481: <http://trac.webkit.org/changeset/118481>