[webkit-reviews] review granted: [Bug 78879] Allow FileSystem API implementation to pass snapshot metadata at File creation time : [Attachment 142414] Patch

[bugzilla-daemon at webkit.org]

Jian Li <jianli at chromium.org> has granted Kinuko Yasuda <kinuko at chromium.org>'s
request for review:
Bug 78879: Allow FileSystem API implementation to pass snapshot metadata at
File creation time
https://bugs.webkit.org/show_bug.cgi?id=78879

Attachment 142414: Patch
https://bugs.webkit.org/attachment.cgi?id=142414&action=review

------- Additional Comments from Jian Li <jianli at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=142414&action=review


> Source/WebCore/fileapi/File.cpp:163
> +    if (m_snapshotSize >= 0 && m_snapshotModificationTime) {

This seems a bit inconsistent w/ if checks in lastModifiedDate() and size().
Your comment below says valid metadata is meant by non-zero modificationTime
and/or non-negative file size. Can we have "or" scenario? If yes, please change
&& to ||. Otherwise, please always use "m_snapshotSize >= 0 &&
m_snapshotModificationTime" for all 3 checks.

> Source/WebCore/fileapi/File.h:57
> +    // If filesystem files live in the remote filesystem, the port might
pass the valid metdata (non-zero modificationTime and/or non-negative file
size) and cache in the File object.

typo: metdata

> Source/WebCore/fileapi/File.h:102
> +#if ENABLE(FILE_SYSTEM)

Please comment that what scenario these 2 members should be used. Also please
illustrate the default values.

> LayoutTests/fast/filesystem/file-metadata-after-write-expected.txt:1
> +Writing 1234567890 to the file...

Better to output a line of description to explain what we want to test here:
  Test that metadata is not cached in the regular temporary filesystem.

> LayoutTests/fast/filesystem/file-metadata-after-write.html:84
> +	 layoutTestController.notifyDone();

nit: wrong indenting.

> LayoutTests/fast/filesystem/file-metadata-after-write.html:90
> +

nit: extra line