193678 - support exslt:common

Status: RESOLVED FIXED

(Core :: XSLT, enhancement)

Description

[Axel Hecht]

Peter wants to do this, probably partly based on the rtf bug he's working on.
Do we want to XPCOMify extensions? How about using contractIDs like
"@mozilla.org/projects/xslt/functions;1?"extension-name-space and same for 
elements?

Comment 1

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

I don't think there is a need to build an entire XPCOM-extension-system just
yet. For now I would prefer if we just created those extension-functions like we
create the XSLT functions inside ::resolveFunctionCall. 

Comment 2

[Axel Hecht]

about XPCOMifying. It's cute on the first look, useless on the second and cute
on the third.
1) xpcom let's you add extensions.
2) that would require all involved interfaces to be xpcom, or people to link
against transformiix. That requires at least a stable content model. As if. ;-)
3) having xpcom and contractIDs makes it easier to add extensions and hook them
up. Then extensions could go into separate dirs and would just need to hook
into the module registration.

Biggest issue: runtime namespace resolution. Looks like we eventually want to 
refcount parsing contexts and give them into resolveFunctionCall. (Currently
some elements store mStyleElement for that.)

Other general question, should extension functions be available to xslt only,
or to XPath, too?

Comment 3

[Axel Hecht]

*** Bug 215242 has been marked as a duplicate of this bug. ***

Comment 4

[Peter Van der Beken [:peterv]]

Attachment: v1 (WIP)

Module-only.

Comment 5

[Jess Holle]

At least the nodeset extension function is a *must* for complex XSLT -- unless,
of course, full XSLT 2.0 is easier -- which is doubtful.

Comment 6

[Jess Holle]

Any thoughts of ever getting this into Firefox?

I'd love to be able to start offloading XSLT to the client, but MSIE's XSLT
crashes or hangs in complex use cases (on the 2nd load only) and Firefox and
Mozilla simply don't have enough oomph here, i.e. I need nodeset at a minimum.

Comment 7

[G. Ken Holman]

My only extension requirement is also just the node-set() function, translating 
a result tree fragment into a node-set for processing purposes through template 
rules.  If no other extension were implemented, I would not be handcuffed as I 
am today.  I hope you'll up the priority on at least this one function.  I will 
be publishing some stylesheets that make heavy use of this, and will note this 
later once posted. Thanks!

Comment 8

[Jess Holle]

Note that XSLT 2.0 essentially makes all result fragments node sets. 
Essentially the XSLT community has admitted that result fragments are too
limiting for the real world.  Until XSLT 2.0, the only solution to such
situations is node-set, which makes this one extension *critical*.

Comment 9

[Peter Van der Beken [:peterv]]

Adding more comments to a bug is not going to get it fixed faster.

Comment 10

[Rein Petersen]

node-set() is the most important function

Comment 11

[Thomas Runkel]

Is there any progress with this bug?

At least node-set() would be fine.

Comment 12

[Peter Van der Beken [:peterv]]

See comment 9.

Comment 13

[Julian Reschke]

Well. But there seem to be a patch available, so it's unclear to me why there's no progress on this issue.

Comment 14

[Peter Van der Beken [:peterv]]

The patch isn't complete, that's why it's marked WIP.

Comment 15

[Jess Holle]

Though I understand more comments does not get this fixed faster (as per comment 9), more comments clearly demonstrates a demand for this fix.  Unfortunately the demand cannot directly translate into effort as not all of us have the right combination of time, energy, and wherewithal to actually do the fix.

While I understand we're not paying for Firefox, it is somewhat surprising that this has not been addressed in over 3 years with all the advancements that have occured in the Mozilla/Firefox browsers.

Comment 16

[Holger Will]

i have seen comment #9 ,  i still like to add to the point, that Opera does also have support for node-set() now.

Comment 17

[Peter Van der Beken [:peterv]]

Attachment: v2

Note that there's some overlap with the fix for bug 354886. I will add a comment explaining the addRoots function, basically it's used to keep some nodes alive for as long as we're processing (XPath or XSLT). It is needed because some extension functions return nodesets with newly created nodes, and nodesets are non-owning so somebody needs to keep those nodes alive.

Comment 18

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

Comment on attachment 240855 [details] [diff] [review]
v2

One thing that I am worried about is that once we start implementing exslt functions like str:tokenize or dyn:map we'll end up creating lots and lots of temporary nodesets that won't be killed until the end of the transformation. Not sure what to do about it though, other than to add refcounting... Maybe it's a problem we should try to solve later.

