[webkit-reviews] review denied: [Bug 45803] Support keyboard operations for <input type=range> : [Attachment 67640] Patch

[bugzilla-daemon at webkit.org]

chris fleizach <cfleizach at apple.com> has denied Kent Tamura
<tkent at chromium.org>'s request for review:
Bug 45803: Support keyboard operations for <input type=range>
https://bugs.webkit.org/show_bug.cgi?id=45803

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

------- Additional Comments from chris fleizach <cfleizach at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=67640&action=review

> LayoutTests/ChangeLog:5
> +	   Support keyboard operations for <input type=range>

end in period

> WebCore/ChangeLog:5
> +	   Support keyboard operations for <input type=range>

end in period.

> WebCore/html/HTMLInputElement.cpp:2264
> +	   } else if (inputType() == RANGE) {

instead of making this if block even longer, i'd suggest putting the RANGE code
into another method for easing readability

> WebCore/html/HTMLInputElement.cpp:2265
> +	       if (key == "Up" || key == "Right" || key == "Down" || key ==
"Left") {

an early return would help here.

> WebCore/html/HTMLInputElement.cpp:2281
> +			   }

the previous 5 lines can be combined with the 5 lines in Down and Left

> WebCore/html/HTMLInputElement.cpp:2296
> +			   step = -1;

shouldn't step honor what's set in the "stepAttr" field?