Bug 191237 – Remove SVG properties tear-off objects

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Said Abou-Hallawa]

SVG properties tear-off objects have been troublesome and confusing part of SVG. These objects are created only when requested from DOM and SMIL animations. They wrap primitive or value members in the SVGElements. Their life cycle requires the SVGElement and all the objects in between to be alive even if they are not accessible from the anything else. Many things can go wrong trying to strongly tie the tear-off object with the SVGElement. Trying to fix these issues by relaxing the relationship leads to different kind of problems.

The current code

-- Does not reflect the specs which makes difficult to correlate between them.   
-- Lacks clear object life cycle, see all different kinds of pointes in SVGAnimatedProperty, SVGAnimatedPropertyTearOff and SVGAnimatedListPropertyTearOff
-- Lacks correct encapsulation, see the SVGPathElement, SVGAnimatedPathSegList and SVGPathSegList
-- Lacks strong type variable declaration, see SVGAnimatedType
-- Does not choose the right time and place to extract information, see many functions in SVGElement which includes HashMap or HashSet of attributes' names. See SVGAnimateElementBase::determineAnimatedPropertyType() and SVGAnimationElement::checkInvalidCSSAttributeType()
-- Have hacks to overcome the limitation of tear-off object creation and caching, see SVGTests::requiredFeatures() and SVGViewSpec::transform()
-- Has loose relationship between the animator and the form and the to values. See SVGAnimateElementBase.
-- Have correctness issues when animating the same attribute with multiple animators. See https://bugs.webkit.org/show_bug.cgi?id=150388.
-- Have correctness issues with optional attributes, see https://bugs.webkit.org/show_bug.cgi?id=154141.

[Said Abou-Hallawa]

Created attachment 353808 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/svg/properties/SVGAnimatedProperty.h:150:  Failed to find complete declaration of class PrimitiveShadow  [build/class] [5]
Total errors found: 1 in 234 files


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

[Said Abou-Hallawa]

Created attachment 353812 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/svg/properties/SVGAnimatedProperty.h:150:  Failed to find complete declaration of class PrimitiveShadow  [build/class] [5]
Total errors found: 1 in 234 files


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

[EWS Watchlist]

Comment on attachment 353812 [details]
Patch

Attachment 353812 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9858568

New failing tests:
svg/W3C-SVG-1.1/animate-elem-30-t.svg

[EWS Watchlist]

Created attachment 353815 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 353812 [details]
Patch

Attachment 353812 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9858574

New failing tests:
svg/W3C-SVG-1.1/animate-elem-30-t.svg

[EWS Watchlist]

Created attachment 353817 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 353812 [details]
Patch

Attachment 353812 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9858695

New failing tests:
svg/W3C-SVG-1.1/animate-elem-30-t.svg

[EWS Watchlist]

Created attachment 353818 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 353812 [details]
Patch

Attachment 353812 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9858742

New failing tests:
svg/dom/SVGScriptElement/script-set-href.svg
svg/W3C-SVG-1.1/animate-elem-30-t.svg
perf/svg-path-appenditem.html

[EWS Watchlist]

Created attachment 353820 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 353812 [details]
Patch

Attachment 353812 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9859515

New failing tests:
svg/W3C-SVG-1.1/animate-elem-30-t.svg

[EWS Watchlist]

Created attachment 353824 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 353812 [details]
Patch

Attachment 353812 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9859516

New failing tests:
svg/dom/SVGScriptElement/script-set-href.svg
svg/W3C-SVG-1.1/animate-elem-30-t.svg
perf/svg-path-appenditem.html

[EWS Watchlist]

Created attachment 353825 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Radar WebKit Bug Importer]

<rdar://problem/45887675>

[Said Abou-Hallawa]

*** Bug 191047 has been marked as a duplicate of this bug. ***

[Said Abou-Hallawa]

*** Bug 173178 has been marked as a duplicate of this bug. ***

[Said Abou-Hallawa]

Created attachment 355382 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/svg/properties/SVGAnimatedProperty.h:166:  Failed to find complete declaration of class PrimitiveShadow  [build/class] [5]
Total errors found: 1 in 268 files


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

[Said Abou-Hallawa]

