258472 - Add the XPath functions needed for XForms

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[aaronr]

This bug is the placeholder for all things XPath in our XForms implementation. 
I will check in my XPath changes in on this bug.  We'll do our changes right in
the XPath code in the browser, not as an extension.  However, we may couple
these changes by checking for namespace on the element where the XPath appears
to make sure that it is an XForms element.  That is, if the powers that be don't
want these functions as part of the default XPath.

Comment 1

[Allan Beaufour]

A different approach could be to add support for custom functions that can be
registred with the XPath engine.

Comment 2

[aaronr]

Yes we could.  

Ideally what should have happened is that XForms follow the basic extensibility
of XPath and create functions in the xforms namespace.  But they wanted the
functions in the default namespace, kinda like XSLT has it.  It just seems to me
to be a bit of work for us to introduce a way to create custom functions in the
default namespace just for XForms.  Can you think of any other need for this
type of code or any upcoming specs that are setting themselves up in default
XPath like Xforms is?  Then it would definitely make sense.  I guess I'm just
optimistic in hoping that other technologies will extend XPath correctly. :=)

Comment 3

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

I don't like the idea of adding these things straight into the xpath engine.
These functions should not exist when using xpath outside of xforms.

Transformiix is already written in such a way that it's possible to add
extension functions without modifying the current code. All you need is to
supply a parse context that will return the proper function-implementations.
What might be needed is for some more symbols to be exported so that you can
create parse context outside of transformiix.dll, or alternativly we need to
create a real XPCOM interface that allows for adding extension functions.

I think peterv has done a lot of thinking on how to do xpath extensionfunctions
so he might have some ideas for good interfaces.

Comment 4

[aaronr]

I got a note from peterv.  He hasn't written up any of his XPath extension
ideas, yet.  I'll look into seeing how we might be able to keep Xforms XPath
extensions out of the main tree.  

I personally think that the functions for the most part would be pretty handy
for developers to have even if XForms wasn't running, but I'll respect the
opinion of the majority and target the work as seperate from the transformiix
code from here on out.

Comment 5

[Axel Hecht]

There are quite a few functions which should definitly be not part of regular
xpath. Starting from the property function.

In addition, I'm interested in the lexical -> schema datatype conversion stuff for
RDF, how do you intend to do that? (For example in seconds(), didn't check all of
them.)

Comment 6

[aaronr]

Attachment: First attempt

This is a patch to show what I've done so far, which isn't nearly as much as I
would have liked at this point.

I'm now working on the hashtable interface that will allow me to do instance()
and index(), and which I'll use to add XForms checking to the code so that
XForms functions won't be executable on non xforms context nodes

Comment 7

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

I'm sorry, but I don't think we want xforms-only stuff so tighly integrated in
the transformiix code. Partly because we'd actually break the XPath spec by
making all these functions available at all times rather then just when used in
XForms.

What would be much better is to create your own separate function-classes for
the xforms functions. Then you create a separate implementation of
txIParseContext and let that return your new classes.

Most likly you'll have to put all this in the transformiix dll since you'll want
to inherit the FunctionCall class. If we ever create a 'real' extensionmechanism
for transformiix we'll move the code out to some existing xforms dll.

This way you'll also be able to resolve namespace prefixes correctly
(resolveNamespacePrefix is a function in txIParseContext)

Comment 8

[aaronr]

As it stands in the patch right now, yes, all of the xforms functions would be
exposed.  My current plan was this.  I am working on a xpcom service that will
tie xforms context nodes (which live in xforms instance documents) back to their
owner document via a hashtable.  So in this way, I had planned to put a check in
every XPath specific function to see if the context node exists in this
hashtable.  if it doesn't, then it isn't a xforms node and then I won't run the
function.  So while the functions would be available, they wouldn't be executed
for non-xforms nodes.

I will work on your suggestion of seperating out the functions into other classes.

Comment 9

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

We still would have the problem of that the expression would actually parse even
though it shouldn't.

It should be fairly easy to create a separate class that implements
txIParseContext. The biggest effort would probably be to make it into an XPCOM
class and make it scriptable. However this too should be a relativly easy task.

Comment 10

[aaronr]

Attachment: attempt with Jonas' approach

Here is a stab using Jonas' input.  Pretty simple after he explained it to me. 
Basically removing XForms use of the document's XPath Evaluator and using my
newly created (and substantially similar) nsXFormsXPathEvaluator instead.  We
still have the XForms XPath extensions in transformiix, but it is (almost)
completely seperated from the rest of the XPath files.

Couple of implementation questions: 
1) I don't know how much of the stuff that I need that I added to
XSLTProcessorModule.cpp.  Do I need all of this DOMCI_EXTENSION and
DOM_CLASSINFO stuff, or do I just need to register my component?  I don't have
a good understanding of what is happening here.

2) I decided not to have nsXFormsXPathEvaluator inherit from nsXPathEvaluator
since I am using my own private txIParseContext and my CreateExpression method
is different.  If you think that I am wrong here, please let me know.

Comments appreciated.

Comment 11

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

are you going to use the XFormsXPathEvaluator from javascript? If not you can
remove most of the stuff in the module.

Comment 12

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

Some first-glance comments.

nsXFormsXPathEvaluator has way more code then it needs to:
You only call CreateExpression from within Evaluate so just merge the function
into there.
Get rid of CreateNSResolver and pass Evalute a node instead. Inside Evaluate you
could just call |new nsXPathNSResolver| directly. Or, if you want to go for
speed, pass XFormsParseContextImpl the node rather then a ns-resolver and inline
the code that's inside nsXPathNSResolver::LookupNamespaceURI in there, it's just
a couple of lines.
Remove XFormsParseContextImpl::mIsCaseSensitive, it will always be false. It's
only true when using xpath on html document (as opposed to xml documents)
Remove SetDocument and mDocument, they're not called or used.

On a separate note. Looking at the nsXFormsUtils::EvaluateXPath function it
seems like XForms doesn't store the parsed XPath expressions. Meaning that every
time an XPath expression is evaluated we'll go through both a "parse" and an
"evaluation" phase. Functionalitywise this doesn't matter. However it might
affect performance. If xpath starts showing up in xforms profiles this would be
a good place to start.

Comment 13

[Allan Beaufour]

