Bug 47905

Summary: Redesign SVGAnimatedProperty concept to share "POD type wrappers" between all bindings (-> add ObjC SVG bindings)
Product: WebKit Reporter: Nikolas Zimmermann <zimmermann>
Component: SVGAssignee: Nikolas Zimmermann <zimmermann>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, eric, gustavo, krit, mdelaney7, staikos, webkit-ews, webkit.review.bot, xan.lopez, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 47973, 48125, 48179, 48204, 48469, 48623, 48686, 48715, 48822, 48898, 48979, 48990, 49067, 49311, 49580, 49796, 50229    
Bug Blocks: 42025    
Attachments:
Description Flags
Patch
none
Patch v2
none
Patch v3
none
Patch v4
none
Patch v5
none
Patch v6
none
Patch v7
none
Patch v8 none

Description Nikolas Zimmermann 2010-10-19 07:38:08 PDT
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.
Comment 1 Nikolas Zimmermann 2010-10-19 08:04:59 PDT
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....
Comment 2 WebKit Review Bot 2010-10-19 08:10:36 PDT
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.
Comment 3 Nikolas Zimmermann 2010-10-19 09:02:37 PDT
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*.
Comment 4 WebKit Review Bot 2010-10-19 09:10:51 PDT
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.
Comment 5 Early Warning System Bot 2010-10-19 09:26:53 PDT
Attachment 71175 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4459102
Comment 6 WebKit Review Bot 2010-10-19 10:14:46 PDT
Attachment 71175 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4466102
Comment 7 Eric Seidel (no email) 2010-10-19 10:56:24 PDT
Attachment 71175 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4509001
Comment 8 WebKit Review Bot 2010-10-19 11:05:31 PDT
Attachment 71175 [details] did not build on win:
Build output: http://queues.webkit.org/results/4514001
Comment 9 WebKit Review Bot 2010-10-19 11:41:47 PDT
Attachment 71175 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/4538001
Comment 10 Nikolas Zimmermann 2010-10-20 00:44:51 PDT
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.
Comment 11 Nikolas Zimmermann 2010-10-20 02:33:30 PDT
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.
Comment 12 WebKit Review Bot 2010-10-20 02:36:43 PDT
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.
Comment 13 Early Warning System Bot 2010-10-20 02:56:56 PDT
Attachment 71266 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4540010
Comment 14 Nikolas Zimmermann 2010-10-20 02:59:15 PDT
Created attachment 71267 [details]
Patch v4

Add all new files to all build systems, that list headers. Fix one style issue. Let's see...
Comment 15 Nikolas Zimmermann 2010-10-20 03:06:42 PDT
Created attachment 71268 [details]
Patch v5

Oops, uploaded the wrong patch.
Comment 16 WebKit Review Bot 2010-10-20 03:45:46 PDT
Attachment 71268 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4566009
Comment 17 Nikolas Zimmermann 2010-10-20 06:26:21 PDT
Created attachment 71282 [details]
Patch v6

More V8 work, let's see if it builds now...
Comment 18 WebKit Review Bot 2010-10-20 07:42:37 PDT
Attachment 71282 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4542013
Comment 19 Nikolas Zimmermann 2010-10-20 07:43:56 PDT
(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...
Comment 20 Nikolas Zimmermann 2010-10-20 13:26:05 PDT
Created attachment 71327 [details]
Patch v7

This should build on chromium, at least it does here locally :-)
Comment 21 Nikolas Zimmermann 2010-10-21 01:04:58 PDT
Created attachment 71402 [details]
Patch v8

It works on JSC & V8, finally! This patch just adds a real ChangeLog.
Comment 22 Dirk Schulze 2010-10-21 02:55:40 PDT
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
Comment 23 Eric Seidel (no email) 2010-10-21 08:14:02 PDT
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.
Comment 24 Nikolas Zimmermann 2010-10-21 08:30:53 PDT
(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...
Comment 25 Nikolas Zimmermann 2010-10-22 05:28:35 PDT
Landed in r70223.

Leaving this bug open as master bug.
Comment 26 Nikolas Zimmermann 2010-10-23 03:46:20 PDT
Comment on attachment 71402 [details]
Patch v8

Clearing review flag.
Comment 27 Nikolas Zimmermann 2010-12-01 02:06:03 PST
This bug is completely fixed, I'm now working on 42025, to avoid including SVGNames.h in the svg/ headers.