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."
(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.
(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; }
(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!
Closing this as we could get rid of the custom binding for this (r118920)
(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?
(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.
Created attachment 145983 [details] Patch
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.
(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.
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 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?
Created attachment 146186 [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? 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)
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 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.
(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.
Created attachment 146203 [details] Patch
Comment on attachment 146203 [details] Patch Attachment 146203 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12913117
Created attachment 146231 [details] Patch
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.
(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.
Created attachment 146504 [details] Patch
Comment on attachment 146504 [details] Patch Looks simple and good.
Committed r119821: <http://trac.webkit.org/changeset/119821>
Re-opened since this is blocked by 88648
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)) {
Committed r120282: <http://trac.webkit.org/changeset/120282>