Bug 43311 – Cleanup all of svg/ code

• Component: SVG
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

The svg/ code needs housekeeping:

1. Make all copyright headers look identical
- That involves adding missing Copyright (C) statements before the dates.
- Renaming my old email address, from wildfox to zimmermann.
- Some headers use stars at the beginning of each line in the copyright statement, remove that.

2. Cleanup headers
- Remove class indention present in lots of SVG headers.
- Remove unnecessary includes from the headers.
- Remove the // namespace... and // ENABLE... lines after the #endif and namespace end brace.

3. Cleanup implementation files
- Remove unncessary includes from each file (removing all, testing compilation by adding back one after the other)
- Move inclusion of the corresponding header directly below the config.h line
- Remove the // ENABLE... lines after the #endif
- Fix general style issues pointed out by check-webkit-style

I'm going to start immediately, it sucks to have an inconsistent coding style.

[Nikolas Zimmermann]

Note that this patch actually prepares an outstanding task: remove createWithNew from svgtags.in, deploy static PassRefPtr create() functionality to all SVG*Element classes, and let all start with refcount 1 instead of zero.

[Dirk Schulze]

- Some headers use stars at the beginning of each line in the copyright statement, remove that.

I would like it the other way arround, like we do it on every other non-SVG file. Means, allways use stars at the beginning.

[Dirk Schulze]

- Remove the // ENABLE... lines after the #endif

The same, please let them in, makes it much easier, if you don't use some kind of hyper modern code viewer to see which #endif closes which #if. And it's also in used on every non-SVG file. IIRC you also get some patches denied because you erased them (or at least got a comment about that).

[Dirk Schulze]

Maybe we should discuss it on webkit-dev and write the result of this discussion to the style rules to end this religious war about '// ENABLE ...' and stars at the beginning of headers and cpp's. I think even we both had a dispute about it multiple times :-)

[Nikolas Zimmermann]

