Bug 164496 – [SVG] Start moving special casing of SVG out of the bindings - SVGAngle

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

[SVG] Start moving special casing of SVG out of the bindings - SVGAngle

[Sam Weinig]

Created attachment 294104 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/svg/SVGAngleValue.h:37:  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/SVGAngleValue.h:38:  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/SVGAngleValue.h:39:  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/SVGAngleValue.h:40:  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/SVGAngleValue.h:41:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 5 in 31 files


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

[Darin Adler]

Comment on attachment 294104 [details]
Patch

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

> Source/WebCore/svg/SVGAngle.h:40
> +    static Ref<SVGAngle> create(SVGAnimatedProperty* animatedProperty, SVGPropertyRole role, SVGAngleValue& value)

Does this first argument need to be a pointer rather than a reference?

> Source/WebCore/svg/SVGAngle.h:140
> +    SVGAngle(SVGAngleValue initialValue)

explicit?

> Source/WebCore/svg/SVGAngleValue.h:34
> +class SVGAngleValue {
> +    WTF_MAKE_FAST_ALLOCATED;

Why WTF_MAKE_FAST_ALLOCATED? Who allocates these on the heap?

> Source/WebCore/svg/SVGAngleValue.h:42
> +    enum Type {
> +        SVG_ANGLETYPE_UNKNOWN = 0,
> +        SVG_ANGLETYPE_UNSPECIFIED = 1,
> +        SVG_ANGLETYPE_DEG = 2,
> +        SVG_ANGLETYPE_RAD = 3,
> +        SVG_ANGLETYPE_GRAD = 4
> +    };

Annoying to repeat this enum as SVGAngleValue::Type and as SVGAngle::SVGAngleType. Would be bad if the two ever got out of sync. Any ideas on how to avoid that?

> Source/WebCore/svg/SVGPathSegList.h:31
>  class SVGElement;
> +template<typename T> class SVGPropertyTearOff;

Blank line here so the templates are in a separate paragraph from the classes?

> Source/WebCore/svg/SVGStringList.h:30
>  class SVGElement;
> +template<typename T> class SVGPropertyTearOff;

Ditto.

[Sam Weinig]

(In reply to comment #3)
> Comment on attachment 294104 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=294104&action=review
> 
> > Source/WebCore/svg/SVGAngle.h:40
> > +    static Ref<SVGAngle> create(SVGAnimatedProperty* animatedProperty, SVGPropertyRole role, SVGAngleValue& value)
> 
> Does this first argument need to be a pointer rather than a reference?

Good question. I am duplicating the interface of SVGPropertyTearOff here, so I didn't want to change anything like that. I'll find out before landing.

> 
> > Source/WebCore/svg/SVGAngle.h:140
> > +    SVGAngle(SVGAngleValue initialValue)
> 
> explicit?

Will change.

> 
> > Source/WebCore/svg/SVGAngleValue.h:34
> > +class SVGAngleValue {
> > +    WTF_MAKE_FAST_ALLOCATED;
> 
> Why WTF_MAKE_FAST_ALLOCATED? Who allocates these on the heap?

SVGPropertyTearOff (and therefore SVGAngle) allocates these. See the SVGPropertyTearOff constructor that takes a const PropertyType*.

> 
> > Source/WebCore/svg/SVGAngleValue.h:42
> > +    enum Type {
> > +        SVG_ANGLETYPE_UNKNOWN = 0,
> > +        SVG_ANGLETYPE_UNSPECIFIED = 1,
> > +        SVG_ANGLETYPE_DEG = 2,
> > +        SVG_ANGLETYPE_RAD = 3,
> > +        SVG_ANGLETYPE_GRAD = 4
> > +    };
> 
> Annoying to repeat this enum as SVGAngleValue::Type and as
> SVGAngle::SVGAngleType. Would be bad if the two ever got out of sync. Any
> ideas on how to avoid that?

I was really sad about this. I tried a few things, like trying to use a using statement to re-export the enum in another place.  I think I will probably need to update the bindings to be able to look in an alternate place for these.

> 
> > Source/WebCore/svg/SVGPathSegList.h:31
> >  class SVGElement;
> > +template<typename T> class SVGPropertyTearOff;
> 
> Blank line here so the templates are in a separate paragraph from the
> classes?

Ok.

> 
> > Source/WebCore/svg/SVGStringList.h:30
> >  class SVGElement;
> > +template<typename T> class SVGPropertyTearOff;
> 
> Ditto.

Ok.

[Sam Weinig]

Created attachment 294271 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/svg/SVGAngleValue.h:33:  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/SVGAngleValue.h:34:  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/SVGAngleValue.h:35:  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/SVGAngleValue.h:36:  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/SVGAngleValue.h:37:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 5 in 31 files


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

[WebKit Commit Bot]

Comment on attachment 294271 [details]
Patch

Clearing flags on attachment: 294271

Committed r208480: <http://trac.webkit.org/changeset/208480>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.