>Index: content/xslt/src/xpath/nsXPathExpression.cpp
>+PRBool nsXPathExpression::EvalContextImpl::addRoot(txNativeNode aNode)

What's the purpose of txNativeNode? Why not simply pass an txXPathNode& ? That's what we do for the owned documents loaded from document().

>Index: content/xslt/src/xpath/txXPCOMExtensionFunction.cpp
>+txFunctionEvaluationContext::AddRoot(nsIDOMNode *aNode)

Is this function really needed by extension functions? Would it maybe be better to supply functions for creating nodesets of various contents? I don't feel strongly either way as long as there are usecases.

>Index: content/xslt/src/xslt/txEXSLTFunctions.cpp
>+convertRtfToNode(txIEvalContext *aContext, txResultTreeFragment *aRtf)

>+    nsCOMPtr<nsIDOMDocumentFragment> domFragment;
>+    rv = domDoc->CreateDocumentFragment(getter_AddRefs(domFragment));
>+    NS_ENSURE_SUCCESS(rv, rv);

Alternatively just call NS_NewDocumentFragment and grab the nimgr from |document|.

>+    txOutputFormat format;
>+    txMozillaXMLOutput mozHandler(&format, domFragment);

I think you may want to signal to the handler that it shouldn't do any fixups of the DOM whatsoever. Like we don't want to insert <tbody> elements and such. Not sure if we actually would seing that XHTML has less fixup than HTML, but we may in the future. Would feel safer if we never called startHTMLElement/endHTMLElement at all in this case.

>+    // The txResultTreeFragment will own this.
>+    const txXPathNode* node = txXPathNativeNode::createXPathNode(domFragment);
>+    NS_ENSURE_TRUE(node, NS_ERROR_OUT_OF_MEMORY);
>+
>+    aRtf->setNode(node);

Why stick this in the rtf? In case someone calls exslt:node-set() on the same rtf multiple times?

