[webkit-reviews] review denied: [Bug 4883] WebCore+SVG missing
<text> support :
[Attachment 5535] a patch that provides basic text support using CSS text
rendering
bugzilla-request-daemon at opendarwin.org
bugzilla-request-daemon at opendarwin.org
Sat Jan 7 17:48:20 PST 2006
Eric Seidel <macdome at opendarwin.org> has denied Maciej Stachowiak
<mjs at apple.com>'s request for review:
Bug 4883: WebCore+SVG missing <text> support
http://bugzilla.opendarwin.org/show_bug.cgi?id=4883
Attachment 5535: a patch that provides basic text support using CSS text
rendering
http://bugzilla.opendarwin.org/attachment.cgi?id=5535&action=edit
------- Additional Comments from Eric Seidel <macdome at opendarwin.org>
Ok a few comments:
1. tabs in ChangeLog
2. Are you sure you want the LGPL on these? or shoudl it be BSD. If you
copied LGPL code it's gotta be LGPL...
3. I think translate* shoudl take a KRenderingDeviceContext * instead of
looking it up. Or alternatively, they could return a QMatrix (KCanvasMatrix
is slated for death)
+void RenderSVGText::paint(PaintInfo &paintInfo, int parentX, int parentY)
should check to make sure it's in the foreground phase, and otherwise return.
PaintInfo & should be PaintInfo&
Just go ahead and kill the entire block of extern from
khtml/rendering/render_canvasimage.h
That also could go in a separate patch, but doesn't need to.
missing newline from
ksvg2/css/svg.css
+ return new (getDocument()->renderArena()) RenderSVGText(this);
in
+khtml::RenderObject *SVGTextElementImpl::createRenderer(RenderArena *arena,
khtml::RenderStyle *style)
should use the passed in areana instead.
Otherwise looks great. I'm going to r- for these silly little nits. But it's
not really necessary to post a new patch if that's all you change.
More information about the webkit-reviews
mailing list