Bug 156462 - [GTK] Rework scrollbars theming code for GTK+ 3.20
Summary: [GTK] Rework scrollbars theming code for GTK+ 3.20
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2016-04-11 04:10 PDT by Carlos Garcia Campos
Modified: 2016-04-12 05:43 PDT (History)
1 user (show)

See Also:


Attachments
Patch (71.24 KB, patch)
2016-04-11 04:51 PDT, Carlos Garcia Campos
mcatanzaro: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2016-04-11 04:10:09 PDT
For the same reasons of bug #156333, now that we have RenderThemeGadget we can use that to render the scrollbars too.
Comment 1 Carlos Garcia Campos 2016-04-11 04:51:53 PDT
Created attachment 276138 [details]
Patch
Comment 2 Michael Catanzaro 2016-04-11 20:31:28 PDT
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.
Comment 3 Carlos Garcia Campos 2016-04-12 05:13:51 PDT
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.
Comment 4 Carlos Garcia Campos 2016-04-12 05:43:05 PDT
Committed r199344: <http://trac.webkit.org/changeset/199344>