292841 - Support context size and position in ns(XForms)XPathExpression

Status: RESOLVED FIXED

(Core :: XSLT, defect)

Description

[Allan Beaufour]

To land XForms bug 265460, we need to have support for setting context size and
position for our XPath expression. As I see it, we can either extend
nsXPathExpression or make a nsXFormsXPathExpression (like nsXFormsXPathEvaluator).

Comment 1

[Allan Beaufour]

Attachment: Patch for approach 1

Here's a patch for approach 1, extending nsXPathExpression.

One problem is that it adds 4xPRUint32 to the expression (two can be saved I
guess, but then we need a pointer instead...)

I would also like to have nsIXFormsXPathEvaluator::CreateExpression return a
(non-DOM) an expression that can take size and position as arguments in
Evaluate(). Either through exposing a call in nsXPathExpression, or creating a
special class (approach 2).

Comments anyone?

Comment 2

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

I'd like to see nsXPathExpression extended to support this. We should add some
internal (scriptable) interface called something like nsIDOMNSXPathExpresion.
Two ways to do that interface is:

interface nsIDOMNSXPathExpresion
{
  nsISupports evaluateWithSize(in nsIDOMNode contextNode, 
                               in unsigned long contextSize,
                               in unsigned long contextPosition,
                               in unsigned short type,
                               in nsISupports result)
}

or

interface nsIDOMNSXPathExpresion
{
  attribute unsigned long contextSize;
  attribute unsigned long contextPosition;
}

We could call the new 'evaluateWithSize' just 'evaluate', but then we'd need
magic in nsDOMClassInfo to handle the overload.

I think I'd prefer the latter since it remove the overload problem. It'd also go
more in line with how variables would work if we add support for them.

Comment 3

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

The patch is sort of short, I'm not sure it would work ;)

Comment 4

[Allan Beaufour]

(In reply to comment #3)
> The patch is sort of short, I'm not sure it would work ;)

LOL! If only life was so easy :-)

Comment 5

[Allan Beaufour]

Attachment: And now for the patch :)

Comment 6

[Allan Beaufour]

Attachment: New approach

I agree with sicking on the DOMNS interface, and the evaluateWithContext().
This also eliminates the need for the extra bytes on nsXPathExpression. So
here's a go at it.

I'll upload a patch for xforms that uses this to bug 265460 in a minute.

Comment 7

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

Comment on attachment 182587 [details] [diff] [review]
New approach

>Index: extensions/transformiix/source/xpath/nsIDOMNSXPathExpression.idl
>+  nsISupports evaluateWithContext(in nsIDOMNode contextNode, 
>+                                  in unsigned short type, 
>+                                  in nsISupports result,
>+                                  in unsigned long position,
>+                                  in unsigned long size)

Please name and order these as:

  nsISupports evaluateWithContext(in nsIDOMNode contextNode,
				  in unsigned long contextPosition,
				  in unsigned long contextSize, 
				  in unsigned short type, 
				  in nsISupports result)

Since the position and size belong more with the contextnode then with the
result.


>Index: extensions/transformiix/source/xpath/nsXPathExpression.h
>-        EvalContextImpl(const txXPathNode& aContextNode, txResultRecycler* aRecycler)
>+        EvalContextImpl(const txXPathNode& aContextNode, txResultRecycler* aRecycler,
>+                        PRUint32 aPosition, PRUint32 aSize)
>             : mContextNode(aContextNode),
>               mLastError(NS_OK),
>-              mRecycler(aRecycler)
>+              mRecycler(aRecycler),
>+              mPosition(aPosition),
>+              mSize(aSize)

Same here

>Index: extensions/transformiix/source/xpath/nsIXFormsXPathEvaluator.h
>-  NS_IMETHOD CreateExpression(const nsAString & aExpression, nsIDOMNode *aResolverNode,nsIDOMXPathExpression **aResult); \
>-  NS_IMETHOD Evaluate(const nsAString & aExpression, nsIDOMNode *aContextNode, nsIDOMNode *aResolverNode, PRUint16 aType, nsISupports *aInResult, nsISupports **aResult); 
>+  NS_IMETHOD CreateExpression(const nsAString & aExpression, nsIDOMNode *aResolverNode, nsIDOMNSXPathExpression **aResult); \
>+  NS_IMETHOD Evaluate(const nsAString & aExpression, nsIDOMNode *aContextNode, nsIDOMNode *aResolverNode, PRUint16 aType, nsISupports *aInResult, nsISupports **aResult, PRUint32 aPosition, PRUint32 aSize); 

And defenetly here since you want the out-value to be the last argument.


You also need to add this new interface in the classinfo list in
XSLTProcessorModule.cpp

With that r=me

Comment 8

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

Btw, me and allan talked over the two interfaces in comment 2 and agreed that
the first one was more usefull.

Comment 9

[Axel Hecht]

I'm not so sure about the class info part. Peter?

And the interface has no documentation at all, that should be fixed.

Comment 10

[Allan Beaufour]

Attachment: New approach v2

I fixed sicking's comments, and also added some documentation to the interface.

Comment 11

[Allan Beaufour]

(In reply to comment #10)
> Created an attachment (id=182661) [edit]
> New approach v2
> 
> I fixed sicking's comments, and also added some documentation to the interface.

I forgot to write that I haven't touched the classinfo part... waiting for
peterv there.

Comment 12

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

Yes, we definetly want to expose this to webscripts. Being able to evaluate with
a specified size and position should be harmless and can be usefull for pages. I
also plan to add the ability to specify variable values on this interface.

Comment 13

[Peter Van der Beken [:peterv]]

Comment on attachment 182661 [details] [diff] [review]
New approach v2

>Index: extensions/transformiix/source/xpath/Makefile.in
>===================================================================

>+		nsIDOMNSXPathExpression.idl \

>Index: extensions/transformiix/source/xpath/nsIDOMNSXPathExpression.idl
>===================================================================

Please move that file to dom/public/idl/xpath.

>Index: extensions/transformiix/source/xpath/nsXPathExpression.cpp
>===================================================================

>+NS_IMETHODIMP
>+nsXPathExpression::EvaluateWithContext(nsIDOMNode *aContextNode,
>+                                       PRUint32 aContextPosition,
>+                                       PRUint32 aContextSize,
>+                                       PRUint16 aType,
>+                                       nsISupports *aInResult,
>+                                       nsISupports **aResult)
>+{

Please enforce that aContextPosition <= aContextSize.

You need to add NS_DOMCI_EXTENSION_ENTRY_INTERFACE(nsIDOMXPathExpression) at
http://lxr.mozilla.org/seamonkey/source/extensions/transformiix/build/XSLTProce
ssorModule.cpp#101

Comment 14

[Allan Beaufour]

Attachment: w/peterv's comments

Now I just need a tree I can check in to...

Comment 15

[Mike Kaply [:mkaply]]

Comment on attachment 184910 [details] [diff] [review]
w/peterv's comments

a=mkaply

Comment 16

[Allan Beaufour]

Checked in.