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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted 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 44047: Proposed patch (rev.9)
https://bugs.webkit.org/attachment.cgi?id=44047&action=review

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