Created attachment 355386 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/svg/properties/SVGAnimatedProperty.h:166:  Failed to find complete declaration of class PrimitiveShadow  [build/class] [5]
Total errors found: 1 in 268 files


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

[Said Abou-Hallawa]

Created attachment 355388 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/svg/properties/SVGAnimatedProperty.h:166:  Failed to find complete declaration of class PrimitiveShadow  [build/class] [5]
Total errors found: 1 in 268 files


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

[EWS Watchlist]

Comment on attachment 355388 [details]
Patch

Attachment 355388 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10096337

New failing tests:
svg/dom/SVGScriptElement/script-set-href.svg

[EWS Watchlist]

Created attachment 355393 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Dirk Schulze]

From the comment it was not entirely clear:
* if you remove SVG DOM or the Amin part of it? W/o replacement?
* SMIL continues to work.

SVG DOM animVal is discontinued by SVG WG. So that part would be fine I guess. For everything else I suggest informing WebKit-dev.

[Said Abou-Hallawa]

(In reply to Dirk Schulze from comment #28)
> From the comment it was not entirely clear:
> * if you remove SVG DOM or the Amin part of it? W/o replacement?

I am replacing the SVG properties and the SVG animated properties by proprieties owned by the SVG elements directly instead of the tear-off objects.

> * SMIL continues to work.
> 

Yes.

> SVG DOM animVal is discontinued by SVG WG. So that part would be fine I
> guess. For everything else I suggest informing WebKit-dev.

No. I did not remove the animVal because this would require a big change in the layout tests. The only SVG2 change, I did in this patch, was the insertion of non detached items in SVGLists. I will send an email to webkit-dev once a plan for landing this patch is determined.

Thanks.

[Said Abou-Hallawa]

Created attachment 355568 [details]
Patch

[Said Abou-Hallawa]

Created attachment 355574 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/svg/properties/SVGAnimatedProperty.h:166:  Failed to find complete declaration of class PrimitiveShadow  [build/class] [5]
Total errors found: 1 in 268 files


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

[Said Abou-Hallawa]

Created attachment 355575 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/svg/properties/SVGAnimatedProperty.h:166:  Failed to find complete declaration of class PrimitiveShadow  [build/class] [5]
Total errors found: 1 in 268 files


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

[Said Abou-Hallawa]

Created attachment 355577 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/svg/properties/SVGAnimatedProperty.h:166:  Failed to find complete declaration of class PrimitiveShadow  [build/class] [5]
Total errors found: 1 in 268 files


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

[Said Abou-Hallawa]

Created attachment 355580 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/svg/properties/SVGAnimatedProperty.h:166:  Failed to find complete declaration of class PrimitiveShadow  [build/class] [5]
Total errors found: 1 in 268 files


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

[Said Abou-Hallawa]

Created attachment 355582 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/svg/properties/SVGAnimatedProperty.h:166:  Failed to find complete declaration of class PrimitiveShadow  [build/class] [5]
Total errors found: 1 in 268 files


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

[EWS Watchlist]

Comment on attachment 355582 [details]
Patch

Attachment 355582 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10138151

New failing tests:
svg/dom/SVGScriptElement/script-set-href.svg

[EWS Watchlist]

Created attachment 355585 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Said Abou-Hallawa]

Created attachment 355666 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/svg/properties/SVGAnimatedProperty.h:166:  Failed to find complete declaration of class PrimitiveShadow  [build/class] [5]
Total errors found: 1 in 266 files


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

[Said Abou-Hallawa]

Created attachment 355672 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/svg/properties/SVGAnimatedProperty.h:166:  Failed to find complete declaration of class PrimitiveShadow  [build/class] [5]
Total errors found: 1 in 266 files


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

[Said Abou-Hallawa]

Created attachment 355693 [details]
Patch

[Said Abou-Hallawa]

Created attachment 355699 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/svg/properties/SVGAnimatedProperty.h:166:  Failed to find complete declaration of class PrimitiveShadow  [build/class] [5]
Total errors found: 1 in 266 files


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

[EWS Watchlist]

Comment on attachment 355699 [details]
Patch

Attachment 355699 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10162110

New failing tests:
svg/dom/SVGScriptElement/script-set-href.svg

[EWS Watchlist]

