To convert SVG to Moz2D, we need Moz2D versions of the nsSVGPathGeometryElement::ConstructPath methods for each SVG element type.

Comment on attachment 825889 [details] [diff] [review] patch >+{ >+ RefPtr<DrawTarget> drawTarget = >+ gfxPlatform::GetPlatform()->ScreenReferenceDrawTarget(); >+ NS_ASSERTION(gfxPlatform::GetPlatform()-> >+ SupportsAzureContentForDrawTarget(drawTarget), >+ "Should support Moz2D content drawing"); >+ >+ // The fill rule only matters for painting, so it's okay to pass the computed >+ // value of our 'fill-rule' property here regardless of who's calling us or >+ // what they're going to do with the path that we return. >+ RefPtr<PathBuilder> pathBuilder = >+ drawTarget->CreatePathBuilder(SVGContentUtils::GetFillRule(this)); Implement BuildPath on nsSVGPathGeometryElement and have it call a virtual method to construct the path that takes a pathBuilder and just does the tag specific bit to build the path. The above code would only exist once as would the pathBuilder->finish() that's also repeated a lot. If that's not feasible, at least move the above code to nsSVGPathGeometryElement so the various element classes can call a single implementation of this. >diff --git a/content/svg/content/src/SVGContentUtils.cpp b/content/svg/content/src/SVGContentUtils.cpp >--- a/content/svg/content/src/SVGContentUtils.cpp >+++ b/content/svg/content/src/SVGContentUtils.cpp >@@ -13,20 +13,22 @@ > #include "nsComputedDOMStyle.h" > #include "nsFontMetrics.h" > #include "nsIFrame.h" > #include "nsIScriptError.h" > #include "nsLayoutUtils.h" > #include "SVGAnimationElement.h" > #include "SVGAnimatedPreserveAspectRatio.h" > #include "nsContentUtils.h" >+#include "nsSVGUtils.h" We've tried hard not to have SVGContentUtils depend on nsSVGUtils so far. >+FillRule >+SVGContentUtils::GetFillRule(Element* aElement) Move this to nsSVGPathGeometryElement as a member function. >+Float >+SVGContentUtils::GetStrokeWidth(Element* aElement) Only used in SVGPathElement::BuildPath so move it there. >+ return nsSVGUtils::CoordToFloat(styleContext->PresContext(), >+ static_cast<nsSVGElement*>(aElement), >+ styleContext->StyleSVG()->mStrokeWidth); Might want to move sSVGUtils::CoordToFloat to SVGContentUtils instead. >+ >+ // Clamp rx and ry to half the rect's width and height respectively: >+ float halfWidth = width/2; >+ float halfHeight = height/2; Float?

Comment on attachment 825889 [details] [diff] [review] patch >+++ b/content/svg/content/src/nsSVGPathGeometryElement.h > virtual void ConstructPath(gfxContext *aCtx) = 0; >+ virtual mozilla::TemporaryRef<Path> BuildPath() = 0; > virtual already_AddRefed<gfxPath> GetPath(const gfxMatrix &aMatrix); So, now we'll have three slightly-differently-named "Spawn Path"-ish functions declared back-to-back. :) (ConstructPath, BuildPath, GetPath) Could you add a comment clarifying the difference between them, so that this looks less arbitrary? (e.g. it'd be worth mentioning that ConstructPath is pre-Moz2D and will be removed at some point, assuming that's correct. Speaking of which -- if there isn't already a bug on that removal, it'd probably be worth filing one (now that it's becoming redundant code), and then you can mention that bug number in this code-comment.) Also: is there any way to run the code you're adding in this patch? AFAICT, there are no callers of this new BuildPath function (yet).

