Just noticed that some .h files in content/svg/content/src/ are missing the conventional "#ifndef/#define/#endif" wrapping. In particular, grep says that these files lack any mention of "ifndef": nsSVGAElement.h nsSVGElementList.h nsSVGFeaturesList.h nsSVGMpathElement.h nsSVGPolyElement.h
Created attachment 447234 [details] [diff] [review] added the header wrappings
Comment on attachment 447234 [details] [diff] [review] added the header wrappings >--- a/content/svg/content/src/nsSVGElementList.h Mon May 24 14:48:24 2010 +0200 >+++ b/content/svg/content/src/nsSVGElementList.h Mon May 24 22:00:21 2010 +0200 >@@ -62,6 +62,9 @@ > * A convenience value indicating support for none of the above > */ > >+#ifndef NS_SVGAELEMENT_H_ >+#define NS_SVGAELEMENT_H_ Copy-paste error -- this should be NS_SVGELEMENTLIST_H_. :) Looks good aside from that! r=dholbert with the above fixed. Requesting additional review from jwatt, even though this is a trivial-ish change, just to be on the safe side.
(In reply to comment #0) > In particular, grep says that these files lack any mention of "ifndef": > [...] > nsSVGMpathElement.h Note that this patch doesn't touch nsSVGMpathElement.h, and that's fine, because it's already got the correct wrapping. I was mistaken to list it in comment 0. (It didn't even exist in m-c until 2 weeks after comment 0. The copy caught by grep in comment 0 was a WIP version in my local tree. :))
Comment on attachment 447234 [details] [diff] [review] added the header wrappings Thanks for checking. nsSVGElementList.h and nsSVGFeaturesList.h should not have include guards. Can you attach a new patch with your changes to nsSVGAElement.h and nsSVGPolyElement.h, and with a comment added to the top of nsSVGElementList.h and nsSVGFeaturesList.h to say "no include guard" please Filipe?
Created attachment 447301 [details] [diff] [review] Fixes the header wrappings and adds "no include guards" message to nsSVGElementList.h and nsSVGFeaturesList.h Sorry about the copy/paste mistake. I am curious to know what is the reason for not having include guards around nsSVGElementList.h and nsSVGFeaturesList.h
Comment on attachment 447301 [details] [diff] [review] Fixes the header wrappings and adds "no include guards" message to nsSVGElementList.h and nsSVGFeaturesList.h Perfect, thanks Filipe! Do you have commit access to be able to push this to mozilla-central yourself?
Sorry, Felipe, not Filipe. The reason is because these are not normal headers with declarations that must only be declared once. The macro inside those headers has to be is defined by the file that includes the header, to do whatever repeated thing is required for all the items listed in the header. Although these headers are not currently used twice in the same file, it should be possible to use them like that. There are other headers like these in the Mozilla codebase that are like this that are included multiple times into the same file, but I can't think of examples offhand.
(In reply to comment #6) > (From update of attachment 447301 [details] [diff] [review]) > Perfect, thanks Filipe! Do you have commit access to be able to push this to > mozilla-central yourself? Not yet. What is the commit access policy?
Thanks, Robert. I am reading it now. But I think that I still don't really need such access. Can somebody else push this code into mozilla-central, please?
In general you should go to the keywords field at the top and set checkin-needed if you don't have access. jwatt may be on the case for this bug even without that though.
Not on the case just yet, so if anyone gets to it first, feel free. Else maybe I'll get it pushed tomorrow if we have a better network connection at the conference and I can watch the tree.
I'll land this along with another patch later today.
Thanks Daniel. And thanks for the patch Felipe.