340859 - Implement pathLength

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Alex Menzies]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1; .NET CLR 1.1.4322; .NET CLR 2.0.50727)
Build Identifier: 


Upcoming patch implements the pathLength attribute for the nsSVGPathElement.  The patch adds code to scale startOffset when it is used in a textPath and pathLength has been specified.  If the following SVG code is used:
<path id="box" d="m 0 0 h 100 v 100 h -100 v 100" pathLength="2"/>
<text><textPath xlink:href="#box" startOffset="1">X</textPath></text>

Then the X will be printed at an offset of (startOffset*(totalLength/pathLength))  which in this case equals 1*(400/2) = 200.  This is the same as if a startOffset of 50% had been used.  I believe this is the correct behavior according to the SVG 1.1 specifications but both the Adobe plugin and Batik render the code above as if pathLength were not set.

Reproducible: Always

Comment 1

[Alex Menzies]

Attachment: Implements pathLength

Comment 2

[tor]

Comment on attachment 224900 [details] [diff] [review]
Implements pathLength

Some mozilla style notes...

>--- content/svg/content/src/nsISVGPathFlatten.h	3 May 2006 17:01:27 -0000	1.3
>+++ content/svg/content/src/nsISVGPathFlatten.h	8 Jun 2006 18:46:46 -0000
>@@ -54,21 +54,23 @@ class nsIFrame;
> 
> class nsSVGPathData
> {
> private:
>   PRUint32 arraysize;
> 
> public:
>   PRUint32 count;
>+  bool pathLengthIsSet;
>+  float pathLength;
>   float *x;
>   float *y;
>   PRUint8 *type;

While you're adding things, move the pointers to the top.

"bool" shouldn't be used in mozilla code - rather PRBool with values PR_TRUE and PR_FALSE.

>--- layout/svg/base/src/nsSVGTextFrame.cpp	11 May 2006 21:24:59 -0000	1.42
>+++ layout/svg/base/src/nsSVGTextFrame.cpp	8 Jun 2006 18:47:10 -0000
>@@ -902,45 +902,52 @@ GetSingleValue(nsISVGGlyphFragmentLeaf *
> #ifdef DEBUG
>   if (count > 1)
>     NS_WARNING("multiple lengths for x/y attributes on <text> elements not implemented yet!");
> #endif
>   if (count) {
>     nsCOMPtr<nsIDOMSVGLength> length;
>     list->GetItem(0, getter_AddRefs(length));
>     length->GetValue(val);
>+    
>+	/* Lookup path data */

nit - fix indent.

>+    nsIFrame *glyph;
>+    CallQueryInterface(fragment, &glyph);
>+
>+    nsISVGPathFlatten *textPath = nsnull;
>+    /* check if we're the child of a textPath */
>+    for (nsIFrame *frame = glyph; frame != nsnull; frame = frame->GetParent())
>+      if (frame->GetType() == nsLayoutAtoms::svgTextPathFrame) {
>+        frame->QueryInterface(NS_GET_IID(nsISVGPathFlatten), 
>+                              (void **)&textPath);

Use CallQueryInterface here.

>+      else if(data->pathLengthIsSet) {	
>+      	if(data->pathLength){
>+      	  length->GetValue(val);
>+      	  *val *= (data->Length() / data->pathLength);
>+      	}

Indentation in mozilla code is two spaces, and make sure your editor doesn't insert tab characters.

Mozilla style is to put a space between the "if" and condition.

Parens on the "*val *= ..." line not needed.

>+  nsresult rv = flatten->GetFlattenedPath(data, this);
>+  NS_ENSURE_SUCCESS(rv,rv);
>+  
>+  if((path->GetContent())->HasAttr(kNameSpaceID_None, nsSVGAtoms::pathLength)) {
>+    nsCOMPtr<nsIDOMSVGPathElement> pe = do_QueryInterface(path->GetContent());
>+    if(!pe)
>+  	  return NS_ERROR_FAILURE;
>+    nsIDOMSVGAnimatedNumber* pathLength = nsnull;
>+    pe->GetPathLength(&pathLength);
>+    if(!pathLength)
>+  	  return NS_ERROR_FAILURE;
>+    pathLength->GetAnimVal(&((*data)->pathLength));
>+    (*data)->pathLengthIsSet = true;
>+  }
>+  return rv;
> }

"if" spacing needs fixing.

Comment 3

[Robert Longson [:longsonr]]

Comment on attachment 224900 [details] [diff] [review]
Implements pathLength

My 2c for what it's worth...

for data in classes shouldn't we should use PRPackedBool (rather than bool or PRBool), if so it should go last.

> +  nsSVGPathData() : arraysize(0), count(0), pathLengthIsSet(false), pathLength(0), x(nsnull), y(nsnull), type(nsnull) {}

Could you split this to keep line lengths to < 80 characters.

Comment 4

[Alex Menzies]

Attachment: Updated Patch

This patch corrects the problems pointed out in comments 2 and 3 (thanks for the feedback) and also makes GetSingleValue return 0 in the case when pathLength="0".    This has the effect of rendering the text at an offset of 0 regardless of the value given in the startOffset attribute.

Comment 5

[Robert Longson [:longsonr]]

Comment on attachment 225100 [details] [diff] [review]
Updated Patch

I'm not a peer or module owner for SVG - http://www.mozilla.org/owners.html#svg

Although I'm happy to comment informally I can't give you a review+ without tor's permission.

Anyway...

+  PRPackedBool pathLengthIsSet;

I think this would fit our naming conventions better as hasPathLength.

+    nsISVGPathFlatten *textPath = nsnull;
+    /* check if we're the child of a textPath */
+    for (nsIFrame *frame = glyph; frame != nsnull; frame = frame->GetParent())
+      if (frame->GetType() == nsLayoutAtoms::svgTextPathFrame) {
+        CallQueryInterface(frame,&textPath);
+        break;
+      }

I think we should take this opportunity to create a FindTextPathParent method on the nsIGlyphFragmentLeaf interface with an implementation in nsSVGGlyphFrame. Then we can call that method here and also in nsSVGGlyphFrame::GetCharacterPosition to replace the existing code. See https://bugzilla.mozilla.org/attachment.cgi?id=224195&action=view although that attempt used the wrong interface. Don't forget to change the GUID of the interface when you add the new member.

+    nsCOMPtr<nsIDOMSVGPathElement> pe = do_QueryInterface(path->GetContent());

I believe this could/should be rewritten to use CallQueryInterface

+  NS_ENSURE_SUCCESS(rv,rv);

nit: Can you put in a space after the comma please?

+      }
+      else if (data->pathLengthIsSet) {	

nit: Should be one line.

+  if ((path->GetContent())->HasAttr(kNameSpaceID_None, 
+                                    nsSVGAtoms::pathLength)) {

Can you use mContent rather than path->GetContent()? - occurs twice

Also s/nsSVGAtoms/nsGkAtoms/

More general issues:

What happens if someone removes the pathLength attribute using DOM? pathLengthIsSet will remain set to PR_TRUE.

I think you can leak "data" in nsSVGTextFrame.cpp::GetSingleValue it is only deleted if type == nsIDOMSVGLength::SVG_LENGTHTYPE_PERCENTAGE.

The code which tracks whether a pathLength attribute exists might be better moved to nsSVGPathElement::AfterSetAttr (you would have to implement this override to the base class) That way it would only be called when the pathLength attribute was changed, rather than every time nsSVGTextPathFrame::GetFlattenedPath is called.

Comment 6

[Robert Longson [:longsonr]]

Alex. I hope you don't mind but I've assigned the bug to you as you are working on it.

Comment 7

[Robert Longson [:longsonr]]

PS. You may wish to hold off any rework till 341292 lands, as that would seem to change the ground under you somewhat.

Comment 8

[Robert Longson [:longsonr]]

Oops, I mean't bug 341292 if that wasn't clear.

Comment 9

[Alex Menzies]

Attachment: Updated Patch

Reworked patch now that nsISVGPathFlatten has been removed.  Created a FindTextPathParent method as suggested in the comments above.  Unlike the last version of this patch, the value and status of the pathLength attribute are looked up directly in GetSingleValue to ensure freshness of data.

Comment 10

[tor]

Comment on attachment 227419 [details] [diff] [review]
Updated Patch

>+nsresult
>+nsSVGPathElement::Init()
>+{
>+  nsresult rv = nsSVGPathElementBase::Init();
>+  NS_ENSURE_SUCCESS(rv,rv);

Space after the comma.

>+  {
>+    rv = NS_NewSVGAnimatedNumber(getter_AddRefs(mPathLength),0.0);

Space after the comma.

> class nsISVGGlyphFragmentLeaf : public nsISVGGlyphFragmentNode
> {
> public:
> 
>   NS_DECLARE_STATIC_IID_ACCESSOR(NS_ISVGGLYPHFRAGMENTLEAF_IID)
> 
>   NS_IMETHOD_(void) SetGlyphPosition(float x, float y)=0;
>   NS_IMETHOD GetGlyphMetrics(nsISVGRendererGlyphMetrics** metrics)=0;
>+  NS_IMETHOD_(void) FindTextPathParent(nsSVGTextPathFrame** textPath)=0;

Make this method just return nsSVGTextPathFrame*.

>+    if (textPath) {
>+      nsAutoPtr<nsSVGFlattenedPath> data(textPath->GetFlattenedPath());
>+      if (!data)
>+        return;
>+
>+      nsIFrame *path = textPath->GetPath();
>+      nsSVGPathElement *pathElement = NS_STATIC_CAST(nsSVGPathElement*,
>+                                                     path->GetContent());

You need to QI and make sure it was a nsSVGPathElement, as GetPath can return any path-based frame (rect, circle, etc...).

>+
>+      /* check for % sizing of textpath */
>+      PRUint16 type;
>+      length->GetUnitType(&type);
>+      if (type == nsIDOMSVGLength::SVG_LENGTHTYPE_PERCENTAGE) {
>         float percent;
>         length->GetValueInSpecifiedUnits(&percent);
> 
>         *val = data->GetLength()*percent/100.0f;
>+      } 
>+      else if (pathElement->HasAttr(kNameSpaceID_None, nsGkAtoms::pathLength)) {

"else" on same line as "}"

Comment 11

[Robert Longson [:longsonr]]

> nsIFrame *
> +nsSVGTextPathFrame::GetPath() {

Wouldn't this be better named getPathFrame since you are returning a frame?

> +NS_IMETHODIMP_(void) 
> +nsSVGGlyphFrame::FindTextPathParent(nsSVGTextPathFrame** textPath)
> +{
> +  /* check if we're the child of a textPath */
> +  for (nsIFrame *frame = this; frame != nsnull; frame = frame->GetParent())
> +    if (frame->GetType() == nsLayoutAtoms::svgTextPathFrame) {
> +      *textPath = NS_STATIC_CAST(nsSVGTextPathFrame*, frame);
> +      break;
> +    }
> +}
> +

Please update this based on the current CVS implementation in GetCharacterPosition. That starts at the parent of the glyph frame and stops if an svgTextFrame is encountered.

Comment 12

[Robert Longson [:longsonr]]

(In reply to comment #11)
> > nsIFrame *
> > +nsSVGTextPathFrame::GetPath() {
> 
> Wouldn't this be better named getPathFrame since you are returning a frame?

Oops meant GetPathFrame not getPathFrame.

Comment 13

[Alex Menzies]

Attachment: Patch with style and QI fixes

-Renamed GetPath to GetPathFrame
-Corrected FindTextPathParent
-Changed return type of FindTextPathParent
-Fixed white space style issues
-Replaced static cast with QI and added type check

Comment 14

[tor]

Comment on attachment 227591 [details] [diff] [review]
Patch with style and QI fixes

>+NS_IMETHODIMP_(nsSVGTextPathFrame*) 
>+nsSVGGlyphFrame::FindTextPathParent()
>+{
>+  nsSVGTextPathFrame* textPath = nsnull;
>+  /* check if we're the child of a textPath */
>+  for (nsIFrame *frame = GetParent();
>+       frame != nsnull;
>+       frame = frame->GetParent()) {
>+    if (frame->GetType() == nsLayoutAtoms::svgTextPathFrame) {
>+      textPath = NS_STATIC_CAST(nsSVGTextPathFrame*, frame);

Just return the cast value, instead of assigning to a variable and breaking.

>+      break;
>+    }
>+    if (frame->GetType() == nsLayoutAtoms::svgTextFrame)

"else if"

>@@ -699,41 +700,50 @@ GetSingleValue(nsISVGGlyphFragmentLeaf *
...
>+      nsIFrame *pathFrame = textPath->GetPathFrame();

You need to check if you got a valid return from this, or you'll crash doing the GetContent().

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

Combine into a single declaration/assignment.

>+nsSVGFlattenedPath *
>+nsSVGTextPathFrame::GetFlattenedPath() {
>+  nsIFrame *path = GetPathFrame();
>+  if(!path)

"if (...)"

Comment 15

[Alex Menzies]

Attachment: Updated Patch

Fixed errors described in comment 14

Comment 16

[Robert Longson [:longsonr]]

> #ifndef __NS_ISVGGLYPHFRAGMENTLEAF_H__
> #define __NS_ISVGGLYPHFRAGMENTLEAF_H__
> 
> #include "nsISVGGlyphFragmentNode.h"
> #include "nsIDOMSVGLengthList.h"
>+#include "nsSVGTextPathFrame.h"

Could get away with 

class nsSVGTextPathFrame; 

instead. Then have the include in the .cpp file.

>+    nsSVGTextPathFrame *textPath = fragment->FindTextPathParent();
> 
>+    if (textPath) {
>+      nsAutoPtr<nsSVGFlattenedPath> data(textPath->GetFlattenedPath());
>+      if (!data)
>+        return;
>+
>+      nsIFrame *pathFrame = textPath->GetPathFrame();
>+      if (!textPath)
>+        return;

Wrong test I think. We already checked for textPath above.

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

Use CallQueryInterface? 

Also we only need the pathElement for the else case in the code below this don't we so all this code could be moved down into there?

Comment 17

[Alex Menzies]

Attachment: Updated Patch

Robert:
Good catch on the "if" check.  Can't believe I did that.  I made the other changes you recommended except I left do_QueryInterface as is because I believe it is the correct way to get an nsIDOMSVGPathElement.  Thanks a lot for the feedback!

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah, you should use do_QueryInterface when the result goes into an nsCOMPtr.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 227690 [details] [diff] [review]
Updated Patch

+    if (frame->GetType() == nsLayoutAtoms::svgTextPathFrame) {
+      return NS_STATIC_CAST(nsSVGTextPathFrame*, frame);
+    } else if (frame->GetType() == nsLayoutAtoms::svgTextFrame)
+      return nsnull;

Just call GetType() once, and reuse the result. with that, sr=roc

Comment 20

[Alex Menzies]

Attachment: Version for checkin

Made change requested in Comment #19 by storing the result of GetType() in a nsIAtom pointer

Comment 21

[tor]

Checked in for Alex.