327878 - Reenable native themes in cairo-gtk2 builds

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Here it comes... As a bonus, these patches also turn on GTK2 themes for HTML content.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Add cairo_try_copy_clip_rectangles

The draw_with_xlib code needs to be able to read the cairo clip to see whether and how the native drawing needs to be clipped. In some situations (when the clip is being stored as a mask), we can't get the clip in a usable form, and trying to convert it to a usable form would be wasted effort. So I'm introducing this 'best-effort' API which is allowed to just bail when the job gets too hard.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Add cairo-xlib getters

The draw_with_xlib code needs to be able to read some of the state of an xlib surface.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Enable native GTK2 themes with Thebes

There are three parts to this patch:

a) add Thebes gfxXlibNativeRenderer API
This is similar to what I've talked about in the past.

b) modify nsNativeThemeGTK2 to use the new API

c) reenable GTK2 native themes in Thebes build, and let them style HTML content

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The browser actually looks pretty good with this patch. There are a couple of issues that might need to be addressed in followup:
-- combobox dropdown buttons use a tiny triangle.
-- focused buttons get two focus rects (I assume one from the native theme and one of our own)

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 212438 [details] [diff] [review]
Add cairo_try_copy_clip_rectangles

Code itself looks fine, but I would rename the new methods for consistency with previous cairo usage of "extract" (cairo_traps_extract_region).  

_cairo_clip_try_copy_rectangles -> _cairo_clip_extract_rectangles

