File.lastModifiedDate must return null if the modified time info is not available

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Kinuko Yasuda]

File.lastModifiedDate must return null if the modified time info is not available.

http://dev.w3.org/2006/webapi/FileAPI/#dfn-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; otherwise, this MUST return null."

[Kinuko Yasuda]

Created attachment 144480 [details]
Patch

[Kinuko Yasuda]

Created attachment 144484 [details]
Patch

[Kinuko Yasuda]

I'm slightly separated if I should use custom binding for this one. (Date(0) itself is totally valid while conventionally we use the value 0 to indicate 'null' value in File and elsewhere in webkit)

[Kentaro Hara]

Comment on attachment 144484 [details]
Patch

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

r+ for IDL, V8 bindings, and JSC bindings. We want to avoid custom bindings, but it seems unavoidable.

> Source/WebCore/ChangeLog:10
> +        Per File API spec, File.lastModifiedDate must return null if the
> +        modified time info is not available.
> +        http://dev.w3.org/2006/webapi/FileAPI/#dfn-lastModifiedDate

What's the behavior of other browsers?

[Kinuko Yasuda]

(In reply to comment #4)
> (From update of attachment 144484 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144484&action=review
> 
> r+ for IDL, V8 bindings, and JSC bindings. We want to avoid custom bindings, but it seems unavoidable.
> 
> > Source/WebCore/ChangeLog:10
> > +        Per File API spec, File.lastModifiedDate must return null if the
> > +        modified time info is not available.
> > +        http://dev.w3.org/2006/webapi/FileAPI/#dfn-lastModifiedDate
> 
> What's the behavior of other browsers?

Safari 5.1 behaves in the same way as the current chrome does (i.e. returns "Jan 01 1970" date/time if unavailable), and as far as I checked FireFox/IE don't support it yet.
[Some more note: there's ongoing discussion to make 'lastModifiedDate' attribute static, and in that case this test will become invalid as we will be returning the same date/time without querying (I'll deprecate this test then), but the change itself should remain valid since there will still be a chance where 'lastModifiedDate' is not available when we capture file metadata]

[Kentaro Hara]

Comment on attachment 144484 [details]
Patch

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

>>> Source/WebCore/ChangeLog:10
>>> +        http://dev.w3.org/2006/webapi/FileAPI/#dfn-lastModifiedDate
>> 
>> What's the behavior of other browsers?
> 
> Safari 5.1 behaves in the same way as the current chrome does (i.e. returns "Jan 01 1970" date/time if unavailable), and as far as I checked FireFox/IE don't support it yet.
> [Some more note: there's ongoing discussion to make 'lastModifiedDate' attribute static, and in that case this test will become invalid as we will be returning the same date/time without querying (I'll deprecate this test then), but the change itself should remain valid since there will still be a chance where 'lastModifiedDate' is not available when we capture file metadata]

Thank you very much for the clarification. The change looks reasonable.

[WebKit Review Bot]

Comment on attachment 144484 [details]
Patch

Attachment 144484 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12853227

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

[WebKit Review Bot]

Created attachment 144527 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Kinuko Yasuda]

Created attachment 144528 [details]
fixed one-char typo

[Darin Adler]

Comment on attachment 144528 [details]
fixed one-char typo

Seems like it would be better to use a separate boolean or a NaN to represent the fact that a date is not available rather than using the number 0.

Generally speaking, instead of a custom binding, I’d like to see our binding machinery support this ability to return null for a date, perhaps by having the date-returning internal DOM functions return a boolean and use an out argument for the date, or something else along those lines.

[Kentaro Hara]

Comment on attachment 144528 [details]
fixed one-char typo

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

> Source/WebCore/bindings/v8/custom/V8FileCustom.cpp:45
> +    return v8DateOrNull(modifiedDate);

Just right now I've landed a patch to pass Isolate to v8DateOrNull(). Please change this to v8DateOrNull(modifiedDate, info.GetIsolate()). (Though please consider if we could kill the custom bindings as darin@ pointed out.)

[Kinuko Yasuda]

