Bug 10490 – remove broken SVGAnimated* code

• Component: SVG
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

remove broken SVGAnimated* code

This code needs to die.  To be replace by a better animation implementation.

[Eric Seidel (no email)]

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".

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Created attachment 10192 [details]
patch containing suplemental manual changes

[Eric Seidel (no email)]

Created attachment 10193 [details]
combined patch (script and manual changes)

This is only for reference.

[Eric Seidel (no email)]

Created attachment 10235 [details]
better version of the ruby script

This is a ruby script to remove the old SVGAnimated* code.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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).

[Eric Seidel (no email)]

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();
}

[Maciej Stachowiak]

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); 
    .... 
}

[Maciej Stachowiak]

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

[Nikolas Zimmermann]

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! :-)

[Nikolas Zimmermann]

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.

[Nikolas Zimmermann]

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!

[Nikolas Zimmermann]

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

[Eric Seidel (no email)]

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()

[Nikolas Zimmermann]

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 :-)

[Nikolas Zimmermann]

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.

[Eric Seidel (no email)]

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.)

[Eric Seidel (no email)]

Landed r16244.