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

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


> Source/WebCore/html/shadow/SliderThumbElement.cpp:154
> +		       if (trackRenderer)
> +			  
trackRenderer->style()->setHeight(thumbRenderer->style()->height());
> +#if ENABLE(DATALIST)

I'd like to change this to

  Length height = thumbRenderer->style()->height();
  if (trackRenderr)
      trackRenderer->style()->setHeight(height);

and

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

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

> Source/WebCore/rendering/RenderTheme.h:217
> +    virtual IntSize sliderTickSize() const;

Unclear if this is for horizontal or vertical.

> Source/WebCore/rendering/RenderTheme.h:218
> +    virtual IntSize sliderThumbSize() const;

This should be integrated with RenderTheme::adjustSliderThumbSize().

> Source/WebCore/rendering/RenderTheme.h:219
> +    virtual int sliderTickOffsetFromTrackCenter() const;

We have an assumption that abs(sliderTickOffsetFromTrackCenter) * 2 >
height-of-slider-thumb, right?	We should add a comment.