Bug 201782 – Replace SVGProperty/SVGPropertyOwner by DOMLiveProperty/DOMLivePropertyObserver

• Status: RESOLVED
• Resolution: INVALID
• Component: SVG

[Said Abou-Hallawa]

This will move some of the functionalities in SVGProperty to DOMLiveObject. It also replaces SVGPropertyOwner by a new class named DOMLiveObjectObserver.

[Said Abou-Hallawa]

Created attachment 378763 [details]
Patch

[Said Abou-Hallawa]

Created attachment 378765 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 378765 [details]
Patch

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

This is an excellent patch. I wanted to point you towards the discussion of re-introducing SVGRect, SVGPoint etc. in SVG2, until I realized that you (shallawa) were involved in the discussion on https://github.com/w3c/svgwg/issues/706.

After reading the whole patch, the DOMLiveObjectObserver <-> DOMLiveObject relation is clear to me, and also why you are using a naked pointer to store the observer.

> Source/WebCore/dom/DOMLiveObject.h:76
> +    DOMLiveObjectObserver* m_observer { nullptr };

You used a naked pointer on purpose, I guess?

> Source/WebCore/dom/DOMLiveObjectObserver.h:40
> +    // Traverse the observers chain tillan Element observer is reached.

typo: tillan -> until an

> Source/WebCore/dom/Element.h:286
> +    const Element* contextElement() const override { return this; }

This is too generic for my taste, and it is not clear for what the context is used.
How about naming this "domLiveObjectContextElement" / "contextElementForDOMLiveObject" ?

> Source/WebCore/svg/properties/SVGAnimatedProperty.cpp:33
> +DOMLiveObjectObserver* SVGAnimatedProperty::observer() const

Any special reason why observer() / contextElement() / commitChange is not inlined?

[Said Abou-Hallawa]

Comment on attachment 378765 [details]
Patch

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

>> Source/WebCore/dom/DOMLiveObject.h:76
>> +    DOMLiveObjectObserver* m_observer { nullptr };
> 
> You used a naked pointer on purpose, I guess?

No really. But because the connection between object and the observer is controlled by the observer, I thought it is okay to use a raw pointer. This pointer is not going to be nullified unless the observer does it. And I made sure all the observers do it at the right time.

But I think you are right. This should be changed to be WeakPtr since it will be safer even if the code changes such that detach() is not called and the observer got deleted. I will make that change.

>> Source/WebCore/dom/DOMLiveObjectObserver.h:40
>> +    // Traverse the observers chain tillan Element observer is reached.
> 
> typo: tillan -> until an

Will Fix.

>> Source/WebCore/dom/Element.h:286
>> +    const Element* contextElement() const override { return this; }
> 
> This is too generic for my taste, and it is not clear for what the context is used.
> How about naming this "domLiveObjectContextElement" / "contextElementForDOMLiveObject" ?

Will change.

>> Source/WebCore/svg/properties/SVGAnimatedProperty.cpp:33
>> +DOMLiveObjectObserver* SVGAnimatedProperty::observer() const
> 
> Any special reason why observer() / contextElement() / commitChange is not inlined?

The casting.

In the header file, SVGElement is just forward declared. If I make these functions inline, the compiler will complain that it does not know how to cast SVGElement* to DOMLiveObjectObserver*. The complier does not know in this context that SVGElement is DOMLiveObjectObserver. Adding SVGElement.h to SVGAnimatedProperty.h is not an option because SVGElement.h has to include SVGAnimatedProperty.h.

I will add a comment here about that.

[Said Abou-Hallawa]

Created attachment 379990 [details]
Patch

[Said Abou-Hallawa]

Created attachment 379995 [details]
Patch

[Said Abou-Hallawa]

Created attachment 380019 [details]
Patch for review

[Sam Weinig]

Comment on attachment 380019 [details]
Patch for review

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

> Source/WebCore/dom/DOMLiveProperty.h:74
> +    virtual String valueAsString() const { return emptyString(); }

Given this is virtual, can this be moved out-of-line? Then you could avoid including WTFString.h

> Source/WebCore/dom/DOMLivePropertyObserver.h:38
> +    // The observer itself may have an observer.
> +    virtual DOMLivePropertyObserver* observer() const { return nullptr; }

When does it make sense not to override observer()?

> Source/WebCore/dom/DOMLivePropertyObserver.h:54
> +    virtual void commitPropertyChange(DOMLiveProperty* property)

Is property ever null? Can it be made into a reference?

[Said Abou-Hallawa]

Replacing SVGRect/SVGPoint/SVGMatrix by DOM equivalents was reverted in the specs: https://github.com/w3c/svgwg/issues/706. So this bug is now invalid and the attached patch should not be considered.