Comment 59 for bug 219385

Revision history for this message
In , Karlt (karlt) wrote :

Comment on attachment 525346
gtk3 directory patch v1

I've had a look at gtk3drawing and nsNativeThemeGTK, thanks.
I'll write some general comments here and attach specifics.

hg copy definitely makes reviewing easier (=faster), thank you.

Even something that compiles is a significant milestone, so in patches we're
not necessarily looking for complete solutions but monotonically improving
steps to get us to the goal. Patches involving logical steps are easier to
review (and track in history) than simply dividing changes across files. In
the beginning, patches to port specific files are fine, but try to pick
patches to avoid breaking APIs for callers changed in other patches.

What will help us track what remains to be done is a single way of marking
items that are unfinished. Most such items are marked to TODO here but some
are not.

If a drawing section needs significant updating and you'd prefer to leave that
for a later patch (which is fine), then I'd prefer an "#if TODO" block than
experimental changes that don't really work.

There should be something stating that the cairo_t passed to the
moz_gtk_widget_paint needs to be a system-cairo cairo_t.

I assume there is no support for tiling themes in GTK3 - at least I can't
imagine how they would work.

It looks like the GTK3 style system is designed so that it is no longer
necessary to have a GtkWidget to draw the appropriate styling. If we were
writing gtk3drawing.c from scratch now, we'd probably use GtkStyleContext and
GtkWidgetPath. Following the GTK2 port and continuing to use widgets is a
sensible step where this works. For child widgets that are no longer
accessible through GTK3, it may be easier and cleaner to use
GtkStyleContext/GtkWidgetPath than searching through private descendants of a
GtkTreeView widget, for example.

As the new style system doesn't pass GtkWidgets to the theme engines, many of
the operations that were performed on the widgets now have no effect on
drawing, so there's no point updating them to new methods. This includes
the following methods and properties:

gtk_widget_grab_default gtk_widget_set_relief gtk_widget_grab_focus
gtk_toggle_button_set_active, gtk_widget_size_allocate,
gtk_adjustment_set_page_size, gtk_tree_view_column_set_sort_indicator,
gtk_widget_size_allocate, gtk_check_menu_item_set_active, "has-default",
"has-focus"

I think most of these can be removed, though some may need replacing with a
TODO indicating what remains to be done.

gtk_widget_override_background_color is an exception because it modifies the
widget's StyleContext, but I'm not sure whether still is necessary.

The clip_rect argument should be unnecessary now, as that is already on the
cairo_t. ThemeRenderer::DrawWithGDK could use some tidying up to remove that
(and I don't follow why it is important to remove the offsets).

In nsNativeThemeGTK.cpp, there is no longer any need to specify the visual to
renderer.Draw() (pass NULL, instead) as themes now draw to a cairo_t with any
target. That makes moz_gtk_widget_get_colormap() unnecessary.