Bug 92640

Here is a Mac build with snapping turned on so we can discuss whether we do this. https://docs.google.com/open?id=0BwRi59l_ri74ZUtkTHZuLUhKUTA

Created attachment 155490 [details] Patch

Comment on attachment 155490 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155490&action=review > Source/WebCore/html/shadow/SliderThumbElement.cpp:277 > +#if ENABLE(DATALIST_ELEMENT) We should have a new function for this code. > Source/WebCore/html/shadow/SliderThumbElement.cpp:283 > + for (unsigned i = 0; Node* node = options->item(i); i++) { Please follow an idiom; for (unsigned i = 0; i < options->length(); ++i) { Your current code disturbs code readers. BTW, this code is called whenever mousemove is dispatched during the thumb dragging, and this code is O(N). We should improve the complexity. > Source/WebCore/html/shadow/SliderThumbElement.cpp:285 > + ASSERT(node->hasTagName(optionTag)); > + HTMLOptionElement* optionElement = static_cast<HTMLOptionElement*>(node); We have toHTMLOptionElement(). > Source/WebCore/html/shadow/SliderThumbElement.cpp:306 > +&& closest.isFinite() && !closest.isNaN()) wrong indentation

Comment on attachment 155490 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155490&action=review > Source/WebCore/html/shadow/SliderThumbElement.cpp:287 > + if (!input->isValidValue(optionValue)) Do we need to use HTMLInputElement functions? Because of we have stepRange for input element. We can do as follows: const Decimal rawTickValue = parseToDecimalForNumberType(optionValue); if (!rawTickValue.isFinite()) continue; const Decimal tickValue = std::max(std::min(rawTickValue, stepRange.maximum()), stepRange.minimum()); > Source/WebCore/html/shadow/SliderThumbElement.cpp:291 > + if (difference > 0 && tickValue < closestRight) You can use Decimal::isPositive() and isNegative() functions instead of comparing to zero. > Source/WebCore/html/shadow/SliderThumbElement.cpp:292 > + closestRight = tickValue; We can write this if (difference.isPositive()) closestRight = std::min(closestRight, tickValue) else if (difference.isNegative()) closestLeft = std::max(closestLeft, tickValue) else closestLeft = closestRight = tickValue; > Source/WebCore/html/shadow/SliderThumbElement.cpp:301 > + double closestFraction = stepRange.proportionFromValue(closest).toDouble(); * Can we do decimal arithmetic instead of double arithmetic? * It is better to check closest.isFinite() and skip L301-L307 if !closest.isFinite(). We can see closest is ininity when !options.length(). >> Source/WebCore/html/shadow/SliderThumbElement.cpp:306 >> +&& closest.isFinite() && !closest.isNaN()) > > wrong indentation closes.isFinite() implies !closes.isNaN(). So, we don't need to have "&& !closes.isNan()".

Comment on attachment 155490 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155490&action=review >> Source/WebCore/html/shadow/SliderThumbElement.cpp:287 >> + if (!input->isValidValue(optionValue)) > > Do we need to use HTMLInputElement functions? Because of we have stepRange for input element. We can do as follows: > const Decimal rawTickValue = parseToDecimalForNumberType(optionValue); > if (!rawTickValue.isFinite()) > continue; > const Decimal tickValue = std::max(std::min(rawTickValue, stepRange.maximum()), stepRange.minimum()); Oops, tickValue should be sanitized, so we can do const Decimal originalTickValue = parseToDecimalForNumberType(optionValue, stepRange.defaultValue()); const Decimal tickValue = stepRange.clampValue(originalTickValue);

Comment on attachment 155490 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155490&action=review >>> Source/WebCore/html/shadow/SliderThumbElement.cpp:287 >>> + if (!input->isValidValue(optionValue)) >> >> Do we need to use HTMLInputElement functions? Because of we have stepRange for input element. We can do as follows: >> const Decimal rawTickValue = parseToDecimalForNumberType(optionValue); >> if (!rawTickValue.isFinite()) >> continue; >> const Decimal tickValue = std::max(std::min(rawTickValue, stepRange.maximum()), stepRange.minimum()); > > Oops, tickValue should be sanitized, so we can do > const Decimal originalTickValue = parseToDecimalForNumberType(optionValue, stepRange.defaultValue()); > const Decimal tickValue = stepRange.clampValue(originalTickValue); We should skip the step-mismatched values, and shouldn't alter the option value here. Skip the value with isValidValue() is ok.

