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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon May 14 15:47:01 PDT 2012


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 141372: Patch
https://bugs.webkit.org/attachment.cgi?id=141372&action=review

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


> Source/WebCore/Modules/filesystem/DOMFileSystem.cpp:126
> +class GetSnapshotFileCallback : public FileSystemCallbacksBase {

I think it'd better to call it as GetMetadataCallback since we're only creating
snapshot for certain scenario.

> Source/WebCore/Modules/filesystem/DOMFileSystem.cpp:139
> +	   // For regular filesystem types (temporary or persistent) we should
not cache file metadata as it could change File semantics.

Add comma before we.

> Source/WebCore/Modules/filesystem/DOMFileSystem.cpp:140
> +	   // For other filesystem types (which could be platform-specific
ones) there's a chance that the files are on remote filesystem. If the port has
returned metadata just pass it to File constructor (so we may cache the
metadata).

ditto.

> Source/WebCore/Modules/filesystem/DOMFileSystemSync.cpp:118
> +	   // For regular filesystem types (temporary or persistent) we should
not cache file metadata as it could change File semantics.

ditto.

> Source/WebCore/Modules/filesystem/DOMFileSystemSync.cpp:119
> +	   // For other filesystem types (which could be platform-specific
ones) there's a chance that the files are on remote filesystem. If the port has
returned metadata just pass it to File constructor (so we may cache the
metadata).

ditto.

> Source/WebCore/Modules/mediastream/PeerConnection00.cpp:99
> +    dictionary.get("has_video", audio);

??

> Source/WebCore/fileapi/Blob.cpp:78
> +    long long size = m_size;

It seems the original code is a bit more efficient. What do you need to change
that?

> Source/WebCore/fileapi/Blob.h:44
> +class File;

Is this forward declaration needed?

> Source/WebCore/fileapi/File.cpp:97
> +    : Blob(createBlobDataForFileWithName(metadata.platformPath, name), -1)

Should we also store both size and modification time, if provided by the remote
system, in the blob data? Should we not pass -1 when caching the metadata?

> Source/WebCore/fileapi/File.cpp:157
> +bool File::hasValidSnapshotMetadata() const

It seems that hasValidSnapshotMetadata might'd better be exposed in
FileMetadata.

> Source/WebCore/fileapi/File.h:57
> +    // For filesystem files. It takes metadata which could optionally give
valid snapshot metadata, which will suppress future metadata query on this File
object. The metadata argument must be used carefully if and only if it is
infeasible to query file metadata on the fly (e.g. file is on remote
filesystem).

nit: grammar error.

> Source/WebCore/fileapi/File.h:105
> +    const FileMetadata m_snapshotMetadata;

This might be duplicate. We could probable get the metadata directly out of the
blob data. It seems that we can use a boolean flag to denote a snapshot file
case.


More information about the webkit-reviews mailing list