[webkit-reviews] review granted: [Bug 22384] svg <text> fails to update when setting x/y : [Attachment 46289] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has granted Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 22384: svg <text> fails to update when setting x/y
https://bugs.webkit.org/show_bug.cgi?id=22384

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

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
> +	   Introduce JSSVGCustomListTemplate, refactoring the existing custom
code for SVG POD type lists.

This is now spelled correctly, but doesn't reflect your patch :-)

> +	   Remove the need for custom JSSVG*List.cpp implementations, but
instead tweak CodeGeneratorJS.pm,
> +	   to call into the new JSSVGCustomListTemplate methods. Fixes dynamic
updates of the SVGTextElement

Ditto.

> +// Helper structure only containing public typedefs, as C++ does not allow
templatified typedefs

I think the common term is "templatized".

> +template<typename JSPODListType, typename PODType>
> +static JSC::JSValue finishGetter(JSC::ExecState* exec, ExceptionCode& ec,
SVGElement* context,
> +				    typename
JSSVGPODListTraits<PODType>::PODList* list,
> +				    PassRefPtr<typename
JSSVGPODListTraits<PODType>::PODListItem> item)
>  {
>      if (ec) {
>	   setDOMException(exec, ec);
> -	   return jsUndefined();
> +	   return JSC::jsUndefined();
>      }
> -    return toJS(exec, deprecatedGlobalObjectForPrototype(exec),
JSSVGPODTypeWrapperCreatorForList<SVGTransform>::create(item.get(),
list->associatedAttributeName()).get(), context);
> +
> +    return toJS(exec, deprecatedGlobalObjectForPrototype(exec),
> +		  
JSSVGPODTypeWrapperCreatorForList<PODType>::create(item.get(),
list->associatedAttributeName()).get(), context);
>  }
>  
> -static JSValue finishSetter(ExecState* exec, ExceptionCode& ec, SVGElement*
context, SVGTransformList* list, PassRefPtr<PODListItem> item)
> +template<typename JSPODListType, typename PODType>
> +static JSC::JSValue finishSetter(JSC::ExecState* exec, ExceptionCode& ec,
SVGElement* context,
> +				    typename
JSSVGPODListTraits<PODType>::PODList* list,
> +				    PassRefPtr<typename
JSSVGPODListTraits<PODType>::PODListItem> item)
>  {
>      if (ec) {
>	   setDOMException(exec, ec);
> -	   return jsUndefined();
> +	   return JSC::jsUndefined();
>      }
> +
>      const QualifiedName& attributeName = list->associatedAttributeName();
>      context->svgAttributeChanged(attributeName);
> -    return toJS(exec, deprecatedGlobalObjectForPrototype(exec),
JSSVGPODTypeWrapperCreatorForList<SVGTransform>::create(item.get(),
attributeName).get(), context);
> +
> +    return toJS(exec, deprecatedGlobalObjectForPrototype(exec),
> +		  
JSSVGPODTypeWrapperCreatorForList<PODType>::create(item.get(),
attributeName).get(), context);
>  }
>  
> -static JSValue finishSetterReadOnlyResult(ExecState* exec, ExceptionCode&
ec, SVGElement* context, SVGTransformList* list, PassRefPtr<PODListItem> item)
> +template<typename JSPODListType, typename PODType>
> +static JSC::JSValue finishSetterReadOnlyResult(JSC::ExecState* exec,
ExceptionCode& ec, SVGElement* context,
> +						  typename
JSSVGPODListTraits<PODType>::PODList* list,
> +						  PassRefPtr<typename
JSSVGPODListTraits<PODType>::PODListItem> item)
>  {
>      if (ec) {
>	   setDOMException(exec, ec);
> -	   return jsUndefined();
> +	   return JSC::jsUndefined();
>      }
>      context->svgAttributeChanged(list->associatedAttributeName());
> -    return toJS(exec, deprecatedGlobalObjectForPrototype(exec),
JSSVGStaticPODTypeWrapper<SVGTransform>::create(*item).get(), context);
> +    return toJS(exec, deprecatedGlobalObjectForPrototype(exec),
JSSVGStaticPODTypeWrapper<PODType>::create(*item).get(), context);
>  }

Seems like these functions don't need the JSPODListType template parameter.

> +    return finishSetter<JSPODListType, PODType>(exec, ec,
wrapper->context(), listImp,
> +						  
listImp->initialize(JSSVGPODListTraits<PODType>::PODListItem::copy(conversion(a
rgs.at(0))), ec));
> +
>  }

If you remove the JSPODListType template parameter from finishSetter, I think
the compiler will be able to deduce the other template parameter. (Ditto for
finishSetterReadOnlyResult and finishGetter.)

> +		   push(@implContent, "    return
JSSVGPODListCustom::$functionImplementationName<$className, " .
GetNativeType($svgPODListType)
> +				    . ">(castedThisObj, exec, args, &to" .
$svgPODListType . ");\n");

You also don't need the "&" here to create the function pointer. (C++ allows
omitting it.)

r=me