Summary: | Cleanup svg coding style | ||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Rob Buis <rwlbuis> | ||||||||||||||||||||||||||||||||||||||||
Component: | SVG | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | mitz, mrowe, sam | ||||||||||||||||||||||||||||||||||||||||
Priority: | P3 | ||||||||||||||||||||||||||||||||||||||||||
Version: | 420+ | ||||||||||||||||||||||||||||||||||||||||||
Hardware: | Mac | ||||||||||||||||||||||||||||||||||||||||||
OS: | OS X 10.4 | ||||||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Rob Buis
2006-10-08 08:16:02 PDT
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 :-) |