[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