Closed Bug 495296 Opened 11 years ago Closed 11 years ago

kill or deCOM nsISVGTextContentMetrics


(Core :: SVG, defect)

Not set





(Reporter: Swatinem, Assigned: Swatinem)





(1 file, 1 obsolete file)

When looking for "was hidden by" warnings, I found these:
/home/swatinem/Coding/mozilla-central/layout/svg/base/src/nsSVGTextContainerFrame.h:72: warning: 'virtual nsresult nsSVGTextContainerFrame::GetNumberOfChars(PRInt32*)' was hidden
/home/swatinem/Coding/mozilla-central/layout/svg/base/src/nsSVGTSpanFrame.h:88: warning:   by 'virtual PRUint32 nsSVGTSpanFrame::GetNumberOfChars()'
/home/swatinem/Coding/mozilla-central/layout/svg/base/src/nsSVGTextContainerFrame.h:73: warning: 'virtual nsresult nsSVGTextContainerFrame::GetComputedTextLength(float*)' was hidden
/home/swatinem/Coding/mozilla-central/layout/svg/base/src/nsSVGTSpanFrame.h:89: warning:   by 'virtual float nsSVGTSpanFrame::GetComputedTextLength()'
/home/swatinem/Coding/mozilla-central/layout/svg/base/src/nsSVGTextContainerFrame.h:74: warning: 'virtual nsresult nsSVGTextContainerFrame::GetSubStringLength(PRUint32, PRUint32, float*)' was hidden
/home/swatinem/Coding/mozilla-central/layout/svg/base/src/nsSVGTSpanFrame.h:90: warning:   by 'virtual float nsSVGTSpanFrame::GetSubStringLength(PRUint32, PRUint32)'
/home/swatinem/Coding/mozilla-central/layout/svg/base/src/nsSVGTextContainerFrame.h:79: warning: 'virtual nsresult nsSVGTextContainerFrame::GetCharNumAtPosition(nsIDOMSVGPoint*, PRInt32*)' was hidden
/home/swatinem/Coding/mozilla-central/layout/svg/base/src/nsSVGTSpanFrame.h:91: warning:   by 'virtual PRInt32 nsSVGTSpanFrame::GetCharNumAtPosition(nsIDOMSVGPoint*)'

nsISVGTextContentMetrics uses COM-style outparameters, while other classes implement the same methods with non-COM signatures, leading to confusions and the above warning.

nsISVGTextContentMetrics should be killed or at least updated to have COM-less methods. I will work up a patch as soon as I have time.
Attached patch WIP (obsolete) — Splinter Review
This turned out to be more complicated then I thought.

There is only one Class implementing nsISVGTextContentMetrics, which is nsSVGTextContainerFrame. Some of nsISVGTextContentMetrics methods collide with those of nsISVGGlyphFragmentNode. nsSVGTSpanFrame implements the FragmentNode interface, forwarding the methods to TextContainerFrame.

I got rid of nsISVGTextContentMetrics, using nsSVGTextContainerFrame instead and synching the method signatured with nsISVGGlyphFragmentNode.

Apart from that I created a base class which implements nsIDOMSVGTextContentElement to get rid of copy pasta on the content side.

The patch still has some issues, failing a few reftests. I haven't run mochitests yet. I will look into the failures soon.
Assignee: nobody → arpad.borsos
The first patch missed some QueryFrame stuff. Passes reftest now. The failures I get on tryserver with this patch seem to be random/unrelated.
Attachment #381595 - Attachment is obsolete: true
Attachment #382125 - Flags: superreview?(roc)
Attachment #382125 - Flags: review?(longsonr)
Attachment #382125 - Flags: superreview?(roc)
Attachment #382125 - Flags: superreview+
Attachment #382125 - Flags: review?(longsonr)
Attachment #382125 - Flags: review+
Comment on attachment 382125 [details] [diff] [review]
patch [pushed: comment 5]

Robert doesn't have to review this unless he wants to
I would have called the new file nsSVGTextContentElement rather than nsSVGTextContentElementBase as none of the other SVG base classes end in Base. (nsSVGElement, nsSVGStyleableElement etc). 

(In reply to comment #3)
> Robert doesn't have to review this unless he wants to

Phew. ;-)
Comment on attachment 382125 [details] [diff] [review]
patch [pushed: comment 5]

I did remove the "Base" from the class/file name, as suggested by comment 4.
Attachment #382125 - Attachment description: patch → patch [pushed: comment 5]
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 536912
You need to log in before you can comment on or make changes to this bug.