Bug 87826 - File::lastModifiedDate should use NaN or separate boolean flag for null Date value
Summary: File::lastModifiedDate should use NaN or separate boolean flag for null Date ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kinuko Yasuda
URL:
Keywords:
Depends on: 88498 88648
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-29 23:37 PDT by Kinuko Yasuda
Modified: 2012-06-13 23:26 PDT (History)
11 users (show)

See Also:


Attachments
Patch (8.14 KB, patch)
2012-06-06 03:44 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (7.63 KB, patch)
2012-06-06 21:00 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (16.24 KB, patch)
2012-06-06 23:00 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (12.82 KB, patch)
2012-06-07 01:45 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (12.84 KB, patch)
2012-06-08 01:23 PDT, Kinuko Yasuda
tkent: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kinuko Yasuda 2012-05-29 23:37:58 PDT
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."
Comment 1 Kent Tamura 2012-05-30 01:07:14 PDT
(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.
Comment 2 Kent Tamura 2012-05-30 01:12:57 PDT
(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;
}
Comment 3 Kinuko Yasuda 2012-05-30 01:15:50 PDT
(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!
Comment 4 Kinuko Yasuda 2012-05-30 07:02:43 PDT
Closing this as we could get rid of the custom binding for this (r118920)
Comment 5 Kent Tamura 2012-05-30 07:30:21 PDT
(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?
Comment 6 Kinuko Yasuda 2012-05-30 07:46:58 PDT
(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.
Comment 7 Kinuko Yasuda 2012-06-06 03:44:08 PDT
Created attachment 145983 [details]
Patch
Comment 8 WebKit Review Bot 2012-06-06 03:49:17 PDT
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.
Comment 9 Kinuko Yasuda 2012-06-06 03:53:50 PDT
(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.
Comment 10 Kinuko Yasuda 2012-06-06 03:55:57 PDT
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).
Comment 11 Kent Tamura 2012-06-06 17:42:46 PDT
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?
Comment 12 Kinuko Yasuda 2012-06-06 21:00:12 PDT
Created attachment 146186 [details]
Patch
Comment 13 Kinuko Yasuda 2012-06-06 21:02:45 PDT
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)
Comment 14 WebKit Review Bot 2012-06-06 21:03:03 PDT
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.
Comment 15 Kent Tamura 2012-06-06 21:13:33 PDT
Comment on attachment 146186 [details]
Patch

Looks ok.
Comment 16 mitz 2012-06-06 21:50:13 PDT
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();
Comment 17 mitz 2012-06-06 21:51:42 PDT
(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.
Comment 18 Kinuko Yasuda 2012-06-06 21:55:20 PDT
(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.
Comment 19 Kinuko Yasuda 2012-06-06 23:00:34 PDT
Created attachment 146203 [details]
Patch
Comment 20 Build Bot 2012-06-06 23:40:12 PDT
Comment on attachment 146203 [details]
Patch

Attachment 146203 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12913117
Comment 21 Kinuko Yasuda 2012-06-07 01:45:36 PDT
Created attachment 146231 [details]
Patch
Comment 22 WebKit Review Bot 2012-06-07 01:50:24 PDT
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.
Comment 23 Kinuko Yasuda 2012-06-07 04:46:30 PDT
(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.
Comment 24 Kinuko Yasuda 2012-06-08 01:23:56 PDT
Created attachment 146504 [details]
Patch
Comment 25 Kent Tamura 2012-06-08 01:50:59 PDT
Comment on attachment 146504 [details]
Patch

Looks simple and good.
Comment 26 Kinuko Yasuda 2012-06-08 04:50:36 PDT
Committed r119821: <http://trac.webkit.org/changeset/119821>
Comment 27 WebKit Review Bot 2012-06-08 05:39:06 PDT
Re-opened since this is blocked by 88648
Comment 28 Kinuko Yasuda 2012-06-13 19:51:46 PDT
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)) {
Comment 29 Kinuko Yasuda 2012-06-13 23:26:37 PDT
Committed r120282: <http://trac.webkit.org/changeset/120282>