xslt/xpath are part of our core browsing experience: the code should be moved to content/xsl and the impl made non-optional. I'm not sure exactly how standalone transformiix works, but I don't think this change will really affect it all that much.

This is the cvsmoves file to move extensions/transformiix to content/xsl... the patch (against the existing files) is forthcoming.

This is the patch. xslt-standalone does not work yet (needs changes to the root Makefile and a few additional makefile changes), but mozilla xslt works fine AFAICT.

Should we take this opportunity to move xslt/util/* and xslt/functions/* into xslt/ ? (at least the former should be moved at some point)

I was doing the least amount of work possible ;-) If you want to do more reorganization, let me know what the consensus is: I do want to get this done very quickly, as it blocks some aspects of libxul/xulrunner.

Yeah, on second thought i think it might be better to leave such things for later. There are quite a bit of rearranging that could be done so lets do all that in a single later step instead.

I do think this needs buy-in from more people than just the XSLT owner/peers. Did you get that from someone else? Note also bug 192773 were people actually argued for a way to disable XSLT completely. See attachment 148461 [details] for the moves we were planning to do. I personally also wanted to change most of the third list of https://bugzilla.mozilla.org/show_bug.cgi?id=235748#c5 to all have a 'tx' prefix. This will cause a lot of pain due to all the patches the XSLT peers have in their own trees, so we might want to combine the moves to make it quick.

Buy-in from the DOM owner (jst) is all else I need. I already did s/TxLog/txLog/ just because it looked so weird. Note that renaming *files* is probably pretty easy right now (change my local symlinks and add some sed), but I am not up to the task of combining headers and whatnot.

Note that all the other stuff was already checked in (see attachment 148771 [details] [diff] [review]), so we're only talking about renaming. Renaming does mean also changing includes and Makefiles though.

I think we should just do that stuff later. We've talked about doing some major restructuring in the source/xml folder too so i think we can just do all the internal cleanups at one time then.

However, I *do* think that this might be a good time to rename content/xsl to content/xslt.

(In reply to comment #9) > I think we should just do that stuff later. We've talked about doing some major > restructuring in the source/xml folder too so i think we can just do all the > internal cleanups at one time then. And I think if we can do it now, we should. I'd rather go through only one of these renaming/move changes to all my trees.

Comment on attachment 192817 [details] [diff] [review] CVSMoves file I got MOA for the general concept of moving everything to content/xslt from jst... all I need are actual code reviews on the little bit that has actually changed (nsLayoutModule is the bit I'm least sure about).

Comment on attachment 192951 [details] [diff] [review] CVSMoves file >mozilla/extensions/transformiix/source/xml/dom/standalone/Document.cpp mozilla/content/xslt/src/xml/dom/txDocument.cpp >mozilla/extensions/transformiix/source/xml/dom/standalone/Element.cpp mozilla/content/xslt/src/xml/dom/txElement.cpp >mozilla/extensions/transformiix/source/xml/dom/standalone/dom.h mozilla/content/xslt/src/xml/dom/txDOM.h >mozilla/extensions/transformiix/source/xml/dom/standalone/Makefile.in mozilla/content/xslt/src/xml/dom/Makefile.in >mozilla/extensions/transformiix/source/xml/dom/standalone/NodeListDefinition.cpp mozilla/content/xslt/src/xml/dom/txNodeListDefinition.cpp NodeListDefinition.cpp is dead. >mozilla/extensions/transformiix/source/xml/dom/standalone/ProcessingInstruction.cpp mozilla/content/xslt/src/xml/dom/txProcessingInstruction.cpp >mozilla/extensions/transformiix/source/xml/dom/standalone/Attr.cpp mozilla/content/xslt/src/xml/dom/txAttr.cpp >mozilla/extensions/transformiix/source/xml/dom/standalone/NamedNodeMap.cpp mozilla/content/xslt/src/xml/dom/txNamedNodeMap.cpp NamedNodeMap.cpp too. >mozilla/extensions/transformiix/source/xml/dom/standalone/NodeDefinition.cpp mozilla/content/xslt/src/xml/dom/txNodeDefinition.cpp >mozilla/extensions/transformiix/source/xml/parser/Makefile.in mozilla/content/xslt/src/xml/parser/Makefile.in >mozilla/extensions/transformiix/source/xml/parser/txXMLParser.h mozilla/content/xslt/src/xml/parser/txXMLParser.h >mozilla/extensions/transformiix/source/xml/parser/txXMLParser.cpp mozilla/content/xslt/src/xml/parser/txXMLParser.cpp I'd prefer to have no subdirectories of xml, so move everything from xml/dom/standalone/ and xml/parser into xml/ >mozilla/extensions/transformiix/source/xslt/functions/ElementAvailableFnCall.cpp mozilla/content/xslt/src/xslt/functions/txElementAvailableFnCall.cpp >mozilla/extensions/transformiix/source/xslt/functions/FunctionAvailableFnCall.cpp mozilla/content/xslt/src/xslt/functions/txFunctionAvailableFnCall.cpp >mozilla/extensions/transformiix/source/xslt/functions/GenerateIdFunctionCall.cpp mozilla/content/xslt/src/xslt/functions/txGenerateIdFunctionCall.cpp >mozilla/extensions/transformiix/source/xslt/functions/SystemPropertyFunctionCall.cpp mozilla/content/xslt/src/xslt/functions/txSystemPropertyFunctionCall.cpp >mozilla/extensions/transformiix/source/xslt/functions/Makefile.in mozilla/content/xslt/src/xslt/functions/Makefile.in >mozilla/extensions/transformiix/source/xslt/functions/XSLTFunctions.h mozilla/content/xslt/src/xslt/functions/txXSLTFunctions.h >mozilla/extensions/transformiix/source/xslt/functions/txFormatNumberFunctionCall.cpp mozilla/content/xslt/src/xslt/functions/txFormatNumberFunctionCall.cpp >mozilla/extensions/transformiix/source/xslt/functions/txKeyFunctionCall.cpp mozilla/content/xslt/src/xslt/functions/txKeyFunctionCall.cpp >mozilla/extensions/transformiix/source/xslt/functions/txKey.h mozilla/content/xslt/src/xslt/functions/txKey.h >mozilla/extensions/transformiix/source/xslt/functions/CurrentFunctionCall.cpp mozilla/content/xslt/src/xslt/functions/txCurrentFunctionCall.cpp >mozilla/extensions/transformiix/source/xslt/functions/DocumentFunctionCall.cpp mozilla/content/xslt/src/xslt/functions/txDocumentFunctionCall.cpp Same for xslt, move everything from xslt/functions into xslt/ Otherwise this looks great.

Comment on attachment 192952 [details] [diff] [review] Move extensions/transformiix You seem to be using txdom.h in this patch and txDOM.h in the attachment 192951 [details] [diff] [review]. (I prefer txDOM.h) >Index: extensions/transformiix/source/xml/XMLUtils.h >=================================================================== >@@ -168,41 +167,41 @@ public: > int result = MOZ_XMLCheckQName(NS_REINTERPRET_CAST(const char*, > aQName.get()), > NS_REINTERPRET_CAST(const char*, > end), > PR_TRUE, &colonPtr); > *aColon = NS_REINTERPRET_CAST(const PRUnichar*, colonPtr); > return result == 0; > #else >- return NS_SUCCEEDED(gTxParserService->CheckQName(aQName, PR_TRUE, aColon)); >+ return NS_SUCCEEDED(nsContentUtils::GetParserServiceWeakRef()->CheckQName(aQName, PR_TRUE, aColon)); Need to null-check the result of nsContentUtils::GetParserServiceWeakRef() > #endif > } > > /** > * Returns true if the given character represents an Alpha letter > */ > static PRBool isLetter(PRUnichar aChar) > { > #ifdef TX_EXE > return MOZ_XMLIsLetter(NS_REINTERPRET_CAST(const char*, &aChar)); > #else >- return gTxParserService->IsXMLLetter(aChar); >+ return nsContentUtils::GetParserServiceWeakRef()->IsXMLLetter(aChar); Need to null-check the result of nsContentUtils::GetParserServiceWeakRef() > #endif > } > > /** > * Returns true if the given character is an allowable NCName character > */ > static PRBool isNCNameChar(PRUnichar aChar) > { > #ifdef TX_EXE > return MOZ_XMLIsNCNameChar(NS_REINTERPRET_CAST(const char*, &aChar)); > #else >- return gTxParserService->IsXMLNCNameChar(aChar); >+ return nsContentUtils::GetParserServiceWeakRef()->IsXMLNCNameChar(aChar); Need to null-check the result of nsContentUtils::GetParserServiceWeakRef() >Index: extensions/transformiix/source/xpath/Makefile.in >=================================================================== >-ifndef DISABLE_XFORMS_HOOKS >+# The following files really ought to be provided through an extensible xpath >+# function mechanism. See bug 278981, so please leave the ifdef. > EXPORTS = nsIXFormsUtilityService.h \ > nsIXFormsXPathEvaluator.h > > CPPSRCS += nsXFormsXPathEvaluator.cpp \ >- XFormsFunctionCall.cpp >-endif >+ txXFormsFunctionCall.cpp >Index: extensions/transformiix/source/xpath/txXPathAtomList.h >=================================================================== >-#ifndef DISABLE_XFORMS_HOOKS >+ > // XForms XPath Extensions >+// The following functions really ought to be provided by xforms through >+// an extensibility mechanism. >-#endif See above. >Index: extensions/transformiix/source/xpath/txXPathNode.h >=================================================================== >@@ -126,30 +125,26 @@ public: >+ nsContentUtils::GetNSManagerWeakRef()->RegisterNameSpace(aNamespaceURI, namespaceID); Long line. >Index: extensions/transformiix/source/xslt/txMozillaXSLTProcessor.cpp >=================================================================== >@@ -524,17 +524,17 @@ txMozillaXSLTProcessor::SetParameter(con > > default: > { > return NS_ERROR_FAILURE; > } > } > > PRInt32 nsId = kNameSpaceID_Unknown; >- nsresult rv = gTxNameSpaceManager->RegisterNameSpace(aNamespaceURI, nsId); >+ nsresult rv = nsContentUtils::GetNSManagerWeakRef()->RegisterNameSpace(aNamespaceURI, nsId); Long line. >@@ -547,34 +547,34 @@ txMozillaXSLTProcessor::SetParameter(con > } > > NS_IMETHODIMP > txMozillaXSLTProcessor::GetParameter(const nsAString& aNamespaceURI, > const nsAString& aLocalName, > nsIVariant **aResult) > { > PRInt32 nsId = kNameSpaceID_Unknown; >- nsresult rv = gTxNameSpaceManager->RegisterNameSpace(aNamespaceURI, nsId); >+ nsresult rv = nsContentUtils::GetNSManagerWeakRef()->RegisterNameSpace(aNamespaceURI, nsId); Long line. > NS_IMETHODIMP > txMozillaXSLTProcessor::RemoveParameter(const nsAString& aNamespaceURI, > const nsAString& aLocalName) > { > PRInt32 nsId = kNameSpaceID_Unknown; >- nsresult rv = gTxNameSpaceManager->RegisterNameSpace(aNamespaceURI, nsId); >+ nsresult rv = nsContentUtils::GetNSManagerWeakRef()->RegisterNameSpace(aNamespaceURI, nsId); Long line. >Index: layout/build/nsLayoutModule.cpp >=================================================================== >@@ -331,16 +484,28 @@ Initialize(nsIModule* aSelf) > if (NS_FAILED(rv)) { > NS_ERROR("Could not initialize nsTextTransformer"); > > Shutdown(); > > return rv; > } > >+ gXPathExceptionProvider = new nsXPathExceptionProvider(); >+ if (!gXPathExceptionProvider || !txXSLTProcessor::init()) { >+ Shutdown(); >+ return NS_ERROR_OUT_OF_MEMORY; >+ } This will cause a problem if txXSLTProcessor::init fails, you'll NS_RELEASE gXPathExceptionProvider in Shutdown without it ever being addref'ed. The stuff in nsLayoutModule.cpp should all be rewritten to use content-specific DOMCI macros, feel free to file a followup bug on that. I don't really understand why we have both TX_EXE and MOZ_XSLT_STANDALONE. And I suppose this breaks building transformiix standalone?

Comment on attachment 192951 [details] [diff] [review] CVSMoves file r=me with the moves requested by peterv

Hmm.. and what about the suggestion to use content/xsl*t*? I'd hate to see all these files go into a bad place in the content tree.

doh! I missed the fact that you are moving the files there. My bad :)

