Bug 138456 – [SVG2] Implement marker orient='auto-start-reverse'

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Nikos Andronikos]

The auto-start-reverse value extends the auto value for markers so that any start marker will point at 180 degrees to the direction of the path - other markers will point along the direction of the path (same behaviour as auto).
This allows arrow heads at each end of a line that both point outwards automatically.

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

This SVG 2 feature is stable and has been implemented in other browsers:
FF: https://bugzilla.mozilla.org/show_bug.cgi?id=879659
Chrome: https://code.google.com/p/chromium/issues/detail?id=367748

[Nikos Andronikos]

Created attachment 241093 [details]
Patch

[WebKit Commit Bot]

Attachment 241093 [details] did not pass style-queue:


ERROR: Source/WebCore/svg/SVGMarkerElement.h:116:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/svg/SVGMarkerElement.h:117:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 2 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nikos Andronikos]

The style warnings are on forward declarations of IDL types and they follow the SVG 2 naming scheme. I think the warnings should be ignored?

[Dirk Schulze]

Comment on attachment 241093 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=241093&action=review

IIRC Erik did the implementation for Blink. So I support the change in general. One comment inline and r- because of that. (Please fix style issue. Didn't check the issue yet.)

> Source/WebCore/svg/SVGMarkerElement.idl:36
> +    const unsigned short SVG_MARKER_ORIENT_AUTOSTARTREVERSE = 3;

I do not want to have it exposed to SVGDOM yet. Please make sure that we return UNKNOWN if the enum is autoreverse.

[Dirk Schulze]

(In reply to comment #4)
> Comment on attachment 241093 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=241093&action=review
> 
> IIRC Erik did the implementation for Blink. So I support the change in
> general. One comment inline and r- because of that. (Please fix style issue.
> Didn't check the issue yet.)
> 
> > Source/WebCore/svg/SVGMarkerElement.idl:36
> > +    const unsigned short SVG_MARKER_ORIENT_AUTOSTARTREVERSE = 3;
> 
> I do not want to have it exposed to SVGDOM yet. Please make sure that we
> return UNKNOWN if the enum is autoreverse.

Oh, and in the consequence we should not support setting the enum to auto-start-reverse of course.

[Dirk Schulze]

(In reply to comment #3)
> The style warnings are on forward declarations of IDL types and they follow
> the SVG 2 naming scheme. I think the warnings should be ignored?

Yes, ignore them for this enum.

[Nikos Andronikos]

Created attachment 269417 [details]
Patch

[WebKit Commit Bot]

Attachment 269417 [details] did not pass style-queue:


ERROR: Source/WebCore/svg/SVGMarkerElement.h:119:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/svg/SVGMarkerElement.h:120:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 2 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 269417 [details]
Patch

Attachment 269417 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/719836

New failing tests:
imported/mozilla/svg/path-03.svg
svg/clip-path/clip-path-on-marker-003.svg
imported/mozilla/svg/dynamic-marker-02.svg
svg/custom/marker-zero-length-linecaps.svg
svg/animations/additive-type-by-animation.html
svg/animations/svgangle-animation-grad-to-deg.html
svg/animations/svgangle-animation-deg-to-grad.html
svg/dom/path-marker-removed-crash.svg
svg/custom/non-scaling-stroke-markers.svg
imported/mozilla/svg/marker-orientation-01.svg
imported/mozilla/svg/dynamic-marker-03.svg
svg/custom/marker-orient-auto.html
svg/custom/painting-marker-07-f-inherit.svg
svg/animations/svgangle-animation-deg-to-rad.html
svg/custom/marker-empty-path.svg
svg/custom/marker-referencePoint.svg
svg/custom/bug86392.html
svg/W3C-SVG-1.1-SE/painting-marker-07-f.svg
svg/animations/svgangle-animation-rad-to-deg.html
svg/animations/svgangle-animation-rad-to-grad.html
svg/animations/animate-marker-orient-from-angle-to-autostartreverse.html
svg/animations/svgangle-animation-grad-to-rad.html

[Build Bot]

Created attachment 269420 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 269417 [details]
Patch

Attachment 269417 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/719856

New failing tests:
imported/mozilla/svg/path-03.svg
svg/clip-path/clip-path-on-marker-003.svg
imported/mozilla/svg/dynamic-marker-02.svg
svg/custom/marker-zero-length-linecaps.svg
svg/animations/additive-type-by-animation.html
svg/animations/svgangle-animation-grad-to-deg.html
svg/animations/svgangle-animation-deg-to-grad.html
svg/dom/path-marker-removed-crash.svg
svg/custom/non-scaling-stroke-markers.svg
imported/mozilla/svg/marker-orientation-01.svg
imported/mozilla/svg/dynamic-marker-03.svg
svg/custom/marker-orient-auto.html
svg/custom/painting-marker-07-f-inherit.svg
svg/animations/svgangle-animation-deg-to-rad.html
svg/custom/marker-empty-path.svg
svg/custom/marker-referencePoint.svg
svg/custom/bug86392.html
svg/W3C-SVG-1.1-SE/painting-marker-07-f.svg
svg/animations/svgangle-animation-rad-to-deg.html
svg/animations/svgangle-animation-rad-to-grad.html
svg/animations/animate-marker-orient-from-angle-to-autostartreverse.html
svg/animations/svgangle-animation-grad-to-rad.html

[Build Bot]

Created attachment 269422 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 269417 [details]
Patch

Attachment 269417 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/719907

New failing tests:
imported/mozilla/svg/path-03.svg
svg/clip-path/clip-path-on-marker-003.svg
imported/mozilla/svg/dynamic-marker-02.svg
svg/custom/marker-zero-length-linecaps.svg
svg/animations/additive-type-by-animation.html
svg/animations/svgangle-animation-grad-to-deg.html
svg/animations/svgangle-animation-deg-to-grad.html
svg/dom/path-marker-removed-crash.svg
svg/custom/non-scaling-stroke-markers.svg
imported/mozilla/svg/marker-orientation-01.svg
imported/mozilla/svg/dynamic-marker-03.svg
svg/custom/marker-orient-auto.html
svg/custom/painting-marker-07-f-inherit.svg
svg/animations/svgangle-animation-deg-to-rad.html
svg/custom/marker-empty-path.svg
svg/custom/marker-referencePoint.svg
svg/custom/bug86392.html
svg/W3C-SVG-1.1-SE/painting-marker-07-f.svg
svg/animations/svgangle-animation-rad-to-deg.html
svg/animations/svgangle-animation-rad-to-grad.html
svg/animations/animate-marker-orient-from-angle-to-autostartreverse.html
svg/animations/svgangle-animation-grad-to-rad.html

[Build Bot]

Created attachment 269424 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Nikos Andronikos]

Created attachment 269631 [details]
Patch

[Nikos Andronikos]

Created attachment 269662 [details]
Patch

[WebKit Commit Bot]

Attachment 269662 [details] did not pass style-queue:


ERROR: Source/WebCore/svg/SVGMarkerElement.h:119:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/svg/SVGMarkerElement.h:120:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 2 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 269662 [details]
Patch

Attachment 269662 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/728876

New failing tests:
svg/animations/animate-marker-orienttype-4.html
svg/animations/animate-marker-orient-from-angle-to-autostartreverse.html

[Build Bot]

Created attachment 269663 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 269662 [details]
Patch

Attachment 269662 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/728863

New failing tests:
svg/animations/animate-marker-orienttype-4.html
svg/animations/animate-marker-orient-from-angle-to-autostartreverse.html

[Build Bot]

Created attachment 269664 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 269662 [details]
Patch

Attachment 269662 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/728906

New failing tests:
svg/animations/animate-marker-orienttype-4.html
svg/animations/animate-marker-orient-from-angle-to-autostartreverse.html

[Build Bot]

Created attachment 269665 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Nikos Andronikos]

Created attachment 269721 [details]
Patch

[WebKit Commit Bot]

Attachment 269721 [details] did not pass style-queue:


ERROR: Source/WebCore/svg/SVGMarkerElement.h:119:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/svg/SVGMarkerElement.h:120:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 2 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 269721 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=269721&action=review

review+ but I saw enough errors that you may want to correct that I am not doing commit-queue+

> Source/WebCore/ChangeLog:11
> +        Reviewed by NOBODY (OOPS!).

Should have a blank line before this.

> Source/WebCore/rendering/svg/SVGResources.cpp:513
> +    m_markerData->reverseStart =  markerStart->markerElement().orientType() == SVGMarkerOrientAutoStartReverse;

Extra space here after the "=".

> Source/WebCore/rendering/svg/SVGResources.h:55
> +    bool markerReverseStart() { return m_markerData ? m_markerData->reverseStart : false; }

Should be marked const.

> Source/WebCore/rendering/svg/SVGResources.h:138
> +        bool reverseStart;

Should just be:

    bool reverseStart { false };

Then we would not need to initialize in the constructor above.

Later someone should do the same with the marker pointers, using nullptr, and then remove the constructor completely.

> Source/WebCore/svg/SVGMarkerElement.cpp:192
> +void SVGMarkerElement::setOrient(const SVGMarkerOrientType orientType, const SVGAngle& angle)

Should just be SVGMarkerOrientType, not "const SVGMarkerOrientType".

> Source/WebCore/svg/SVGMarkerElement.cpp:206
> +    setOrient(SVGMarkerOrientAutoStartReverse, SVGAngle());

Could use { } instead of SVGAngle().

> Source/WebCore/svg/SVGMarkerElement.cpp:211
> +    setOrient(SVGMarkerOrientAuto, SVGAngle());

Ditto.

> Source/WebCore/svg/SVGMarkerElement.h:95
>          if (value == "auto")
>              return SVGMarkerOrientAuto;
> +        if (value == "auto-start-reverse")
> +            return SVGMarkerOrientAutoStartReverse;

Are these intentionally case sensitive? Can we add a test case covering that?

> Source/WebCore/svg/SVGMarkerElement.h:127
> +    void setOrientToAutoStartReverse();

I wasn’t able to find the code that calls this. Did you intend to modify the IDL file to add this function?

> Source/WebCore/svg/SVGMarkerElement.h:148
> +    void setOrient(const SVGMarkerOrientType, const SVGAngle&);

Should just be SVGMarkerOrientType, not const SVGMarkerOrientType.

> LayoutTests/svg/animations/animate-marker-orienttype-4-expected.txt:1
> +layer at (0,0) size 800x600

Can you investigate making this a reference test instead of a render tree dump? Is there some reason you were not able to do that?

[Nikos Andronikos]

Created attachment 270968 [details]
Patch

[WebKit Commit Bot]

Attachment 270968 [details] did not pass style-queue:


ERROR: Source/WebCore/svg/SVGMarkerElement.h:119:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/svg/SVGMarkerElement.h:120:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 2 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nikos Andronikos]

(In reply to comment #26)
> Comment on attachment 269721 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=269721&action=review
> 
> review+ but I saw enough errors that you may want to correct that I am not
> doing commit-queue+

Thanks, have addressed all your comments. A couple of replies inline below.
I also found an issue with animating from auto to auto-start-reverse which I've now resolved.

> 
> > Source/WebCore/ChangeLog:11
> > +        Reviewed by NOBODY (OOPS!).
> 
> Should have a blank line before this.
> 
> > Source/WebCore/rendering/svg/SVGResources.cpp:513
> > +    m_markerData->reverseStart =  markerStart->markerElement().orientType() == SVGMarkerOrientAutoStartReverse;
> 
> Extra space here after the "=".
> 
> > Source/WebCore/rendering/svg/SVGResources.h:55
> > +    bool markerReverseStart() { return m_markerData ? m_markerData->reverseStart : false; }
> 
> Should be marked const.
> 
> > Source/WebCore/rendering/svg/SVGResources.h:138
> > +        bool reverseStart;
> 
> Should just be:
> 
>     bool reverseStart { false };
> 
> Then we would not need to initialize in the constructor above.
> 
> Later someone should do the same with the marker pointers, using nullptr,
> and then remove the constructor completely.
> 
> > Source/WebCore/svg/SVGMarkerElement.cpp:192
> > +void SVGMarkerElement::setOrient(const SVGMarkerOrientType orientType, const SVGAngle& angle)
> 
> Should just be SVGMarkerOrientType, not "const SVGMarkerOrientType".
> 
> > Source/WebCore/svg/SVGMarkerElement.cpp:206
> > +    setOrient(SVGMarkerOrientAutoStartReverse, SVGAngle());
> 
> Could use { } instead of SVGAngle().
> 
> > Source/WebCore/svg/SVGMarkerElement.cpp:211
> > +    setOrient(SVGMarkerOrientAuto, SVGAngle());
> 
> Ditto.
> 
> > Source/WebCore/svg/SVGMarkerElement.h:95
> >          if (value == "auto")
> >              return SVGMarkerOrientAuto;
> > +        if (value == "auto-start-reverse")
> > +            return SVGMarkerOrientAutoStartReverse;
> 
> Are these intentionally case sensitive? Can we add a test case covering that?

Yes they are. Test included in SVGAnimatedEnumeration-SVGMarkerElement.html

> 
> > Source/WebCore/svg/SVGMarkerElement.h:127
> > +    void setOrientToAutoStartReverse();
> 
> I wasn’t able to find the code that calls this. Did you intend to modify the
> IDL file to add this function?

This method wasn't added to the IDL for SVG 2 so I've removed it from the c++ source as well.

> 
> > Source/WebCore/svg/SVGMarkerElement.h:148
> > +    void setOrient(const SVGMarkerOrientType, const SVGAngle&);
> 
> Should just be SVGMarkerOrientType, not const SVGMarkerOrientType.
> 
> > LayoutTests/svg/animations/animate-marker-orienttype-4-expected.txt:1
> > +layer at (0,0) size 800x600
> 
> Can you investigate making this a reference test instead of a render tree
> dump? Is there some reason you were not able to do that?

[Build Bot]

Comment on attachment 270968 [details]
Patch

Attachment 270968 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/807737

New failing tests:
svg/animations/animate-marker-orienttype-4.html

[Build Bot]

Created attachment 270971 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 270968 [details]
Patch

Attachment 270968 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/807740

New failing tests:
svg/animations/animate-marker-orienttype-4.html

[Build Bot]

Created attachment 270972 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 270968 [details]
Patch

Attachment 270968 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/807733

New failing tests:
svg/animations/animate-marker-orienttype-4.html

[Build Bot]

Created attachment 270973 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Nikos Andronikos]

Created attachment 271114 [details]
Patch

[WebKit Commit Bot]

Attachment 271114 [details] did not pass style-queue:


ERROR: Source/WebCore/svg/SVGMarkerElement.h:119:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/svg/SVGMarkerElement.h:120:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 2 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nikos Andronikos]

