[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