We should allow embedders who don't want XSLT to not have *any* of it, including the content/xsl directory. This also means pretty printing XML gets disabled, because that depends on XSLT.

excellent find, john. cc'ing some other interested parties.

This patch creates a --disable-xslt-bindings and MOZ_XSLT_BINDINGS define. I can compile both with and without the option. Should we just call it --disable-xslt?

Do we need another configure option for this? Is there anytime that you'd want the bindings w/o building xslt? Hrm, I'd guess so if you wanted to make xslt an optional add-on. If we're not concerned about the add-on case, then I'd say skip the option and just check to see if transformiix is already in the MOZ_EXTENSIONS list and set MOZ_XSLT_BINDINGS as appropriate.

I can't speak for it as an optional add-on; I kind of assumed it could be done that way since it seems to be built for portability. Perhaps Jonas can answer that. I'd be happy to not have an option if transformiix has to be compiled in.

Note that this is not a huge codesize savings, but at least it's an easy one :) On an opt build of mine, the xsldoc object files add up to around 20K and pretty printing is another 20K. Add in the savings for the functions that were removed and we're at roughly 45K. 45K is pretty respectable but nothing earthshaking. I don't have numbers yet on the size difference for the layout dll.

Sigh. IIRC, one of the goals of the current architecture was to allow easy removal of XSLT by just not including the Transformiix library. Removing the supporting code from Gecko means that those builds lose any optional installation of XSLT support. Please consider that carefully before adding these ifdefs. The XSL content sink will die in the near future as we move to compiled XSLT stylesheets.

Yes, the point of this is for tiny embedders who do not want xslt and never will. Most Mozillas will want this option enabled. If optional XSLT drop-in support is an expected feature (as it sounds like you are saying), then I think we should keep the options in this patch the way they are (--disable-xslt-binding).

peterv: its not that we need to drop this from the standard gecko distribution, its just that some people want an absolutely bare HTML rendering engine.. this isn't the place to discuss if "absolutely bare" means that we're including xslt or not, but adding these options gives embeddors the option of removing it if they need to be aggressive about removing features.

P3P summary view will also not work as it depends on XSLT.

I have a couple of tinderboxen on the Minimo branch that are compiling with this patch now; I'd like to drop it on that branch if this works.

You might as well also disable everything involving DOM-XPath too. That includes the dom/xpath directory as well as some stuff in nsDocument

The xpath disabling idea is a good one. Maybe dom/public/idl has indications of other things we can optionally turn off. Testing on an aggressively minimized branch (Minimo) shows only a 20K savings in codesize of TestGTKEmbed (0.2%, out of 10M); but since a very small embedded customer is unlikely to use xslt, this is pretty much free savings for them--it's not like they will be weighing "should I save 20K or have XSLT and pretty printing in my browser?"

This patch includes not linking the library in. Searching through the tree there's a pretty print xml, xsl and css file I still need to get rid of (duh!). There is also a "components/content_xslt.xpt" file (why the heck is that being generated, its generation doesn't show up in the log and I deleted it before the cycle!). But grepping for XSL and PrettyPrint in dist/bin (and subdirs) does not yield me anything else obvious.

Hrm. I don't see any clean way to disable the .xml/.css/.xsl files from getting into the jar when MOZ_XSLT_BINDINGS is not defined. I could disable the content/xml/document/resources directory entirely, but that's a bad hack since future files dropped in that directory would also be disabled. Suggestions?

I'd really REALLY like to see this called --disable-xslt .. We already have things like --disable-xul and --disable-mathml I mean what good is it without the bindings?

Comment on attachment 114227 [details] [diff] [review] Patch v1.1 please remove transformiix from the extensions for disabled xslt.

Personally i would prefer to see this called --disable-all-but-minimal-browser-engine or some such. The current incarnation of --disable-xslt should be kept as is, otherwise we might as well move transformiix from mozilla/extensions to mozilla/content or mozilla/modules and just use the --disable-xslt flag to disable it.

