Use new WebThemeEngine api on chromium / linux to draw scrollbars.
Created attachment 70409 [details] Patch
Created attachment 70410 [details] Patch
The WebThemeEngine api was added to the chromium linux port in https://bugs.webkit.org/show_bug.cgi?id=47278 Support has been added to chromium as of http://src.chromium.org/viewvc/chrome?view=rev&revision=62093 This patch makes the chromium linux scrollbar implementation use that api. It also removes the top level WinThemeEngine.h file which has been moved to the win subdir. The top level include is no longer referenced from chromium.
Comment on attachment 70410 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=70410&action=review Overall, looks ok. Mostly naming and style nits. > WebCore/platform/chromium/ChromiumBridge.h:259 > + enum Part { Can we pick a more descriptive name than Part? ChromiumBridge has a lot of stuff in it so it's not obvious what WebCore::ChromiumBridge::Part is. Maybe ThemeScrollbarPart? > WebCore/platform/chromium/ChromiumBridge.h:271 > + enum State { Same thing, maybe name this ThemePaintState or something. > WebCore/platform/chromium/ChromiumBridge.h:286 > + union ExtraParams { Maybe ThemePaintExtraParams? > WebCore/platform/chromium/ChromiumBridge.h:293 > + static IntSize getSize(Part); getThemePartSize? > WebCore/platform/chromium/ChromiumBridge.h:295 > + static void paint(GraphicsContext*, Part, State, const IntRect&, const ExtraParams*); paintThemePart? > WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:56 > + ChromiumBridge::State state = > + scrollbar->hoveredPart() == partType ? ChromiumBridge::StateHover : ChromiumBridge::StateNormal; Nit: I would just put this on a single line. > WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:67 > + scrollbar->orientation() == HorizontalScrollbar ? > + ChromiumBridge::PartScrollbarHoriztonalTrack : > + ChromiumBridge::PartScrollbarVerticalTrack, Nit: single line > WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:79 > if (scrollbar->orientation() == HorizontalScrollbar) { FWIW, checking the orientation doesn't seem to matter here. Would be nice to remove this check if we can. > WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:97 > + if (checkMin && (scrollbar->currentPos() <= 0) > + || (checkMax && scrollbar->currentPos() == scrollbar->maximum())) { I think you need another pair of () here or the checkMin && () || () will cause a gcc warning. > WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:122 > + scrollbar->orientation() == HorizontalScrollbar ? > + ChromiumBridge::PartScrollbarHorizontalThumb : > + ChromiumBridge::PartScrollbarVerticalThumb, nit: single line > WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:-196 > - if (scrollbar->orientation() == VerticalScrollbar) > - return IntSize(scrollbarThicknessValue, scrollbar->height() < 2 * buttonLength ? scrollbar->height() / 2 : buttonLength); > - > - // HorizontalScrollbar > - return IntSize(scrollbar->width() < 2 * buttonLength ? scrollbar->width() / 2 : buttonLength, scrollbarThicknessValue); This is relatively new code. It looks like the code on the chrome side doesn't include the 2x check. > WebKit/chromium/src/ChromiumBridge.cpp:756 > + default: return WebThemeEngine::PartScrollbarDownArrow; Please remove the default: and just enumerate all the cases. It avoids errors when new values are added. > WebKit/chromium/src/ChromiumBridge.cpp:767 > + default: return WebThemeEngine::StateDisabled; no default
Created attachment 70484 [details] Patch
Comments address except FWIW, checking the orientation doesn't seem to matter here. Would be nice to remove this check if we can. Perhaps I don't understand but the line I think you're referring to is attempting to make sure we ask for the right button to be painted.
Comment on attachment 70484 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=70484&action=review You'll probably have to roll WebKit/WebKit/chromium/DEPS to pick up your chromium fixes. Please watch the build.webkit.org DRT bots and the build.chromium.org experimental bots when landing. > WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:76 > if (scrollbar->orientation() == HorizontalScrollbar) { Sorry, you're right, this condition is needed.
Committed r69586: <http://trac.webkit.org/changeset/69586>