1277818 - [GTK+ 3.20] Export CreateCSSNode() and potentially divide pre/post 3.20 code.

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Martin Stránský [:stransky] (ni? me)]

We may export CreateCSSNode() to allow other modules to create styles on the fly. We may also divide the pre/post 3.20 code to add more flexibility to style construction.

Follow up from:

https://bugzilla.mozilla.org/show_bug.cgi?id=1271523#c15 (tooltips background bug)
https://bugzilla.mozilla.org/show_bug.cgi?id=1275407 (scrollbar bug)
https://bugzilla.mozilla.org/show_bug.cgi?id=1250704 (tooltip text color bug)

Comment 1

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch

The simplest patch.

Comment 2

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8759636 [details] [diff] [review]
patch

A final patch also needs to remove GetStyleInternal() declaration which I forget to do in this patch.

Comment 3

[Martin Stránský [:stransky] (ni? me)]

Looks like we really need to create all styles differently for 3.20. There's the conversation I had with Company on #gtk+

<stransky> Company, Hi, is there a problem when we create the root style from widget and then child CSS nodes by the path? In Gtk 3.20
<Company> stransky: i wouldn't do that
<stransky> Company, for instance for scrollbar widgets. to get the root style by gtk_widget_get_style_context(scrollbar widget) and then use path to get CSS child styles
<Company> because then you have parts of the code that update based on the widget tree and parts of the code that update based on your tree
<Company> and your tree changes the widget tree by hooking into it, too
<Company> i don't know off-hand of things that would go wrong
<Company> but i wouldn't be surprised by it
<stransky> Company, okay, But it's okay to still get style from widgets if we don't use the hierarchy?
<Company> stransky: why do you want to do that? btw?
<stransky> Company, to share code between pre/post 3.20 code in Firefox
<stransky> Company, because FF needs to keep compatibility from gtk 3.4
<Company> hrm
<Company> i'm not sure sharing code there is that good of an idea
<stransky> Company, well, so do i need to create all styles from path for gtk3.20?
<Company> GTK3.4 and 3.20 style machinery is almost as far apart internally as GTK2 and GTK3
<Company> stransky: i would do that
<Company> stransky: you can get the widget path from a widget and create the style from that
<stransky> and how?
<Company> gtk_widget_get_path()
<Company> and then create a style context with that path
<stransky> Company, so do you basically say that call gtk_widget_get_style_context(widget) just for base styles is a bad idea on 3.20?
<Company> stransky: I think mixing those style contexts with style contexts created via paths is tricky
<stransky> Company, Ok, thanks. 
<Company> stransky: both because the first kind get changed all the time and because adding style contexts changes :first-child etc behaviors by adding new nodes to the trtee

Comment 4

[Karl Tomlinson (:karlt)]

Comment on attachment 8759636 [details] [diff] [review]
patch

> static GtkStyleContext*
>+GetCssNodeStyleInternal(WidgetNodeType aNodeType)

Can you add a comment here please to indicate that this is used only when the
GTK version > 3.20?

>+      style = CreateCSSNode(GTK_STYLE_CLASS_TROUGH,
>+                            GetCssNodeStyleInternal(MOZ_GTK_SCROLLBAR_HORIZONTAL));

Please add

static StyleContext*
CreateChildCSSNode(const char* aName, WidgetNodeType aParentNodeType)

or

static StyleContext*
CreateCSSNode(const char* aName, WidgetNodeType aParentNodeType,
              GType aType = G_TYPE_NONE)

so that these simplify to

      style = CreateCSSNode(GTK_STYLE_CLASS_TROUGH,
                            MOZ_GTK_SCROLLBAR_HORIZONTAL);

(and will fit in 80 columns.)

>+      /* Actual progress bar indicator for Gtk3.20+ only. */

Remove "for Gtk3.20+ only." because this whole function is 3.20+ now.

>+    default:
>+      GtkWidget* widget = GetWidget(aNodeType);
>+      if (widget) {
>+        return gtk_widget_get_style_context(widget);
>+      }
>+      break;
>+  }