(In reply to comment #2)
> - Some headers use stars at the beginning of each line in the copyright statement, remove that.
> 
> I would like it the other way arround, like we do it on every other non-SVG file. Means, allways use stars at the beginning.

Hm, okay, whatever is consistent.

(In reply to comment #3)
> - Remove the // ENABLE... lines after the #endif
> 
> The same, please let them in, makes it much easier, if you don't use some kind of hyper modern code viewer to see which #endif closes which #if. And it's also in used on every non-SVG file. IIRC you also get some patches denied because you erased them (or at least got a comment about that).

I don't think it makes anything easier. But you're right, most use it.

[Nikolas Zimmermann]

Created attachment 63306 [details]
Header update script

Adding the script I used to unify all header templates in svg/, might be of general interesst.

[Nikolas Zimmermann]

Created attachment 63307 [details]
Patch - Updating all license headers

There's no risk that I forgot to add a copyright in the new header template, as it's all auto-generated.

[Dirk Schulze]

Comment on attachment 63307 [details]
Patch - Updating all license headers

> There's no risk that I forgot to add a copyright in the new header template, as it's all auto-generated.

Nevertheless, I checked them ;-)

Thank you Niko. This was the most important patch I've ever reviewed :-D

[Gabor Loki]

Well, I think you can not convert BSD licence to LGPL one. The BSD license says:

1. Redistributions of source code must retain the above copyright notice, this list of conditions and the following disclaimer.

I am not a lawyer, but this means the copyright notice, the first and second point of condition list and the ending disclaimer must be in source code, but the new LGPL licence does not hold those.

[Nikolas Zimmermann]

(In reply to comment #9)
> Well, I think you can not convert BSD licence to LGPL one. The BSD license says:
> 
> 1. Redistributions of source code must retain the above copyright notice, this list of conditions and the following disclaimer.
> 
> I am not a lawyer, but this means the copyright notice, the first and second point of condition list and the ending disclaimer must be in source code, but the new LGPL licence does not hold those.

I didn't change a BSD license to a LGPL one, did I?

[Nikolas Zimmermann]

Oh you're right, I need permission from the author.
For the IDL files it's easy, as I checked them in in r16337, obviously with the wrong license, as I intended to use LGPL (See http://trac.webkit.org/changeset/16337/trunk/WebCore/ksvg2/svg/SVGCircleElement.idl)

CC'ing Darin Adler. I need to hear some comments whether I have to revert the license changes.
FYI I did NOT notice that I actually changed some BSD -> LGPL licenses, sorry about that.

Is it okay to change the licenses? Do I need to ask permission from anyone listed in the headers, that are affected?

[Nikolas Zimmermann]

Darin, can you please have a look?

[Gabor Loki]

> I didn't change a BSD license to a LGPL one, did I?

:D
for example:
WebCore/svg/ElementTimeControl.h
WebCore/svg/SVGAElement.idl
WebCore/svg/SVGAllInOne.cpp
... etc ...

Look for the '* 1. ' strings. ;)

[Nikolas Zimmermann]

Most IDL files contain the wrong copyright anyways, I'm the original author of those files, Eric imported them from KDE CVS more than 5 years ago (w/o licenses), and later ddkilzer added the standard Apple BSD license to them, which was wrong. Using trac you can find out that I'm the original author, so I can change the license.

The full list of files that I accidently converted, w/o asking for permission are:
svg/ElementTimeControl.h
svg/ElementTimeControl.idl
svg/SVGAllInOne.cpp
svg/SVGAltGlyphElement.idl
svg/SVGElementInstance.idl
svg/SVGElementInstanceList.idl
svg/SVGFEConvolveMatrix.idl
svg/SVGFEMorphologyElement.idl
svg/SVGFontElement.idl
svg/SVGFontFaceElement.idl
svg/SVGFontFaceFormatElement.idl
svg/SVGFontFaceNameElement.idl
svg/SVGFontFaceSrcElement.idl
svg/SVGFontFaceUriElement.idl
svg/SVGGlyphElement.idl
svg/SVGMissingGlyphElement.idl
svg/SVGViewSpec.idl
svg/animation/SMILTime.cpp
svg/animation/SMILTime.h
svg/animation/SMILTimeContainer.cpp
svg/animation/SMILTimeContainer.h
svg/animation/SVGSMILElement.cpp
svg/animation/SVGSMILElement.h
svg/graphics/SVGImage.cpp
svg/graphics/SVGImage.h
svg/graphics/filters/SVGFELighting.cpp
svg/graphics/filters/SVGFELighting.h

Shall I revert these? Or ask the authors wheter it's fine?

[Dirk Schulze]

(In reply to comment #14)
> The full list of files that I accidently converted, w/o asking for permission are:
> svg/ElementTimeControl.h
> svg/ElementTimeControl.idl
> svg/SVGAllInOne.cpp
> svg/SVGAltGlyphElement.idl
> svg/SVGElementInstance.idl
> svg/SVGElementInstanceList.idl
> svg/SVGFEConvolveMatrix.idl
> svg/SVGFEMorphologyElement.idl
> svg/SVGFontElement.idl
> svg/SVGFontFaceElement.idl
> svg/SVGFontFaceFormatElement.idl
> svg/SVGFontFaceNameElement.idl
> svg/SVGFontFaceSrcElement.idl
> svg/SVGFontFaceUriElement.idl
> svg/SVGGlyphElement.idl
> svg/SVGMissingGlyphElement.idl
> svg/SVGViewSpec.idl
> svg/animation/SMILTime.cpp
> svg/animation/SMILTime.h
> svg/animation/SMILTimeContainer.cpp
> svg/animation/SMILTimeContainer.h
> svg/animation/SVGSMILElement.cpp
> svg/animation/SVGSMILElement.h
> svg/graphics/SVGImage.cpp
> svg/graphics/SVGImage.h
> svg/graphics/filters/SVGFELighting.cpp
> svg/graphics/filters/SVGFELighting.h
> 
> Shall I revert these? Or ask the authors wheter it's fine?

If it is realy a BSD license, there are no strong arguments about changing the license here, since WebKit is dual licensed.

[Dirk Schulze]

Comment on attachment 63307 [details]
Patch - Updating all license headers

Sorry, that I did not take the licensing into account enough. It is a BSD license and like I wrote in the previous comment, there is no reason to change the license of these files.

[Nikolas Zimmermann]

Dirk, you're aware this already landed?

[Gabor Loki]

> If it is realy a BSD license, there are no strong arguments about changing the license here, since WebKit is dual licensed.

Yes, WebKit is dual licensed, but it does not mean that you can switch any license (BSD or LGPL) to the another one.

> Sorry, that I did not take the licensing into account enough. It is a BSD license and like I wrote in the previous comment, there is no reason to change the license of these files.

I would say there is no chance to switch a BSD license to a LGPL one (at least in this way ;) ).

[Nikolas Zimmermann]

(In reply to comment #17)
> Dirk, you're aware this already landed?

Eek, the comment did not get through (mid air collision)
Landed "Patch - Updating all license headers" in r64539.

Rolling it out, to be safe...

[Nikolas Zimmermann]

Created attachment 63336 [details]
Patch - Updating all license headers (v2)

I've updated my script to account for the different licenses: Now I'm unifying all LGPL files, and all BSD files, seperated.

[Eric Seidel (no email)]

FYI, all new code from Google or from Apple is very intentionally BSD instead of LGPL.  Does RIM ask you to be adding new files under the LGPL?  I'd be slightly surprised by that.

[Eric Seidel (no email)]

Comment on attachment 63336 [details]
Patch - Updating all license headers (v2)

The ChangeLog looks wrong.  Otherwise this looks OK.

This seems like a useful script to run over all of WebKit and to check into Scripts as tidy-licence-headers or something.

[Gabor Loki]

(In reply to comment #22)
> (From update of attachment 63336 [details])
> The ChangeLog looks wrong.  Otherwise this looks OK.

I do not think so. I still do not understand why the ending disclaimers are changed at SVGFELighting* files. The University of Szeged and Apple Inc. are not the same group - at least for now ;).

[Dirk Schulze]

(In reply to comment #23)
> (In reply to comment #22)
> > (From update of attachment 63336 [details] [details])
> > The ChangeLog looks wrong.  Otherwise this looks OK.
> 
> I do not think so. I still do not understand why the ending disclaimers are changed at SVGFELighting* files. The University of Szeged and Apple Inc. are not the same group - at least for now ;).

And you introduced a space in:
* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 
;-)

[Nikolas Zimmermann]

(In reply to comment #23)
> (In reply to comment #22)
> > (From update of attachment 63336 [details] [details])
> > The ChangeLog looks wrong.  Otherwise this looks OK.
> 
> I do not think so. I still do not understand why the ending disclaimers are changed at SVGFELighting* files. The University of Szeged and Apple Inc. are not the same group - at least for now ;).

Good catch. I just replaced BSD style licenses with a newer template. That's why it replaced your copyright, I'm going to restore it.

[Nikolas Zimmermann]

(In reply to comment #24)
> (In reply to comment #23)
> > (In reply to comment #22)
> > > (From update of attachment 63336 [details] [details] [details])
> > > The ChangeLog looks wrong.  Otherwise this looks OK.
> > 
> > I do not think so. I still do not understand why the ending disclaimers are changed at SVGFELighting* files. The University of Szeged and Apple Inc. are not the same group - at least for now ;).
> 
> And you introduced a space in:
> * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 
> ;-)

Yes, all other BSD licenses (most .idl files) have this trailing space, so it just saves svn diff length, to add the space in the other two files..

[Nikolas Zimmermann]

Landed patch v2 in r64579 - Gabor can you please have a look? I hope I resolved all your issues as well.

[Gabor Loki]

Your script is very ardent. ;)

Look at WebCore/svg/SVGFEOffsetElement.h.
In this file the LGPL licence is changed to BSD one.

[Nikolas Zimmermann]

(In reply to comment #28)
> Your script is very ardent. ;)
> 
> Look at WebCore/svg/SVGFEOffsetElement.h.
> In this file the LGPL licence is changed to BSD one.

*sigh*, fixed in r64633. Sorry for that :-) I hope that was the last failure?

[Eric Seidel (no email)]

This was approved over 4 months ago.  Did it get landed?

[Dirk Schulze]

At least the license texts were cleaned up. I think we can close this bug since the headline is a bit vague.