(In reply to comment #10)
> (From update of attachment 144528 [details])
> Seems like it would be better to use a separate boolean or a NaN to represent the fact that a date is not available rather than using the number 0.

I have considered using NaN, but I was afraid NaN may not be really portable (though I just found that both js and v8 code use numeric_limits<>::quiet_NaN without any guards so it looks to be ok to assume we can use it) and we need to change all related code to take care of NaN specifically.

> Generally speaking, instead of a custom binding, I’d like to see our binding machinery support this ability to return null for a date, perhaps by having the date-returning internal DOM functions return a boolean and use an out argument for the date, or something else along those lines.

Sounds reasonable.  Since current code assumes 0 is uninitialized value not only in File but at several places I'd like to land this one for now (with haraken's v8DateOrNull update) but file a new bug to track the Date issue.

[Kinuko Yasuda]

(In reply to comment #12)
> (In reply to comment #10)
> > (From update of attachment 144528 [details] [details])
> > Seems like it would be better to use a separate boolean or a NaN to represent the fact that a date is not available rather than using the number 0.
> 
> I have considered using NaN, but I was afraid NaN may not be really portable (though I just found that both js and v8 code use numeric_limits<>::quiet_NaN without any guards so it looks to be ok to assume we can use it) and we need to change all related code to take care of NaN specifically.
> 
> > Generally speaking, instead of a custom binding, I’d like to see our binding machinery support this ability to return null for a date, perhaps by having the date-returning internal DOM functions return a boolean and use an out argument for the date, or something else along those lines.
> 
> Sounds reasonable.  Since current code assumes 0 is uninitialized value not only in File but at several places I'd like to land this one for now (with haraken's v8DateOrNull update) but file a new bug to track the Date issue.

Filed a separate bug for the Date issue: http://webkit.org/b/87826

[Kinuko Yasuda]

Created attachment 144761 [details]
Patch

[Kentaro Hara]

Comment on attachment 144761 [details]
Patch

This is a great idea. Maybe we can reduce other custom bindings by using the ImplementedAs trick:)

[Kinuko Yasuda]

Thanks, credit goes to tkent-san :D  I'll be submitting if the test looks ok.

[WebKit Review Bot]

Comment on attachment 144761 [details]
Patch

Clearing flags on attachment: 144761

Committed r118920: <http://trac.webkit.org/changeset/118920>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

Skipped http/tests/local/fileapi/file-last-modified-after-delete.html on WK2, because beginDragWithFiles is not implemented there.

[Darin Adler]

Comment on attachment 144761 [details]
Patch

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

> Source/WebCore/fileapi/File.cpp:148
> +    double value = lastModifiedDate();
> +    return (!value) ? std::numeric_limits<double>::quiet_NaN() : value;

This needs a why comment. The comment in File.h is not all that useful, but it would be here where the code is doing this. My comment would be something like this:

    // The lastModifiedDate function uses 0 to represent a failure, but the DOM uses a JavaScript null.
    // Our JavaScript DOM binding maps NaN to JavaScript null, so we need to map 0 to NaN.
    // FIXME: If we used NaN instead of 0 to represent the lack of a modification time, then we would not need this any more.

[Kinuko Yasuda]

Created attachment 145003 [details]
adding why comment

[Kinuko Yasuda]

(In reply to comment #20)
> (From update of attachment 144761 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144761&action=review
> 
> > Source/WebCore/fileapi/File.cpp:148
> > +    double value = lastModifiedDate();
> > +    return (!value) ? std::numeric_limits<double>::quiet_NaN() : value;
> 
> This needs a why comment. The comment in File.h is not all that useful, but it would be here where the code is doing this. My comment would be something like this:
> 
>     // The lastModifiedDate function uses 0 to represent a failure, but the DOM uses a JavaScript null.
>     // Our JavaScript DOM binding maps NaN to JavaScript null, so we need to map 0 to NaN.
>     // FIXME: If we used NaN instead of 0 to represent the lack of a modification time, then we would not need this any more.

Uploaded a separate tiny patch to add the comment.
https://bugs.webkit.org/attachment.cgi?id=145003

[Kinuko Yasuda]

Reopening until the comment fix gets in.

[Kinuko Yasuda]

Comment on attachment 145003 [details]
adding why comment

Addressing this issue in https://bugs.webkit.org/show_bug.cgi?id=87826 rather than fixing the comment.