>+createTextNode(txIEvalContext *aContext, nsString& aValue,
>+    nsCOMPtr<nsIDOMText> domText;
>+    rv = domDoc->CreateTextNode(aValue, getter_AddRefs(domText));
>+    NS_ENSURE_SUCCESS(rv, rv);

NS_NewTextNode?

>+txEXSLTObjectTypeFunctionCall::evaluate(txIEvalContext *aContext,
...
>+    switch (exprResult->getResultType()) {

You could do this using a table of strings. See txLiteralExpr.cpp


>Index: content/xslt/src/xslt/txRtfHandler.h

>+    const txXPathNode& getNode() const
>+    {
>+        NS_ASSERTION(mNode, "Not converted!");
>+
>+        return *mNode;
>+    }
>+    PRBool isConverted() const
>+    {
>+        return !!mNode;
>+    }

You could get rid of isConverted if you made getNode return a pointer.


>Index: content/xslt/src/xslt/txStylesheetCompiler.cpp

>+extern nsresult TX_EXSLTCommon(nsIAtom* aName, FunctionCall** aResult);

>+static nsresult
>+findFunction(nsIAtom* aName, PRInt32 aNamespaceID, FunctionCall** aResult)
>+{
>+    if (kExtensionFunctions[0].mNamespaceID == kNameSpaceID_Unknown) {
>+        PRUint32 i;
>+        for (i = 0; i < NS_ARRAY_LENGTH(kExtensionFunctions); ++i) {
>+            txFunctionFactoryMapping& mapping = kExtensionFunctions[i];
>+            NS_ConvertASCIItoUTF16 namespaceURI(mapping.mNamespaceURI);
>+            mapping.mNamespaceID =
>+                txNamespaceManager::getNamespaceID(namespaceURI);

No need for the temporary, just do
txNamespaceManager::getNamespaceID(NS_ConvertASCIItoUTF16(mapping.mNamespaceURI))

>+txStylesheetCompilerState::resolveFunctionCall(nsIAtom* aName, PRInt32 aID,
>+                                               FunctionCall **aFunction)
>+{
>+    *aFunction = nsnull;
>+
>+    if (aID != kNameSpaceID_None) {
>+        nsresult rv = findFunction(aName, aID, aFunction);
>+        if (rv == NS_ERROR_XPATH_UNKNOWN_FUNCTION) {
>+            *aFunction = new txErrorFunctionCall(aName, aID);
>+            rv = *aFunction ? NS_OK : NS_ERROR_OUT_OF_MEMORY;
>         }
>+
>+        return rv;
>+    }
>+
>+    if (aName == txXSLTAtoms::document) {

If you pass mElementContext to the txFunctionFactory as an argument you could do the xslt functions the same way that the exslt functions are done. It's pretty likely that some of the exslt functions will need stuff from the mElementContext in the future anyway (such as the namespace mappings to do the dynamic functions).

The downside of doing the xslt functions the same way as the exslt ones is that you'll have to add the pile of |if (aResult)| statements. However IMHO it'd be fine to even from TX_XSLTFunctionAvailable actually create the function and just throw it away. Ideally we should at some point optimize constant expressions like "function-available('foo:bar')" anyway.

Of course, alternatively we could just add to the current list in txXSLTEnvironmentFunctionCall::evaluate. At this point that's still less code.

Up to you. But if you're keeping the current convention, please document somewhere that aFunction can be null and how the implementation should behave.

>Index: content/xslt/src/xslt/txXSLTEnvironmentFunctionCall.cpp

>+                val = NS_SUCCEEDED(TX_XSLTFunctionAvailable(qname.mLocalName,
>+                                                            qname.mNamespaceID));

Please move the NS_SUCCEEDED call into TX_XSLTFunctionAvailable and make it return a PRBool instead.

Would like to see a new patch for the txMozillaXMLOutput changes.

Comment 19

[Peter Van der Beken [:peterv]]

(In reply to comment #18)
> One thing that I am worried about is that once we start implementing exslt
> functions like str:tokenize or dyn:map we'll end up creating lots and lots of
> temporary nodesets that won't be killed until the end of the transformation.
> Not sure what to do about it though, other than to add refcounting... Maybe
> it's a problem we should try to solve later.

You do mean that the nodes will stay alive, not the nodesets, right? The only alternative is to store a bit in the txXPathNode's that means that the root ancestor of the node needs to be refcounted. It shouldn't affect the treewalking much, since we only walk the subtree of the root ancestor but in general it will cost a bit even when someone doesn't use the extension functions.

> >Index: content/xslt/src/xpath/nsXPathExpression.cpp
> >+PRBool nsXPathExpression::EvalContextImpl::addRoot(txNativeNode aNode)
> 
> What's the purpose of txNativeNode? Why not simply pass an txXPathNode& ?
> That's what we do for the owned documents loaded from document().

It avoids creating txXPathNode's just to get the nsINode out of them again.

> >Index: content/xslt/src/xpath/txXPCOMExtensionFunction.cpp
> >+txFunctionEvaluationContext::AddRoot(nsIDOMNode *aNode)
> 
> Is this function really needed by extension functions? Would it maybe be better
> to supply functions for creating nodesets of various contents? I don't feel
> strongly either way as long as there are usecases.

Unless we start refcounting the roots in txXPathNode it is.

> >+    aRtf->setNode(node);
> 
> Why stick this in the rtf? In case someone calls exslt:node-set() on the same
> rtf multiple times?

Yes, can just create a new one every time instead if you prefer.

> >+            NS_ConvertASCIItoUTF16 namespaceURI(mapping.mNamespaceURI);
> >+            mapping.mNamespaceID =
> >+                txNamespaceManager::getNamespaceID(namespaceURI);
> 
> No need for the temporary, just do
> txNamespaceManager::getNamespaceID(NS_ConvertASCIItoUTF16(mapping.mNamespaceURI))

The line gets way too long.

Comment 20

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

(In reply to comment #19)
> (In reply to comment #18)
> > One thing that I am worried about is that once we start implementing exslt
> > functions like str:tokenize or dyn:map we'll end up creating lots and lots 
> > of temporary nodesets that won't be killed until the end of the 
> > transformation. Not sure what to do about it though, other than to add
> > refcounting... Maybe it's a problem we should try to solve later.
> 
> You do mean that the nodes will stay alive, not the nodesets, right? The only
> alternative is to store a bit in the txXPathNode's that means that the root
> ancestor of the node needs to be refcounted. It shouldn't affect the
> treewalking much, since we only walk the subtree of the root ancestor but in
> general it will cost a bit even when someone doesn't use the extension
> functions.

Yeah, that sounds like a good idea. Maybe something that could wait until this patch has landed though...

> > >+    aRtf->setNode(node);
> > 
> > Why stick this in the rtf? In case someone calls exslt:node-set() on the 
> > same rtf multiple times?
> 
> Yes, can just create a new one every time instead if you prefer.

Nah, it seems like a good idea. Especially to avoid creating more nodes than necessary.

Comment 21

[Peter Van der Beken [:peterv]]

Attachment: v3 (Allow txXPathNode to hold a strong ref to its root)

Comment 22

[Peter Van der Beken [:peterv]]

Attachment: v3 (exslt:common functions implementation)

This one should be diff-able against previous versions.

Comment 23

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

Please use a real C++ bitfield rather than doing all the bit operations by hand. That should be more readable. I.e. do something like:

PRUint32 mIndex : 31;
PRUint32 mRefCountRoot : 1;

No need for a new patch yet though, still looking...

Comment 24

[Peter Van der Beken [:peterv]]

Attachment: v3.1 (Allow txXPathNode to hold a strong ref to its root)

Comment 25

[Peter Van der Beken [:peterv]]

Attachment: v3.2 (Allow txXPathNode to hold a strong ref to its root)

Comment 26

[Peter Van der Beken [:peterv]]

Attachment: v3.3 (Allow txXPathNode to hold a strong ref to its root)

r/sr=sicking.

Comment 27

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

Comment on attachment 245342 [details] [diff] [review]
v3 (exslt:common functions implementation)

mNoFixup is never set, i.e. you never initialize it to false nor set it to true for exslt:node-set.

>Index: content/xslt/src/xslt/txEXSLTFunctions.cpp

>+static const char * const sTypes[] = {
>+  "node-set",

Put a comment here and in txExprResult.h noting that the order here is dependant on the enum so that if someone changes one place they change both.

The naming between findXSLTFunction and TX_EXSLTCommon is a bit inconsistent. I prefer something like TX_ConstructXSLTFunction/TX_ConstructEXSLTCommonFunction or something. No big deal as long as it's consistent.

With that, r=me.

Comment 28

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

You should also not do the script fixups if mNoFixup is set, both in endElement and startElementInternal

Comment 29

[Jess Holle]

So when will this be in a Firefox release?  Firefox 2.0.x?  That's too much to ask I assume.  Firefox 3.0?

Comment 30

[[e-mail]]

I couldn't find the patch that added this code which was blamed to this bug:
+typedef nsresult (*txFunctionFactory)(nsIAtom* aName,
+                                      txStylesheetCompilerState* aState,
+                                      FunctionCall** aResult);
+struct txFunctionFactoryMapping
+{
+    const char* const mNamespaceURI;
+    PRInt32 mNamespaceID;
+    txFunctionFactory mFactory;
+};
+
+extern nsresult
+TX_ConstructEXSLTCommonFunction(nsIAtom *aName,
+                                txStylesheetCompilerState* aState,
+                                FunctionCall **aResult);
+
+static txFunctionFactoryMapping kExtensionFunctions[] = {
+    { "", kNameSpaceID_Unknown, TX_ConstructXSLTFunction },
+    { "http://exslt.org/common", kNameSpaceID_Unknown,
+      TX_ConstructEXSLTCommonFunction }
+};
My C++ fu isn't up to knowing whether you are allowed to have a const member of a POD type (const char* mNamespaceURI; would definitely be OK).

Comment 31

[Peter Van der Beken [:peterv]]

(In reply to comment #29)
> Firefox 2.0.x?

No.

> Firefox 3.0?

Yes.

(In reply to comment #30)
> I couldn't find the patch that added this code which was blamed to this bug:

And yet attachment 245342 [details] [diff] [review] contains this code.

Comment 32

[[e-mail]]

(In reply to comment #31)
>(In reply to comment #30)
>>I couldn't find the patch that added this code which was blamed to this bug:
>And yet attachment 245342 [details] [diff] [review] contains this code.
Sorry, I must have mistyped my search string, or something.

Comment 33

[Eric Shepherd [:sheppy]]

The entry on the Firefox 3 for developers page says that node-set was implemented for Fx3 but it looks like all of the common functions were implemented.  I'd like to confirm that that's in fact true and supported before I start documenting this.  Thanks for anyone that can do so.

Comment 34

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

The following functions were implemented:

exsl:node-set
exsl:object-type
regexp:test
regexp:match
regexp:replace
set:difference
set:distinct
set:intersection
set:distinct
set:has-same-node
set:leading
set:trailing
str:tokenize
str:concat
str:split
math:min
math:max
math:highest
math:lowest

You can also look at the source here:

http://lxr.mozilla.org/mozilla/source/content/xslt/src/xslt/txEXSLTFunctions.cpp#229

but that is probably tricky to interpret.