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

[bugzilla-daemon at webkit.org]

TAMURA, Kent <tkent at chromium.org> has asked  for review:
Bug 27451: stepUp() and stepDown() support for <input type=number/range>
https://bugs.webkit.org/show_bug.cgi?id=27451

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

------- Additional Comments from TAMURA, Kent <tkent at chromium.org>
(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.