Bug 51490 - Cleanup SVG code according to the webkit style rules 3
Summary: Cleanup SVG code according to the webkit style rules 3
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-12-22 13:08 PST by Dirk Schulze
Modified: 2010-12-30 00:38 PST (History)
10 users (show)

See Also:


Attachments
Patch (64.10 KB, patch)
2010-12-22 13:17 PST, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (64.25 KB, patch)
2010-12-22 23:36 PST, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (64.21 KB, patch)
2010-12-27 08:42 PST, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (65.83 KB, patch)
2010-12-27 11:02 PST, Dirk Schulze
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Schulze 2010-12-22 13:08:49 PST
see bug 51411
Comment 1 Dirk Schulze 2010-12-22 13:17:30 PST
Created attachment 77253 [details]
Patch
Comment 2 WebKit Review Bot 2010-12-22 13:27:07 PST
Attachment 77253 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7319092
Comment 3 Early Warning System Bot 2010-12-22 13:40:33 PST
Attachment 77253 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7237125
Comment 4 Build Bot 2010-12-22 13:50:02 PST
Attachment 77253 [details] did not build on win:
Build output: http://queues.webkit.org/results/7325107
Comment 5 WebKit Review Bot 2010-12-22 14:03:00 PST
Attachment 77253 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7200131
Comment 6 Eric Seidel (no email) 2010-12-22 14:21:03 PST
Attachment 77253 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7256113
Comment 7 WebKit Review Bot 2010-12-22 14:25:45 PST
Attachment 77253 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7281123
Comment 8 Dirk Schulze 2010-12-22 23:36:26 PST
Created attachment 77309 [details]
Patch
Comment 9 Andreas Kling 2010-12-23 01:01:05 PST
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 :)
Comment 10 Eric Seidel (no email) 2010-12-23 01:08:23 PST
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.
Comment 11 Andreas Kling 2010-12-23 01:48:01 PST
(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 :)
Comment 12 Dirk Schulze 2010-12-27 05:57:01 PST
(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.
Comment 13 Dirk Schulze 2010-12-27 08:42:16 PST
Created attachment 77509 [details]
Patch
Comment 14 Nikolas Zimmermann 2010-12-27 10:04:02 PST
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.
Comment 15 Dirk Schulze 2010-12-27 10:48:23 PST
(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.
Comment 16 Dirk Schulze 2010-12-27 11:02:43 PST
Created attachment 77513 [details]
Patch
Comment 17 Dirk Schulze 2010-12-29 22:50:27 PST
Committed r74782: <http://trac.webkit.org/changeset/74782>
Comment 18 WebKit Review Bot 2010-12-29 23:37:48 PST
http://trac.webkit.org/changeset/74782 might have broken Leopard Intel Debug (Tests)
Comment 19 Dirk Schulze 2010-12-29 23:48:37 PST
(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.
Comment 20 WebKit Review Bot 2010-12-30 00:38:30 PST
http://trac.webkit.org/changeset/74783 might have broken GTK Linux 64-bit Debug