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().
Created attachment 233586 [details] Patch
This patch makes sure that the purpose of code is to get m_svgExtensions as changing accessSVGExtensions() to svgExtensions(). Please take a look.
Comment on attachment 233586 [details] Patch Clearing flags on attachment: 233586 Committed r170287: <http://trac.webkit.org/changeset/170287>
All reviewed patches have been landed. Closing bug.
(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>
Created attachment 235879 [details] Patch
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.
(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.
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?
(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?
(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.
(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,