[webkit-reviews] review denied: [Bug 63797] Add a possibility to retrieve the associated SVGAnimatedProperty object for a certain XML attribute : [Attachment 99835] Patch v3

[bugzilla-daemon at webkit.org]

Rob Buis <rwlbuis at gmail.com> has denied Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 63797: Add a possibility to retrieve the associated SVGAnimatedProperty
object for a certain XML attribute
https://bugs.webkit.org/show_bug.cgi?id=63797

Attachment 99835: Patch v3
https://bugs.webkit.org/attachment.cgi?id=99835&action=review

------- Additional Comments from Rob Buis <rwlbuis at gmail.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=99835&action=review

Just a first quick review, I think the ChangeLog can be improved a lot,
especially the enum addition/changes. r- because of my enum request and build
failures mac-ews.

>> Source/WebCore/ChangeLog:46
>> +	    These lookups are all performed dynamically. Each
synchronizeProperty() call does a lot of comparisions.
> 
> comparisions -> comparisons

comparisions -> comparisons

>> Source/WebCore/ChangeLog:58
>> +	    Using these information, we can replace all the various
synchronizeProperty/fillAttributeToPropertyMap implementations, with a single
one in SVGElement.
> 
> Using this information

Using this information

>> Source/WebCore/ChangeLog:102
>> +		This is the main piece of the logic that replaces the manual
synchronizeProperty/fillAttributeToPropertyMap implementation. It expans to
following:
> 
> expans -> expands

expans -> expands

>> Source/WebCore/ChangeLog:137
>> +		    A generic way to synchronize a SVGAnimatedProperty with its
XML DOM attribute. Any SVG DOM change to eg. <rects> x property will now
trigger
> 
> <rects> -> <rect>s

<rects> -> <rect>s

>> Source/WebCore/ChangeLog:147
>> +		    This method is not used yet, but allows us to collect all
SVGAnimatedProperies for a QualifiedName -- the initial goal for this patch.
> 
> SVGAnimatedProperies -> SVGAnimatedProperties

SVGAnimatedProperies -> SVGAnimatedProperties

>> Source/WebCore/ChangeLog:562
>> +	    (WebCore::SVGPropertyTearOff::updateAnimVal):
> 
> These may need more in detail explanations. The one big thing I couldn't find
in the ChangeLog is why the enums like SVG_TEXTPATH_SPACINGTYPE_AUTO <->
TextPathSpacingAuto change was done? You hint at it in the comments but I think
it should be in the ChangeLog.

These may need more in detail explanations. The one big thing I couldn't find
in the ChangeLog is why the enums like SVG_TEXTPATH_SPACINGTYPE_AUTO <->
TextPathSpacingAuto change was done? You hint at it in the comments but I think
it should be in the ChangeLog.