Bug 186751 – Remove the SVG elements' attributes macros

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

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

Created attachment 342916 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Said Abou-Hallawa]

Created attachment 342985 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Said Abou-Hallawa]

Created attachment 343114 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Said Abou-Hallawa]

Created attachment 343162 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Radar WebKit Bug Importer]

<rdar://problem/41333951>

[Said Abou-Hallawa]

Created attachment 343327 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Said Abou-Hallawa]

Created attachment 343979 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Said Abou-Hallawa]

Created attachment 344513 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Said Abou-Hallawa]

Created attachment 344534 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Said Abou-Hallawa]

Created attachment 344988 [details]
Patch

[Said Abou-Hallawa]

Created attachment 344990 [details]
Patch

[Dirk Schulze]

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.

[Said Abou-Hallawa]

(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.

[Ryosuke Niwa]

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.

[Dirk Schulze]

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.

[Said Abou-Hallawa]

Created attachment 346097 [details]
Introduce SVGAttribute

[Said Abou-Hallawa]

Created attachment 346103 [details]
Introduce SVGAttributeAccessor

[Simon Fraser (smfr)]

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.

[Said Abou-Hallawa]

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

[Said Abou-Hallawa]

Created attachment 346124 [details]
Introduce SVGAttributeRegistry

[Said Abou-Hallawa]

Created attachment 346129 [details]
Introduce SVGAttributeOwnerProxy

[Simon Fraser (smfr)]

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.

[Said Abou-Hallawa]

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)

[Said Abou-Hallawa]

Created attachment 346177 [details]
Add TearOff creation methods

[Said Abou-Hallawa]

Created attachment 346179 [details]
Delete SVG macros for SVGAnimatedLength

[Said Abou-Hallawa]

Created attachment 346180 [details]
Delete SVG macros for SVGAnimatedLengthList

[Said Abou-Hallawa]

Created attachment 346182 [details]
Delete SVG macros for SVGAnimatedBoolean

[Said Abou-Hallawa]

Created attachment 346185 [details]
Delete SVG macros for SVGAnimatedInteger

[Said Abou-Hallawa]

Created attachment 346188 [details]
Delete SVG macros for SVGAnimatedNumber

[Said Abou-Hallawa]

Created attachment 346190 [details]
Add registration methods for SVGZoomAndPan

[Said Abou-Hallawa]

Created attachment 346192 [details]
Delete SVG macros for SVGAnimatedAngle

[Said Abou-Hallawa]

Created attachment 346193 [details]
Delete SVG macros for SVGAnimatedEnumeration

[Said Abou-Hallawa]

Created attachment 346197 [details]
Add animated type and animated attribute for PathSegList

[Said Abou-Hallawa]

Created attachment 346199 [details]
Add a registration method for SVGAnimatedPointList

[Said Abou-Hallawa]

Created attachment 346201 [details]
Delete SVG macros for SVGAnimatedPreserveAspectRatio

[Said Abou-Hallawa]

Created attachment 346202 [details]
Delete SVG macros for SVGAnimatedRect

[Said Abou-Hallawa]

Created attachment 346203 [details]
Delete SVG macros for SVGAnimatedString

[Said Abou-Hallawa]

Created attachment 346204 [details]
Delete SVG macros for SVGAnimatedTransformList

[Said Abou-Hallawa]

Created attachment 346205 [details]
Delete SVG macros for SVGAnimatedNumberList

[Said Abou-Hallawa]

Created attachment 346206 [details]
Add a registration method for SVGStringListValues

[Said Abou-Hallawa]

Created attachment 346207 [details]
Move AnimatedPropertyType to a separate file.

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

Created attachment 346211 [details]
Remove the reference to SVGPropertyInfo.h from SVGAnimatedProperty.h

[Said Abou-Hallawa]

Created attachment 346212 [details]
Register SVGLangSpace attributes into SVGAttributeRegistry

[EWS Watchlist]

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.

[Said Abou-Hallawa]

Created attachment 346217 [details]
Remove SVG macros from SVGElement

[Said Abou-Hallawa]

Created attachment 346224 [details]
Register SVGTests attributes into SVGAttributeRegistry

[Said Abou-Hallawa]

Created attachment 346230 [details]
Remove SVG macros from SVGGraphicsElement

[Said Abou-Hallawa]

Created attachment 346233 [details]
Remove SVG macros from SVGGeometryElement

