Bug 152830 – [GTK] Cleanup ScrollbarThemeGtk

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

Use a common path for GTK+ 3.19 and previous versions, simplifying the code and removing a lot of ifdefs.

[Carlos Garcia Campos]

Created attachment 268447 [details]
Patch

I've tested this with GTK+ from current master and with the wk internal jhbuild

[Carlos Garcia Campos]

Created attachment 268448 [details]
Patch

[Michael Catanzaro]

Comment on attachment 268448 [details]
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).

[Carlos Garcia Campos]

(In reply to comment #3)
> Comment on attachment 268448 [details]
> 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.

You can ask any question without rejecting the patch.

> 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.

The ScrollbarTheme methods are called either to get layout information (mainly scrollbarThickness) or to actually paint. Paint is the one calling all others (paintScrollbarBackground, paintTrackBackground, paintThumb, etc.). What I'm doing is caching the parent style context only for the paint() scope, to avoid creating new style contexts from any other method called from paint(). The style properties we cache are all properties of the parent scrollbar class.

> > 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.

The context is only cached when painting, the properties are not invalidated, because they are overwritten when a new style context is created, and only used when after calling getOrCreateStyleContext().

> > 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.

No, that's on purpose, when the style context is created outside the paint method, we don't always know the orientation, for example scrollbarThickness doesn't receive a Scrollbar&. In that case it's safe to use the default orientation, since we only need to get layout information.

> > 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.

I think -1 is always safer, it allows to reorder things without having to change all values.

> > 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.

No, GtkScrollbar inherits from GtkRange that implements GtkOrientable. See _gtk_orientable_set_style_classes(). There's indeed some specific CSS depending on the orientation for scrollbars in Adwaita, see:

  &.vertical {

    slider {
      margin-left: 1px + $_slider_margin;

      &:dir(rtl) {
        margin-left: $_slider_margin;
        margin-right: 1px + $_slider_margin;
      }
    }

    &.fine-tune slider {
      margin-left: 1px + $_slider_fine_tune_margin;

      &:dir(rtl) {
        margin-left: $_slider_fine_tune_margin;
        margin-right: 1px + $_slider_fine_tune_margin;
      }
    }

    trough {
      border-left-style: solid;

      &:dir(rtl) {
        border-left-style: none;
        border-right-style: solid;
      }
    }
  }


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

I don't

> > 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?

I had a previous version where I passed a state, this looks like a leftover.

> > 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).

Yes.

> > 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.

Because the style context is only cached for the paint() scope, and the theme can't change in the middle of paint that is all synchronous.

> > 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).

This is the first call. Note that all other methods calling getOrCreateStyleContext() are not caching it, only paint is caching this context.

[Michael Catanzaro]

(In reply to comment #4)
> You can ask any question without rejecting the patch.

I really was rejecting this version of the patch. ;) Because...

> The ScrollbarTheme methods are called either to get layout information
> (mainly scrollbarThickness) or to actually paint. Paint is the one calling
> all others (paintScrollbarBackground, paintTrackBackground, paintThumb,
> etc.). What I'm doing is caching the parent style context only for the
> paint() scope, to avoid creating new style contexts from any other method
> called from paint(). The style properties we cache are all properties of the
> parent scrollbar class.

...I did not understand that this cache existed only for the scope of paint(). This invalidates all of my concerns.
 
> No, GtkScrollbar inherits from GtkRange that implements GtkOrientable. See
> _gtk_orientable_set_style_classes(). There's indeed some specific CSS
> depending on the orientation for scrollbars in Adwaita, see:

Hm, a documentation issue then; the documentation for other widgets mentions this clearly.

> I had a previous version where I passed a state, this looks like a leftover.

OK. This is the only thing that needs fixed.

> This is the first call. Note that all other methods calling
> getOrCreateStyleContext() are not caching it, only paint is caching this
> context.

OK, I understand now.

[Carlos Garcia Campos]

Committed r194844: <http://trac.webkit.org/changeset/194844>