The code of MathML creates some "private" attributes (see atoms MOZcolumnalign, MOZcolumnline etc). Their names begin with a "-". This is not good because: - it is not valid in XML. - it doesn't follow the implicit usage in Gecko of the "_moz" prefix in "private" attribute names. - it introduced a hack in the XML serializer. See nsXMLContentSerializer.cpp line 735: // XXX Hack to get around the fact that MathML can add // attributes starting with '-', which makes them // invalid XML. if (!nameStr.IsEmpty() && nameStr.First() == '-') continue; The serializer should have only one hack on "_moz" attributes (and this will be fixed in bug 422403), and MathML attributes should follow this "normalization".

Definition of XML attribute names. "-" is allowed inside the name but not as the first character: http://www.w3.org/TR/REC-xml/#NT-AttDef http://www.w3.org/TR/REC-xml/#NT-Name http://www.w3.org/TR/REC-xml/#NT-NameStartChar http://www.w3.org/TR/REC-xml/#NT-NameChar -moz-math-font-size does not seem to be used, and apparently is likely to be implemented later when bug 69409 is fixed. Hence this patch removes this GkAtom. This patch also removes the hack in the XML serializer.

Laurent, I guess the serializer should not generate these private MathML attributes. How does the it behave with the "_moz*" attributes?

"_moz*" attributes are removed, according to the test on sharedName, few lines above the code you removed. In fact, I think the code you removed was already useless because of this previous test. The hack should also be removed from the XML serializer, but here we have to add a test on sharedName similar to the XHTML serializer one, so all _moz* attributes will be also removed when using the XML serializer (ex: when serializing SVG+MathML for ex..).

(In reply to comment #3) > "_moz*" attributes are removed, according to the test on sharedName, few lines > above the code you removed. In fact, I think the code you removed was already > useless because of this previous test. > > The hack should also be removed from the XML serializer, but here we have to > add a test on sharedName similar to the XHTML serializer one, so all _moz* > attributes will be also removed when using the XML serializer (ex: when > serializing SVG+MathML for ex..). OK, actually I had not really looked carefully to the code of the serializers and did not see the two versions... I think we should also remove the test ('-' == *sharedName) but apparently there are still some "-moz*" attributes in nsGkAtomList.h so it's probably better not to do it now. Hence I guess what I have to do for this bug is removing the two hacks and adding a test on sharedName in the XML serializer. Do you agree?

>we should also remove the test ('-' == *sharedName) but apparently there are still some "-moz*" attributes in nsGkAtomList.h so it's probably better not to do it now Even if I think those other names are CSS names only, I think we shouldn't remove this test on '-' because perhaps all "-moz" attributes are not stored in an Atom, and then some of them are hardcoded somewhere. We have to verify it, but can be hard.. >Do you agree? Yes, except if there is a very old reason I don't know, to not unserialize these attributes (except MathML attributes). Do it, and we will see if a super reviewer is not agree with us :-)

OK, thanks for the information. Here is an updated patch.

Comment on attachment 411215 [details] [diff] [review] More changes in the XML/XHTML serializers >@@ -848,38 +850,40 @@ nsXMLContentSerializer::SerializeAttribu > continue; > } > > const nsAttrName* name = aContent->GetAttrNameAt(index); > PRInt32 namespaceID = name->NamespaceID(); > nsIAtom* attrName = name->LocalName(); > nsIAtom* attrPrefix = name->GetPrefix(); > >+ // Filter out any attribute starting with [-|_]moz >+ const char* sharedName; >+ attrName->GetUTF8String(&sharedName); >+ if ((('_' == *sharedName) || ('-' == *sharedName)) && >+ !nsCRT::strncmp(sharedName+1, kMozStr, PRUint32(sizeof(kMozStr)-1))) { >+ continue; >+ } >+ If we do this, it should now be updated in line with the recent change to nsXHTMLContentSerializer: http://hg.mozilla.org/mozilla-central/diff/cb17f4d92942/content/base/src/nsXHTMLContentSerializer.cpp Sorry I'm slow here. Requesting sr from bz to confirm that filtering [-|_]moz attributes is the right thing to do.

Comment on attachment 411215 [details] [diff] [review] More changes in the XML/XHTML serializers The filtering is fine, but why do we no longer need it in the XHTML serializer?

The filtering is already done in the XHTML serializer here: http://hg.mozilla.org/mozilla-central/annotate/1008f5c2e057/content/base/src/nsXHTMLContentSerializer.cpp#l348

Comment on attachment 411215 [details] [diff] [review] More changes in the XML/XHTML serializers Perfect, thanks! sr=me if this is updated to the new atom api.

Thanks for the review. I've updated the patch to fix conflicts.

@Frédéric : it seems you included wrong removal of files (toolkit/crashreporter/google-breakpad/src/common/dwarf/* ) in your last patch, isn't it ?

Yes, sorry I don't know where this change comes from.