342857 - Add support for XPath function 'current()'

Status: RESOLVED FIXED

(Core Graveyard :: XForms, enhancement)

Description

[Steve Speicher]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4

This is defined in XForms 1.1 spec but currently implemented in formsPlayer and XSmiles 1.0 support.  I recommend tht Mozilla also implement this as it is near impossible to define bindings that work with many existing cross-referenced instances.  For example, a shopping cart that has an instance that represents the catalog and another that represents the order.  What would be ideal is just to have references to the product in the catalog from the order.  Take for example this bind's calculate:
  <xforms:bind nodeset="items/item/description" 
     calculate="instance('inst_catalog')/product[@productId =       
                  current()/../@product]/description" />  

This current() function is also implemented for XSLT.

Reproducible: Always

Comment 1

[Steve Speicher]

Attachment: shows current() in action

Comment 2

[aaronr]

We'll accept a fix for this into the trunk, putting it under the same restrictions as the other 1.1 work (trunk only, protected by a pref).

I'm working on a patch now.

Comment 3

[aaronr]

Attachment: testcase1

Comment 4

[aaronr]

Attachment: testcase2

Comment 5

[aaronr]

Attachment: patch1

This patch works for the most part.  There is still a bug in the dependency node chain for the calculate MIP, though it might have always been there.  I'll work that out tomorrow.

The concept was simple.  We just need to return the context node that was used to evaluate the expression.  But getting that information from xpath is tricky!  Ended up having to do the same thing we had to do to get the orignial xforms node that the expression was being evaluated on to our xforms xpath functions -> we have to pass it in the state of the expression.  But since the context node can change for the cached expressions, I had to introduce a new class that can hold both the original xforms node AND the original context node.  Plus I had to pass this node along with the expression to cache to the MDG engine since I couldn't figure out a way to get the state from the expression from this side of the fence.  We can get it simply enough on the xforms function side of the fence due to the xpath extension stuff.

Comment 6

[aaronr]

Attachment: patch2

This patch works for me on the 3 testcases I tried.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 253693 [details] [diff] [review]
patch2


>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsIXFormsXPathFunctions.idl,v
>retrieving revision 1.1
>diff -u -8 -p -r1.1 nsIXFormsXPathFunctions.idl
>--- nsIXFormsXPathFunctions.idl	13 Jul 2006 14:21:52 -0000	1.1
>+++ nsIXFormsXPathFunctions.idl	2 Feb 2007 00:45:35 -0000
>@@ -52,9 +52,10 @@ interface nsIXFormsXPathFunctions : nsIS
>     txINodeSet instance(in txIFunctionEvaluationContext aContext, in DOMString aString);
>     double max(in txINodeSet aNodeSet);
>     double min(in txINodeSet aNodeSet);
>     double months(in DOMString aDuration);
>     DOMString now();
>     DOMString property(in DOMString aProperty);
>     double seconds(in DOMString aDuration);
>     double secondsFromDateTime(in DOMString aDateTime);
>+    txINodeSet current(in txIFunctionEvaluationContext aContext);
> };

Update IID when changing an interface.


