|Summary:||Chromium Linux does not support sliders|
|Product:||WebKit||Reporter:||Tony Chang <tony>|
|Component:||WebCore Misc.||Assignee:||Tony Chang <tony>|
|Version:||528+ (Nightly build)|
Description Tony Chang 2009-09-02 17:30:54 PDT
The paint methods for sliders are blank so we don't paint anything. We're also hitting an assert in adjustSliderThumbSize because it's not fully implemented. Upcoming patch to implement this using skia . . .
Comment 2 Tony Chang 2009-09-02 17:36:25 PDT
Created attachment 38954 [details] screenshot Here's a screenshot of what the slider looks like.
Comment 3 Tony Chang 2009-09-02 18:42:59 PDT
Chromium Linux layout test rebaselines are here: http://codereview.chromium.org/199002
Comment 4 Eric Seidel (no email) 2009-09-03 00:14:44 PDT
Comment on attachment 38953 [details] v1 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.
Comment 5 Tony Chang 2009-09-03 11:29:50 PDT
Created attachment 38998 [details] v2 Thanks for the fast review! I think I addressed all your feedback. Yes, mac and windows have existing platform code for this, linux is the only platform that this falls back to.
Comment 6 Eric Seidel (no email) 2009-09-04 01:23:37 PDT
Comment on attachment 38998 [details] v2 Looks OK.
Comment 7 Eric Seidel (no email) 2009-09-04 01:47:35 PDT
Comment on attachment 38998 [details] v2 Clearing flags on attachment: 38998 Committed r48045: <http://trac.webkit.org/changeset/48045>
Comment 8 Eric Seidel (no email) 2009-09-04 01:47:39 PDT
All reviewed patches have been landed. Closing bug.