(In reply to comment #12)
> On a separate note. Looking at the nsXFormsUtils::EvaluateXPath function it
> seems like XForms doesn't store the parsed XPath expressions. Meaning that every
> time an XPath expression is evaluated we'll go through both a "parse" and an
> "evaluation" phase. Functionalitywise this doesn't matter. However it might
> affect performance. If xpath starts showing up in xforms profiles this would be
> a good place to start.

Good comment. I'll be working on nsXFormsUtils as part of bug 265467, and I'll
try to not evaluate more XPath expressions than necessary.

Comment 14

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

The issue is not how much or often you evaluate (although of course that's
important too), but rather what you evaluate.

Either you can parse the expression-string into an expresion-object once and
evaluate that expression-object multiple times. Or you can evaluate the
string-expresion directly every time (and thus internally force the xpath engine
to reparse the expression every time).

The simplest way to not force a reparse every time is to simply stick the parsed
expressions into members on the element, so rather then grabbing an elements
'readonly' attribute you'd grab an internal 'readonlyExpr' member.

However, i'm not sure if this reparse thing is an issue, it very well might not
be. I don't think anyone has ever done any numbers comparing parsespeed vs.
evalspeed for common expressions.

Comment 15

[Allan Beaufour]

(In reply to comment #14)
> The issue is not how much or often you evaluate (although of course that's
> important too), but rather what you evaluate.

Sorry, that includes limiting the number of parses too of course. I'll do my best :)

Comment 16

[aaronr]

Attachment: proposed fix

This patch implements all of the XForms XPath extensions except for 5 which are
blocked on synchronizing with other code (repeat element and schema
validation).  I have #if 0'd out these functions.  I have isolated the
xforms-specific code from the transformiix code as much as possible so that
when XPath is extendable, it should be simple to seperate out this work. 
XForms uses my new evaluator (nsXFormsXPathEvaluator) with its own
implementation of the ParseContext interface.  I also had to introduce a
service to XForms to interact with the XForm in a way that wouldn't put any
extra requirements on transformiix.  This is where the schema-based work will
be done in the future, too.

Comments welcome.

Comment 17

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

Most of the second paragraph of comment 12 still applies. Except that you
probably shouldn't merge the 'CreateExpression' method into 'Evaluate'. Do pass
a node rather then an nsIDOMXPathNSResolver though. That way you won't have to
do that non-workin cast to an nsDOMXPathNSResolver. And nsXFormsXPathEvaluator
doesn't need to inherit nsIXPathEvaluatorInternal.

All the changes in XSLTProcessorModule.cpp are unneccesary except the last one.

All changes to existing files (including makefiles) needs to inside
|#ifdef XFORMS| or whatever define it is that you're using.

I think I would prefer if all the xforms specific files went into some
xpath/extensions directory. Talk with peterv about that.

The license header is wrong. You don't work at Netscape :). IIRC IBM has a
pretty firm oppinion on what they want you to write there.

In general your arguments doesn't line up very nicely. For example:
+nsXFormsUtilitiesService::IsThisModelAssocWithThisNode(nsIDOMNode *aModel,
+                                                    nsIDOMNode *aNode,
+                                                    PRBool *aModelAssocWith...

Do you really need an .idl file for nsIXFormsUtilitiesService? Seems like just a
normal .h file should do fine. idl files are only needed when you expect the
interface to be used from scripts.

Comment 18

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

Comment on attachment 168949 [details] [diff] [review]
proposed fix

>--- junk	1969-12-31 18:00:00.000000000 -0600
>+++ extensions/transformiix/source/xpath/XFormsFunctionCall.cpp	2004-12-16 18:20:28.000000000 -0600
...
>+nsresult
>+XFormsFunctionCall::evaluate(txIEvalContext* aContext, txAExprResult** aResult)
>+{
>+  *aResult = nsnull;
>+  nsresult rv = NS_OK;
>+  txListIterator iter(&params);
>+
>+  switch (mType) {
>+    case AVG:
>+    {

You need an |requireParams| call here. Same applies to some other types.

>+      nsRefPtr<txNodeSet> nodes;
>+      nsresult rv = evaluateToNodeSet((Expr*)iter.next(), aContext,
>+                                      getter_AddRefs(nodes));
>+      NS_ENSURE_SUCCESS(rv, rv);
>+   
>+      double res = 0;
>+      PRInt32 i;
>+      for (i = 0; i < nodes->size(); ++i) {
>+        nsAutoString resultStr;
>+        txXPathNodeUtils::appendNodeValue(nodes->get(i), resultStr);
>+        res += Double::toDouble(resultStr);

What should happen if one of the values can't be converted to a number? Try
poking someone in the WG as the spec doesn't seem to say.

>+      }
>+   
>+      if (nodes->size() > 0)
>+      {
>+        res = (res/(nodes->size()));

You could just use 'i' here. And move that '{' up one line.

>+    case BOOLEANFROMSTRING:
>+    {
>+      if (!requireParams(1, 1, aContext))
>+        return NS_ERROR_XPATH_BAD_ARGUMENT_COUNT;
>+
>+      PRInt32 retvalue = -1;
>+      nsAutoString booleanValue;
>+      if (iter.hasNext()) {

No need for this check, the requireParams call has already ensured that there
is one argument.

>+        evaluateToString((Expr*)iter.next(), aContext, booleanValue);
>+
>+        if (!booleanValue.IsEmpty()) {
>+          if (booleanValue.EqualsLiteral("1")) {
>+            aContext->recycler()->getBoolResult(PR_TRUE, aResult);
>+          }
>+          else if (booleanValue.EqualsLiteral("0")) {
>+            aContext->recycler()->getBoolResult(PR_FALSE, aResult);
>+          }
>+          else if (booleanValue.LowerCaseEqualsLiteral("true")) {
>+            aContext->recycler()->getBoolResult(PR_TRUE, aResult);
>+          }
>+          else if (booleanValue.LowerCaseEqualsLiteral("false")) {
>+            aContext->recycler()->getBoolResult(PR_FALSE, aResult);
>+          }
>+          else {
>+            // if not "0","1", "true", or "false" (case insensitive), then send 
>+            //   return value will be false.  This per the XForms errata
>+            //   dated 2004/10/15
>+            aContext->recycler()->getBoolResult(PR_FALSE, aResult);
>+          }
>+        }

This entire |if| can be written as

aContext->recycler()->getBoolResult(booleanValue.EqualsLiteral("1") ||
				   
booleanValue.LowerCaseEqualsLiteral("true"), aResult);


>+
>+      }
>+      return NS_OK;
>+    }
>+    case COUNTNONEMPTY:
>+    {
>+      nsRefPtr<txNodeSet> nodes;
>+      nsresult rv = evaluateToNodeSet((Expr*)iter.next(), aContext,
>+                                      getter_AddRefs(nodes));
>+      NS_ENSURE_SUCCESS(rv, rv);
>+   
>+      double res = 0, test = 0;
>+      PRInt32 i, count=0;
>+      for (i = 0; i < nodes->size(); ++i) {
>+        nsAutoString resultStr;
>+        txXPathNodeUtils::appendNodeValue(nodes->get(i), resultStr);
>+        if (!resultStr.IsEmpty())
>+        {

move the '{' up a line. If you care about speed this function could probably be
written more efficiently by for each node looping through all its descenants
until a non-empty child is found. Probably not worth the work though...

>+          count++;
>+        }
>+      }
>+   
>+      return aContext->recycler()->getNumberResult(count, aResult);
>+    }
>+    case DAYSFROMDATE:
>+    {
>+      if (!requireParams(1, 1, aContext))
>+        return NS_ERROR_XPATH_BAD_ARGUMENT_COUNT;
>+   
>+#if 0
>+      // first make sure that the parameter is a proper xsd:dateTime or
>+      //   xsd:date
>+      nsAutoString dateTime;
>+      if (iter.hasNext()) {

No need for this check. Same applies in a lot of places I see. You only need
checks like this if the function has optional arguments that you want to test
if they were provided.

>+        evaluateToString((Expr*)iter.next(), aContext, dateTime);
>+   
>+        if (!duration.IsEmpty()) {

should this be |dateTime.IsEmpty()|?

>+          nsCOMPtr<nsIXFormsUtilitiesService>xformsService = 
>+                do_GetService("@mozilla.org/xforms-utility-service;1", &rv);
>+
>+          if (xformsService) {
>+            nsAutoString dateTimeType(NS_LITERAL_STRING("dateTime"));
>+            PRBool isDateTime, isDate;
>+            nsAutoString schemaNS(NS_LITERAL_STRING(NS_NAMESPACE_SCHEMA));

You could use NS_NAMED_LITERAL_STRING here.

>+            xformsService->ValidateString(dateTime, dateTimeType, 
>+                                          schemaNS, &isDateTime);
>+            if (!isDateTime) {
>+              nsAutoString dateType(NS_LITERAL_STRING("date"));
>+              xformsService->ValidateString(dateTime, dateType, 
>+                                            schemaNS, &isDate);
>+              if (!isDate) {
>+                return aContext->recycler()->getNumberResult(Double::NaN, aResult);
>+              }
>+            }
>+
>+            // XXX TODO up to this point, it will work.  This is where we need
>+            //   to probably call into the XForms service and have it do the work
>+            //   of calling the schema utility function to parse the dateTime
>+            //   and have it convert the seconds, minutes, etc. to seconds
>+            //   since it is desirous to not have XPATH require the schema 
>+            //   validator stuff.
>+          }
>+        }
>+      }
>+      return aContext->recycler()->getNumberResult(res, aResult);
>+#endif
>+      return NS_ERROR_NOT_IMPLEMENTED;
>+    }
>+    case IF:
>+    {
>+      if (!requireParams(3, 3, aContext))
>+        return NS_ERROR_XPATH_BAD_ARGUMENT_COUNT;
>+   
>+      PRBool test;
>+      nsAutoString valueToReturn;
>+      if (iter.hasNext()) {
>+        test = evaluateToBoolean((Expr*)iter.next(), aContext);
>+
>+        // grab 'true' value to return
>+        Expr *getvalue = (Expr*)iter.next();
>+   
>+        if (!test) {
>+          // grab 'false' value to return
>+          getvalue = (Expr*)iter.next();
>+        }
>+        evaluateToString(getvalue, aContext, valueToReturn);
>+      }
>+   
>+      return aContext->recycler()->getStringResult(valueToReturn, aResult);
>+    }
>+    case INDEX:
>+    {
>+      // Given an element's id as the parameter, need to query the element and 
>+      //   make sure that it is a xforms:repeat node.  Given that, must query 
>+      //   its index.
>+      if (!requireParams(1, 1, aContext))
>+          return NS_ERROR_XPATH_BAD_ARGUMENT_COUNT;
>+   
>+#if 0
>+      // XXX TODO This code is all done other than figuring out how to get the
>+      //   current index from the repeat element.  We'll use the xforms
>+      //   utility service to get that information for us from the repeat
>+      //   node that we will pass it.
>+      nsAutoString instanceId;
>+      if (iter.hasNext())
>+      {
>+        evaluateToString((Expr*)iter.next(), aContext, instanceId);
>+   
>+        if (!instanceId.IsEmpty())
>+        {
>+          // here document is the XForms document
>+          nsCOMPtr<nsIDOMDocument> document;
>+          rv = mResolverNode->GetOwnerDocument(
>+                                           getter_AddRefs(document));
>+          NS_ENSURE_SUCCESS(rv, rv);
>+   
>+          nsCOMPtr<nsIDOMElement> repeatEle;
>+          rv = document->GetElementById(instanceId, 
>+                                        getter_AddRefs(repeatEle));
>+   
>+          // make sure that repeatEle really IS a xforms:repeat
>+          //   element.
>+          if (repeatEle)
>+          {
>+            nsAutoString localname, namespaceURI;
>+            
>+            nsCOMPtr<nsIDOMNode> node(do_QueryInterface(repeatEle));
>+            PRInt32 currIndex = 1;
>+            node->GetLocalName(localname);
>+            if (localname.EqualsLiteral("repeat")) {
>+              node->GetNamespaceURI(namespaceURI);
>+              if (namespaceURI.EqualsLiteral(NS_NAMESPACE_XFORMS)) {
>+                // XXX TODO Get current index from the repeat element and
>+                //   return it.  Probably have to do this inside the
>+                //   xforms utility service.
>+   
>+                return aContext->recycler()->getNumberResult(currIndex, aResult);
>+              }
>+            }
>+             
>+          }
>+   
>+          // if not a repeat element, send xforms-compute-exception
>+          //   event to the model as per spec

Eep! This seems very scary. I'd much prefer if you threw some special errorcode
which was cought in the XFormsXPathEvaluator and then thrown from there.

>+          nsCOMPtr<nsIDOMEvent> event;
>+          nsCOMPtr<nsIDOMDocumentEvent> doc = do_QueryInterface(document);
>+          doc->CreateEvent(NS_LITERAL_STRING("Events"), 
>+                                getter_AddRefs(event));
>+          NS_ENSURE_TRUE(event, NS_ERROR_OUT_OF_MEMORY);
>+   
>+          event->InitEvent(NS_LITERAL_STRING("xforms-compute-exception"),
>+                           PR_TRUE, PR_FALSE);
>+   
>+          // find the model to send the event to.  We'll send it to the
>+          //   resolver's model.  
>+          //   XXX TODO Better double check this once this function works.
>+          nsCOMPtr<nsIDOMNode> modelNode;
>+          nsCOMPtr<nsIXFormsUtilitiesService>xformsService = 
>+                do_GetService("@mozilla.org/xforms-utility-service;1", &rv);
>+          NS_ENSURE_SUCCESS(rv, rv);
>+   
>+          rv = xformsService->GetModelFromNode(mResolverNode, 
>+                                               getter_AddRefs(modelNode));
>+          NS_ENSURE_SUCCESS(rv, rv);
>+          nsCOMPtr<nsIDOMEventTarget> target = do_QueryInterface(modelNode);
>+          PRBool cancelled;
>+          return target->DispatchEvent(event, &cancelled);
>+        }
>+      }
>+   
>+      return NS_OK;
>+#endif /* if 0 */
>+      return NS_ERROR_NOT_IMPLEMENTED;
>+    }
>+    case INSTANCE:

note to self: I need to review this function

>+    case MAX:
>+    {
>+      nsRefPtr<txNodeSet> nodes;
>+      nsresult rv = evaluateToNodeSet((Expr*)iter.next(), aContext,
>+                                      getter_AddRefs(nodes));
>+      NS_ENSURE_SUCCESS(rv, rv);
>+   
>+      double res = Double::NaN, test = 0;
>+      PRInt32 i;
>+      for (i = 0; i < nodes->size(); ++i) {
>+        nsAutoString resultStr;
>+        txXPathNodeUtils::appendNodeValue(nodes->get(i), resultStr);
>+        test = Double::toDouble(resultStr);
>+        if (Double::isNaN(test))
>+        {

Move '{'. There's more of these, i'll stop commenting.

>+          res = Double::NaN;
>+          break;
>+        }
>+        if ((test > res) || (i==0))

You don't need this many parenthesis. And put whitespace around '=='. Same in
MIN.

>+        {
>+          res = test;
>+        }
>+      }
>+   
>+      return aContext->recycler()->getNumberResult(res, aResult);
>+    }

>+    case NOW:
>+    {
>+      PRExplodedTime time;
>+      char ctime[60];
>+   
>+      PR_ExplodeTime(PR_Now(), PR_LocalTimeParameters, &time);
>+      int gmtoffsethour = time.tm_params.tp_gmt_offset < 0 ? -1*time.tm_params.tp_gmt_offset / 3600 : time.tm_params.tp_gmt_offset / 3600;

Wrap at 80 columns

>+      int remainder = time.tm_params.tp_gmt_offset%3600;
>+      int gmtoffsetminute = remainder ? remainder/60 : 00;
>+      char zone_location[20];
>+      sprintf(zone_location, "%c%02d:%02d\0", 
>+              time.tm_params.tp_gmt_offset < 0 ? '-' : '+',
>+              gmtoffsethour, gmtoffsetminute);

Are you sure there can be no buffer overruns here? There are safe versions of
sprintf in NSPR.

>+   
>+      PR_FormatTime(ctime, sizeof(ctime), "%Y-%m-%dT%H:%M:%S\0", &time);
>+      strcat(ctime, zone_location);

Again, are you sure ctime is big enough?

>+      NS_ConvertASCIItoUTF16 localtime(ctime);
>+   
>+      return aContext->recycler()->getStringResult(localtime, aResult);

Could you just do |...getStringResult(NS_Convert...(ctime), aResult);|?

>+    case SECONDS:

note to self: Need to review this function.

Comment 19

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

I havn't looked through nsXFormsUtilitiesService. It'd probably be better if
some xforms knowledgeable person did that.

Comment 20

[Darin Fisher]

Comment on attachment 168949 [details] [diff] [review]
proposed fix

>Index: extensions/xforms/nsXFormsModule.cpp

>+  { "XForms Utility Service",
>+    NS_XFORMSUTILITIESSERVICE_CID,
>+    NS_XFORMS_UTILITIES_CONTRACTID,
>+    nsXFormsUtilitiesServiceConstructor },

Is it the "Utility" service or the "Utilities" service?  I think
it'd be good to be consistent.	I prefer "Utility" FWIW.


>+++ extensions/transformiix/source/xpath/nsIXFormsUtilitiesService.idl	2004-12-16 16:39:49.000000000 -0600

>+/**
>+ * Private interface implemented by the instance element.
>+ */

by the instance element, really?  typo, right?


>+  /**
>+   * Function to see if the given node is associated with the given model.
>+   */
>+  PRBool isThisModelAssocWithThisNode(in nsIDOMNode aModel, in nsIDOMNode aNode);

I think the description of this method could be better.  I had to
read the source to figure out what it does.  Also, the name seems
overly verbose to me.  I'd prefer: IsNodeAssocWithModel.


>+   * Function to ensure that aValue is of the shema type aType.  Will basically

typo: schema


>+// bb0d9c8b-3096-4b66-92a0-6c1ddf80e65f
>+#define NS_XFORMSUTILITIESSERVICE_CID \
>+{ 0xbb0d9c8b, 0x3096, 0x4b66, { 0x92, 0xa0, 0x6c, 0x1d, 0xdf, 0x80, 0xe6, 0x5f }}
>+
>+#define NS_XFORMS_UTILITIES_CONTRACTID   "@mozilla.org/xforms-utility-service;1"

I don't think the XPath module needs to know the ClassID of the
XForms implementation.	It should be sufficient to just record
the ContractID here.


>+++ extensions/xforms/nsXFormsUtilitiesService.cpp	2004-12-16 18:48:50.000000000 -0600

>+NS_IMETHODIMP
>+nsXFormsUtilitiesService::GetModelFromNode(nsIDOMNode *aNode, 
>+                                           nsIDOMNode **aModel)
>+{
>+  NS_ENSURE_ARG(aNode);
>+  NS_ENSURE_ARG_POINTER(aModel);

runtime null checks are not required here (from a mozilla coding
perspective).  it is sufficient to simply add some NS_ASSERTIONs.
let the caller null-check if he is unsure of what he is giving you
since in most cases he'll know that he is passing you something
valid.


>+  nsCOMPtr<nsIDOMElement> element = do_QueryInterface(aNode);
>+  nsCOMPtr<nsIXTFElement> xtfele = do_QueryInterface(aNode);

If you are going to do the XTF element check, then you might as
well do it up front, and early return if not satisfied.

Same goes for the namespace check.  Better to early return instead
of indenting your code.

Also, I don't see *aModel being initialized to NULL.  What if it
is not NULL going into this function and GetModel fails?


>+NS_IMETHODIMP
>+nsXFormsUtilitiesService::IsThisModelAssocWithThisNode(nsIDOMNode *aModel,
>+                                                    nsIDOMNode *aNode,
>+                                                    PRBool *aModelAssocWithNode)

nit: indentation is off


>+  NS_ENSURE_ARG(aModel);
>+  NS_ENSURE_ARG(aNode);
>+  NS_ENSURE_ARG_POINTER(aModelAssocWithNode);

again, this null-checking is gratuitous.  you are writing
the code that calls this API, so you can ensure that it
passes valid objects.  same nit applies elsewhere.


>+nsXFormsUtilitiesService::GetInstanceDocumentRoot(const nsAString& aID,
>+                                           nsIDOMNode *aModelNode,
>+                                           nsIDOMNode **aInstanceRoot)

nit: indentation


>+  if (!aID.IsEmpty()) {

early return in this case so you don't have to indent everything
else.


>+    nsCOMPtr<nsIDOMElement> element;
>+    rv = doc->GetDocumentElement(getter_AddRefs(element));
>+
>+    if (element) {
>+      nsCOMPtr<nsIDOMNode> node = do_QueryInterface(element);
>+      NS_IF_ADDREF(*aInstanceRoot = element);

if you're going to capture |rv| from GetDocumentElement,
then you might as well do a NS_ENSURE_SUCCESS(rv, rv);

also, there is no reason to QI from nsIDOMElement to nsIDOMNode
since a nsIDOMElement "is a" nsIDOMNode.  doh!	in fact, you 
don't even use the result of the QI ;-)


>+  nsXFormsUtilitiesService(){};
>+
>+private:
>+  ~nsXFormsUtilitiesService(){};

no need to code up the empty ctor and dtor.

Comment 21

[Axel Hecht]

(In reply to comment #20)
<...>
> 
> >+  /**
> >+   * Function to see if the given node is associated with the given model.
> >+   */
> >+  PRBool isThisModelAssocWithThisNode(in nsIDOMNode aModel, in nsIDOMNode
aNode);
> 
> I think the description of this method could be better.  I had to
> read the source to figure out what it does.  Also, the name seems
> overly verbose to me.  I'd prefer: IsNodeAssocWithModel.

Nit, if you change the order of Node and Model in the method name, change their
order in the argument list, too.

Comment 22

[Axel Hecht]

note to all, I just started to hack on tx-xpcom wrappers last night. I'm about one
third to half done, though right now, the stuff is obviously only compiling.
But it seems to be rather trivial so far. I'll do more tonight and hopefully 
attach a proposed patch soon.

Comment 23

[Axel Hecht]

Attachment: partly impl of tx <-> xpcom bridge

Here is patch for Peter and Jonas to look at.

Basically, for the xforms stuff I'd propose to add an interface 
nsPIXPathEvaluator to our object with would add a "addFunctionResolver" to it.
That would take an
txIFunctionResolver object, implemented by the xforms module.
The rest is pretty much straightforward.
What I didn't manage to do yet is getting nodesets as function return values 
working. That requires something to own the generated txXPathNodes, maybe the
txExecutionState would need to do that. (Likely, even.) And passing RTFs into
functions. I'd probably do that like a call to exslt:node-set() first.

Anyway, I'd like to get some initial feedback on whether I should try to get
something like this up or rather not.

For generic extension functions, I'd go with the category manager, I guess.
I don't see us doing the nodeset function externally, btw. That'd be hell.

Comment 24

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

This looks great. Though it needs some work of course. But I think we should
deal with this in another bug, blocking this one. I defenetly agree that a
exslt:node-set function should be 'native' to transformiix (that's the one you
ment, right?)

Comment 25

[aaronr]

I agree, let's deal with it in a seperate bug, but please NOT blocking this one.
 At least not unless we can be sure that it will make it into 1.8b.  I don't
know that we've finalized a schedule, yet, but we'd like to at least entertain
the possibility of paralleling the 1.8 schedule.  Possibly making something
available that can run on 1.8b and again on 1.8 release.

I am almost done with all of the changes resulting from the comments above. 
Just need time to finish a couple of the suggestions.  Should definitely have
another patch ready by the weekend.

Comment 26

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

Aaron, the problem is that I don't think that any of the xpath people are really
happy with having the xforms stuff always built inside transformiix (I didn't
know that was the plan until yesterday). And if you guys want to have xforms be
a dropin that works across browser versions then we can't just fix this later
since it'll break the xforms plugin. However i'm not sure if you're using other
unfrozen interfaces (nsIAtom might be one), so xpath might not make a
difference? It would be very helpfull to know what your plans are.

That said, it's not impossible that a tx-xpcom bridge will be done for 1.8.
Though of course it's up to pike since he's doing the work.

Comment 27

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

Aaron, you also need to add some way to set the contextsize when evaluating an
expression. One way to do this would be to add an another interface to
nsXPathExpression that looks something like

nsIXPathExpressionInternal
{
  attribute unsigned long        contextSize;
  attribute unsigned long        contextPosition;
}

(i'd also like to add something like setVariableValue to it sometime in the
future). Then modify nsXPathExpression::EvalContextImpl::size() and position().

both contextSize and contextPosition should default to 1.

Comment 28

[aaronr]

Attachment: fix incorporating reviews

proposed fix taking into account review comments.  Following comments NOT
incorporated:
1) "Get rid of CreateNSResolver and pass Evalute a node instead".  I need to
clarify with Jonas.
2) "What should happen if one of the values can't be converted to a number".  I
have asked W3C reps to clarify.  Right now we handle like other XForms
processors and return NaN.

Comment 29

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

Comment on attachment 171481 [details] [diff] [review]
fix incorporating reviews

Hmm.. Since it now looks like the xforms code is going to just temporarily live
in transformiix i'm not sure that it's worth creating an extensions subdir.
Peterv, Pike, oppinions?

You should still put all changes inside #ifdef MOZ_XFORMS. This way people that
doesn't care about xforms (like embedding) can disable it rather then disable
all of transformiix.  It'll also make it easier to find the stuff that we can
cut out once the xforms code can be moved to the xforms dll.

Don't forget to remove the changes to nsXPathNSResolver.h when you start
passing a node rather then an nsIDOMXPathNSResolver.

Why all the #if 0-disabled stuff?

>+nsXFormsXPathEvaluator::CreateNSResolver(nsIDOMNode *aNodeResolver,
>+                                   nsIDOMXPathNSResolver **aResult)

Ugh, defenetly get rid of this function once you're no longer implementing the
nsIDOMXPathEvaluator interface. And I see you havn't fixed your indentation...
Don't make me come over there!

>+++ transformiix\source\xpath\extensions\xforms\XFormsFunctionCall.cpp	2005-01-17
...
>+    case INSTANCE:
>+    {
>+      nsresult rv;
>+      if (!requireParams(1, 1, aContext))
>+        return NS_ERROR_XPATH_BAD_ARGUMENT_COUNT;
>+   
>+      // mResolverNode is the node in the XForms document that contained
>+      //   the expression we are evaluating.  We'll use this to get the
>+      //   document.  If this isn't here, then something is wrong. Bail.
>+      if (!mResolverNode) {
>+        return NS_ERROR_FAILURE;
>+      }

You really should have checked this when creating the XFormsFunctionCall
object, so all that should be needed here is an assertion.

>+      if (!instanceId.IsEmpty()) {

Do you really need this check? Won't the right thing happen anyway? If you do
(or want to optimize for when it happens) please add an earlyreturn for when
instanceId is empty rather then indenting the whole implementation below.

>+        // here document is the XForms document
>+        nsCOMPtr<nsIDOMDocument> document;
>+        rv = mResolverNode->GetOwnerDocument(
>+                                         getter_AddRefs(document));
>+        NS_ENSURE_SUCCESS(rv, rv);

You should check that you're not getting a null document here. GetOwnerDocument
won't return an errorvalue if you do (it'll only happen if the document has
been deleted, so it's quite unlikly, but just in case).

>+        nsCOMPtr<nsIDOMElement> instEle;
>+        rv = document->GetElementById(instanceId, 
>+                                      getter_AddRefs(instEle));
>+   
>+        PRBool foundInstance = PR_FALSE;
>+        nsAutoString localname, namespaceURI;
>+        if (instEle)
>+          instEle->GetLocalName(localname);
>+        if (!localname.IsEmpty() && localname.EqualsLiteral("instance")) {

No need to test for emptyness, it's it's equal to the literal there no way it
can be empty, right :). Goes for namespaceURI too. Also, you could simplify the
code a tad by getting both localname and namespace at the same time and
checking them in a single if. It's quite unlikly that they both won't be true
so there's no point in trying to save a few cycles.

>+          instEle->GetNamespaceURI(namespaceURI);
>+          if (!namespaceURI.IsEmpty() && 
>+              namespaceURI.EqualsLiteral(NS_NAMESPACE_XFORMS)) {
>+              foundInstance = PR_TRUE;
>+          }
>+        }
>+   
>+        if (!foundInstance) {
>+          // We found a node, but it
>+          //   return it.
>+          *aResult = resultSet;
>+          NS_ADDREF(*aResult);
>+      
>+          return NS_OK;

That comment seems to lack a few words.

>+        }
>+   
>+        // Make sure that this element is contained in the same
>+        //   model as the context node of the expression as per
>+        //   the XForms 1.0 spec.
>+   
>+        // first step is to get the contextNode passed in to
>+        //   the evaluation
>+   
>+        const txXPathNode& xpNode = aContext->getContextNode();
>+        nsCOMPtr<nsIDOMNode> xfContextNode;
>+        rv = txXPathNativeNode::getNode(xpNode, 
>+                                   getter_AddRefs(xfContextNode));

Line up the arguments. And pass aContext->getContextNode() directly to getNode.

>+        NS_ENSURE_SUCCESS(rv, rv);
>+   
>+        // now see if the node we found (instEle) and the 
>+        //   context node for the evaluation (xfContextNode) link
>+        //   back to the same model. 
>+        nsCOMPtr<nsIXFormsUtilityService>xformsService = 
>+              do_GetService("@mozilla.org/xforms-utility-service;1", &rv);
>+        NS_ENSURE_SUCCESS(rv, rv);
>+   
>+        nsCOMPtr<nsIDOMNode> instNode, modelInstance;
>+        instNode = do_QueryInterface(instEle);
>+        rv = xformsService->GetModelFromNode(instNode, 
>+                                   getter_AddRefs(modelInstance));

And here.

>+        NS_ENSURE_SUCCESS(rv, rv);
>+   
>+        PRBool modelContainsNode = PR_FALSE;
>+        rv = xformsService->IsNodeAssocWithModel(xfContextNode, 
>+                                                 modelInstance, 
>+                                                 &modelContainsNode);
>+        NS_ENSURE_SUCCESS(rv, rv);
>+   
>+        if (modelContainsNode) {
>+          // ok, we've found an instance node with the proper id
>+          //   that fulfills the requirement of being from the
>+          //   same model as the context node.  Now we need to
>+          //   return a 'node-set containing just the root
>+          //   element node of the referenced instance data'.
>+          //   Wonderful.
>+   
>+            nsCOMPtr<nsIDOMNode> instanceRoot;
>+            rv = xformsService->GetInstanceDocumentRoot(
>+                                  instanceId,
>+                                  modelInstance,
>+                                  getter_AddRefs(instanceRoot));
>+            NS_ENSURE_SUCCESS(rv, rv);
>+   
>+            nsAutoPtr<txXPathNode> txNode(txXPathNativeNode::createXPathNode(instanceRoot));
>+            if (txNode) {
>+              resultSet->add(*txNode);
>+          }
>+        }

That entire |if| needs reindenting.
Man.. you don't like to line stuff up, do you :)


>+    case MAX:
...
>+      double res = Double::NaN, test = 0;

No need to init test to 0. And you could declare it inside the loop where it's
first used. Same for MIN.

>+    case NOW:
...
>+      char zone_location[40];
>+      const int zoneBufSize = sizeof(zone_location);

Hmm.. i wonder if you need to subtract one here, or if PR_snprintf will
nullterminate even when running out of bufferspace. Same for ctime.

>+    case SECONDS:
...
>+            while(end != start) {
>+              --end;
>+              if( *end == 'S' ) {
>+                subend = end;
>+                substart = end;
>+                while( (*substart>='0') && (*substart<='9') ) {

make that
while (*substart >= '0' && *substart <= '9')

>+                  --substart;
>+                }
>+                end = substart;
>+                ++substart;
>+                seconds = Substring(substart, subend);
>+              } /* seconds */
>+              else if( *end == 'M' ) {
>+                subend = end;
>+                substart = end;
>+                while( (*substart>='0') && (*substart<='9') ) {
>+                  --substart;
>+                }
>+                end = substart;
>+                ++substart;
>+                seconds = Substring(substart, subend);

This should be |minutes = Sub...| right? Same error further down.

Comment 30

[aaronr]

You are right, my indentations do look sloppy.  I guess I just ended up fixing
them in the file you complained about before and not throughout all of my files.
 Doh!

I'm against #ifdef XFORMS.  I wouldn't mind something like #ifndef
DISABLE_XFORMS or some such.  Until the extensibility of XPath is all done,
tested, etc., we need our XPath additions to be available to default Mozilla
builds so that users can grab the latest Mozilla and the latest XForms extension
and just work.

I am #if 0'ing stuff that still needs tie ins on the XForms side.  Like
currently we don't have a way to get the index from a repeat element and we are
waiting for some of the schema stuff to get tied in with XForms to more easily
format values from xsd:duration parameters.

Should have a new patch ready by the end of the day.  I hope!

Comment 31

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

sure, DISABLE_XFORMS works fine too. Though I was thinking that the MOZ_XFORMS
define would be on by default resulting in the same thing. Though possibly a
better name would then be MOZ_XFORMS_HOOKS. I'll leave such politics to you
xforms people to fight out with mozilla.org :)

Comment 32

[aaronr]

Attachment: proposed fix

incorporates Jonas' suggestions, including a compile flag for disabling xforms
hooks in transformiix (DISABLE_XFORMS_HOOKS).  The size difference of
transformiix in my latest optimized trunk build was 196,608 with my patch,
188,416 without.

Comment 33

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

Comment on attachment 172045 [details] [diff] [review]
proposed fix

>Index: xforms/nsXFormsUtils.cpp
...
>@@ -333,56 +334,60 @@ nsXFormsUtils::EvaluateXPath(const nsASt
...
>   /// @todo Evaluate() should use aContextPosition and aContextSize
>   nsCOMPtr<nsISupports> supResult;
>-  expression->Evaluate(aContextNode,
>+  nsresult rv = expression->Evaluate(aContextNode,
>                        aResultType,
>                        nsnull,
>                        getter_AddRefs(supResult));

Dude!

>   nsIDOMXPathResult *result = nsnull;
>   if (supResult) {

You shouldn't rely on supResult being null in case of error here. It'll always
be true, but it's bad XPCOM style to use result-values when an error is
returned. Instead check NS_SUCCEEDED(rv) and rely on that subResult is set in
that case.

...
>+  else {
>+    if(rv == NS_ERROR_XFORMS_CALCUATION_EXCEPTION) {
>+      nsCOMPtr<nsIDOMElement> resolverElement = do_QueryInterface(aResolverNode);
>+      nsCOMPtr<nsIDOMNode> model = GetModel(resolverElement);
>+      DispatchEvent(model, eEvent_ComputeException);
>+    }
>+  }

Should you maybe dispatch an event here always? XPath expressions can fail for
other reasons too. Like "count(2)" will fail.


>Index: xforms/xforms.css
...
>+  border: 10px solid red;

I don't think you want to check in this :)

>+++ transformiix/source/xpath/XFormsFunctionCall.cpp	2005-01-21 13:00:22.948017600 
...
>+    case INSTANCE:
>+    {
>+      nsresult rv;
>+      if (!requireParams(1, 1, aContext))
>+        return NS_ERROR_XPATH_BAD_ARGUMENT_COUNT;
>+   
>+      // mResolverNode is the node in the XForms document that contained
>+      //   the expression we are evaluating.  We'll use this to get the
>+      //   document.  If this isn't here, then something is wrong. Bail.
>+      if (!mResolverNode) {
>+        return NS_ERROR_FAILURE;
>+      }

You want to keep this as is?

>+   
>+      nsRefPtr<txNodeSet> resultSet;
>+      rv = aContext->recycler()->getNodeSet(getter_AddRefs(resultSet));
>+      NS_ENSURE_SUCCESS(rv, rv);
>+   
>+      nsAutoString instanceId;
>+      evaluateToString((Expr*)iter.next(), aContext, instanceId);
>+   
>+      // here document is the XForms document
>+      nsCOMPtr<nsIDOMDocument> document;
>+      rv = mResolverNode->GetOwnerDocument(getter_AddRefs(document)); 
>+      NS_ENSURE_SUCCESS(rv, rv);
>+      NS_ENSURE_STATE(document);

Ugh, I would not use that macro. That's for checking membervariables and the
likes iirc. Use NS_ENSURE_TRUE(document, NS_ERROR_NULL_POINTER)

>>+    case NOW:
>...
>>+      char zone_location[40];
>>+      const int zoneBufSize = sizeof(zone_location);
>
>Hmm.. i wonder if you need to subtract one here, or if PR_snprintf will
>nullterminate even when running out of bufferspace. Same for ctime.

Did you check that comment?

You didn't make any changes to the SECONDS code.

Whaa!! Don't create a new nsXPathNSResolver for every resolveNamespacePrefix
call! Either create one when you set up the context, or, even better, just do
what nsXPathNSResolver does. See

http://lxr.mozilla.org/mozilla/source/extensions/transformiix/source/xpath/nsXP
athNSResolver.cpp#62

Strictly speaking you don't need to call CanCallerAccess since this class can't
be called from script (it doesn't have security stuff set up and has no idl
interface).

Comment 34

[aaronr]

Attachment: better patch than last patch

(In reply to comment #33)
> (From update of attachment 172045 [details] [diff] [review] [edit])
> >Index: xforms/nsXFormsUtils.cpp
> ...
> >@@ -333,56 +334,60 @@ nsXFormsUtils::EvaluateXPath(const nsASt
> ...
> >   /// @todo Evaluate() should use aContextPosition and aContextSize
> >   nsCOMPtr<nsISupports> supResult;
> >-  expression->Evaluate(aContextNode,
> >+  nsresult rv = expression->Evaluate(aContextNode,
> >			   aResultType,
> >			   nsnull,
> >			   getter_AddRefs(supResult));
> 
> Dude!
> 

b*tch!	Dammit, I was doing like 5 things at once and ended up creating the
patch from the wrong tree!  Ugh!  Well, that explains a couple of these
problems.

> >   nsIDOMXPathResult *result = nsnull;
> >   if (supResult) {
> 
> You shouldn't rely on supResult being null in case of error here. It'll
always
> be true, but it's bad XPCOM style to use result-values when an error is
> returned. Instead check NS_SUCCEEDED(rv) and rely on that subResult is set in

> that case.
> 

This test was already here, I just built off of the test.  But I will add a
check for NS_SUCCEEDED.

> ...
> >+  else {
> >+	if(rv == NS_ERROR_XFORMS_CALCUATION_EXCEPTION) {
> >+	  nsCOMPtr<nsIDOMElement> resolverElement =
do_QueryInterface(aResolverNode);
> >+	  nsCOMPtr<nsIDOMNode> model = GetModel(resolverElement);
> >+	  DispatchEvent(model, eEvent_ComputeException);
> >+	}
> >+  }
> 
> Should you maybe dispatch an event here always? XPath expressions can fail
for
> other reasons too. Like "count(2)" will fail.
> 

I was just looking to generate the proper XForms exception for this one
condition since it was specific to one of my XPath extension functions.  It is
another bug (https://bugzilla.mozilla.org/show_bug.cgi?id=278448) to generate
all of the other events that XForms requires.

> 
> >Index: xforms/xforms.css
> ...
> >+  border: 10px solid red;
> 
> I don't think you want to check in this :)

This was the tipoff that I grabeed the patch from the wrong tree.  Doh!

> 
> >+++ transformiix/source/xpath/XFormsFunctionCall.cpp 2005-01-21
13:00:22.948017600 
> ...
> >+	case INSTANCE:
> >+	{
> >+	  nsresult rv;
> >+	  if (!requireParams(1, 1, aContext))
> >+	    return NS_ERROR_XPATH_BAD_ARGUMENT_COUNT;
> >+   
> >+	  // mResolverNode is the node in the XForms document that contained
> >+	  //   the expression we are evaluating.  We'll use this to get the
> >+	  //   document.  If this isn't here, then something is wrong. Bail.
> >+	  if (!mResolverNode) {
> >+	    return NS_ERROR_FAILURE;
> >+	  }
> 
> You want to keep this as is?
> 

Again, from wrong tree.  Fixed in latest patch.

> >+   
> >+	  nsRefPtr<txNodeSet> resultSet;
> >+	  rv = aContext->recycler()->getNodeSet(getter_AddRefs(resultSet));
> >+	  NS_ENSURE_SUCCESS(rv, rv);
> >+   
> >+	  nsAutoString instanceId;
> >+	  evaluateToString((Expr*)iter.next(), aContext, instanceId);
> >+   
> >+	  // here document is the XForms document
> >+	  nsCOMPtr<nsIDOMDocument> document;
> >+	  rv = mResolverNode->GetOwnerDocument(getter_AddRefs(document)); 
> >+	  NS_ENSURE_SUCCESS(rv, rv);
> >+	  NS_ENSURE_STATE(document);
> 
> Ugh, I would not use that macro. That's for checking membervariables and the
> likes iirc. Use NS_ENSURE_TRUE(document, NS_ERROR_NULL_POINTER)
> 

See, and another Mozilla guy told me to use this test for another bug I fixed. 
Man....  I like this error better than NS_ERROR_UNEXPECTED which is returned
from NS_ENSURE_STATE, so I will bow to your wisdom :=)

> >>+	 case NOW:
> >...
> >>+	   char zone_location[40];
> >>+	   const int zoneBufSize = sizeof(zone_location);
> >
> >Hmm.. i wonder if you need to subtract one here, or if PR_snprintf will
> >nullterminate even when running out of bufferspace. Same for ctime.
> 
> Did you check that comment?
> 

I did check this out, but forgot to comment this bug.  PR_snprintf calls
PR_vsnprintf to do most of the work and that function will always do *(ss.cur -
1) = '\0'.  Since this unconditionally null terminates, I won't worry about it.



> You didn't make any changes to the SECONDS code.
> 

another forgotten comment.  My bad, I know that I need to be more diligent.  I
didn't change SECONDS since it is #if 0'd right now and it won't remain like
this when this function is eventually implemented.  Doron's schema work has the
utility functions that will do this same kind of parsing and that is what I
will use in xformsService->SecondsFromDuration() since XForms will already have
access to those utility functions.  I have removed the offending code so that
no one will accidently use it.

> Whaa!! Don't create a new nsXPathNSResolver for every resolveNamespacePrefix
> call! Either create one when you set up the context, or, even better, just do

> what nsXPathNSResolver does. See
> 
>
http://lxr.mozilla.org/mozilla/source/extensions/transformiix/source/xpath/nsXP

> athNSResolver.cpp#62
> 

Done.

> Strictly speaking you don't need to call CanCallerAccess since this class
can't
> be called from script (it doesn't have security stuff set up and has no idl
> interface).
> 

Done.  Removed.

Comment 35

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

Comment on attachment 172135 [details] [diff] [review]
better patch than last patch

Nit: In XFormsParseContextImpl::resolveNamespacePrefix, rather then setting ns
to null and then further down returning error if it is null, you could just
return error right away. Possibly even using NS_ENSURE_TRUE(dom3Node,
NS_ERROR_DOM_NAMESPACE_ERR).

r=me!!

Comment 36

[aaronr]

Comment on attachment 172135 [details] [diff] [review]
better patch than last patch

updating patch for trunk.

Comment 37

[aaronr]

Attachment: update patch for trunk

made changes to update patch to trunk and take into account txIXPathContext
changes that Jonas made for 277777

Comment 38

[Axel Hecht]

Comment on attachment 172308 [details] [diff] [review]
update patch for trunk

Does this patch include xpidl generated headers but no idl? Or how do the
nsIXFormsUtilityService.h and nsIXFormsXPathEvaluator.h come about?
I have nothing against those being non-scriptable interfaces, by why all the
magic generating those header files by hand? Or not checking in the idls?

Comment 39

[aaronr]

I had .idl's and then Jonas wisely suggested that it might be best to not have
.idl's since we really didn't care if our interfaces were available to script. 
Since I already had the generated headers, I just cleaned them up quickly and
put them in the patch.  No special reason that they look like that.  They appear
that way simply out of convenience.

Comment 40

[Axel Hecht]

(In reply to comment #39)
> I had .idl's and then Jonas wisely suggested that it might be best to not have
> .idl's since we really didn't care if our interfaces were available to script. 
> Since I already had the generated headers, I just cleaned them up quickly and
> put them in the patch.  No special reason that they look like that.  They appear
> that way simply out of convenience.

I'd put them in idl anyway. Unless you want to de-XPCOM stuff in there, there's
no need to code header files by hand. Whether an interface is scriptable or not
is just a flag in the idl, that has nothing to do with whether it is in an IDL
file or a .h.

Comment 41

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

the downside with .idls is that they create xpt files, even if they only contain
non-scriptable interfaces.

Comment 42

[Peter Van der Beken [:peterv]]

Comment on attachment 172308 [details] [diff] [review]
update patch for trunk

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

>@@ -56,16 +56,17 @@ REQUIRES +=	  dom \

> endif
> 
>+

Don't add blank lines.

>Index: xforms/nsXFormsUtils.cpp
>===================================================================

>+      rv = analyzer.Analyze(aContextNode,
>+                            xNode,
>+                            expression,
>+                            &aExpression,
>+                            aSet);
>       NS_ENSURE_SUCCESS(rv, nsnull);
>     }
>     CallQueryInterface(supResult, &result);  // addrefs
>   }
>+  else {
>+    if(rv == NS_ERROR_XFORMS_CALCUATION_EXCEPTION) {

Space before brace and make this an |else if|.

>+class NS_NO_VTABLE nsIXFormsXPathEvaluator : public nsISupports {
>+ public: 
>+
>+  NS_DEFINE_STATIC_IID_ACCESSOR(TRANSFORMIIX_XFORMS_XPATH_EVALUATOR_CID)

This is just wrong. This is supposed to be an IID for the interface, not the
CID for the component.

>+/* Gotta do this via the service since we don't want transformiix to require
>+ * any of the new extensions, like schema-validation
>+ */

This isn't inside transformiix anyway?

Remove the #if 0 bits.
I didn't really review this very thoroughly since most of it is temporary.
Let's get this out of Transformiix again soon. :-)

Comment 43

[aaronr]

Attachment: last patch for trunk

patch for trunk with peterv's comment fixes.

Comment 44

[Doron Rosenberg (IBM)]

checked in

Comment 45

[Doron Rosenberg (IBM)]

checked in

Comment 46

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Significant parts of this checkin were landed with CR-LF line endings instead of
LF line endings.  This broke some tinderboxes (a few Unix compilers choke
unconditionally, IIRC, but a bunch more choke on multi-line macros with CR-LF
since the backslash escapes the CR instead of the newline).

I fixed the line endings.