Created attachment 355711 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Said Abou-Hallawa]

Created attachment 355752 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/svg/properties/SVGAnimatedProperty.h:166:  Failed to find complete declaration of class PrimitiveShadow  [build/class] [5]
Total errors found: 1 in 266 files


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

[EWS Watchlist]

Comment on attachment 355752 [details]
Patch

Attachment 355752 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/10170799

New failing tests:
svg/dom/SVGScriptElement/script-set-href.svg
svg/animations/svglength-element-removed-crash.svg

[EWS Watchlist]

Created attachment 355777 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[EWS Watchlist]

Comment on attachment 355752 [details]
Patch

Attachment 355752 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10173237

New failing tests:
svg/dom/SVGScriptElement/script-set-href.svg

[EWS Watchlist]

Created attachment 355820 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Said Abou-Hallawa]

Created attachment 356333 [details]
Patch

[Said Abou-Hallawa]

Created attachment 356334 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/svg/properties/SVGAnimatedDecoratedProperty.h:33:  Failed to find complete declaration of class DecoratedProperty  [build/class] [5]
Total errors found: 1 in 291 files


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

[EWS Watchlist]

Comment on attachment 356334 [details]
Patch

Attachment 356334 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10239407

New failing tests:
svg/dom/SVGScriptElement/script-set-href.svg

[EWS Watchlist]

Created attachment 356338 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Said Abou-Hallawa]

Created attachment 356340 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/svg/properties/SVGAnimatedDecoratedProperty.h:33:  Failed to find complete declaration of class DecoratedProperty  [build/class] [5]
Total errors found: 1 in 291 files


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

[Said Abou-Hallawa]

Created attachment 356342 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/svg/properties/SVGAnimatedDecoratedProperty.h:33:  Failed to find complete declaration of class DecoratedProperty  [build/class] [5]
Total errors found: 1 in 291 files


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

[Said Abou-Hallawa]

Created attachment 356344 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/svg/properties/SVGAnimatedDecoratedProperty.h:33:  Failed to find complete declaration of class DecoratedProperty  [build/class] [5]
Total errors found: 1 in 291 files


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

[EWS Watchlist]

Comment on attachment 356344 [details]
Patch

Attachment 356344 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/10241566

New failing tests:
svg/dom/SVGScriptElement/script-set-href.svg
svg/animations/svglength-element-removed-crash.svg

[EWS Watchlist]

Created attachment 356346 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Said Abou-Hallawa]

Created attachment 356351 [details]
1-SVGProperty

[Said Abou-Hallawa]

Created attachment 356352 [details]
2-SVGPathSeg

[Said Abou-Hallawa]

Created attachment 356358 [details]
3-SVGList

[EWS Watchlist]

Comment on attachment 356344 [details]
Patch

Attachment 356344 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10245250

New failing tests:
svg/dom/SVGScriptElement/script-set-href.svg

[EWS Watchlist]

Created attachment 356360 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Said Abou-Hallawa]

Created attachment 356380 [details]
4-SVGDecoratedEnumeration

[Said Abou-Hallawa]

Created attachment 356497 [details]
5-SVGPathSegList

[Said Abou-Hallawa]

Created attachment 356500 [details]
6-SVGAnimatedProperty

[Said Abou-Hallawa]

Created attachment 356507 [details]
7-SVGAnimationFunction

[Said Abou-Hallawa]

Created attachment 356579 [details]
8-SVGAnimator.patch

[Said Abou-Hallawa]

Created attachment 356607 [details]
9-SVGAccessor

[Said Abou-Hallawa]

Created attachment 356728 [details]
10-SVGPropertyRegistry

[Said Abou-Hallawa]

Created attachment 356729 [details]
11-JSBinding

[Said Abou-Hallawa]

Created attachment 356730 [details]
12-FilesRemoval

[Said Abou-Hallawa]

Created attachment 356731 [details]
13-BasicShapes

[Said Abou-Hallawa]

Created attachment 356732 [details]
14-Resources

[Said Abou-Hallawa]

Created attachment 356733 [details]
15-Filters

[Said Abou-Hallawa]

Created attachment 356752 [details]
16-Text

[Said Abou-Hallawa]

Created attachment 356810 [details]
17-BaseTypes

