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 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 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 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
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
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
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 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
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
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
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
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
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
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
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
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
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
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 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
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 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
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 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
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.
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.
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)
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.
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 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 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?
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?
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.
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
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
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
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
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 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.
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.
2018-06-17 19:14 PDT, Said Abou-Hallawa
2018-06-17 21:05 PDT, EWS Watchlist
2018-06-18 15:53 PDT, Said Abou-Hallawa
2018-06-18 18:26 PDT, EWS Watchlist
2018-06-19 15:39 PDT, Said Abou-Hallawa
2018-06-19 17:35 PDT, EWS Watchlist
2018-06-19 17:43 PDT, EWS Watchlist
2018-06-19 18:15 PDT, EWS Watchlist
2018-06-19 19:49 PDT, EWS Watchlist
2018-06-20 10:14 PDT, Said Abou-Hallawa
2018-06-20 11:26 PDT, EWS Watchlist
2018-06-20 11:33 PDT, EWS Watchlist
2018-06-20 11:55 PDT, EWS Watchlist
2018-06-20 12:06 PDT, EWS Watchlist
2018-06-20 21:47 PDT, EWS Watchlist
2018-06-20 23:33 PDT, EWS Watchlist
2018-06-22 08:36 PDT, Said Abou-Hallawa
2018-06-22 10:00 PDT, EWS Watchlist
2018-06-22 10:32 PDT, EWS Watchlist
2018-06-22 10:33 PDT, EWS Watchlist
2018-06-22 10:49 PDT, EWS Watchlist
2018-06-22 20:05 PDT, EWS Watchlist
2018-06-29 17:15 PDT, Said Abou-Hallawa
2018-06-29 19:13 PDT, EWS Watchlist
2018-06-29 23:39 PDT, EWS Watchlist
2018-07-06 21:51 PDT, Said Abou-Hallawa
2018-07-06 23:32 PDT, EWS Watchlist
2018-07-07 07:41 PDT, EWS Watchlist
2018-07-07 18:17 PDT, Said Abou-Hallawa
2018-07-07 20:55 PDT, EWS Watchlist
2018-07-13 15:03 PDT, Said Abou-Hallawa
2018-07-13 15:16 PDT, Said Abou-Hallawa
2018-07-30 13:50 PDT, Said Abou-Hallawa
2018-07-30 14:42 PDT, Said Abou-Hallawa
2018-07-30 16:44 PDT, Said Abou-Hallawa
2018-07-30 17:26 PDT, Said Abou-Hallawa
2018-07-31 10:41 PDT, Said Abou-Hallawa
2018-07-31 10:56 PDT, Said Abou-Hallawa
2018-07-31 11:04 PDT, Said Abou-Hallawa
2018-07-31 11:28 PDT, Said Abou-Hallawa
2018-07-31 11:59 PDT, Said Abou-Hallawa
2018-07-31 12:12 PDT, Said Abou-Hallawa
2018-07-31 12:34 PDT, Said Abou-Hallawa
2018-07-31 12:47 PDT, Said Abou-Hallawa
2018-07-31 12:55 PDT, Said Abou-Hallawa
2018-07-31 13:25 PDT, Said Abou-Hallawa
2018-07-31 13:42 PDT, Said Abou-Hallawa
2018-07-31 14:32 PDT, Said Abou-Hallawa
2018-07-31 14:33 PDT, Said Abou-Hallawa
2018-07-31 14:34 PDT, Said Abou-Hallawa
2018-07-31 14:34 PDT, Said Abou-Hallawa
2018-07-31 14:35 PDT, Said Abou-Hallawa
2018-07-31 14:36 PDT, Said Abou-Hallawa
2018-07-31 14:45 PDT, Said Abou-Hallawa
2018-07-31 15:30 PDT, Said Abou-Hallawa
2018-07-31 15:57 PDT, Said Abou-Hallawa
2018-07-31 16:24 PDT, Said Abou-Hallawa
2018-07-31 16:49 PDT, Said Abou-Hallawa
2018-07-31 17:06 PDT, Said Abou-Hallawa
2018-07-31 17:18 PDT, Said Abou-Hallawa
2018-07-31 18:07 PDT, Said Abou-Hallawa
2018-07-31 18:22 PDT, Said Abou-Hallawa
2018-07-31 18:29 PDT, Said Abou-Hallawa
2018-08-01 17:40 PDT, Said Abou-Hallawa
2018-08-01 17:40 PDT, Said Abou-Hallawa
2018-08-01 17:41 PDT, Said Abou-Hallawa
2018-08-01 17:42 PDT, Said Abou-Hallawa
2018-08-01 17:43 PDT, Said Abou-Hallawa
2018-08-01 17:43 PDT, Said Abou-Hallawa
2018-08-01 17:44 PDT, Said Abou-Hallawa
2018-08-01 17:45 PDT, Said Abou-Hallawa
2018-08-01 17:45 PDT, Said Abou-Hallawa
2018-08-01 17:46 PDT, Said Abou-Hallawa
2018-08-01 17:46 PDT, Said Abou-Hallawa
2018-08-01 17:47 PDT, Said Abou-Hallawa
2018-08-01 17:47 PDT, Said Abou-Hallawa
2018-08-01 17:48 PDT, Said Abou-Hallawa
2018-08-01 17:48 PDT, Said Abou-Hallawa
2018-08-01 17:49 PDT, Said Abou-Hallawa
2018-08-01 17:51 PDT, Said Abou-Hallawa
2018-08-01 17:52 PDT, Said Abou-Hallawa
2018-08-01 17:53 PDT, Said Abou-Hallawa
2018-08-01 17:53 PDT, Said Abou-Hallawa
2018-08-01 17:54 PDT, Said Abou-Hallawa
2018-08-01 17:54 PDT, Said Abou-Hallawa
2018-08-01 17:55 PDT, Said Abou-Hallawa
2018-08-01 17:56 PDT, Said Abou-Hallawa
2018-08-01 17:56 PDT, Said Abou-Hallawa
2018-08-01 17:57 PDT, Said Abou-Hallawa
2018-08-01 17:58 PDT, Said Abou-Hallawa
2018-08-01 17:58 PDT, Said Abou-Hallawa
2018-08-01 17:59 PDT, Said Abou-Hallawa
2018-08-01 17:59 PDT, Said Abou-Hallawa
2018-08-01 18:00 PDT, Said Abou-Hallawa
2018-08-01 18:01 PDT, Said Abou-Hallawa
2018-08-01 18:01 PDT, Said Abou-Hallawa
2018-08-01 18:02 PDT, Said Abou-Hallawa
2018-08-01 18:02 PDT, Said Abou-Hallawa
2018-08-03 12:36 PDT, Said Abou-Hallawa
2018-08-03 12:36 PDT, Said Abou-Hallawa
2018-08-03 12:37 PDT, Said Abou-Hallawa
2018-08-03 12:38 PDT, Said Abou-Hallawa
2018-08-03 12:38 PDT, Said Abou-Hallawa
2018-08-03 12:39 PDT, Said Abou-Hallawa
2018-08-03 12:40 PDT, Said Abou-Hallawa
2018-08-03 12:40 PDT, Said Abou-Hallawa
2018-08-03 12:41 PDT, Said Abou-Hallawa
2018-08-03 12:41 PDT, Said Abou-Hallawa
2018-08-03 12:42 PDT, Said Abou-Hallawa
2018-08-03 12:42 PDT, Said Abou-Hallawa
2018-08-03 12:43 PDT, Said Abou-Hallawa
2018-08-03 12:44 PDT, Said Abou-Hallawa
2018-08-03 12:44 PDT, Said Abou-Hallawa
2018-08-03 12:45 PDT, Said Abou-Hallawa
2018-08-03 12:45 PDT, Said Abou-Hallawa
2018-08-03 12:46 PDT, Said Abou-Hallawa
2018-08-03 12:46 PDT, Said Abou-Hallawa
2018-08-03 12:47 PDT, Said Abou-Hallawa
2018-08-03 12:47 PDT, Said Abou-Hallawa
2018-08-03 12:48 PDT, Said Abou-Hallawa
2018-08-03 12:53 PDT, Said Abou-Hallawa
2018-08-03 13:49 PDT, EWS Watchlist
2018-08-03 14:02 PDT, EWS Watchlist
2018-08-03 14:29 PDT, EWS Watchlist
2018-08-03 14:52 PDT, EWS Watchlist
2018-08-03 15:20 PDT, Said Abou-Hallawa
2018-08-05 10:28 PDT, Said Abou-Hallawa
2018-08-05 10:41 PDT, Said Abou-Hallawa
2018-08-05 21:31 PDT, Said Abou-Hallawa
2018-08-06 11:09 PDT, Said Abou-Hallawa
2018-08-06 12:42 PDT, Said Abou-Hallawa