[webkit-reviews] review denied: [Bug 32698] [JSC] Date binding support : [Attachment 45166] Proposed patch (rev.2)

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied TAMURA, Kent <tkent at chromium.org>'s
request for review:
Bug 32698: [JSC] Date binding support
https://bugs.webkit.org/show_bug.cgi?id=32698

Attachment 45166: Proposed patch (rev.2)
https://bugs.webkit.org/attachment.cgi?id=45166&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    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