Bug 134186 - Make it clear to get m_svgExtensions using svgExtensions()
Summary: Make it clear to get m_svgExtensions using svgExtensions()
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-06-22 23:27 PDT by Jeongeun Kim
Modified: 2014-08-23 07:33 PDT (History)
11 users (show)

See Also:


Attachments
Patch (7.78 KB, patch)
2014-06-22 23:38 PDT, Jeongeun Kim
no flags Details | Formatted Diff | Diff
Patch (5.98 KB, patch)
2014-08-01 00:24 PDT, Jeongeun Kim
dbates: review-
dbates: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeongeun Kim 2014-06-22 23:27:16 PDT
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().
Comment 1 Jeongeun Kim 2014-06-22 23:38:39 PDT
Created attachment 233586 [details]
Patch
Comment 2 Jeongeun Kim 2014-06-23 01:14:00 PDT
This patch makes sure that the purpose of code is to get m_svgExtensions
as changing accessSVGExtensions() to svgExtensions().

Please take a look.
Comment 3 WebKit Commit Bot 2014-06-23 04:11:44 PDT
Comment on attachment 233586 [details]
Patch

Clearing flags on attachment: 233586

Committed r170287: <http://trac.webkit.org/changeset/170287>
Comment 4 WebKit Commit Bot 2014-06-23 04:11:50 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 Daniel Bates 2014-06-26 22:36:40 PDT
(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>
Comment 6 Jeongeun Kim 2014-08-01 00:24:59 PDT
Created attachment 235879 [details]
Patch
Comment 7 Jeongeun Kim 2014-08-01 01:31:35 PDT
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.
Comment 8 Jeongeun Kim 2014-08-21 19:16:04 PDT
(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 9 Daniel Bates 2014-08-21 23:22:06 PDT
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?
Comment 10 Jeongeun Kim 2014-08-22 03:10:17 PDT
(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?
Comment 11 Daniel Bates 2014-08-22 23:18:56 PDT
(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.
Comment 12 Jeongeun Kim 2014-08-23 07:33:33 PDT
(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,