I've marked this r? again because I discovered and fixed a bug in my code, and then came up against another bug in the synchronization code for the orient attribute (filed as bug 154141).
The changes from when this was marked r+ aren't hugely significant, but I thought it would be worth having another quick look over.

[Said Abou-Hallawa]

Comment on attachment 271114 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=271114&action=review

Unofficial r=me.

> Source/WebCore/ChangeLog:10
> +        the orient attribute of the marker is set to 'auto-start'reverse'.

missing dash in auto-start-reverse

> Source/WebCore/rendering/svg/SVGMarkerData.h:51
> +    SVGMarkerData(Vector<MarkerPosition>& positions, bool reverseStart)

Should not we use enum instead?

SVGMarkerData(Vector<MarkerPosition>& positions, SVGMarkerOrientType startMarkerIrient)

> Source/WebCore/rendering/svg/SVGMarkerData.h:91
> +            if (m_reverseStart)

I think if enum is used, this if statement will be clearer:
    if (m_ startMarkerIrient == SVGMarkerOrientAutoStartReverse)

> Source/WebCore/rendering/svg/SVGResources.cpp:308
> +bool SVGResources::markerReverseStart() const

Should not this function return an enum instead?

SVGMarkerOrientType SVGResources::markerStartOrient() const
{
    return markerStart() ? markerStart()->markerElement().orientType() : SVGMarkerOrientUnknown;
}

