Bug 47473 – Use new WebThemeEngine api on chromium / linux to draw scrollbars.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dave Moore]

Use new WebThemeEngine api on chromium / linux to draw scrollbars.

[Dave Moore]

Created attachment 70409 [details]
Patch

[Dave Moore]

Created attachment 70410 [details]
Patch

[Dave Moore]

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.

[Tony Chang]

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

[Dave Moore]

Created attachment 70484 [details]
Patch

[Dave Moore]

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.

[Tony Chang]

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.

[Tony Chang]

Committed r69586: <http://trac.webkit.org/changeset/69586>