Bug 32698

Created attachment 45128 [details] Proposed patch Generated code will be: JSValue jsHTMLInputElementValueAsDate(ExecState* exec, const Identifier&, const PropertySlot& slot) { JSHTMLInputElement* castedThis = static_cast<JSHTMLInputElement*>(asObject(slot.slotBase())); UNUSED_PARAM(exec); HTMLInputElement* imp = static_cast<HTMLInputElement*>(castedThis->impl()); return (isnan(imp->valueAsDate()) || isinf(imp->valueAsDate())) ? jsNull() : new (exec) DateInstance(exec, imp->valueAsDate()); } void setJSHTMLInputElementValueAsDate(ExecState* exec, JSObject* thisObject, JSValue value) { HTMLInputElement* imp = static_cast<HTMLInputElement*>(static_cast<JSHTMLInputElement*>(thisObject)->impl()); ExceptionCode ec = 0; imp->setValueAsDate(value.isUndefinedOrNull() ? std::numeric_limits<double>::quiet_NaN() : static_cast<DateInstance*>(value.toObject(exec))->internalNumber(), ec); setDOMException(exec, ec); }

style-queue ran check-webkit-style on attachment 45128 [details] without any errors.

Comment on attachment 45128 [details] Proposed patch > + return "$value.isUndefinedOrNull() ? std::numeric_limits<double>::quiet_NaN() : static_cast<DateInstance*>($value.toObject(exec))->internalNumber()" if $type eq "Date"; This isn't right. We can't just call toObject and then cast to DateInstance* without some sort of type check. I also suggest considering putting this logic into a function in JSDOMBinding.h and just have the generated code call the function instead of putting the logic into the bindings generator. I think the patch needs to include one use of this binding so we can implement the test cases we'll need.

(In reply to comment #3) > This isn't right. We can't just call toObject and then cast to DateInstance* > without some sort of type check. I also suggest considering putting this logic > into a function in JSDOMBinding.h and just have the generated code call the > function instead of putting the logic into the bindings generator. Ok, I'll update the patch. > I think the patch needs to include one use of this binding so we can implement > the test cases we'll need. As you can guess, I'm implementing HTMLInputElement::valueAsDate. It will have code which is more complex than this patch. Is it ok to implement valueAsDate() and setValueAsDate() as just placeholder functions?

Created attachment 45166 [details] Proposed patch (rev.2) * Follow the Darin's comments. Produced code will be: JSValue jsHTMLInputElementValueAsDate(ExecState* exec, const Identifier&, const PropertySlot& slot) { JSHTMLInputElement* castedThis = static_cast<JSHTMLInputElement*>(asObject(slot.slotBase())); UNUSED_PARAM(exec); HTMLInputElement* imp = static_cast<HTMLInputElement*>(castedThis->impl()); return jsDateOrNull(exec, imp->valueAsDate()); } void setJSHTMLInputElementValueAsDate(ExecState* exec, JSObject* thisObject, JSValue value) { HTMLInputElement* imp = static_cast<HTMLInputElement*>(static_cast<JSHTMLInputElement*>(thisObject)->impl()); ExceptionCode ec = 0; imp->setValueAsDate(valueToDateWithUndefinedOrNullCheck(exec, value), ec); setDOMException(exec, ec); }

style-queue ran check-webkit-style on attachment 45166 [details] without any errors.

Attachment 45166 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/135251

Comment on attachment 45166 [details] Proposed patch (rev.2) > + 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

Created attachment 45235 [details] Proposed patch (rev.3) (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.

style-queue ran check-webkit-style on attachment 45235 [details] without any errors.

Comment on attachment 45235 [details] Proposed patch (rev.3) > +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

I'll commit manually with the following changes. (In reply to comment #11) > (From update of attachment 45235 [details]) > > +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. Fixed. > > + if (value.isNumber()) > > + return value.toNumber(exec); > > 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. I changed it to uncheckedGetNumber(). > > + // 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. Fixed. > 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. I made a bug for this issue: Bug#32809. And bugs for other bindings: V8: Bug#32699 ObjC: Bug#32810 COM: Bug#32811

Landed as r52434.