[Simon Fraser (smfr)]

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..."

[Jon Lee]

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.

[Simon Fraser (smfr)]

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

[Simon Fraser (smfr)]

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?

[Simon Fraser (smfr)]

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.

[Simon Fraser (smfr)]

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)

[Simon Fraser (smfr)]

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.

[Simon Fraser (smfr)]

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.

[Simon Fraser (smfr)]

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?

[Said Abou-Hallawa]

Created attachment 346241 [details]
Register SVGExternalResourcesRequired attributes into SVGAttributeRegistry

[Said Abou-Hallawa]

Created attachment 346244 [details]
Remove SVG macros from SVGRectElement

[Said Abou-Hallawa]

Created attachment 346245 [details]
Remove SVG macros from SVGCircleElement

[Simon Fraser (smfr)]

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?

[Simon Fraser (smfr)]

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).

[Said Abou-Hallawa]

Created attachment 346315 [details]
Remove SVG macros from SVGEllipseElement

[Said Abou-Hallawa]

Created attachment 346316 [details]
Remove SVG macros from SVGLineElement

[Said Abou-Hallawa]

Created attachment 346317 [details]
Remove SVG macros from SVGPolyElement

[Said Abou-Hallawa]

Created attachment 346318 [details]
Register SVGURIReference attributes into SVGAttributeRegistry

[Said Abou-Hallawa]

Created attachment 346319 [details]
Remove SVG macros from SVGAElement

[Said Abou-Hallawa]

Created attachment 346320 [details]
Remove SVG macros from SVGTextContentElement

[Said Abou-Hallawa]

Created attachment 346322 [details]
Remove SVG macros from SVGTextPositioningElement

[Said Abou-Hallawa]

Created attachment 346323 [details]
Remove SVG macros from SVGAltGlyphElement

[Said Abou-Hallawa]

Created attachment 346324 [details]
Remove SVG macros from SVGClipPathElement

[Said Abou-Hallawa]

Created attachment 346325 [details]
Remove SVG macros from SVGFilterPrimitiveStandardAttributes

[Said Abou-Hallawa]

Created attachment 346326 [details]
Remove SVG macros from SVGFEBlendElement

[Said Abou-Hallawa]

Created attachment 346327 [details]
Remove SVG macros from SVGFEColorMatrixElement

[Said Abou-Hallawa]

Created attachment 346328 [details]
Remove SVG macros from SVGFEComponentTransferElement

[Said Abou-Hallawa]

Created attachment 346329 [details]
Remove SVG macros from SVGFECompositeElement

[Said Abou-Hallawa]

Created attachment 346330 [details]
Remove SVG macros from SVGFEConvolveMatrixElement

[Said Abou-Hallawa]

Created attachment 346331 [details]
Remove SVG macros from SVGFEDiffuseLightingElement

[Said Abou-Hallawa]

Created attachment 346332 [details]
Remove SVG macros from SVGFEDisplacementMapElement

[Said Abou-Hallawa]

Created attachment 346333 [details]
Remove SVG macros from SVGFEDropShadowElement

[Said Abou-Hallawa]

Created attachment 346334 [details]
Remove SVG macros from SVGFEGaussianBlurElement

[Said Abou-Hallawa]

Created attachment 346335 [details]
Remove SVG macros from SVGFEImageElement

[Said Abou-Hallawa]

Created attachment 346336 [details]
Remove SVG macros from SVGFELightElement

[Said Abou-Hallawa]

Created attachment 346337 [details]
Remove SVG macros from SVGFEMergeNodeElement

[Said Abou-Hallawa]

Created attachment 346338 [details]
Remove SVG macros from SVGFEMorphologyElement

[Said Abou-Hallawa]

Created attachment 346339 [details]
Remove SVG macros from SVGFEOffsetElement

[Said Abou-Hallawa]

Created attachment 346340 [details]
Remove SVG macros from SVGFESpecularLightingElement

[Said Abou-Hallawa]

Created attachment 346341 [details]
Remove SVG macros from SVGFETileElement

[Said Abou-Hallawa]

Created attachment 346342 [details]
Remove SVG macros from SVGFETurbulenceElement

[Said Abou-Hallawa]

Created attachment 346343 [details]
Remove SVG macros from SVGCursorElement

[Said Abou-Hallawa]

