Bug 86995 – Cleanup: add a file system call which captures the file metadata at once.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Kinuko Yasuda]

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.

[Kinuko Yasuda]

Created attachment 142967 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Build Bot]

Comment on attachment 142967 [details]
Patch

Attachment 142967 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12741192

[Kinuko Yasuda]

Created attachment 142975 [details]
Patch

[Build Bot]

Comment on attachment 142975 [details]
Patch

Attachment 142975 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12744151

[Kinuko Yasuda]

Created attachment 142989 [details]
Patch

[Build Bot]

Comment on attachment 142989 [details]
Patch

Attachment 142989 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12736521

[Kinuko Yasuda]

Created attachment 143003 [details]
Patch

[Kinuko Yasuda]

Created attachment 143005 [details]
Patch

[Adam Barth]

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?

[Adam Barth]

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

[Kinuko Yasuda]

Created attachment 143059 [details]
Patch

[Kinuko Yasuda]

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.

[Kinuko Yasuda]

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.

[Adam Barth]

Comment on attachment 143059 [details]
Patch

WebKit API changes LGTM

[Kinuko Yasuda]

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!

[David Levin]

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)?

[Kinuko Yasuda]

Created attachment 143768 [details]
Patch

[Kinuko Yasuda]

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.

[David Levin]

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?

[Kinuko Yasuda]

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.

[Kinuko Yasuda]

Committed r118481: <http://trac.webkit.org/changeset/118481>