934305 - Convert much of the SVG code for calculating path lengths and position at an offset along a path to Moz2D

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

Attachment: patch

The reason for having the GetPathForLengthOrPositionMeasuring method in addition to the normal BuildPath method is because of the square-caps-on-zero-length-paths requirement in the SVG spec. Since I hold you partially responsible for that I'll request review from you, Cameron. ;)

Comment 1

[Cameron McCormack (:heycam)]

Comment on attachment 826539 [details] [diff] [review]
patch

Review of attachment 826539 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.

::: content/svg/content/src/SVGPathData.cpp
@@ +808,5 @@
> +  // matter what we pass to BuildPath as aFillRule. Hawever, we do want to
> +  // pass something other than NS_STYLE_STROKE_LINECAP_SQUARE as aStrokeLineCap
> +  // to avoid the insertion of extra little lines (by
> +  // ApproximateZeroLengthSubpathSquareCaps), in which case the value that we
> +  // pass as aStrokeWidth doesn't matter.

Can you say a bit more about why aStrokeWidth doesn't matter when we pass in a stroke-linecap value other than square?

::: layout/svg/nsSVGTextFrame2.cpp
@@ +4737,3 @@
>  nsSVGTextFrame2::GetTextPath(nsIFrame* aTextPathFrame)
>  {
> +  nsIFrame *textPathFrame = GetTextPathPathFrame(aTextPathFrame);

Call this variable something else?  Looks confusing since aTextPathFrame is the nsSVGTextPathFrame, but textPathFrame will be the nsSVGPathGeometryFrame.

@@ +4748,5 @@
> +    if (!matrix.IsIdentity()) {
> +      RefPtr<PathBuilder> builder =
> +        path->TransformedCopyToBuilder(ToMatrix(matrix));
> +      path = builder->Finish();
> +    }

This (transform a Path using TransformedCopyToBuilder if a gfxMatrix is not identity) seems to be a common idiom that we're needing for the Moz2D conversions -- there are three instances of it in this patch.  Should we hoist it up into a utility method somewhere?

@@ +4780,3 @@
>      return data ?
> +      length->GetAnimValInSpecifiedUnits() * data->ComputeLength() / 100.0 :
> +      0.0;

Guess it doesn't need to be done now, but at some point the 100.0 and 0.0 should change to floats.  We're still returning gfxFloat at the moment though.

::: layout/svg/nsSVGTextPathFrame.h
@@ +7,5 @@
>  #define NSSVGTEXTPATHFRAME_H
>  
>  #include "mozilla/Attributes.h"
> +#include "mozilla/gfx/2D.h"
> +#include "mozilla/RefPtr.h"

Ordering seems off.

Comment 2

[Jonathan Watt [:jwatt]]

(In reply to Cameron McCormack (:heycam) from comment #1)
> This (transform a Path using TransformedCopyToBuilder if a gfxMatrix is not
> identity) seems to be a common idiom that we're needing for the Moz2D
> conversions -- there are three instances of it in this patch.  Should we
> hoist it up into a utility method somewhere?

I don't think so, personally.

> Guess it doesn't need to be done now, but at some point the 100.0 and 0.0
> should change to floats.  We're still returning gfxFloat at the moment
> though.

Right, but that would be done when the return type changes.

> ::: layout/svg/nsSVGTextPathFrame.h
> @@ +7,5 @@
> >  #define NSSVGTEXTPATHFRAME_H
> >  
> >  #include "mozilla/Attributes.h"
> > +#include "mozilla/gfx/2D.h"
> > +#include "mozilla/RefPtr.h"
> 
> Ordering seems off.

I've been treating everything between the quotes as a opaque string for the purposes of ordered includes case insensitively. It's correct in that regard, and consistent with other code I've landed and reviewed.

Comment 3

[Jonathan Watt [:jwatt]]

The commit for this had the wrong bug number so the push links were noted in the wrong bug. This patch has landed and been merged:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f086025350f
https://hg.mozilla.org/mozilla-central/rev/4f086025350f