Each SVG element has to define its attributes via a set of macros. These macros, define the class member, the setter, the getter and the tear-off object creator. These set of macros have the following problems: -- There is no way to debug through these macros. -- The type of the the attributes' owner is not strongly typed. See the casting in SVGAnimatedPropertyMacros.h. -- There is no actual inheritance between the C++ classes. For example SVGRectElement and SVGCircle both inherit SVGExternalResourcesRequired but they both have to hold a member named m_externalResourcesRequired. -- Customizing the synchroniable attributes are awkward. See customizing m_orientType in SVGMarkerElement and m_textLength in SVGTextContentElement.
Created attachment 342916 [details] Patch
Comment on attachment 342916 [details] Patch Attachment 342916 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/8226505 New failing tests: svg/animations/path-animation.svg svg/animations/repeating-path-animation.svg imported/mozilla/svg/dynamic-pattern-02.svg
Created attachment 342917 [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.4
Created attachment 342985 [details] Patch
Comment on attachment 342985 [details] Patch Attachment 342985 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/8238532 New failing tests: svg/animations/path-animation.svg svg/animations/repeating-path-animation.svg imported/mozilla/svg/dynamic-pattern-02.svg
Created attachment 343001 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4
Created attachment 343114 [details] Patch
Comment on attachment 343114 [details] Patch Attachment 343114 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/8256900 New failing tests: svg/animations/repeating-path-animation.svg svg/animations/path-animation.svg svg/custom/marker-changes.svg imported/mozilla/svg/dynamic-pattern-02.svg
Created attachment 343123 [details] Archive of layout-test-results from ews106 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 343114 [details] Patch Attachment 343114 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/8256595 New failing tests: svg/animations/path-animation.svg svg/animations/repeating-path-animation.svg imported/mozilla/svg/dynamic-pattern-02.svg
Created attachment 343124 [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.4
Comment on attachment 343114 [details] Patch Attachment 343114 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/8257801 New failing tests: svg/custom/marker-changes.svg svg/animations/path-animation.svg svg/animations/repeating-path-animation.svg imported/mozilla/svg/dynamic-pattern-02.svg
Created attachment 343127 [details] Archive of layout-test-results from ews102 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 343114 [details] Patch Attachment 343114 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/8258917 New failing tests: svg/animations/repeating-path-animation.svg svg/animations/path-animation.svg svg/custom/marker-changes.svg imported/mozilla/svg/dynamic-pattern-02.svg
Created attachment 343129 [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
Created attachment 343162 [details] Patch
Comment on attachment 343162 [details] Patch Attachment 343162 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/8266780 New failing tests: svg/animations/repeating-path-animation.svg svg/animations/path-animation.svg svg/custom/marker-changes.svg imported/mozilla/svg/dynamic-pattern-02.svg
Created attachment 343167 [details] Archive of layout-test-results from ews102 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 343162 [details] Patch Attachment 343162 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/8266802 New failing tests: svg/custom/marker-changes.svg svg/animations/path-animation.svg svg/animations/repeating-path-animation.svg imported/mozilla/svg/dynamic-pattern-02.svg
Created attachment 343169 [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 on attachment 343162 [details] Patch Attachment 343162 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/8266853 New failing tests: svg/custom/marker-changes.svg svg/animations/path-animation.svg svg/animations/repeating-path-animation.svg imported/mozilla/svg/dynamic-pattern-02.svg
Created attachment 343171 [details] Archive of layout-test-results from ews117 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 343162 [details] Patch Attachment 343162 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/8266951 New failing tests: svg/animations/path-animation.svg svg/animations/repeating-path-animation.svg imported/mozilla/svg/dynamic-pattern-02.svg
Created attachment 343174 [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.4
Comment on attachment 343162 [details] Patch Attachment 343162 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8272884 New failing tests: svg/animations/repeating-path-animation.svg svg/animations/path-animation.svg svg/custom/marker-changes.svg imported/mozilla/svg/dynamic-pattern-02.svg
Created attachment 343213 [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 on attachment 343162 [details] Patch Attachment 343162 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8273902 New failing tests: svg/custom/marker-changes.svg svg/animations/path-animation.svg svg/animations/repeating-path-animation.svg imported/mozilla/svg/dynamic-pattern-02.svg
Created attachment 343217 [details] Archive of layout-test-results from ews204 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews204 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
<rdar://problem/41333951>
Created attachment 343327 [details] Patch
Comment on attachment 343327 [details] Patch Attachment 343327 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/8291275 New failing tests: imported/mozilla/svg/dynamic-pattern-02.svg
Created attachment 343335 [details] Archive of layout-test-results from ews106 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 343327 [details] Patch Attachment 343327 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/8291451 New failing tests: imported/mozilla/svg/dynamic-pattern-02.svg
Created attachment 343338 [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.4
Comment on attachment 343327 [details] Patch Attachment 343327 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/8292095 New failing tests: imported/mozilla/svg/dynamic-pattern-02.svg
Created attachment 343339 [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 on attachment 343327 [details] Patch Attachment 343327 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/8291335 New failing tests: inspector/model/remote-object-dom.html imported/mozilla/svg/dynamic-pattern-02.svg
Created attachment 343343 [details] Archive of layout-test-results from ews113 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 343327 [details] Patch Attachment 343327 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8299829 New failing tests: svg/dom/SVGViewSpec.html imported/mozilla/svg/dynamic-pattern-02.svg
Created attachment 343415 [details] Archive of layout-test-results from ews202 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews202 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 343979 [details] Patch
Comment on attachment 343979 [details] Patch Attachment 343979 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/8390649 New failing tests: inspector/model/remote-object-dom.html
Created attachment 343989 [details] Archive of layout-test-results from ews112 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 343979 [details] Patch Attachment 343979 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8392684 New failing tests: http/tests/security/local-video-source-from-remote.html
Created attachment 344002 [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
Created attachment 344513 [details] Patch
Comment on attachment 344513 [details] Patch Attachment 344513 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/8464300 New failing tests: inspector/model/remote-object-dom.html
Created attachment 344516 [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 on attachment 344513 [details] Patch Attachment 344513 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8466579 New failing tests: svg/dom/SVGViewSpec.html
Created attachment 344523 [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.9.0-0.318-5-3-x86_64-64bit
Created attachment 344534 [details] Patch
Comment on attachment 344534 [details] Patch Attachment 344534 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8470673 New failing tests: svg/dom/SVGViewSpec.html
Created attachment 344536 [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
Created attachment 344988 [details] Patch
Created attachment 344990 [details] Patch
Though I understand and share the concern, the macros made it easier to manage attributes in general. Especially with regards to deprecatiom of animated attributes and re-linking them to base values, the macros would make it easier to archive that. Again, just something to consider.
(In reply to Dirk Schulze from comment #56) > Though I understand and share the concern, the macros made it easier to > manage attributes in general. Especially with regards to deprecatiom of > animated attributes and re-linking them to base values, the macros would > make it easier to archive that. Again, just something to consider. Thanks for the advice. But here is why I consider macros a bad design for the SVG attributes: 1. I agree macros make significant change, like the deprecation of animated attributes, would be easier. But you can get stuck easily with very simple implementation detail and you have to do an ugly work around. An example of that is the inheritance of the SVG attributes. With the current implementation, the attributes of non SVGElement-based objects, like SVGURIReference, SVGExternalResourcesRequired and SVGFitToViewBox, have to be repeated in all the derived classes. Simialrly you have to repeat the same handling for changing these attributes in the svgAttributeChanged() method of all the derived classes. In fact, these base objects do not own any data. You see the ugliness of the macros implementation in functions like SVGFitToViewBox::parseAttribute(). 2. You can't debug in the macros easily. For example you can't step into or set a break point in any of the functions that the macros generate for each attribute. 3. SVG macros are hard to customize. Changing, even for experimenting, the behavior for a single attribute is challenging. You can see how it is difficult if you look at the customization of the 'orientType' attribute of the SVGMarkerElement and the 'textLength' attribute of the SVGTextContentElement. 4. The macros violates the object oriented encapsulation principal by adding the implementation of the attributes' methods to the owner class. For example, the SVGRectElement has the the following methods which are related to the 'x' attribute: SVGLength& x() const; SVGLength& xBaseValue() const; SVGLength& setXBaseValue() const; Ref<SVGAnimatedLength> xAnimated() const; bool xIsValid() const; void synchronizeX() const; static Ref<SVGAnimatedProperty> lookupOrCreateXWrapper(SVGElement); static void synchronizeX(SVGElement); By the way these functions are not searchable since they do not exist in the code. If you receive a crash trace in any of these functions you have to find the caller instead. 5. The design of the optional attribute or the MULTIPLE_WRAPPERS is a little bit confusing. It is hard to grasp how the two data members are linked to the same attribute. You can see the connection between them only when you realize that SVGAttributeToPropertyMap maps from QualifiedName to a Vector<const SVGPropertyInfo*>. Then you see that elements like SVGFEGaussianBlurElement adds the two members stdDeviationX and stdDeviationY to the SVGAttributeToPropertyMap under the same name: SVGName::stdDeviationAttr. This adds the two members into a single entry in SVGAttributeToPropertyMap. The confusion is: why we use a Vector for the value of the HasMap in SVGAttributeToPropertyMap although we only have an element or two at maximum in this Vector? 6. The SVG TearOff objects have been a source of many security crashes. I think the biggest part of this problem is we do not understand the state of the code clearly. The TearOff objects keep raw pointers to the properties data of the SVG attributes. But the TearOff objects use all kinds of pointers: raw, WeakPtr and RefPtr to ensure the owner SVGElement is alive when accessing the data. I believe that the difficulty of knowing the state of the code starts from these macros. And to fix these security issues and heave a clean implementation of the SVG attributes we have to get rid of these macros.
If there is a concern about the code duplication, maintenance, etc... we should be generating all that code as CPP files as we do elsewhere instead of using macros. These macros makes it impossible to debug SVG code.
I am not arguing for or against macros in general. The question is just the order of the work. If WebKit should deprecate and replace animated properties as specified by SVG 2, then this work might make sense to prioritize. If there aren’t plans like that then doing your work first is fine.
Created attachment 346097 [details] Introduce SVGAttribute
Created attachment 346103 [details] Introduce SVGAttributeAccessor
Comment on attachment 346103 [details] Introduce SVGAttributeAccessor View in context: https://bugs.webkit.org/attachment.cgi?id=346103&action=review > Source/WebCore/svg/properties/SVGAttributeAccessor.h:53 > + virtual ~SVGAttributeAccessor() = default; I prefer to see this after the constructor. > Source/WebCore/svg/properties/SVGAttributeAccessor.h:61 > + const QualifiedName& m_attributeName; sizeof(QualifiedName) == sizeof(void*) so maybe you should just store it by value.
*** Bug 187258 has been marked as a duplicate of this bug. ***
Created attachment 346124 [details] Introduce SVGAttributeRegistry
Created attachment 346129 [details] Introduce SVGAttributeOwnerProxy
Comment on attachment 346124 [details] Introduce SVGAttributeRegistry View in context: https://bugs.webkit.org/attachment.cgi?id=346124&action=review > Source/WebCore/ChangeLog:9 > + attributes of a given owner. It takes into account the inheritance of the By "owner" I think you mean a given attribute identified by a name/type pair? It's not that every instance of an attribute has a unique registry entry, but this is static data that is one per attribute class? > Source/WebCore/svg/properties/SVGAttributeRegistry.h:48 > + auto it = std::find_if(m_map.begin(), m_map.end(), [&attributeName](const auto& entry) -> bool { > + return entry.key.matches(attributeName); > + }); > + return it != m_map.end(); Why isn't this just m_map.contains()? Seems like you're doing manual hash key iteration here? > Source/WebCore/svg/properties/SVGAttributeRegistry.h:62 > + for (auto* attributeAccessor : m_map.values()) > + attributeAccessor->synchronizeProperty(owner, element); > + synchronizeAttributesBaseTypes(owner, element); The indentation is off here. > Source/WebCore/svg/properties/SVGAttributeRegistry.h:84 > + template<size_t I = 0> > + static typename std::enable_if<I == sizeof...(BaseTypes), Vector<AnimatedPropertyType>>::type animatedTypesBaseTypes(const QualifiedName&) { return { }; } > + > + template<size_t I = 0> > + static typename std::enable_if<I < sizeof...(BaseTypes), Vector<AnimatedPropertyType>>::type animatedTypesBaseTypes(const QualifiedName& attributeName) I think this template magic deserves some comments to explain how it works.
Comment on attachment 346124 [details] Introduce SVGAttributeRegistry View in context: https://bugs.webkit.org/attachment.cgi?id=346124&action=review >> Source/WebCore/ChangeLog:9 >> + attributes of a given owner. It takes into account the inheritance of the > > By "owner" I think you mean a given attribute identified by a name/type pair? It's not that every instance of an attribute has a unique registry entry, but this is static data that is one per attribute class? Yes this is correct. The attribute is registered only once with owner/element pair types. To access the value of an attribute you need to provide owner/element pair instance. The owner/element pair instance will be provided to the attribute when creating it as SVGAttributeOwnerProxy. >> Source/WebCore/svg/properties/SVGAttributeRegistry.h:48 >> + return it != m_map.end(); > > Why isn't this just m_map.contains()? Seems like you're doing manual hash key iteration here? No. We can't use m_map.contains. We have to compare the attribute names using QualifiedName::matches() since matches() takes the namespace into account while operator==() does not. Please have a look at: bool SVGURIReference::isKnownAttribute(const QualifiedName& attrName) bool SVGLangSpace::isKnownAttribute(const QualifiedName& attrName)
Created attachment 346177 [details] Add TearOff creation methods
Created attachment 346179 [details] Delete SVG macros for SVGAnimatedLength
Created attachment 346180 [details] Delete SVG macros for SVGAnimatedLengthList
Created attachment 346182 [details] Delete SVG macros for SVGAnimatedBoolean
Created attachment 346185 [details] Delete SVG macros for SVGAnimatedInteger
Created attachment 346188 [details] Delete SVG macros for SVGAnimatedNumber
Created attachment 346190 [details] Add registration methods for SVGZoomAndPan
Created attachment 346192 [details] Delete SVG macros for SVGAnimatedAngle
Created attachment 346193 [details] Delete SVG macros for SVGAnimatedEnumeration
Created attachment 346197 [details] Add animated type and animated attribute for PathSegList
Created attachment 346199 [details] Add a registration method for SVGAnimatedPointList
Created attachment 346201 [details] Delete SVG macros for SVGAnimatedPreserveAspectRatio
Created attachment 346202 [details] Delete SVG macros for SVGAnimatedRect
Created attachment 346203 [details] Delete SVG macros for SVGAnimatedString
Created attachment 346204 [details] Delete SVG macros for SVGAnimatedTransformList
Created attachment 346205 [details] Delete SVG macros for SVGAnimatedNumberList
Created attachment 346206 [details] Add a registration method for SVGStringListValues
Created attachment 346207 [details] Move AnimatedPropertyType to a separate file.
Comment on attachment 346207 [details] Move AnimatedPropertyType to a separate file. View in context: https://bugs.webkit.org/attachment.cgi?id=346207&action=review > Source/WebCore/ChangeLog:12 > + * svg/properties/:SVGAnimatedPropertyType.h: Added. The file name should be SVGAnimatedPropertyType.h.
Created attachment 346211 [details] Remove the reference to SVGPropertyInfo.h from SVGAnimatedProperty.h
Created attachment 346212 [details] Register SVGLangSpace attributes into SVGAttributeRegistry
Attachment 346212 [details] did not pass style-queue: ERROR: Source/WebCore/svg/SVGLangSpace.cpp:25: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebCore/svg/SVGLangSpace.cpp:26: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 346217 [details] Remove SVG macros from SVGElement
Created attachment 346224 [details] Register SVGTests attributes into SVGAttributeRegistry
Created attachment 346230 [details] Remove SVG macros from SVGGraphicsElement
Created attachment 346233 [details] Remove SVG macros from SVGGeometryElement
Comment on attachment 346177 [details] Add TearOff creation methods View in context: https://bugs.webkit.org/attachment.cgi?id=346177&action=review > Source/WebCore/svg/properties/SVGAttributeRegistry.h:103 > + // It is a template function with parameter 'I' whose default value = 0. So you can call it without any parameter from animatedTypes(). "This is..."
Comment on attachment 346124 [details] Introduce SVGAttributeRegistry View in context: https://bugs.webkit.org/attachment.cgi?id=346124&action=review >>> Source/WebCore/svg/properties/SVGAttributeRegistry.h:48 >>> + return it != m_map.end(); >> >> Why isn't this just m_map.contains()? Seems like you're doing manual hash key iteration here? > > No. We can't use m_map.contains. > > We have to compare the attribute names using QualifiedName::matches() since matches() takes the namespace into account while operator==() does not. Please have a look at: > > bool SVGURIReference::isKnownAttribute(const QualifiedName& attrName) > bool SVGLangSpace::isKnownAttribute(const QualifiedName& attrName) This might be worth adding a comment, then, if there is a risk some time later someone decides to "optimize" this code.
Comment on attachment 346190 [details] Add registration methods for SVGZoomAndPan View in context: https://bugs.webkit.org/attachment.cgi?id=346190&action=review > Source/WebCore/svg/SVGZoomAndPanType.h:33 > +enum SVGZoomAndPanType { Later this should be enum class : uint8_t
Comment on attachment 346193 [details] Delete SVG macros for SVGAnimatedEnumeration View in context: https://bugs.webkit.org/attachment.cgi?id=346193&action=review > Source/WebCore/svg/SVGAnimatedEnumeration.h:31 > +using SVGAnimatedEnumeration = SVGAnimatedStaticPropertyTearOff<unsigned>; Will this still cause problems with 8-bit enum classes? Do we eventually need to provide tear-off types for each unique enum class?
Comment on attachment 346197 [details] Add animated type and animated attribute for PathSegList View in context: https://bugs.webkit.org/attachment.cgi?id=346197&action=review > Source/WebCore/svg/properties/SVGAnimatedPathSegListPropertyTearOff.cpp:51 > + // This is an expensive operation and only done, if someone actually observes the animatedPathSegList() while an animation is running. "and only done if" (no comma) Makes me wonder why this isn't done everywhere.
Comment on attachment 346207 [details] Move AnimatedPropertyType to a separate file. View in context: https://bugs.webkit.org/attachment.cgi?id=346207&action=review > Source/WebCore/svg/properties/:SVGAnimatedPropertyType.h:30 > +enum AnimatedPropertyState { Enum class (eventually) > Source/WebCore/svg/properties/:SVGAnimatedPropertyType.h:35 > +enum AnimatedPropertyType { Enum class (eventually)
Comment on attachment 346211 [details] Remove the reference to SVGPropertyInfo.h from SVGAnimatedProperty.h View in context: https://bugs.webkit.org/attachment.cgi?id=346211&action=review > Source/WebCore/svg/properties/SVGAnimatedProperty.h:81 > + static NeverDestroyed<Cache> s_cache; We don't tend to use the s_ naming convention now.
Comment on attachment 346212 [details] Register SVGLangSpace attributes into SVGAttributeRegistry View in context: https://bugs.webkit.org/attachment.cgi?id=346212&action=review > Source/WebCore/svg/SVGLangSpace.cpp:71 > + if (auto* renderer = downcast<RenderSVGShape>(m_contextElement.renderer())) { Weird, why is RenderSVGShape stuff here? > Source/WebCore/svg/SVGLangSpace.h:45 > + static auto& attributeRegistry() { return AttributeOwnerProxy::attributeRegistry(); } I hate that auto. I've no idea what the return type is.
Comment on attachment 346217 [details] Remove SVG macros from SVGElement View in context: https://bugs.webkit.org/attachment.cgi?id=346217&action=review > Source/WebCore/svg/SVGElement.h:152 > + const auto& className() const { return m_className.currentValue(); } > + auto classNameAnimated() { return m_className.animatedProperty(); } Those autos are bad too. Are they Strings, AtomicStrings, StringViews, StringImpls?
Created attachment 346241 [details] Register SVGExternalResourcesRequired attributes into SVGAttributeRegistry
Created attachment 346244 [details] Remove SVG macros from SVGRectElement
Created attachment 346245 [details] Remove SVG macros from SVGCircleElement
Comment on attachment 346241 [details] Register SVGExternalResourcesRequired attributes into SVGAttributeRegistry View in context: https://bugs.webkit.org/attachment.cgi?id=346241&action=review > Source/WebCore/svg/SVGExternalResourcesRequired.cpp:30 > +SVGExternalResourcesRequired::SVGExternalResourcesRequired(SVGElement* contextElement) Can contextElement be a reference? > Source/WebCore/svg/SVGExternalResourcesRequired.cpp:69 > + if (renderer && is<RenderSVGShape>(renderer)) { Again more RenderSVGShape specialness. Why?
Comment on attachment 346244 [details] Remove SVG macros from SVGRectElement View in context: https://bugs.webkit.org/attachment.cgi?id=346244&action=review > Source/WebCore/svg/SVGRectElement.h:74 > + AttributeOwnerProxy m_attributeOwnerProxy { *this }; > + SVGAnimatedLengthAttribute m_x { m_attributeOwnerProxy, LengthModeWidth }; > + SVGAnimatedLengthAttribute m_y { m_attributeOwnerProxy, LengthModeHeight }; > + SVGAnimatedLengthAttribute m_width { m_attributeOwnerProxy, LengthModeWidth }; > + SVGAnimatedLengthAttribute m_height { m_attributeOwnerProxy, LengthModeHeight }; > + SVGAnimatedLengthAttribute m_rx { m_attributeOwnerProxy, LengthModeWidth }; > + SVGAnimatedLengthAttribute m_ry { m_attributeOwnerProxy, LengthModeHeight}; How does the size of SVGRectElement change (use dump-class-layout to tell).
Created attachment 346315 [details] Remove SVG macros from SVGEllipseElement
Created attachment 346316 [details] Remove SVG macros from SVGLineElement
Created attachment 346317 [details] Remove SVG macros from SVGPolyElement
Created attachment 346318 [details] Register SVGURIReference attributes into SVGAttributeRegistry
Created attachment 346319 [details] Remove SVG macros from SVGAElement
Created attachment 346320 [details] Remove SVG macros from SVGTextContentElement
Created attachment 346322 [details] Remove SVG macros from SVGTextPositioningElement
Created attachment 346323 [details] Remove SVG macros from SVGAltGlyphElement
Created attachment 346324 [details] Remove SVG macros from SVGClipPathElement
Created attachment 346325 [details] Remove SVG macros from SVGFilterPrimitiveStandardAttributes
Created attachment 346326 [details] Remove SVG macros from SVGFEBlendElement
Created attachment 346327 [details] Remove SVG macros from SVGFEColorMatrixElement
Created attachment 346328 [details] Remove SVG macros from SVGFEComponentTransferElement
Created attachment 346329 [details] Remove SVG macros from SVGFECompositeElement
Created attachment 346330 [details] Remove SVG macros from SVGFEConvolveMatrixElement
Created attachment 346331 [details] Remove SVG macros from SVGFEDiffuseLightingElement
Created attachment 346332 [details] Remove SVG macros from SVGFEDisplacementMapElement
Created attachment 346333 [details] Remove SVG macros from SVGFEDropShadowElement
Created attachment 346334 [details] Remove SVG macros from SVGFEGaussianBlurElement
Created attachment 346335 [details] Remove SVG macros from SVGFEImageElement
Created attachment 346336 [details] Remove SVG macros from SVGFELightElement
Created attachment 346337 [details] Remove SVG macros from SVGFEMergeNodeElement
Created attachment 346338 [details] Remove SVG macros from SVGFEMorphologyElement
Created attachment 346339 [details] Remove SVG macros from SVGFEOffsetElement
Created attachment 346340 [details] Remove SVG macros from SVGFESpecularLightingElement
Created attachment 346341 [details] Remove SVG macros from SVGFETileElement
Created attachment 346342 [details] Remove SVG macros from SVGFETurbulenceElement
Created attachment 346343 [details] Remove SVG macros from SVGCursorElement
Created attachment 346344 [details] Remove SVG macros from SVGDefsElement
Created attachment 346345 [details] Remove SVG macros from SVGGradientElement
Created attachment 346346 [details] Remove SVG macros from SVGLinearGradientElement
Created attachment 346347 [details] Remove SVG macros from SVGRadialGradientElement
Created attachment 346348 [details] Remove SVG macros from SVGFontElement
Created attachment 346349 [details] Remove SVG macros from SVGForeignObjectElement
Created attachment 346350 [details] Remove SVG macros from SVGGElement
Attachment 346336 [details] did not pass style-queue: ERROR: Source/WebCore/svg/SVGFELightElement.cpp:132: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
I can't do it. rs=me for all the mechanical changes.
Created attachment 346518 [details] Remove SVG macros from SVGFilterElement
Created attachment 346519 [details] Remove SVG macros from SVGSwitchElement
Created attachment 346520 [details] Register SVGFitToViewBox attributes into SVGAttributeRegistry
Created attachment 346521 [details] Remove SVG macros from SVGSymbolElement
Created attachment 346522 [details] Remove SVG macros from SVGComponentTransferFunctionElement
Created attachment 346524 [details] Remove SVG macros from SVGPatternElement
Created attachment 346525 [details] Remove SVG macros from SVGImageElement
Created attachment 346526 [details] Remove SVG macros from SVGGlyphRefElement
Created attachment 346527 [details] Remove SVG macros from SVGMarkerElement
Created attachment 346528 [details] Remove SVG macros from SVGUseElement
Created attachment 346529 [details] Register SVGZoomAndPan attributes into SVGAttributeRegistry
Created attachment 346530 [details] Remove SVG macros from SVGViewElement
Created attachment 346531 [details] Remove SVG macros from SVGTRefElement
Created attachment 346532 [details] Remove SVG macros from SVGTextPathElement
Created attachment 346533 [details] Remove SVG macros from SVGSVGElement
Created attachment 346534 [details] Remove SVG macros from SVGScriptElement
Created attachment 346535 [details] Remove SVG macros from SVGPathElement
Created attachment 346536 [details] Remove SVG macros from SVGMPathElement
Created attachment 346537 [details] Remove SVG macros from SVGMaskElement
Created attachment 346538 [details] Remove SVG macros from SVGAnimationElement
Created attachment 346539 [details] Register SVGViewSpec attributes into SVGAttributeRegistry
Created attachment 346540 [details] Remove macro files, fix build and address some of the review comments
Created attachment 346541 [details] Patch for EWS
Comment on attachment 346541 [details] Patch for EWS Attachment 346541 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/8754374 New failing tests: imported/mozilla/svg/dynamic-pattern-02.svg
Created attachment 346545 [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 on attachment 346541 [details] Patch for EWS Attachment 346541 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/8754470 New failing tests: imported/mozilla/svg/dynamic-pattern-02.svg
Created attachment 346546 [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 on attachment 346541 [details] Patch for EWS Attachment 346541 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/8754425 New failing tests: imported/mozilla/svg/dynamic-pattern-02.svg
Created attachment 346549 [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.4
Comment on attachment 346541 [details] Patch for EWS Attachment 346541 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/8754590 New failing tests: imported/mozilla/svg/dynamic-pattern-02.svg
Created attachment 346551 [details] Archive of layout-test-results from ews117 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 346552 [details] Patch for EWS
Created attachment 346601 [details] Memory shrinking patch
Created attachment 346602 [details] Patch for EWS
Created attachment 346611 [details] Patch for EWS
Attachment 346611 [details] did not pass style-queue: ERROR: Source/WebCore/svg/properties/SVGAttributeOwnerProxyImpl.h:47: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/WebCore/svg/properties/SVGAttributeOwnerProxyImpl.h:48: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] Total errors found: 2 in 170 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 346601 [details] Memory shrinking patch View in context: https://bugs.webkit.org/attachment.cgi?id=346601&action=review > Source/WebCore/ChangeLog:20 > + that use it from the SVGElemnet and its super classes. SVGElemnet
Created attachment 346636 [details] Patch
Comment on attachment 346177 [details] Add TearOff creation methods View in context: https://bugs.webkit.org/attachment.cgi?id=346177&action=review >> Source/WebCore/svg/properties/SVGAttributeRegistry.h:103 >> + // It is a template function with parameter 'I' whose default value = 0. So you can call it without any parameter from animatedTypes(). > > "This is..." Fixed. The current wording is: // This is a template function with parameter 'I' whose default value = 0. So you can call it without any parameter from animatedTypes(). // It returns Vector<AnimatedPropertyType> and is enable_if<I == sizeof...(BaseTypes)>. So it is mainly for breaking the recursion. If // it is called, this means no attribute was found in this registry for this QualifiedName. So return an empty Vector<AnimatedPropertyType>.
Comment on attachment 346124 [details] Introduce SVGAttributeRegistry View in context: https://bugs.webkit.org/attachment.cgi?id=346124&action=review >>>> Source/WebCore/svg/properties/SVGAttributeRegistry.h:48 >>>> + return it != m_map.end(); >>> >>> Why isn't this just m_map.contains()? Seems like you're doing manual hash key iteration here? >> >> No. We can't use m_map.contains. >> >> We have to compare the attribute names using QualifiedName::matches() since matches() takes the namespace into account while operator==() does not. Please have a look at: >> >> bool SVGURIReference::isKnownAttribute(const QualifiedName& attrName) >> bool SVGLangSpace::isKnownAttribute(const QualifiedName& attrName) > > This might be worth adding a comment, then, if there is a risk some time later someone decides to "optimize" this code. The reason I gave above is incorrect. The correct reason I added as a comment to this function: // Here we need to loop through the entries in the map and use matches() to compare them with attributeName. // m_map.contains() uses QualifiedName::operator==() which compares the impl pointers only while matches() // compares the contents if the impl pointers differ. >> Source/WebCore/svg/properties/SVGAttributeRegistry.h:62 >> + synchronizeAttributesBaseTypes(owner, element); > > The indentation is off here. Fixed. >> Source/WebCore/svg/properties/SVGAttributeRegistry.h:84 >> + static typename std::enable_if<I < sizeof...(BaseTypes), Vector<AnimatedPropertyType>>::type animatedTypesBaseTypes(const QualifiedName& attributeName) > > I think this template magic deserves some comments to explain how it works. Two comments are added to this function and the one before it. And since the rest of template functions are very similar I did not any more comments.
Comment on attachment 346197 [details] Add animated type and animated attribute for PathSegList View in context: https://bugs.webkit.org/attachment.cgi?id=346197&action=review >> Source/WebCore/svg/properties/SVGAnimatedPathSegListPropertyTearOff.cpp:51 >> + // This is an expensive operation and only done, if someone actually observes the animatedPathSegList() while an animation is running. > > "and only done if" (no comma) > Makes me wonder why this isn't done everywhere. Comma is removed. I am not sure understand your question regarding this optimization. My understanding is this class tries to avoid building the PathSegListValues from the byte stream unless it really needs to. But in either case, it calls the Base::animValDidChange(). The only place that overrides this function is this place.
Comment on attachment 346211 [details] Remove the reference to SVGPropertyInfo.h from SVGAnimatedProperty.h View in context: https://bugs.webkit.org/attachment.cgi?id=346211&action=review >> Source/WebCore/svg/properties/SVGAnimatedProperty.h:81 >> + static NeverDestroyed<Cache> s_cache; > > We don't tend to use the s_ naming convention now. s_ is removed.
Comment on attachment 346212 [details] Register SVGLangSpace attributes into SVGAttributeRegistry View in context: https://bugs.webkit.org/attachment.cgi?id=346212&action=review >> Source/WebCore/svg/SVGLangSpace.cpp:71 >> + if (auto* renderer = downcast<RenderSVGShape>(m_contextElement.renderer())) { > > Weird, why is RenderSVGShape stuff here? I do not know. Similar code was in SVGCircleElement::svgAttributeChanged(), SVGEllipseElement::svgAttributeChanged() and other classes to handle the changes in the SVGLangSpace attributes. I will try removing this code and writing more tests for changing the attributes. >> Source/WebCore/svg/SVGLangSpace.h:45 >> + static auto& attributeRegistry() { return AttributeOwnerProxy::attributeRegistry(); } > > I hate that auto. I've no idea what the return type is. All autos are removed. Explicit types are now used for the attributes and the registry.
Comment on attachment 346193 [details] Delete SVG macros for SVGAnimatedEnumeration View in context: https://bugs.webkit.org/attachment.cgi?id=346193&action=review >> Source/WebCore/svg/SVGAnimatedEnumeration.h:31 >> +using SVGAnimatedEnumeration = SVGAnimatedStaticPropertyTearOff<unsigned>; > > Will this still cause problems with 8-bit enum classes? Do we eventually need to provide tear-off types for each unique enum class? I do not think so. The definition of SVGAnimatedEnumeration should be like this: template<typename EnumType> using SVGAnimatedEnumeration = SVGAnimatedStaticPropertyTearOff<EnumType>; And we need to clean SVGAnimatedEnumerationPropertyTearOff or get rid of it if possible.
Created attachment 346642 [details] Patch
Comment on attachment 346241 [details] Register SVGExternalResourcesRequired attributes into SVGAttributeRegistry View in context: https://bugs.webkit.org/attachment.cgi?id=346241&action=review >> Source/WebCore/svg/SVGExternalResourcesRequired.cpp:30 >> +SVGExternalResourcesRequired::SVGExternalResourcesRequired(SVGElement* contextElement) > > Can contextElement be a reference? No it can't be a reference because this will conflict with the copy constructor since the context element is a super class of SVGExternalResourcesRequired.
Comment on attachment 346244 [details] Remove SVG macros from SVGRectElement View in context: https://bugs.webkit.org/attachment.cgi?id=346244&action=review >> Source/WebCore/svg/SVGRectElement.h:74 >> + SVGAnimatedLengthAttribute m_ry { m_attributeOwnerProxy, LengthModeHeight}; > > How does the size of SVGRectElement change (use dump-class-layout to tell). The size with this patch is 568. With the macros, the size is 400 bytes. The difference is the SVGRectElement and each of its base classes now hold a member of AttributeOwnerProxy which is 32 bytes. This can be removed if SVGElement has these functions as virtual functions and each SVGElement-base-class overrides them and provide a temporary SVGAttributeOwnerProxy or even better sending "this" to the SVGAttributeRegistry method. void synchronizeAttribute(const QualifiedName& name) { attributeOwnerProxy().synchronizeAttribute(name); } void synchronizeAttributes() { attributeOwnerProxy().synchronizeAttributes(); } Vector<AnimatedPropertyType> animatedTypes(const QualifiedName& attributeName) const { return attributeOwnerProxy().animatedTypes(attributeName); } RefPtr<SVGAnimatedProperty> lookupAnimatedProperty(const SVGAttribute& attribute) const { return attributeOwnerProxy().lookupAnimatedProperty(attribute); } RefPtr<SVGAnimatedProperty> lookupOrCreateAnimatedProperty(const SVGAttribute& attribute) { return attributeOwnerProxy().lookupOrCreateAnimatedProperty(attribute); } Vector<RefPtr<SVGAnimatedProperty>> lookupOrCreateAnimatedProperties(const QualifiedName& name) { return attributeOwnerProxy().lookupOrCreateAnimatedProperties(name); } I introduced the SVGAttributeOwnerProxy to remove the code duplication. If the code duplication is returned back, the size will be back to its original size.
Comment on attachment 346642 [details] Patch Clearing flags on attachment: 346642 Committed r234620: <https://trac.webkit.org/changeset/234620>
All reviewed patches have been landed. Closing bug.