Bug 134186 – Make it clear to get m_svgExtensions using svgExtensions()

• Component: New Bugs
• Status: REOPENED

[Jeongeun Kim]

There are two APIs, svgExtensions() and accessSVGExtensions() to access m_svgExtensions. 
svgExtensions() is a getter API and accessSVGExtensions() checks the presence or absence of m_svgExtensions and create m_svgExtensions if it does not exist. 
After checking presence of 'm_svgExtensions', we don't need to use accessSVGExtensions().

[Jeongeun Kim]

Created attachment 233586 [details]
Patch

[Jeongeun Kim]

This patch makes sure that the purpose of code is to get m_svgExtensions
as changing accessSVGExtensions() to svgExtensions().

Please take a look.

[WebKit Commit Bot]

Comment on attachment 233586 [details]
Patch

Clearing flags on attachment: 233586

Committed r170287: <http://trac.webkit.org/changeset/170287>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Daniel Bates]

(In reply to comment #3)
> (From update of attachment 233586 [details])
> Clearing flags on attachment: 233586
> 
> Committed r170287: <http://trac.webkit.org/changeset/170287>

This regressed the re-association of elements to their target element in SVGDocumentExtensions::rebuildElements(). See bug #134304 for more details.

Rolled out <http://trac.webkit.org/changeset/170287> in <http://trac.webkit.org/changeset/170519>

[Jeongeun Kim]

Created attachment 235879 [details]
Patch

[Jeongeun Kim]

This patch makes svgExtensions() return mutual pointer, not const pointer and uses it instead of accessSVGExtensions() where svgExyensions() pointer checking is already done.
Daniel, Please take a look.

[Jeongeun Kim]

(In reply to comment #7)
> This patch makes svgExtensions() return mutual pointer, not const pointer and uses it instead of accessSVGExtensions() where svgExyensions() pointer checking is already done.
> Daniel, Please take a look.
He seems to be busy.
If anyone of reviewers is available, review please.

[Daniel Bates]

Comment on attachment 235879 [details]
Patch

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?

[Jeongeun Kim]

(In reply to comment #9)
> (From update of attachment 235879 [details])
> 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.
It's a great idea. I'll try the latter approach. Thank you for you suggestion.

> 
> > 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?
Actually, that is one of my worries and the part that I'd like to hear your opinion. 
I changed it to non-const and it's according to change of  'svgExtensions()' but I think it's not good.
Do I need to keep 'const SVGDocumentExtensions* svgExtensions()' for this part?

[Daniel Bates]

(In reply to comment #10)
> > > 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?
> Actually, that is one of my worries and the part that I'd like to hear your opinion. 
> I changed it to non-const and it's according to change of  'svgExtensions()' but I think it's not good.
> Do I need to keep 'const SVGDocumentExtensions* svgExtensions()' for this part?

I don't know at the moment. I will look through the code shortly. Can you save me some time and elaborate on the callers of svgFontFaceElements()? Regardless, it doesn't seem correct to make svgFontFaceElements() non-const given that it doesn't modify state.

[Jeongeun Kim]

(In reply to comment #11)
> (In reply to comment #10)
> > > > 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?
> > Actually, that is one of my worries and the part that I'd like to hear your opinion. 
> > I changed it to non-const and it's according to change of  'svgExtensions()' but I think it's not good.
> > Do I need to keep 'const SVGDocumentExtensions* svgExtensions()' for this part?
> 
> I don't know at the moment. I will look through the code shortly. Can you save me some time and elaborate on the callers of svgFontFaceElements()? Regardless, it doesn't seem correct to make svgFontFaceElements() non-const given that it doesn't modify state.

When StyleResolver is created, svg font face rules are added to fontSelector using svgFontFaceElements through iterating svgFontFaceElements(). That code is located at StyleResolver::StyleResolver(Document& document, bool matchAuthorAndUserStyles) in StyleResolver.cpp.
At that time, it's used only for collecting data without any modification. 

The field, m_svgFontFaceElements, which is gotten from svgFontFaceElements(),
is collected when SVGFontFaceElement is inserted at SVGFontFaceElement::insertedInto.

Thanks,