Created attachment 346344 [details]
Remove SVG macros from SVGDefsElement

[Said Abou-Hallawa]

Created attachment 346345 [details]
Remove SVG macros from SVGGradientElement

[Said Abou-Hallawa]

Created attachment 346346 [details]
Remove SVG macros from SVGLinearGradientElement

[Said Abou-Hallawa]

Created attachment 346347 [details]
Remove SVG macros from SVGRadialGradientElement

[Said Abou-Hallawa]

Created attachment 346348 [details]
Remove SVG macros from SVGFontElement

[Said Abou-Hallawa]

Created attachment 346349 [details]
Remove SVG macros from SVGForeignObjectElement

[Said Abou-Hallawa]

Created attachment 346350 [details]
Remove SVG macros from SVGGElement

[EWS Watchlist]

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.

[Simon Fraser (smfr)]

I can't do it. rs=me for all the mechanical changes.

[Said Abou-Hallawa]

Created attachment 346518 [details]
Remove SVG macros from SVGFilterElement

[Said Abou-Hallawa]

Created attachment 346519 [details]
Remove SVG macros from SVGSwitchElement

[Said Abou-Hallawa]

Created attachment 346520 [details]
Register SVGFitToViewBox attributes into SVGAttributeRegistry

[Said Abou-Hallawa]

Created attachment 346521 [details]
Remove SVG macros from SVGSymbolElement

[Said Abou-Hallawa]

Created attachment 346522 [details]
Remove SVG macros from SVGComponentTransferFunctionElement

[Said Abou-Hallawa]

Created attachment 346524 [details]
Remove SVG macros from SVGPatternElement

[Said Abou-Hallawa]

Created attachment 346525 [details]
Remove SVG macros from SVGImageElement

[Said Abou-Hallawa]

Created attachment 346526 [details]
Remove SVG macros from SVGGlyphRefElement

[Said Abou-Hallawa]

Created attachment 346527 [details]
Remove SVG macros from SVGMarkerElement

[Said Abou-Hallawa]

Created attachment 346528 [details]
Remove SVG macros from SVGUseElement

[Said Abou-Hallawa]

Created attachment 346529 [details]
Register SVGZoomAndPan attributes into SVGAttributeRegistry

[Said Abou-Hallawa]

Created attachment 346530 [details]
Remove SVG macros from SVGViewElement

[Said Abou-Hallawa]

Created attachment 346531 [details]
Remove SVG macros from SVGTRefElement

[Said Abou-Hallawa]

Created attachment 346532 [details]
Remove SVG macros from SVGTextPathElement

[Said Abou-Hallawa]

Created attachment 346533 [details]
Remove SVG macros from SVGSVGElement

[Said Abou-Hallawa]

Created attachment 346534 [details]
Remove SVG macros from SVGScriptElement

[Said Abou-Hallawa]

Created attachment 346535 [details]
Remove SVG macros from SVGPathElement

[Said Abou-Hallawa]

Created attachment 346536 [details]
Remove SVG macros from SVGMPathElement

[Said Abou-Hallawa]

Created attachment 346537 [details]
Remove SVG macros from SVGMaskElement

[Said Abou-Hallawa]

Created attachment 346538 [details]
Remove SVG macros from SVGAnimationElement

[Said Abou-Hallawa]

Created attachment 346539 [details]
Register SVGViewSpec attributes into SVGAttributeRegistry

[Said Abou-Hallawa]

Created attachment 346540 [details]
Remove macro files, fix build and address some of the review comments

[Said Abou-Hallawa]

Created attachment 346541 [details]
Patch for EWS

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Said Abou-Hallawa]

Created attachment 346552 [details]
Patch for EWS

[Said Abou-Hallawa]

Created attachment 346601 [details]
Memory shrinking patch

[Said Abou-Hallawa]

Created attachment 346602 [details]
Patch for EWS

[Said Abou-Hallawa]

Created attachment 346611 [details]
Patch for EWS

[EWS Watchlist]

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.

[Simon Fraser (smfr)]

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

[Said Abou-Hallawa]

Created attachment 346636 [details]
Patch

[Said Abou-Hallawa]

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>.

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

Created attachment 346642 [details]
Patch

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

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.

[WebKit Commit Bot]

Comment on attachment 346642 [details]
Patch

Clearing flags on attachment: 346642

Committed r234620: <https://trac.webkit.org/changeset/234620>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.