Bug 156462 – [GTK] Rework scrollbars theming code for GTK+ 3.20

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

For the same reasons of bug #156333, now that we have RenderThemeGadget we can use that to render the scrollbars too.

[Carlos Garcia Campos]

Created attachment 276138 [details]
Patch

[Michael Catanzaro]

Comment on attachment 276138 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=276138&action=review

> Source/WebCore/ChangeLog:9
> +        needed for scrollbars, this patch uses the RenderthemGadget classes introduced in r199292 to render the native

Renderthem!

> Source/WebCore/platform/gtk/RenderThemeGadget.cpp:70
> +    // Scrollbars need to use its GType to be able to get non-CSS style properties.

Wow, I thought the type was ignored....

> Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:86
> +static GRefPtr<GtkStyleContext> createChildStyleContext(GtkStyleContext* parent, const char* name)

I'd use className instead of name to make it more clear what that parameter is for.

> Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:157
> +        FALLTHROUGH;

Didn't know we had this. Cool.

> Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:309
> +    // painting the thumb can be skipped. We don't have to be exact here.

I would name the method something like ScrollbarThemeGtk::probablyHasThumb.

> Source/WebCore/platform/gtk/ScrollbarThemeGtk.h:65
> +    bool m_hasForwardButtonStartPart : 1;

I don't pretend to know if using a bitfield here would improve performance because due to better cache locality, or hurt performance due to the nature of bitfields (seems more likely to me?), but it only saves three bytes, and it's not like this structure is going to be allocated frequently (should be just once, right?) so I would personally use normal bools here.

[Carlos Garcia Campos]

Comment on attachment 276138 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=276138&action=review

>> Source/WebCore/platform/gtk/RenderThemeGadget.cpp:70
>> +    // Scrollbars need to use its GType to be able to get non-CSS style properties.
> 
> Wow, I thought the type was ignored....

Me too, but otherwise gtk_style_context_get_style() fails when trying to get has-*-steppers properties.

>> Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:309
>> +    // painting the thumb can be skipped. We don't have to be exact here.
> 
> I would name the method something like ScrollbarThemeGtk::probablyHasThumb.

This is inherited, it's pure virtual of ScrollbarThemeComposite

>> Source/WebCore/platform/gtk/ScrollbarThemeGtk.h:65
>> +    bool m_hasForwardButtonStartPart : 1;
> 
> I don't pretend to know if using a bitfield here would improve performance because due to better cache locality, or hurt performance due to the nature of bitfields (seems more likely to me?), but it only saves three bytes, and it's not like this structure is going to be allocated frequently (should be just once, right?) so I would personally use normal bools here.

This is a singleton. We didn't use bool nor bitfield before because we used the member addresses directly in gtk_style_context_get_style(), but now that with newer GTK+ we have a different approach and we have to set the values individually, I thought it wouldn't hurt to use bool : 1 for these values, but I agree this doesn't make a huge difference.

[Carlos Garcia Campos]

Committed r199344: <http://trac.webkit.org/changeset/199344>