Bug 27451

I focus on type=number and type=range in this bug entry. A work for date&time types will be another bug.

Created attachment 42007 [details] Patch part 1: Expose ECMA-262-version of dtoa() This exposed function will be used in Patch part 3.

Created attachment 42008 [details] Patch part 2: step attribute and ValidityState.stepMismatch

Created attachment 42009 [details] Patch part 3: stepUp() and stepDown() This patch has changes to JSC/V8 binding code because the parameters of stepUp() and stepDown() are optional.

Comment on attachment 42009 [details] Patch part 3: stepUp() and stepDown() > +JSValue JSHTMLInputElement::stepUp(ExecState* exec, const ArgList& args) > +{ > + HTMLInputElement* input = static_cast<HTMLInputElement*>(impl()); > + ExceptionCode ec = 0; > + int step; > + if (args.size() < 1) > + input->stepUp(1, ec); > + else { > + bool ok; > + step = args.at(0).toInt32(exec, ok); > + if (exec->hadException()) > + return jsUndefined(); > + if (!ok) > + ec = TYPE_MISMATCH_ERR; > + else > + input->stepUp(step, ec); > + } > + setDOMException(exec, ec); > + return jsUndefined(); > +} This sort of custom binding may be needed for this function for V8, but I do not think it’s needed for WebKit’s own JavaScript engine. DOM bindings already treat all arguments as optional, and missing arguments turn into the number 0 if the argument is of numeric type. The only unusual bit is the idea that an argument of the type needs to lead to TYPE_MISMATCH_ERR. Other DOM functions that take integral parameters do not do this kind of type checking; they simply convert to a number. I don't think is a good idea to write custom bindings for these functions. If we do need to move to more type checking for functions that take integral parameters in the DOM, I'd like to do it within the automatic generation mechanism. Again, the issues may be different for V8. I have not looked at the V8 DOM bindings at all.

