Comment 11 for bug 345207

(From update of attachment 236868)
My 2c, for what its worth.

I think my fundamental structural concern is that this does not really use either the GetNextGlyphFragmentChildNode or GetNextGlyphFragment to traverse the fragment trees using functions defined in the node/fragment interfaces and I think, if possible it should. I'm not absolutely sure which one you want. The first is recursive and skips nsSVGGlyphFrames, the second traverses rather than recurses and includes nsSVGGlyphFrames. I see signs of both in the code below.

> *val = data->GetLength()*percent/100.0f;
>- } else if (pathFrame->GetContent()->HasAttr(kNameSpaceID_None,
>+ } else if (pathFrame->GetContent()->HasAttr(kNameSpaceID_None,
> nsGkAtoms::pathLength)) {
>- nsCOMPtr<nsIDOMSVGPathElement> pathElement =
>+ nsCOMPtr<nsIDOMSVGPathElement> pathElement =
> do_QueryInterface(pathFrame->GetContent());

These are the same to my eye. Could you have inserted DOS style LF/CR into these lines?

> pathLength->GetAnimVal(&pl);
>- if (pl)
>+ if (pl)
> *val *= data->GetLength() / pl;
>- else
>+ else
> *val = 0;

As above.

>+PRBool
>+IsAbsolutelyPositioned(nsIFrame* frame)

I'm not sure why you implemented this here rather than using the implementation in nsSVGGlyphFrame.cpp. If you are going to change where it lives, I would have thought it made more sense to make this a member function of nsSVGTextContainerFrame it would not need any arguments then.

>- {
>- nsCOMPtr<nsIDOMSVGLengthList> list = GetX();
>- GetSingleValue(firstFragment, list, &x);
>+/*
>+ * Performs a pre-order traversal on the subtree rooted at “node” until an
>+ * absolutely positioned node is found or until the entire subtree has been
>+ * explored. Sum is increased by the total advance of all the nodes visited.
>+ * Returns PR_TRUE if the subtree contained an absolutely positioned node.
>+ */
>+PRBool
>+GetSubTreeGlyphAdvance(nsIFrame* node,
>+ nsSVGTextPathFrame** textPath,
>+ float *sum)
>+{
>+ if (!node)
>+ return PR_FALSE;
>+
>+ if (node->GetType() == nsLayoutAtoms::svgTextPathFrame) {
>+ *textPath = NS_STATIC_CAST(nsSVGTextPathFrame*, node);
>+ } else if (node->GetType() == nsLayoutAtoms::svgTSpanFrame) {
>+ nsSVGTextContainerFrame *cf = NS_STATIC_CAST(nsSVGTextContainerFrame*,node);
>+
>+ float dx = 0;
>+ nsCOMPtr<nsIDOMSVGLengthList> list = cf->GetDx();
>+ GetSingleValue(*textPath, list, &dx);
>+ *sum += dx;
>+
>+ } else if (node->GetType() == nsLayoutAtoms::svgGlyphFrame) {
>+ nsCOMPtr<nsISVGGlyphFragmentLeaf> fragment = do_QueryInterface(node);
>+ *sum += fragment->GetAdvance();
> }
>- {
>- nsCOMPtr<nsIDOMSVGLengthList> list = GetY();
>- GetSingleValue(firstFragment, list, &y);
>+
>+ nsIFrame* nextNode = node->GetFirstChild(nsnull);

Could you work with GetFirstGlyphFragmentChildNode and GetNextGlyphFragmentChildNode. Can GetSubTreeGlyphAdvance be part of the nsSVGGlyphFragmentNode interface?

>+ while (nextNode) {
>+ if (IsAbsolutelyPositioned(nextNode) ||
>+ GetSubTreeGlyphAdvance(nextNode, textPath, sum))
>+ return PR_TRUE;
>+ nextNode = nextNode->GetNextSibling();
> }
>+ return PR_FALSE;
>+}
>

>+/*
>+ * Returns the total advancement for all glyphs between “node” and the next
>+ * absolutely positioned node.
>+ */
>+float
>+GetChunkLength(nsIFrame* node, nsSVGTextPathFrame* textPath, nsIFrame* root)
>+{
>+ float sum = 0;
>+ while (node) {
>+ if (GetSubTreeGlyphAdvance(node, &textPath, &sum)) {
>+ return sum;
>+ //The subtree rooted at NODE doesn't contain any absolutely positioned nodes
>+ if (node->GetType() == nsLayoutAtoms::svgTextPathFrame)
>+ textPath = nsnull;
>+
>+ nsIFrame* nextNode = node->GetNextSibling();
>+ if (!nextNode) {
>+ //The entire subtree rooted at NODE's parent has been explored. Go up
>+ //the tree until a parent with siblings is found or until we reach the
>+ //root textFrame element

This kind of navigation suggests you should be traversing with GetNextGlyphFragment and working with glyph fragments.

>+ nsIFrame* parent = node->GetParent();
>+ while (parent && node != root) {
>+ if (parent->GetType() == nsLayoutAtoms::svgTextPathFrame)
>+ textPath = nsnull;
>+ nextNode = parent->GetNextSibling();
>+ if (nextNode)
> break;
>+ parent = parent->GetParent();
> }
> }

>+void
>+nsSVGTextFrame::UpdateGlyphPositioningHelper(nsIFrame* node,
>+ nsSVGTextPathFrame *textPath,
>+ nsCOMPtr<nsISVGGlyphFragmentLeaf> *fragment,
>+ float *x, float *y)
>+{

>+ if (node->GetType() == nsLayoutAtoms::svgTextFrame ||
>+ node->GetType() == nsLayoutAtoms::svgTSpanFrame ||
>+ node->GetType() == nsLayoutAtoms::svgTextPathFrame)

Use CallQueryInterface and see if the node supports nsISVGTextContentMetrics instead of the above tests.

>+ {
>+ nsSVGTextContainerFrame *cf = NS_STATIC_CAST(nsSVGTextContainerFrame*,node);

Nit: space after comma.

>+ if (node->GetType() == nsLayoutAtoms::svgTextPathFrame) {
>+ textPath = NS_STATIC_CAST(nsSVGTextPathFrame*, node);
>+ *fragment = nsnull;
>+ *x = *y = 0;
>+ } else {
> {
>- nsCOMPtr<nsIDOMSVGLengthList> list = fragment->GetDx();
>- GetSingleValue(fragment, list, &dx);
>+ nsCOMPtr<nsIDOMSVGLengthList> list = cf->GetX();
>+ GetSingleValue(textPath, list, x);
> }
> {
>- nsCOMPtr<nsIDOMSVGLengthList> list = fragment->GetDy();
>- GetSingleValue(fragment, list, &dy);
>+ nsCOMPtr<nsIDOMSVGLengthList> list = cf->GetY();
>+ GetSingleValue(textPath, list, y);
>+ }
>+ }
>+ {
>+ float dx = 0;
>+ nsCOMPtr<nsIDOMSVGLengthList> list = cf->GetDx();
>+ GetSingleValue(textPath, list, &dx);
>+ *x += dx;
>+ }
>+ {
>+ float dy = 0;
>+ nsCOMPtr<nsIDOMSVGLengthList> list = cf->GetDy();
>+ GetSingleValue(textPath, list, &dy);
>+ *y += dy;
>+ }
>+
>+ PRUint8 anchor = node->GetStyleSVG()->mTextAnchor;
>+ if (anchor != NS_STYLE_TEXT_ANCHOR_START && IsAbsolutelyPositioned(node)) {
>+ float chunkLength = GetChunkLength(node, textPath, this);
>+ if (anchor == NS_STYLE_TEXT_ANCHOR_MIDDLE)
>+ *x -= chunkLength/2.0f;
>+ else if (anchor == NS_STYLE_TEXT_ANCHOR_END)
>+ *x -= chunkLength;
>+ }
>+ } else if (node->GetType() == nsLayoutAtoms::svgGlyphFrame) {
>+ nsCOMPtr<nsISVGGlyphFragmentLeaf> curFrag = do_QueryInterface(node);
>+ float baseline_offset = curFrag->GetBaselineOffset(GetBaseline());
>+ curFrag->SetGlyphPosition(*x, *y - baseline_offset);
>+ *x += curFrag->GetAdvance();
>+
>+ if (textPath && curFrag->GetNumberOfChars() > 0) {
>+ nsIDOMSVGPoint* pt;
>+ curFrag->GetEndPositionOfChar(0, &pt);
>+ if (pt) {
>+ *fragment = curFrag;
> }
>+ }
>+ }
>
>- float baseline_offset = fragment->GetBaselineOffset(baseline);
>- fragment->SetGlyphPosition(x + dx, y + dy - baseline_offset);
>+ nsIFrame* nextNode = node->GetFirstChild(nsnull);
>+ while (nextNode) {
>+ UpdateGlyphPositioningHelper(nextNode, textPath, fragment, x, y);
>+ nextNode = nextNode->GetNextSibling();
>+ }
>
>- x += dx + fragment->GetAdvance();
>- y += dy;
>- fragment = fragment->GetNextGlyphFragment();
>- if (fragment && fragment->IsAbsolutelyPositioned())
>- break;
>+ if (node->GetType() == nsLayoutAtoms::svgTextPathFrame) {
>+ if (*fragment) {
>+ nsIDOMSVGPoint* pt;
>+ PRUint32 numChars = (*fragment)->GetNumberOfChars();
>+ for (PRUint32 i = numChars - 1; i >= 0; --i) {
>+ (*fragment)->GetEndPositionOfChar(i, &pt);

This is going to be very slow. I think you need a function to all the positions at once and return them in an array if you need to do this.

>+ if (pt) {
>+ pt->GetX(x);

>Index: layout/svg/base/src/nsSVGTextFrame.h
>===================================================================
>RCS file: /cvsroot/mozilla/layout/svg/base/src/nsSVGTextFrame.h,v
>retrieving revision 1.1
>diff -u -8 -p -r1.1 nsSVGTextFrame.h
>--- layout/svg/base/src/nsSVGTextFrame.h 28 Jun 2006 15:23:40 -0000 1.1
>+++ layout/svg/base/src/nsSVGTextFrame.h 5 Sep 2006 21:09:22 -0000
>@@ -37,16 +37,17 @@
> * ***** END LICENSE BLOCK ***** */
>
> #ifndef NS_SVGTEXTFRAME_H
> #define NS_SVGTEXTFRAME_H
>
> #include "nsSVGTextContainerFrame.h"
> #include "nsISVGValueObserver.h"
> #include "nsWeakReference.h"
>+#include "nsSVGTextPathFrame.h"

You only need class nsSVGTextPathFrame here.

>+ /*
>+ * Performs a pre-order traversal on the subtree rooted at “node” and updates
>+ * the position of each glyph fragment as its svgGlyphFrame is visited.
>+ * node – may be any type of frame that can be contained within an
>+ * svgTextFrame
>+ * textPath – node's most recent ancestor of type svgTextPathFrame. nsnull
>+ * if no such ancestor exists.
>+ * fragment - If node has an ancestor of type svgTextPathFrame, then fragment
>+ * is the most recent glyph fragment to have drawn at least 1
>+ * character onto the path.

Shouldn't this function argument be an ordinary pointer rather than an XPCOM thing?

>+ * x, y – absolute coordinates containing the current text position.
>+ */
>+ void UpdateGlyphPositioningHelper(nsIFrame* node,
>+ nsSVGTextPathFrame *textPath,
>+ nsCOMPtr<nsISVGGlyphFragmentLeaf> *fragment,
>+ float *x, float *y);