[Said Abou-Hallawa]

Created attachment 356811 [details]
18-Conatiners

[Said Abou-Hallawa]

Created attachment 356879 [details]
19-SVGAnimateElement

[Said Abou-Hallawa]

Created attachment 356880 [details]
20-Miscellaneous

[Said Abou-Hallawa]

Created attachment 356881 [details]
21-Extra

[Said Abou-Hallawa]

Created attachment 356882 [details]
22-LayoutTests

[Said Abou-Hallawa]

Created attachment 356883 [details]
EWSPatch

[EWS Watchlist]

Comment on attachment 356882 [details]
22-LayoutTests

Attachment 356882 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10319655

New failing tests:
svg/dom/SVGAnimatedEnumeration-SVGMarkerElement.html
svg/dom/SVGLengthList-appendItem.xhtml
svg/dom/SVGLengthList-insertItemBefore.xhtml
svg/dom/SVGPointList-basics.xhtml
svg/dom/SVGPathSegList-insertItemBefore.xhtml
svg/dom/SVGPathSegList-clear-and-initialize.xhtml
svg/dom/SVGLengthList-replaceItem.xhtml
svg/dom/SVGLengthList-basics.xhtml
svg/dom/SVGNumberList-basics.xhtml
svg/dom/SVGLengthList-removeItem.xhtml
svg/dom/SVGLengthList-initialize.xhtml
svg/dom/SVGPathSegList-appendItem.xhtml
svg/dom/SVGPathSegList-replaceItem.xhtml
svg/dom/SVGTransformList-basics.xhtml
svg/animations/svglength-element-removed-crash.svg

[EWS Watchlist]

Created attachment 356884 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 356882 [details]
22-LayoutTests

Attachment 356882 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10319652

New failing tests:
svg/dom/SVGAnimatedEnumeration-SVGMarkerElement.html
svg/dom/SVGLengthList-appendItem.xhtml
svg/dom/SVGLengthList-insertItemBefore.xhtml
svg/dom/SVGPointList-basics.xhtml
svg/dom/SVGPathSegList-insertItemBefore.xhtml
svg/dom/SVGPathSegList-clear-and-initialize.xhtml
svg/dom/SVGLengthList-replaceItem.xhtml
svg/dom/SVGLengthList-basics.xhtml
svg/dom/SVGNumberList-basics.xhtml
svg/dom/SVGLengthList-removeItem.xhtml
svg/dom/SVGLengthList-initialize.xhtml
svg/dom/SVGPathSegList-appendItem.xhtml
svg/dom/SVGPathSegList-replaceItem.xhtml
svg/dom/SVGTransformList-basics.xhtml
svg/animations/svglength-element-removed-crash.svg

[EWS Watchlist]

Created attachment 356885 [details]
Archive of layout-test-results from ews105 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 356882 [details]
22-LayoutTests

Attachment 356882 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10319710

New failing tests:
svg/dom/SVGAnimatedEnumeration-SVGMarkerElement.html
svg/dom/SVGLengthList-appendItem.xhtml
svg/dom/SVGLengthList-insertItemBefore.xhtml
svg/dom/SVGPointList-basics.xhtml
svg/dom/SVGPathSegList-insertItemBefore.xhtml
svg/dom/SVGPathSegList-clear-and-initialize.xhtml
svg/dom/SVGLengthList-initialize.xhtml
svg/dom/SVGNumberList-basics.xhtml
svg/dom/SVGLengthList-removeItem.xhtml
svg/dom/SVGLengthList-replaceItem.xhtml
svg/dom/SVGPathSegList-replaceItem.xhtml
svg/dom/SVGPathSegList-appendItem.xhtml
svg/dom/SVGLengthList-basics.xhtml
svg/dom/SVGTransformList-basics.xhtml

[EWS Watchlist]

Created attachment 356887 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 356882 [details]
22-LayoutTests

Attachment 356882 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/10319911

