Convert SVGAnimatedNumber/SVGAnimatedNumberList to the new SVGAnimatedPropertyTearOff concept
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.
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.
Attachment 72400 [details] did not build on qt: Build output: http://queues.webkit.org/results/4876017
Attachment 72400 [details] did not build on mac: Build output: http://queues.webkit.org/results/4807089
Attachment 72400 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4774081
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!
Forgot to mention V8 build is still expected to fail, just syncing my local chrome copy, then testing the patch on v8/chrome.
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.
Attachment 72430 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4782092
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.
(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.
Created attachment 72433 [details] Patch v3 Address Robs comments. This also builds on V8/Chrome.
Comment on attachment 72433 [details] Patch v3 Looks great!
Landed in r70979.
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!
(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.