Bug 51490 – Cleanup SVG code according to the webkit style rules 3

• Component: SVG
• Status: RESOLVED FIXED

[Dirk Schulze]

see bug 51411

[Dirk Schulze]

Created attachment 77253 [details]
Patch

[WebKit Review Bot]

Attachment 77253 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7319092

[Early Warning System Bot]

Attachment 77253 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7237125

[Build Bot]

Attachment 77253 [details] did not build on win:
Build output: http://queues.webkit.org/results/7325107

[WebKit Review Bot]

Attachment 77253 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7200131

[Eric Seidel (no email)]

Attachment 77253 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7256113

[WebKit Review Bot]

Attachment 77253 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7281123

[Dirk Schulze]

Created attachment 77309 [details]
Patch

[Andreas Kling]

Comment on attachment 77309 [details]
Patch

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

r=me with one small thing and one medium:

> WebCore/ChangeLog:8
> +        Last patch to fix indention and other style issues according to the WebKit style rules in the SVG code.

s/indention/indentation/

> WebCore/svg/SVGSVGElement.cpp:133
> -    double _x = 0.0;
> -    double _y = 0.0;
> +    double m_x = 0.0;
> +    double m_y = 0.0;

This looks quite wrong, m_x and m_y are member variables, and you are shadowing them here.
Use better names please :)

[Eric Seidel (no email)]

Comment on attachment 77309 [details]
Patch

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

In general this looks fine, but the m_x shadowing is bad-news bears.  I'll leave kling's r+, but that definitely needs to be fixed before landing.

>> WebCore/svg/SVGSVGElement.cpp:133
>> +    double m_y = 0.0;
> 
> This looks quite wrong, m_x and m_y are member variables, and you are shadowing them here.
> Use better names please :)

We can't r+ this as-is... This would be bad news bears.

> WebCore/svg/SVGSwitchElement.cpp:53
> +    for (Node* node = firstChild(); node; node = node->nextSibling()) {
> +        if (node->isSVGElement()) {
> +            SVGElement* element = static_cast<SVGElement*>(node);
>              if (element && element->isValid())
> -                return (n == child); // Only allow this child if it's the first valid child
> +                return (node == child); // Only allow this child if it's the first valid child

Does isValid possibly run javascript or triggger any events which could invalidate the node pointer?

> WebCore/svg/animation/SVGSMILElement.h:45
> +    static bool isSMILElement(Node* node);

unneeded arg name.

[Andreas Kling]

(In reply to comment #10)
> (From update of attachment 77309 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77309&action=review
> 
> In general this looks fine, but the m_x shadowing is bad-news bears.  I'll leave kling's r+, but that definitely needs to be fixed before landing.

Bad-news bears indeed, I should've been more clear that this must be fixed before landing.

> > WebCore/svg/animation/SVGSMILElement.h:45
> > +    static bool isSMILElement(Node* node);
> 
> unneeded arg name.

Good catch :)

[Dirk Schulze]

(In reply to comment #10)
> (From update of attachment 77309 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77309&action=review
> > WebCore/svg/SVGSwitchElement.cpp:53
> > +    for (Node* node = firstChild(); node; node = node->nextSibling()) {
> > +        if (node->isSVGElement()) {
> > +            SVGElement* element = static_cast<SVGElement*>(node);
> >              if (element && element->isValid())
> > -                return (n == child); // Only allow this child if it's the first valid child
> > +                return (node == child); // Only allow this child if it's the first valid child

isValid() is just implemented in SVGTest and is const. It just checks for a certain feature.

Uploading a new patch soon.

[Dirk Schulze]

Created attachment 77509 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 77509 [details]
Patch

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

Still needs some tweaks, r-.

> WebCore/ChangeLog:11
> +        * SVGAllInOne.cpp: check-webkit-style wants a config.h at the beginning
> +        * SVGImage.cpp: One header needs to be in a non-alphabetic order, see FIXME.

Do you know about the "NOLINT" comment.
int _I_want_to_violate_the_style_result; // NOLINT
This suppresses the error message from check-webkit-style. I guess you can make check-webkit-style happy when playing around with this comment, to make it report 0 errors for svg/.

> WebCore/svg/SVGSwitchElement.cpp:50
> +        if (node->isSVGElement()) {

if (!node->isSVGElement())
    continue;

> WebCore/svg/SVGSwitchElement.cpp:52
>              if (element && element->isValid())

if (!element || !element->isValid())
    continue;

> WebCore/svg/SVGSwitchElement.cpp:53
> +                return (node == child); // Only allow this child if it's the first valid child

return node == child.

> WebCore/svg/SVGTransformDistance.cpp:240
> +    return m_transform == AffineTransform() && !m_angle;

m_transform.isIdentity() ?

> WebCore/svg/animation/SVGSMILElement.cpp:800
> -        if (fmod(repeatingDuration.value(), simpleDuration.value() == 0.))
> +        if (fmod(repeatingDuration.value(), !simpleDuration.value()))

Is this correct??

> WebCore/svg/animation/SVGSMILElement.h:60
> +    enum Restart { RestartAlways, RestartWhenNotActive, RestartNever };

I think these should be
enum Restart {
    RestartAlways,
     .....
}

> WebCore/svg/animation/SVGSMILElement.h:63
> +    enum FillMode { FillRemove, FillFreeze };

Ditto.

> WebCore/svg/animation/SVGSMILElement.h:107
> +    enum BeginOrEnd { Begin, End };

enum BeginOrEnd {
    Begin,
    End
};

> WebCore/svg/animation/SVGSMILElement.h:122
> +        enum Type { EventBase, Syncbase, AccessKey };

Ditto, needs reformatting.

> WebCore/svg/animation/SVGSMILElement.h:150
> +    enum ActiveState { Inactive, Active, Frozen };

Ditto.

[Dirk Schulze]

(In reply to comment #14)
> (From update of attachment 77509 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77509&action=review
> 
> Still needs some tweaks, r-.
> 
> > WebCore/ChangeLog:11
> > +        * SVGAllInOne.cpp: check-webkit-style wants a config.h at the beginning
> > +        * SVGImage.cpp: One header needs to be in a non-alphabetic order, see FIXME.
> 
> Do you know about the "NOLINT" comment.
No, didn't know this. But I'd add this after every line on SVGGAllInOne, not the best choice. But it helps on SVGImage. Do you have another suggestion?

> > WebCore/svg/animation/SVGSMILElement.cpp:800
> > -        if (fmod(repeatingDuration.value(), simpleDuration.value() == 0.))
> > +        if (fmod(repeatingDuration.value(), !simpleDuration.value()))
> 
> Is this correct??
Checked it, it is correct. The operator ! just checks if value is finite as well, doesn't influence the result.

The rest is fixed.

[Dirk Schulze]

Created attachment 77513 [details]
Patch

[Dirk Schulze]

Committed r74782: <http://trac.webkit.org/changeset/74782>

[WebKit Review Bot]

http://trac.webkit.org/changeset/74782 might have broken Leopard Intel Debug (Tests)

[Dirk Schulze]

(In reply to comment #18)
> http://trac.webkit.org/changeset/74782 might have broken Leopard Intel Debug (Tests)

Checked it, must be a previous bug that broke Leopard. It was broken before the patch got landed.

To the red style bot: I checked the code before uploading this patch. So I guess it is a problem of the bot, not of the code style. It doesn't mention the problem anyway.

[WebKit Review Bot]

http://trac.webkit.org/changeset/74783 might have broken GTK Linux 64-bit Debug