Comment 20 for bug 573365

(From update of attachment 276088)
General points:

If you are working with gfxFloat then literal assignment should not use f as gfxFloat is a double. If you are working with float then you should use an f suffix on literals.

Does inheritance of tspan values work? Have you tested that? Something like this contrived example...

<text x="1 2 3 4 5" y="4 5 6 7">h<tspan dx="5 6">e<tspan>l</tspan>lo</tspan></text>

The first l in hello has no dx in its tspan so it should pick up the parent tspan dx, presumably the 6.

Neither of the tspans have x or y values so they should pick up the text values where they exist.

>+float
>+nsSVGGlyphFrame::GetSubStringAdvance(PRUint32 charnum, PRUint32 fragmentChars)
>+{

>+ if (fragmentChars == 0) // do the whole run
>+ fragmentChars = text.Length();

Fix the caller to pass in the right value rather than having a magic number.

>+ for (PRUint32 i = charnum; i < dxcount; i++) {
>+ float dx = 0.0;

0.0f

>+void
>+nsSVGGlyphFrame::SetCharPosition(gfxTextRun *textRun,
>+ gfxPoint &ctp)
>+{

>+ gfxFloat pathScale = 1.0f; //only applied to dx,dy lists since x,y lists nsnull for nsSVGTextPathFrame

Put the comment on the previous line to avoid a long line.

>+ x -= advance/2.0f;

2.0

> NS_IMETHODIMP
> nsSVGGlyphFrame::GetStartPositionOfChar(PRUint32 charnum, nsIDOMSVGPoint **_retval)
> {

>+ if (angle != 0.0f) {

0.0

> NS_IMETHODIMP
> nsSVGGlyphFrame::GetEndPositionOfChar(PRUint32 charnum, nsIDOMSVGPoint **_retval)
>@@ -834,23 +1013,30 @@ nsSVGGlyphFrame::GetEndPositionOfChar(PR
> return NS_ERROR_OUT_OF_MEMORY;
>
>+ gfxFloat angle = cp[charnum].angle;
>+ if (angle != 0.0f) {

0.0

>Index: layout/svg/base/src/nsSVGGlyphFrame.h
>===================================================================
>RCS file: /cvsroot/mozilla/layout/svg/base/src/nsSVGGlyphFrame.h,v
>retrieving revision 1.26
>diff -u -8 -p -r1.26 nsSVGGlyphFrame.h
>--- layout/svg/base/src/nsSVGGlyphFrame.h 3 Aug 2007 08:39:13 -0000 1.26
>+++ layout/svg/base/src/nsSVGGlyphFrame.h 10 Aug 2007 06:01:19 -0000

>- gfxPoint mPosition;
>+ gfxPoint mPosition; // character position of first glyph, includes baseline offset
>+ nsAutoArrayPtr<gfxPoint> mCharPositions; // character position of each glyph,
>+ // valid if not a simple textrun and/or textPath

Put the comments above the variables so we don't have long lines.

How about storing mCharPositions in a property. It would not be a member variable here then so the memory used by nsSVGGlyphFrame would be that bit smaller in most cases.

You would have

gfxPoint * charPositions = new gfxPoint[strLength];
SetProperty(nsGkAtoms::d, charPositions, positionDtorFunc);

positionDtorFunc would delete[] the charPositions data.

I picked d as there is no xy atom and d goes with paths.

Then if you needed individual character positions you could call

      charPositions = static_cast<gfxPoint *>
                            (GetProperty(nsGkAtoms::d));

There are various examples of Set/GetProperty: nsBlockFrame shows some usage as does nsSVGUtils (although that uses pointers to classes rather than pointers to POD). Note that when the frame is destroyed the Property goes with it and the positionDtorFunction is automatically called.

>+ float mBaselineOffset;
> PRUint8 mWhitespaceHandling;
> };
>
> #endif
>Index: layout/svg/base/src/nsSVGTextFrame.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/layout/svg/base/src/nsSVGTextFrame.cpp,v
>retrieving revision 1.61
>diff -u -8 -p -r1.61 nsSVGTextFrame.cpp
>--- layout/svg/base/src/nsSVGTextFrame.cpp 3 Aug 2007 08:39:13 -0000 1.61
>+++ layout/svg/base/src/nsSVGTextFrame.cpp 10 Aug 2007 06:01:19 -0000
>@@ -293,71 +293,29 @@ nsSVGTextFrame::GetCanvasTM()

> void
> nsSVGTextFrame::UpdateGlyphPositioning()
> {

>- float x = 0, y = 0;
>+ gfxPoint ctp(0.0,0.0);

add a space after the comma.

>
>===================================================================
>RCS file: /cvsroot/mozilla/layout/svg/base/src/nsSVGTextPathFrame.cpp,v
>retrieving revision 1.31
>diff -u -8 -p -r1.31 nsSVGTextPathFrame.cpp
>--- layout/svg/base/src/nsSVGTextPathFrame.cpp 25 Jul 2007 09:16:02 -0000 1.31
>+++ layout/svg/base/src/nsSVGTextPathFrame.cpp 10 Aug 2007 06:01:19 -0000

> already_AddRefed<gfxFlattenedPath>
>-nsSVGTextPathFrame::GetFlattenedPath()
>+nsSVGTextPathFrame::GetFlattenedPath(nsIFrame *path)

Better to have an private function that takes a path and another public function that does not. The public function calls GetPathFrame() and then calls the private function.

>+float
>+nsSVGTextPathFrame::GetStartOffset() {

>+ startOffset = val * data->GetLength() / 100.0f;

just return val * data->GetLength() / 100.0f;

>+ } // else startOffset remains 0
>+ } else {
>+ startOffset = val * GetPathScale();

return val * GetPathScale();

>+ }
>+ }
>+ return startOffset;

return 0.0f;

>+float
>+nsSVGTextPathFrame::GetPathScale()
>+{
>+ float scale = 1.0f;
>+ nsIFrame *pathFrame = GetPathFrame();
>+ if (pathFrame && pathFrame->GetContent()->HasAttr(kNameSpaceID_None,
>+ nsGkAtoms::pathLength)) {

You really need some early returns in this function rather than all this indenting. E.g.

  if (!pathFrame || !pathFrame->GetContent()...)
    return 1.0f;

>+ nsCOMPtr<nsIDOMSVGPathElement> pathElement =
>+ do_QueryInterface(pathFrame->GetContent());
>+ if (pathElement) {

  if (!pathElement)
    return 1.0f;

etc. And just return the result if you calculate something non-zero.