filing for peterv, he created the branch.

deps and whiteboard party

I don't like the helper. Both logMessage and createRTFDocument are independent of the state of the transformation, and shouldn't need a processor state to work. How about moving those to virtual methods of txXSLTProcessor? ::getOutputHandler is different. It's state dependent and should be part of ProcessorState. Or, if we wanna think about that here, a factored ps. See http://www.mozilla.org/projects/xslt/whitepaper/xslt-states.html

I was trying to make the branch build on standalone, and came to a pretty rough area when trying to copy the OutputFormat from the old handler to the new one in ::getOutputHandler. (that method should be really called createOutputHandler, right?) I'm a bit confused about the ownership here. Maybe the output method mustn't be part of the OutputFormat. Who owns the outputformat, the processorstate or the handler?

makes the branch build on standalone unix, win is that other bug. I don't like our use of streams for input, as they loose the baseURI for the documents. How about nsresult transform(String& aXMLPath, ostream& aOut, ostream& aErr = cerr); nsresult transform(String& aXMLPath, String& aXSLPath, ostream& aOut, ostream& aErr = cerr); nsresult transform(Document* aXMLDoc, ostream& aOut, ostream& aErr = cerr); nsresult transform(Document* aXMLDoc, Document* aXSLDoc, ostream& aOut, ostream& aErr = cerr); for txStandaloneXSLTProcessor. Or should err be a ErrorObserver, or even a list of those?

Comment on attachment 95255 [details] [diff] [review] build the branch standalone checked in, I made the ProcessorState constructor not take a ostream argument and added a method for that. I need the mOut(0) and mStandaloneOutputHandler(0), though, as at least solaris doesn't init them to 0 and we have checks for that. I didn't have them in the beginning and it crashed, so I know I need them.

... the standalone API issue is pending. Not sure that txStandaloneProcessor should be all static, as we want to add parameter support to that. Which really lies in ProcessorState, but standalone doesn't expose that.

This patch takes a deep cut into the standalone API. Plus, I made the build create a static lib, which both transformiix and the XSLTMark external driver link to. Yes, one lib, two exes. I'll attach the txStandaloneXSLTProcessor.h for easier review in a second.

