[webkit-reviews] review denied: [Bug 27451] Support HTML5 step attribute for <input type=number/range> : [Attachment 42633] Patch part 3: stepUp() and stepDown() (rev.3)

[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: Support HTML5 step attribute for <input type=number/range>
https://bugs.webkit.org/show_bug.cgi?id=27451

Attachment 42633: Patch part 3: stepUp() and stepDown() (rev.3)
https://bugs.webkit.org/attachment.cgi?id=42633&action=review

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