<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:mcatanzaro@igalia.com" title="Michael Catanzaro <mcatanzaro@igalia.com>"> <span class="fn">Michael Catanzaro</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Cleanup ScrollbarThemeGtk"
href="https://bugs.webkit.org/show_bug.cgi?id=152830">bug 152830</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #268448 Flags</td>
<td>review?
</td>
<td>review-
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Cleanup ScrollbarThemeGtk"
href="https://bugs.webkit.org/show_bug.cgi?id=152830#c3">Comment # 3</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Cleanup ScrollbarThemeGtk"
href="https://bugs.webkit.org/show_bug.cgi?id=152830">bug 152830</a>
from <span class="vcard"><a class="email" href="mailto:mcatanzaro@igalia.com" title="Michael Catanzaro <mcatanzaro@igalia.com>"> <span class="fn">Michael Catanzaro</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=268448&action=diff" name="attach_268448" title="Patch">attachment 268448</a> <a href="attachment.cgi?id=268448&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=268448&action=review">https://bugs.webkit.org/attachment.cgi?id=268448&action=review</a>
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.
<span class="quote">> Source/WebCore/ChangeLog:10
> + GtkStyleContext, but when painting cache the newly created one so</span >
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.
<span class="quote">> Source/WebCore/ChangeLog:15
> + only cached when we create a new GtkStyleContext.</span >
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.
<span class="quote">> Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:177
> + if (m_cachedStyleContext)</span >
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.
<span class="quote">> Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:184
> + gtk_widget_path_iter_set_object_name(path.get(), -1, "scrollbar");</span >
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.
<span class="quote">> Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:186
> + gtk_widget_path_iter_add_class(path.get(), -1, "scrollbar");</span >
Ditto.
<span class="quote">> Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:188
> + gtk_widget_path_iter_add_class(path.get(), -1, orientationStyleClass(orientation));</span >
It's annoying that we need this. According to <a href="https://developer.gnome.org/gtk3/unstable/GtkScrollbar.html">https://developer.gnome.org/gtk3/unstable/GtkScrollbar.html</a> 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.
<span class="quote">> Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:190
> + gtk_style_context_set_state(styleContext.get(), gtk_widget_path_iter_get_state(path.get(), -1));</span >
Why this line? You have not set any state flags on the widget path, so won't it be a no-op?
<span class="quote">> Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:210
> + gtk_widget_path_iter_set_object_name(path.get(), -1, name);</span >
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).
<span class="quote">> Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:-211
> - gtk_style_context_invalidate(gtkScrollbarStyleContext());</span >
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.
<span class="quote">> Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:357
> + TemporaryChange<GRefPtr<GtkStyleContext>> tempStyleContext(m_cachedStyleContext, getOrCreateStyleContext(scrollbar.orientation()));</span >
I'm confused; this looks like a no-op (because the orientation parameter to getOrCreateStyleContext() is not properly honored except on the first call).</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>