[webkit-reviews] review denied: [Bug 92640] Slider should snap to datalist tick marks : [Attachment 156004] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Keishi Hattori
<keishi at webkit.org>'s request for review:
Bug 92640: Slider should snap to datalist tick marks
https://bugs.webkit.org/show_bug.cgi?id=92640

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

------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=156004&action=review


> Source/WebCore/html/InputType.cpp:914
> +    return Decimal::nan();

ASSERT_NOT_REACHED().

> Source/WebCore/html/RangeInputType.cpp:380
> +	   middle = (static_cast<long>(left) + static_cast<long>(right)) / 2;

Still have possibility of integer overflow because sizeof(size_t) is usually
same as sizeof(long).

middle = left + (right - left) / 2;

> LayoutTests/fast/forms/datalist/range-snap-to-datalist.html:66
> +<script>
> +    var input = document.getElementById("input");
> +    var thumbWidth = 15;
> +    var halfThumbWidth = (thumbWidth - 1) / 2;
> +    function clickSlider(offsetLeft) {
> +	   var centerY = input.offsetTop + input.offsetHeight / 2;
> +	   eventSender.mouseMoveTo(input.offsetLeft + offsetLeft, centerY);
> +	   eventSender.mouseDown();
> +	   eventSender.mouseUp();
> +    }
> +    function resetSliderPosition() {
> +	   clickSlider(0);
> +	   shouldBe('input.value', '"0"');
> +    }
> +    function expectedValueForPosition(pos) {
> +	   clickSlider(pos);
> +	   var value = Math.round((pos - halfThumbWidth - 0.5) /
(input.offsetWidth - thumbWidth) * (input.max - input.min) + input.min);
> +	   value = Math.max(Math.min(value, input.max), input.min);
> +	   return value;
> +    }
> +    resetSliderPosition();
> +    clickSlider(45);
> +    shouldBeTrue('parseInt(input.value, 10) < 500');
> +    resetSliderPosition();
> +    clickSlider(46);
> +    shouldBeTrue('parseInt(input.value, 10) < 500');
> +    resetSliderPosition();
> +    clickSlider(47);
> +    shouldBeTrue('parseInt(input.value, 10) < 500');
> +    resetSliderPosition();
> +    clickSlider(48);
> +    shouldBe('input.value', "'500'");
> +    resetSliderPosition();
> +    clickSlider(49);
> +    shouldBe('input.value', "'500'");
> +    resetSliderPosition();
> +    clickSlider(50);
> +    shouldBe('input.value', "'500'");
> +    resetSliderPosition();
> +    clickSlider(51);
> +    shouldBe('input.value', "'500'");
> +    resetSliderPosition();
> +    clickSlider(52);
> +    shouldBe('input.value', "'500'");
> +    resetSliderPosition();
> +    clickSlider(53);
> +    shouldBeTrue('parseInt(input.value, 10) > 500');
> +    resetSliderPosition();
> +    clickSlider(54);
> +    shouldBeTrue('parseInt(input.value, 10) > 500');
> +    resetSliderPosition();
> +    clickSlider(55);
> +    shouldBeTrue('parseInt(input.value, 10) > 500');
> +</script>

nit: Indenting everything is not helpful.