341638 - Rationalise nsSVGTextFrame, nsSVGTextPathFrame and nsSVGTSpanFrame

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

Should be able to derive these from a common base class nsTextContainerFrame inheriting from nsDisplayContainerFrame and thereby remove nsISVGTextFrame and possibly nsISVGTextContainerFrame.

May attempt the following idea courtesy of Tor too...

> Something that's been bothering me lately about the text code is the
> concept of a fragment tree, which just seems to be copying the actual
> text content out of nsITextContent into the glyph frames.  This seems a
> waste, and perhaps could be eliminated and in the process simplifying
> the code somewhat.

Comment 1

[tor]

(In reply to comment #0)
> > Something that's been bothering me lately about the text code is the
> > concept of a fragment tree, which just seems to be copying the actual
> > text content out of nsITextContent into the glyph frames.  This seems a
> > waste, and perhaps could be eliminated and in the process simplifying
> > the code somewhat.

This should be tackled as a seperate bug if possible, to avoid trying to change too many things at once.

Comment 2

[Robert Longson [:longsonr]]

Attachment: patch

Removes nsISVGTextFrame and nsISVGTextContainerFrame.

Comment 3

[tor]

Could you put the new containers into nsSVGTextContainerFrame.{h,cpp}?

You added the mOverrideCTM and mPropogateTransform fields to nsSVGTextContainerFrame, which is inherited by nsSVGTextPathFrame and nsSVGTSpanFrame, neither of which need those.  They should only exist in nsSVGTextFrame.

Comment 4

[Robert Longson [:longsonr]]

Attachment: address review comments

Added nsSVGTextContainerFrame.{h,cpp}

> You added the mOverrideCTM and mPropogateTransform fields to
> nsSVGTextContainerFrame, which is inherited by nsSVGTextPathFrame and
> nsSVGTSpanFrame, neither of which need those.  They should only exist in
> nsSVGTextFrame.

nsSVGTSpanFrame and nsSVGTextPathFrame need the above fields for GetCanvasTM which I'd put in the nsSVGGlyphContainerFrame class that they both inherit from . They previously both used the GetCanvasTM method in nsSVGTSpanFrame and I just moved this unchanged. Was the implementation of GetCanvasTM in nsSVGTSpanFrame incorrect?

Currently I've done part of your suggestion. I've moved the variables from nsSVGTextContainerFrame down to nsSVGGlyphContainerFrame and nsSVGTextFrame which I think saves some memory as it packs better it also means all of the functions that use these variables are together and they can become private rather than protected.

If the implementation of GetCanvasTM is incorrect could you give me some pointers as to what it should look like? Alternatively I could raise a different bug to rewrite that as it is an unrelated issue.

I also added missing include guards for the new nsSVGTextFrame.h file.

Comment 5

[tor]

(In reply to comment #4)
> nsSVGTSpanFrame and nsSVGTextPathFrame need the above fields for GetCanvasTM
> which I'd put in the nsSVGGlyphContainerFrame class that they both inherit from
> . They previously both used the GetCanvasTM method in nsSVGTSpanFrame and I
> just moved this unchanged. Was the implementation of GetCanvasTM in
> nsSVGTSpanFrame incorrect?

Ugh, now that I look closer at the code, that GetCanvasTM is needed because content can have a filter set on just a tspan or textPath, not just the enclosing text element.

Comment 6

[tor]

Comment on attachment 226512 [details] [diff] [review]
address review comments

You've created a nsSVGGlyphContainerFrame that both tspan and textPath inherit from, but tspan doesn't add anything in the inheritence, so we now have one extra vtable versus the previous textPath inheriting from tspan situation.  Change nsCSSFrameConstructor to create a nsSVGGlyphContainerFrame in the tspan case or go back the old inheritence?

You can remove nsSVGTextContainerFrame::PaintSVG, as the inherited nsSVGDisplayContainerFrame is what we want - the svg effects (opacity/filter/etc...) apply to tspan and textPath as well.  That's actually a bug in the current code.

Comment 7

[tor]

Comment on attachment 226512 [details] [diff] [review]
address review comments

nsSVGGlyphContainerFrame has a number of methods using QueryInterface instead of CallQueryInterface.

Comment 8

[Robert Longson [:longsonr]]

Attachment: address additional review comments

Addressed issues in comment 6 and comment 7.

Reverted to the old inheritance i.e. textPath inherits from tspan. nsSVGGlyphContainerFrame class removed.

nsSVGTextContainerFrame::PaintSVG removed.

replaced QueryInterface by CallQueryInterface in nsTSpanFrame

Comment 9

[Robert Longson [:longsonr]]

Checked in.