New failing tests:
svg/dom/SVGAnimatedEnumeration-SVGMarkerElement.html
svg/dom/SVGLengthList-appendItem.xhtml
svg/dom/SVGLengthList-insertItemBefore.xhtml
svg/dom/SVGPointList-basics.xhtml
svg/dom/SVGPathSegList-insertItemBefore.xhtml
svg/dom/SVGPathSegList-clear-and-initialize.xhtml
svg/dom/SVGLengthList-replaceItem.xhtml
svg/dom/SVGLengthList-basics.xhtml
svg/dom/SVGNumberList-basics.xhtml
svg/dom/SVGLengthList-removeItem.xhtml
svg/dom/SVGLengthList-initialize.xhtml
svg/dom/SVGPathSegList-appendItem.xhtml
svg/dom/SVGPathSegList-replaceItem.xhtml
svg/dom/SVGTransformList-basics.xhtml
svg/animations/svglength-element-removed-crash.svg

[EWS Watchlist]

Created attachment 356888 [details]
Archive of layout-test-results from ews203 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews203  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[EWS Watchlist]

Comment on attachment 356882 [details]
22-LayoutTests

Attachment 356882 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10319702

New failing tests:
svg/dom/SVGAnimatedEnumeration-SVGMarkerElement.html
svg/dom/SVGLengthList-appendItem.xhtml
svg/dom/SVGLengthList-insertItemBefore.xhtml
svg/dom/SVGPointList-basics.xhtml
svg/dom/SVGPathSegList-insertItemBefore.xhtml
svg/dom/SVGPathSegList-clear-and-initialize.xhtml
svg/dom/SVGLengthList-replaceItem.xhtml
svg/dom/SVGLengthList-basics.xhtml
svg/dom/SVGNumberList-basics.xhtml
svg/dom/SVGLengthList-removeItem.xhtml
svg/dom/SVGLengthList-initialize.xhtml
svg/dom/SVGPathSegList-appendItem.xhtml
svg/dom/SVGPathSegList-replaceItem.xhtml
svg/dom/SVGTransformList-basics.xhtml
svg/animations/svglength-element-removed-crash.svg

[EWS Watchlist]

Created attachment 356889 [details]
Archive of layout-test-results from ews114 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Said Abou-Hallawa]

Created attachment 356892 [details]
EWSPatch

[EWS Watchlist]

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


ERROR: Source/WebCore/svg/properties/SVGAnimatedDecoratedProperty.h:33:  Failed to find complete declaration of class DecoratedProperty  [build/class] [5]
Total errors found: 1 in 292 files


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

[EWS Watchlist]

Comment on attachment 356892 [details]
EWSPatch

Attachment 356892 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/10321663

New failing tests:
svg/dom/SVGScriptElement/script-set-href.svg
svg/animations/svglength-element-removed-crash.svg

[EWS Watchlist]

Created attachment 356895 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[EWS Watchlist]

Comment on attachment 356892 [details]
EWSPatch

Attachment 356892 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10321653

New failing tests:
svg/dom/SVGScriptElement/script-set-href.svg

[EWS Watchlist]

Created attachment 356896 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Said Abou-Hallawa]

Created attachment 359721 [details]
EWSPatch

[Said Abou-Hallawa]

Created attachment 359735 [details]
EWSPatch

[EWS Watchlist]

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


ERROR: Source/WebCore/svg/properties/SVGAnimatedDecoratedProperty.h:33:  Failed to find complete declaration of class DecoratedProperty  [build/class] [5]
Total errors found: 1 in 300 files


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

[EWS Watchlist]

Comment on attachment 359735 [details]
EWSPatch

Attachment 359735 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10839846

New failing tests:
imported/w3c/web-platform-tests/webrtc/simplecall.https.html

[EWS Watchlist]

Created attachment 359747 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Said Abou-Hallawa]

Created attachment 363581 [details]
Rebased EWSPatch

[Said Abou-Hallawa]

*** Bug 168586 has been marked as a duplicate of this bug. ***

[EWS Watchlist]

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


ERROR: Source/WebCore/svg/SVGURIReference.cpp:45:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/svg/properties/SVGAnimatedDecoratedProperty.h:33:  Failed to find complete declaration of class DecoratedProperty  [build/class] [5]
Total errors found: 2 in 292 files


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

[EWS Watchlist]

Comment on attachment 363581 [details]
Rebased EWSPatch

Attachment 363581 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11371326

New failing tests:
svg/dom/SVGScriptElement/script-set-href.svg
svg/animations/svglength-element-removed-crash.svg

