Bug 87826 – File::lastModifiedDate should use NaN or separate boolean flag for null Date value

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Kinuko Yasuda]

Currently we use the value 0 (in double) to indicate uninitialized or invalid Date value at several places, but this could be a valid date value as 0 epoch time (i.e. Jan 1 1970) and the binding code just returns Date(0) unless we use custom binding, while null should be returned instead.

We should probably use NaN instead 0 (which is automatically converted to Date null) to indicate uninitialized Date value, or should have some binding machinery support to return null.

This issue is separated from issue 87709.
https://bugs.webkit.org/show_bug.cgi?id=87709#c10

"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."

[Kent Tamura]

(In reply to comment #0)
> Currently we use the value 0 (in double) to indicate uninitialized or invalid Date value at several places, but this could be a valid date value as 0 epoch time (i.e. Jan 1 1970) and the binding code just returns Date(0) unless we use custom binding, while null should be returned instead.
> 
> We should probably use NaN instead 0 (which is automatically converted to Date null) to indicate uninitialized Date value, or should have some binding machinery support to return null.

Our binding code already supports NaN -> null Date mapping.

[Kent Tamura]

(In reply to comment #1)
> (In reply to comment #0)
> > Currently we use the value 0 (in double) to indicate uninitialized or invalid Date value at several places, but this could be a valid date value as 0 epoch time (i.e. Jan 1 1970) and the binding code just returns Date(0) unless we use custom binding, while null should be returned instead.
> > 
> > We should probably use NaN instead 0 (which is automatically converted to Date null) to indicate uninitialized Date value, or should have some binding machinery support to return null.
> 
> Our binding code already supports NaN -> null Date mapping.

Ah, you mean 0->null mapping?
I think we can use ImplementedAs to avoid custom binding code.

readonly attribute [ImplementedAs=lastModifiedDateForBinding] Date lastModifiedDate;

double lastModifiedDateForBinding()
{
    double value = lastModifiedDate();
    return !value ? std::numeric_limits:quiet_nan() : value;
}

[Kinuko Yasuda]

(In reply to comment #2)
> (In reply to comment #1)
> > (In reply to comment #0)
> > > Currently we use the value 0 (in double) to indicate uninitialized or invalid Date value at several places, but this could be a valid date value as 0 epoch time (i.e. Jan 1 1970) and the binding code just returns Date(0) unless we use custom binding, while null should be returned instead.
> > > 
> > > We should probably use NaN instead 0 (which is automatically converted to Date null) to indicate uninitialized Date value, or should have some binding machinery support to return null.
> > 
> > Our binding code already supports NaN -> null Date mapping.
> 
> Ah, you mean 0->null mapping?
> I think we can use ImplementedAs to avoid custom binding code.
> 
> readonly attribute [ImplementedAs=lastModifiedDateForBinding] Date lastModifiedDate;
> 
> double lastModifiedDateForBinding()
> {
>     double value = lastModifiedDate();
>     return !value ? std::numeric_limits:quiet_nan() : value;
> }

Phew looks cool.  Let me try!

[Kinuko Yasuda]

Closing this as we could get rid of the custom binding for this (r118920)

[Kent Tamura]

(In reply to comment #4)
> Closing this as we could get rid of the custom binding for this (r118920)

However, we still have an issue that we can't represent Unix epoch with lastModifiedDate, right?

[Kinuko Yasuda]

(In reply to comment #5)
> (In reply to comment #4)
> > Closing this as we could get rid of the custom binding for this (r118920)
> 
> However, we still have an issue that we can't represent Unix epoch with lastModifiedDate, right?

Yes, though I wonder if it makes sense as file modification time... Well strictly speaking you're right as we can set such mtime by, say touch.   Ok I'm reopening this.

[Kinuko Yasuda]

Created attachment 145983 [details]
Patch

[WebKit Review Bot]

Attachment 145983 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/chromium/support/WebHTTPBody.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kinuko Yasuda]

(In reply to comment #8)
> Attachment 145983 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
> Source/WebCore/platform/chromium/support/WebHTTPBody.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
> Total errors found: 1 in 8 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Putting <public/WebHTTPBody.h> at the top of the headers seems to generate this warning.

[Kinuko Yasuda]

Chromium fix consists of multiple parts:
http://codereview.chromium.org/10447110/
http://codereview.chromium.org/10536025/

I hope other ports should be fine (watching EWS results).

[Kent Tamura]

Comment on attachment 145983 [details]
Patch

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

> Source/WebCore/fileapi/File.cpp:42
> +    return microsec * 1000.0;

It seems this can be infinity.  Is it ok?

[Kinuko Yasuda]

Created attachment 146186 [details]
Patch

[Kinuko Yasuda]

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

>> Source/WebCore/fileapi/File.cpp:42
>> +    return microsec * 1000.0;
> 
> It seems this can be infinity.  Is it ok?

infinity will be converted to null as well.

Actually I guess I didn't need this function at all. (Since nan/inf results in nan/inf whatever operation we apply)

[WebKit Review Bot]

Attachment 146186 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/chromium/support/WebHTTPBody.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kent Tamura]

Comment on attachment 146186 [details]
Patch

Looks ok.

Comment on attachment 146186 [details]
Patch

Looks like this was committed as http://trac.webkit.org/r119680 and broke the build due to adding this global initializer:

const double invalidTime = std::numeric_limits<double>::quiet_NaN();

(In reply to comment #16)
> (From update of attachment 146186 [details])
> Looks like this was committed as http://trac.webkit.org/r119680 and broke the build due to adding this global initializer:
> 
> const double invalidTime = std::numeric_limits<double>::quiet_NaN();

See <http://build.webkit.org/builders/Lion%20Release%20%28Build%29/builds/10376/steps/compile-webkit/logs/stdio/text> for example.

[Kinuko Yasuda]

(In reply to comment #17)
> (In reply to comment #16)
> > (From update of attachment 146186 [details] [details])
> > Looks like this was committed as http://trac.webkit.org/r119680 and broke the build due to adding this global initializer:
> > 
> > const double invalidTime = std::numeric_limits<double>::quiet_NaN();
> 
> See <http://build.webkit.org/builders/Lion%20Release%20%28Build%29/builds/10376/steps/compile-webkit/logs/stdio/text> for example.

Yup sorry for the breakage, I just rolled out the change.  Will rework on it.

[Kinuko Yasuda]

Created attachment 146203 [details]
Patch

[Build Bot]

Comment on attachment 146203 [details]
Patch

Attachment 146203 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12913117

[Kinuko Yasuda]

Created attachment 146231 [details]
Patch

[WebKit Review Bot]

Attachment 146231 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/chromium/support/WebHTTPBody.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kinuko Yasuda]

(In reply to comment #22)
> Attachment 146231 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
> Source/WebCore/platform/chromium/support/WebHTTPBody.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
> Total errors found: 1 in 12 files
>
> If any of these errors are false positives, please file a bug against check-webkit-style.

Filed bug 88524 for this one.

Removed the global constructor from the previous patch that caused a build breakage.

[Kinuko Yasuda]

Created attachment 146504 [details]
Patch

[Kent Tamura]

Comment on attachment 146504 [details]
Patch

Looks simple and good.

[Kinuko Yasuda]

Committed r119821: <http://trac.webkit.org/changeset/119821>

[WebKit Review Bot]

Re-opened since this is blocked by 88648

[Kinuko Yasuda]

The last commit attempt caused regression on Mac because I missed to replace isValidFileTime with null comparison in platform/FileStream.cpp. I'm going to try re-landing this with following additional change:

Source/WebCore/platform/FileStream.cpp, line 70:
-    if (expectedModificationTime) {
+    if (isValidFileTime(expectedModificationTime)) {

[Kinuko Yasuda]

Committed r120282: <http://trac.webkit.org/changeset/120282>