Closed Bug 1277818 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Widget: Gtk, defect)

Unspecified
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox47 --- wontfix
firefox48 --- fix-optional
firefox49 --- fixed
firefox50 --- fixed

People

(Reporter: stransky, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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)
Attached patch patch (obsolete) — Splinter Review
The simplest patch.
Attachment #8759636 - Flags: review?(karlt)
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.
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 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!
Attachment #8759636 - Flags: review?(karlt) → review+
Attached patch patch v.2Splinter Review
Thanks! There's a new one attached.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=05b1fbe8cd4c
Attachment #8760235 - Attachment description: patch v.2 → patch v.2 for check-in
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?
Attachment #8760235 - Flags: review+
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();
(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.
Thanks, there's the indent fix attached.
Attachment #8760235 - Attachment description: patch v.2 for check-in → patch v.2
Keywords: checkin-needed
(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.
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/da10d713f676
exports CreateCSSNode() and divides pre/post 3.20 styles construction, r=karlt
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/da10d713f676
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Attachment #8759636 - Attachment is obsolete: true
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.
Attachment #8760661 - Flags: approval-mozilla-aurora?
Marking other affected branches. Cornel, can your team verify the fix on aurora once this lands? Thanks!
Flags: needinfo?(cornel.ionce)
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.
Attachment #8760661 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Flags: needinfo?(cornel.ionce)
You need to log in before you can comment on or make changes to this bug.