[webkit-reviews] review denied: [Bug 87844] Implement painting slider tick marks : [Attachment 145751] 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 87844: Implement painting slider tick marks
https://bugs.webkit.org/show_bug.cgi?id=87844

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

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


> Source/WebCore/html/shadow/SliderThumbElement.cpp:168
> +			   style()->setHeight(Length(trackHeight, Fixed));
> +		       } else
> +#endif
> +			   style()->setHeight(height);

height = Length(trachHeight, Fixed));
}
style()->setHeight(height);

> Source/WebCore/rendering/RenderTheme.cpp:992
> +    int tickRegionMargin = (thumbSize.width() - tickSize.width()) / 2.0;

You assume horizontal thumb width == vertical thumb height.  I don't think we
should introduce such restriction.  We should have
RenderThem::sliderThmbSize(ControlPart) or we should obtain the thumb size from
an existing renderer.

> Source/WebCore/rendering/RenderTheme.cpp:994
> +    int tickRegionSideMargin = rect.x() + tickRegionMargin;
> +    int tickRegionWidth = rect.width() - tickRegionMargin * 2 -
tickSize.width();

The initial values are not used.

> Source/WebCore/rendering/RenderTheme.cpp:1014
> +	   double value =
parseToDoubleForNumberType(input->sanitizeValue(optionElement->value()),
std::numeric_limits<double>::quiet_NaN());

You can omit the last argument of parseToDoubleForNumberType().
Also, you have to check isfinite(value).