Bug 47905 – Redesign SVGAnimatedProperty concept to share "POD type wrappers" between all bindings (-> add ObjC SVG bindings)

• Component: SVG
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

The current SVGAnimatedTemplate code is hacky and slow.
It requires all bindings to implement special concepts to support SVGAnimated* properties (SVGAnimatedLength etc.)
Only JS implements this for now, provided by JSSVGPODTypeWrapper & JSSVGContextCache.

There should be a central binding agnostic implementation, so each language bindings can support a working (!) SVG DOM API.

I'll highlight the problems by looking at the simple property SVGLength used in SVGRectElement. Per SVG DOM definition, SVGRectElement exposes
"readonly attribute SVGAnimatedLength x".

SVGAnimatedLength exposes
"readonly attribute SVGLength baseVal"
"readonly attribute SVGLength animVal"

SVG demands these properties to be "live" that means you can do from JS:
var foobar = rect.x.baseVal; foobar.value = 150;

This kind of liveness would be easy to achive if we'd store and pass around SVGLength* objects (heap allocated, and refcounted). Then a simple
JSSVGLength wrapper would just modify the value and the change would be immediately reflected, as the same SVGLength* object would be stored
in SVGRectElement.

As we're storing POD types stack allocated in SVGRectElement (SVGLength m_x/y/width/height..) we need to invent special logic to allow the liveness.
We currently do:

Create a JSSVGPODTypeWrapper<SVGLength> object, which owns function pointers to SVGRectElement::setX() / x() methods.

If the JS wrapper for a SVGLength object is created, it calls the rect->x() method, to obtain a copy of a SVGLength object, which is stored on stack in the JSSVGLength wrapper.
Upon any function call or property change (length.setValue(150) / length.value = 150) following happens:

SVGLength newLength(oldLength);
newLength.setValue(150);
rectElement->setX(newLength);
rectElement->svgAttributeChanged(SVGNames::xAttr);

This is slow and hacky. Next example: SVGTextElement and SVGLengthList. All the SVG*List classes are highly questionable. As SVG DOM demands any list item to be live,
we're now storing a SVGLengthList internally as Vector<RefPtr<SomeObjectThatWrapsSVGLength> >, where we really want a Vector<SVGLength>.

Solve all these problems by redesigning SVGAnimatedProperty, and offering a central concept to implement the SVG DOM API for all languages (JS / ObjC / CPP...)
Remove JSSVGContextCache, the ANIMATED_PROPERTY_* macros, JSSVGPODTypeWrapper, SVGAnimatedTemplate and all its friends.

NOTE: To keep the patch size reasonable, I'm only converting SVGLength & SVGLengthList as first step, including extensive tests, that the SVGList API is finally implemented correctly. The ANIMATED_PROPERTY* macros also had to stay, otherwhise the patch size would blow. The macros should be expanded and removed, once all primitives types are converted to the new style SVGAnimatedProperty.

[Nikolas Zimmermann]

Created attachment 71171 [details]
Patch

Not meant for review yet, still need to write a ChangeLog and finish the CodeGeneratorV8.pm changes. Need a chromium EWS result to continue though....

[WebKit Review Bot]

Attachment 71171 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2
Updating OpenSource
RA layer request failed: OPTIONS of 'http://svn.webkit.org/repository/webkit': could not connect to server (http://svn.webkit.org) at /usr/lib/git-core/git-svn line 2295

Died at WebKitTools/Scripts/update-webkit line 129.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nikolas Zimmermann]

Created attachment 71175 [details]
Patch v2

Regenerate the patch, something went wrong before. Left out the LayoutTests, as I only want EWS results now.
NOTE: A large part of this patch is about renaming the existing SVGAnimated* files to DeprecatedSVGAnimated*.

[WebKit Review Bot]

Attachment 71175 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/svg/properties/SVGAnimatedPropertyMacros.h:27:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/svg/SVGAnimatedLengthList.h:25:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/svg/SVGElement.h:113:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/bindings/v8/custom/V8SVGLengthCustom.cpp:52:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/svg/SVGAnimatedLength.h:25:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/svg/properties/SVGAnimatedProperty.h:25:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/bindings/scripts/CodeGeneratorV8.pm:454:  Line contains tab character.  [whitespace/tab] [5]
WebCore/bindings/scripts/CodeGeneratorV8.pm:916:  Line contains tab character.  [whitespace/tab] [5]
WebCore/bindings/scripts/CodeGeneratorV8.pm:1029:  Line contains tab character.  [whitespace/tab] [5]
WebCore/bindings/scripts/CodeGeneratorV8.pm:1030:  Line contains tab character.  [whitespace/tab] [5]
WebCore/bindings/scripts/CodeGeneratorV8.pm:1031:  Line contains tab character.  [whitespace/tab] [5]
WebCore/bindings/scripts/CodeGeneratorV8.pm:2575:  Line contains tab character.  [whitespace/tab] [5]
WebCore/bindings/scripts/CodeGeneratorV8.pm:2851:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 13 in 54 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Attachment 71175 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4459102

