[webkit-reviews] review denied: [Bug 30183] Kill virtual contextElement() method spread all over SVG code : [Attachment 41005] Initial patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Oct 12 07:05:41 PDT 2009


Eric Seidel <eric at webkit.org> has denied Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 30183: Kill virtual contextElement() method spread all over SVG code
https://bugs.webkit.org/show_bug.cgi?id=30183

Attachment 41005: Initial patch
https://bugs.webkit.org/attachment.cgi?id=41005&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
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.


More information about the webkit-reviews mailing list