Bug 11217 – Cleanup svg coding style

• Component: SVG
• Status: RESOLVED FIXED

[Rob Buis]

Svg code has the following style problems:

- use of if(, for(, while(, switch(
- unsorted inludes
- usage of * on the right, for instance Foo *foo instead of Foo* foo, same for &
- using namespace Webcore; instead of wrapping the implementation code in namespace WebCore { ... }
- unneeded includes, { not on the same line as if, incorrect use of whitespace

[Rob Buis]

Created attachment 10979 [details]
Cleanup for svg filter code

Should be clear what this about looking at the description :)
Cheers,

Rob.

[Rob Buis]

Created attachment 10980 [details]
Cleanup for svg SVGPath* code

This one is pretty straightforward.
Cheers,

Rob.

[Rob Buis]

Created attachment 10986 [details]
Cleanup for svg code in shape classes

This time a cleanup for shape classes.
Cheers,

Rob.

[Adam Roben (:aroben)]

Comment on attachment 10986 [details]
Cleanup for svg code in shape classes

r=me

[Oliver Hunt]

Comment on attachment 10980 [details]
Cleanup for svg SVGPath* code

remove 

-    filterStyle->deref(view()->renderArena());
+    filterStyle->deref(document()->renderArena());

and associated bits before landing, as we discussed.

[Darin Adler]

Comment on attachment 10979 [details]
Cleanup for svg filter code

Looks good.

[Oliver Hunt]

Comment on attachment 10980 [details]
Cleanup for svg SVGPath* code

looks sane to me

[Rob Buis]

Created attachment 10992 [details]
Cleaning up of anim* classes

Small patch this time, anim* classes...
Cheers,

Rob.

Comment on attachment 10992 [details]
Cleaning up of anim* classes

             Node *target = parentNode();
-            while(target != 0) {
+            while (target != 0) {

Forgot to move the star there. Also, per the style guidelines, the second line should say just
+            while (target) {

Comment on attachment 10992 [details]
Cleaning up of anim* classes

Rob is going to post an updated patch

[Rob Buis]

Created attachment 10995 [details]
Improved patch

This one solves some issues with the previous, now obsoleted anim patch.
Cheers,

Rob.

Comment on attachment 10995 [details]
Improved patch

I asked Rob to make a few more changes: change NULLs to 0s, keep the #include "SVGColor.h" in SVGAnimateColorElement.cpp, and move the open braces in switch statements to the same line.

[Rob Buis]

Created attachment 11010 [details]
New anim patch

Addreessing Mitz' issues this time I hope :)
Cheers,

Rob.

Comment on attachment 11010 [details]
New anim patch

r=me

[Rob Buis]

Created attachment 11023 [details]
Cleanup for svg text code

This time a cleanup for text related classes.
Cheers,

Rob.

[Rob Buis]

Created attachment 11035 [details]
Improved patch

New patch after olliej' text changes.
Cheers,

Rob.

Comment on attachment 11035 [details]
Improved patch

There are several declarations (repeating in several headers) where you could drop the parameter names:

+        virtual void parseMappedAttribute(MappedAttribute* attr);
+        virtual RenderObject* createRenderer(RenderArena* arena, RenderStyle* style);
+        bool childShouldCreateRenderer(Node* child) const;
+        virtual void updateLocalTransform(SVGTransformList* localTransforms);

The second line here should be indented:

+SVGTSpanElement::SVGTSpanElement(const QualifiedName& tagName, Document* doc)
 : SVGTextPositioningElement(tagName, doc)

[Rob Buis]

Created attachment 11042 [details]
Improved patch

Fixing the headers...
Cheers,

Rob.

Comment on attachment 11042 [details]
Improved patch

r=me

[Rob Buis]

Created attachment 11052 [details]
First attempt

Various style fixes for svg structure classes.
Cheers,

Rob.

Comment on attachment 11052 [details]
First attempt

Unnecessary parameter names in
+        SVGDocument(DOMImplementation* i, FrameView* view);

These should be two lines each:
+    if (SVGLangSpace::parseMappedAttribute(attr)) return;
+    if (SVGExternalResourcesRequired::parseMappedAttribute(attr)) return;
+    if (SVGFitToViewBox::parseMappedAttribute(attr)) return;

I also discussed with Rob the commented-out code in SVGSVGElement.cpp. Those comments should be deleted and replaced with FIXMEs, preferably referring to bugs about the missing functionality.

[Rob Buis]

Created attachment 11062 [details]
Improved patch

Now includes FIXMEs in SVGSVGElement referencing to bug numbers.
Cheers,

Rob.

Comment on attachment 11062 [details]
Improved patch

Looks great!
+    //FIXME: implement me (see bug 11273)
1) There should be a space between // and FIXME.
2) You should use sentence capitalization for the part after "FIXME:", e.g. "// FIXME: Implement me (see bug 11273)". (This applies also to comments that you added that have the space after the //).

[Rob Buis]

Created attachment 11065 [details]
Improved patch

Fixing remaining issues.
Cheers,

Rob.

Comment on attachment 11065 [details]
Improved patch

Two stars remaining to be moved here:
         virtual bool rendererIsNeeded(RenderStyle *style) { return StyledElement::rendererIsNeeded(style); }
-        virtual RenderObject *createRenderer(RenderArena *arena, RenderStyle *style);
+        virtual RenderObject *createRenderer(RenderArena*, RenderStyle*);

You can fix this when landing. r=me

[Rob Buis]

Created attachment 11073 [details]
First attempt

Cleaning up the house, err paint servers ;->
Cheers,

Rob.

Comment on attachment 11073 [details]
First attempt

Redundant parameter names (some of these show up multiple times):

-        virtual void buildGradient(KRenderingPaintServerGradient *grad) const = 0;
+        virtual void buildGradient(KRenderingPaintServerGradient* grad) const = 0;

-        const SVGStyledElement *pushAttributeContext(const SVGStyledElement *context);
+        const SVGStyledElement* pushAttributeContext(const SVGStyledElement* context);

-        virtual void buildGradient(KRenderingPaintServerGradient *grad) const;
+        virtual void buildGradient(KRenderingPaintServerGradient* grad) const;

Don't need the "!= 0" here:

+    for (Node* n = target->firstChild(); n != 0; n = n->nextSibling()) {

Don't need the space before the > here (some of these show up multiple times):

         SVGStyledElement* e = static_cast<SVGStyledElement* >(elem);

         SVGStyledElement* styled = (current ? static_cast<SVGStyledElement* >(current->element()) : 0);

         m_paintServer->setListener(const_cast<SVGPatternElement* >(this));

-    KRenderingPaintServerRadialGradient *grad = static_cast<KRenderingPaintServerRadialGradient *>(_grad);
+    KRenderingPaintServerRadialGradient* grad = static_cast<KRenderingPaintServerRadialGradient* >(_grad);

-        KRenderingPaintServerGradient *gradient = static_cast<KRenderingPaintServerGradient *>(pserver);
+        KRenderingPaintServerGradient* gradient = static_cast<KRenderingPaintServerGradient* >(pserver);

+            KRenderingPaintServerRadialGradient* radial = static_cast<KRenderingPaintServerRadialGradient* >(pserver);

[Rob Buis]

Created attachment 11077 [details]
Improved patch

This should address Mitz' issues.
Cheers,

Rob.

Comment on attachment 11077 [details]
Improved patch

r=me

[Rob Buis]

Created attachment 11089 [details]
First attempt

This time cleaning up basic datastructures.
Cheers,

Rob.

Comment on attachment 11089 [details]
First attempt

My only comments are about lines that the patch doesn't touch:

The opening brace should go on the same line as the case:

         case SVG_LENGTHTYPE_EXS:
         {

This needs spaces after the semicolons:

     for (unsigned long i = 0;i < list->numberOfItems();i++) {

The way this is split into lines doesn't make sense. I think two lines with 4 parameters in each will suffice, but otherwise, it should be 2 parameters on each line:
+        SVGMatrix* getCTM(float logicX, float logicY,
                               float logicWidth, float logicHeight,
                               float physX, float physY, float physWidth,
                               float physHeight);

[Rob Buis]

Created attachment 11092 [details]
Improved patch

This one should address Mitz' issues.
Cheers,

Rob.

Comment on attachment 11092 [details]
Improved patch

r=me

[Rob Buis]

Created attachment 11114 [details]
First attempt

This one should be easy to review :-)
Cheers,

Rob.

Comment on attachment 11114 [details]
First attempt

Don't need "!= 0" here:
+    for (Node* n = firstChild(); n != 0; n = n->nextSibling()) {

Don't need the "angle" and "attr" parameter names: 
+        void setOrientToAngle(SVGAngle* angle);

         virtual void attributeChanged(Attribute* attr, bool preserveDecls);

[Rob Buis]

Created attachment 11163 [details]
Improved patch

This patch should fix the remaining issues.
Cheers,

Rob.

[Rob Buis]

Created attachment 11165 [details]
Even better patch

Mitz noted some problems with my previous patch, so I invalidated that one, this
one should be better.
Cheers,

Rob.

Comment on attachment 11165 [details]
Even better patch

r=me

[Mark Rowe (bdash)]

Do any of these patches remain to be landed?  It's still showing up in the pending commit queue, but it's not at all obvious which of these patches have been landed.

[Rob Buis]

Hi,

(In reply to comment #39)
> Do any of these patches remain to be landed?  It's still showing up in the
> pending commit queue, but it's not at all obvious which of these patches have
> been landed.

All patches that have been appended are reviewed and okayed (sometimes not on the first try). I expect there is still one big patch and one or two small patches left before all of the svg code has been cleaned up, I plan to do the big one soon. Once these are done, the whole bug can be closed. I hope that clears things up a bit.
Cheers,

Rob.

[Sam Weinig]

(In reply to comment #40)

Rob,
It would be helpful if you checked the "obsolete" check-mark for the patches that have been applied (or rejected) so that we can figure out what, if anything, is left to commit.  If it's possible, it would also be nice if you (or the commiter) noted what revision the patches were landed in.

[Rob Buis]

Hi Sam,

(In reply to comment #41)
> (In reply to comment #40)
> 
> Rob,
> It would be helpful if you checked the "obsolete" check-mark for the patches
> that have been applied (or rejected) so that we can figure out what, if
> anything, is left to commit.  If it's possible, it would also be nice if you
> (or the commiter) noted what revision the patches were landed in.

I tried to obsolete some but I get an error after checking the obsolete flag.
Cheers,

Rob.

Comment on attachment 10986 [details]
Cleanup for svg code in shape classes

Landed in r16918

Comment on attachment 10980 [details]
Cleanup for svg SVGPath* code

Landed in r16920

Comment on attachment 11010 [details]
New anim patch

Landed in r16963

Comment on attachment 11042 [details]
Improved patch

Landed in r17008

Comment on attachment 11065 [details]
Improved patch

Landed in r17027

Comment on attachment 11077 [details]
Improved patch

Landed in r17041

Comment on attachment 11092 [details]
Improved patch

Landed in r17062

Comment on attachment 11165 [details]
Even better patch

Landed in r17158

I guess we're done here :-)