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
Created attachment 10979 [details] Cleanup for svg filter code Should be clear what this about looking at the description :) Cheers, Rob.
Created attachment 10980 [details] Cleanup for svg SVGPath* code This one is pretty straightforward. Cheers, Rob.
Created attachment 10986 [details] Cleanup for svg code in shape classes This time a cleanup for shape classes. Cheers, Rob.
Comment on attachment 10986 [details] Cleanup for svg code in shape classes r=me
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.
Comment on attachment 10979 [details] Cleanup for svg filter code Looks good.
Comment on attachment 10980 [details] Cleanup for svg SVGPath* code looks sane to me
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
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.
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
Created attachment 11023 [details] Cleanup for svg text code This time a cleanup for text related classes. Cheers, Rob.
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)
Created attachment 11042 [details] Improved patch Fixing the headers... Cheers, Rob.
Comment on attachment 11042 [details] Improved patch r=me
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.
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 //).
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
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);
Created attachment 11077 [details] Improved patch This should address Mitz' issues. Cheers, Rob.
Comment on attachment 11077 [details] Improved patch r=me
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);
Created attachment 11092 [details] Improved patch This one should address Mitz' issues. Cheers, Rob.
Comment on attachment 11092 [details] Improved patch r=me
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);
Created attachment 11163 [details] Improved patch This patch should fix the remaining issues. Cheers, Rob.
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
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.
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.
(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.
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 :-)