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

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #45235|review?                     |review+
               Flag|                            |




--- Comment #11 from Darin Adler <darin at apple.com>  2009-12-19 10:10:21 PST ---
(From update of attachment 45235)
> +JSValue jsDateOrNull(ExecState* exec, double value)
> +{
> +    if (isfinite(value))
> +        return new (exec) DateInstance(exec, value);
> +    return jsNull();
> +}

We normally prefer to do early exit for failures. So if (!isfinite) return
jsNull. We assume that the success case code might later get longer so we want
it to be non-indented.

> +double valueToDate(ExecState* exec, JSValue value)
> +{
> +    if (value.isNumber())
> +        return value.toNumber(exec);
> +    if (!value.inherits(&DateInstance::info))
> +        return std::numeric_limits<double>::quiet_NaN();
> +    return static_cast<DateInstance*>(value.toObject(exec))->internalNumber();
> +}

I think we still need to think through how we want to handle various types.
It's good that actual numbers will now work, but there are other things like
number wrappers (made with expressions like "new Number(1)") and strings.
Generally we want to consider all JavaScript types when writing a conversion
function like this.

If you have already gotten true from isNumber, you should not call toNumber.
Instead you call uncheckedGetNumber. Or you can combine the two operations into
one, using the getNumber function which returns a boolean to indicate whether
the value is a number or not.

But all those are coding tips about how to write the tightest, most efficient,
version of the code that handles only a true JavaScript number, and I am not
sure that's the behavior we want here.

> +    // NaN if the value is null, undefined, or non-Date object.
> +    double valueToDate(JSC::ExecState*, JSC::JSValue);

The comment is too specific. I would say "NaN if the value can't be converted
to a date", without giving the specifics of how the date conversion works.

I really think we need a specification of how the conversion to date works for
all types. And this unambiguous specification should be in whatever document
defines the JavaScript binding. Maybe WebIDL or HTML5. That would include both
how the conversion works, and also what behavior is expected when the value
can't be converted (a specific exception raise, perhaps) I am not happy with
all the current choices reflected in our valueToDate function.

r=me as is -- lets keep working on some of the issues I mentioned above, but
they need not hold up this patch

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