This builds standalone and integrated. I have changed --enable-application=content/xslt to match future changes I have planned for the configure script. I know that the changes to mozilla/Makefile.in are a little hackish, but that will get better soon.

Comment on attachment 193072 [details] [diff] [review] Move extensions/transformiix > Index: extensions/transformiix/source/base/Makefile.in > =================================================================== > RCS file: /cvsroot/mozilla/extensions/transformiix/source/base/Makefile.in,v > retrieving revision 1.40 > diff -u -4 -r1.40 Makefile.in > --- extensions/transformiix/source/base/Makefile.in 4 Apr 2005 18:38:01 -0000 1.40 > +++ extensions/transformiix/source/base/Makefile.in 18 Aug 2005 17:29:21 -0000 > @@ -49,12 +49,14 @@ > REQUIRES = string \ > xpcom \ > $(NULL) > > -ifndef TX_EXE > +ifndef MOZ_XSLT_STANDALONE > REQUIRES += unicharutil \ > dom \ > content \ I guess we can remove content from all the REQUIRES? > Index: extensions/transformiix/source/main/Makefile.in > =================================================================== > -INCLUDES += -I$(srcdir)/../xslt -I$(srcdir)/../base -I$(srcdir)/../net \ > - -I$(srcdir)/../xml -I$(srcdir)/../xml/dom \ > - -I$(srcdir)/../xml/parser \ > - -I$(srcdir)/../xpath -I$(srcdir)/../xslt/util \ > - -I$(srcdir)/../xslt/functions $(MARK_INC) > +INCLUDES += \ > + -I$(srcdir)/../base \ > + -I$(srcdir)/../xml \ > + -I$(srcdir)/../xpath \ Remove one space > Index: extensions/transformiix/source/xslt/functions/Makefile.in > =================================================================== > ifndef TX_EXE I think this should be MOZ_XSLT_STANDALONE? I haven't checked all Makefile.in's, please make sure you converted them all.

