Bug 30183 - Kill virtual contextElement() method spread all over SVG code
Summary: Kill virtual contextElement() method spread all over SVG code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on: 30184 30218 30230
Blocks:
  Show dependency treegraph
 
Reported: 2009-10-07 14:08 PDT by Nikolas Zimmermann
Modified: 2009-10-14 16:54 PDT (History)
3 users (show)

See Also:


Attachments
Initial patch (74.61 KB, patch)
2009-10-11 16:15 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Updated patch (77.07 KB, patch)
2009-10-12 10:34 PDT, Nikolas Zimmermann
staikos: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2009-10-07 14:08:03 PDT
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.
Comment 1 Nikolas Zimmermann 2009-10-11 16:15:21 PDT
Created attachment 41005 [details]
Initial patch
Comment 2 Eric Seidel (no email) 2009-10-12 06:57:40 PDT
Is this safe?
 422             Attribute* old = namedAttrMap->getAttributeItem(attributeName);
Just want to make sure it's not another cause of bug 29620.
Comment 3 Eric Seidel (no email) 2009-10-12 07:05:41 PDT
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.
Comment 4 Nikolas Zimmermann 2009-10-12 09:34:28 PDT
(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!
Comment 5 Nikolas Zimmermann 2009-10-12 10:28:43 PDT
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.
Comment 6 Nikolas Zimmermann 2009-10-12 10:34:20 PDT
Created attachment 41050 [details]
Updated patch
Comment 7 Nikolas Zimmermann 2009-10-14 16:54:53 PDT
Committed r49602: <http://trac.webkit.org/changeset/49602>