Bug 48686 – Convert SVGAnimatedNumber/SVGAnimatedNumberList to the new SVGAnimatedPropertyTearOff concept

• Component: SVG
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

Convert SVGAnimatedNumber/SVGAnimatedNumberList to the new SVGAnimatedPropertyTearOff concept

[Nikolas Zimmermann]

Created attachment 72400 [details]
Patch

Not meant for final review, not even sure if it builds, but I'd like to see EWS results for V8 in the morning, whether it already builds.
I'm only certain so far that the JSSVG* and DOMSVG* bindings generation works as expected.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/svg/SVGComponentTransferFunctionElement.cpp:57:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Total errors found: 1 in 41 files


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

[Early Warning System Bot]

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

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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

[Nikolas Zimmermann]

Created attachment 72430 [details]
Patch v2

Tested a local mac build, now works w/o problems and zero regressions. It fixed two svg/dynamic-updates tests, that were silently failing before. Also discovered errors in two of them, regarding SVGLengthList (which did not properly remove items from a list, if they were already present in another list - fixed by switching to the new svg animated list property handling).

Patch is larger, as tests have been added that check StrictTypeChecking for all SVGAnimated* types that have been converted recently. The actual code size is smaller!

[Nikolas Zimmermann]

Forgot to mention V8 build is still expected to fail, just syncing my local chrome copy, then testing the patch on v8/chrome.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/svg/SVGFEConvolveMatrixElement.cpp:152:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/svg/SVGComponentTransferFunctionElement.cpp:57:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Total errors found: 2 in 87 files


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

[Eric Seidel (no email)]

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

[Rob Buis]

Comment on attachment 72430 [details]
Patch v2

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

R- mainly for the tests that are not relevant to the change.

> WebCore/ChangeLog:10
> +

Typo (wheter -> wether).
The ChangeLog does not seem to state that SVGAnimatedNumber/SVGAnimatedNumberList are being converted, apart from the Bug title, may need to be restated/rephrased.

> WebCore/ChangeLog:21
> +

Only SVGAnimatedNumber(List) needs to be tested, others should be done in another patch.

> WebCore/ChangeLog:44
> +        * svg/SVGAnimatedRect.idl: Ditto.

Are these idl changes needed (except for SVGAnimatedNumber(List).idl)?

> WebCore/svg/SVGFEConvolveMatrixElement.cpp:152
> +	int kernelMatrixSize = kernelMatrix.size();

Layout

> WebCore/svg/SVGNumberList.h:37
>  

Layout.

> LayoutTests/ChangeLog:20
> +        * svg/dom/SVGAnimatedBoolean.html: Copied from LayoutTests/svg/dom/SVGExternalResourcesRequired.html.

Is that a git mesage? Should it just say Added?

> LayoutTests/ChangeLog:39
> +        * svg/dom/script-tests/SVGAnimatedBoolean.js: Copied from LayoutTests/svg/dom/script-tests/SVGExternalResourcesRequired.js.

Ditto.

[Nikolas Zimmermann]

(In reply to comment #10)
> (From update of attachment 72430 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72430&action=review
> 
> R- mainly for the tests that are not relevant to the change.
> 
> > WebCore/ChangeLog:10
> > +
> 
> Typo (wheter -> wether).
> The ChangeLog does not seem to state that SVGAnimatedNumber/SVGAnimatedNumberList are being converted, apart from the Bug title, may need to be restated/rephrased.
Fixed.

> 
> > WebCore/ChangeLog:21
> > +
> 
> Only SVGAnimatedNumber(List) needs to be tested, others should be done in another patch.
And SVGNumber needs to be tested.

> 
> > WebCore/ChangeLog:44
> > +        * svg/SVGAnimatedRect.idl: Ditto.
> 
> Are these idl changes needed (except for SVGAnimatedNumber(List).idl)?
None, I've seperated them.

> 
> > WebCore/svg/SVGFEConvolveMatrixElement.cpp:152
> > +	int kernelMatrixSize = kernelMatrix.size();
> 
> Layout
Fixed,

> 
> > WebCore/svg/SVGNumberList.h:37
> >  
> 
> Layout.
I don't see a problem here.

> 
> > LayoutTests/ChangeLog:20
> > +        * svg/dom/SVGAnimatedBoolean.html: Copied from LayoutTests/svg/dom/SVGExternalResourcesRequired.html.
> 
> Is that a git mesage? Should it just say Added?
No I moved the file around, will do it in a later patch.

[Nikolas Zimmermann]

Created attachment 72433 [details]
Patch v3

Address Robs comments. This also builds on V8/Chrome.

[Rob Buis]

Comment on attachment 72433 [details]
Patch v3

Looks great!

[Nikolas Zimmermann]

Landed in r70979.

[Darin Adler]

The patch did not include the file SVGAnimatedNumber.js, so the SVGAnimatedNumber.html test is now failing on all the bots. Please add that file!

[Nikolas Zimmermann]

(In reply to comment #15)
> The patch did not include the file SVGAnimatedNumber.js, so the SVGAnimatedNumber.html test is now failing on all the bots. Please add that file!

Oops, sorry. When I landed the patch, I noticed that build.webkit.org was down, so I couldn't verify everything worked :(

Thanks for pinging me, fixed in r70990.