There is no current --disable-xslt switch. It's simply in the list of extensions. I think the point here is that an embedder is likely to compile everything at once (I think) so if they're not using transformiix at compile time they never will. Here's an alternate idea though: how about we have some switch like --disable-extension-dropin, which means "the extensions I specify now are the *only* extensions I'll ever have." Then if they define that and (for example) transformiix is not in the extensions, we disable these bindings. That way you only need one switch and we can key off of it for stuff like this in the future. Easier for the embedder, easier for us.

Actually what I would *really* prefer is to enable more things as dropins without specific hooks in the main codebase. One good example that me and jkeiser just talked about on irc was to make XSLT and prettyprinting just use nsIDocumentObserver. Same thing should be possible with xul, view-source, mathml, svg and so on. This should also give the code cleaner separateion and make it more maintainable. I suspect that otherwise we'll end up in ifdef-hell attempting to allow minimal distributions and custom configurations.

This patch: - moves xmlprettyprinter out into its own module - creates --disable-extra-extension-bindings, which disables bindings for extensions you don't compile in; applies that to XSLT. This should keep a flood of bindings - note that I did not #ifdef out the pretty printing binding code; that code is so minimal that the #ifdefs probably aren't worth it--it's just an interface and some booleans. There's already enough ifdefs for xslt, but that's justifiable by removing content/xsl/* from the compile / link flow. We could potentially handle xslt the same way--leave the interfaces around but drop the document stuff--but this seems like a good place to stop for me :) sicking has suggested we create a devtools module and put xmlprettyprinter into it. Thinking about it, I wouldn't mind a subdirectory extensions/devtools/ with xmlprettyprinter module under that to reduce the sheer volume of directories under modules; and then an xpi creator could be put in devtools/build or some such at a later time. But I'd like to see the rest of this patch reviewed as I debate that.

Comment on attachment 114310 [details] [diff] [review] Patch v2.0 Oh, it also fixes Pike's comment by disabling xmlprettyprint when these bindings are disabled. If the bindings are not disabled prettyprint can stay in, on the theory that one could drop in xslt later.

Heh, having configure in there makes the patch big (I *do* need to check in configure, no?). Patch uploaded to Patch Viewer so you can collapse that file: http://landfill.bugzilla.org/bz176656/attachment.cgi?id=121&action=prettyview

http://landfill.bugzilla.org/bz176656/attachment.cgi?id=121&action=prettyview#mozilla/layout/html/tests/Makefile.in_sec1 changes the linkorder, that might hurt. Which is just a technical comment, I'll leave the religious debate up to the believers.

you don't check in configure, only configure.in there's some build magic which handles configure.