Thank you for reviewing. I referred to JSHTMLOptionsCollection::add() for this code. (In reply to comment #5) > This sort of custom binding may be needed for this function for V8, but I do > not think it’s needed for WebKit’s own JavaScript engine. DOM bindings already > treat all arguments as optional, and missing arguments turn into the number 0 > if the argument is of numeric type. We need to distinguish input.stepUp(0) and input.stepUp(). The latter is equivalent to input.stepUp(1). So we need such custom binding code, right? > The only unusual bit is the idea that an argument of the type needs to lead to > TYPE_MISMATCH_ERR. Other DOM functions that take integral parameters do not do > this kind of type checking; they simply convert to a number. I don't think is a > good idea to write custom bindings for these functions. If we do need to move > to more type checking for functions that take integral parameters in the DOM, > I'd like to do it within the automatic generation mechanism. I checked behavior of specifying strings to some functions which requires integer parameters, and confirmed no TYPE_MISMATCH_ERR. I'll remove TYPE_MISMATCH_ERR in my patch.

Created attachment 42630 [details] Patch part 3: stepUp() and stepDown() (rev.2) - Stop to throw TYPE_MISMATCH_ERR.

Created attachment 42633 [details] Patch part 3: stepUp() and stepDown() (rev.3) Oops, forgot to remove unnecessary '#include "ExceptionCode.h"'

Comment on attachment 42633 [details] Patch part 3: stepUp() and stepDown() (rev.3) We need a test case for what happens when we pass undefined or null to stepUp. The binding currently handles that differently from just leaving out the argument, and we need to document that so we don't accidentally change it later. Also need test cases for extra arguments, and arguments of the wrong type. All this is especially important because we have hand-written bindings. Otherwise we could just fall back on "its the same as everything else" and make excuses for less thorough testing. The "rint" function is the wrong one to use for rounding. It rounds based on the current rounding mode, and we do not want the code's behavior to be different depending on the rounding mode. Instead you should use the "round" function, which always rounds in the same way. > + Vector<UChar> uchars = WTF::ecma262dtoa(num); > + return String(uchars.data(), uchars.size()); This patch does not include the change to add this function, ecma262dtoa, to WTF. I'm not certain that's a good name for the function, by the way, but we can debate that in a patch to export the function. Besides putting it into a header file it would need to be added to JavaScriptCore.exp to avoid breaking the Mac build and other changes as well. The style of calling WTF is wrong here. Functions exported from WTF are supposed to have using declarations in the header file, and callers are supposed to simply call them without an explicit WTF prefix. > + // Implementations of HTMLInputElement::stepUp() and stepDown(). > + void stepUp(int, ExceptionCode&); > + void stepDown(int, ExceptionCode&); The overloading of stepUp/stepDown where there is one version without the int that goes up by 1 and another with the int should be handled by overloading in the C++ DOM too. Eventually, the [Optional] on the argument in the IDL file could be enough to let the bindings generator handle this instead of hand-writing a binding. It is not good to hand-write a binding just for this simple reason. And I think we should look at enhancing the bindings generators to eliminate these hand-written functions. > + static String doubleToFormString(double); Typically we name functions based on what they return, not what they take. So this would be formStringFromDouble. This makes the code read more clearly, otherwise it looks like you are calling a function named "doubleXXX" and getting a string.

Created attachment 42733 [details] Patch part 3: stepUp() and stepDown() (rev.4) Thank you for the comments. (In reply to comment #9) > The "rint" function is the wrong one to use for rounding. It rounds based on > the current rounding mode, and we do not want the code's behavior to be > different depending on the rounding mode. Instead you should use the "round" > function, which always rounds in the same way. Done. > > + Vector<UChar> uchars = WTF::ecma262dtoa(num); > > + return String(uchars.data(), uchars.size()); > > This patch does not include the change to add this function, ecma262dtoa, to > WTF. I'm not certain that's a good name for the function, by the way, but we > can debate that in a patch to export the function. Besides putting it into a > header file it would need to be added to JavaScriptCore.exp to avoid breaking > the Mac build and other changes as well. Would you take a look at Part 1 and Part 2 patches in this bug please? > The style of calling WTF is wrong here. Functions exported from WTF are > supposed to have using declarations in the header file, and callers are > supposed to simply call them without an explicit WTF prefix. I couldn't find examples of "using namespace WTF" in dom/*.h and html/*.h, and found it at the top of *.cpp. I followed it in the updated patch. > > + // Implementations of HTMLInputElement::stepUp() and stepDown(). > > + void stepUp(int, ExceptionCode&); > > + void stepDown(int, ExceptionCode&); > > The overloading of stepUp/stepDown where there is one version without the int > that goes up by 1 and another with the int should be handled by overloading in > the C++ DOM too. Eventually, the [Optional] on the argument in the IDL file > could be enough to let the bindings generator handle this instead of > hand-writing a binding. It is not good to hand-write a binding just for this > simple reason. And I think we should look at enhancing the bindings generators > to eliminate these hand-written functions. Oh, I didn't know the bindings generator was so clever. I removed the custom binding code. So we don't need to test parameter errors like the following your comment, right? > We need a test case for what happens when we pass undefined or null to stepUp. > The binding currently handles that differently from just leaving out the > argument, and we need to document that so we don't accidentally change it > later. Also need test cases for extra arguments, and arguments of the wrong > type. All this is especially important because we have hand-written bindings. > Otherwise we could just fall back on "its the same as everything else" and make > excuses for less thorough testing. > > > + static String doubleToFormString(double); > > Typically we name functions based on what they return, not what they take. So > this would be formStringFromDouble. This makes the code read more clearly, > otherwise it looks like you are calling a function named "doubleXXX" and > getting a string. Done.

(In reply to comment #10) > > > + Vector<UChar> uchars = WTF::ecma262dtoa(num); > > > + return String(uchars.data(), uchars.size()); > > > > This patch does not include the change to add this function, ecma262dtoa, to > > WTF. I'm not certain that's a good name for the function, by the way, but we > > can debate that in a patch to export the function. Besides putting it into a > > header file it would need to be added to JavaScriptCore.exp to avoid breaking > > the Mac build and other changes as well. > > Would you take a look at Part 1 and Part 2 patches in this bug please? OK, I see now. Confusing! > > The style of calling WTF is wrong here. Functions exported from WTF are > > supposed to have using declarations in the header file, and callers are > > supposed to simply call them without an explicit WTF prefix. > > I couldn't find examples of "using namespace WTF" in dom/*.h and html/*.h, and > found it at the top of *.cpp. > I followed it in the updated patch. That is not what I was talking about. I was talking about using declarations in the header files inside WTF. For example, look at the bottom of ASCIICType.h. > > The overloading of stepUp/stepDown where there is one version without the int > > that goes up by 1 and another with the int should be handled by overloading in > > the C++ DOM too. Eventually, the [Optional] on the argument in the IDL file > > could be enough to let the bindings generator handle this instead of > > hand-writing a binding. It is not good to hand-write a binding just for this > > simple reason. And I think we should look at enhancing the bindings generators > > to eliminate these hand-written functions. > > Oh, I didn't know the bindings generator was so clever. I removed the custom > binding code. > So we don't need to test parameter errors like the following your comment, > right? We do need to test them. Just because the bindings generator provides the support doesn't mean we don't want test cases. We should always test such things to make sure we are getting the behavior we need.

I'll move Part 1 and 2 patches to their own bugs.

Created attachment 42926 [details] stepUp() and stepDown() (rev.5) (In reply to comment #11) > > Would you take a look at Part 1 and Part 2 patches in this bug please? > OK, I see now. Confusing! I moved them to their own bugs. > That is not what I was talking about. I was talking about using declarations in > the header files inside WTF. For example, look at the bottom of ASCIICType.h. Oh, I have understood. Updated dtoa.h in the patch part 1. > > So we don't need to test parameter errors like the following your comment, > > right? > > We do need to test them. Just because the bindings generator provides the > support doesn't mean we don't want test cases. We should always test such > things to make sure we are getting the behavior we need. I added tests for null/undefined/extra arguments.

Comment on attachment 42926 [details] stepUp() and stepDown() (rev.5) I need to update the patch due to the ecma262dtoa() change.

Created attachment 43489 [details] Proposed patch (rev.6) - sync with the function name change of ecma262dtoa() - improve the test result readability - RenderSlider uses the double-to-string function introduced by this change

Comment on attachment 43489 [details] Proposed patch (rev.6) > +Tests stepDown()/stepUp() for unsuppoted typpes A couple spelling errors. > +void HTMLInputElement::addForNumber(double step, double count, ExceptionCode& ec) This function name doesn't seem great. I would call it something more like "applyStep". Having "for number" in the name of a function that works on both number and range seems like an immediate mistake. > + double current = 0.0; > + if (!formStringToDouble(value(), &current)) { > + ec = INVALID_STATE_ERR; > + return; > + } No need to initialize "current" here. > + double minimum = 0.0; > + if (formStringToDouble(getAttribute(minAttr), &minimum)) { > + if (newValue < minimum) { > + ec = INVALID_STATE_ERR; > + return; > + } > + } > + double n = round((newValue - minimum) / step); > + newValue = minimum + n * step; We might want to initialize minimum to "defaultMinimum" rather than an explicit 0. I think the code would read more clearly as a single-line expression without the local variable "n". > + double maximum = 0.0; > + if (formStringToDouble(getAttribute(maxAttr), &maximum)) { > + if (newValue > maximum) { > + ec = INVALID_STATE_ERR; > + return; > + } > + } No need to initialize "maximum" here. I think it would be better to factor things so that the step, minimum, maximum fetching was in separate functions. I don't see any reason to have entirely separate code for range and number just because the minimum and maximum are computed differently for those two. I would have minimum and maximum functions, and return infinity if there is no maximum. Or a getMaximum function that returns a boolean. Generally speaking it's annoying to have all the logic twice in this function! I think you should just call getAllowedValueStep inside the applyStep function, and make the stepUp and stepDown functions just pass things through to applyStep. > +String HTMLInputElement::formStringFromDouble(double num) Please use whole words. Use "number" instead of "num". > + // Converts the specified number to a string. This is an implementation of > + // HTML5's "algorithm to convert a number to a string" for NUMBER/RANGE types. > + static String formStringFromDouble(double); We use one space after a period. I'll say review- because I think at least the name "addForNumber" needs to change.

Created attachment 43535 [details] Proposed patch (rev.7) (In reply to comment #16) > (From update of attachment 43489 [details]) > > +Tests stepDown()/stepUp() for unsuppoted typpes > > A couple spelling errors. Fixed. > > +void HTMLInputElement::addForNumber(double step, double count, ExceptionCode& ec) > > This function name doesn't seem great. I would call it something more like > "applyStep". Having "for number" in the name of a function that works on both > number and range seems like an immediate mistake. Renamed to applyStepForNumberOrRange(). > > + double current = 0.0; > > + if (!formStringToDouble(value(), &current)) { > > + ec = INVALID_STATE_ERR; > > + return; > > + } > > No need to initialize "current" here. Fixed. > > + double minimum = 0.0; > > + if (formStringToDouble(getAttribute(minAttr), &minimum)) { > > + if (newValue < minimum) { > > + ec = INVALID_STATE_ERR; > > + return; > > + } > > + } > > + double n = round((newValue - minimum) / step); > > + newValue = minimum + n * step; > > We might want to initialize minimum to "defaultMinimum" rather than an explicit > 0. Removed the code. > I think the code would read more clearly as a single-line expression without > the local variable "n". Done. > > + double maximum = 0.0; > > + if (formStringToDouble(getAttribute(maxAttr), &maximum)) { > > + if (newValue > maximum) { > > + ec = INVALID_STATE_ERR; > > + return; > > + } > > + } > > No need to initialize "maximum" here. Removed the code. > I think it would be better to factor things so that the step, minimum, maximum > fetching was in separate functions. I don't see any reason to have entirely > separate code for range and number just because the minimum and maximum are > computed differently for those two. I would have minimum and maximum functions, > and return infinity if there is no maximum. Or a getMaximum function that > returns a boolean. > > Generally speaking it's annoying to have all the logic twice in this function! I made some changes to unify the code for NUMBER and RANGE. - Renamd rangeMinimum() and rangeMaximum() to minimum() and maximum(), and add support for NUMBER. - Introduce stepBase(). Now rangeUnderflow(), rangeOverflow() and applyStepForNumberOrRange() get cleaner :-) > I think you should just call getAllowedValueStep inside the applyStep function, > and make the stepUp and stepDown functions just pass things through to > applyStep. Done. > > +String HTMLInputElement::formStringFromDouble(double num) > > Please use whole words. Use "number" instead of "num". Done. > > + // Converts the specified number to a string. This is an implementation of > > + // HTML5's "algorithm to convert a number to a string" for NUMBER/RANGE types. > > + static String formStringFromDouble(double); > > We use one space after a period. Done.

Created attachment 43536 [details] Proposed patch (rev.8) Oops, I uploaded an old patch.

Attachment 43536 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/html/HTMLInputElement.cpp:307: One line control clauses should not use braces. [whitespace/braces] [4] Done processing WebCore/html/HTMLInputElement.cpp Done processing WebCore/rendering/RenderSlider.cpp Done processing WebCore/html/HTMLInputElement.h Total errors found: 1

Created attachment 44047 [details] Proposed patch (rev.9) > WebCore/html/HTMLInputElement.cpp:307: One line control clauses should not use > braces. [whitespace/braces] [4] Fixed.

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

Comment on attachment 44047 [details] Proposed patch (rev.9) > + static const double defaultMinimumForNumber = -DBL_MAX; > // The range type's "default minimum" is 0. > + static const double defaultMinimumForRange = 0.0; > + static const double defaultMaximumForNumber = DBL_MAX; > // The range type's "default maximum" is 100. > + static const double defaultMaximumForRange = 100.0; > + static const double defaultStepBase = 0.0; I think these constants should go at the top of the file instead of in the various functions that use them. > + // A remedy for the inconsistent min/max values for RANGE. > + // Sets the maxmimum to the default or the minimum value. > + const double min = minimum(); Normally we do not use const for local variables in WebKit. > + if (max < min) > + max = min < defaultMaximum ? defaultMaximum : min; This could be written more cleanly using the max function: max = std::max(std::max(min, defaultMaximum), max); The idea is that you use the highest of the three: min, max, and defaultMaximum. Also, if the local variable was not named max, you would not need the "std::" prefixes either. r=me as is -- maybe you can adopt my suggestions in a later patch

I modified for Darin's suggestions, and committed manually as r51822.