Bug 22432 – Add tickmark plumbing support for scrollbars

• Component: New Bugs
• Status: RESOLVED FIXED

[Finnur Thorarinsson]

Dave Hyatt and Darin Fischer (Google) have been in communication about adding tickmark support for WebKit's scrollbar. I have been implementing this on the Chromium side and this bug is meant to track the WebKit side of that effort.

During my implementation I found that very little needs to change in WebKit to support this; effectively only three lines or so. I will be uploading a simple patch shortly.

[Finnur Thorarinsson]

Created attachment 25385 [details]
Provide plumbing for scrollbar tickmark support

[Darin Adler]

Comment on attachment 25385 [details]
Provide plumbing for scrollbar tickmark support

> The painting code for the scrollbar
> +        just needed to call paintTickmarks at the right time and a default no-op implementation is
> +        provided, which the ports will need to override.

Makes sense, and it seems fine to add this hook.

I don't understand why tick marks are specific to vertical scrollbars, though.

>           This also provides a paintTickmark (singular)
> +        function definition, which paintTickmarks should call and the port also needs to override.

I don't understand how this is helpful. If only the paintTickmarks function is going to call paintTickmark, then there's no reason for the paintTickmark function to be virtual or to be declared in the base class.

Separate question: Is this going to be part of the Windows scrollbar theme, or is there going to be a separate one for Chrome?

I'm not going to review this + or - because of my questions above.

[Darin Fisher (:fishd, Google)]

> Separate question: Is this going to be part of the Windows scrollbar theme, or
> is there going to be a separate one for Chrome?

What we have right now is ScrollbarThemeChromium.cpp defined within PLATFORM(CHROMIUM).  We are not building with PLATFORM(WIN), PLATFORM(MAC), or PLATFORM(GTK).  You can see this here:

http://src.chromium.org/viewvc/chrome/trunk/src/webkit/port/platform/chromium/ScrollbarThemeChromium.cpp?view=markup
http://src.chromium.org/viewvc/chrome/trunk/src/webkit/port/platform/chromium/ScrollbarThemeChromiumWin.cpp?view=markup
http://src.chromium.org/viewvc/chrome/trunk/src/webkit/port/platform/chromium/ScrollbarThemeChromiumLinux.cpp?view=markup

Once we land PLATFORM(CHROMIUM), our plan is to work to unify the scrollbar theme code across the various ports.  This would likely involve inventing a WIN_THEME define, and so on.  It is difficult to share the code until we have everything living in svn.webkit.org.

[Finnur Thorarinsson]

Created attachment 25409 [details]
Second attempt

Darin Adler,

Thank you for taking a stab at this on a weekend day.

You are right, the paintTickmarks call doesn't need to be specific to the vertical scrollbars. We (Chromium) don't want the tickmarks for horizontal scrollbars, but that doesn't mean others wont. I have removed that check.

I also removed paintTickmark (singular). I thought it might make sense to provide guidance for the implementors of the tickmarks (for a given port), but you are right: we don't need that in the base class.

[Finnur Thorarinsson]

Created attachment 25430 [details]
Slightly more recent...

Slightly more recent (updated changelog, etc)

[Darin Adler]

Comment on attachment 25430 [details]
Slightly more recent...

This doesn't have the changes you described. It looks like an even older version of the patch than the original one you posted for review, without your name or email address.

[Finnur Thorarinsson]

Created attachment 25449 [details]
Right version this time

Ooops, my bad. Too early in the morning for me to be working. Sorry about that.  :)

Here is the right file...

[Darin Adler]

Comment on attachment 25449 [details]
Right version this time

r=me

[Darin Fisher (:fishd, Google)]

http://trac.webkit.org/changeset/38730