ideally yes, all extensions/etc would have to only be plugged in via XPCom ContractIDs... I love the idea of not needing the bindings at all (or at least moving them to extensions/ do we have a list of issues that are preventing us from simply switching to nsIDocumentObserver? (i.e. nsIFrame isn't frozen, etc) even if its just a far off dream, we should at least have bugs filed against the different issues...

the configure.in help comment should state more clearly that one looses functionality and that this is targetting embedders with severe limitations. I don't want to end up with a bug each week saying "fix XML", just because yet another wise guy thought minimal builds would be fancy. (And yes, there are users with the vision that XML and XSLT support are one thing) alecf, if you want to remove the hooks for XSLT, there are a bunch of issues to resolve. Basically, we need to hook up custom result documents into the viewer, triggered by events. If anybody feels like filing bugs, don't file them for the current architecture, this is supposed to die as it is blocking the UI during XSLT transforms. Another thing that the hooks try to acomplish is having a replacable XSLT engine. One would need a design doc to do this right. Volunteers? Honestly, it might be nice to have, but nowhere near essential, I can imagine better use for our limited resources.

I'll post it in the bug since I replied privately to jkeiser and sicking. I think using nsIDocumentObserver for calling the XSLT engine is overkill. I am all for minimalizing the bindings, but 1) nsIDocumentObserver notifies about way too many things and 2) this would mean notifying the XSLT processor of every XML file being loaded, even if none of them use a xml-stylesheet processing instruction. XSLT needs two types of notifications: 1 for every xml-stylesheet PI and 1 for the end of the source document load. The XML content sink is the best place to judge when we should instantiate the XSLT processor and start notifying it, which is probably never in 99% of the document loads. Let's not make those 99% suffer for the 1% (if that much), even if it costs us a little footprint. John: you didn't disable DOM XPath. See http://lxr.mozilla.org/seamonkey/source/content/base/src/nsDocument.cpp#469 http://lxr.mozilla.org/seamonkey/source/content/base/src/nsDocument.cpp#645 http://lxr.mozilla.org/seamonkey/source/content/base/src/nsGenericElement.cpp#1134 Note that this code also only enables itself at runtime when the XPathEvaluator has been registered. I guess we could register additional document interfaces through the category manager so this would be even more dynamic, I'll leave that to someone who has time to work on this.

Peter, there is at least one other instance where XSLT can be invoked: HTTP headers.

Yeah, you're right about that issue. And it's a general issue that needs to be addressed (it was the big problem with link toolbar, for example). We need more focused document observers. Otherwise you way more notifications than you need, even *on every attribute change*. That hurts. bug 172058 is the relevant bug for that. This is why PrettyPrinter is just a module and hooks up through that nsI interface now. I'll get xpath disabled next, thanks. The comment and link order stuff is fixed.

so is there a new patch comming that includes the XPath stuff, or do you want me to review this one first?

This disables the xpath bindings as well and fixes the comment issue / link order issue.

Comment on attachment 114395 [details] [diff] [review] Patch v2.1 >@@ -500,8 +502,7 @@ > > nsDocument::nsDocument() : mSubDocuments(nsnull), > mIsGoingAway(PR_FALSE), >- mCSSLoader(nsnull), >- mXPathDocument(nsnull) >+ mCSSLoader(nsnull) Could you please put mXPathDocument before mCSSLoader so that you can put the #ifdef in the initializer-list rather then in the ctor. >Index: content/xml/document/src/nsXMLContentSink.cpp >=================================================================== >@@ -224,10 +227,12 @@ > mInTitle = PR_FALSE; > mCSSLoader = nsnull; > mNeedToBlockParser = PR_FALSE; >+ mHasProcessedBase = PR_FALSE; > mPrettyPrintXML = PR_TRUE; >+#ifdef MOZ_XSLT > mPrettyPrintHasSpecialRoot = PR_FALSE; > mPrettyPrintHasFactoredElements = PR_FALSE; >- mHasProcessedBase = PR_FALSE; >+#endif Couldn't you put the mPrettyPrintXML in the #ifdef too? >@@ -445,6 +455,7 @@ > mXSLTransformMediator = nsnull; > } > else { >+#endif > // Kick off layout for non-XSLT transformed documents. > nsCOMPtr<nsIScriptLoader> loader; > mDocument->GetScriptLoader(getter_AddRefs(loader)); If you move the '{' to after the #endif you'll save yourself an #ifdef further down. Also, the way you've done it now runs the risk that someone defines a variable inside the |else| and using it after that scope. >Index: extensions/xmlprettyprinter/Makefile.in This boilerplate looks somewhat weird. Did Aaron really make it? And it's missing some other cruft that is usually in the boilerplates. You might want to get a brand new one from the boilerplate page. Same applies to all new makefiles. >Index: extensions/xmlprettyprinter/public/Makefile.in I don't think you want this file to contain typeahead stuff :-) Oh, and nsIXMLPrettyPrinter.idl should be under MPL/GPL/LGPL, not NPL/GPL/LGPL >Index: extensions/xmlprettyprinter/src/Makefile.in >+REQUIRES = content \ >+ dom \ >+ pref \ >+ xpcom \ >+ widget \ >+ string \ >+ necko \ mind fixing these tabs? >Index: extensions/xmlprettyprinter/src/nsXMLPrettyPrinter.cpp >+NS_IMETHODIMP >+nsXMLPrettyPrinter::ContentChanged(nsIDocument* aDocument, >+ nsIContent *aContent, >+ nsISupports *aSubContent) >+{ >+ return NS_OK; >+} Could you add a |MaybeUnhook(aContent);| here, I missed that when i originally developed this thing. I'm not truly happy with calling the define |MOZ_XSLT|, i'd prefer something like |MOZ_XSLT_BINDINGS|, but it's no biggie. r=sicking

make sure you sync with bug 195886

It took me a while to get back to this because there was some problem in the tree that caused pretty printing not to work. That problem is rectified. This patch includes the changes sicking asked for except mPrettyPrintXML, which I deemed not worth the #ifdefs. I also synced with several changes to PrettyPrintXML.css and .xml (cvs is my friend, it says when people change a removed file :)

Comment on attachment 117881 [details] [diff] [review] Patch v2.2 r=sicking

Just wanted to make a note... jkeiser and I were talking and similar changes would be good for editor (see nsHTMLDocument) and Inspector (see nsInspectorCSSUtils)

Comment on attachment 117881 [details] [diff] [review] Patch v2.2 sr=alecf Just talked with jkeiser, we're moving this stuff into a new DLL in extensions.. which means a new dll, but its in extensions/ which is good.. so we'll stick with that plan, under the assumption that someday we'll be able to link all of extensions into one dll...

This patch is updated to trunk, especially the extensive xslt changes. Nonetheless, they did not affect things much. I have verified that xslt and prettyprinting work properly with the switch and do not work without the switch.

Comment on attachment 119352 [details] [diff] [review] Patch v2.3 Asking for re-r and -sr to make sure both sicking and alecf are still ok with the changes post-xslt landing. This is now more of a "compile out all xpath and xslt interfaces and move xml pretty printing to a module."

Comment on attachment 119352 [details] [diff] [review] Patch v2.3 this looks good.. however see if you can get leaf or dmose or someone to copy the cvs repository files so that we still have cvs blame on the files..

Comment on attachment 119352 [details] [diff] [review] Patch v2.3 >- nsCOMPtr<nsXMLPrettyPrinter> printer; >- nsresult rv = NS_NewXMLPrettyPrinter(getter_AddRefs(printer)); >+ nsresult rv; >+ nsCOMPtr<nsIXMLPrettyPrinter> printer = do_CreateInstance(NS_XMLPRETTYPRINTER_CONTRACTID, &rv); > NS_ENSURE_SUCCESS(rv, rv); You might want to do |if (NS_FAILED(rv)); { return NS_OK; }| here since it's an optional component. >+#ifdef MOZ_XSLT > if (mXSLTProcessor) { > nsCOMPtr<nsIDOMDocument> currentDOMDoc(do_QueryInterface(mDocument)); > mXSLTProcessor->SetSourceContentModel(currentDOMDoc); >@@ -445,6 +449,7 @@ > mXSLTProcessor = nsnull; > } > else { >+#endif It might be neater to place the '{' after the #endif here. That way you won't run into problems with different scopes with the switch on and off, and you won't have to #ifdef a bit down. >@@ -1778,11 +1791,13 @@ > mDocElement = content; > NS_ADDREF(mDocElement); > >+#ifdef MOZ_XSLT > // For XSLT, we need to wait till after the transform > // to set the root content object. > if (!mXSLTProcessor) { > mDocument->SetRootContent(mDocElement); > } >+#endif You always call |mDocument->SetRootContent(mDocElement)| when the bindings are disabled, no? >@@ -196,14 +204,16 @@ > PRPackedBool mConstrainSize; > PRPackedBool mInTitle; > PRPackedBool mNeedToBlockParser; >+ PRPackedBool mHasProcessedBase; > PRPackedBool mPrettyPrintXML; > PRPackedBool mPrettyPrintHasSpecialRoot; > PRPackedBool mPrettyPrintHasFactoredElements; >- PRPackedBool mHasProcessedBase; You could ifdef out mPrettyPrintHasSpecialRoot and mPrettyPrintHasFactoredElements too. >Index: extensions/xmlprettyprinter/resources/Makefile.in I don't think you need this file. It also seems that you no longer ifdef out building the xpath stuff in mozilla/dom. Feel free to leave some of these things and fix in a later patch. Except for the SetRootContent-call which AFAICT breaks things. r=sicking

that should read: "You *should* always call |mDocument->SetRootContent(mDocElement)| when the bindings are disabled, no?"

http://benjamin.smedbergs.us/blog/2005-07-29/the-testing-matrix/ XSLT is core web platform; even minimo wants it. WONTFIX.