[WebKit Review Bot]

Attachment 71175 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4466102

[Eric Seidel (no email)]

Attachment 71175 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4509001

[WebKit Review Bot]

Attachment 71175 [details] did not build on win:
Build output: http://queues.webkit.org/results/4514001

[WebKit Review Bot]

Attachment 71175 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/4538001

[Nikolas Zimmermann]

Comment on attachment 71175 [details]
Patch v2

Clear review flag. I'm splitting off the SVGAnimated* -> DeprecatedSVGAnimated* move first, to upload a new version of this patch fixing the style issues, adding a ChangeLog, and updating the build systems. The patch is again larger, so I want to do the SVGAnimated* -> DeprecatedSVGAnimated* first in a seperate bug, otherwhise this is just too large.

[Nikolas Zimmermann]

Created attachment 71266 [details]
Patch v3

Didn't try a local build so far, just want some EWS feedback from other platforms. The SVGAnimatedProperty -> DeprecatedSVGAnimatedProperty patch landed.
Chromium is most interessting, as the V8 changes aren't done yet, it definately won't build there yet.

[WebKit Review Bot]

Attachment 71266 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/svg/SVGAnimatedLength.h:25:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 45 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Attachment 71266 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4540010

[Nikolas Zimmermann]

Created attachment 71267 [details]
Patch v4

Add all new files to all build systems, that list headers. Fix one style issue. Let's see...

[Nikolas Zimmermann]

Created attachment 71268 [details]
Patch v5

Oops, uploaded the wrong patch.

[WebKit Review Bot]

Attachment 71268 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4566009

[Nikolas Zimmermann]

Created attachment 71282 [details]
Patch v6

More V8 work, let's see if it builds now...

[WebKit Review Bot]

Attachment 71282 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4542013

[Nikolas Zimmermann]

(In reply to comment #18)
> Attachment 71282 [details] did not build on chromium:
> Build output: http://queues.webkit.org/results/4542013

Got a local chromium build now, otherwhise I'm not able to fix V8...

[Nikolas Zimmermann]

Created attachment 71327 [details]
Patch v7

This should build on chromium, at least it does here locally :-)

[Nikolas Zimmermann]

Created attachment 71402 [details]
Patch v8

It works on JSC & V8, finally! This patch just adds a real ChangeLog.

[Dirk Schulze]

Comment on attachment 71402 [details]
Patch v8

View in context: https://bugs.webkit.org/attachment.cgi?id=71402&action=review

> WebCore/bindings/scripts/CodeGeneratorV8.pm:1065
> +    

remove the spaces

Had a looong review session on skype. Really great patch. And a very detailed test suite.
r=me

[Eric Seidel (no email)]

I'm glad those reviews happen.  I wish Nico wrote smaller patches, and more reviews happened over bugs.webkit.org so the rest of us would share the knowledge.

As is, I'm not sure I really understand SVG anymore, partially due to all the development happening in private.

Thanks for the patch.

[Nikolas Zimmermann]

(In reply to comment #23)
> I'm glad those reviews happen.  I wish Nico wrote smaller patches, and more reviews happened over bugs.webkit.org so the rest of us would share the knowledge.
I wish the patch should have been smaller as well. But I only took one primitive type and one list to start. SVGLength and SVGLengthList.
 
> As is, I'm not sure I really understand SVG anymore, partially due to all the development happening in private.
Please have a look at the ChangeLog, I do my best to describe the changes in detail, so anyone interessted can follow the work. I'm not sure what else I could do...

[Nikolas Zimmermann]

Landed in r70223.

Leaving this bug open as master bug.

[Nikolas Zimmermann]

Comment on attachment 71402 [details]
Patch v8

Clearing review flag.

[Nikolas Zimmermann]

This bug is completely fixed, I'm now working on 42025, to avoid including SVGNames.h in the svg/ headers.