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.
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....
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.
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*.
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.
Attachment 71175 [details] did not build on qt: Build output: http://queues.webkit.org/results/4459102
Attachment 71175 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4466102
Attachment 71175 [details] did not build on mac: Build output: http://queues.webkit.org/results/4509001
Attachment 71175 [details] did not build on win: Build output: http://queues.webkit.org/results/4514001
Attachment 71175 [details] did not build on gtk: Build output: http://queues.webkit.org/results/4538001
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.
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.
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.
Attachment 71266 [details] did not build on qt: Build output: http://queues.webkit.org/results/4540010
Created attachment 71267 [details] Patch v4 Add all new files to all build systems, that list headers. Fix one style issue. Let's see...
Created attachment 71268 [details] Patch v5 Oops, uploaded the wrong patch.
Attachment 71268 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4566009
Created attachment 71282 [details] Patch v6 More V8 work, let's see if it builds now...
Attachment 71282 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4542013
(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...
Created attachment 71327 [details] Patch v7 This should build on chromium, at least it does here locally :-)
Created attachment 71402 [details] Patch v8 It works on JSC & V8, finally! This patch just adds a real ChangeLog.
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
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.
(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...
Landed in r70223. Leaving this bug open as master bug.
Comment on attachment 71402 [details] Patch v8 Clearing review flag.
This bug is completely fixed, I'm now working on 42025, to avoid including SVGNames.h in the svg/ headers.