[GTK] Warning spam from GtkStyleContext

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

Fix the spam of warnings coming from GTK+ 3.19:

(WebKitWebProcess:17185): Gtk-WARNING **: State 0 for context 0x1b5eb70 doesn't match state 128 set via gtk_style_context_set_state ()

(WebKitWebProcess:17185): Gtk-WARNING **: State 0 for context 0x1b5eb70 doesn't match state 128 set via gtk_style_context_set_state ()

(WebKitWebProcess:17185): Gtk-WARNING **: State 0 for context 0x1b5eb70 doesn't match state 128 set via gtk_style_context_set_state ()

(WebKitWebProcess:17185): Gtk-WARNING **: State 0 for context 0x1b5eb70 doesn't match state 128 set via gtk_style_context_set_state ()

(WebKitWebProcess:17185): Gtk-WARNING **: State 0 for context 0x1b5eb70 doesn't match state 128 set via gtk_style_context_set_state ()

[Michael Catanzaro]

Created attachment 266006 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 266006 [details]
Patch

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

I guess all these changes are compatible with previous versions of GTK+, right?

> Source/WebCore/ChangeLog:9
> +        Audit every use of gtk_style_context_get_* to fix compatibility with GTK+ 3.19. Some of
> +        these were already fine and are only changed for clarity.

This GTK+ API is more and more confusing . . .

> Source/WebCore/ChangeLog:12
> +        Company:  gtk_style_context_get() (and _get_padding/border/color()) should only ever be
> +                  called with the same state as gtk_style_context_get_state()

What's the state parameter for then? Why doesn't it takes the state internally since you have to set it and then pass what you have just set?

> Source/WebCore/ChangeLog:17
> +        Company:  and in very rare cases it needs a gtk_style_context_save() before the set_state(),
> +                  too

How do we know what are those rare cases? Would it be safe to always use save/restore? or does it have an impact in performance?

> Source/WebCore/rendering/RenderThemeGtk.cpp:279
> +    gtk_style_context_save(containerContext);
> +
> +    gtk_style_context_set_state(containerContext, GTK_STATE_FLAG_NORMAL);
> +    gtk_style_context_get_background_color(containerContext, gtk_style_context_get_state(containerContext), &color);

Maybe we should add a style context wrapper to hide all this weird things and ensure it's not misused in newly written code. It's not easy to use this API right.

[Michael Catanzaro]

(In reply to comment #2)
> I guess all these changes are compatible with previous versions of GTK+,
> right?

This patch should all be backwards-compatible, yes. I'll use #ifdefs where needed in the next patch.

> > Source/WebCore/ChangeLog:12
> > +        Company:  gtk_style_context_get() (and _get_padding/border/color()) should only ever be
> > +                  called with the same state as gtk_style_context_get_state()
> 
> What's the state parameter for then? Why doesn't it takes the state
> internally since you have to set it and then pass what you have just set?

GTK+ could track the state internally with gtk_style_context_save(), gtk_style_context_set_state(), gtk_style_context_restore(). I'm not positive why it was doesn't: maybe Ben could explain. My guess is that save() and restore() are expensive operations, but it could simply check whether the state is right and do it only if not needed. Instead we're supposed to do this manually where needed.

> > Source/WebCore/ChangeLog:17
> > +        Company:  and in very rare cases it needs a gtk_style_context_save() before the set_state(),
> > +                  too
> 
> How do we know what are those rare cases? Would it be safe to always use
> save/restore? or does it have an impact in performance?

It would always be safe to save/restore, but it might impact performance; my understanding is that save() requires a full traversal of the CSS tree to mark nodes that are not to be destroyed when restore() is called, then restore() would have similar complexity. I don't want to look at the implementation unless I have to, though.

The "rare" cases are the cases where leaving the state of the GtkStyleContext object changed would be the wrong thing to do: e.g. we don't want to modify the ones we have cached in getStyleContext() or the one in initMediaColors(). In most cases in this patch, all uses were already within save()/restore() pairs. In adjustRectAccordingToMargin() it looked like the state of the GtkStyleContext was not set probably only by mistake; that was the only use I wasn't entirely sure about. The other changes are just style changes, to call the function with the result of gtk_style_context_get_state() rather than with the state flags that had previously been passed to it.

I wonder if the cache is really useful for performance; I don't know whether creating a new GtkStyleContext each time would nowadays be worse than the cost of avoiding save()/restore() pairs.

> Maybe we should add a style context wrapper to hide all this weird things
> and ensure it's not misused in newly written code. It's not easy to use this
> API right.

I'm not sure we need one, because we only use it in RenderThemeGtk.cpp and ScrollbarThemeGtk.cpp.

[Benjamin Otte]

(In reply to comment #2)
> What's the state parameter for then? Why doesn't it takes the state
> internally since you have to set it and then pass what you have just set?
>
That parameter is for GTK 3.0. As there is no way to deprecate parameters and gtk_style_context_get_padding_without_state() is kinda crap...
 
> How do we know what are those rare cases? Would it be safe to always use
> save/restore? or does it have an impact in performance?
>
Those are the cases where no save()/restore() had happened before.
 
> Maybe we should add a style context wrapper to hide all this weird things
> and ensure it's not misused in newly written code. It's not easy to use this
> API right.
>
It's a bit tricky. The styling code is evolving all the time and webkit is diving deep down into regions that pretty much depend on cooperation between webkit, gtk and themes to keep working.

[WebKit Commit Bot]

Comment on attachment 266006 [details]
Patch

Clearing flags on attachment: 266006

Committed r192723: <http://trac.webkit.org/changeset/192723>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.