>+NS_IMETHODIMP
>+nsXFormsXPathFunctions::Current(txIFunctionEvaluationContext *aContext,
>+                                txINodeSet **aResult)
>+{
>+  *aResult = nsnull;
>+  nsresult rv;
>+
>+  // make sure that we can create our return buffer
>+  nsCOMPtr<txINodeSet> result =
>+    do_CreateInstance("@mozilla.org/transformiix-nodeset;1", &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);

Move this to |if (experimental) {...}|


>+
>+  // Check for the preference that surrounds all of our 1.1 work.  Return
>+  // an empty nodeset if it isn't set.
>+  PRBool enableExperimental = PR_FALSE;
>+  nsCOMPtr<nsIPrefBranch> pref = do_GetService(NS_PREFSERVICE_CONTRACTID, &rv);
>+  if (NS_SUCCEEDED(rv) && pref) {
>+    PRBool val;
>+    if (NS_SUCCEEDED(pref->GetBoolPref("xforms.enableExperimentalFeatures",
>+                                       &val)))
>+      enableExperimental = val;
>+  }
>+

Could we have a helper function |PRBool nsXFormsUtils::ExperimentalFeaturesEnabled()| 
and then use that here and in submission code.


>+#include "nsISupports.idl"
>+#include "nsIDOMNode.idl"
>+
>+[uuid(5c5eb4e7-5693-46e2-aa05-f630d33217e5)]
>+interface nsIXFormsXPathState : nsISupports
>+{
>+  attribute nsIDOMNode xformsNode;
>+  attribute nsIDOMNode originalContextNode;
>+};

Please document these. And is xformsNode really a good name?
At least writing GetXformsNode in C++ looks a bit weird.
And should xformsNode be readonly, since you're always only using constructor to set it.
(Not sure if originalContextNode could be made readonly too)



>+++ nsXFormsXPathState.h	2007-02-01 18:39:24.937500000 -0600

Do you need the .h file? nsXFormsXPathState is so simple that having the class definition and implementation
in the .cpp file should be ok, IMO.

Comment 8

[aaronr]

(In reply to comment #7)
> (From update of attachment 253693 [details] [diff] [review])
> 
> >===================================================================
> >RCS file: /cvsroot/mozilla/extensions/xforms/nsIXFormsXPathFunctions.idl,v
> >retrieving revision 1.1
> >diff -u -8 -p -r1.1 nsIXFormsXPathFunctions.idl
> >--- nsIXFormsXPathFunctions.idl	13 Jul 2006 14:21:52 -0000	1.1
> >+++ nsIXFormsXPathFunctions.idl	2 Feb 2007 00:45:35 -0000
> >@@ -52,9 +52,10 @@ interface nsIXFormsXPathFunctions : nsIS
> >     txINodeSet instance(in txIFunctionEvaluationContext aContext, in DOMString aString);
> >     double max(in txINodeSet aNodeSet);
> >     double min(in txINodeSet aNodeSet);
> >     double months(in DOMString aDuration);
> >     DOMString now();
> >     DOMString property(in DOMString aProperty);
> >     double seconds(in DOMString aDuration);
> >     double secondsFromDateTime(in DOMString aDateTime);
> >+    txINodeSet current(in txIFunctionEvaluationContext aContext);
> > };
> 
> Update IID when changing an interface.
> 

Doh!  Will fix.

> 
> >+NS_IMETHODIMP
> >+nsXFormsXPathFunctions::Current(txIFunctionEvaluationContext *aContext,
> >+                                txINodeSet **aResult)
> >+{
> >+  *aResult = nsnull;
> >+  nsresult rv;
> >+
> >+  // make sure that we can create our return buffer
> >+  nsCOMPtr<txINodeSet> result =
> >+    do_CreateInstance("@mozilla.org/transformiix-nodeset;1", &rv);
> >+  NS_ENSURE_SUCCESS(rv, rv);
> 
> Move this to |if (experimental) {...}|
> 

Can't.  XPath expects a nodeset back.  Actually crashes in txXPCOMExtensionFunctionCall::evaluate if you just set *aResult to nsnull.

> 
> >+
> >+  // Check for the preference that surrounds all of our 1.1 work.  Return
> >+  // an empty nodeset if it isn't set.
> >+  PRBool enableExperimental = PR_FALSE;
> >+  nsCOMPtr<nsIPrefBranch> pref = do_GetService(NS_PREFSERVICE_CONTRACTID, &rv);
> >+  if (NS_SUCCEEDED(rv) && pref) {
> >+    PRBool val;
> >+    if (NS_SUCCEEDED(pref->GetBoolPref("xforms.enableExperimentalFeatures",
> >+                                       &val)))
> >+      enableExperimental = val;
> >+  }
> >+
> 
> Could we have a helper function |PRBool
> nsXFormsUtils::ExperimentalFeaturesEnabled()| 
> and then use that here and in submission code.
> 

Good idea.  Will do.

> 
> >+#include "nsISupports.idl"
> >+#include "nsIDOMNode.idl"
> >+
> >+[uuid(5c5eb4e7-5693-46e2-aa05-f630d33217e5)]
> >+interface nsIXFormsXPathState : nsISupports
> >+{
> >+  attribute nsIDOMNode xformsNode;
> >+  attribute nsIDOMNode originalContextNode;
> >+};
> 
> Please document these. And is xformsNode really a good name?
> At least writing GetXformsNode in C++ looks a bit weird.
> And should xformsNode be readonly, since you're always only using constructor
> to set it.
> (Not sure if originalContextNode could be made readonly too)
> 

will copy comments from nsXFormsXPathState.h and add a couple more for what nxIXFormsXPathState is for in general.  

I really didn't like the name much either, but couldn't really come up with anything better.  Please read my comments about the node and see if you can think of a better, more descriptive name.  I thought of maybe 'expression AnchorNode' but people could confuse it with html anchor element (html:a).

Making xformsNode readonly.  I didn't think that I could make contextNode readonly but the last rework of my patch now makes it possible so I'll do it.  That may need to change in the future.  Rework also eliminates the need for one of the contstructors.

> 
> 
> >+++ nsXFormsXPathState.h	2007-02-01 18:39:24.937500000 -0600
> 
> Do you need the .h file? nsXFormsXPathState is so simple that having the class
> definition and implementation
> in the .cpp file should be ok, IMO.
> 

I need to instantiate a nsXFormsXPathState in nsXFormsUtils.cpp.  Don't I need the header declaration (for constructor signature) so that nsXFormsUtils will compile?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #8)
> 
> I need to instantiate a nsXFormsXPathState in nsXFormsUtils.cpp.  Don't I need
> the header declaration (for constructor signature) so that nsXFormsUtils will
> compile?
> 

Ah, ofc.

Comment 10

[aaronr]

Attachment: patch3

Comment 11

[timeless]

> Can't.  XPath expects a nodeset back.  Actually crashes in
> txXPCOMExtensionFunctionCall::evaluate if you just set *aResult to nsnull.

can't you initialize rv to a failure, null *aResult and move it to the experimental section and only set rv to success if it's happy?

the pref code really should use a static int and register a pref change observer so that you don't constantly getservice+getpref.

Comment 12

[aaronr]

(In reply to comment #11)
> > Can't.  XPath expects a nodeset back.  Actually crashes in
> > txXPCOMExtensionFunctionCall::evaluate if you just set *aResult to nsnull.
> 
> can't you initialize rv to a failure, null *aResult and move it to the
> experimental section and only set rv to success if it's happy?

I will now return NS_ERROR_NOT_IMPLEMENTED in this case.

> 
> the pref code really should use a static int and register a pref change
> observer so that you don't constantly getservice+getpref.
> 

Good idea.  Done.

Comment 13

[aaronr]

Attachment: patch4

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 254239 [details] [diff] [review]
patch4


> 
>+  /**
>+   * Variable is true if the 'xforms.enableExperimentalFeatures' preference has
>+   * been enabled in the browser.  We currently use this preference to surround
>+   * code that implements features from future XForms specs.  Specs that have
>+   * not yet reached recommendation.
>+   */
>+  static NS_HIDDEN_(PRBool) experimentalFeaturesEnabled;
>+

Make this a static method ExperimentalFeaturesEnabled() and hide
the variable in the .cpp file:
static PRBool gExperimentalFeaturesEnabled = PR_FALSE;

PRBool
nsXFormsUtils::ExperimentalFeaturesEnabled()
{
  return gExperimentalFeaturesEnabled;
}
...

>+protected:
>+
>+  static NS_HIDDEN_(int) PR_CALLBACK PrefChangedCallback(const char*, void*);
>+

This could be a static method in .cpp file.



>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsXPathFunctions.cpp,v
>retrieving revision 1.2
>diff -u -8 -p -r1.2 nsXFormsXPathFunctions.cpp
>--- nsXFormsXPathFunctions.cpp	11 Dec 2006 10:54:09 -0000	1.2
>+++ nsXFormsXPathFunctions.cpp	7 Feb 2007 01:47:36 -0000
>@@ -43,16 +43,19 @@
> #include "nsIDOMElement.h"
> #include "nsString.h"
> #include "nsXFormsUtils.h"
> #include "prprf.h"
> #include "txDouble.h"
> #include "txIFunctionEvaluationContext.h"
> #include "txINodeSet.h"
> #include "nsIClassInfoImpl.h"
>+#include "nsIPrefBranch.h"
>+#include "nsIPrefService.h"

You don't need these anymore, right?


>+
>+NS_IMETHODIMP
>+nsXFormsXPathFunctions::Current(txIFunctionEvaluationContext *aContext,
>+                                txINodeSet **aResult)
>+{
>+  *aResult = nsnull;
>+  nsresult rv;

Define |rv| where you use it.

With those, r=me

Comment 15

[aaronr]

Attachment: patch5

fixed smaug's review comments.

Comment 16

[Doron Rosenberg (IBM)]

One possible optimization is to hook the pref observer on the first call of nsXFormsUtils::ExperimentalFeaturesEnabled(), thus saving the observer cost (which is probably minimal) for most forms.

Comment 17

[aaronr]

(In reply to comment #16)
> One possible optimization is to hook the pref observer on the first call of
> nsXFormsUtils::ExperimentalFeaturesEnabled(), thus saving the observer cost
> (which is probably minimal) for most forms.
> 

I was looking to the future.  I figure at some point we may need to look for other prefs, too.  So I'd rather just register one callback at known one time rather than testing each time to see if the callback is registered and registering it if it isn't.  The possibly unnecessary overhead will only be encountered when someone changes a pref which should hardly ever happen except under the use of extreme power users.

Comment 18

[aaronr]

checked into trunk.

Comment 20

[aaronr]

Attachment: patch for 1.8

Jonas, please let us know if you think that these transformiix changes are acceptable for the 1.8 branch.  I tried to keep them as simple as possible.

Comment 21

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

Isn't mNode the node containing the current() expression, i.e. a node in the forms document. Whereas what current() should return is a node in the data document (sorry, my xforms lingo is probably wrong)

Comment 22

[aaronr]

(In reply to comment #21)
> Isn't mNode the node containing the current() expression, i.e. a node in the
> forms document. Whereas what current() should return is a node in the data
> document (sorry, my xforms lingo is probably wrong)
> 

I figured that it would be better to just have one node in the member data since I couldn't think of a scenario where both the xforms node and a data node would be needed.  So mNode can be either, depending on the xforms xpath function that is called.  I have this comment in XFormsFunctions.h:

 // mNode could be either the resolver node or the original context node from
 // the expression, depending what node the XForms XPath function requires.

Comment 23

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

Comment on attachment 255550 [details] [diff] [review]
patch for 1.8

>Index: extensions/transformiix/source/xpath/nsIXFormsXPathEvaluator.h
>@@ -78,22 +78,26 @@ class nsIDOMNSXPathExpression; /* forwar
>-  NS_IMETHOD CreateExpression(const nsAString & aExpression, nsIDOMNode *aResolverNode, nsIDOMNSXPathExpression **aResult) = 0;
>+  NS_IMETHOD CreateExpression(const nsAString & aExpression, nsIDOMNode *aResolverNode, nsIDOMNode *aOrigCtxt, nsIDOMNSXPathExpression **aResult) = 0;

I'm not exited about passing in aOrigCtxt to CreateExpression since it's easy to by mistake keep that expression around and reuse it for several evaluations without considering current() calls.

>-  NS_IMETHOD Evaluate(const nsAString & aExpression, nsIDOMNode *aContextNode, PRUint32 aContextPosition, PRUint32 aContextSize, nsIDOMNode *aResolverNode, PRUint16 aType, nsISupports *aInResult, nsISupports **aResult) = 0;
>+  NS_IMETHOD Evaluate(const nsAString & aExpression, nsIDOMNode *aContextNode, PRUint32 aContextPosition, PRUint32 aContextSize, nsIDOMNode *aResolverNode, nsIDOMNode *aOrigCtxt, PRUint16 aType, nsISupports *aInResult, nsISupports **aResult) = 0;

What is the need for aOrigCtxt here? Isn't it always the same as aContextNode?

Comment 24

[aaronr]

(In reply to comment #23)
> (From update of attachment 255550 [details] [diff] [review])
> >Index: extensions/transformiix/source/xpath/nsIXFormsXPathEvaluator.h
> >@@ -78,22 +78,26 @@ class nsIDOMNSXPathExpression; /* forwar
> >-  NS_IMETHOD CreateExpression(const nsAString & aExpression, nsIDOMNode *aResolverNode, nsIDOMNSXPathExpression **aResult) = 0;
> >+  NS_IMETHOD CreateExpression(const nsAString & aExpression, nsIDOMNode *aResolverNode, nsIDOMNode *aOrigCtxt, nsIDOMNSXPathExpression **aResult) = 0;
> 
> I'm not exited about passing in aOrigCtxt to CreateExpression since it's easy
> to by mistake keep that expression around and reuse it for several evaluations
> without considering current() calls.
> 
> >-  NS_IMETHOD Evaluate(const nsAString & aExpression, nsIDOMNode *aContextNode, PRUint32 aContextPosition, PRUint32 aContextSize, nsIDOMNode *aResolverNode, PRUint16 aType, nsISupports *aInResult, nsISupports **aResult) = 0;
> >+  NS_IMETHOD Evaluate(const nsAString & aExpression, nsIDOMNode *aContextNode, PRUint32 aContextPosition, PRUint32 aContextSize, nsIDOMNode *aResolverNode, nsIDOMNode *aOrigCtxt, PRUint16 aType, nsISupports *aInResult, nsISupports **aResult) = 0;
> 
> What is the need for aOrigCtxt here? Isn't it always the same as aContextNode?
> 

Here is what happens in our processor.  Consider this from 'testcase1':

<xforms:instance xmlns="">
  <converter>
    <amount>100</amount>
    <currency>jpy</currency>
    <convertedAmount></convertedAmount>
  </converter>
</xforms:instance>

<xforms:instance xmlns="" id="convTable">
  <convTable date="20040212" currencyName="Canadian Dollars" currency="cdn">
    <rate currencyName="Euros" currency="eur">0.59376</rate>
    <rate currencyName="Mexican Pesos" currency="mxn">8.37597</rate>
    <rate currencyName="Yen" currency="jpy">80.23451</rate>
    <rate currencyName="US Dollars" currency="usd">0.76138</rate>
  </convTable>
</xforms:instance>

<xforms:bind id="myBind"
             nodeset="convertedAmount" 
             calculate="../amount * instance('convTable')/rate[@currency=current()/../currency]"/>

So this loosely means that we'll set the value of <convertedAmount/> anytime the <amount> or <currency> values change.  And we'll set it based on the rate specified by the node in another instance document who has an @currency that matches the value of ../currency.  We process the xforms:bind element only on document load and then when the whole mdg gets rebuilt since those are the only times that the selected nodeset can change.  When we analyze this large expression recursively, breaking the expression down into parts to get our node dependencies, when we reach the call to current() (expression would be current()/../currency) the current context node is 'rate' due to the predicate brackets, yet the original context node is 'convertedAmount'.

Comment 25

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

Ok, makes sense. I'm still not exited about letting CreateExpression take a context node since chances of bugs are too big. Can we get rid of that?

Comment 26

[aaronr]

(In reply to comment #25)
> Ok, makes sense. I'm still not exited about letting CreateExpression take a
> context node since chances of bugs are too big. Can we get rid of that?
> 

There's a problem with not passing the context into CreateExpression.  How would the context node get into the parser context for cached expressions?  For example, if we do CreateExpression to cache the expression to re-use (as we do with the MDG) and then call expression->EvaluateWithContext when it comes time to run it, there is no way to get that context into the parser context.  The only thing I can think of off of the top of my head is make CreateExpression private and create xpath expressions for caching by calling ::Evaluate first and have it return the expression that it created.  But then we might have to do a throw-away evaluation for any expression that we want to cache if we don't have a need to know the result right away.

Comment 27

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

> There's a problem with not passing the context into CreateExpression.  How
> would the context node get into the parser context for cached expressions? 

That's exactly it, since the context node is compiled in, you can't cache the expression unless you are sure you are going to evaluate it with the same context node every time.

If you use a cached expression and pass in a different context node the current() function will produce the wrong result, no?

Comment 28

[aaronr]

(In reply to comment #27)
> > There's a problem with not passing the context into CreateExpression.  How
> > would the context node get into the parser context for cached expressions? 
> 
> That's exactly it, since the context node is compiled in, you can't cache the
> expression unless you are sure you are going to evaluate it with the same
> context node every time.
> 
> If you use a cached expression and pass in a different context node the
> current() function will produce the wrong result, no?
> 

I've talked to sicking.  He's ok with the patch as long as we are well aware of the potential risk.  I've spelled out the options and risks to the other XForms developers and the vote is to go ahead with the patch as it is.  The consensus is the benefit outweighs the risk which can be minimized to almost nil with care on our part.

So Jonas, we are ready for your r+ if you are willing to give it :-)

Comment 29

[aaronr]

Comment on attachment 255550 [details] [diff] [review]
patch for 1.8

Implementation for a very useful xforms xpath function.  Code is executable only using an internal interface meant for only the xforms extension.

Comment 30

[Daniel Veditz [:dveditz]]

Comment on attachment 255550 [details] [diff] [review]
patch for 1.8

You'll have to wait until the tag is cut, which makes it "next time".

Comment 31

[Daniel Veditz [:dveditz]]

Comment on attachment 255550 [details] [diff] [review]
patch for 1.8

Jonas: is it OK to change these interfaces on the branch? Are they strictly XForms even thought they're in the xpath directory?

Comment 32

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

Comment on attachment 255550 [details] [diff] [review]
patch for 1.8

Yeah, these interfaces are xforms only so no-one should be using them other than the xforms plugin.

Comment 33

[Daniel Veditz [:dveditz]]

Comment on attachment 255550 [details] [diff] [review]
patch for 1.8

Thanks Jonas.

approved for 1.8.1.5 and 1.8.0.13, a=dveditz

Comment 34

[aaronr]

Attachment: patch for 1.8 checkin

Had to tweak to get cvs to apply it after the other 0.8 changes that were made since I made the original patch for 1.8.  This is the code I'm checking in.

Comment 35

[aaronr]

checked into 1.8

Comment 36

[Daniel Veditz [:dveditz]]

Comment on attachment 255550 [details] [diff] [review]
patch for 1.8

closing the tree for 1.8.0.13