[webkit-reviews] review granted: [Bug 94469] enable <animate> of viewBox attribute in <view> element : [Attachment 378490] Patch

[bugzilla-daemon at webkit.org]

Said Abou-Hallawa <sabouhallawa at apple.com> has granted Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 94469: enable <animate> of viewBox attribute in <view> element
https://bugs.webkit.org/show_bug.cgi?id=94469

Attachment 378490: Patch

https://bugs.webkit.org/attachment.cgi?id=378490&action=review




--- Comment #23 from Said Abou-Hallawa <sabouhallawa at apple.com> ---
Comment on attachment 378490
  --> https://bugs.webkit.org/attachment.cgi?id=378490
Patch

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

> Source/WebCore/svg/SVGSVGElement.cpp:671
> +		   ASSERT(rootElement->m_currentViewElement->targetElement());
> +		   ASSERT(rootElement->m_currentViewElement->targetElement() ==
rootElement);

I think the second assertion is sufficient since at this point we are sure
rootElement != nullptr.

> Source/WebCore/svg/SVGSVGElement.cpp:678
> +	       if (!rootElement->m_currentViewElement ||
rootElement->m_currentViewElement != viewElement)

The first clause is not needed. We know that viewElement != nullptr so if
rootElement->m_currentViewElement == nullptr, rootElement->m_currentViewElement
!= viewElement will be true.

> Source/WebCore/svg/SVGSVGElement.cpp:680
> +	      
rootElement->m_currentViewElement->setTargetElement(*rootElement);

I think this statement should be moved under the previous if-statement. If we
are not setting rootElement->m_currentViewElement to a new value, then the
target element of the rootElement->m_currentViewElement should be equal to
rootElement. In fact this what we assert a few lines above.

> Source/WebCore/svg/SVGSVGElement.h:164
> +    WeakPtr<SVGViewElement> m_currentViewElement { nullptr };

Why is this a WeakPtr? My understanding is SVGSVGElement should own its current
view so there should an ownership relation from SVGSVGElement to
SVGViewElement. Even in the new code, SVGSVGElement is the one that controls
the life cycle of both SVGSVGElement::m_currentViewElement and
SVGViewElement::m_targetElement.

SVGSVGElement::m_currentViewElement is changed here:

    rootElement->m_currentViewElement = makeWeakPtr(viewElement); 

SVGViewElement::m_targetElement is changed here:

    rootElement->m_currentViewElement->resetTargetElement();

and

    rootElement->m_currentViewElement->setTargetElement(*rootElement);

On contrary to this SVGViewElement does not change any of these pointers. So I
think this should be RefPtr while SVGViewElement::m_targetElement should stay
WeakPtr.