Comment 4 for bug 573365

(From update of attachment 272746)
One general comment.

I think it would be better all round if you tried to adapt the existing character placement mechanism used for textPath elements rather than having two separate implementations. i.e. use nsSVGCharacterPosition and make GetCharacterPosition return character positions if you have multiple y values. You may need to consider extending this so that you can have multiple characters with a single position for the first one so that you can write a string abcde with positions "x y" as character a at position x and then bcde in one go at position y.

>Index: layout/svg/base/src/nsISVGGlyphFragmentLeaf.h
>===================================================================
...
>- NS_IMETHOD_(void) SetGlyphPosition(float x, float y)=0;
>+ NS_IMETHOD_(void) SetGlyphPosition(float x, float y, PRUint16 baseline)=0;
>+ NS_IMETHOD_(void) GetNextGlyphPosition(float *x, float *y)=0;

If you change an interface you must update its ID i.e.
// {2C466AED-CF7B-4479-A807-98151215A645}
#define NS_ISVGGLYPHFRAGMENTLEAF_IID \
{ 0x2c466aed, 0xcf7b, 0x4479, { 0xa8, 0x7, 0x98, 0x15, 0x12, 0x15, 0xa6, 0x45 } }

> nsSVGGlyphFrame::nsSVGGlyphFrame(nsStyleContext* aContext)
> : nsSVGGlyphFrameBase(aContext),
> mWhitespaceHandling(COMPRESS_WHITESPACE)
> {
>+ mDoCharPlacement = PR_FALSE;
>+ mStrLength = 0;

Initialise member variables in the initialiser bit rather than the constructor body c.f. mWhitespaceHandling above. However see below.

>Index: layout/svg/base/src/nsSVGGlyphFrame.h
>===================================================================
>RCS file: /cvsroot/mozilla/layout/svg/base/src/nsSVGGlyphFrame.h,v
>retrieving revision 1.25
>diff -u -8 -p -r1.25 nsSVGGlyphFrame.h
>--- layout/svg/base/src/nsSVGGlyphFrame.h 4 Jul 2007 03:39:03 -0000 1.25
>+++ layout/svg/base/src/nsSVGGlyphFrame.h 18 Jul 2007 04:32:36 -0000
>@@ -114,29 +114,30 @@ public:
> nsRefPtr<gfxFontGroup> mFontGroup;
> nsAutoPtr<gfxFontStyle> mFontStyle;
> gfxPoint mPosition;
> PRUint8 mWhitespaceHandling;
>+ nsAutoArrayPtr<nsSVGBaselinePosition> mBaselinePositions;
>+ PRBool mDoCharPlacement; // do per character placement

This bloats the class size. Reuse the textPath mechanism and calculate this when you need it.

Also don't use PRBool as a class member, use PRPackedBool.

And finally order any member variables in sizeof order to ensure minimal sizeof of the class as a whole.

>+ gfxPoint mNextGlyphPosition;
>+ float mBaselineOffset;
>+ PRUint32 mStrLength;

mStrLength just bloats the class size.

How many of these new members are absolutely mandatory? Most text will not have multiple x and y positions so we don't want to pay for that all the time. Neither do we want to store anything we can easily calculate.