Bug 201782

Created attachment 378763 [details] Patch

Created attachment 378765 [details] Patch

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?

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.

Created attachment 379990 [details] Patch

Created attachment 379995 [details] Patch

Created attachment 380019 [details] Patch for review

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?

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.