Comment on attachment 95569 [details] [diff] [review] standalone API, cut 1. Build static lib, two progs I added a aOut.sync_with_stdio(false); to txStandaloneXSLTProcessor::transform (the one doing the real work). This boost standalone performance by 0-50%, average 33% for XSLTMark. (This essentially doesn't make the streams output each char, though we call them char by char)

merged the latest branch check-ins. Fixes compile issues for standalone, which have been introduced by the parameter and d-o-e work. Fixed minor nits for parallel builds, sync'ing of streams

Comment on attachment 96073 [details] [diff] [review] merging, fixed a few nits checked in, with a few nits by peterv

checked in the fix for the crasher in global vars and make parameter values not owned by the processorstate. Module needs a fix, peterv said that he clones the result. TODO.

Looking at a diff for the branch, I'm kinda puzzled why we have another buffering handler. Could we reuse txUnknownHandler for module instead of DelayedNode? Module should have issues with "just text" outputs, as that's not handled gracefully in endDocument. I'll attach a patch for trivial stuff to land in a minute.

Trivial parts #1 from the branch, to land on the trunk. I just reordered the functions as they will be ordered on the branch, which cuts down the diff from *bleh* to actually readable, esp. for XSLTProcessor.cpp. This patch includes minor changes to txExpandedNameMap, adding a remove() function, so I need r/sr=

Comment on attachment 97029 [details] [diff] [review] reordering in XSLTProcessor, txExpandedName, -uw >+TxObject* txExpandedNameMap::remove(const txExpandedName& aKey) >+{ >+ TxObject* value = 0; >+ int i; >+ for (i = 0; i < mItemCount; ++i) { >+ if (mItems[i].mLocalName == aKey.mLocalName && >+ mItems[i].mNamespaceID == aKey.mNamespaceID) { You could just do |mItems[i] == aKey|. We should do that for the other functions as well, not sure why I didn't do that... >+ if (i != mItemCount) { >+ memmove(&mItems[i], &mItems[mItemCount], sizeof(MapItem)); >+ } Use memcpy since you know you won't have any overlap. Assuming that the rest is *just* moving functions around it looks good, the patch was pretty much unreadable.

the txExpandedNameMap holds |MapItem|s, not txExpandedNames, so I can't do the simple compare. I did the memcpy, though. To make this clear, this is stuff that is on the branch already, but doesn't really change things. Like the reordering in XSLTProcessor. Once we land this on the trunk, the diff of branch vs. trunk will show the real changes, so the stuff that needs review there will pop out.

Comment on attachment 97029 [details] [diff] [review] reordering in XSLTProcessor, txExpandedName, -uw txExpandedNameMap::remove's return behavior is odd... it returns 0 if the deleted item was owned and the item otherwise. Why?

if the list owns the items, the value is deleted and therefor, you can't return a reference to it. If the list does not own the values, you can return the value, and so we do.

My point is, maybe it's better to never return the value?

Comment on attachment 97029 [details] [diff] [review] reordering in XSLTProcessor, txExpandedName, -uw sticked to return the value if we can, as that's what remove() commonly does. noting sr=bz and obsoleting, as I checked it in.

Hrm. we call loader->Suspend() if we don't have an observer. For the transformNode(), we actually do that twice. But we never call Resume(), that looks wrong to me. Do we really want scripts not loaded and not evaluated, if we don't have an observer? I'm not sure. I can't make up a way how it should work, but I guess that it's bad that it doesn't work at all.

How about adding addObserver(in nsITransformObserver) and removeObserver(in nsITransformObserver) to nsIXSLTProcessor for those that want scripts loaded and run?

This splits off sicking's context node removal from the branch. We passed around the context node without using it most of the time, and where we did use it we could just get it through the ProcessorState.

Comment on attachment 97220 [details] [diff] [review] Contextnode removal r=axel@pike.org

IMHO we shouldn't execute scripts when transforming through the nsIXSLTProcessor interface. For the same reason that when you load a document using XMLHttpRequest we shouldn't be executing scripts in it. If we do decide anyway that we want to execute script then that should IMHO be settable through a property. I'm also not sure if really need to have some mechanism for notifying back to the user that all scripts have been loaded and evaluated. In any case I think we could do without any of this this first iteration. We can always revisit for 1.2beta

Comment on attachment 97220 [details] [diff] [review] Contextnode removal sr=bzbarsky

Comment on attachment 97315 [details] [diff] [review] Cleanup r=axel@pike.org

Comment on attachment 97315 [details] [diff] [review] Cleanup Checked in with rs=jst (no functional changes).

content/xsl/document/public and content/xsl/public need to have different XPIDL_MODULEs, as we're only generating one typelib of a given name, and don't append. At least on gmake builds. I did a XPIDL_MODULE = content_xsltdoc in content/xsl/document/public locally, which helped to get the classinfo stuff right, and scripting.

hmm.. we might wanna move the files instead IMHO content/xsl/public sounds like the right place to me

Ok, here goes a big one. This does some pretty big surgury to how we handle and set up the outputhandlers. Basically I needed to do that to be able to handle having different outputhandlers for different entrypoints in module. So the way it now works is that whoever calls txXSLTProcessor::transform has to first set the mOutputHandlerFactory property of the processorstate. That is an implementation of txIOutputHandlerFactory which has two getHandlerFor methods. One of the functions only specifies an txOutputMethod and the other additionally specifies a root name and namespace. The txUnknownHandler has been generalized a bit to be able to work in module too. It is now the one in change of detecting if xml or html should be used as outputmethod if none is specified in the stylesheet. When it gets it's first request it simply uses the txIOutputHandlerFactory interface to get the appropriate outputhandler and then replaces itself with that and flushes all stacked transactions. This also allows us to get rid of the txXSLTProcessor::startElement function! I've also removed the txStreamXMLEventHandler and txIMozillaXMLEventHandler interfaces since the new factory removes the need for those interfaces, instead everything can be set in the different outputhandlers ctors. The only non-neatness left is the fact that the outputhandler in module is refcounted whereas in module it is (of course) not. We need that since the scriptloader will a) require us to be refcounted since we'll implement nsIScriptLoaderObserver and b) keep us alive as long as any scripts are being loaded. There is a way to solve this, and that is to not have txMozillaXMLOutput implement nsIScriptLoaderObserver, but instead let it hold onto an internal object which takes care of keeping track of all loading scripts and notify the contentsink when it's done. However i'm not sure if I should do that in this patch though. I think I would like to land this on the branch first and then do that change if we want to. Another thing that I would like to do is to have txMozillaTextOutput not create it's document in ::startDocument but rather in the ctor like txMozillaXMLOutput does. That way we get a cleaner separation for when we don't create the document but instead transform into a fragment. However that we could do after landing on the trunk since it's just a beautification.

same as above with -w. (oh, and this one contains the comments for txIOutputHandlerFactory). The txMozillaXMLOutput.cpp changes should be easier to review in this much of that is removing |if|s

The only new thing in this patch is: @@ -768,7 +761,9 @@ rv = parser->QueryInterface(NS_GET_IID(nsIStreamListener), (void**)getter_AddRefs(sl)); if (NS_FAILED(rv)) return rv; - rv = NS_OpenURI(sl, nsnull, aUrl); + nsCOMPtr<nsILoadGroup> loadGroup; + mDocument->GetDocumentLoadGroup(getter_AddRefs(loadGroup)); + rv = NS_OpenURI(sl, nsnull, aUrl, nsnull, loadGroup); return rv; } in the XML contentsink. And syncing with tip

XSLTProcessorModule.cpp adds DOMCI info for nsIDocumentTransformer, which isn't scriptable anymore. Which in itself is a good thing, peterv and I see that interface turning into content-only, so that we can easily change it without breaking stuff. If we add a transformDocument method for compat, it should go into nsIXSLTProcessor now. With a deprecated comment. I wonder if txMozillaTextOutput should be HTML. Document and content. Plain text files are, peterv is all for keeping it XML/XHTML. Jonas, throw a dice.

XHTML! :)

Jonas, the stuff in content/xsl/public is out of date, AFAICT.

I removed the scoping blocks for the processorstates in txMozillaXSLTProcessor. C++ requires that objects are destroyed in reversed order.

this is how i think we should suffel the obsolete and internal interfaces. Basically it doesn't touch nsIDocumentTransformer more then changing the last argument to be an nsISupports rather then an observer. And then creates a separate .h which contains everything that the xml-content needs. If we want to rename any interfaces then i'm ok with that

Comment on attachment 100388 [details] [diff] [review] diff to content opps, this patch should cvs-remove nsITransformObserver.idl as well.

this'll make it easier and cleaner to implement the old-style transformDocument

Comment on attachment 100391 [details] [diff] [review] patch to txMozillaTextOutput checked in

this together with attachment 100388 [details] [diff] [review] makes us implement nsIDocumentTransformer and nsIXSLTProcessorInternal

re attachments 100388 and 100402: Naming: As the architecture in content, namely nsTransformMediator and nsITransformObserver, use the terminology "transform", I would like to keep the nsIDocumentTransformer interface name for use in content. As it changes from scriptable to non-scriptable, it should have a different CID than the trunk. I can go with it being defined in a .h. But the .h should contain a NS_DECL, just like idl generated .h's do. As js doesn't access the interface due to class info, but just the method signature, a change of the interface name containing the transformDocument signature for compatibility doesn't matter. I suggest using nsIXSLTProcessorObsolete, basically because it's a few chars less than nsIXSLTProcessorDeprecated. This plays much nicer with the nsIXSLTProcessor interface name, and clearly indicates the use and justification for another interface. + // If we don't have an observer we are parsing into a non-displayed + // new document, so textoutput doesn't make sence. well, sence ain't important, but sense is. We want stuff like this for applications like buster. Not that this is this patch's fault, but IMHO it's wrong. The transform mediator still uses the scriptable interface in attachment 100388 [details] [diff] [review]. content/xsl/document/public should die.

I don't see any reason to change the IID of the old interface. If we keep the current one we get C++ compatibility for free and since that interface exists for the sole purpose of compatibility i say why not. I'm ok with renaming nsIDocumentTransformer -> nsIXSLTProcessorObselete nsIXSLTProcessorInternal -> nsIDocumentTransformer peterv?

Comment on attachment 100388 [details] [diff] [review] diff to content oh, and this needs some mac makefile love

ok, as C++ compat is just IID and vtable, which works as long as we bail for 0, I'm fine with leaving the IID of trunk::nsIDocumentTransformer for branch::nsIXSLTProcessorInternal. Talked with peterv about this, so let's rename the interfaces as said in #45. /me eagerly waits to see this glued together, and content/xsl/document/public dead.

this is the patch between the branch and the tip

sorry, had temporarily disabled -N. This one is with all new files

build/mac stuff is missing, ie. changing the xsl:document:public links to xsl:public. I guess that's just two lines. petermacv? AFAICT, nsIDocumentTransformer should die in XSLTProcessorModule. Do we need AddCategoryEntry for nsIXSLTProcessorObsolete? should transformiix.cpp use nsBuildID? return NS_SUCCEEDED(rv); in main returns 1 on success. UNICODE_CHAR turned into char when XSLTProcessor::parseStylesheetPI -> txStandaloneXSLTProcessor::parseStylesheetPI mo' to come, I have some comment nits in my tree as well, which I will just land once I'm done.

txToDocHandlerFactory::createHandlerWith text output does make sense to me. At least for buster, which should use transformToDocument soon after this lands. txMozillaXSLTProcessor has a + NS_INTERFACE_MAP_ENTRY(nsIDocumentTransformer) txMozillaXSLTProcessor::TransformDocument asks + // Do we really need this now that this is not a scriptable function? I'd say no. nsIXSLTProcessor has ::init, I'd prefer ::importStylesheet. built gmake standalone. checked in minor cleanups, a current version for transformiix.cpp and fixed a few comments in xslt. Guess I'm thru the patch. Note, I didn't build module.

> txToDocHandlerFactory::createHandlerWith > text output does make sense to me. At least for buster, which should use > transformToDocument soon after this lands. Hmm.. good point. Either we should have buster use transformToFragment or we need to come up with something sane to do for transformToDocument. Maybe wrapping a textnode in a <transformiix:result> element might be a good idea. That's what we do for other things in transformToDocument. > txMozillaXSLTProcessor has a > + NS_INTERFACE_MAP_ENTRY(nsIDocumentTransformer) Err.. it has to, otherwise we can't QI to nsIDocumentTransformer. However i did remove the NS_DOMCI_EXTENSION_ENTRY_INTERFACE(nsIDocumentTransformer) from XSLTProcessorModule since that interface shouldn't be part of DOMCI. > txMozillaXSLTProcessor::TransformDocument asks > + // Do we really need this now that this is not a scriptable function? > I'd say no. Agreed, i'll remove it. > nsIXSLTProcessor has ::init, I'd prefer ::importStylesheet. Hmm.. how about addStylesheet?

the spec says: <q> This document does not specify how an XSLT stylesheet is associated with an XML document. It is recommended that XSL processors support the mechanism described in [XML Stylesheet]. When this or any other mechanism yields a sequence of more than one XSLT stylesheet to be applied simultaneously to a XML document, then the effect should be the same as applying a single stylesheet that imports each member of the sequence in order (see [2.6.2 Stylesheet Import]). </q> so I'm for having the "import" in the function name. To say what it does. Another argument against add is that we should have a remove then. And I doubt that I like that. Not fundamentally against it, but it may block one or the other neat thing we could do when constructing the processorstate.

A comment for drivers, I just checked duplicates.xul, and bug 130161, html is emulated by xhtml has 7 duplicates. That's the only XSLT bug showing up on duplicates.xul, so this is really topnotch in user experience.

IMHO we shouldn't hold this for bug 169155. Lets try to fix that and remove the dependency on layout asap after this lands instead

Comment on attachment 102779 [details] [diff] [review] fixes pikes comments checked in. Also added throwing execption in importStylesheet if mStylesheet is already set

> Tabs in XSLTProcessorModule.cpp. fixed, added modeline. > Need to close mOut? fixed. > Why not globVar->mFlags == GlobalVariableValue::evaluating? ... and friends. Changed the enum to {none=0, evaluating, owned}, as it can be only in one of those states. I reordered them to reflect the state changes. The if's are changed, too. (the deletion is just a if (mFlags) now) > "through" and remove the space after "the" > wrapper instead of head done > NSIMETHOD_IMP? This would need to be changed in txIOutputHandlerFactory, too. But this isn't a XPCOM interface, so I don't think we need it (now).

>Index: source/xslt/txStandaloneXSLTProcessor.h >=================================================================== >RCS file: source/xslt/txStandaloneXSLTProcessor.h >diff -N source/xslt/txStandaloneXSLTProcessor.h >--- /dev/null 1 Jan 1970 00:00:00 -0000 >+++ source/xslt/txStandaloneXSLTProcessor.h 15 Oct 2002 15:21:08 -0000 >+ /** >+ * Transform a XML document given by path. >+ * The stylesheet is retrieved by a processing instruction, >+ * or an error is returned. >+ * >+ * @param aXMLPath path to the source document >+ * @param aOut stream to which the result is feeded fed (also in other places in txStandaloneXSLTProcessor.h)

Comment on attachment 101687 [details] [diff] [review] content >+[scriptable, uuid(e06dfaea-92d5-47f7-a800-c5f5404d8771)] >+interface nsIXSLTException : nsIException { >+ /* >+ * The node in the stylesheet that triggered the exception. The new idl files should use /** for comments.

I checked in some changes to the parameters code. Basically my review comments. Hope you don't mind me checking it in directly. There's one more comment i have, but we could take that in a later bug if we decide to do something about it. Why lazy convert nsIVariant->ExprResult. Seems like it is farily unlikly that someone is going to set a parameter that is never used, and if it does happen i doubt that it'll be so many that cycles or memory matters. By not lazyinitilizing we can get better errors from SetParameter, and we could get rid of one of the switchstatements. I'd be perfectly happy with seing that done in a separate bug though.

> > void txMozillaXMLOutput::endDocument() > > { > > closePrevious(eCloseElement | eFlushText); > >- if (!mHaveTitleElement) { > >+ // XXX is this really needed since we reset the document? > > IIRC it was, did you check if it still is? It seems like it still is. nsDocument::SetTitle does a bunch of things that doesn't happen in Reset. I added a similar call to txMozillaTextOutput::createResultDocument. > Why move TX_ENSURE_CURRENTNODE? My misstake. Moved back. > Why remove this return? Don't know how that happened. I've put it back (together with another one a couple of lines up) > Why remove the check on cont? because all elements implements nsIContent, we rely on that all over the place. Added an assertion though. > We don't load scripts when transforming into an existing document? > or set the title, handle base and meta? Right, mCreatingNewDocument is false when and only when we're transforming into a fragment. When the output of the XSLT creates a <title> or similar that shouldn't IMHO affect the document that is used to create that fragment. Ideally that should be handled when that fragment is inserted into the document, although i know that for some things (like <meta>) that doesn't happen. However it seems worse to me if that happened during the creation of the the fragment. My general idea was that transforming into a fragment should never affect anything outside of that fragment, i.e. the document, so whereever we poked at the document directly i added that check. > Will this work? Seems like we always have mDocument except for OOM? Good point. Not sure what we should do here, but at least we should be able to deal with mDocument being null... > >+txToDocHandlerFactory::createHandlerWith(txOutputFormat* aFormat, > >+ const String& aName, > >+ PRInt32 aNsID, > >+ txIOutputXMLEventHandler*& aHandler) > > Why not txIOutputXMLEventHandler**? Fixed Unless something drastic happens i would rather put off merging the TransformX functions. It's late and i need sleep.

everything i didn't comment on was either fixed or answered/fixed by Pike

This should include all reviewcomments from everybody. The stuff that is in extesions/transformiix also lives on the branch.

+++ content/xsl/public/nsIXSLTProcessor.idl + * @param style The root-node of a XSLT stylesheet. This can be either + * a document node or an element node. If a document node + * then the document can contain either a XSLT stylesheet + * or a LRE stylesheet. If the argument is a document node... + * Transforms the node source applying the stylesheet given by ... applying the stylesheets given by ... All comments imply that there's only one stylesheet. Which is true right now. Should we adjust the comments now, or when we support multiple ones? Index: extensions/transformiix/resources/contents.rdf - chrome:localeVersion="1.2b" + chrome:localeVersion="1.2a" probably not.

With this it builds on mac.

Comment on attachment 103161 [details] [diff] [review] the whole enchilada r=axel@pike.org, with the comments in #67, #68

Comment on attachment 103161 [details] [diff] [review] the whole enchilada sr=peterv

i'll land this

Test results (1666 tests run): Success Failure Trunk: 1388 268 With patch: 1400 266

Comment on attachment 103161 [details] [diff] [review] the whole enchilada a=asa for checkin to 1.2 (on behalf of drivers).

check in!! We rock! :)

mass verifying