Once this is done we can remove the dynamic extension of nsDocument and always support QIing to nsIDOMXPathEvaluator

How about telling Camino that this was going to happen?

I've got lots of build-config patchery coming up, and I didn't realize in particular that this was going to break camino. FYI, there are going to be similar bugs for xmlextras and webservices coming down the pipe soonish.

Fixed on trunk.

In Camino build, Tinderbox burns. http://tinderbox.mozilla.org/showlog.cgi?log=Camino/1130953920.28438.gz Please update Camino.xcode. http://lxr.mozilla.org/mozilla/source/camino/Camino.xcode/

(In reply to comment #27) > In Camino build, Tinderbox burns. > http://tinderbox.mozilla.org/showlog.cgi?log=Camino/1130953920.28438.gz > > Please update Camino.xcode. > http://lxr.mozilla.org/mozilla/source/camino/Camino.xcode/ > This has been fixed by pink's checkin. http://tinderbox.mozilla.org/bonsai/cvsquery.cgi?module=Camino&branch=HEAD&cvsroot=/cvsroot&date=explicit&mindate=1130962860&maxdate=1130963160&who=pinkerton%25aol.net

Followup bug on the DOMCI macros is bug 314931.

Err.. so it seems like extensions/transformiix was cvs-removed without all files being moved to the new location. I thought you were going to keep the directory in extensions until the remaining stuff was moved. There were quite a lot of stuff in transformiix/resouces that was still used, and some nice designdocs in transformiix/docs. I'd say that at the very least tx/resources should be moved to content/xslt/resources. The docs stuff should probably move into the devmo wiki.

Strangely with all those moves configure still accepts (don't break at) --enable-extensions="transformix" which leads to build boostage if this option remained in .mozconfig file.

--enable-extensions=unknown is allowed on purpose so that people can add custom bits to the build without altering configure

hmm, maybe configure should check that all such subdirectories exist: for i in $MOZ_EXTENSIONS; do if ! test -d extensions/$i; then AC_MSG_ERROR([Extension "$i" does not exist]); fi done or something