[Webkit-unassigned] [Bug 32698] [JSC] Date binding support

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Dec 18 16:19:14 PST 2009


https://bugs.webkit.org/show_bug.cgi?id=32698


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #45166|review?                     |review-
               Flag|                            |




--- Comment #8 from Darin Adler <darin at apple.com>  2009-12-18 16:19:14 PST ---
(From update of attachment 45166)
> +    return (isnan(value) || isinf(value)) ? jsNull() : new (exec) DateInstance(exec, value);

I just remembered that there's a function called isfinite that combines both
these checks into one. I think you should use that instead.

Also, does this expression compile? I'm surprised, happily, that it works given
the two sides of the ?: expression have different types.

> +    if (value.isUndefinedOrNull() || !value.isObject() || !asObject(value)->inherits(&DateInstance::info))
> +        return std::numeric_limits<double>::quiet_NaN();

No need for a separate check of isUndefinedOrNull. Neither of those are
objects!

You can just call inherits directly on the value. The inherits function in
JSValue combines the isObject, asObject, and inherits call all into one
convenient package.

    if (!value.inherits(&DateInstance::info))

> +    // NaN if the value if null or undefined.
> +    double valueToDateWithUndefinedOrNullCheck(JSC::ExecState*, JSC::JSValue);

I think it's strange to call out the undefined/null check in the name of the
function, since you also check for any other kind of non-date object and handle
them the same way you would undefined or null.

It would be more typical in JavaScript API to try to convert the type, for
example make a date from the number if the argument is a number, and perhaps
the same thing for a string. Are you sure you have the right expected behavior
here for numeric values? Does the behavior you chose come from the HTML5 or
WebIDL specification?

> +    // FIXME: This is a temporal implementation to check Date binding.

Slight grammar mistake. This should be "temporary" instead of "temporal".

I feel a little bad about landing something that includes known-bad code for
valueAsDate, but I know it was my own suggestion, so lets do it that way.

> +#if defined(LANGUAGE_JAVASCRIPT) && LANGUAGE_JAVASCRIPT
> +                 attribute Date            valueAsDate setter raises(DOMException);
> +#endif

It's a little ugly to have to put LANGUAGE_JAVASCRIPT around any use of Date.
It probably would be easy to make the Objective-C binding handle dates, for
example.

review- because I think there are enough comments above

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.



More information about the webkit-unassigned mailing list