Bug 30183 – Kill virtual contextElement() method spread all over SVG code

• Component: SVG
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

SVG uses "virtual const SVGElement* contextElement() const" for the sake of being able to declare ANIMATED_* properties in base classes like SVGFitToViewBox, SVGURIReference etc..
We can just move the ANIMATED_* macro declarations to the classes that currently inherit from SVGURIReference and completly save the need of a virtual function.

[Nikolas Zimmermann]

Created attachment 41005 [details]
Initial patch

[Eric Seidel (no email)]

Is this safe?
 422             Attribute* old = namedAttrMap->getAttributeItem(attributeName);
Just want to make sure it's not another cause of bug 29620.

[Eric Seidel (no email)]

Comment on attachment 41005 [details]
Initial patch

I now see that the moved code is no different before, easing my concerns.

I don't understand when the "false" case of PropertySynchronizer is ever used?

Why?
         virtual const SVGElement* contextElement() const;
 54         const SVGElement* contextElement() const;

Your ChangeLog is pretty empty. :(

I don't understand when synchronize is called, if ever.

Why is this removal OK?
454          PassRefPtr<SVGAnimatedProperty##UpperProperty::TearOff> LowerProperty##Animated() const { return m_##LowerProperty.animatedTearOff(); } \
455          void synchronize##UpperProperty() const { m_##LowerProperty.synchronize(); }
 451         PassRefPtr<SVGAnimatedProperty##UpperProperty::TearOff> LowerProperty##Animated() const { return m_##LowerProperty.animatedTearOff(); }

I think in general this mostly needs a big update to the ChangeLog to explain all these individual changes, or you soudl split this patch up into separate parts.  I get the sense that a bunch of this change is a work in progress, and that the logic you're adding/changing isn't actually used for anything yet.

[Nikolas Zimmermann]

(In reply to comment #3)
> (From update of attachment 41005 [details])
> I now see that the moved code is no different before, easing my concerns.
> 
> I don't understand when the "false" case of PropertySynchronizer is ever used?
The false case is _only_ used for SVGViewSpec at the moment - the only class neededing ANIMATED_ macros that does not inherit from SVGElement (now that SVGFitToViewBox/SVGURIReference/SVGExternalResourcesRequired don't define any ANIMATED_ properties themselves anymore).

> 
> Why?
>          virtual const SVGElement* contextElement() const;
>  54         const SVGElement* contextElement() const;
For the reason above, contextElement() is needed for SVGViewSpec only, hence devirtualizing it.

> 
> Your ChangeLog is pretty empty. :(
Sorrz for that.

> 
> I don't understand when synchronize is called, if ever.
The synchronization logic is covered by existing LayoutTests in svg/custom and works just fine. To clarify it to ease your reviewing, here's a short summary of the structure of that code:

What we need:
someSVGElement.someAnimatedProperty.baseVal and someSVGElement.getAttribute("someAnimatedProperty") must stay synchronized all the time

How is it done:
Element::getAttribute() calls the virtual function updateAnimatedSVGProperty(String(..))
(hasAttribute() / attributes() also do this, but not relevant for the example)

SVGElement::updateAnimatedSVGProperty() either calls
a) invokeAllSVGPropertySynchronizers (if the passed String is empty, that means 'synchronize all possible attributes' - invoked through Element::attributes())
b ) invokeSVGPropertySynchronizer(..) (invoked through Element::getAttribute(...))

These invoke(All)SVG* methods traverse the list of registered animated properties and call the "synchronize()" method on the desired SVGAnimatedProperty objects.

That's basically how the attribute synchronization works. To summarize: any SVG DOM change doesn't trigger any attribute synchronization, as soon as you getAttribute(..) on a SVG animated property, it synchronizes the XML attribute based on the SVG DOM representation.)

This code is all tested and works. Hope that clarifies it a bit.

> 
> Why is this removal OK?
> 454          PassRefPtr<SVGAnimatedProperty##UpperProperty::TearOff>
> LowerProperty##Animated() const { return m_##LowerProperty.animatedTearOff(); }
> \
> 455          void synchronize##UpperProperty() const {
> m_##LowerProperty.synchronize(); }
>  451         PassRefPtr<SVGAnimatedProperty##UpperProperty::TearOff>
> LowerProperty##Animated() const { return m_##LowerProperty.animatedTearOff(); }

The removal is OK, because this method is not used anywhere. No one calls ie. "synchronizeWidth" in a manual fashion. I added this when implementing the synchronization facility for testing purposes.

> 
> I think in general this mostly needs a big update to the ChangeLog to explain
> all these individual changes, or you soudl split this patch up into separate
> parts.  I get the sense that a bunch of this change is a work in progress, and
> that the logic you're adding/changing isn't actually used for anything yet.
Ok will investigate how to split up this patch.

Thanks for the review!

[Nikolas Zimmermann]

Hm, I don't think splitting up this patch makes any sense, except it would be less to review :-)
The only thing I could rip out is changing the first argument of the ANIMATED_ property macros (moving from SVGExternal/FitToViewBox/URIReference to the actual class) - but a follow-up patch would need to touch all files again, just removing contextElement().

I'd highly appreciate if we could let it as is. Coming up with a detailed ChangeLog soon.

[Nikolas Zimmermann]

Created attachment 41050 [details]
Updated patch

[Nikolas Zimmermann]

Committed r49602: <http://trac.webkit.org/changeset/49602>