(In reply to Robert Longson from comment #2) > Implement BuildPath on nsSVGPathGeometryElement and have it call a virtual > method to construct the path that takes a pathBuilder and just does the tag > specific bit to build the path. The above code would only exist once as > would the pathBuilder->finish() that's also repeated a lot. I was doing that in an earlier incarnation of this patch, but it doesn't work for SVGPathElement which needs to be able to do lots of different. It then works even less well for the future patches that I'm building on top of this where textPath, animateMotion, etc. want to play. > If that's not feasible, at least move the above code to > nsSVGPathGeometryElement so the various element classes can call a single > implementation of this. I can do that, but I'm not sure it's worth it given it's only really two lines of code. (The assert will die in the near future.) > We've tried hard not to have SVGContentUtils depend on nsSVGUtils so far. It's a single .cpp file, not the header. Why does it matter? I'm putting this code here because, thinking ahead, I want to be able to let script do things like get the bbox for an element without having to have a frame tree, making this the logical place. > >+SVGContentUtils::GetFillRule(Element* aElement) > > Move this to nsSVGPathGeometryElement as a member function. Okay. > >+SVGContentUtils::GetStrokeWidth(Element* aElement) > > Only used in SVGPathElement::BuildPath so move it there. I use it in other classes in future patches, so I'll move it to nsSVGPathGeometryElement too. > Might want to move sSVGUtils::CoordToFloat to SVGContentUtils instead. Can do, I suppose. I can then get rid of that include you're worried about. > Float? Yup, thanks.

(In reply to Daniel Holbert [:dholbert] from comment #3) > Could you add a comment clarifying the difference between them, so that this > looks less arbitrary? I'll do that in my next patch, since I rename one of those functions and comment it in that patch. > Also: is there any way to run the code you're adding in this patch? AFAICT, > there are no callers of this new BuildPath function (yet). No, the follow-up patches start using this stuff.

(In reply to Jonathan Watt [:jwatt] from comment #4) > > If that's not feasible, at least move the above code to > > nsSVGPathGeometryElement so the various element classes can call a single > > implementation of this. > > I can do that, but I'm not sure it's worth it given it's only really two > lines of code. (The assert will die in the near future.) Per IRC, I think it's worth it. (it's more than two lines if you consider that both lines are split, plus there's a comment). Nice to reduce copypasting and just use a single more concise invocation. The only other nit I have so far is: >+TemporaryRef<Path> >+nsSVGPolyElement::BuildPath() >+{ [...] >+ const SVGPointList &points = mPoints.GetAnimValue(); >+ >+ if (points.Length()) { This should be "if (!points.IsEmpty()) {" (Looks like the equivalent ConstructPath() code checks Length(); but we might as well have better style in the new version. :))

Comment on attachment 826115 [details] [diff] [review] patch >diff --git a/content/svg/content/src/SVGContentUtils.h b/content/svg/content/src/SVGContentUtils.h > #include "gfxMatrix.h" > #include "mozilla/RangedPtr.h" >+#include "mozilla/RefPtr.h" I don't think this #include belongs here. (Nothing in this header mentions RefPtr, particularly in the chunk added in this patch.) >diff --git a/content/svg/content/src/SVGImageElement.cpp b/content/svg/content/src/SVGImageElement.cpp [...] >+TemporaryRef<Path> >+SVGImageElement::BuildPath() >+{ >+ RefPtr<PathBuilder> pathBuilder = CreatePathBuilder(); >+ >+ float x, y, width, height; >+ GetAnimatedLengthValues(&x, &y, &width, &height, nullptr); >+ >+ if (width <= 0 || height <= 0) { >+ pathBuilder->MoveTo(Point(x, y)); >+ pathBuilder->LineTo(Point(x + width, y)); >+ pathBuilder->LineTo(Point(x + width, y + height)); >+ pathBuilder->LineTo(Point(x, y + height)); >+ pathBuilder->Close(); >+ } Two things: 1) Please copy the comment above ConstructPath somewhere here ("For the purposes of the update/invalidation logic pretend to be a rectangle.") 2) Let's simplify all of the LineTo() etc. commands with a Rect, e.g.: Rect r(x, y, width, height); pathBuilder->MoveTo(r.TopLeft()); pathBuilder->MoveTo(r.TopRight()); ...etc, rather than all the Point() lines w/ hardcoded addition. (Rect derives from BaseRect which provides these convenience methods.) >+++ b/content/svg/content/src/SVGPathData.cpp >+ * Since we don't have any information about transforms from user space to >+ * device space, we choose the length of the small line that we insert by >+ * making it a small percentage of the stroke width of the path. This should >+ * hopefully allow us to make the line as long as possible (to avoid rounding >+ * issues in the backend resulting in the backend seeing it as having zero >+ * length) while still avoiding the small rectangle being noticably different >+ * from a square. [...] >+ Float tinyLength = aStrokeWidth / 32; Does this break down if aStrokeWidth is 0? (I forget -- are line caps not supposed to draw in that case? (are they sized based on stroke-width?)) It's probably worth briefly mentioning that case in the comment here, since otherwise it looks like a situation where this "tinyLength" hack might break down. > #define MAYBE_APPROXIMATE_ZERO_LENGTH_SUBPATH_SQUARE_CAPS_TO_DT \ >- do { \ >- if (capsAreSquare && !subpathHasLength && subpathContainsNonArc && \ >- SVGPathSegUtils::IsValidType(prevSegType) && \ >- (!IsMoveto(prevSegType) || \ >- segType == PATHSEG_CLOSEPATH)) { \ >- ApproximateZeroLengthSubpathSquareCaps(segStart, aDT, builder); \ >- } \ >- } while(0) >+ if (capsAreSquare && !subpathHasLength && aStrokeWidth > 0 && \ >+ subpathContainsNonArc && SVGPathSegUtils::IsValidType(prevSegType) && \ >+ (!IsMoveto(prevSegType) || segType == PATHSEG_CLOSEPATH)) { \ >+ ApproximateZeroLengthSubpathSquareCaps(builder, segStart, aStrokeWidth); \ >+ } I think we need to preserve the "do { ... } while (0)" wrapper here. This allows callers to invoke it with a semicolon, like so: MAYBE_APPROXIMATE_ZERO_LENGTH_SUBPATH_SQUARE_CAPS_TO_DT; (which is how we invoke it everywhere) http://stackoverflow.com/questions/154136/do-while-and-if-else-statements-in-c-c-macros Right now, you'll end up with ";" after the closing brace of your "if" statement, which is a bit odd. (and might make compilers angry...? I forget) Alternately, you could drop the trailing semicolon from the callers (but I think it's nicer to have it, so that it looks more like normal code). >+SVGPathData::BuildPath(FillRule aFillRule, >+ uint8_t aStrokeLineCap, >+ Float aStrokeWidth) const > { [...] >+ // The path we return is used for painting, hit-testing and various bounds >+ // calculations. The fill rule only matters for painting, Per IRC, the fill-rule matters for hit-testing too, so please reword this to account for that. (sounds like we'll still do the right thing, which is good) >+++ b/content/svg/content/src/SVGPathElement.cpp >+#include "nsComputedDOMStyle.h" > #include "nsGkAtoms.h" >+#include "nsStyleConsts.h" >+#include "nsStyleStruct.h" >+#include "nsSVGUtils.h" >+#include "SVGContentUtils.h" Looks like the #include nsSVGUtils.h is unused. (probably left over from an earlier patch version?) Remove it, unless I'm missing something. >+++ b/content/svg/content/src/SVGRectElement.cpp >+SVGRectElement::BuildPath() >+{ >+ if (rx == 0 && ry == 0) { >+ // Optimization for the no rounded corners case. >+ pathBuilder->MoveTo(Point(x, y)); >+ pathBuilder->LineTo(Point(x + width, y)); >+ pathBuilder->LineTo(Point(x + width, y + height)); >+ pathBuilder->LineTo(Point(x, y + height)); As above for image, let's use a helper Rect here, and its TopLeft/TopRight/BottomRight/BottomLeft convenience methods. >+ } else { >+ // If either the 'rx' or the 'ry' attribute isn't set, then we have to >+ // set it to the value of the other: >+ bool hasRx = mLengthAttributes[ATTR_RX].IsExplicitlySet(); >+ bool hasRy = mLengthAttributes[ATTR_RY].IsExplicitlySet(); Maybe add a MOZ_ASSERT(hasRx || hasRy) here? >+ // Clamp rx and ry to half the rect's width and height respectively: >+ Float halfWidth = width/2; >+ Float halfHeight = height/2; I disagree w/ longsonr -- these should be lowercase "float", as you originally had them, since they're set from "float" values (width, height) and they're only used for comparisons to other "float" values (rx, ry). So they should be "float". >+ if (rx > halfWidth) { >+ rx = halfWidth; >+ } >+ if (ry > halfHeight) { >+ ry = halfHeight; >+ } Simplify to: rx = std::min(rx, halfWidth); ry = std::min(ry, halfHeight); >+++ b/content/svg/content/src/nsSVGPathGeometryElement.cpp >+TemporaryRef<PathBuilder> >+nsSVGPathGeometryElement::CreatePathBuilder() >+{ [...] >+ // The fill rule only matters for painting, so it's okay to pass the computed >+ // value of our 'fill-rule' property here regardless of who's calling us or >+ // what they're going to do with the path that we return (e.g. paint, >+ // hit-test, calculate various bounds, measure, etc.). This comment needs tweaking as well (since the fill rule matters for hit-testing as well - not only painting). >+nsSVGPathGeometryElement::GetFillRule() >+{ >+ FillRule fillRule = FILL_WINDING; [...] >+ if (styleContext) { >+ MOZ_ASSERT(styleContext->StyleSVG()->mFillRule == >+ NS_STYLE_FILL_RULE_NONZERO || >+ styleContext->StyleSVG()->mFillRule == >+ NS_STYLE_FILL_RULE_EVENODD); >+ >+ if (styleContext->StyleSVG()->mFillRule == NS_STYLE_FILL_RULE_EVENODD) { >+ fillRule = FILL_EVEN_ODD; I take it WINDING is equivalent to NONZERO? (Please add a comment here to mention that, since otherwise it looks kinda like NONZERO gets dropped on the floor) >+Float >+nsSVGPathGeometryElement::GetStrokeWidth() >+{ [...] >+ return styleContext ? >+ SVGContentUtils::CoordToFloat(styleContext->PresContext(), this, >+ styleContext->StyleSVG()->mStrokeWidth) : >+ 0; s/0/0.0/ >+++ b/content/svg/content/src/nsSVGPathGeometryElement.h >+ /** >+ * Returns the current computed value for the CSS 'fill-rule' property for >+ * this element. >+ */ >+ FillRule GetFillRule(); >+ >+ /** >+ * Returns the current stroke width of this element. (Note: this does not >+ * take account of the value of the 'stroke' and 'stroke-opacity' >+ * properties.) >+ */ >+ Float GetStrokeWidth(); s/current stroke width/current computed value for the CSS 'stroke-width' property/ (for clarity & consistency w/ the GetFillRule() documentation) I'm not sure the parenthetical is necessary, but I'm fine with keeping it if you think it's worthwhile. >+++ b/content/svg/content/src/nsSVGPolyElement.cpp >+nsSVGPolyElement::BuildPath() >+{ >+ RefPtr<PathBuilder> pathBuilder = CreatePathBuilder(); >+ >+ const SVGPointList &points = mPoints.GetAnimValue(); >+ >+ if (!points.IsEmpty()) { >+ pathBuilder->MoveTo(ToPoint(points[0])); >+ for (uint32_t i = 1; i < points.Length(); ++i) { >+ pathBuilder->LineTo(ToPoint(points[i])); Don't use ToPoint() at all here. Instead, add an "operator Point()" to SVGPoint, here: http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/SVGPoint.h#56 (w/ current patch, this code patch is starting with a SVGPoint, converting it to a gfxPoint (via operator gfxPoint), and converting *that* to a Point. Might as well skip the gfxPoint intermediary). Also: With that changed, I bet you can drop the gfx2DGlue.h include that you're currently adding here. r=me with the above addressed

(In reply to Daniel Holbert [:dholbert] from comment #8) > Does this break down if aStrokeWidth is 0? (I forget -- are line caps not > supposed to draw in that case? (are they sized based on stroke-width?)) It's based on stroke-width, so no, it's fine. Comment added.

SVGImageElement::BuildPath() seems wrong, we only do something if the width or height <= 0 shouldn't that be the opposite way round?

Good catch (& sorry for missing that)! rs=me on pushing a followup to fix that. (We should check width > 0 && height > 0, to match the equivalent SVGImageElement::ConstructPath functionality)

[needinfo=jwatt for that followup]

Yeah, I have a fix for that in one of my follow-up patches.

(In reply to Jonathan Watt [:jwatt] from comment #14) > Yeah, I have a fix for that in one of my follow-up patches. Specifically, it's fixed in bug 934156's patch.