Bug 22432 - Add tickmark plumbing support for scrollbars
Summary: Add tickmark plumbing support for scrollbars
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Finnur Thorarinsson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-11-22 19:36 PST by Finnur Thorarinsson
Modified: 2008-11-24 15:12 PST (History)
4 users (show)

See Also:


Attachments
Provide plumbing for scrollbar tickmark support (2.65 KB, patch)
2008-11-22 19:53 PST, Finnur Thorarinsson
no flags Details | Formatted Diff | Diff
Second attempt (2.42 KB, patch)
2008-11-23 21:19 PST, Finnur Thorarinsson
no flags Details | Formatted Diff | Diff
Slightly more recent... (2.49 KB, patch)
2008-11-24 07:49 PST, Finnur Thorarinsson
darin: review-
Details | Formatted Diff | Diff
Right version this time (2.28 KB, patch)
2008-11-24 13:51 PST, Finnur Thorarinsson
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Finnur Thorarinsson 2008-11-22 19:36:26 PST
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.
Comment 1 Finnur Thorarinsson 2008-11-22 19:53:18 PST
Created attachment 25385 [details]
Provide plumbing for scrollbar tickmark support
Comment 2 Darin Adler 2008-11-23 19:56:09 PST
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.
Comment 3 Darin Fisher (:fishd, Google) 2008-11-23 20:11:36 PST
> 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.
Comment 4 Finnur Thorarinsson 2008-11-23 21:19:30 PST
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.
Comment 5 Finnur Thorarinsson 2008-11-24 07:49:52 PST
Created attachment 25430 [details]
Slightly more recent...

Slightly more recent (updated changelog, etc)
Comment 6 Darin Adler 2008-11-24 13:30:06 PST
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.
Comment 7 Finnur Thorarinsson 2008-11-24 13:51:02 PST
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...
Comment 8 Darin Adler 2008-11-24 14:31:30 PST
Comment on attachment 25449 [details]
Right version this time

r=me
Comment 9 Darin Fisher (:fishd, Google) 2008-11-24 15:12:12 PST
http://trac.webkit.org/changeset/38730