Bug 12029 – Implement SVGElementInstance

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Antoine Quint]

Events triggered from an element in a <use> shadow tree should have an Event::target of type SVGElementInstance. Read the following email for more details:

http://tech.groups.yahoo.com/group/svg-developers/message/57762

[Antoine Quint]

Created attachment 12105 [details]
Test for SVGElementInstance::correspondingElement and SVGElementInstance::correspondingUseElement

[Nikolas Zimmermann]

Created attachment 12524 [details]
Initial patch

Initial patch rewriting <use> support including SVGElementInstance.
Event targets are not yet done, as well as complex deep references.
Another updating bug is also in - Eric knows.

[Eric Seidel (no email)]

Comment on attachment 12524 [details]
Initial patch

+    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.

[Nikolas Zimmermann]

Created attachment 12528 [details]
Updated patch

Updated patch, fixing the "repaint bug" (wrong HashSet usage, discussed with Eric).
Including several new LayoutTests & updated ChangeLogs.

This bug itself is NOT yet fixed - event handling will be done in a follow-up patch.

[Eric Seidel (no email)]

Comment on attachment 12528 [details]
Updated patch

Really this is fine as is.  I think there are some design changes we still need to make.  Most notably using the real shadowNode support which is already built into webcore.

Also, I'm still not sure it's necessary to have a whole double-layer of SVGElementInstance nodes, but we'll talk about that more in the next round of patches.

You were going to add at least a couple more tests, some of which we talked about over IRC.  Go ahead and land your improved patch with those test cases when you find the time.  g'luck with your studies. :)

[Sam Weinig]

Landed in r18979.