Bug 10490

Created attachment 10138 [details] ruby script to remove (most of) broken animation code Pretty much the only part missing here is a solution for baseVal() and setBaseVal() calls. We will need to implement the new baseValueHash code before removal of those calls is "a good idea".

Created attachment 10191 [details] Newer version of ruby script This script, when used in conjunction with the about-to-be-attached patch, will completely remove all of the old broken SVGAnimated* code. The final result nearly compiles (~20 minor errors left, and some linking to work out). I'm posting this now to begin an architectural discussion.

Created attachment 10192 [details] patch containing suplemental manual changes

Created attachment 10193 [details] combined patch (script and manual changes) This is only for reference.

Created attachment 10235 [details] better version of the ruby script This is a ruby script to remove the old SVGAnimated* code.

Created attachment 10236 [details] patch containing manual changes, applied by the script

Created attachment 10237 [details] complete patch, containing script and manaul changes (compiles, links, crashes)

mjs, wildfox/rwlbuis and/or darin should take a breeze through this patch & script. I'm not looking for a formal review. Just an architectural commentary. The old system, involved SVGAnimated* objects which were held by RefPtrs for each property for each element. This was an 8byte waste for every property, because a pointer to the real SVG* object was kept for both the base and anim value, even though the anim value was almost never used. This also lead to extra-ugly c++ code like x()->baseValue()->value() just to get the damn float value out of a coordinate. The new system is ugly in its own way. It uses macros to define custom accessors for every animated property on every SVGElement subclass which needs them. The good part here is that this results in a space savings of 8 bytes as we no longer keep these intermediate pointers. Base values are now stored off in their own hash in the SVGDocumentExtensions, animVals are just the normal values stored in the renderer/dom objects. We also have cleaner access to these values in the c++ code: x() => gives you the real float anim value xBaseValue() => gives you the real float base value There is one problem with this patch which causes crashes currently. That is old code like this: foo()->baseVal()->doSomething() would assume that baseVal() would end up lazy_create-ing a new default value on first access (lazy_create is an old macro) the modern equivilent re-write of that code (from the script) is: fooBaseValue()->doSomething() which ends up crashing. fooBaseValue() has no default value (currently) so just returns 0. There are three ways to fix this: 1. initialize all default values in the constructors 2. bring back lazy_create, by passing a default value to the macros which define these accessors 3. change all the callsites of this type to call setFooBaseValue() instead. #3 is probably actually the "cleanest" approach. I'll probably go with #2 for now though (as that should be the smallest code change).

Actually, the old system was a 16 byte waste for every property: 8 bytes, SVGAnimatedFoo RefPtr (pointer and refcount) 8 bytes, SVGFoo baseVal RefPtr 8 bytes, SVGFoo animVal RefPtr In the new system, we just have 8 bytes, SVGFoo "animated value" RefPtr The base value is stored off in a separate hash, and only created if needed (for animation). All of the base value functions fall through to call their animated value equivalents if there is no base value stored. Also, I never quite explained why I needed to use macros. There are a number of reasons, but mostly it's that I couldn't find any other way to implement this sort of barBaseValue() semantics: Foo* MyElement::barBaseValue() const { Foo* baseValue = document()->accessSVGDocumentExtensions->fooBaseValue(barAttr); if (baseValue) return baseValue; return bar(); }

I think a lot of the macro stuff could be done more cleanly with templates. For instance, instead of the macro for the various baseValue getters, you could have: template <typename ValueType, ValueType emptyValue> ValueType SVGDocumentExtensions::baseValue(const SVGElement* element, const AtomicString& propertyName) const {     HashMap<StringImpl*, ValueType> propertyMap = baseValueMap<ValueType>().get(element);     .... }

ANIMATED_PROPERTY_DEFINITIONS can probably be templatized too. Also, the reinterpret_cast in there is very likely unsafe.