_cairo_clip_try_copy_region_to_rectangles -> _cairo_region_to_clip_rectangles (nothing clip-specific about this function, so I wouldn't call it _cairo_clip_*)

_cairo_gstate_try_copy_clip_rectangles -> _cairo_gstate_extract_clip_rectangles

cairo_try_copy_clip_rectangles -> cairo_extract_clip_rectangles

I'm not sold on cairo_extract_clip_rectangles(), but it should do for now until we figure out what a more general clip-extraction API should look like.

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 212439 [details] [diff] [review]
Add cairo-xlib getters

r=vladimir

0/NULL are fine for non-xlib surface error cases; I'd expose cairo_surface_is_xlib() as well myself, but I have a plan to expose those for all the surface types soon.  You should also return 0/NULL if surface->base.status isn't CAIRO_STATUS_SUCCESS in all the getters.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 212441 [details] [diff] [review]
Enable native GTK2 themes with Thebes

>--- gfx/thebes/src/Makefile.in	13 Feb 2006 22:37:13 -0000	1.15
>+++ gfx/thebes/src/Makefile.in	14 Feb 2006 04:48:22 -0000
>@@ -41,25 +41,25 @@ EXTRA_DSO_LDOPTS += \

>-CPPSRCS +=      gfxXlibSurface.cpp gfxPlatformGtk.cpp
>+CPPSRCS +=      gfxXlibSurface.cpp gfxPlatformGtk.cpp   gfxXlibNativeRenderer.cpp
                                                       ^ extra spaces

>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ gfx/thebes/src/gfxXlibNativeRenderer.cpp	20 Feb 2006 03:02:41 -0000

This whole file needs to go to 4-space indents (seems to be using 2-space) for consistency with thebes (and cairo! :)

>+ * License.
>+ *
>+ * The Original Code is Novell code.7
                                      ^ no 7, unless that has some s3kr1t meaning


>+static cairo_surface_t* _get_current_target (cairo_t* cr)
>+{
>+  cairo_surface_t* target = cairo_get_group_target (cr);
>+  if (target == NULL) {
>+    target = cairo_get_target (cr);
>+  }
>+} 

Heh; I couldn't decide what the API for this should look like.  This should probably become a cairo toplevel function at some point...


>+
>+typedef enum _cairo_xlib_drawing_opacity {
>+  CAIRO_XLIB_DRAWING_IS_OPAQUE,
>+  CAIRO_XLIB_DRAWING_NOT_OPAQUE
>+} cairo_xlib_drawing_opacity_t;
>+typedef enum _cairo_xlib_drawing_supports_offset {
>+  CAIRO_XLIB_DRAWING_SUPPORTS_OFFSET,
>+  CAIRO_XLIB_DRAWING_NO_OFFSET
>+} cairo_xlib_drawing_supports_offset_t;
>+typedef enum _cairo_xlib_drawing_supports_nondefault_visual {
>+  CAIRO_XLIB_DRAWING_SUPPORTS_NONDEFAULT_VISUAL,
>+  CAIRO_XLIB_DRAWING_DEFAULT_VISUAL
>+} cairo_xlib_drawing_supports_nondefault_visual_t;
>+typedef enum _cairo_xlib_drawing_supports_clip_rect {
>+  CAIRO_XLIB_DRAWING_SUPPORTS_CLIP_RECT,
>+  CAIRO_XLIB_DRAWING_NO_CLIP_RECT
>+} cairo_xlib_drawing_supports_clip_rect_t;

This is pretty verbose; I'd just make this into a single bitfield like you do for the C++ verion -- they're all on/off flags anyway, so it seems like it would make the API simpler.

>+static cairo_surface_t *
>+_create_temp_xlib_surface (cairo_t *cr, Display *dpy, int width, int height)
>+{
>+  /* base the temp surface on the *screen* surface, not any intermediate
>+   * group surface, because the screen surface is more likely to have
>+   * characteristics that the xlib-using code is likely to be happy with */
>+  cairo_surface_t *target = _get_current_target (cr);

Erm.. _get_current_target will return the intermediate group target, no?

>+static void
>+_release_temp_xlib_surface (Display *dpy, cairo_surface_t *temp_xlib_surface)
>+{
>+  Drawable d = cairo_xlib_surface_get_drawable (temp_xlib_surface);
>+  /* XXX maybe save the surface for recycling? but watch out for the surface
>+   * being returned in surface_out below */
>+  cairo_surface_destroy (temp_xlib_surface);
>+  if (d) {
>+    XFreePixmap (dpy, d);
>+  }
>+}

Just set the Pixmap as user data on the surface, with a custom destroy function that will do the XFreePixmap -- that way you don't have to worry about any pixmap-vs-surface lifetime issues.  See http://lxr.mozilla.org/seamonkey/source/gfx/thebes/src/gfxXlibSurface.cpp#42 and http://lxr.mozilla.org/seamonkey/source/gfx/thebes/src/gfxXlibSurface.cpp#150 .

There's actually a owns_pixmap flag in the xlib surface that will do the free as part of the surface's finish call -- you could always export cairo_xlib_surface_take_ownership() or something, or do the user data bit and put a comment in here saying "this should just set owns_pixmap if it moves into cairo".

>+static int
>+_draw_onto_temp_xlib_surface (cairo_surface_t *temp_xlib_surface,
>+                              cairo_xlib_drawing_callback callback,
>+                              void *closure,
>+                              float background_color)

I'd call this something other than background_color, since it's just a single float.. background_value maybe.

>+{
>+  /* Note that if the create_surface_similar path is taken above to create
>+   * the temporary surface, then we don't have a clue what the display and
>+   * visual might be without using the getters here. */
>+  Drawable d = cairo_xlib_surface_get_drawable (temp_xlib_surface);
>+  Display *dpy = cairo_xlib_surface_get_display (temp_xlib_surface);
>+  Visual *visual = cairo_xlib_surface_get_visual (temp_xlib_surface);
>+  int result;
>+  cairo_t *cr = cairo_create (temp_xlib_surface);
>+  cairo_set_source_rgb (cr, background_color, background_color, background_color);

set OPERATOR_SOURCE (though I may be too paranoid about that)

>+  cairo_paint (cr);
>+  cairo_destroy (cr);


>+static cairo_surface_t *
>+_copy_xlib_surface_to_image (cairo_surface_t *temp_xlib_surface,
>+                             cairo_format_t format,
>+                             int width, int height,
>+                             unsigned char **data_out)
>+{
>+  unsigned char *data;
>+  cairo_surface_t *result;
>+  cairo_t *cr;
>+  
>+  *data_out = data = (unsigned char*)malloc (width*height*4);
>+  if (!data)
>+    return NULL;
>+
>+  result = cairo_image_surface_create_for_data (data, format, width, height, width*4);
>+  cr = cairo_create (result);
>+  cairo_set_source_surface (cr, temp_xlib_surface, 0, 0);

OPERATOR_SOURCE again

>+  cairo_paint (cr);
>+  cairo_destroy (cr);
>+  return result;
>+}
>+

Everything else looks fine..

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Let me revise the patch to move the cairo code out into its own file. I will also modify the draw_with_xlib interface a bit.

> This is pretty verbose; I'd just make this into a single bitfield like you do
> for the C++ verion -- they're all on/off flags anyway, so it seems like it
> would make the API simpler.

I have the impression that Carl doesn't like bitfields, although I may be wrong.

I'm considering just ditching these verbose parameters and using a different interface where the callback can just abort saying "I can't handle this!" I'll try it to see how it looks.

>>+  /* base the temp surface on the *screen* surface, not any intermediate
>>+   * group surface, because the screen surface is more likely to have
>>+   * characteristics that the xlib-using code is likely to be happy with */
>>+  cairo_surface_t *target = _get_current_target (cr);
>
> Erm.. _get_current_target will return the intermediate group target, no?

Yeah. I'll change it to _get_target.

> Just set the Pixmap as user data on the surface

OK

> background_value maybe.

background_gray_value?

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Updated extract-clip-rectangles patch

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: updated xlib-getters patch

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Updated GTK2 theme/cairo-xlib-utils patch

Updated in response to comments ... also moves cairo-ish code out to cairo-xlib-utils.[ch]

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 212804 [details] [diff] [review]
Updated GTK2 theme/cairo-xlib-utils patch

r=me, with some indentation fixes:

>Index: gfx/thebes/src/cairo-xlib-utils.c
>===================================================================

>+
>+ void cairo_draw_with_xlib (cairo_t *cr,
>+                            cairo_xlib_drawing_callback callback,
>+                            void *closure,
>+                            Display *dpy,
>+                            unsigned int width, unsigned int height,
>+                            cairo_xlib_drawing_opacity_t is_opaque,
>+                            cairo_xlib_drawing_support_t capabilities,
>+                            cairo_xlib_drawing_result_t *result)

void on its own separate line, remove leading space


>Index: gfx/thebes/src/gfxXlibNativeRenderer.cpp
>===================================================================

4-space indents everywhere in this file (it's using 2-space now)

Comment 13

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 212691 [details] [diff] [review]
Updated extract-clip-rectangles patch

r=me with these fixes:

>Index: gfx/cairo/cairo/src/cairo.h

>+/**
>+ * cairo_clip_rect_t:
>+ * 
>+ * A data structure for holding clip rectangles.
>+ */
>+typedef struct _cairo_clip_rect {
>+  double x, y, width, height;
>+} cairo_clip_rect_t;

4 spaces before double (yeah, I know that cairo_glyph_t is broken with 2-spaces)


>--- gfx/cairo/cairo/src/cairo-clip-private.h	10 Jan 2006 22:56:38 -0000	1.2
>+++ gfx/cairo/cairo/src/cairo-clip-private.h	7 Feb 2006 22:44:42 -0000

>+cairo_private cairo_bool_t
>+_cairo_clip_try_copy_rectangles (cairo_clip_t *clip,
>+                                 int max_rectangles,
>+                                 cairo_clip_rect_t *rectangles_out,
>+                                 int *num_rectangles_out);

This got renamed to _cairo_clip_extract_rectangles, think you missed updating the prototype

Comment 14

[Stuart Parmenter]

Comment on attachment 212804 [details] [diff] [review]
Updated GTK2 theme/cairo-xlib-utils patch

+CSRCS = cairo-xlib-utils.c
this should be conditional on GTK2

and what vlad said.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

all patches checked in!

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So, for what it's worth (not sure if you know this already), I did a profile of attachment 213304 [details] in a cairo build a few weeks ago when native themes were turned on for content.  And slightly over 25% of the time in the profile was spent in the following stack:

XGetGeometry
gdk_pixmap_foreign_new_for_display
ThemeRenderer::NativeDraw(_XDisplay*, unsigned long, Visual*, short, ...
NativeRendering(void*, _XDisplay*, unsigned long, Visual*, short, ...
_draw_onto_temp_xlib_surface
cairo_draw_with_xlib
gfxXlibNativeRenderer::Draw(_XDisplay*, gfxContext*, int, int, ...

and probably around 20% was spent in the following stack:

XGetImage
_get_image_surface
_cairo_xlib_surface_acquire_source_image
_cairo_surface_acquire_source_image
_cairo_pattern_acquire_surface_for_surface
_cairo_pattern_acquire_surface
_cairo_pattern_acquire_surfaces
_cairo_image_surface_composite
_cairo_surface_composite
_composite_trap_region
_clip_and_composite_trapezoids
_cairo_surface_fallback_paint
_cairo_surface_paint
_cairo_gstate_paint
cairo_paint
_copy_xlib_surface_to_image
cairo_draw_with_xlib
gfxXlibNativeRenderer::Draw(_XDisplay*, gfxContext*, int, int, ...

(I'm filtering manually from a full profile, but it's pretty clear which paths are within gfxXlibNativeRenderer::Draw because it's about half of the total time, including idling.  And a sizable chunk of the rest is another XGetGeometry call that I think is not related to this patch.)

Comment 17

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

David, I filed bug 329846 for the re-enabling of content native theme on Linux (I should've mentioned it in this bug).  Mind copying your findings there as well?

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The latter is suckitude of the slow-path theme renderer. We need a native theme renderering cache that vlad and I have talked about but not yet implemented.

The former I didn't know about. I'll have to look into it. It looks like gdk_pixmap_foreign_new is doing a horrible server round-trip to get the details of the pixmap. It might be solved if we cached the temporary surface used by draw_with_xlib, which would be a good idea in general.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Even with themes turned off for content, we're probably still eating some of this just drawing native scrollbars and chrome.

Comment 20

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #19)
> Even with themes turned off for content, we're probably still eating some of
> this just drawing native scrollbars and chrome.

Yeah, we definitely are -- the native themed UI "lags" when redrawing the entire window very noticably.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

FWIW, I filed a bug suggesting GDK have a better version of gdk_foreign_new_for_display, and that bug now has a patch on it:
http://bugzilla.gnome.org/show_bug.cgi?id=334954

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

er, gdk_pixmap_foreign_new_for_display.  Knew it wasn't long enough. :-)

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: add missing debugging printf

When debugging why we were missing the fast path for bug 333250, I noticed this printf was missing.  It didn't actually matter for my testing, but it might for somebody.

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I filed bug 333249 and bug 333250 on the performance problems in comment 16.

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 217694 [details] [diff] [review]
add missing debugging printf

Checked in to trunk.