Closed Bug 934305 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: jwatt, Assigned: jwatt)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qa-])

Attachments

(1 file)

Attached patch patchSplinter Review
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. ;)
Attachment #826539 - Flags: review?(cam)
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.
Attachment #826539 - Flags: review?(cam) → review+
Depends on: 935049
Blocks: 926258
(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.
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
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Summary: Convert most of the SVG code for calculating path lengths and position at an offset along a path to Moz2D → Convert much of the SVG code for calculating path lengths and position at an offset along a path to Moz2D
Target Milestone: --- → mozilla28
Depends on: 950549
Whiteboard: [qa-]
Depends on: 1160471
You need to log in before you can comment on or make changes to this bug.