[webkit-reviews] review denied: [Bug 10652] Need SVGFontFace*Element(s) DOM objects : [Attachment 16548] complete fix

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Oct 5 14:38:42 PDT 2007

Darin Adler <darin at apple.com> has denied Eric Seidel <eric at webkit.org>'s
request for review:
Bug 10652: Need SVGFontFace*Element(s) DOM objects

Attachment 16548: complete fix

------- Additional Comments from Darin Adler <darin at apple.com>
+ This file is part of the WebKit project

We don't normally include that in headers. I'd prefer to just omit it.

+    class SVGDefinitionSrcElement : public SVGElement
+    SVGDefinitionSrcElement(const QualifiedName&, Document*);
+    virtual ~SVGDefinitionSrcElement();
+    virtual void childrenChanged();

This looks indented wrong. Brace goes on same line as function definition.

+namespace WebCore

Brace goes on same line as namespace.

+    virtual ~SVGDefinitionSrcElement();

There's no need to declare and define the destructor explicitly in a case like
this where the destructor is already virtual, unless there's something about
the constructor that you can't compile given just the header (like a data
member that is a RefPtr for a type that's forward declared). For simple classes
it's far better to remove it.

+#define SVGDefinitionSrcElement_h

Normally we include "Platform.h" before using macros like ENABLE.

+// Defined in CSSGrammar.y, but not in any header, so just declare it here for
+int getPropertyID(const char* str, int len);

Lets not replicate this hack. If we need to use this in more places, then lets
fix the problem.

+#if 0
+    if (definitionSrc)
+	 m_styleDeclaration->setProperty(CSS_PROP_DEFINITION_SRC,
definitionSrc->getAttribute(XLinkNames::hrefAttr), false);

Please don't check in code with #if 0; if you must please include a comment.

I think we need some sort of deferral of the work in rebuildFontFace -- if you
do a sequence of DOM modifications, the SVGFontFaceElement will keep rebuilding
itself over and over again. Maybe we can make the style declaration get rebuild
as part of recalcStyle?

More information about the webkit-reviews mailing list