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

[bugzilla-daemon at webkit.org]

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

--- Comment #4 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(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.

-- 
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/20160108/c855d859/attachment-0001.html>