Closed Bug 193678 Opened 18 years ago Closed 14 years ago

support exslt:common

Categories

(Core :: XSLT, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: axel, Assigned: peterv)

References

()

Details

Attachments

(2 files, 5 obsolete files)

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?
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. 
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?
*** Bug 215242 has been marked as a duplicate of this bug. ***
Attached patch v1 (WIP) (obsolete) — Splinter Review
Module-only.
At least the nodeset extension function is a *must* for complex XSLT -- unless,
of course, full XSLT 2.0 is easier -- which is doubtful.
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.
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!
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*.
Adding more comments to a bug is not going to get it fixed faster.
node-set() is the most important function
Is there any progress with this bug?

At least node-set() would be fine.
See comment 9.
Well. But there seem to be a patch available, so it's unclear to me why there's no progress on this issue.
The patch isn't complete, that's why it's marked WIP.
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.
i have seen comment #9 ,  i still like to add to the point, that Opera does also have support for node-set() now.
Attached patch v2 (obsolete) — Splinter Review
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.
Attachment #149332 - Attachment is obsolete: true
Attachment #240855 - Flags: superreview?(bugmail)
Attachment #240855 - Flags: review?(bugmail)
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.
(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.
(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.
Attachment #240855 - Attachment is obsolete: true
Attachment #245341 - Flags: superreview?(bugmail)
Attachment #245341 - Flags: review?(bugmail)
Attachment #240855 - Flags: superreview?(bugmail)
Attachment #240855 - Flags: review?(bugmail)
This one should be diff-able against previous versions.
Attachment #245342 - Flags: superreview?(bugmail)
Attachment #245342 - Flags: review?(bugmail)
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...
Attachment #245341 - Attachment is obsolete: true
Attachment #245468 - Flags: superreview?(bugmail)
Attachment #245468 - Flags: review?(bugmail)
Attachment #245341 - Flags: superreview?(bugmail)
Attachment #245341 - Flags: review?(bugmail)
Attachment #245468 - Attachment is obsolete: true
Attachment #245510 - Flags: superreview?(bugmail)
Attachment #245510 - Flags: review?(bugmail)
Attachment #245468 - Flags: superreview?(bugmail)
Attachment #245468 - Flags: review?(bugmail)
Attachment #245510 - Flags: superreview?(bugmail)
Attachment #245510 - Flags: superreview-
Attachment #245510 - Flags: review?(bugmail)
Attachment #245510 - Flags: review-
r/sr=sicking.
Attachment #245510 - Attachment is obsolete: true
Attachment #245612 - Flags: superreview+
Attachment #245612 - Flags: review+
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.
Attachment #245342 - Flags: superreview?(bugmail)
Attachment #245342 - Flags: superreview+
Attachment #245342 - Flags: review?(bugmail)
Attachment #245342 - Flags: review+
You should also not do the script fixups if mNoFixup is set, both in endElement and startElementInternal
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
So when will this be in a Firefox release?  Firefox 2.0.x?  That's too much to ask I assume.  Firefox 3.0?
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).
(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.
(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.
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.
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.
You need to log in before you can comment on or make changes to this bug.