Bug 191237

Summary: Remove SVG properties tear-off objects
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: SVGAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: anvaka, cdumez, commit-queue, darin, dino, ews-watchlist, heycam, jlewis3, jonlee, krit, pdr, rniwa, ryanhaddad, simon.fraser, thorton, tsavell, webkit-bug-importer, yannick.poirier, zimmermann
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=197954
Bug Depends on: 195722, 195859, 195862, 195863, 195905, 195949, 195960, 196025, 196037, 196065, 196083, 196084, 196085, 196086, 196087, 196263, 245844    
Bug Blocks: 191292, 150388    
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from ews103 for mac-sierra
none
Archive of layout-test-results from ews107 for mac-sierra-wk2
none
Archive of layout-test-results from ews116 for mac-sierra
none
Archive of layout-test-results from ews125 for ios-simulator-wk2
none
Archive of layout-test-results from ews115 for mac-sierra
none
Archive of layout-test-results from ews123 for ios-simulator-wk2
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews126 for ios-simulator-wk2
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews125 for ios-simulator-wk2
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews121 for ios-simulator-wk2
none
Patch
none
Archive of layout-test-results from ews206 for win-future
none
Archive of layout-test-results from ews121 for ios-simulator-wk2
none
Patch
none
Patch
none
Archive of layout-test-results from ews126 for ios-simulator-wk2
none
Patch
none
Patch
none
Patch
ews-watchlist: commit-queue-
Archive of layout-test-results from ews200 for win-future
none
1-SVGProperty
none
2-SVGPathSeg
none
3-SVGList
none
Archive of layout-test-results from ews122 for ios-simulator-wk2
none
4-SVGDecoratedEnumeration
none
5-SVGPathSegList
none
6-SVGAnimatedProperty
none
7-SVGAnimationFunction
none
8-SVGAnimator.patch
none
9-SVGAccessor
none
10-SVGPropertyRegistry
none
11-JSBinding
none
12-FilesRemoval
none
13-BasicShapes
none
14-Resources
none
15-Filters
none
16-Text
none
17-BaseTypes
none
18-Conatiners
none
19-SVGAnimateElement
none
20-Miscellaneous
none
21-Extra
none
22-LayoutTests
ews-watchlist: commit-queue-
EWSPatch
none
Archive of layout-test-results from ews100 for mac-sierra
none
Archive of layout-test-results from ews105 for mac-sierra-wk2
none
Archive of layout-test-results from ews122 for ios-simulator-wk2
none
Archive of layout-test-results from ews203 for win-future
none
Archive of layout-test-results from ews114 for mac-sierra
none
EWSPatch
ews-watchlist: commit-queue-
Archive of layout-test-results from ews200 for win-future
none
Archive of layout-test-results from ews125 for ios-simulator-wk2
none
EWSPatch
none
EWSPatch
ews-watchlist: commit-queue-
Archive of layout-test-results from ews126 for ios-simulator-wk2
none
Rebased EWSPatch
ews-watchlist: commit-queue-
Archive of layout-test-results from ews201 for win-future
none
Archive of layout-test-results from ews126 for ios-simulator-wk2
none
Patch for review
none
Unified Patch
none
Archive of layout-test-results from ews106 for mac-highsierra-wk2
none
Patch
none
Patch
none
Patch none

