[webkit-reviews] review denied: [Bug 92640] Slider should snap to datalist tick marks : [Attachment 155950] 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 155950: Patch
https://bugs.webkit.org/attachment.cgi?id=155950&action=review

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


> Source/WebCore/html/HTMLInputElement.cpp:320
> +Decimal HTMLInputElement::findClosestTickMarkValue(Decimal value)
> +{
> +    return m_inputType->findClosestTickMarkValue(value);
> +}
> +

Please move it into the existing #if ENABLE(DATALIST_ELEMENT) block.

> Source/WebCore/html/HTMLInputElement.h:77
> +#if ENABLE(DATALIST_ELEMENT)
> +    Decimal findClosestTickMarkValue(Decimal);
> +#endif

Please move the declaration into the existing #if ENABLE(DATALIST_ELEMENT)
block.

> Source/WebCore/html/RangeInputType.cpp:58
> +#if ENABLE(TOUCH_EVENTS)

Maybe DATALIST_ELEMENT ?

> Source/WebCore/html/RangeInputType.cpp:352
> +    Vector<Decimal> tickValues;

This is not used.

> Source/WebCore/html/RangeInputType.cpp:366
> +	   String optionValue = optionElement->value();
> +	   if (typeMismatchFor(optionValue))
> +	       continue;
> +	   Decimal rawOptionValue = parseToNumber(optionValue, Decimal::nan());

> +	   if (!rawOptionValue.isFinite()
> +	       || stepRange.stepMismatch(rawOptionValue)
> +	       || rawOptionValue < stepRange.minimum()
> +	       || rawOptionValue > stepRange.maximum())

We should use HTMLInputElement::isValidValue() because
 - This is a duplication of HTMLInputElement::isValidValue(). It's hard to
maintain both to be synchronized forever.
 - Now we have a cache of datalist values. The cost of StepRange creation is
not important.

If you'd like to improve the performance, you should improve isValidValue().

> Source/WebCore/html/RangeInputType.cpp:385
> +	   middle = (left + right) / 2;

possible integer overflow.

> Source/WebCore/html/RangeInputType.h:80
>  #endif
> +
> +    bool m_tickMarkValuesDirty;
> +    Vector<Decimal> m_tickMarkValues;

They should be in #if ENABLE(DATALIST_ELEMENT)

> Source/WebCore/html/shadow/SliderThumbElement.cpp:241
>  
> +
> +

this change is unnecessary.