[Webkit-unassigned] [Bug 42076] Keyboard operations for <input type=number>

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=42076


Darin Fisher (:fishd, Google) <fishd at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #61224|review?                     |review+, commit-queue-
               Flag|                            |




--- Comment #4 from Darin Fisher (:fishd, Google) <fishd at chromium.org>  2010-07-16 10:48:31 PST ---
(From update of attachment 61224)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.