Bug 28928

Summary: Chromium Linux does not support sliders
Product: WebKit Reporter: Tony Chang <tony>
Component: WebCore Misc.Assignee: Tony Chang <tony>
Severity: Normal CC: eric
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Description Flags
eric: review-
v2 none

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 1 Tony Chang 2009-09-02 17:35:28 PDT
Created attachment 38953 [details]
Comment 2 Tony Chang 2009-09-02 17:36:25 PDT
Created attachment 38954 [details]

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:
Comment 4 Eric Seidel (no email) 2009-09-03 00:14:44 PDT
Comment on attachment 38953 [details]

There are a few style nits:
 52         virtual void adjustSliderThumbSize(RenderObject* object) const;
no arg name needed.

 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]

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]

Looks OK.
Comment 7 Eric Seidel (no email) 2009-09-04 01:47:35 PDT
Comment on attachment 38998 [details]

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.