[webkit-reviews] review denied: [Bug 86811] REGRESSION: We should allow null modificationTime when snapshot metadata is given : [Attachment 143720] 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 86811: REGRESSION: We should allow null modificationTime when snapshot
metadata is given
https://bugs.webkit.org/show_bug.cgi?id=86811

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

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


> Source/WebCore/ChangeLog:8
> +	   No new tests as this change is not for regular files/filesystems.

Please describe the reason why we need to fix this.

> Source/WebCore/fileapi/File.cpp:133
> +    if (m_snapshotSize >= 0)

It would be better if we can introduce a helper function like isValidSnapshot
so that we have one centralized place to perform this check and you can put
comment there.

> Source/WebCore/fileapi/File.h:103
> +    // If valid file metadata (whose length field is non-negative) is given
at construction time we use the metadata in size(), lastModifiedTime() and
webkitSlice().

nit: comma before "we use the ..."

> Source/WebCore/fileapi/File.h:105
> +    // Note that we may return null lastModifiedTime if it is given
(together with positive file length).

The whole comment is a bit confusing. How about rephrasing to something like:
// If m_snapShotSize is negative (initialized to -1 by default), the snapshot
metadata is invalid and we retrieve the latest metadata synchronously in
size(), lastModifiedTime() and webkitSlice(). Otherwise, the valid snapshot
metadata can be used directly in those methods.
// Note that the valid snapshot metadata could have non-negative size and null
modified time which mean that ......