[Webkit-unassigned] [Bug 152830] [GTK] Cleanup ScrollbarThemeGtk

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=152830

Michael Catanzaro <mcatanzaro at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #268448|review?                     |review-
              Flags|                            |

--- Comment #3 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 268448
  --> https://bugs.webkit.org/attachment.cgi?id=268448
Patch

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

This looks like a good patch. r- because I have a few questions and I want to see it again before you push it.

I'm a bit concerned that the correctness of these functions will now depend on nobody ever modifying the cached parentStyleContext outside a save()/restore() pair, but I think that is OK.

> Source/WebCore/ChangeLog:10
> +        GtkStyleContext, but when painting cache the newly created one so

Hm, when I read this first I thought, "Yeah, I had been thinking we should try this approach, both here and in RenderThemeGtk." But after looking at your code, this is a bit different than what I had been considering. I had been thinking we could cache each style context once per function that needs it, that way we don't have to worry about creating a bunch of unneeded style contexts, and we don't have to worry about a bunch of unneeded save()/restore() pairs, getting the best of both worlds. You have it cached once globally, and rely on not changing it to avoid save()/restore(). Your approach is sort of a middle ground; you managed to get rid of the save()/restore() using child style contexts that are created on the spot, but have cached the parent context. I think this is fine.

> Source/WebCore/ChangeLog:15
> +        only cached when we create a new GtkStyleContext.

Hm, when I read this I thought "OK that works, nice," but looking at your code, it seems you never create a new parent style context, so the cached properties are never invalidated. They're certainly not checked again on state changes. I don't understand this.

> Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:177
> +    if (m_cachedStyleContext)

You ignore the orientation parameter all but the first time this function is called, and return the same style context all the time. I think you need two cached style contexts, so that you can return the proper one dependent on orientation.

> Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:184
> +    gtk_widget_path_iter_set_object_name(path.get(), -1, "scrollbar");

This is OK, but as a matter of style, I prefer to pass 0 rather than -1.

-1 means "the last widget in the widget path" and in this case that is indeed widget 0, but I prefer to use -1 only when it's actually more convenient than passing the exact position. For our purposes in ScrollbarThemeGtk and RenderThemeGtk, it's always easy to pass the exact position, so I would never use -1. E.g. here it's clear that you want to apply the object name to widget 0, so I would write 0.

> Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:186
> +    gtk_widget_path_iter_add_class(path.get(), -1, "scrollbar");

Ditto.

> Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:188
> +    gtk_widget_path_iter_add_class(path.get(), -1, orientationStyleClass(orientation));

It's annoying that we need this. According to https://developer.gnome.org/gtk3/unstable/GtkScrollbar.html the scrollbar is not supposed to receive the orientation (horizontal, vertical) style classes, only the positional (left, right, up, down) classes. So in theory, this should go inside the #else portion of the #ifdef above, so that it doesn't affect GTK+ 3.19. But if I remember correctly, the scrollbar is not drawn at all without these. I think it's a bug in GTK+, either the theme or the documentation.

Also, again I prefer 0 rather than -1 here.

> Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:190
> +    gtk_style_context_set_state(styleContext.get(), gtk_widget_path_iter_get_state(path.get(), -1));

Why this line? You have not set any state flags on the widget path, so won't it be a no-op?

> Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:210
> +    gtk_widget_path_iter_set_object_name(path.get(), -1, name);

OK, this is a good example of when I would pass -1.

Were you looking at the foreign drawing test in GTK+? Your code looks similar to that (which is fine).

> Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:-211
> -    gtk_style_context_invalidate(gtkScrollbarStyleContext());

Can you explain why this isn't needed anymore? Since you have a cached style context always now, I would think we need to remove the conditional and make this call always, not remove it.

> Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:357
> +    TemporaryChange<GRefPtr<GtkStyleContext>> tempStyleContext(m_cachedStyleContext, getOrCreateStyleContext(scrollbar.orientation()));

I'm confused; this looks like a no-op (because the orientation parameter to getOrCreateStyleContext() is not properly honored except on the first call).

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160107/43706500/attachment-0001.html>