[SVG2] Add support for marker orient="auto-start-reverse".

[SVG2] Add support for marker orient="auto-start-reverse".

Spec: https://svgwg.org/svg2-draft/painting.html#OrientAttribute

BUG=367748
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181732

[Erik Dahlström (inactive), 2014-09-08 14:29:26]

ed@opera.com changed reviewers:
+ fs@opera.com, pdr@chromium.org

[Erik Dahlström (inactive), 2014-09-08 14:29:27]

Mozilla added support in: https://bugzilla.mozilla.org/show_bug.cgi?id=879659

[fs, 2014-09-08 15:31:41]

https://codereview.chromium.org/539833004/diff/1/LayoutTests/svg/markers/mark...
File LayoutTests/svg/markers/marker-orientation-02-expected.svg (right):

https://codereview.chromium.org/539833004/diff/1/LayoutTests/svg/markers/mark...
LayoutTests/svg/markers/marker-orientation-02-expected.svg:8: arcs when
orient="auto-start-reverse-start-reverse" is used</title>
Nit: Again

https://codereview.chromium.org/539833004/diff/1/LayoutTests/svg/markers/mark...
File LayoutTests/svg/markers/marker-orientation-02.svg (right):

https://codereview.chromium.org/539833004/diff/1/LayoutTests/svg/markers/mark...
LayoutTests/svg/markers/marker-orientation-02.svg:7:
orient="auto-start-reverse-start-reverse" is used</title>
Nit: auto-start-reverse-start-reverse ???

https://codereview.chromium.org/539833004/diff/1/Source/core/svg/SVGAngle.cpp
File Source/core/svg/SVGAngle.cpp (right):

https://codereview.chromium.org/539833004/diff/1/Source/core/svg/SVGAngle.cpp...
Source/core/svg/SVGAngle.cpp:405: if (fromOrientType == SVGMarkerOrientAuto ||
fromOrientType == SVGMarkerOrientAutoStartReverse) {
What about 'auto' <-> 'auto-start-reverse'? (Looks like it'll end up here, but
always set the from-value.)

[Erik Dahlström (inactive), 2014-09-09 09:25:07]

https://codereview.chromium.org/539833004/diff/1/LayoutTests/svg/markers/mark...
File LayoutTests/svg/markers/marker-orientation-02-expected.svg (right):

https://codereview.chromium.org/539833004/diff/1/LayoutTests/svg/markers/mark...
LayoutTests/svg/markers/marker-orientation-02-expected.svg:8: arcs when
orient="auto-start-reverse-start-reverse" is used</title>
On 2014/09/08 15:31:41, fs wrote:
> Nit: Again

Done.

https://codereview.chromium.org/539833004/diff/1/LayoutTests/svg/markers/mark...
File LayoutTests/svg/markers/marker-orientation-02.svg (right):

https://codereview.chromium.org/539833004/diff/1/LayoutTests/svg/markers/mark...
LayoutTests/svg/markers/marker-orientation-02.svg:7:
orient="auto-start-reverse-start-reverse" is used</title>
On 2014/09/08 15:31:41, fs wrote:
> Nit: auto-start-reverse-start-reverse ???

Done.

https://codereview.chromium.org/539833004/diff/1/Source/core/svg/SVGAngle.cpp
File Source/core/svg/SVGAngle.cpp (right):

https://codereview.chromium.org/539833004/diff/1/Source/core/svg/SVGAngle.cpp...
Source/core/svg/SVGAngle.cpp:405: if (fromOrientType == SVGMarkerOrientAuto ||
fromOrientType == SVGMarkerOrientAutoStartReverse) {
On 2014/09/08 15:31:41, fs wrote:
> What about 'auto' <-> 'auto-start-reverse'? (Looks like it'll end up here, but
> always set the from-value.)

Done.

[fs, 2014-09-09 12:18:22]

https://codereview.chromium.org/539833004/diff/20001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGResourceMarker.cpp (right):

https://codereview.chromium.org/539833004/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGResourceMarker.cpp:128: markerAngle += 180;
I'm curious if this (adjustment) would be nicer in the SVGMarkerData-helper
(i.e. pass an auto-start-reverse flag to it, and then compute different
(auto-)angle in currentAngle(StartMarker) instead. That ought to cut
RenderSVGResourceMarker out of the patch entirely.

[Erik Dahlström (inactive), 2014-09-09 13:20:00]

On 2014/09/09 12:18:22, fs wrote:
>
https://codereview.chromium.org/539833004/diff/20001/Source/core/rendering/sv...
> File Source/core/rendering/svg/RenderSVGResourceMarker.cpp (right):
> 
>
https://codereview.chromium.org/539833004/diff/20001/Source/core/rendering/sv...
> Source/core/rendering/svg/RenderSVGResourceMarker.cpp:128: markerAngle += 180;
> I'm curious if this (adjustment) would be nicer in the SVGMarkerData-helper
> (i.e. pass an auto-start-reverse flag to it, and then compute different
> (auto-)angle in currentAngle(StartMarker) instead. That ought to cut
> RenderSVGResourceMarker out of the patch entirely.

See patchset #3.

[fs, 2014-09-09 14:24:49]

LGTM w/ nit

https://codereview.chromium.org/539833004/diff/40001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGShape.cpp (right):

https://codereview.chromium.org/539833004/diff/40001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGShape.cpp:456: if (!resources)
Nit: ASSERT-ing this should suffice (shouldGenerateMarkerPositions already
checks this).

[Erik Dahlström (inactive), 2014-09-10 14:19:06]

The CQ bit was checked by ed@opera.com

[commit-bot: I haz the power, 2014-09-10 14:19:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ed@opera.com/539833004/60001

[commit-bot: I haz the power, 2014-09-10 14:23:23]

Committed patchset #4 (id:60001) as 181732