[EWS Watchlist]

Created attachment 363595 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit

[EWS Watchlist]

Comment on attachment 363581 [details]
Rebased EWSPatch

Attachment 363581 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11373978

New failing tests:
svg/dom/SVGScriptElement/script-set-href.svg

[EWS Watchlist]

Created attachment 363613 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Said Abou-Hallawa]

Created attachment 363690 [details]
Patch for review

[EWS Watchlist]

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


ERROR: Source/WebCore/svg/properties/SVGAnimatedDecoratedProperty.h:33:  Failed to find complete declaration of class DecoratedProperty  [build/class] [5]
Total errors found: 1 in 299 files


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

[Said Abou-Hallawa]

Created attachment 364172 [details]
Unified Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/svg/properties/SVGAnimatedDecoratedProperty.h:33:  Failed to find complete declaration of class DecoratedProperty  [build/class] [5]
Total errors found: 1 in 299 files


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

[EWS Watchlist]

Comment on attachment 364172 [details]
Unified Patch

Attachment 364172 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11445830

New failing tests:
accessibility/mac/selection-notification-focus-change.html

[EWS Watchlist]

Created attachment 364180 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Darin Adler]

Comment on attachment 364172 [details]
Unified Patch

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

Is there any way to make this change in a set of smaller steps? Perhaps a smaller patch that makes the behavior change, and then a bigger one that removes all the now-unnecessary code? I’m a bit overwhelmed by the total size here.

> LayoutTests/ChangeLog:15
> +        newItem from its old list before adding it to the new list. SVG2 states

"that the newItem from its old list" doesn’t make sense. Maybe "that newItem is deleted from its old list"?

> LayoutTests/ChangeLog:16
> +        that only a copy of the newItem will be inserted.

It’s not quite correct grammar to say "the newItem". I would suggest either "the new item" or "newItem" without the article.