> Source/WebCore/svg/SVGAnimatedAngle.cpp:106
> +            animatedAngleAndEnumeration.first.setValue(0);

I like early returns too but I do not like this one since it does not add much clarity. Both the then-block and the else-block are just one line.

> Source/WebCore/svg/SVGAnimatedAngle.cpp:114
> +        animatedAngleAndEnumeration.first.setValue(0);

Ditto.

> Source/WebCore/svg/SVGAnimatedAngle.cpp:119
> +    if (fromAngleAndEnumeration.second == SVGMarkerOrientAuto || fromAngleAndEnumeration.second == SVGMarkerOrientAutoStartReverse) {

Should not this if-statment include also SVGMarkerOrientUnknown so the following if-statement can be deleted:

if (fromAngleAndEnumeration.second >= SVGMarkerOrientUnknown && fromAngleAndEnumeration.second  < SVGMarkerOrientMax) {
    animatedAngleAndEnumeration.first.setValue(0);
    animatedAngleAndEnumeration.second = fromAngleAndEnumeration.second;
    return;
}

> Source/WebCore/svg/SVGMarkerElement.cpp:212
>  }

Should not these inline function be moved to the header?

> Source/WebCore/svg/SVGMarkerElement.cpp:245
> +        return;

I think there is no need for this last return;

