Bug 152181 – Refactoring HTMLMediaElement getStartDate() implementation

• Status: NEW
• Component: WebCore JavaScript

[Pascal Jacquemart]

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

[Pascal Jacquemart]

Created attachment 267179 [details]
Just checking if this change may not break other tests

Just checking if this change may not break other tests

[Darin Adler]

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.

[Pascal Jacquemart]

Created attachment 267745 [details]
Just checking if this change may not break other tests 2

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Pascal Jacquemart]

Created attachment 267750 [details]
Just checking if this change may not break other tests 3

[Pascal Jacquemart]

Created attachment 268284 [details]
For testing

[Eric Carlson]

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?

[Darin Adler]

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.

[Pascal Jacquemart]

Created attachment 270113 [details]
Make sure HTMLMediaElement getStartDate() always returns a valid Date object

[Eric Carlson]

Comment on attachment 270113 [details]
Make sure HTMLMediaElement getStartDate() always returns a valid Date object

Thank you for sticking with this Pascal!

[Darin Adler]

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?

[Radar WebKit Bug Importer]

<rdar://problem/25513326>