> Source/WebCore/css/CSSCursorImageValue.cpp:108
> +    if (const auto* cursorElement = updateCursorElement(*loader.document())) {

This explicit use of const should not be needed. The auto* should match to the const type.

> Source/WebCore/rendering/svg/SVGPathData.cpp:46
> +    const auto* renderer = element.renderer();

Should not need const with auto here either. And in many other places. One of the nice things about "auto*" is that it works with both const and non-const pointers depending on what’s returned from the expression on the right.

[Darin Adler]

Really looking forward to this change, but I am concerned with my ability to review it. Neither the smaller patches nor the unified patch seem just right to me.

But someone else may find they can review.

[Said Abou-Hallawa]

(In reply to Darin Adler from comment #131)
> Really looking forward to this change, but I am concerned with my ability to
> review it. Neither the smaller patches nor the unified patch seem just right
> to me.
> 
> But someone else may find they can review.

I removed the SVG tear off objects for SVGAnimatedInteger in https://bugs.webkit.org/show_bug.cgi?id=195722. This the minimum of change I could get to move one property from the tear off world. Once the patch for SVGAnimatedInteger is landed, the other types will be converted similarly.

[Nikolas Zimmermann]

Dear Said,

glad that you are tackling this. I feel guilty about the tear-off objects, since I am responsible for the mess.


Have a nice day,
Niko

[Said Abou-Hallawa]

(In reply to Nikolas Zimmermann from comment #133)
> Dear Said,
> 
> glad that you are tackling this. I feel guilty about the tear-off objects,
> since I am responsible for the mess.
> 
> 
> Have a nice day,
> Niko

Hi Niko,

Actually it was an honor for me to work in the code that you wrote. I learnt a lot from it. And I understand a big part of it was written at the time the septs was being changed. I was lucky to find a working code and finalized specs.

I wish you will have some time to write some patches for SVG 2 and review my patches also.

Thanks for dropping this message.

Said

[Nikolas Zimmermann]

(In reply to Said Abou-Hallawa from comment #134)
> (In reply to Nikolas Zimmermann from comment #133)
> Actually it was an honor for me to work in the code that you wrote. I learnt
> a lot from it. And I understand a big part of it was written at the time the
> septs was being changed. I was lucky to find a working code and finalized
> specs.
Oh yes, the SVG DOM turned out to be difficult to implement, and some parts were really obscure (SVG DOM live list synchronization, with attributes comes to my mind, etc.). Glad that you are working on it.

> 
> I wish you will have some time to write some patches for SVG 2 and review my
> patches also.
I am no longer a reviewer - even if I am technically, I would not feel comfortable with reviewing after my long absence. I've followed each WebKit commit since I left in early 2013, but I am no longer actively working on it - busy with finishing my PhD at present. I hope to find time again for SVG once I am done with my journey in physics...

Have a nice day,
Niko

[Said Abou-Hallawa]

Created attachment 366512 [details]
Patch

[Said Abou-Hallawa]

Created attachment 366557 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 366557 [details]
Patch

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

> Source/WebCore/svg/SVGAnimateElementBase.cpp:97
> +    m_hasInvalidCSSAttributeType = WTF::nullopt;

= { }

> Source/WebCore/svg/SVGAnimateElementBase.cpp:154
> +void SVGAnimateElementBase::calculateAnimatedValue(float percentage, unsigned repeatCount, SVGSMILElement*)

"percentage" is the wrong term here, because it's 0-1, not 0-100. We usually call this "progress"

> Source/WebCore/svg/SVGAnimateElementBase.cpp:192
> +        return -1;

Better to return an Optional<float>

> Source/WebCore/svg/SVGElement.cpp:567
> +void SVGElement::synchronizeAllAnimatedSVGAttribute(SVGElement* svgElement)

Can we pass SVGElement&?

> Source/WebCore/svg/SVGViewElement.h:48
> +    // FIXME: svgAttributeChanged missing.

File a bug for this? It's not clear what the implications are.

[Said Abou-Hallawa]

Created attachment 366636 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 366557 [details]
Patch

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

>> Source/WebCore/svg/SVGAnimateElementBase.cpp:97
>> +    m_hasInvalidCSSAttributeType = WTF::nullopt;
> 
> = { }

Fixed.

>> Source/WebCore/svg/SVGAnimateElementBase.cpp:154
>> +void SVGAnimateElementBase::calculateAnimatedValue(float percentage, unsigned repeatCount, SVGSMILElement*)
> 
> "percentage" is the wrong term here, because it's 0-1, not 0-100. We usually call this "progress"

Done.

But I had to rename the 'progress()' methods of the SVGAttributeAnimator and the SVGAnimationFunction to 'animate()'. So I can rename their parameter 'percentage' to 'process'

>> Source/WebCore/svg/SVGAnimateElementBase.cpp:192
>> +        return -1;
> 
> Better to return an Optional<float>

Done.

But I had to change the base class and all the other driven classes as well. And I had also to change the return type of SVGAttributeAnimator::calculateDistance().

>> Source/WebCore/svg/SVGElement.cpp:567
>> +void SVGElement::synchronizeAllAnimatedSVGAttribute(SVGElement* svgElement)
> 
> Can we pass SVGElement&?

Done.

This function is only referenced from SelectorCodeGenerator::generateSynchronizeAllAnimatedSVGAttribute(). It looks like it gets its addresses but I do not know if it is really called or not.

>> Source/WebCore/svg/SVGViewElement.h:48
>> +    // FIXME: svgAttributeChanged missing.
> 
> File a bug for this? It's not clear what the implications are.

Filed https://bugs.webkit.org/show_bug.cgi?id=196554. And I changed the comment above to include the bug number.

[WebKit Commit Bot]

Comment on attachment 366636 [details]
Patch

Clearing flags on attachment: 366636

Committed r243830: <https://trac.webkit.org/changeset/243830>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Said Abou-Hallawa]

*** Bug 191372 has been marked as a duplicate of this bug. ***

[Said Abou-Hallawa]

*** Bug 135040 has been marked as a duplicate of this bug. ***

[Said Abou-Hallawa]

*** Bug 132557 has been marked as a duplicate of this bug. ***

[Said Abou-Hallawa]

*** Bug 91191 has been marked as a duplicate of this bug. ***

[Said Abou-Hallawa]

*** Bug 184670 has been marked as a duplicate of this bug. ***

[Said Abou-Hallawa]

*** Bug 125561 has been marked as a duplicate of this bug. ***