At the F2F this week, we discussed a new value of <marker orient> that means the same as "auto", but for marker-start, means "auto" + 180deg. This allows a single arrowhead marker to be defined and used in marker-start and marker-end and for this to result in both arrowheads pointing out beyond the ends of the path. https://svgwg.org/svg2-draft/painting.html#OrientAttribute

There are a bunch of marker improvements in SVG 2, so let's put them all behind the one pref.

Comment on attachment 760615 [details] [diff] [review] Part 2: Refactor SVG marker handling to support more types of markers in the future. >+ float xmid = (x1 + x2) / 2; >+ float ymid = (y1 + y2) / 2; These variables aren't used. >+ >+ aMarks->AppendElement(nsSVGMark(x1, y1, angle, nsSVGMark::eStart)); >+ aMarks->AppendElement(nsSVGMark(x2, y2, angle, nsSVGMark::eEnd)); > } > > >+ // info on the segment marker for this command: >+ SVGPathTraversalState state; >+ state.mode = SVGPathTraversalState::eUpdateAll; eUpdateAll is the default so the second line is not required. More than that though, I can't see why this is required at all? Why create and keep the state variable up to date? You don't seem to be using it.

Sorry, I didn't do a good enough job splitting out the refactoring from the part that implements marker-segment (for bug 793613). I'll upload another try soon.

Comment on attachment 760365 [details] [diff] [review] Part 3: Implement <marker orient="auto-start-reverse">. Waiting for that other try...

Comment on attachment 760615 [details] [diff] [review] Part 2: Refactor SVG marker handling to support more types of markers in the future. ditto

> uint16_t GetBaseValue() const > { return mBaseVal; } >+ // we want to avoid exposing SVG_MARKER_ORIENT_AUTO_START_REVERSE to >+ // Web content > uint16_t GetAnimValue() const >+ { return mAnimVal == SVG_MARKER_ORIENT_AUTO_START_REVERSE ? >+ SVG_MARKER_ORIENT_UNKNOWN : mAnimVal; } Why didn't you modify GetBaseValue like you did GetAnimValue?

Although on the whole I'm not sure it's the greatest idea in the world to do this for GetAnimValue. I would have thought this would be rather unexpected for users of this new functionality.

(In reply to Robert Longson from comment #10) > Why didn't you modify GetBaseValue like you did GetAnimValue? An oversight. (In reply to Robert Longson from comment #11) > Although on the whole I'm not sure it's the greatest idea in the world to do > this for GetAnimValue. I would have thought this would be rather unexpected > for users of this new functionality. Yeah, I recently had second thoughts about that, and raised it on the mailing list (the general issue of avoiding new integer constants): http://lists.w3.org/Archives/Public/www-svg/2013Jul/0019.html Looks like Tab thinks we should go ahead with adding a non-integer-constant API for this. What do you think about implementing as in the patch now, and adding the string-based accessor when we get around to adding it to the spec? I don't know if too many people would use the SVG DOM interface for this attribute, anyway.

Fix a test problem.

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ba070286a470 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/89f980fdb567 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/583188626939

Follow fix for unused variable warning/error: https://hg.mozilla.org/integration/mozilla-inbound/rev/71de191da647

https://hg.mozilla.org/mozilla-central/rev/ba070286a470 https://hg.mozilla.org/mozilla-central/rev/89f980fdb567 https://hg.mozilla.org/mozilla-central/rev/583188626939 https://hg.mozilla.org/mozilla-central/rev/71de191da647