[Webkit-unassigned] [Bug 22384] svg <text> fails to update when setting x/y
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jan 11 08:27:36 PST 2010
https://bugs.webkit.org/show_bug.cgi?id=22384
--- Comment #11 from Adam Roben (aroben) <aroben at apple.com> 2010-01-11 08:27:36 PST ---
(From update of attachment 46273)
> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog (revision 53075)
> +++ WebCore/ChangeLog (working copy)
> @@ -1,3 +1,46 @@
> +2010-01-11 Nikolas Zimmermann <nzimmermann at rim.com>
> +
> + 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?
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list