(In reply to comment #6) > (From update of attachment 155490 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=155490&action=review > > >>> Source/WebCore/html/shadow/SliderThumbElement.cpp:287 > >>> + if (!input->isValidValue(optionValue)) > >> > >> Do we need to use HTMLInputElement functions? Because of we have stepRange for input element. We can do as follows: > >> const Decimal rawTickValue = parseToDecimalForNumberType(optionValue); > >> if (!rawTickValue.isFinite()) > >> continue; > >> const Decimal tickValue = std::max(std::min(rawTickValue, stepRange.maximum()), stepRange.minimum()); > > > > Oops, tickValue should be sanitized, so we can do > > const Decimal originalTickValue = parseToDecimalForNumberType(optionValue, stepRange.defaultValue()); > > const Decimal tickValue = stepRange.clampValue(originalTickValue); > > We should skip the step-mismatched values, and shouldn't alter the option value here. Skip the value with isValidValue() is ok. HTMLInputElement::isValidValue() creates StepRange class many times. How about adding bool StepRange::isValidValue(const Decimal&)? It can be bool StepRange::isValidalue(const Decimal& value) { if (!value.isFinite()) return false; if (value < m_minimum || value > m_maximum) return false; return value.remainder(m_step).isZero(); }

Created attachment 155950 [details] Patch

> HTMLInputElement::isValidValue() creates StepRange class many times. I tried to avoid this by moving the code into RangeInputType and doing the equivalent to isValidValue with one StepRange.

Comment on attachment 155950 [details] Patch Attachment 155950 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13411697

Comment on attachment 155950 [details] Patch Attachment 155950 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13411695

Comment on attachment 155950 [details] Patch 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.

Created attachment 156004 [details] Patch

Comment on attachment 156004 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156004&action=review > Source/WebCore/html/HTMLInputElement.cpp:317 > +Decimal HTMLInputElement::findClosestTickMarkValue(Decimal value) nit: Is it better to use const Decimal& as other functions which take Decimal? > Source/WebCore/html/RangeInputType.cpp:340 > +static bool decimalCompare(Decimal a, Decimal b) Please use "const Decimal&" to avoid copying Decimal objects. > Source/WebCore/html/RangeInputType.cpp:363 > + m_tickMarkValues.append(stepRange.clampValue(parseToNumber(optionValue, stepRange.defaultValue()))); We don't need to call StepRange::clampValue, because isValidValue is true. > Source/WebCore/html/RangeInputType.cpp:395 > + Decimal closestLeft = middle ? m_tickMarkValues[middle - 1] : Decimal::infinity(Decimal::Negative); nit: const Decimal > Source/WebCore/html/RangeInputType.cpp:396 > + Decimal closestRight = middle != m_tickMarkValues.size() ? m_tickMarkValues[middle] : Decimal::infinity(Decimal::Positive); nit: const Decimal > Source/WebCore/html/shadow/SliderThumbElement.cpp:278 > + Decimal closest = input->findClosestTickMarkValue(value); nit: const Decimal > Source/WebCore/html/shadow/SliderThumbElement.cpp:280 > + double closestFraction = stepRange.proportionFromValue(closest).toDouble(); Is it better to do in decimal arithmetic? Could you tell me why do we use double arithmetic? For speed?

Comment on attachment 156004 [details] Patch 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.

Comment on attachment 156004 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156004&action=review > LayoutTests/fast/forms/datalist/range-snap-to-datalist.html:32 > + 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; > + } This function is not used.

Created attachment 156010 [details] Patch

> > Source/WebCore/html/shadow/SliderThumbElement.cpp:280 > > + double closestFraction = stepRange.proportionFromValue(closest).toDouble(); > > Is it better to do in decimal arithmetic? > Could you tell me why do we use double arithmetic? For speed? Should I use decimal arithmetic? My thinking was that Decimal precision is unnecessary and it would be slightly faster to use double.

