Bug 22384 – svg <text> fails to update when setting x/y

• Component: SVG
• Status: RESOLVED FIXED

[Dmitry Stadnik]

- Add SVG text element
- Define js function that changes x and y properties of the text
- Call it from setTimeout(f, 0)
=> Text remains in old position

[Dmitry Stadnik]

Created attachment 25315 [details]
Sample

[Eric Seidel (no email)]

I think that we might not be correctly updating from setting an SVGLengthList.  The list code is kinda scary.

[Eric Seidel (no email)]

I suspect this has nothing to do with the timeout.  I bet if you set the x/y (via the list apis) from JS we'll fail to update in all cases.

[Rob Buis]

Created attachment 31027 [details]
First attempt

There was no code signalling the element that the text position list was changed, all that seems
to have failed is some glue code from the js side. A test is also included.
Cheers,

Rob.

[Eric Seidel (no email)]

Hum... Sad that we have to have custom bindings for every SVGList type.  Maybe we should be wrapping lists when we return them.  SVGList is just one big disaster anyway.  It's not clear to me if the SVGList implementation or the JS wrapper should be handling these notifications.

[Maciej Stachowiak]

Comment on attachment 31027 [details]
First attempt

I'm pretty sure that handling this in the JS bindings is the wrong layer. Won't that mean other language bindings, or mutation via the core C++ interfaces, have the same bug? I think the notification should be at the core object level, or else we need to provide a generic way to do it for all language bindings and apply it to, for instance, the ObjC bindings.

The code itself looks fine, but r- to consider this change of approach.

[Rob Buis]

Hi Maciej,

(In reply to comment #6)
> (From update of attachment 31027 [details] [review])
> I'm pretty sure that handling this in the JS bindings is the wrong layer. Won't
> that mean other language bindings, or mutation via the core C++ interfaces,
> have the same bug? I think the notification should be at the core object level,
> or else we need to provide a generic way to do it for all language bindings and
> apply it to, for instance, the ObjC bindings.
> The code itself looks fine, but r- to consider this change of approach.

Thanks for the review!

I do agree that that would be the better design. However I think that right now only the (js) bindings have enough context to know what node to notify, ie. SVGList.cpp and for example SVGPointList.cpp do not know that. I did not follow the POD work too closely so I do not know if that is the only way for it to work, WildFox or eseidel should know more about that. I'll try to contact one of them to find out what is best to do in this case.
Cheers,

Rob.

[Nikolas Zimmermann]

Created attachment 46273 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 46273 [details]
Patch

Aren't the pixel test results incorrect?

[Dirk Schulze]

Comment on attachment 46273 [details]
Patch

LGTM r=me

[Adam Roben (:aroben)]

Comment on attachment 46273 [details]
Patch

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 53075)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,46 @@
> [phone]  Nikolas Zimmermann  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        svg <text> fails to update when setting x/y
> +        https://bugs.webkit.org/show_bug.cgi?id=22384
> +
> +        Introduce JSSVGCusotmListTemplate, refactoring the existing custom code for SVG POD type lists.

Typo: JSSVGCusotmListTemplate

> +        * Android.jscbindings.mk:
> +        * GNUmakefile.am:
> +        * WebCore.gypi:
> +        * WebCore.pro:
> +        * WebCore.vcproj/WebCore.vcproj:
> +        * WebCore.xcodeproj/project.pbxproj:
> +        * bindings/js/JSSVGCustomListTemplate.h: Added.
> +        (WebCore::JSSVGCustomListTemplate::clear):
> +        (WebCore::JSSVGCustomListTemplate::initialize):
> +        (WebCore::JSSVGCustomListTemplate::getItem):
> +        (WebCore::JSSVGCustomListTemplate::insertItemBefore):
> +        (WebCore::JSSVGCustomListTemplate::replaceItem):
> +        (WebCore::JSSVGCustomListTemplate::removeItem):
> +        (WebCore::JSSVGCustomListTemplate::appendItem):
> +        (WebCore::JSSVGCustomListTemplate::finishGetter):
> +        (WebCore::JSSVGCustomListTemplate::finishSetter):
> +        (WebCore::JSSVGCustomListTemplate::finishSetterReadOnlyResult):
> +        * bindings/js/JSSVGPointListCustom.cpp: Removed.
> +        * bindings/js/JSSVGTransformListCustom.cpp: Removed.
> +        * bindings/scripts/CodeGeneratorJS.pm:
> +        * svg/SVGNumberList.cpp:
> +        (WebCore::SVGNumberList::SVGNumberList):
> +        * svg/SVGNumberList.h:
> +        * svg/SVGPointList.idl:
> +        * svg/SVGTransformList.idl:

It would be great to have some function- and file-level comments about the
changes you made.

> --- WebCore/bindings/js/JSSVGCustomListTemplate.h	(revision 0)
> +++ WebCore/bindings/js/JSSVGCustomListTemplate.h	(revision 0)