> LayoutTests/svg/animations/animate-marker-orienttype-4-expected.html:1
> +<!doctype html>

Is there a reason to have this test not an .svg file?

> LayoutTests/svg/custom/marker-auto-start-reverse.html:1
> +<!doctype html>

Is there a reason to have this test not an .svg file?

[Said Abou-Hallawa]

Comment on attachment 271114 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=271114&action=review

>> Source/WebCore/svg/SVGAnimatedAngle.cpp:119
>> +    if (fromAngleAndEnumeration.second == SVGMarkerOrientAuto || fromAngleAndEnumeration.second == SVGMarkerOrientAutoStartReverse) {
> 
> Should not this if-statment include also SVGMarkerOrientUnknown so the following if-statement can be deleted:
> 
> if (fromAngleAndEnumeration.second >= SVGMarkerOrientUnknown && fromAngleAndEnumeration.second  < SVGMarkerOrientMax) {
>     animatedAngleAndEnumeration.first.setValue(0);
>     animatedAngleAndEnumeration.second = fromAngleAndEnumeration.second;
>     return;
> }

Sorry I just realized that this check should exclude SVGMarkerOrientAngle. So the if-statment can be written like this:

if (fromAngleAndEnumeration.second == SVGMarkerOrientUnknown
    || fromAngleAndEnumeration.second == SVGMarkerOrientAuto
    || fromAngleAndEnumeration.second == SVGMarkerOrientAutoStartReverse) {
    ...
}

[Nikos Andronikos]

Comment on attachment 271114 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=271114&action=review

>> Source/WebCore/rendering/svg/SVGMarkerData.h:51
>> +    SVGMarkerData(Vector<MarkerPosition>& positions, bool reverseStart)
> 
> Should not we use enum instead?
> 
> SVGMarkerData(Vector<MarkerPosition>& positions, SVGMarkerOrientType startMarkerIrient)

Using an enum is a valid alternative, but I don't really think it's an improvement. We don't need the additional information the other enum values would offer so why not just restrict it to the two cases?

>> Source/WebCore/svg/SVGAnimatedAngle.cpp:106
>> +            animatedAngleAndEnumeration.first.setValue(0);
> 
> I like early returns too but I do not like this one since it does not add much clarity. Both the then-block and the else-block are just one line.

I actually agree with you here. I've had previous reviews where I was told to always return early so I went with that. Happy to change though =)

>>> Source/WebCore/svg/SVGAnimatedAngle.cpp:119
>>> +    if (fromAngleAndEnumeration.second == SVGMarkerOrientAuto || fromAngleAndEnumeration.second == SVGMarkerOrientAutoStartReverse) {
>> 
>> Should not this if-statment include also SVGMarkerOrientUnknown so the following if-statement can be deleted:
>> 
>> if (fromAngleAndEnumeration.second >= SVGMarkerOrientUnknown && fromAngleAndEnumeration.second  < SVGMarkerOrientMax) {
>>     animatedAngleAndEnumeration.first.setValue(0);
>>     animatedAngleAndEnumeration.second = fromAngleAndEnumeration.second;
>>     return;
>> }
> 
> Sorry I just realized that this check should exclude SVGMarkerOrientAngle. So the if-statment can be written like this:
> 
> if (fromAngleAndEnumeration.second == SVGMarkerOrientUnknown
>     || fromAngleAndEnumeration.second == SVGMarkerOrientAuto
>     || fromAngleAndEnumeration.second == SVGMarkerOrientAutoStartReverse) {
>     ...
> }

There's a subtle difference there as before using != SVGMarkerOrientAngle was a catch all for any value (e.g. anything outside the correct range would end up being made unknown). I doubt this is that important in practice. But I've tried to maintain that behaviour. So I've made some changes based on your comments, just not exactly what you suggested.

>> Source/WebCore/svg/SVGMarkerElement.cpp:212
>>  }
> 
> Should not these inline function be moved to the header?

Are you saying that these should be inlined and put in the header? Are there any project guidelines around making inlines? My preference here would be to not make these inline because of the nature of the function (they're mapped to methods in the IDL and not likely to be called repeatedly).

>> LayoutTests/svg/animations/animate-marker-orienttype-4-expected.html:1
>> +<!doctype html>
> 
> Is there a reason to have this test not an .svg file?

Not for this particular file. In general I prefer using svg in html because of the restrictions on XML (e.g. CDATA).

[Nikos Andronikos]

Created attachment 272200 [details]
Patch

[WebKit Commit Bot]

Attachment 272200 [details] did not pass style-queue:


ERROR: Source/WebCore/svg/SVGMarkerElement.h:119:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/svg/SVGMarkerElement.h:120:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 2 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 272200 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=272200&action=review

> Source/WebCore/rendering/svg/SVGResources.cpp:313
> +    if (m_markerData && m_markerData->markerStart)
> +        return m_markerData->markerStart->markerElement().orientType() == SVGMarkerOrientAutoStartReverse;
> +
> +    return false;

I would have written this as a single return statement:

    return m_markerData
        && m_markerData->markerStart
        && m_markerData->markerStart->markerElement().orientType() == SVGMarkerOrientAutoStartReverse;

> Source/WebCore/svg/SVGAnimatedAngle.cpp:117
> +        // Regular from angle to angle animation, with all features like additive etc.

You just moved this comment, but "all features like additive etc." is not good grammar.

> Source/WebCore/svg/SVGMarkerElement.cpp:199
>      // Mark orientAttr dirty - the next XML DOM access of that attribute kicks in synchronization.
>      m_orientAngle.shouldSynchronize = true;
>      m_orientType.shouldSynchronize = true;

Doesn’t seem right that this invalidation work (and what follows) is unconditional, even if the new value is the same as the old.

> Source/WebCore/svg/SVGMarkerElement.cpp:206
> +    setOrient(SVGMarkerOrientAuto, SVGAngle { });

Do you actually need to specify SVGAngle here? I think this works:

    setOrient(SVGMarkerOrientAuto, { });

[Nikos Andronikos]

(In reply to comment #44)
> Comment on attachment 272200 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=272200&action=review
> 
> > Source/WebCore/rendering/svg/SVGResources.cpp:313
> > +    if (m_markerData && m_markerData->markerStart)
> > +        return m_markerData->markerStart->markerElement().orientType() == SVGMarkerOrientAutoStartReverse;
> > +
> > +    return false;
> 
> I would have written this as a single return statement:
> 
>     return m_markerData
>         && m_markerData->markerStart
>         && m_markerData->markerStart->markerElement().orientType() ==
> SVGMarkerOrientAutoStartReverse;
> 
> > Source/WebCore/svg/SVGAnimatedAngle.cpp:117
> > +        // Regular from angle to angle animation, with all features like additive etc.
> 
> You just moved this comment, but "all features like additive etc." is not
> good grammar.
> 
> > Source/WebCore/svg/SVGMarkerElement.cpp:199
> >      // Mark orientAttr dirty - the next XML DOM access of that attribute kicks in synchronization.
> >      m_orientAngle.shouldSynchronize = true;
> >      m_orientType.shouldSynchronize = true;
> 
> Doesn’t seem right that this invalidation work (and what follows) is
> unconditional, even if the new value is the same as the old.

There's definitely some broken behaviour with the synchronization of the orient attribute. 
A couple of weeks ago while developing tests for this patch, I noticed some bugs and filed bug 154141. I intend to look at that bug this week or next.

> 
> > Source/WebCore/svg/SVGMarkerElement.cpp:206
> > +    setOrient(SVGMarkerOrientAuto, SVGAngle { });
> 
> Do you actually need to specify SVGAngle here? I think this works:
> 
>     setOrient(SVGMarkerOrientAuto, { });

That does work, but 'SVGAngle { }' seemed more expressive to me. I take it you'd prefer just '{ }'?

[Nikos Andronikos]

Created attachment 272590 [details]
Patch

[WebKit Commit Bot]

Attachment 272590 [details] did not pass style-queue:


ERROR: Source/WebCore/svg/SVGMarkerElement.h:119:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/svg/SVGMarkerElement.h:120:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 2 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 272590 [details]
Patch

Clearing flags on attachment: 272590

Committed r197738: <http://trac.webkit.org/changeset/197738>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.