[webkit-reviews] review denied: [Bug 12029] Implement SVGElementInstance : [Attachment 12524] Initial patch

[bugzilla-request-daemon at macosforge.org]

Eric Seidel <macdome at opendarwin.org> has denied Eric Seidel
<macdome at opendarwin.org>'s request for review:
Bug 12029: Implement SVGElementInstance
http://bugs.webkit.org/show_bug.cgi?id=12029

Attachment 12524: Initial patch
http://bugs.webkit.org/attachment.cgi?id=12524&action=edit

------- Additional Comments from Eric Seidel <macdome at opendarwin.org>
+    ASSERT(instance && element);
should be separate asserts

+HashSet<SVGElementInstance*>
SVGDocumentExtensions::instancesForElement(SVGElement* element)

should be const if possible to prevent folks from manipulating it outside of
the other calls.  OR the other two calls should go away.

I wonder if this shouldn't use pointers:
+    HashMap<SVGElement*, HashSet<SVGElementInstance*> > m_elementInstances;

Otherwise I bet HashMap copies could be pretty expensive as return values. 
Unless you used const&.

one at a time is better:
+    ASSERT(m_useElement && m_element && m_clonedElement);

more:
+    ASSERT(element == m_element && !element->hasChildNodes());
+fprintf(stderr, "G O BABY!\n");

Why do this last?

+    // Assign new node to RefPtr.
+    m_clonedElement = svgClone;

it should just assign it right away.

Comments like this aren't generally needed:
+    // Clone single node.

the false in cloneNode(false) should be sufficient.

We use _h now:

-#ifndef SVGElementInstance_h
-#define SVGElementInstance_h
+#ifndef SVGElementInstance_H
+#define SVGElementInstance_H
+

I wrote WebKitTools/Scripts/clean-header-guards to keep things consistent.  I
preferred _H, but I was outnumbered ;)

Eeek.  Are you sure you really want SVGElementInstance to no be an EventTarget?


-    class SVGElementInstance : public EventTarget
+
+    class SVGElementInstance : public TreeShared<SVGElementInstance>

I think that's gonna make your life harder.

Indenting:
+	unsigned length = 0;
+	SVGElementInstance* instance;
+	for (instance = m_rootInstance->firstChild(); instance; instance =
instance->nextSibling())
+	    length++;


Are you sure you wnat to reinvent the wheel for the SVGElementInstance*?  I
would think we could just "turn off" normal functionality in Node and NodeList.


Why are you passing extensions to updateElemetnInstance?  Shouldn't it just
grab them of of it's document()?

Does element invalidation on gradient insert still work?  I don't see how.

is SVGUseElement buildPendingResource only called when it has a valid
reference?  Otherwise it will crash...

Do we have tests to test this behavior?
+    String widthString = String::number(width().value());
+    String heightString = String::number(height().value()); 
+    String transformString = String::format("translate(%f, %f)", x().value(),
y().value());

that the current javascript-specified width/height should be the one set on the
ElementInstance, instead of the the width/height as specified by the XML
attributes?

This conterdicts with your previous code:
+	 // Explicitely re-set width/height values
+	 svgElement->setAttribute(SVGNames::widthAttr,
hasAttribute(SVGNames::widthAttr) ? widthString : "100%");
+	 svgElement->setAttribute(SVGNames::heightAttr,
hasAttribute(SVGNames::heightAttr) ? heightString : "100%");

and would fail the test I mentioend about javascript values overriding xml
values for width/height.  We need to make that test and see which is correct.

Shouldn't the SVGHiddenElement code handle this?

+	 // IMPORTANT! Before inserted into the document, clear the id of the
clone!
+	 ExceptionCode ec = 0;	  
+	 svgElement->removeAttribute(HTMLNames::idAttr, ec);
+	 ASSERT(ec == 0);

There were already some shadow-tree solutions cooked up for form controls.  We
should ask beth or hyatt about those.

Looks good though.  We should talk more when you get up.  I'm unsure about the
HiddenElement stuff, and a little fuzzy on what the SVGElementInstance tree
looks like, but I look forward to hearing more from you in a few hours.