516973 - Mismatch between Cairo and Thebes surface types

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Steffen Imhof]

In gfxASurface::GetType() there is a quite optimistic cast from one surface type enum into the other:
   
   return (gfxSurfaceType)cairo_surface_get_type(mSurface);

That only works as long as both enums have an identical list of values. Which is currently not the case. Observe:

typedef enum _cairo_surface_type {      typedef enum {

    CAIRO_SURFACE_TYPE_IMAGE,                SurfaceTypeImage,
    CAIRO_SURFACE_TYPE_PDF,                  SurfaceTypePDF,
    CAIRO_SURFACE_TYPE_PS,                   SurfaceTypePS,
    CAIRO_SURFACE_TYPE_XLIB,                 SurfaceTypeXlib,
    CAIRO_SURFACE_TYPE_XCB,                  SurfaceTypeXcb,
    CAIRO_SURFACE_TYPE_GLITZ,                SurfaceTypeGlitz,
    CAIRO_SURFACE_TYPE_QUARTZ,               SurfaceTypeQuartz,
    CAIRO_SURFACE_TYPE_WIN32,                SurfaceTypeWin32,
    CAIRO_SURFACE_TYPE_BEOS,                 SurfaceTypeBeOS,
    CAIRO_SURFACE_TYPE_DIRECTFB,             SurfaceTypeDirectFB,
    CAIRO_SURFACE_TYPE_SVG,                  SurfaceTypeSVG,
    CAIRO_SURFACE_TYPE_OS2,                  SurfaceTypeOS2,
    CAIRO_SURFACE_TYPE_WIN32_PRINTING,       SurfaceTypeWin32Printing,
    CAIRO_SURFACE_TYPE_QUARTZ_IMAGE,         SurfaceTypeQuartzImage,
    CAIRO_SURFACE_TYPE_SCRIPT,               SurfaceTypeQPainter,
    CAIRO_SURFACE_TYPE_QT,                   SurfaceTypeDDraw
    CAIRO_SURFACE_TYPE_META,
    CAIRO_SURFACE_TYPE_DDRAW                 } gfxSurfaceType;

} cairo_surface_type_t;


This leads for example to non-rendering native widgets in the Qt build, because the surface type returned by gfxASurface is wrong.

One could simply bring both enums in sync again, but I think a better long-term solution would be to have a real mapping, so that errors like that don't creep in silently.

Comment 1

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

Sounds like this crept in with the latest cairo update, and jeff didn't update the gfxSurfaceType list.

We can't have a direct mapping in the enum, since we can't include the cairo.h header in the (public) thebes header.   However, gfxASurface.cpp should have some static asserts for each of the enums, to ensure that their values are equal to the cairo enum.

Comment 2

[Steffen Imhof]

I like the idea of the compile time asserts.

The main issue must have crept in by a cairo update a good while ago. It is  broken in the 1.9.2 branch, too. (Which is where I stumbled upon it, I just used the latest trunk code to write this bug.)

So maybe we want to just fix the enum values in the 1.9.2 branch and implement compile time asserts in the trunk to prevent future problems.

Comment 3

[Steffen Imhof]

Attachment: Patch for trunk that only adds the missing enum value to gfxASurface

I attached the patch I'm currently using on the 1.9.2 branch.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 401169 [details] [diff] [review]
Patch for trunk that only adds the missing enum value to gfxASurface

This puts enums in sync on 1.9.2 right? If so, this looks good to me.

Comment 5

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

Yeah, though we should do the static asserts on 1.9.2 as well -- otherwise if we end up taking a cairo update down the line, this might bite us.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

I assume this patch won't make it into 1.9.2 anymore.

Steffen, does this patch apply cleanly to mozilla-central? Is it needed for mozilla-central?

Comment 7

[James Cook]

As of today (revision a43e2f7eda8f) this applies on trunk without any problems.

Comment 8

[Steffen Imhof]

And it is needed for mozilla-central, too.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

let's rename the patch to make sure it gets to "trunk" and add checkin-needed keyword.

Comment 10

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/8e7933e5525e