Bug 138456 - [SVG2] Implement marker orient='auto-start-reverse'
Summary: [SVG2] Implement marker orient='auto-start-reverse'
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nikos Andronikos
URL:
Keywords:
Depends on: 137942 152814
Blocks: 269477
  Show dependency treegraph
 
Reported: 2014-11-06 00:01 PST by Nikos Andronikos
Modified: 2024-02-15 11:23 PST (History)
5 users (show)

See Also:


Attachments
Patch (23.28 KB, patch)
2014-11-06 00:38 PST, Nikos Andronikos
no flags Details | Formatted Diff | Diff
Patch (24.50 KB, patch)
2016-01-20 19:40 PST, Nikos Andronikos
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (1.74 MB, application/zip)
2016-01-20 20:32 PST, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-yosemite (1.68 MB, application/zip)
2016-01-20 20:43 PST, Build Bot
no flags Details
Archive of layout-test-results from ews102 for mac-yosemite (1.64 MB, application/zip)
2016-01-20 20:51 PST, Build Bot
no flags Details
Patch (26.58 KB, patch)
2016-01-22 18:40 PST, Nikos Andronikos
no flags Details | Formatted Diff | Diff
Patch (26.95 KB, patch)
2016-01-23 02:38 PST, Nikos Andronikos
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-yosemite (756.49 KB, application/zip)
2016-01-23 03:29 PST, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-yosemite (838.57 KB, application/zip)
2016-01-23 03:35 PST, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (948.48 KB, application/zip)
2016-01-23 03:43 PST, Build Bot
no flags Details
Patch (29.20 KB, patch)
2016-01-24 21:08 PST, Nikos Andronikos
no flags Details | Formatted Diff | Diff
Patch (35.22 KB, patch)
2016-02-09 20:51 PST, Nikos Andronikos
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-yosemite (770.22 KB, application/zip)
2016-02-09 21:39 PST, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (830.09 KB, application/zip)
2016-02-09 21:43 PST, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-yosemite (832.92 KB, application/zip)
2016-02-09 21:48 PST, Build Bot
no flags Details
Patch (34.59 KB, patch)
2016-02-11 18:27 PST, Nikos Andronikos
no flags Details | Formatted Diff | Diff
Patch (35.80 KB, patch)
2016-02-25 03:40 PST, Nikos Andronikos
no flags Details | Formatted Diff | Diff
Patch (35.81 KB, patch)
2016-03-01 14:10 PST, Nikos Andronikos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikos Andronikos 2014-11-06 00:01:11 PST
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
Comment 1 Nikos Andronikos 2014-11-06 00:38:00 PST
Created attachment 241093 [details]
Patch
Comment 2 WebKit Commit Bot 2014-11-06 00:39:19 PST
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.
Comment 3 Nikos Andronikos 2014-11-06 00:52:38 PST
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?
Comment 4 Dirk Schulze 2014-11-06 01:33:02 PST
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.
Comment 5 Dirk Schulze 2014-11-06 01:35:23 PST
(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.
Comment 6 Dirk Schulze 2014-11-06 12:25:56 PST
(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.
Comment 7 Nikos Andronikos 2016-01-20 19:40:33 PST
Created attachment 269417 [details]
Patch
Comment 8 WebKit Commit Bot 2016-01-20 19:42:24 PST
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.
Comment 9 Build Bot 2016-01-20 20:32:40 PST
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
Comment 10 Build Bot 2016-01-20 20:32:43 PST
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
Comment 11 Build Bot 2016-01-20 20:43:25 PST
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
Comment 12 Build Bot 2016-01-20 20:43:28 PST
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
Comment 13 Build Bot 2016-01-20 20:51:25 PST
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
Comment 14 Build Bot 2016-01-20 20:51:30 PST
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
Comment 15 Nikos Andronikos 2016-01-22 18:40:38 PST
Created attachment 269631 [details]
Patch
Comment 16 Nikos Andronikos 2016-01-23 02:38:53 PST
Created attachment 269662 [details]
Patch
Comment 17 WebKit Commit Bot 2016-01-23 02:42:00 PST
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.
Comment 18 Build Bot 2016-01-23 03:29:41 PST
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
Comment 19 Build Bot 2016-01-23 03:29:45 PST
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
Comment 20 Build Bot 2016-01-23 03:35:21 PST
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
Comment 21 Build Bot 2016-01-23 03:35:24 PST
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
Comment 22 Build Bot 2016-01-23 03:43:20 PST
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
Comment 23 Build Bot 2016-01-23 03:43:23 PST
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
Comment 24 Nikos Andronikos 2016-01-24 21:08:37 PST
Created attachment 269721 [details]
Patch
Comment 25 WebKit Commit Bot 2016-01-24 21:11:22 PST
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.
Comment 26 Darin Adler 2016-01-25 09:24:10 PST
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?
Comment 27 Nikos Andronikos 2016-02-09 20:51:55 PST
Created attachment 270968 [details]
Patch
Comment 28 WebKit Commit Bot 2016-02-09 20:52:45 PST
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.
Comment 29 Nikos Andronikos 2016-02-09 20:57:09 PST
(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?
Comment 30 Build Bot 2016-02-09 21:38:59 PST
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
Comment 31 Build Bot 2016-02-09 21:39:01 PST
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
Comment 32 Build Bot 2016-02-09 21:43:08 PST
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
Comment 33 Build Bot 2016-02-09 21:43:10 PST
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
Comment 34 Build Bot 2016-02-09 21:48:07 PST
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
Comment 35 Build Bot 2016-02-09 21:48:09 PST
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
Comment 36 Nikos Andronikos 2016-02-11 18:27:22 PST
Created attachment 271114 [details]
Patch
Comment 37 WebKit Commit Bot 2016-02-11 18:29:26 PST
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.
Comment 38 Nikos Andronikos 2016-02-18 16:22:38 PST
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.
Comment 39 Said Abou-Hallawa 2016-02-19 16:45:10 PST
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?
Comment 40 Said Abou-Hallawa 2016-02-19 17:12:17 PST
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) {
    ...
}
Comment 41 Nikos Andronikos 2016-02-25 03:35:43 PST
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).
Comment 42 Nikos Andronikos 2016-02-25 03:40:16 PST
Created attachment 272200 [details]
Patch
Comment 43 WebKit Commit Bot 2016-02-25 03:42:38 PST
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.
Comment 44 Darin Adler 2016-02-26 08:17:23 PST
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, { });
Comment 45 Nikos Andronikos 2016-02-28 17:41:51 PST
(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 '{ }'?
Comment 46 Nikos Andronikos 2016-03-01 14:10:32 PST
Created attachment 272590 [details]
Patch
Comment 47 WebKit Commit Bot 2016-03-01 14:15:15 PST
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.
Comment 48 WebKit Commit Bot 2016-03-08 00:20:12 PST
Comment on attachment 272590 [details]
Patch

Clearing flags on attachment: 272590

Committed r197738: <http://trac.webkit.org/changeset/197738>
Comment 49 WebKit Commit Bot 2016-03-08 00:20:19 PST
All reviewed patches have been landed.  Closing bug.