Bug 111403 - File.lastModifiedDate() should return the current date/time if the file date/time is not available
Summary: File.lastModifiedDate() should return the current date/time if the file date/...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Kinuko Yasuda
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-04 21:00 PST by Kinuko Yasuda
Modified: 2013-03-06 21:22 PST (History)
8 users (show)

See Also:


Attachments
Patch (2.77 KB, patch)
2013-03-04 23:39 PST, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (5.81 KB, patch)
2013-03-05 04:05 PST, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (6.05 KB, patch)
2013-03-05 21:53 PST, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (6.05 KB, patch)
2013-03-06 02:39 PST, Kinuko Yasuda
tkent: review+
tkent: commit-queue-
Details | Formatted Diff | Diff
Patch for submit (6.04 KB, patch)
2013-03-06 05:46 PST, Kinuko Yasuda
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kinuko Yasuda 2013-03-04 21:00:06 PST
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.
Comment 1 Kinuko Yasuda 2013-03-04 23:39:51 PST
Created attachment 191413 [details]
Patch
Comment 2 WebKit Review Bot 2013-03-05 01:58:57 PST
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
Comment 3 Build Bot 2013-03-05 02:58:30 PST
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
Comment 4 WebKit Review Bot 2013-03-05 03:10:22 PST
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
Comment 5 Kinuko Yasuda 2013-03-05 04:05:06 PST
Created attachment 191459 [details]
Patch
Comment 6 Kent Tamura 2013-03-05 04:13:44 PST
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.
Comment 7 Alexey Proskuryakov 2013-03-05 10:01:43 PST
What does it mean for a file to not have a modification time? Is that when there is no read access to its attributes?
Comment 8 Kinuko Yasuda 2013-03-05 19:54:38 PST
(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.
Comment 9 Kinuko Yasuda 2013-03-05 21:53:54 PST
Created attachment 191650 [details]
Patch
Comment 10 Kinuko Yasuda 2013-03-05 21:56:22 PST
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.
Comment 11 Kent Tamura 2013-03-06 01:33:34 PST
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.
Comment 12 Kinuko Yasuda 2013-03-06 02:39:16 PST
Created attachment 191696 [details]
Patch
Comment 13 Kinuko Yasuda 2013-03-06 02:40:31 PST
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.
Comment 14 Kent Tamura 2013-03-06 04:28:01 PST
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.
Comment 15 Kinuko Yasuda 2013-03-06 05:46:00 PST
Created attachment 191732 [details]
Patch for submit
Comment 16 Build Bot 2013-03-06 19:28:24 PST
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
Comment 17 Kinuko Yasuda 2013-03-06 21:22:37 PST
Committed r145035: <http://trac.webkit.org/changeset/145035>