Bug 111403 – File.lastModifiedDate() should return the current date/time if the file date/time is not available

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Kinuko Yasuda]

Before 2012 Oct the expected behavior in the previous spec was to return 'null', but the latest spec says it should return a new Date object with the current date/time.

[Kinuko Yasuda]

Created attachment 191413 [details]
Patch

[WebKit Review Bot]

Comment on attachment 191413 [details]
Patch

Attachment 191413 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17052026

New failing tests:
http/tests/local/fileapi/file-last-modified-after-delete.html

[Build Bot]

Comment on attachment 191413 [details]
Patch

Attachment 191413 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17029053

New failing tests:
inspector/console/console-eval-global.html

[WebKit Review Bot]

Comment on attachment 191413 [details]
Patch

Attachment 191413 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17009046

New failing tests:
http/tests/local/fileapi/file-last-modified-after-delete.html

[Kinuko Yasuda]

Created attachment 191459 [details]
Patch

[Kent Tamura]

Comment on attachment 191459 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191459&action=review

> Source/WebCore/ChangeLog:9
> +        File.lastModifiedDate() should return the current date/time if the file date/time is not available
> +        https://bugs.webkit.org/show_bug.cgi?id=111403
> +
> +        Per the recent File API spec change:
> +        http://www.w3.org/TR/2012/WD-FileAPI-20121025/#dfn-lastModifiedDate
> +
> +        Reviewed by NOBODY (OOPS!).

Normal ChangeLog format is:

<summary>
<bug URL>

Reviewed by

<details>

Test: ..

> Source/WebCore/fileapi/File.cpp:154
> +    time_t modificationTime;

You don't need to move the declaration.

> Source/WebCore/fileapi/File.cpp:162
> +        return modificationTime * 1000.0;

nit: Use WTF::msPerSecond instead of 1000.0.

> Source/WebCore/fileapi/File.cpp:164
> +    return currentTime() * 1000.0;

Ditto.

[Alexey Proskuryakov]

What does it mean for a file to not have a modification time? Is that when there is no read access to its attributes?

[Kinuko Yasuda]

(In reply to comment #7)
> What does it mean for a file to not have a modification time? Is that when there is no read access to its attributes?

I read the spec as the availability of file modification time could be platform- -dependent (like file creation time attribute is not available on Windows). In this patch I'm returning the current date/time when WebCore::getFileModificationTime() (implemented by platform layer) returns false.

The exact phrase in the spec is:
spec> lastModifiedDate: The last modified date of the file. On getting, if user agents can make this information available, this must return a new Date[HTML] object initialized to the last modified date of the file.  If the last modification date and time are not known, the attribute must return the current date and time as a Date object.

[Kinuko Yasuda]

Created attachment 191650 [details]
Patch

[Kinuko Yasuda]

Comment on attachment 191459 [details]
Patch

Thanks, updated the patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=191459&action=review

>> Source/WebCore/ChangeLog:9
>> +        Reviewed by NOBODY (OOPS!).
> 
> Normal ChangeLog format is:
> 
> <summary>
> <bug URL>
> 
> Reviewed by
> 
> <details>
> 
> Test: ..

Oh... yes, thanks. Fixed.

>> Source/WebCore/fileapi/File.cpp:154
>> +    time_t modificationTime;
> 
> You don't need to move the declaration.

Fixed.

>> Source/WebCore/fileapi/File.cpp:162
>> +        return modificationTime * 1000.0;
> 
> nit: Use WTF::msPerSecond instead of 1000.0.

It's for micro-secs per milliseconds, so I defined another constant.

[Kent Tamura]

Comment on attachment 191459 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191459&action=review

>>> Source/WebCore/fileapi/File.cpp:162
>>> +        return modificationTime * 1000.0;
>> 
>> nit: Use WTF::msPerSecond instead of 1000.0.
> 
> It's for micro-secs per milliseconds, so I defined another constant.

Really? I think time_t represents seconds in many platforms, and our Date binding code assume a double value as milliseconds.

[Kinuko Yasuda]

Created attachment 191696 [details]
Patch

[Kinuko Yasuda]

Comment on attachment 191459 [details]
Patch

Updated the patch again. Can you take another look? Thanks!

View in context: https://bugs.webkit.org/attachment.cgi?id=191459&action=review

>>>> Source/WebCore/fileapi/File.cpp:162
>>>> +        return modificationTime * 1000.0;
>>> 
>>> nit: Use WTF::msPerSecond instead of 1000.0.
>> 
>> It's for micro-secs per milliseconds, so I defined another constant.
> 
> Really? I think time_t represents seconds in many platforms, and our Date binding code assume a double value as milliseconds.

No... sorry I wrote something totally wrong. Changed to use WTF::msPerSecond.

[Kent Tamura]

Comment on attachment 191696 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191696&action=review

> Source/WebCore/fileapi/File.cpp:157
> +        return m_snapshotModificationTime * WTF::msPerSecond;

nit: We usually omit "WTF::" prefix.

> Source/WebCore/fileapi/File.cpp:162
> +        return modificationTime * WTF::msPerSecond;

Ditto.

> Source/WebCore/fileapi/File.cpp:164
> +    return currentTime() * WTF::msPerSecond;

Ditto.

[Kinuko Yasuda]

Created attachment 191732 [details]
Patch for submit

[Build Bot]

Comment on attachment 191696 [details]
Patch

Attachment 191696 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17013234

New failing tests:
inspector/debugger/debugger-expand-scope.html

[Kinuko Yasuda]

Committed r145035: <http://trac.webkit.org/changeset/145035>