Bug 22432

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

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.

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

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.

Created attachment 25430 [details] Slightly more recent... Slightly more recent (updated changelog, etc)

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.

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 on attachment 25449 [details] Right version this time r=me