[webkit-reviews] review denied: [Bug 40887] requiredFeatures does not adapt to SVGStringList changes : [Attachment 59271] All buildfiles updated

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Rob Buis
<rwlbuis at gmail.com>'s request for review:
Bug 40887: requiredFeatures does not adapt to SVGStringList changes
https://bugs.webkit.org/show_bug.cgi?id=40887

Attachment 59271: All buildfiles updated
https://bugs.webkit.org/attachment.cgi?id=59271&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
WebCore/svg/SVGCircleElement.cpp:93
 +	    if (!isValid())
I'd reverse the logic, to avoid the not operator:
if (isValid())
    renderer->setNeedsLayout(true);
else
    detach();

Hm, this pattern seems to repeat quite often, how about creating a new static
helper function for that in SVGTests, which gets just the SVGElement* passed?
if (SVGTests::processTestAttributeChange(this))
    return;

processTestAttributeChange (maybe you can come up with a better name) would
just contain the logic spread around several files.
WebCore/svg/SVGUseElement.cpp:178
 +		invalidateShadowTree();
An exception would be SVGUseElement, here you could not use the new static
helper method from SVGTests, because it doesn't use setNeedsLayout(true).

WebCore/svg/SVGTextContentElement.cpp:211
 +  void SVGTextContentElement::svgAttributeChanged(const QualifiedName&
attrName)
You need to be careful here, SVGTextContentElement::isKnownAttribute should not
recognize SVGTests' attributes anymore, if you override svgAttributeChanged
here.

Some further comments:
I am not sure how the updates should actually work, if you're not adapting
CodeGeneratorJS and friends.
If you check "JSSVGLengthList.cpp" you'll see that it contains code like:

EncodedJSValue JSC_HOST_CALL jsSVGLengthListPrototypeFunctionGetItem(ExecState*
exec)
{
    JSValue thisValue = exec->hostThisValue();
    if (!thisValue.inherits(&JSSVGLengthList::s_info))
	return throwVMTypeError(exec);
    JSSVGLengthList* castedThis =
static_cast<JSSVGLengthList*>(asObject(thisValue));
    return JSValue::encode(JSSVGPODListCustom::getItem<JSSVGLengthList,
SVGLength>(castedThis, exec, toSVGLength));
}

You basically have to change SVGStringList, to be a SVGPODList<String> instead
of SVGList<String>. Then you do not need any custom JSSVGStringList bindings at
all.
In CodeGenerator.pm, the "podTypeHashWithWritableProperties" would need to
recognize "DOMString", otherwhise there will be need JSSVGPODListCustom*
wrappers created in JSSVGStringList.cpp.
I am not sure how easy it is to support strings at all, just give it a try :-)

Ah and of course this needs tests tests tests :-) Ideally all within the new
"script-tests" framework.