Bug 28928 – Chromium Linux does not support sliders

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Tony Chang]

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 . . .

[Tony Chang]

Created attachment 38953 [details]
v1

[Tony Chang]

Created attachment 38954 [details]
screenshot

Here's a screenshot of what the slider looks like.

[Tony Chang]

Chromium Linux layout test rebaselines are here:
http://codereview.chromium.org/199002

[Eric Seidel (no email)]

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.

[Tony Chang]

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.

[Eric Seidel (no email)]

Comment on attachment 38998 [details]
v2

Looks OK.

[Eric Seidel (no email)]

Comment on attachment 38998 [details]
v2

Clearing flags on attachment: 38998

Committed r48045: <http://trac.webkit.org/changeset/48045>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.