The spec says [1]: The getStartDate() method must return a new Date object representing the current timeline offset. where "a new Date object" is defined as: When this specification requires a user agent to create a Date object representing a particular time (which could be the special value Not-a-Number) ... Current implementation have some JSC quirks to return NaN directly instaead of a valid Date object
Created attachment 267179 [details] Just checking if this change may not break other tests Just checking if this change may not break other tests
Comment on attachment 267179 [details] Just checking if this change may not break other tests View in context: https://bugs.webkit.org/attachment.cgi?id=267179&action=review This patch should only contain the change to HTMLMediaElement.idl. Deleting all the unneeded code can be done in a follow-up that does not change behavior. > Source/WebCore/bindings/js/JSDOMBinding.cpp:124 > JSValue jsDateOrNull(ExecState* exec, double value) > { > - if (!std::isfinite(value)) > + if (std::isinf(value)) > return jsNull(); > + if (std::isnan(value)) > + return DateInstance::create(exec, exec->lexicalGlobalObject()->dateStructure()); > return DateInstance::create(exec->vm(), exec->lexicalGlobalObject()->dateStructure(), value); > } This changes the behavior for every callers of jsDateOrNull when the value is a NaN. We do not want to make that global change.
Created attachment 267745 [details] Just checking if this change may not break other tests 2
Comment on attachment 267745 [details] Just checking if this change may not break other tests 2 Attachment 267745 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/588913 New failing tests: imported/w3c/web-platform-tests/html/dom/interfaces.html http/tests/media/hls/video-controller-getStartDate.html http/tests/local/fileapi/file-last-modified.html imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/text.html imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/hidden.html
Created attachment 267746 [details] Archive of layout-test-results from ews103 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 267745 [details] Just checking if this change may not break other tests 2 Attachment 267745 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/588915 New failing tests: imported/w3c/web-platform-tests/html/dom/interfaces.html imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/hidden.html imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/text.html http/tests/media/hls/video-controller-getStartDate.html
Created attachment 267747 [details] Archive of layout-test-results from ews107 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 267745 [details] Just checking if this change may not break other tests 2 Attachment 267745 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/588914 New failing tests: http/tests/local/fileapi/file-last-modified.html imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/hidden.html imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/text.html http/tests/media/hls/video-controller-getStartDate.html
Created attachment 267748 [details] Archive of layout-test-results from ews113 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 267750 [details] Just checking if this change may not break other tests 3
Created attachment 268284 [details] For testing
Comment on attachment 268284 [details] For testing This looks fine, but should this patch also change HTMLMediaElement.idl because getStartDate will now always return a valid Date object?
Comment on attachment 268284 [details] For testing View in context: https://bugs.webkit.org/attachment.cgi?id=268284&action=review > Source/WebCore/bindings/js/JSDOMBinding.cpp:120 > JSValue jsDateOrNaN(ExecState* exec, double value) > { > - if (std::isnan(value)) > - return jsDoubleNumber(value); > - return jsDateOrNull(exec, value); > + return DateInstance::create(exec->vm(), exec->lexicalGlobalObject()->dateStructure(), value); > } If this function no longer returns a NaN when passed a NaN, then it should not be named jsDateOrNaN. We should not change what it does without changing its name.
Created attachment 270113 [details] Make sure HTMLMediaElement getStartDate() always returns a valid Date object
Comment on attachment 270113 [details] Make sure HTMLMediaElement getStartDate() always returns a valid Date object Thank you for sticking with this Pascal!
Comment on attachment 270113 [details] Make sure HTMLMediaElement getStartDate() always returns a valid Date object View in context: https://bugs.webkit.org/attachment.cgi?id=270113&action=review This doesn’t seem right. Is it really acceptable to create a date object with the value NaN? Why no new test case? What’s the point of making this change if it doesn’t fix a bug? > Source/WebCore/bindings/js/JSDOMBinding.cpp:120 > +JSValue jsDate(ExecState* exec, double value) > +{ > + return DateInstance::create(exec->vm(), exec->lexicalGlobalObject()->dateStructure(), value); > +} What does this function do if it’s passed null or infinity? Is it legal to create a date instance with such values? Is it a programming error to call this? If so, then should something ASSERT(std::isfinite(value))? Maybe DateInstance::create already does that?
<rdar://problem/25513326>