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

[bugzilla-daemon at webkit.org]

Jian Li <jianli at chromium.org> has denied 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 141940: Patch
https://bugs.webkit.org/attachment.cgi?id=141940&action=review

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


> Source/WebCore/fileapi/File.cpp:68
> +	   contentType =
MIMETypeRegistry::getWellKnownMIMETypeForExtension(fileSystemName.substring(ind
ex + 1));

Can this content type extraction code be shared among all these 3 static
methods?

> Source/WebCore/fileapi/File.cpp:146
> +    if (m_size >= 0)

Though this logic works, I think it is inclined to mistakes in the future since
m_size is indeed defined and initialized in Blob. Suppose someone changes the
default value for m_size or assign some value for m_size in Blob
implementation, we will return m_size without doing query for normal filesystem
files. I think it is safer to explicitly define a boolean flag in File to mean
that a snapshot is provided. Or, can we solely use m_snapshotModificationTime
to find out if the snapshot is valid?

> Source/WebCore/fileapi/File.h:30
> +#include "FileMetadata.h"

It seems that we can use forward declaration instead.

> Source/WebCore/fileapi/File.h:57
> +    // For filesystem files. It takes metadata argument, which may suppress
future metadata query on this File object if it has valid information (i.e.
non-zero modificationTime and/or non-negative file length). The metadata
argument must be given carefully only when it is infeasible to query file
metadata on the fly (e.g. file is on remote filesystem).

Maybe simpler and easier to understand if this is rephrased to something like:
  If filesystem files lives in the remote filesystem, the port might pass the
valid metadata (non-zero modificationTime and/or non-negative file size) and
cache it in the File object.

> LayoutTests/ChangeLog:9
> +	   * fast/filesystem/file-read-after-write.html: Added.

This tests seems to purely verify that the metadata is not cached, right?

> LayoutTests/fast/filesystem/file-metadata-after-write.html:59
> +function test(expect, actual)

Can we rely on those test utils provided by js-test-pre.js?