[webkit-reviews] review denied: [Bug 134186] Make it clear to get m_svgExtensions using svgExtensions() : [Attachment 235879] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has denied Jeongeun Kim
<je_julie.kim at samsung.com>'s request for review:
Bug 134186: Make it clear to get m_svgExtensions using svgExtensions()
https://bugs.webkit.org/show_bug.cgi?id=134186

Attachment 235879: Patch
https://bugs.webkit.org/attachment.cgi?id=235879&action=review

------- Additional Comments from Daniel Bates <dbates at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=235879&action=review


> Source/WebCore/dom/Document.h:1088
> -    const SVGDocumentExtensions* svgExtensions();
> +    SVGDocumentExtensions* svgExtensions();
>      SVGDocumentExtensions* accessSVGExtensions();

What do you envision we do with accessSVGExtensions() given its similarity in
signature to to svgExtensions()? I mean, it's unclear which function should be
called to ensure we get a non-null SVGDocumentExtensions without looking at the
implementation of each function, looking at other call sites and reasoning
about their usage. One way to resolve this is to remove accessSVGExtensions()
entirely. Another way is to take an approach similar to
Node::ensureEventTargetData() and rename accessSVGExtensions() to
ensureSVGExtensions() and have it return a reference to SVGDocumentExtensions.
If we choose to take the latter approach then I suggest we rename
svgExtensions() to svgExtensionsIfExists() to better describe the
conditionality of its return value.

> Source/WebCore/svg/SVGDocumentExtensions.h:76
> -    const HashSet<SVGFontFaceElement*>& svgFontFaceElements() const { return
m_svgFontFaceElements; }
> +    const HashSet<SVGFontFaceElement*>& svgFontFaceElements() { return
m_svgFontFaceElements; }

How did you come to the decision to make this function non-const?