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

[bugzilla-daemon at webkit.org]

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
http://bugs.webkit.org/show_bug.cgi?id=10652

Attachment 16548: complete fix
http://bugs.webkit.org/attachment.cgi?id=16548&action=edit

------- 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
+{
+public:
+    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
+#if ENABLE(SVG)

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
now.
+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);
+#endif

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?