Given the code below is asserting that style is set, the "if (widget)" test
and "break;" are not required here.  MOZ_ASSERT(widget), if you like.
 
>+  if (style) {
>+    sStyleStorage[aNodeType] = style;
>+    return style;
>+  }
>+
>+  MOZ_ASSERT_UNREACHABLE("missing style context for node type");
>+  return nullptr;

  MOZ_ASSERT(style, "missing style context for node type");
  sStyleStorage[aNodeType] = style;
  return style;

>+static GtkStyleContext*
>+CreateWidgetStyle(WidgetNodeType aWidgetType, 
>+                  const gchar*   aStyleClass)
>+{

"GetWidgetStyle" because this does not "Create", and ownership is not passed
to the caller.

Or "GetWidgetStyleWithClass" may be clearer even.

>+  if (aStyleClass) {

aStyleClass is always non-null and so this check is not required.

>+// CreateCSSNode is useful for gtk >= 3.20 only.

Perhaps this function could be useful pre-3.20 one day if adjusted to skip
sGtkWidgetPathIterSetObjectName when not available, so I suggest
"CreateCSSNode is implemented for gtk >= 3.20 only."

(In reply to Martin Stránský from comment #2)
> A final patch also needs to remove GetStyleInternal() declaration which I
> forget to do in this patch.

OK.  Thanks!

Comment 5

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch v.2

Thanks! There's a new one attached.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=05b1fbe8cd4c

Comment 6

[Karl Tomlinson (:karlt)]

Comment on attachment 8760235 [details] [diff] [review]
patch v.2

>+      return GetWidgetStyleWithClass(MOZ_GTK_SCROLLBAR_HORIZONTAL,
>+                               GTK_STYLE_CLASS_TROUGH);
>+    case MOZ_GTK_SCROLLBAR_THUMB_HORIZONTAL:
>+      return GetWidgetStyleWithClass(MOZ_GTK_SCROLLBAR_HORIZONTAL,
>+                               GTK_STYLE_CLASS_SLIDER);

Looks good, thanks.

Can you touch up the indentation in these calls, please?

Comment 7

[Bill Gianopoulos [:WG9s]]

With both the patch for this bug and the patch for bug 1275407 applied, I find the browser crashing often with this (or similar) stack:

Program wg9s_64/firefox (pid = 2301) received signal 11.
Stack:
#01: ???[/home/wag/wg9s_64/libxul.so +0x6ea478d]
#02: ???[/lib64/libpthread.so.0 +0x10c10]
#03: gtk_widget_path_copy[/lib64/libgtk-3.so.0 +0x373d68]
#04: ???[/lib64/libgtk-3.so.0 +0x36c19d]
#05: ???[/lib64/libgtk-3.so.0 +0x15806e]
#06: gtk_container_get_path_for_child[/lib64/libgtk-3.so.0 +0x15e1be]
#07: ???[/lib64/libgtk-3.so.0 +0x15806e]
#08: gtk_container_get_path_for_child[/lib64/libgtk-3.so.0 +0x15e1be]
#09: ???[/lib64/libgtk-3.so.0 +0x18e8b5]
#10: ???[/lib64/libgtk-3.so.0 +0x2c6d8e]
#11: gtk_widget_style_get_valist[/lib64/libgtk-3.so.0 +0x370775]
#12: gtk_widget_style_get[/lib64/libgtk-3.so.0 +0x370aa4]
#13: ???[/home/wag/wg9s_64/libxul.so +0x460b1e8]
#14: ???[/home/wag/wg9s_64/libxul.so +0x4631f15]
#15: ???[/home/wag/wg9s_64/libxul.so +0x4c8c260]
#16: ???[/home/wag/wg9s_64/libxul.so +0x4c8bf73]
#17: ???[/home/wag/wg9s_64/libxul.so +0x4c8db4d]
#18: ???[/home/wag/wg9s_64/libxul.so +0x4c91885]
#19: ???[/home/wag/wg9s_64/libxul.so +0x4cc089b]
#20: ???[/home/wag/wg9s_64/libxul.so +0x4c8f7d2]
#21: ???[/home/wag/wg9s_64/libxul.so +0x4ca57c4]
#22: ???[/home/wag/wg9s_64/libxul.so +0x4cc07dd]
#23: ???[/home/wag/wg9s_64/libxul.so +0x4c8f7d2]
#24: ???[/home/wag/wg9s_64/libxul.so +0x4cc07dd]
#25: ???[/home/wag/wg9s_64/libxul.so +0x4c8f7d2]
#26: ???[/home/wag/wg9s_64/libxul.so +0x4b101d1]
#27: ???[/home/wag/wg9s_64/libxul.so +0x4cc07dd]
#28: ???[/home/wag/wg9s_64/libxul.so +0x4c8f7d2]
#29: ???[/home/wag/wg9s_64/libxul.so +0x4cc07dd]
#30: ???[/home/wag/wg9s_64/libxul.so +0x4c8f7d2]
#31: ???[/home/wag/wg9s_64/libxul.so +0x4cc07dd]
#32: ???[/home/wag/wg9s_64/libxul.so +0x4c8f7d2]
#33: ???[/home/wag/wg9s_64/libxul.so +0x4ca7387]
#34: ???[/home/wag/wg9s_64/libxul.so +0x4ca394c]
#35: ???[/home/wag/wg9s_64/libxul.so +0x4c8c981]
#36: ???[/home/wag/wg9s_64/libxul.so +0x4cbe9dd]
#37: ???[/home/wag/wg9s_64/libxul.so +0x4c8fdc2]
#38: ???[/home/wag/wg9s_64/libxul.so +0x4c8c981]
#39: ???[/home/wag/wg9s_64/libxul.so +0x4cbe9dd]
#40: ???[/home/wag/wg9s_64/libxul.so +0x4c8fdc2]
#41: ???[/home/wag/wg9s_64/libxul.so +0x4c8c981]
#42: ???[/home/wag/wg9s_64/libxul.so +0x4b1ac7b]
#43: ???[/home/wag/wg9s_64/libxul.so +0x4b1b3b6]
#44: ???[/home/wag/wg9s_64/libxul.so +0x4b105f6]
#45: ???[/home/wag/wg9s_64/libxul.so +0x4c8c981]
#46: ???[/home/wag/wg9s_64/libxul.so +0x4cbe9dd]
#47: ???[/home/wag/wg9s_64/libxul.so +0x4c8fdc2]
#48: ???[/home/wag/wg9s_64/libxul.so +0x4c8c981]
#49: ???[/home/wag/wg9s_64/libxul.so +0x4cbe9dd]
#50: ???[/home/wag/wg9s_64/libxul.so +0x4c8fdc2]
#51: ???[/home/wag/wg9s_64/libxul.so +0x4c8c981]
#52: ???[/home/wag/wg9s_64/libxul.so +0x4cbe9dd]
#53: ???[/home/wag/wg9s_64/libxul.so +0x4c8fdc2]
#54: ???[/home/wag/wg9s_64/libxul.so +0x4c8c981]
#55: ???[/home/wag/wg9s_64/libxul.so +0x4cbe9dd]
#56: ???[/home/wag/wg9s_64/libxul.so +0x4c8fdc2]
#57: ???[/home/wag/wg9s_64/libxul.so +0x4c8c981]
#58: ???[/home/wag/wg9s_64/libxul.so +0x4cbe9dd]
#59: ???[/home/wag/wg9s_64/libxul.so +0x4c8fdc2]
#60: ???[/home/wag/wg9s_64/libxul.so +0x4c8c981]
#61: ???[/home/wag/wg9s_64/libxul.so +0x4cbe9dd]
#62: ???[/home/wag/wg9s_64/libxul.so +0x4c8fdc2]
#63: ???[/home/wag/wg9s_64/libxul.so +0x4c8c981]
#64: ???[/home/wag/wg9s_64/libxul.so +0x4cc223f]
#65: ???[/home/wag/wg9s_64/libxul.so +0x4c8fdc2]
#66: ???[/home/wag/wg9s_64/libxul.so +0x4c93225]
#67: ???[/home/wag/wg9s_64/libxul.so +0x4c8c981]
#68: ???[/home/wag/wg9s_64/libxul.so +0x4cbe9dd]
#69: ???[/home/wag/wg9s_64/libxul.so +0x4c8fdc2]
#70: ???[/home/wag/wg9s_64/libxul.so +0x4c8c981]
#71: ???[/home/wag/wg9s_64/libxul.so +0x4cc223f]
#72: ???[/home/wag/wg9s_64/libxul.so +0x4c8fdc2]
#73: ???[/home/wag/wg9s_64/libxul.so +0x4c8c981]
#74: ???[/home/wag/wg9s_64/libxul.so +0x4c8f509]
#75: ???[/home/wag/wg9s_64/libxul.so +0x4caff92]
#76: ???[/home/wag/wg9s_64/libxul.so +0x4accaa1]
#77: ???[/home/wag/wg9s_64/libxul.so +0x4bc0bab]
#78: ???[/home/wag/wg9s_64/libxul.so +0x4a4a8ea]
#79: ???[/home/wag/wg9s_64/libxul.so +0x4a4b308]
#80: ???[/home/wag/wg9s_64/libxul.so +0x4a385f8]
#81: ???[/home/wag/wg9s_64/libxul.so +0x4917487]
#82: ???[/home/wag/wg9s_64/libxul.so +0x4919de0]
#83: ???[/home/wag/wg9s_64/libxul.so +0x4919c02]
#84: ???[/home/wag/wg9s_64/libxul.so +0x4919d01]
#85: ???[/home/wag/wg9s_64/libxul.so +0x491ae87]
#86: ???[/home/wag/wg9s_64/libxul.so +0x491aa82]
#87: ???[/home/wag/wg9s_64/libxul.so +0x4926aaf]
#88: ???[/home/wag/wg9s_64/libxul.so +0x49269c2]
#89: ???[/home/wag/wg9s_64/libxul.so +0x4926863]
#90: ???[/home/wag/wg9s_64/libxul.so +0xe04294]
#91: NS_InvokeByIndex[/home/wag/wg9s_64/libxul.so +0xe2d68a]
#92: ???[/home/wag/wg9s_64/libxul.so +0x1e1b395]
#93: ???[/home/wag/wg9s_64/libxul.so +0x1e18fb5]
#94: ???[/home/wag/wg9s_64/libxul.so +0x1dfe310]
#95: ???[/home/wag/wg9s_64/libxul.so +0x1e06d2a]
#96: ??? (???:???)

This would appear to be from here:

GtkStyleContext*
CreateCSSNode(const char* aName, GtkStyleContext* aParentStyle, GType aType)
{
  static auto sGtkWidgetPathIterSetObjectName =
    reinterpret_cast<void (*)(GtkWidgetPath *, gint, const char *)>
    (dlsym(RTLD_DEFAULT, "gtk_widget_path_iter_set_object_name"));

  GtkWidgetPath* path = aParentStyle ?
    gtk_widget_path_copy(gtk_style_context_get_path(aParentStyle)) :
    gtk_widget_path_new();

Comment 8

[Karl Tomlinson (:karlt)]

(In reply to Martin Stránský from comment #3)
> <stransky> Company, Hi, is there a problem when we create the root style
> from widget and then child CSS nodes by the path? In Gtk 3.20
> <Company> stransky: i wouldn't do that
> <stransky> Company, for instance for scrollbar widgets. to get the root
> style by gtk_widget_get_style_context(scrollbar widget) and then use path to
> get CSS child styles
> <Company> because then you have parts of the code that update based on the
> widget tree and parts of the code that update based on your tree
> <Company> and your tree changes the widget tree by hooking into it, too

I'm not clear what is meant by this.

I'm not aware of any CSS state that is affected by descendants, and so adding
child style contexts will only inherit from the parent.  The parent/widget
context is not affected by having a child.

There may be good reasons not to use widget style contexts, but
I don't follow how this reasoning applies to Gecko.

> <Company> i don't know off-hand of things that would go wrong
> <Company> but i wouldn't be surprised by it

There is a risk there, but there is proven cost in constructing styles from
scratch.

> <stransky> Company, okay, But it's okay to still get style from widgets if
> we don't use the hierarchy?
> <Company> stransky: why do you want to do that? btw?
> <stransky> Company, to share code between pre/post 3.20 code in Firefox
> <stransky> Company, because FF needs to keep compatibility from gtk 3.4
> <Company> hrm
> <Company> i'm not sure sharing code there is that good of an idea
> <stransky> Company, well, so do i need to create all styles from path for
> gtk3.20?
> <Company> GTK3.4 and 3.20 style machinery is almost as far apart internally
> as GTK2 and GTK3

Yes, but the widget and the style context object are in common.

> <Company> stransky: i would do that
> <Company> stransky: you can get the widget path from a widget and create the
> style from that
> <stransky> and how?
> <Company> gtk_widget_get_path()
> <Company> and then create a style context with that path

That raises some good options, thanks.  It would pick up the object/node name
automatically.  It needs gtk_widget_get_style_context() beforehand for the
side-effect of setting widget->priv->context for getting the classes in
gtk_widget_path_append_for_widget().  That would also work for pre-3.20 GTK.

I suspect it is simpler to just use gtk_widget_path_append_for_widget() than
to create all ancestor widgets for the hierarchy for gtk_widget_get_path().

> <stransky> Company, so do you basically say that call
> gtk_widget_get_style_context(widget) just for base styles is a bad idea on
> 3.20?
> <Company> stransky: I think mixing those style contexts with style contexts
> created via paths is tricky
> <stransky> Company, Ok, thanks. 
> <Company> stransky: both because the first kind get changed all the time

That's why trying to construct all contexts from scratch would be a losing
battle.  Gecko needs these changes reflected in its style context.

But gtk_widget_path_append_for_widget() should address that, it seems.

> and because adding style contexts changes :first-child etc behaviors by
> adding new nodes to the trtee

I think this would actually work out OK unless there a subtlety that I'm
missing here.

Take, for example, the selector

  notebook tab:first-child label

If the notebook context comes from a widget and the tab and label contexts are
constructed from scratch and gtk_style_context_set_parent(), then the
:first-child status is dependent only on the siblings specified when
constructing the path for the tab context.

For style contexts, :first-child is not determined by examining the child
nodes of the notebook.  (If it were, then Gecko would need to add child style
contexts in order.)

If notebook has other tab subnodes (or another widget has other subnodes),
they would be affected by the new tab child, I assume, but Gecko doesn't use
those subnodes.

Still, I like constructing a style context from a path constructed from widgets, thanks, if it means not having to keep widgets alive with the style context.

Comment 9

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch v.3 patch for check-in

Thanks, there's the indent fix attached.

Comment 10

[Martin Stránský [:stransky] (ni? me)]

(In reply to Bill Gianopoulos [:WG9s] from comment #7)
> With both the patch for this bug and the patch for bug 1275407 applied, I
> find the browser crashing often with this (or similar) stack:

I don't see it but let's solve that in Bug 1275407.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/da10d713f676
exports CreateCSSNode() and divides pre/post 3.20 styles construction, r=karlt

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/da10d713f676

Comment 13

[Karl Tomlinson (:karlt)]

Comment on attachment 8760661 [details] [diff] [review]
patch v.3 patch for check-in

Approval Request Comment
[Feature/regressing bug #]: GTK3 port
[User impact if declined]:
Bug 1250704 (incorrect tooltip text color, unreadable with some themes)
depends on this.
[Describe test coverage new/current, TreeHerder]:
none.
[Risks and why]: 
This is a refactor, with no intended side effects and which has been on m-c since 2016-06-07.
[String/UUID change made/needed]:
none.

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Marking other affected branches. Cornel, can your team verify the fix on aurora once this lands? Thanks!

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8760661 [details] [diff] [review]
patch v.3 patch for check-in

Fix for GTK themes, so this has been an issue since 46. Let's uplift to aurora and make sure it works as intended.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2e16cca367c0

Comment 17

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

Hello! We tried to test this issue (see Bug 1250704, comments 42-44), but unfortunately, we do not have available the required Linux distribution, so we asked the reporter Martin Giger [:freaktechnik] to verify this.