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

[bugzilla-daemon at webkit.org]

TAMURA, Kent <tkent at chromium.org> has asked  for review:
Bug 27451: Support HTML5 step attribute for <input type=number/range>
https://bugs.webkit.org/show_bug.cgi?id=27451

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

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