[webkit-reviews] review requested: [Bug 32698] [JSC] Date binding support : [Attachment 45235] Proposed patch (rev.3)

[bugzilla-daemon at webkit.org]

TAMURA, Kent <tkent at chromium.org> has asked  for review:
Bug 32698: [JSC] Date binding support
https://bugs.webkit.org/show_bug.cgi?id=32698

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

------- Additional Comments from TAMURA, Kent <tkent at chromium.org>
(In reply to comment #8)
> (From update of attachment 45166 [details])
> > +	 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.

Done.

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

Yes. I confirmed with Xcode and VS2005. But I changed it to a "if" statement
just in case.

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

Done.

> > +	 // 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.

Renamed to valueToDate().


> 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?

Neither HTML5 nor WebIDL mention it. I think accepting numbers is reasonable,
and changed the code for it.

> > +	 // FIXME: This is a temporal implementation to check Date binding.
> 
> Slight grammar mistake. This should be "temporary" instead of "temporal".

Fixed.

> > +#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.

Indeed.  I'll make patches for Objective-C and COM soon. I already have V8
support code.