155578 - (txbranch) branch for refactoring XSLTProcessor and interfaces

Status: VERIFIED FIXED

(Core :: XSLT, defect, P2)

Description

[Axel Hecht]

filing for peterv, he created the branch.

Comment 1

[Axel Hecht]

deps and whiteboard party

Comment 2

[Axel Hecht]

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

Comment 3

[Axel Hecht]

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?

Comment 4

[Axel Hecht]

Attachment: build the branch standalone

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 5

[Axel Hecht]

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.

Comment 6

[Axel Hecht]

... 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.

Comment 7

[Axel Hecht]

Attachment: standalone API, cut 1. Build static lib, two progs

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 8

[Axel Hecht]

Attachment: txStandaloneXSLTProcessor.h

Comment 9

[Axel Hecht]

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)

Comment 10

[Axel Hecht]

Attachment: merging, fixed a few nits

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 11

[Axel Hecht]

Comment on attachment 96073 [details] [diff] [review]
merging, fixed a few nits

checked in, with a few nits by peterv

Comment 12

[Axel Hecht]

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.

Comment 13

[Axel Hecht]

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.

Comment 14

[Axel Hecht]

Attachment: reordering in XSLTProcessor, txExpandedName, -uw

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 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 16

[Axel Hecht]

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 17

[Boris Zbarsky [:bzbarsky]]

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?

Comment 18

[Axel Hecht]

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.

Comment 19

[Boris Zbarsky [:bzbarsky]]

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

Comment 20

[Axel Hecht]

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.

Comment 21

[Axel Hecht]

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.

Comment 22

[Axel Hecht]

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

Comment 23

[Peter Van der Beken [:peterv]]

Attachment: Contextnode removal

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 24

[Axel Hecht]

Comment on attachment 97220 [details] [diff] [review]
Contextnode removal

r=[e-mail]

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 97220 [details] [diff] [review]
Contextnode removal

sr=bzbarsky

Comment 27

[Peter Van der Beken [:peterv]]

Attachment: Cleanup

Comment 28

[Axel Hecht]

Comment on attachment 97315 [details] [diff] [review]
Cleanup

r=[e-mail]

Comment 29

[Peter Van der Beken [:peterv]]

Comment on attachment 97315 [details] [diff] [review]
Cleanup

Checked in with rs=jst (no functional changes).

Comment 30

[Axel Hecht]

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.

Comment 31

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

hmm.. we might wanna move the files instead IMHO

content/xsl/public sounds like the right place to me

Comment 32

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: output love, take 1

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.

Comment 33

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: output love, take 1 -w

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

Comment 34

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: diff to content

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

Comment 35

[Axel Hecht]

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.

Comment 36

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

XHTML! :)

Comment 37

[Axel Hecht]

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

Comment 38

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 39

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: diff to content

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 40

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 100388 [details] [diff] [review]
diff to content

opps, this patch should cvs-remove nsITransformObserver.idl as well.

Comment 41

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: patch to txMozillaTextOutput

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

Comment 42

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

checked in

Comment 43

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: patch to implement nsIDocumentTransformer

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

Comment 44

[Axel Hecht]

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.

Comment 45

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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 46

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 100388 [details] [diff] [review]
diff to content

oh, and this needs some mac makefile love

Comment 47

[Axel Hecht]

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.

Comment 48

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: the whooole shebang (except for content)

this is the patch between the branch and the tip

Comment 49

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: the whooole shebang (except for content) with -N

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

Comment 50

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: content

Comment 51

[Axel Hecht]

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.

Comment 52

[Axel Hecht]

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.

Comment 53

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> 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?

Comment 54

[Axel Hecht]

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.

Comment 55

[Axel Hecht]

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.

Comment 56

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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 57

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: fixes pikes comments

Comment 58

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 102779 [details] [diff] [review]
fixes pikes comments

checked in. Also added throwing execption in importStylesheet if mStylesheet is
already set

Comment 59

[Peter Van der Beken [:peterv]]

Attachment: First comments

Comment 60

[Axel Hecht]

> 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).

Comment 61

[Peter Van der Beken [:peterv]]

>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 62

[Peter Van der Beken [:peterv]]

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.

Comment 63

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 64

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> > 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.

Comment 65

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 66

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: the whole enchilada

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

Comment 67

[Axel Hecht]

+++ 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.

Comment 68

[Peter Van der Beken [:peterv]]

Attachment: Remove nsIDocumenttransformer.idl from contentIDL.xml

With this it builds on mac.

Comment 69

[Axel Hecht]

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

r=[e-mail], with the comments in #67, #68

Comment 70

[Peter Van der Beken [:peterv]]

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

sr=peterv

Comment 71

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

i'll land this

Comment 72

[Peter Van der Beken [:peterv]]

Test results (1666 tests run):

            Success Failure
Trunk:       1388    268
With patch:  1400    266

Comment 73

[Asa Dotzler [:asa]]

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

a=asa for checkin to 1.2 (on behalf of drivers).

Comment 74

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

check in!! We rock! :)

Comment 75

[Axel Hecht]

mass verifying