Heya(In reply to comment #11) > ANIMATED_PROPERTY_DEFINITIONS can probably be templatized too. Also, the > reinterpret_cast in there is very likely unsafe. > Heya Eric/Maciej, I've worked quite a lot on this patch - now it actually works. What did I change? : * Removed the reinterpret_cast, and offer two macros ANIMATED_PROPERTY_OFFERS_CONTEXT / ANIMATED_PROPERTY_NEEDS_CONTEXT Classes which don't inherit from SVGElement themselves, but still store animated properties need a way to access the SVGDocumentExtensions. Using these macros in ie. SVGURIReference it just says NEEDS_CONTEXT, which will evaluate to a pure virtual function. All top-base classes specify OFFERS_CONTEXT. This way the access to the SVG document extensions will work w/o any ugly casts. I adjusted the Ruby script to generate these OFFERS/NEEDED macros in the right places. * After running the script the default value initialization had to be fixed (manually :( ) I went through all classes where the lazy_creates have been removed from, and moved the initialization directly into the constructor. This makes the whole thing work, and the regression tests pass w/o any intrduced problem! Because it's very hard to split these patches up into parts (as I fiddled around _after_ the script has been run), I will just post a big 280k patch. Eric has already seen the relevant things in the patch. Happy reviewing! :-)

Created attachment 10390 [details] Improved patch, no crashes. All layout tests pass, except one SVG tests which makes problems: inner-percent.svg - KCanvasContainer {svg} at (0,0) size 100x100 - KCanvasItem {rect} at (0,0) size 100x100 [fill={[type=SOLID] [color=#008000]}] [data="M0.00,0.00L100.00,0.00L100.00,100.00L0.00,100.00"] + KCanvasContainer {svg} at (-2147483648,-2147483648) size -2147483648x-2147483648 + KCanvasItem {rect} at (-2147483648,-2147483648) size -2147483648x-2147483648 [fill={[type=SOLID] [color=#008000]}] [data="M0.00,0.00L100.00,0.00L100.00,100.00L0.00,100.00"] As you can see x/y/width/height are screwed up. I'll look into fixing that soon. Not setting review flag yet, until this last issue is fixed.

Created attachment 10391 [details] Final patch, all tests pass! Finally all tests pass, stupid error in SVGSVGElement in the previous patch. Now officially asking for review. Eric - let's make it rock!

I didn't include a ChangeLog in the "Final patch, all tests pass!" It's 15k and you can find it here: http://ktown.kde.org/~wildfox/ktown/WebKit-Patches/anim-patch-changelog.diff Cheers, Niko

Comment on attachment 10391 [details] Final patch, all tests pass! Definitely should add a ChangeLog, even if it's huge. ;) + + # FIXME: TEMPORARY HACK! That note needs to explain what's a hack about this and why it's necessary. your WebCore/CMakeLists.txt change looks wrong. These lines should eventually change, not necessarily in this patch: + float xOffset = image->xBaseValue() ? image->xBaseValue()->value() : 0; + float yOffset = image->yBaseValue() ? image->yBaseValue()->value() : 0; Yet another reason why the current SVGLength code needs to change. + , m_x(new SVGLength(this, LM_WIDTH, viewportElement())) These will not resolve correctly, since there is no parent during construction of an element, thus no viewportElement(). Length objects (in html) resolve during layout time. SVG needs to move towards that as well. I hope these get ref'd by the SVGLength, otherwise this is just waiting to crash: + m_height->setValueAsString(String("100%").impl()); I'm not sure that all of the places we're currently using baseVal() are correct. This being one example: KCComponentTransferFunction SVGComponentTransferFunctionElement::transferFunction() const Since that function is generating something to be rendered, it proably should use plain old val(). We'll have lots of these issues when we start dealing with animation. These can of course take contextElement() now that you've made such: + : m_viewBox(new SVGRect(0 /* FIXME */)) + , m_preserveAspectRatio(new SVGPreserveAspectRatio(0 /* FIXME */)) This code indicates that our storage model is still broken: bool hasRx = hasAttribute(String("rx").impl()); bool hasRy = hasAttribute(String("ry").impl()); if(hasRx || hasRy) the fact that we have to check hasAttribute (a secondary storage method) and then grab the "actual" value off of the element, seems broken. In this case, better would be to check to see if the value on the element was something other than the default. In general, I wonder why we're still storing the values in at least 3 palces (1. attributes hash, 2. on the dom element. 3. in the renderer). I think this macro should just be removed, and the function name used instead: + ANIMATED_PROPERTY_OFFERS_CONTEXT I don't think the macro offers anything in terms of clarity. When all of the length resolution is moved to be done during layout() time in the rendering tree, this sort of "context" setting can go away: + xBaseValue()->setContext(context); Does this default to percent? + // Spec: If the attribute is not specified, the effect is as if a value of "50%" were specified. + m_cx->setValue(0.5); Eventually we should remove this: value.deprecatedString().toDouble() since depreatedString() is not necessary there. I'm about half way through, but I need to take a break. KCanvasMarker *SVGMarkerElement::canvasResource()

Created attachment 10406 [details] Updated patch Incorporated Eric's comments. This patch opens the doors for a lot of upcoming cleanup patches. I am aware of broken parts like the viewportElement() calls from the constructor, though it was broken before, and can be fixed in an other patch, as this one is already big enough :-)

Created attachment 10407 [details] Updated patch II The last patch was just a ChangeLog, my mistake. This time with a more verbose ChangeLog + the actual patch included.

Comment on attachment 10407 [details] Updated patch II So I've gone through this line-by-line with WildFox. That, and I helped write the initial changes. I'm confident this is ready to land. But I'd like a go-ahead from someone actually internal before I do so. (I don't want to break any big internal patches I don't know about.)

Landed r16244.