Bug 1239100 implemented the SVGGeometryElement interface from the SVG 2 spec. for path elements. Support for elements other than path still needs to be added. Sebastian

This patch is kind of doing two things at once: (1) A non-functional refactoring (tweaking the SVGGeometryElement::GetOrBuildPath(...) API to take a const pointer rather than a const reference and updating all the callsites). (2) A mix of functional changes, to actually support this interface on these elements. Would you mind splitting this into two separate patches, so the non-functional s/reference/pointer/ refactoring isn't mixed together with the functional changes?

Note that part of what was here is now split off into bug 1443685. That bug will also need to land before we can pass SVGGeometryElement-rect.svg

Comment on attachment 8956670 [details] [diff] [review] part 1 - use reference rather than pointer when all callers have a pointer Review of attachment 8956670 [details] [diff] [review]: ----------------------------------------------------------------- The commit message (which I'm inferring from the attachment title) is a bit vague right now. Specifically: - It should probably mention SVGGeometryElement::GetOrBuildPath, because that is the one API that is being modified in this patch. - I'm not sure what it means by "when all callers have a pointer". (Of course all callers have a pointer - that's necessarily true, for any API that takes a pointer-valued arg.) Maybe you meant to say non-null pointer here? (I'm willing to believe that's true, though it's also not superficially clear that all callers are enforcing/verifying that themselves. E.g. SVGGeometryFrame::Render() passes in the result of a function called "GetDrawTarget" which sounds fallible. Though looking at gfxContext.h/cpp, it looks like it can typically be expected to return true.) Maybe commit message should be something like: Bug 1325320: Change SVGGeometryElement::GetOrBuildPath() to take DrawTarget as a reference (not pointer)" ...and then in a second line, some minor justification like the following: Its implementation already depends on this arg being non-null, so we might as well be consistent with some other DrawTarget-handling code and use a reference.

> (In reply to Daniel Holbert [:dholbert] from comment #6) > E.g. SVGGeometryFrame::Render() passes in the result of a > function called "GetDrawTarget" which sounds fallible. Though looking at > gfxContext.h/cpp, it looks like it can typically be expected to return true.) (er, I meant "non-null" rather than "true" there, of course)

Comment on attachment 8956674 [details] [diff] [review] part 2 - change interface and implement GetOrBuildPath for all geometry elements Review of attachment 8956674 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/svg/SVGGeometryElement.cpp @@ +128,5 @@ > + RefPtr<DrawTarget> drawTarget = > + gfxPlatform::GetPlatform()->ScreenReferenceDrawTarget(); > + FillRule fillRule = mCachedPath ? > + mCachedPath->GetFillRule() : FillRule::FILL_WINDING; > + return GetOrBuildPath(drawTarget, fillRule); For the "no cached path yet" case here: is it bad that we're just using FILL_WINDING? (Should we be looking at our "fill-rule" property and deferring to that, or does it not matter?) Note that GetOrBuildPath does end up *setting* mCachedPath in this scenario, so if other APIs may come along and use a different fill-rule (and potentially get incorrect results or make us throw away our cached path), that might be bad from an either correctness or perf perspective. So maybe better to use the correct fill-rule up front? Anyway -- even if this is fine for some reason, it probably merits a brief explanatory/justifying code-comment here.

(Thanks for splitting this up, BTW -- much easier to reason about like this.)

(In reply to Daniel Holbert [:dholbert] from comment #6) > - I'm not sure what it means by "when all callers have a pointer". (Of I meant that in the old API all callers started out with a pointer to a drawTarget which they converted using * before calling the API which took a reference. drawTarget is generally obtained as a pointer and then on the whole passed around as one.

OK, gotcha. (I misread that part as a partial justification for the s/pointer/reference/ API conversion.)

Pushed by longsonr@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2bd1e1288722 change SVGGeometryElement::GetOrBuildPath to take DrawTarget as a pointer since all its callers have drawTarget as a pointer themselves.

I've changed it to call GetFillRule() instead.

Comment on attachment 8957484 [details] [diff] [review] part 2 - change interface and implement GetOrBuildPath for all geometry elements Review of attachment 8957484 [details] [diff] [review]: ----------------------------------------------------------------- OK, thanks. r=me

With this (and part 2) we pass these tests: https://github.com/w3c/web-platform-tests/pull/9408/files

Comment on attachment 8957484 [details] [diff] [review] part 2 - change interface and implement GetOrBuildPath for all geometry elements Needs a DOM peer review. In SVG 2 there is a new SVGGeometryElement interface and all basic shapes such as rect should derive from that rather than SVGGraphicsElement. E.g. https://www.w3.org/TR/SVG2/shapes.html#InterfaceSVGRectElement Note that we already have the SVGGeometryElement interface in the tree, path elements implement it so this is just for the rest of the SVG shapes.

Comment on attachment 8957967 [details] [diff] [review] part 3 - move pathLength code to SVGGeometry element Review of attachment 8957967 [details] [diff] [review]: ----------------------------------------------------------------- r=me, one nit: ::: dom/svg/SVGGeometryElement.h @@ +56,4 @@ > const nsAttrValue* aOldValue, > nsIPrincipal* aSubjectPrincipal, > bool aNotify) override; > + virtual bool IsNodeOfType(uint32_t aFlags) const override; Drop the "virtual" keyword here -- it's implied by "override" and coding style says to use at most one of these keywords. ( https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Methods ) (Existing code doesn't follow this particularly well yet, but we're getting there -- see e.g. bug 1436263 and bug 1428535.)

Comment on attachment 8957968 [details] [diff] [review] Additional reftest for part 3 Review of attachment 8957968 [details] [diff] [review]: ----------------------------------------------------------------- r=me, one nit: ::: layout/reftests/svg/textPath-line-01.svg @@ +7,5 @@ > + > + <defs> > + <line id="x" x1="100" y1="100" x2="500" y2="100"/> > + </defs> > + nix whitespace on blank line there

Pushed by longsonr@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e18f1f40d080 part 2 - change shapes so they implement SVGGeometryElement and implement SVGGeometryElement::GetOrBuildPath r=dholbert r=mystor (DOM Peer)

Pushed by longsonr@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e8276eda78b8 part 3 - move GetPathLengthScale to SVGGeometryElement and convert callers of SVGPathElement methods to work on the base SVGGeometryElement class and work for all shapes, not just paths r=dholbert

Pushed by longsonr@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/788a11631abd part 4 - additional reftest for textPath pointing to a line r=dholbert

https://hg.mozilla.org/mozilla-central/rev/e18f1f40d080 https://hg.mozilla.org/mozilla-central/rev/e8276eda78b8 https://hg.mozilla.org/mozilla-central/rev/788a11631abd

Updated: https://developer.mozilla.org/en-US/docs/Web/API/SVGGeometryElement#Browser_compatibility https://developer.mozilla.org/en-US/Firefox/Releases/61#SVG https://developer.mozilla.org/en-US/docs/Web/SVG/SVG_2_support_in_Mozilla

I think there's a bit more "dev-doc-needed" here, actually --> adding back keyword. In particular, the MDN page https://developer.mozilla.org/en-US/docs/Web/API/SVGPathElement/getTotalLength has a reference to this very bug, saying: > This method is currently only supported on <path> elements > (where it was already supported in earlier Firefox versions > through the SVGPathElement interface). Support for other elements > will be added in bug 1325320. The future tense "will be" is stale now that this bug has made it to release (in version 61). Also, the compat table on that page incorrectly (?) says that this API *stopped being supported* in Firefox 53, whereas https://developer.mozilla.org/en-US/docs/Web/API/SVGGeometryElement says it started being supported in Firefox 53. I'm not sure that makes sense (maybe this is trying to capture the fact that it moved from one interface to another in version 53? But I don't think so, because both pages have this API marked as "supported" in Chrome.)

https://developer.mozilla.org/docs/Web/API/SVGPathElement/getPointAtLength has the same problem, BTW (it's got a link to this bug saying that "support for other elements will be added" in this bug.) I think https://github.com/mdn/browser-compat-data/blob/master/api/SVGPathElement.json is what we need to update to address these issues, but I'm not entirely sure what the correct update to make here is. It seems like: (1) the notes pointing to this bug want to be removed (2) I'm not sure if that JSON file page should be updated to indicate that the API *is supported* in Firefox, vs. the API is not-supported in other browsers (e.g. Chrome, which like Firefox supports these APIs via inheriting it from the SVGGeometryElement interface). i.e. it's listed here: https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/dom/webidl/SVGGeometryElement.webidl#17 https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/svg/svg_geometry_element.idl?l=39 ...but not here: https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/dom/webidl/SVGPathElement.webidl https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/svg/svg_path_element.idl The correct solution for (2) depends on the semantics of these compat tables in MDN, I guess, so I'm not submitting a pull request myself because I don't fully understand what the correct way to capture this "supported via a superclass/super-interface" data is in MDN.

Hi Daniel, I've submitted a new PR to our BCD repo to try and clear up this confusion. In the notes on those methods, I've: * Removed references to this bug. It is now supported on other types, as of Fx61, and web developers don't really care about this kind of historical detail. * Made it clear what the relationship is between the two interfaces (i.e. the inheritance) * Removed the notion that they stopped being supported on SVGPathElement — they are still available, but are now specified on its parent. Let me know if this clears things up OK. Thanks!

Thanks, Chris! That sounds great.