Bug 13825 - [CAIRO] Implement PathCairo
Summary: [CAIRO] Implement PathCairo
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 523.x (Safari 3)
Hardware: All All
: P2 Normal
Assignee: Nobody
Depends on:
Reported: 2007-05-22 12:35 PDT by Alp Toker
Modified: 2007-05-27 18:35 PDT (History)
1 user (show)

See Also:

Implement PathCairo (17.03 KB, patch)
2007-05-22 12:37 PDT, Alp Toker
sam: review-
Details | Formatted Diff | Diff
Implement PathCairo, take two (14.77 KB, patch)
2007-05-22 13:44 PDT, Alp Toker
oliver: review-
Details | Formatted Diff | Diff
Implement PathCairo, take three (15.40 KB, patch)
2007-05-23 05:54 PDT, Alp Toker
no flags Details | Formatted Diff | Diff
Implement PathCairo, take four (16.00 KB, patch)
2007-05-25 09:46 PDT, Alp Toker
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alp Toker 2007-05-22 12:35:55 PDT
Patch attached.
Comment 1 Alp Toker 2007-05-22 12:37:33 PDT
Created attachment 14661 [details]
Implement PathCairo
Comment 2 Oliver Hunt 2007-05-22 12:45:28 PDT
Comment on attachment 14661 [details]
Implement PathCairo

are you sure strokeContains and strokeBoundingRect are meant to be svg only?
Comment 3 Alp Toker 2007-05-22 13:03:00 PDT
(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.
Comment 4 Alp Toker 2007-05-22 13:44:57 PDT
Created attachment 14663 [details]
Implement PathCairo, take two
Comment 5 Oliver Hunt 2007-05-22 14:10:20 PDT
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 :-(
Comment 6 Alp Toker 2007-05-23 05:54:55 PDT
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.
Comment 7 Oliver Hunt 2007-05-23 06:07:17 PDT
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
Comment 8 Oliver Hunt 2007-05-24 19:23:20 PDT
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
Comment 9 Alp Toker 2007-05-25 09:46:10 PDT
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.
Comment 10 Sam Weinig 2007-05-27 18:35:51 PDT
Landed, with formatting corrections, in r21828.