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 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.
(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.
(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.
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.
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
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
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
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.
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
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
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
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 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?
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.
(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?
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
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
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
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.
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 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 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 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).
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 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, { });
(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 '{ }'?
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.
2014-11-06 00:38 PST, Nikos Andronikos
2016-01-20 19:40 PST, Nikos Andronikos
2016-01-20 20:32 PST, Build Bot
2016-01-20 20:43 PST, Build Bot
2016-01-20 20:51 PST, Build Bot
2016-01-22 18:40 PST, Nikos Andronikos
2016-01-23 02:38 PST, Nikos Andronikos
2016-01-23 03:29 PST, Build Bot
2016-01-23 03:35 PST, Build Bot
2016-01-23 03:43 PST, Build Bot
2016-01-24 21:08 PST, Nikos Andronikos
2016-02-09 20:51 PST, Nikos Andronikos
2016-02-09 21:39 PST, Build Bot
2016-02-09 21:43 PST, Build Bot
2016-02-09 21:48 PST, Build Bot
2016-02-11 18:27 PST, Nikos Andronikos
2016-02-25 03:40 PST, Nikos Andronikos
2016-03-01 14:10 PST, Nikos Andronikos