Bug 13825 – [CAIRO] Implement PathCairo

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alp Toker]

Patch attached.

[Alp Toker]

Created attachment 14661 [details]
Implement PathCairo

[Oliver Hunt]

Comment on attachment 14661 [details]
Implement PathCairo

are you sure strokeContains and strokeBoundingRect are meant to be svg only?

[Alp Toker]

(In reply to comment #2)
> (From update of attachment 14661 [details] [edit])
> are you sure strokeContains and strokeBoundingRect are meant to be svg only?
> 

These functions are SVG-specific but it could be argued that they belong in graphics/svg/cairo (which we do not have yet). If you're more comfortable leaving the code out for now, that's fine, otherwise it'll be useful to have the code around for when we start to work on SVG support.

[Alp Toker]

Created attachment 14663 [details]
Implement PathCairo, take two

[Oliver Hunt]

Comment on attachment 14663 [details]
Implement PathCairo, take two

I'm looking at this and i don't like the need to have different handling  of the PlatformPath, and currently it looks like you're begging for memory leaks.

i would suggest
typedef struct cairopath { 
    cairo_t* m_context; 
    cairopath() {
        cairo_surface_t* pathSurface = cairo_image_surface_create(CAIRO_FORMAT_A8, 1, 1);
        m_cr = cairo_create(pathSurface);
        moveTo(FloatPoint(0, 0));
    }
    ~cairopath() {
        //appriate delete stuff
    }
} PlatformPath;

then do everything in terms of that, that way you don't need ifdefs in the actual definition part of the header. 

This is an ugly ugly byproduct of cairo's borked path api :-/

It's far to similar to the canvas api for my liking :-(

[Alp Toker]

Created attachment 14682 [details]
Implement PathCairo, take three

This fixes some leaks in the previous patch, and also takes the approach I think olliej was supporting -- though I'm not too sure about the verbosity it adds without a clear gain in readability.

[Oliver Hunt]

Will attempt to review later

The reason for having a CairoPath struct is not to enhance the readability of the gtk stuff -- although if you made the cairo path stuff have appropriate arcTo, etc methods on it it *might*, i'm not sure if svg uses the same path object -- if it did it might make life easier/cleaner later on

The real reason is it reduces if defs in the platform headers -- which reduces the possiblity of requiring platform hacks elsewhere

[Oliver Hunt]

Comment on attachment 14682 [details]
Implement PathCairo, take three

you could
#define notImplemented() ASSERT(!"Not implemented");

for non gdk cairo...
I believe all platforms have ASSERT


I would be tempted to say that you shoud add CairoPath.h to define the CairoPath struct, and then include that file, that should minimise changes to the platform header.

But otherwise r=me

[Alp Toker]

Created attachment 14723 [details]
Implement PathCairo, take four

Generalises notImplemented() and splits out CairoPath into CairoPath.h, minimising modifications to Path.h as recommended in the review.

[Sam Weinig]

Landed, with formatting corrections, in r21828.