[webkit-reviews] review denied: [Bug 27451] stepUp() and stepDown() support for <input type=number/range> : [Attachment 43489] Proposed patch (rev.6)

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied TAMURA, Kent <tkent at chromium.org>'s
request for review:
Bug 27451: stepUp() and stepDown() support for <input type=number/range>
https://bugs.webkit.org/show_bug.cgi?id=27451

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

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