Closed Bug 436864 Opened 12 years ago Closed 11 years ago

XPath needs to support an 'object' type as a parameter and return type

Categories

(Core :: XSLT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.1b2

People

(Reporter: msterlin, Unassigned)

References

Details

Attachments

(1 file, 2 obsolete files)

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.9pre) Gecko/2008050115 Minefield/3.0pre

The XForms 1.1 spec defines several new XPath functions that rely on an 'object' type as a parameter or return value. The intent is to allow the author to pass any of the XPath data types (boolean, number, string, nodeset) as well as return any type. The 'Choose' function is an example: http://www.w3.org/TR/xforms11/#fn-choose

The closet thing to an object is txINodeset since you can get a number, string, or boolean result from a Nodeset. However, txXPCOMExtensionFunctionCall::evaluate looks at the type of the parameter and calls evaluateToNodeset/Boolean/String/etc. If the type of the parameter is declared to be txINodeset but the evaluation results in anything other than a nodeset, NS_ERROR_XSLT_NODESET_EXPECTED is returned.

We need another type that can be anything just to get around the checking done by the various EvaluateTo methods. Note that the evaluation happens before the call to the XForms XPath processor so there is no way to work around the issue in the XForms code.






Reproducible: Always
Attached patch txAny patch (obsolete) — Splinter Review
This patch is an example of what would be needed to support 'object' XPath datatypes for XForms. The class is named txAnyResult because there is already a txObject. It follows the same inheritance hierarchy as the other txAExpr types and txAnyAdaptor is similar to txNodesetAdaptor because XForms currently needs to treat the result type as a txINodeSet.

It works for most examples of the 'Choose' function and should at least be a good starting point for anyone that wants to implement a generic XPath object type. Without it or something that behaves similarly, XForms cannot support any of the new XPath functions that are defined to accept 'object' parameters or return values.
Blocks: 436866
See bug436866
Attachment #323364 - Flags: review?(jonas)
Comment on attachment 323364 [details] [diff] [review]
txAny patch

Actually, this looks more like petervs area.
Attachment #323364 - Flags: review?(jonas) → review?(peterv)
Comment on attachment 323364 [details] [diff] [review]
txAny patch

Why do you even need txAnyResult?
I think I'd rather have a txXPathObjectAdaptor instead of txtxAnyAdaptor. Derive it from a txIXPathObject interface with a |[noscript, notxpcom] txAExprResultPtr getResult()| method. txNodeSetAdaptor should then probably derive from txExprResultAdaptor, and you should remove getTxNodeSet from txINodeSet (replace it with getTxNodeSet in callers).
Don't you need to be able to return an XPath object too? You'll need to add code to the end of txXPCOMExtensionFunctionCall::evaluate that handles that.
Attachment #323364 - Flags: review?(peterv) → review-
(In reply to comment #4)
> (From update of attachment 323364 [details] [diff] [review])
> Why do you even need txAnyResult?
> I think I'd rather have a txXPathObjectAdaptor instead of txtxAnyAdaptor.
You're right, I don't really need txAnyResult because I can just give the adaptor class a member variable of type txAExprResult. I do like the new name of txXPathObject and changed it to that.

> Derive it from a txIXPathObject interface with a |[noscript, notxpcom]
> txAExprResultPtr getResult()| method. txNodeSetAdaptor should then probably
> derive from txExprResultAdaptor, and you should remove getTxNodeSet from
> txINodeSet (replace it with getTxNodeSet in callers).
That's confusing to me. I assume you meant to derive txNodeSetAdaptor from txXPathObjectAdaptor so that the txXPathObjectAdaptor can inherit the implementations of the txINodeSet interface but that isn't quite what I need. I need the txIXPathObject interface to inherit from txINodeSet not the other way around as would be necessary to derive from txXPathObject. When I inherit from txINodeSet I then must implement those methods as I do in the txXPathObjectAdaptor.

> Don't you need to be able to return an XPath object too? You'll need to add
> code to the end of txXPCOMExtensionFunctionCall::evaluate that handles that.
Yes and I did in the previous patch. Difference now is simply that the names have been changed: the argument type is now eObject instead of eAny and I call evaluateToXPathObject instead of evaluatetoAnyResult.

For XForms, the XPath object I need is an object that implements txINodeSet but may actually be a number, string, boolean or nodeset.

Attached patch patch (obsolete) — Splinter Review
Updated patch with class/interface name changes. This patch works for the XForms 'choose' function to pass any type of parameter and return any kind of result. The updated patch for choose is in bug436866.
Attachment #323364 - Attachment is obsolete: true
Attachment #332640 - Flags: review?(peterv)
Attached patch v2Splinter Review
I think we should do this. Merle, this should allow you to implement a function that takes txIXPathObject as arguments and/or returns a txIXPathObject. Let me know if this isn't enough for what you want to do.
Attachment #332640 - Attachment is obsolete: true
Attachment #344949 - Flags: superreview?(jonas)
Attachment #344949 - Flags: review?(jonas)
Attachment #332640 - Flags: review?(peterv)
(In reply to comment #7)
> Created an attachment (id=344949) [details]
> v2
> I think we should do this. Merle, this should allow you to implement a function
> that takes txIXPathObject as arguments and/or returns a txIXPathObject. Let me
> know if this isn't enough for what you want to do.
The XPath 'choose' function works with this patch now. Thanks.
Attachment #344949 - Flags: superreview?(jonas)
Attachment #344949 - Flags: superreview+
Attachment #344949 - Flags: review?(jonas)
Attachment #344949 - Flags: review+
Comment on attachment 344949 [details] [diff] [review]
v2

remove the .get() and use a QI instead of casting. r/sr=me with that.
Status: UNCONFIRMED → RESOLVED
Closed: 11 years ago
OS: Windows XP → All
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b2
any chance of getting this on the 1.9 branch?
doubt it as we generally only take stability and security fixes
Keywords: checkin-needed
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.