[webkit-reviews] review granted: [Bug 42076] Keyboard operations for <input type=number> : [Attachment 61224] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has granted Kent Tamura
<tkent at chromium.org>'s request for review:
Bug 42076: Keyboard operations for <input type=number>
https://bugs.webkit.org/show_bug.cgi?id=42076

Attachment 61224: Patch
https://bugs.webkit.org/attachment.cgi?id=61224&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
WebCore/html/HTMLInputElement.cpp:2131
> +	 }
nit: please add a new line before the "if (isTextField()..."

WebCore/html/HTMLInputElement.cpp:2119
> +	 if (hasSpinButton() && evt->type() == eventNames().keydownEvent &&
evt->isKeyboardEvent()) {
nit: maybe it is better to put the isTextField case about the spin button
case since text fields are probably more common than spin buttons?

WebCore/html/HTMLInputElement.cpp:2121
> +	     if (key == "Up") {
nit: how about using a temporary to avoid having to repeat the
work of calling those functions twice?

  int step = 0;
  if (key == "Up")
      step = 1;
  else if (key == "Down")
      step = -1;
  if (step != 0) {
      stepUpFromRenderer(step);
      evt->setDefaultHandled();
      return;
  }


WebCore/html/HTMLInputElement.cpp:2399
> +  static bool isNumberCharacter(UChar ch)
nit: normally better to put static helper functions at the top of the file.
that way the flow of HTMLInputElement methods is not interrupted by a non-
member function implementation.

i'm a little surprised that there isn't already a function like this
somewhere in webkit.  i didn't see it in ASCIICType.h :(]

R=me with these minor issues addressed.