[Webkit-unassigned] [Bug 22384] svg <text> fails to update when setting x/y

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=22384


Nikolas Zimmermann <zimmermann at kde.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |zimmermann at kde.org




--- Comment #13 from Nikolas Zimmermann <zimmermann at kde.org>  2010-01-11 10:42:53 PST ---
(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!

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.