[webkit-reviews] review granted: [Bug 176577] SVG scrolling anchor should be reset if the fragmentIdentifier does not exist or is not provided : [Attachment 321113] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 176577: SVG scrolling anchor should be reset if the fragmentIdentifier does
not exist or is not provided
https://bugs.webkit.org/show_bug.cgi?id=176577

Attachment 321113: Patch

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




--- Comment #15 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 321113
  --> https://bugs.webkit.org/attachment.cgi?id=321113
Patch

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

> Source/WebCore/page/FrameView.cpp:2517
> +    document.updateStyleIfNeeded();

This is a new call. Is it necessary?

> Source/WebCore/svg/SVGSVGElement.cpp:495
> +	   if (svgRootParent.m_scrollRootElement == this)
> +	       svgRootParent.m_scrollRootElement = nullptr;

Seems a bit weird to access m_scrollRootElement directly. Can this be hidden
inside a member function?

> Source/WebCore/svg/SVGSVGElement.cpp:663
> +	       m_scrollRootElement =
&downcast<SVGSVGElement>(*viewportElement);

Pretty sure that you can downcast pointers too: m_scrollRootElement =
downcast<SVGSVGElement>(viewportElement);

> Source/WebCore/svg/SVGSVGElement.h:164
> +    SVGSVGElement* m_scrollRootElement { nullptr };

This goes against our desire to not have raw pointers. Please talk to rniwa
about the best way to do something like this.