418734 - [1.1] Implement XPath function: context()

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Merle Sterling]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1; .NET CLR 1.1.4322; .NET CLR 2.0.50727)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008020507 Minefield/3.0b4pre

We need to implement the XPath function context().


Reproducible: Always

Comment 1

[Merle Sterling]

Attachment: testcase: context()

Comment 2

[Merle Sterling]

Attachment: patch

Note that context is a reserved word in .idl so we have to rewrite the expression to rename the function to be called. The real function is named contextNode.

Comment 3

[Merle Sterling]

Attachment: patch for 1.8 branch

Comment 4

[aaronr]

Comment on attachment 304625 [details] [diff] [review]
patch


>Index: nsXFormsUtils.cpp
>===================================================================

>+  if (start != kNotFound) {
>+    if (start > 0) {
>+      // Append any part of the expression before 'context()'
>+      expr.Append(Substring(aExpression, start));
>+    }
>+    // Replace 'context()' with 'contextNode()'
>+    expr.AppendLiteral("contextNode()");
>+    if (start + contextExpr.Length() > aExpression.Length()) {
>+      // Append any remaining part of the expression after 'context()'
>+      expr.Append(Substring(aExpression, start + contextExpr.Length()));
>+    }

shouldn't that be '<'?  And shouldn't your Substring call begin at start+contextExpr.Length() and not end there?

>Index: nsXFormsXPathFunctions.cpp
>===================================================================
>+NS_IMETHODIMP
>+nsXFormsXPathFunctions::ContextNode(txIFunctionEvaluationContext *aContext,
>+                                    txINodeSet **aResult)
>+{

...

>+
>+  // Get the context node of the xforms node.
>+  nsCOMPtr<nsIDOMNode> contextNode;
>+  PRUint32 contextNodesetSize = 0;
>+  PRInt32 contextPosition;
>+  nsCOMPtr<nsIModelElementPrivate> model;
>+  nsCOMPtr<nsIDOMElement> bindElement;
>+  nsCOMPtr<nsIXFormsControl> parentControl;
>+  PRBool outerBind;
>+
>+  nsCOMPtr<nsIDOMElement> element(do_QueryInterface(xfNode));
>+  if (!element) {
>+    result.swap(*aResult);
>+    return NS_OK;
>+  }

nit: this failure should never, ever happen since our xpath ns resolver node should always be an element.  But if it does, it is more serious than returning a successful return code and an empty nodeset.  I'd say either NS_ENSURE_TRUE(element, NS_ERROR_FAILURE) like you do if there is no xfNode or just don't do a check for element (in a similar situation in ::Index we do just that...assume the QI worked).

Also by eliminating the need to be able to return an empty nodeset in case of error, you can now move the do_CreateInstance down to after the return code check for GetNodeContext.  That way we don't do this create instance until we know we've passed all of our error checks.

r-'ing for the logic error and Substring error above.

Comment 5

[Merle Sterling]

(In reply to comment #4)
> (From update of attachment 304625 [details] [diff] [review])
> >Index: nsXFormsUtils.cpp
> >===================================================================
> >+  if (start != kNotFound) {
> >+    if (start > 0) {
> >+      // Append any part of the expression before 'context()'
> >+      expr.Append(Substring(aExpression, start));
> >+    }
> >+    // Replace 'context()' with 'contextNode()'
> >+    expr.AppendLiteral("contextNode()");
> >+    if (start + contextExpr.Length() > aExpression.Length()) {
> >+      // Append any remaining part of the expression after 'context()'
> >+      expr.Append(Substring(aExpression, start + contextExpr.Length()));
> >+    }
> shouldn't that be '<'?  And shouldn't your Substring call begin at
> start+contextExpr.Length() and not end there?

No, it should be > 0. Find returns the starting index of the string so if the result is zero then the expression starts with context() and there are no characters before it that must be copied to the new expression.

The Substring call is wrong though; I was looking at the wrong overload and expected it to be the substring starting at zero and going for 'count' characters. Just need to change it to Substring(aExpression, 0, start).

Comment 6

[Merle Sterling]

Attachment: patch2

Fixing Substring call; rearranging code in ContextNode() function per Aaron's review comments.

Comment 7

[Merle Sterling]

Attachment: patch3

Comment 8

[aaronr]

Comment on attachment 304793 [details] [diff] [review]
patch3

Looks like you forgot the other EvaluateXPath in nsxformsutils.cpp.  Which means you should probably separate out the logic into a separate function.  Especially if we'll have to do something similar for id().

Comment 9

[Merle Sterling]

Attachment: patch4

Created a separate function, TranslateExpression, to do the work of rewriting the XPath expression so it can be called by both versions of EvaluateXPath.

Comment 10

[aaronr]

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


>Index: nsXFormsUtils.cpp
>===================================================================

>@@ -657,19 +663,25 @@ nsXFormsUtils::EvaluateXPath(nsIXPathEva
>                              nsIDOMXPathNSResolver      *aResolver,
>                              nsIXFormsXPathState        *aState,
>                              PRUint16                    aResultType,
>                              PRInt32                     aContextPosition,
>                              PRInt32                     aContextSize,
>                              nsIDOMXPathResult          *aInResult,
>                              nsIDOMXPathResult         **aResult)
> {
>+  // If the XPath expression contains a function name that differs from its
>+  // internal name, translate the external name to the internal name.
>+  nsAutoString expr;
>+  nsresult rv = TranslateExpression(aExpression, expr);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>   nsCOMPtr<nsIDOMXPathExpression> expression;
>-  nsresult rv = CreateExpression(aEvaluator, aExpression, aResolver, aState,
>-                                 getter_AddRefs(expression));
>+  rv = CreateExpression(aEvaluator, aExpression, aResolver, aState,
>+                        getter_AddRefs(expression));

Shouldn't you be passing 'expr' here and not aExpression?

with that, r=me

Comment 11

[Merle Sterling]

(In reply to comment #10)
> >+  rv = CreateExpression(aEvaluator, aExpression, aResolver, aState,
> >+                        getter_AddRefs(expression));
> Shouldn't you be passing 'expr' here and not aExpression?
> with that, r=me

Yep.

Comment 12

[Merle Sterling]

Attachment: patch5

Comment 13

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

Comment on attachment 304868 [details] [diff] [review]
patch5

>+    txINodeSet contextNode(in txIFunctionEvaluationContext aContext);
Add a comment that contextNode is translated from context or something

>+  // context() is a valid XPath function but context is a reserved word
>+  // in .idl so we rename it to contextNode() if it exists.
>+  NS_NAMED_LITERAL_STRING(contextExpr, "context()");

Is context( ) a valid function call? Or context(    ) ? or
context   (). See http://www.w3.org/TR/xpath#exprlex .
I think context(   ) is valid but context  () isn't, because
there is possible expression in context((expr)?), although in this case it is empty.
Replacing context( with contextNode( should be ok, I guess.

With those fixed or otherwise addressed, r=me

Comment 14

[Merle Sterling]

Only context() is valid but it can be part of a larger expression so I copy anything before it and after it to the renamed expression.

So context()/y becomes contextNode()/y. 

Comment 15

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

Comment on attachment 304626 [details] [diff] [review]
patch for 1.8 branch

this is still the right patch for 1.8?

Comment 16

[Merle Sterling]

(In reply to comment #15)
> (From update of attachment 304626 [details] [diff] [review])
> this is still the right patch for 1.8?

Yes, the 1.8 patch does not require renaming context() to contextNode() because the function is not defined using .idl.

Comment 17

[aaronr]

Comment on attachment 304626 [details] [diff] [review]
patch for 1.8 branch

the branch patch affects the xforms xpath evaluator which is used only by xforms

Comment 18

[Merle Sterling]

Attachment: patch for check-in

Updating to trunk.

Comment 19

[aaronr]

checked into trunk for msterlin

Comment 20

[aaronr]

Attachment: updated 1.8 patch

same as 1.8 branch patch, updated to current level

Comment 21

[aaronr]

checked in 1.8 branch patch for msterlin