Using svn cp to create this file by copying JSSVGTransformListCustom.cpp or
JSSVGPointListCustom.cpp would make it clearer where this code came from
(especially since your ChangeLog doesn't explicitly say).

> +template<typename JSWrapperType, typename PODType, typename PODTypeList>
> +class JSSVGCustomListTemplate {

I worry that adding even more templates to SVG code will increase the code size
of WebCore again. Is there any way we can use type erasure to reduce the number
of template parameters, at least? (E.g., is there some base class that all
JSWrapperTypes share that we could use instead?)

I don't think the name of this class is great. Certainly I don't think
"Template" makes its purpose any clearer.

Since this class is never instantiated and only has static member functions, I
think it might make more sense to move the functions out of the class entirely
and get rid of the class.

> +    static JSC::JSValue clear(JSWrapperType* wrapper, JSC::ExecState* exec, const JSC::ArgList&, ConversionCallback)
> +    {
> +        ExceptionCode ec = 0;
> +        SVGListBase* listImp = wrapper->impl();
> +        listImp->clear(ec);
> +        setDOMException(exec, ec);
> +        wrapper->context()->svgAttributeChanged(listImp->associatedAttributeName());
> +        return JSC::jsUndefined();
> +    }

Is it bad to call svgAttributeChanged if the list was already cleared before
this function was called? (E.g., if you called clear twice in a row.) The same
question applies to all the other setters.

> +    static JSC::JSValue initialize(JSWrapperType* wrapper, JSC::ExecState* exec, const JSC::ArgList& args, ConversionCallback conversion)
> +    {
> +        ExceptionCode ec = 0;
> +        SVGListBase* listImp = wrapper->impl();
> +        return finishSetter(exec, ec, wrapper->context(), wrapper->impl(),
> +                            listImp->initialize(PODListItem::copy((*conversion)(args.at(0))), ec));
> +    }

The listImp local variable doesn't seem very helpful here.

You can use function pointers without the (*) syntax. Just treat it as a normal
function call.

> +    static JSC::JSValue getItem(JSWrapperType* wrapper, JSC::ExecState* exec, const JSC::ArgList& args, ConversionCallback)
> +    {
> +        bool indexOk = false;
> +        unsigned index = args.at(0).toUInt32(exec, indexOk);
> +        if (!indexOk) {
> +            setDOMException(exec, TYPE_MISMATCH_ERR);
> +            return JSC::jsUndefined();
> +        }
> +
> +        ExceptionCode ec = 0;
> +        SVGListBase* listImp = wrapper->impl();
> +        return finishGetter(exec, ec, wrapper->context(), wrapper->impl(),
> +                            listImp->getItem(index, ec));
> +    }
> +
> +    static JSC::JSValue insertItemBefore(JSWrapperType* wrapper, JSC::ExecState* exec, const JSC::ArgList& args, ConversionCallback conversion)
> +    {
> +        bool indexOk = false;
> +        unsigned index = args.at(1).toUInt32(exec, indexOk);
> +        if (!indexOk) {
> +            setDOMException(exec, TYPE_MISMATCH_ERR);
> +            return JSC::jsUndefined();
> +        }
> +
> +        ExceptionCode ec = 0;
> +        SVGListBase* listImp = wrapper->impl();
> +        return finishSetter(exec, ec, wrapper->context(), wrapper->impl(),
> +                            listImp->insertItemBefore(PODListItem::copy((*conversion)(args.at(0))), index, ec));
> +    }
> +
> +    static JSC::JSValue replaceItem(JSWrapperType* wrapper, JSC::ExecState* exec, const JSC::ArgList& args, ConversionCallback conversion)
> +    {
> +        bool indexOk = false;
> +        unsigned index = args.at(1).toUInt32(exec, indexOk);
> +        if (!indexOk) {
> +            setDOMException(exec, TYPE_MISMATCH_ERR);
> +            return JSC::jsUndefined();
> +        }
> +
> +        ExceptionCode ec = 0;
> +        SVGListBase* listImp = wrapper->impl();
> +        return finishSetter(exec, ec, wrapper->context(), wrapper->impl(),
> +                            listImp->replaceItem(PODListItem::copy((*conversion)(args.at(0))), index, ec));
> +    }
> +
> +    static JSC::JSValue removeItem(JSWrapperType* wrapper, JSC::ExecState* exec, const JSC::ArgList& args, ConversionCallback)
> +    {
> +        bool indexOk = false;
> +        unsigned index = args.at(0).toUInt32(exec, indexOk);
> +        if (!indexOk) {
> +            setDOMException(exec, TYPE_MISMATCH_ERR);
> +            return JSC::jsUndefined();
> +        }
> +
> +        ExceptionCode ec = 0;
> +        SVGListBase* listImp = wrapper->impl();
> +        return finishSetterReadOnlyResult(exec, ec, wrapper->context(), wrapper->impl(),
> +                                          listImp->removeItem(index, ec));
> +    }
> +
> +    static JSC::JSValue appendItem(JSWrapperType* wrapper, JSC::ExecState* exec, const JSC::ArgList& args, ConversionCallback conversion)
> +    {
> +        ExceptionCode ec = 0;
> +        SVGListBase* listImp = wrapper->impl();
> +        return finishSetter(exec, ec, wrapper->context(), wrapper->impl(),
> +                            listImp->appendItem(PODListItem::copy((*conversion)(args.at(0))), ec));
> +    }

Same comments in these functions re: listImp and function pointers.

> +++ WebCore/bindings/scripts/CodeGeneratorJS.pm	(working copy)
> @@ -1588,8 +1588,28 @@ sub GenerateImplementation
>                  push(@implContent, "        return jsUndefined();\n");
>              }
>  
> +            # Special case for JSSVGLengthList / JSSVGTransformList / JSSVGPointList / JSSVGNumberList
> +            # Instead of having JSSVG*Custom.cpp implementations for the SVGList interface for all of these
> +            # classes, we directly forward the calls to JSSVGCustomListTemplate, which centralizes the otherwhise

Typo: otherwhise

> +    <text id="ttt" x="10" y="200">Passes, if text is at 200x20</text>

I initially misread this as "200x200" and thought the pixel results were wrong.
Maybe choose some numbers that don't look so similar?

[Nikolas Zimmermann]

Created attachment 46289 [details]
Patch

[Nikolas Zimmermann]

(In reply to comment #11)
> 
> Typo: JSSVGCusotmListTemplate
Fixed.

> It would be great to have some function- and file-level comments about the
> changes you made.
Fixed.

> Using svn cp to create this file by copying JSSVGTransformListCustom.cpp or
> JSSVGPointListCustom.cpp would make it clearer where this code came from
> (especially since your ChangeLog doesn't explicitly say).
Fixed.

> 
> > +template<typename JSWrapperType, typename PODType, typename PODTypeList>
> > +class JSSVGCustomListTemplate {
> 
> I worry that adding even more templates to SVG code will increase the code size
> of WebCore again. Is there any way we can use type erasure to reduce the number
> of template parameters, at least? (E.g., is there some base class that all
> JSWrapperTypes share that we could use instead?)
> 
> I don't think the name of this class is great. Certainly I don't think
> "Template" makes its purpose any clearer.

As disucssed on IRC, it's now named: JSSVGPODListCustom, to show the analogy between the old-solution and the new one. Furhtermore it doesn't contain a class anymore, but JSSVGPODListCustom is a namespace now, containing public static templatified helper functions. Could reduce the whole thing two two template arguments instead of three.

> 
> Since this class is never instantiated and only has static member functions, I
> think it might make more sense to move the functions out of the class entirely
> and get rid of the class.
Fixed.
 
> Is it bad to call svgAttributeChanged if the list was already cleared before
> this function was called? (E.g., if you called clear twice in a row.) The same
> question applies to all the other setters.
Hm, that's for sure a possible further optimization, but it out-of-scope for this patch.
 
> The listImp local variable doesn't seem very helpful here.
It's useful again now, as the duplicated wrapper->impl() call is gone.
 
> You can use function pointers without the (*) syntax. Just treat it as a normal
> function call.
Hot, I did not even know that :-)

> Same comments in these functions re: listImp and function pointers.
All fixed.

> Typo: otherwhise
Fixed.

> 
> > +    <text id="ttt" x="10" y="200">Passes, if text is at 200x20</text>
> 
> I initially misread this as "200x200" and thought the pixel results were wrong.
> Maybe choose some numbers that don't look so similar?

Well I don't think this matters, as you'd notice a "FAIL:" if we ever break this test again, as I said before, the png really doesn't matter here. And I already started webkit-patch to upload the patch, so I'm leaving this at 200x20 now :-)

Thanks for the good review!

[Adam Roben (:aroben)]

Comment on attachment 46289 [details]
Patch

> +        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(args.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

[Nikolas Zimmermann]

(In reply to comment #14)
> (From update of attachment 46289 [details])
> > +        Introduce JSSVGCustomListTemplate, refactoring the existing custom code for SVG POD type lists.
> 
> This is now spelled correctly, but doesn't reflect your patch :-)
Awww, fixed ;-)

> Ditto.
Ditto :-)

> I think the common term is "templatized".
Fixed,
 
> Seems like these functions don't need the JSPODListType template parameter.
True.

> 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.)
Unfortunately that won't help still need to pass <PODType>. But it's better than passing two arguments.

> You also don't need the "&" here to create the function pointer. (C++ allows
> omitting it.)
Again news to me :-)

Will fix before landing.

[Nikolas Zimmermann]

Committed r53086: <http://trac.webkit.org/changeset/53086>