[webkit-reviews] review denied: [Bug 28928] Chromium Linux does not support sliders : [Attachment 38953] v1

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Tony Chang (Google)
<tony at chromium.org>'s request for review:
Bug 28928: Chromium Linux does not support sliders
https://bugs.webkit.org/show_bug.cgi?id=28928

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
There are a few style nits:
 52	    virtual void adjustSliderThumbSize(RenderObject* object) const;
no arg name needed.

name:
 770	 int vertical_center = rect.y() + rect.height() / 2;

We could use named colors here, if Skia has them:
     SkPaint paint;
 794	 if (hovered)
 795	     paint.setARGB(0xff, 0xff, 0xff, 0xff);
 796	 else
 797	     paint.setARGB(0xff, 0xf4, 0xf2, 0xef);
paint = hovered ? white : grey;
Color has some if that's of use.

IntRect might make this easier:
 799	 SkIRect skrect;
 800	 if (vertical)
 801	     skrect.set(rect.x(), rect.y(), midx + 1, rect.y() +
rect.height());
 802	 else
 803	     skrect.set(rect.x(), rect.y(), rect.x() + rect.width(), midy + 1);


IntRect newRect(rect.location(), vertical ? IntSize(midx+1, rect.bottom()) :
IntSize(rect.right(), midy + 1));

I'm not sure if bottom/right exist, but maybe that's cleaner?  Not sure.

SkIRect transparently converts to IntRect, likewise SkRect to FloatRect, and
back and forth.  So I tend to use whichever results in cleaner code.

I guess Windows and Mac all use custom platform code for this stuff?

In general this looks fine.  r- for the above nits.