(In reply to comment #15) > (From update of attachment 156004 [details]) > 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; > It is impossible to have elements in Vector due by available memory.

(In reply to comment #19) > > Still have possibility of integer overflow because sizeof(size_t) is usually same as sizeof(long). > > > > middle = left + (right - left) / 2; > > > > It is impossible to have elements in Vector due by available memory. Right. However we should always write correct code because the code might be copied, and the copy might have integer overflow.

Comment on attachment 156010 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156010&action=review > Source/WebCore/html/RangeInputType.cpp:356 > + StepRange stepRange(createStepRange(RejectAny)); This StepRange looks unnecessary because parseToNumber call below never fails.

(In reply to comment #20) > (In reply to comment #19) > > > Still have possibility of integer overflow because sizeof(size_t) is usually same as sizeof(long). > > > > > > middle = left + (right - left) / 2; > > > > > > > It is impossible to have elements in Vector due by available memory. > > Right. However we should always write correct code because the code might be copied, and the copy might have integer overflow. Can we use binarySearch in WTF/wtf/StdLibExtras.h? http://trac.webkit.org/browser/trunk/Source/WTF/wtf/StdLibExtras.h

(In reply to comment #22) > (In reply to comment #20) > > (In reply to comment #19) > > > > Still have possibility of integer overflow because sizeof(size_t) is usually same as sizeof(long). > > > > > > > > middle = left + (right - left) / 2; > > > > > > > > > > It is impossible to have elements in Vector due by available memory. > > > > Right. However we should always write correct code because the code might be copied, and the copy might have integer overflow. > > Can we use binarySearch in WTF/wtf/StdLibExtras.h? > http://trac.webkit.org/browser/trunk/Source/WTF/wtf/StdLibExtras.h I guess its possible by creating a new class that represents the space between tick marks. And doing a binary search against that.

(In reply to comment #18) > > > Source/WebCore/html/shadow/SliderThumbElement.cpp:280 > > > + double closestFraction = stepRange.proportionFromValue(closest).toDouble(); > > > > Is it better to do in decimal arithmetic? > > Could you tell me why do we use double arithmetic? For speed? > > Should I use decimal arithmetic? My thinking was that Decimal precision is unnecessary and it would be slightly faster to use double. Agree, we don't need to use decimal arithmetic. However, it seems it is better to use LayoutUnit instead of double. Because postion and trackSize are LayoutUnit and closestPostion also can be LayoutUnit.

(In reply to comment #24) > (In reply to comment #18) > > > > Source/WebCore/html/shadow/SliderThumbElement.cpp:280 > > > > + double closestFraction = stepRange.proportionFromValue(closest).toDouble(); > > > > > > Is it better to do in decimal arithmetic? > > > Could you tell me why do we use double arithmetic? For speed? > > > > Should I use decimal arithmetic? My thinking was that Decimal precision is unnecessary and it would be slightly faster to use double. > > Agree, we don't need to use decimal arithmetic. > However, it seems it is better to use LayoutUnit instead of double. Because postion and trackSize are LayoutUnit and closestPostion also can be LayoutUnit. We shouldn't use LayoutUnit here. LayoutUnit is an alias of int if !ENABLE(SUBPIXEL_LAYOUT).

(In reply to comment #25) > > However, it seems it is better to use LayoutUnit instead of double. Because postion and trackSize are LayoutUnit and closestPostion also can be LayoutUnit. > > We shouldn't use LayoutUnit here. LayoutUnit is an alias of int if !ENABLE(SUBPIXEL_LAYOUT). We discussed offline. We had better make 'closestPosition' and 'snapThreashold' LayoutUnit because they are pixel lengths.

Created attachment 156028 [details] Patch

Comment on attachment 156028 [details] Patch ok

Comment on attachment 156028 [details] Patch Clearing flags on attachment: 156028 Committed r124549: <http://trac.webkit.org/changeset/124549>

All reviewed patches have been landed. Closing bug.

Unfortunately the newly added test (fast/forms/datalist/range-snap-to-datalist.html) fails on Qt and EFL bots since r124549. I created a bug entry for the issue: https://bugs.webkit.org/show_bug.cgi?id=93074 Would you give it a look please?