[webkit-reviews] review denied: [Bug 18700] [CAIRO] No pattern-support for SVG : [Attachment 20877] Pattern for SVG/Cairo

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Dirk Schulze <vbs85 at gmx.de>'s request
for review:
Bug 18700: [CAIRO] No pattern-support for SVG
http://bugs.webkit.org/show_bug.cgi?id=18700

Attachment 20877: Pattern for SVG/Cairo
http://bugs.webkit.org/attachment.cgi?id=20877&action=edit

------- Additional Comments from Eric Seidel <eric at webkit.org>
A couple comments:

1. What does it mean to be "too early"?  Comments should explain why, not just
what.  That comment sorta does... but the reader doesn't have enough context to
know what "too early" means here:
if (!image) // If it's too early we won't have an image yet.

2.  You can be the first platform to support platform extents!
+    cairo_pattern_set_extend(pattern, CAIRO_EXTEND_REPEAT);

I'm not sure that it would actually work, but you can try. :)  The repeat rule
is on the style, you can grab it and turn it into the right cairo enum and it
might "just work"

3.  This patch should do at least part of this:
+    // TODO: share this code with other PaintServers

Just move the "applyStrokeStyleToContext()" code out into its own separate
funciton.  our CG code does this, you can follow a similar model.  You might
not yet use that function from other paint servers yet, but you might as well
write that code as a function instead of just inline.

4.  Use a OwnArrayPtr for your dash array instead of manual delete.  Using the
scoped pointers is strongly encouraged to make it impossible to leak memory.

5.  Is cairo_pattern_destroy really safe?  If so, that's a very badly named
method.  Since you're either assuming that the pattern has already been
"copied" into the cairo context, or that _destroy actually means _release (and
that cairo_patterns are refcounted).

Also, creating a new pattern every time you paint is rather inefficient.  We
try to avoid that for the CG code path.  Maybe it doesn't matter yet for
cairo...

r- for the lack of separate function for the applyStrokeStyleToContext code
block.