Description Said Abou-Hallawa 2018-11-04 14:37:58 PST
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.
Comment 1 Said Abou-Hallawa 2018-11-04 15:02:45 PST
Created attachment 353808 [details]
Patch
Comment 2 EWS Watchlist 2018-11-04 15:07:50 PST
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.
Comment 3 Said Abou-Hallawa 2018-11-04 16:41:58 PST
Created attachment 353812 [details]
Patch
Comment 4 EWS Watchlist 2018-11-04 16:46:23 PST
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.
Comment 5 EWS Watchlist 2018-11-04 17:53:58 PST
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
Comment 6 EWS Watchlist 2018-11-04 17:54:00 PST
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
Comment 7 EWS Watchlist 2018-11-04 18:04:59 PST
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
Comment 8 EWS Watchlist 2018-11-04 18:05:01 PST
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
Comment 9 EWS Watchlist 2018-11-04 18:48:14 PST
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
Comment 10 EWS Watchlist 2018-11-04 18:48:16 PST
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
Comment 11 EWS Watchlist 2018-11-04 18:59:41 PST
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
Comment 12 EWS Watchlist 2018-11-04 18:59:43 PST
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
Comment 13 EWS Watchlist 2018-11-04 20:50:59 PST
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
Comment 14 EWS Watchlist 2018-11-04 20:51:01 PST
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
Comment 15 EWS Watchlist 2018-11-04 20:54:17 PST
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
Comment 16 EWS Watchlist 2018-11-04 20:54:19 PST
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
Comment 17 Radar WebKit Bug Importer 2018-11-07 13:45:13 PST
<rdar://problem/45887675>
Comment 18 Said Abou-Hallawa 2018-11-07 13:49:16 PST
*** Bug 191047 has been marked as a duplicate of this bug. ***
Comment 19 Said Abou-Hallawa 2018-11-07 13:49:52 PST
*** Bug 173178 has been marked as a duplicate of this bug. ***
Comment 20 Said Abou-Hallawa 2018-11-20 21:13:49 PST
Created attachment 355382 [details]
Patch
Comment 21 EWS Watchlist 2018-11-20 21:17:37 PST
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.
Comment 22 Said Abou-Hallawa 2018-11-20 22:07:13 PST
Created attachment 355386 [details]
Patch
Comment 23 EWS Watchlist 2018-11-20 22:11:06 PST
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.
Comment 24 Said Abou-Hallawa 2018-11-20 22:47:07 PST
Created attachment 355388 [details]
Patch
Comment 25 EWS Watchlist 2018-11-20 22:50:55 PST
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.
Comment 26 EWS Watchlist 2018-11-21 00:58:01 PST
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
Comment 27 EWS Watchlist 2018-11-21 00:58:03 PST
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
Comment 28 Dirk Schulze 2018-11-21 01:18:32 PST
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.
Comment 29 Said Abou-Hallawa 2018-11-24 18:25:03 PST
(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.
Comment 30 Said Abou-Hallawa 2018-11-24 18:31:43 PST
Created attachment 355568 [details]
Patch
Comment 31 Said Abou-Hallawa 2018-11-24 20:27:25 PST
Created attachment 355574 [details]
Patch
Comment 32 EWS Watchlist 2018-11-24 20:32:29 PST
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.
Comment 33 Said Abou-Hallawa 2018-11-24 20:47:26 PST
Created attachment 355575 [details]
Patch
Comment 34 EWS Watchlist 2018-11-24 20:52:09 PST
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.
Comment 35 Said Abou-Hallawa 2018-11-24 21:03:44 PST
Created attachment 355577 [details]
Patch
Comment 36 EWS Watchlist 2018-11-24 21:08:07 PST
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.
Comment 37 Said Abou-Hallawa 2018-11-24 21:19:19 PST
Created attachment 355580 [details]
Patch
Comment 38 EWS Watchlist 2018-11-24 21:22:49 PST
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.
Comment 39 Said Abou-Hallawa 2018-11-24 21:50:26 PST
Created attachment 355582 [details]
Patch
Comment 40 EWS Watchlist 2018-11-24 21:54:15 PST
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.
Comment 41 EWS Watchlist 2018-11-25 00:07:40 PST
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
Comment 42 EWS Watchlist 2018-11-25 00:07:42 PST
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
Comment 43 Said Abou-Hallawa 2018-11-26 12:17:16 PST
Created attachment 355666 [details]
Patch
Comment 44 EWS Watchlist 2018-11-26 12:21:06 PST
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.
Comment 45 Said Abou-Hallawa 2018-11-26 13:55:04 PST
Created attachment 355672 [details]
Patch
Comment 46 EWS Watchlist 2018-11-26 13:59:42 PST
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.
Comment 47 Said Abou-Hallawa 2018-11-26 16:34:25 PST
Created attachment 355693 [details]
Patch
Comment 48 Said Abou-Hallawa 2018-11-26 18:06:06 PST
Created attachment 355699 [details]
Patch
Comment 49 EWS Watchlist 2018-11-26 18:12:05 PST
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.
Comment 50 EWS Watchlist 2018-11-26 21:31:24 PST
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
Comment 51 EWS Watchlist 2018-11-26 21:31:26 PST
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
Comment 52 Said Abou-Hallawa 2018-11-27 11:42:24 PST
Created attachment 355752 [details]
Patch
Comment 53 EWS Watchlist 2018-11-27 11:47:52 PST
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.
Comment 54 EWS Watchlist 2018-11-27 14:05:04 PST
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
Comment 55 EWS Watchlist 2018-11-27 14:05:18 PST
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
Comment 56 EWS Watchlist 2018-11-27 16:46:40 PST
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
Comment 57 EWS Watchlist 2018-11-27 16:46:42 PST
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
Comment 58 Said Abou-Hallawa 2018-12-02 02:18:33 PST
Created attachment 356333 [details]
Patch
Comment 59 Said Abou-Hallawa 2018-12-02 03:40:48 PST
Created attachment 356334 [details]
Patch
Comment 60 EWS Watchlist 2018-12-02 03:45:27 PST
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.
Comment 61 EWS Watchlist 2018-12-02 08:07:22 PST
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
Comment 62 EWS Watchlist 2018-12-02 08:07:24 PST
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
Comment 63 Said Abou-Hallawa 2018-12-02 09:42:04 PST
Created attachment 356340 [details]
Patch
Comment 64 EWS Watchlist 2018-12-02 09:47:50 PST
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.
Comment 65 Said Abou-Hallawa 2018-12-02 11:11:45 PST
Created attachment 356342 [details]
Patch
Comment 66 EWS Watchlist 2018-12-02 11:16:01 PST
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.
Comment 67 Said Abou-Hallawa 2018-12-02 11:36:24 PST
Created attachment 356344 [details]
Patch
Comment 68 EWS Watchlist 2018-12-02 11:40:50 PST
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.
Comment 69 EWS Watchlist 2018-12-02 13:29:08 PST
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
Comment 70 EWS Watchlist 2018-12-02 13:29:19 PST
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
Comment 71 Said Abou-Hallawa 2018-12-02 17:19:24 PST
Created attachment 356351 [details]
1-SVGProperty
Comment 72 Said Abou-Hallawa 2018-12-02 18:24:03 PST
Created attachment 356352 [details]
2-SVGPathSeg
Comment 73 Said Abou-Hallawa 2018-12-02 23:31:36 PST
Created attachment 356358 [details]
3-SVGList
Comment 74 EWS Watchlist 2018-12-02 23:59:54 PST
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
Comment 75 EWS Watchlist 2018-12-02 23:59:57 PST
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
Comment 76 Said Abou-Hallawa 2018-12-03 08:00:49 PST
Created attachment 356380 [details]
4-SVGDecoratedEnumeration
Comment 77 Said Abou-Hallawa 2018-12-04 07:56:12 PST
Created attachment 356497 [details]
5-SVGPathSegList
Comment 78 Said Abou-Hallawa 2018-12-04 08:59:32 PST
Created attachment 356500 [details]
6-SVGAnimatedProperty
Comment 79 Said Abou-Hallawa 2018-12-04 09:45:02 PST
Created attachment 356507 [details]
7-SVGAnimationFunction
Comment 80 Said Abou-Hallawa 2018-12-04 21:09:27 PST
Created attachment 356579 [details]
8-SVGAnimator.patch
Comment 81 Said Abou-Hallawa 2018-12-05 08:28:54 PST
Created attachment 356607 [details]
9-SVGAccessor
Comment 82 Said Abou-Hallawa 2018-12-06 08:01:50 PST
Created attachment 356728 [details]
10-SVGPropertyRegistry
Comment 83 Said Abou-Hallawa 2018-12-06 08:02:18 PST
Created attachment 356729 [details]
11-JSBinding
Comment 84 Said Abou-Hallawa 2018-12-06 08:02:55 PST
Created attachment 356730 [details]
12-FilesRemoval
Comment 85 Said Abou-Hallawa 2018-12-06 08:03:22 PST
Created attachment 356731 [details]
13-BasicShapes
Comment 86 Said Abou-Hallawa 2018-12-06 08:03:48 PST
Created attachment 356732 [details]
14-Resources
Comment 87 Said Abou-Hallawa 2018-12-06 08:04:20 PST
Created attachment 356733 [details]
15-Filters
Comment 88 Said Abou-Hallawa 2018-12-06 14:04:11 PST
Created attachment 356752 [details]
16-Text
Comment 89 Said Abou-Hallawa 2018-12-07 07:47:21 PST
Created attachment 356810 [details]
17-BaseTypes
Comment 90 Said Abou-Hallawa 2018-12-07 08:13:34 PST
Created attachment 356811 [details]
18-Conatiners
Comment 91 Said Abou-Hallawa 2018-12-08 11:33:05 PST
Created attachment 356879 [details]
19-SVGAnimateElement
Comment 92 Said Abou-Hallawa 2018-12-08 11:33:33 PST
Created attachment 356880 [details]
20-Miscellaneous
Comment 93 Said Abou-Hallawa 2018-12-08 11:34:14 PST
Created attachment 356881 [details]
21-Extra
Comment 94 Said Abou-Hallawa 2018-12-08 11:34:41 PST
Created attachment 356882 [details]
22-LayoutTests
Comment 95 Said Abou-Hallawa 2018-12-08 11:38:37 PST
Created attachment 356883 [details]
EWSPatch
Comment 96 EWS Watchlist 2018-12-08 12:43:56 PST
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
Comment 97 EWS Watchlist 2018-12-08 12:43:59 PST
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
Comment 98 EWS Watchlist 2018-12-08 12:49:24 PST
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
Comment 99 EWS Watchlist 2018-12-08 12:49:27 PST
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
Comment 100 EWS Watchlist 2018-12-08 13:28:16 PST
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
Comment 101 EWS Watchlist 2018-12-08 13:28:19 PST
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
Comment 102 EWS Watchlist 2018-12-08 13:30:22 PST
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
Comment 103 EWS Watchlist 2018-12-08 13:30:36 PST
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
Comment 104 EWS Watchlist 2018-12-08 13:32:45 PST
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
Comment 105 EWS Watchlist 2018-12-08 13:32:48 PST
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
Comment 106 Said Abou-Hallawa 2018-12-08 16:24:38 PST
Created attachment 356892 [details]
EWSPatch
Comment 107 EWS Watchlist 2018-12-08 16:28:38 PST
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.
Comment 108 EWS Watchlist 2018-12-08 18:17:43 PST
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
Comment 109 EWS Watchlist 2018-12-08 18:17:56 PST
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
Comment 110 EWS Watchlist 2018-12-08 18:26:50 PST
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
Comment 111 EWS Watchlist 2018-12-08 18:26:53 PST
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
Comment 112 Said Abou-Hallawa 2019-01-22 00:24:51 PST
Created attachment 359721 [details]
EWSPatch
Comment 113 Said Abou-Hallawa 2019-01-22 07:59:13 PST
Created attachment 359735 [details]
EWSPatch
Comment 114 EWS Watchlist 2019-01-22 08:12:29 PST
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.
Comment 115 EWS Watchlist 2019-01-22 10:16:47 PST
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
Comment 116 EWS Watchlist 2019-01-22 10:16:50 PST
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
Comment 117 Said Abou-Hallawa 2019-03-04 18:42:02 PST
Created attachment 363581 [details]
Rebased EWSPatch
Comment 118 Said Abou-Hallawa 2019-03-04 18:46:59 PST
*** Bug 168586 has been marked as a duplicate of this bug. ***
Comment 119 EWS Watchlist 2019-03-04 18:49:52 PST
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.
Comment 120 EWS Watchlist 2019-03-04 20:50:22 PST
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
Comment 121 EWS Watchlist 2019-03-04 20:50:36 PST
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
Comment 122 EWS Watchlist 2019-03-05 00:36:49 PST
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
Comment 123 EWS Watchlist 2019-03-05 00:36:52 PST
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
Comment 124 Said Abou-Hallawa 2019-03-05 14:33:20 PST
Created attachment 363690 [details]
Patch for review
Comment 125 EWS Watchlist 2019-03-05 14:38:38 PST
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.
Comment 126 Said Abou-Hallawa 2019-03-09 23:26:15 PST
Created attachment 364172 [details]
Unified Patch
Comment 127 EWS Watchlist 2019-03-09 23:30:59 PST
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.
Comment 128 EWS Watchlist 2019-03-10 01:26:20 PST
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
Comment 129 EWS Watchlist 2019-03-10 01:26:23 PST
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
Comment 130 Darin Adler 2019-03-10 13:07:18 PDT
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.
Comment 131 Darin Adler 2019-03-10 13:08:37 PDT
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.
Comment 132 Said Abou-Hallawa 2019-03-13 18:48:21 PDT
(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.
Comment 133 Nikolas Zimmermann 2019-03-20 12:57:16 PDT
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
Comment 134 Said Abou-Hallawa 2019-03-20 15:05:31 PDT
(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
Comment 135 Nikolas Zimmermann 2019-03-24 02:05:28 PDT
(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
Comment 136 Said Abou-Hallawa 2019-04-02 11:36:09 PDT
Created attachment 366512 [details]
Patch
Comment 137 Said Abou-Hallawa 2019-04-02 17:18:11 PDT
Created attachment 366557 [details]
Patch
Comment 138 Simon Fraser (smfr) 2019-04-03 11:45:04 PDT
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.
Comment 139 Said Abou-Hallawa 2019-04-03 13:43:10 PDT
Created attachment 366636 [details]
Patch
Comment 140 Said Abou-Hallawa 2019-04-03 13:52:44 PDT
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.
Comment 141 WebKit Commit Bot 2019-04-03 15:08:06 PDT
Comment on attachment 366636 [details]
Patch

Clearing flags on attachment: 366636

Committed r243830: <https://trac.webkit.org/changeset/243830>
Comment 142 WebKit Commit Bot 2019-04-03 15:08:10 PDT
All reviewed patches have been landed.  Closing bug.
Comment 143 Said Abou-Hallawa 2019-04-13 02:26:16 PDT
*** Bug 191372 has been marked as a duplicate of this bug. ***
Comment 144 Said Abou-Hallawa 2019-05-06 12:40:15 PDT
*** Bug 135040 has been marked as a duplicate of this bug. ***
Comment 145 Said Abou-Hallawa 2019-05-06 14:57:33 PDT
*** Bug 132557 has been marked as a duplicate of this bug. ***
Comment 146 Said Abou-Hallawa 2019-05-06 15:20:26 PDT
*** Bug 91191 has been marked as a duplicate of this bug. ***
Comment 147 Said Abou-Hallawa 2019-05-06 15:28:26 PDT
*** Bug 184670 has been marked as a duplicate of this bug. ***
Comment 148 Said Abou-Hallawa 2023-05-10 10:39:37 PDT
*** Bug 125561 has been marked as a duplicate of this bug. ***