Implement DOM Level 3 XPath WD

VERIFIED FIXED in mozilla1.0.1

Status

()

Core
DOM
P3
normal
VERIFIED FIXED
16 years ago
4 years ago

People

(Reporter: peterv, Assigned: peterv)

Tracking

Trunk
mozilla1.0.1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 7 obsolete attachments)

(Assignee)

Description

16 years ago
Subject says it all. Even though this is only a working draft, I think we should
start implementing it (IE has the functionality, though with a proprietary
interface). Now, incidently, I happen to have some code laying around in one of
my trees, so I'll attach a patch. I still have to review it myself, but if you
guys would take a look or even review, that'd be great :).
(Assignee)

Comment 1

16 years ago
Created attachment 52138 [details] [diff] [review]
Implementation v1
(Assignee)

Comment 2

16 years ago
Things that aren't implemented:

- namespace nodes, they're still unimplemented in Transformiix
- the NSResolver isn't used when parsing the expression, we need to rewrite
Transformiix to resolve the prefixes at parse time
- we always return results in document order (this is correct according to the
spec, but might be bad performance-wise)
- the exceptions are probably of the wrong type and have the wrong code, this
will require changes in the DOM and XPConnect

I don't plan to fix any of these as part of this bug.
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → mozilla0.9.6

Comment 3

16 years ago
A draft comment.
The interfaces should have a 3 somewhere, to indicate that they're not final.
And you should have the version of the WD in the idls, so one can see what is
supposed to be in there.
I see alot of very small files, could we pack those together somehow?
The idls as well as the .h and .cpp.

I will take a closer look later, Axel
(Assignee)

Comment 4

16 years ago
Axel, no 3 in the interfaces, I don't like it and I think that it would hork
DOMClassInfo. I'd *like* to keep the files separated.

Comment 5

16 years ago
No problem with 3's in DOMClassInfo, nsIDOM3Node already does it.
If this is the first version of the file I don't see the need for a '3' in the
name, the only reason I use that in nsIDOM3Node is that I'm extending an
existing interface.
(Assignee)

Comment 7

16 years ago
Moving out.
Target Milestone: mozilla0.9.6 → mozilla0.9.8

Comment 8

16 years ago
just a question, why does this patch remove the XSLT classes from classinfo?
i got aware of this bug because the W3C released a new working draft about
this... is XPath only available in transformiix that most of the code is in
transformiix? Doesn't the WD I mentionned apply to any DOM implementation?
(Assignee)

Comment 9

16 years ago
Fabian: Transformiix is the only XPath/XSLT processor for Mozilla and is an
optional extension, although it's built by default. Removing the XPath/XSLT
classes from nsDOMClassInfo breaks a build dependency of DOM on Transformiix.
The only side-effect of that should be that we can't do the prototype stuff on
the XPath and XSLT DOM objects. I don't think that that is a serious issue
though. I'm not sure I understand your last question.

Comment 10

16 years ago
evil thought:
<doc>
 <a xmlns:foo="bar">
  <aNode />
 </a>
 <b xmlns:foo="baz" />
<doc>

do 
ares = aXPathProc.createNSResolver(aNode);
b.appendChild(a.removeChild(aNode));

now, ares would map foo to "baz", but I guess it should be "bar", cause the spec
says:
createNSResolver 
  Adapts any DOM node to resolve namespaces so that an XPath expression can be
  easily evaluated relative to the context of the node where it appeared
  within the document.

See the "appeared"? Sounds like this implies the node position at the time the
resolver was created, not at the time a expression get's evaluated.

CC Ray for clarification

Comment 11

16 years ago
actually, pondering my words a bit, the XPathNSResolver should copy the 
namespace definitions from the node. It's just not sane to have the namespace
resolving of an expression depend on the current position (if it's in the at
all) of a once known node.
However, we might want to have a nsIDOMXPathNSResolverInternal, using atoms
and namespace IDs, so that we can construct an efficient namespace resolver
from a node.
From talking to Ray about this it seemd very clear that the NS resolver that's
created from a DOM node would be very much live. If you move the node you
created a NS resolver from, you're effectively modifying the NS resolver. I
don't think there's a need for something like nsIDOMXPathNSResolverInternal, if
there's a performance problem in this code, the NS resolver can be smart about
caching things that matter. 

Comment 13

16 years ago
That is right.  If the application wants a resolver not tied to the state of a
node, it is free to implement its own resolver.  But that was never the intent.
 The resolution of namespaces is often tied to the context of a node in the
hierarchy, and there is no reaason to try to make that static when it is not
naturally so.
(Assignee)

Comment 14

16 years ago
Moving this out a bit. I might pull it back in if I find the time to finish the
implementation.
Target Milestone: mozilla0.9.8 → mozilla1.0.1
(Assignee)

Comment 15

16 years ago
Attaching new patch, updated to
http://www.w3.org/TR/2002/WD-DOM-Level-3-XPath-20020208. Now with exceptions.
Anyone want to review so we can check this in someday?
(Assignee)

Comment 16

16 years ago
Created attachment 68873 [details] [diff] [review]
Implementation v2
Attachment #52138 - Attachment is obsolete: true

Comment 17

16 years ago
When I tried to look at this, my problem was there was no IDL.  Are the XPath
interfaces visible from JS?
(Assignee)

Comment 18

16 years ago
Ray, the idl files are definitely in the patch. Their path info is absent :/,
but they should end up in mozilla/dom/public/idl/xpath.

Comment 19

16 years ago
What I intended to say was that the XPath IDL was not there when I looked for it
prior to this patch (and it is not in my latest build that I pulled down).  If
it is in the patch, I will try to look at it ASAP.  Thanks.
in nsXPathEvaluator::CreateExpression you're leaking the Expr* if |new
nsXPathExpression(expression)| fails.


You shouldn't create an nsIDOMXPathNSResolver using the contextnode if the
resolver is null in nsXPathEvaluator::Evaluate. Just forward the null to
CreateExpression.


the list of allowed nodetypes is missing nsIDOMXPathNamespace::XPATH_NAMESPACE_NODE


+    // We need a result object and it must be our implementation.
+    nsXPathResult* xpathResult = NS_STATIC_CAST(nsXPathResult*, aInResult);
+    if (!xpathResult) {
+        // Either no aInResult or not one of ours.
i doubt thiss will work


+    nsCOMPtr<nsIDOMDocument> ownerDOMDocument;
+    aContextNode->GetOwnerDocument(getter_AddRefs(ownerDOMDocument));
+    nsCOMPtr<nsIDocument> ownerDocument = do_QueryInterface(ownerDOMDocument);
need to handle the case when aContextNode is a document-node


+    ProcessorState processorState;
+    ExprResult* exprResult = mExpression->evaluate(node, &processorState);
+    NS_ENSURE_TRUE(exprResult, NS_ERROR_OUT_OF_MEMORY);
throw INVALID_EXPRESSION_ERR instead, that's much more likely to be the problem


+    if (aType == nsIDOMXPathResult::ANY_TYPE) {
+        short exprResultType = exprResult->getResultType();
+        if (exprResultType == ExprResult::NODESET) {
+            resultType = nsIDOMXPathResult::UNORDERED_NODE_ITERATOR_TYPE;
since all nodesets are ordered nowadays make that ORDERED_NODE_ITERATOR_TYPE


you should maybe throw an error if mNode is null in
nsXPathNSResolver::LookupNamespaceURI. The spec isn't clear about what should
happen. Ray?
http://lists.w3.org/Archives/Public/www-dom/2002JanMar/0149.html

in nsXPathExpression::Evaluate:
+            resultType = exprResultType;
our ExprResult::ResultType enum doesn't have the same values as the DOM-XPath
one (and IMHO we don't want to rely on that).


set aInvalidIteratorState to false if the result is not an iterator in
nsXPathResult::GetInvalidIteratorState


Invalidate() in nsXPathResult::AttributeChanged


In nsXPathResult::IterateNext:
+    if (mElements && mCurrentPos < mElements->Count()) {
+        nsISupports *tmp = (nsISupports*)mElements->ElementAt(mCurrentPos++);
+
+        if (tmp)
+            return CallQueryInterface(tmp, aResult);
+    }
+    *aResult = nsnull;
+    return NS_OK;
You never increase mCurrentPos.


in nsXPathResult::SetExprResult:
you don't need to observe the document when keeping a single-node-value.

+                mElements->InsertElementAt((void*)mozNode, i);
use AppendElement

+            content = do_QueryInterface(mozNode);
Need to handle that the returned node being the document node.



in nsXPathResult::Reset:
+        NS_IF_RELEASE(mNode);
+        mNode = 0;
NS_IF_RELEASE will nullify for you

+    delete mElements;
only do this when you know it's pointing to the array


+        nsVoidArray* mElements;
should be an nsSupportsArray (that'll stop the current leaking). Once it is you
can use QueryElementAt and use the bigger-then-count safty in
nsXPathResult::IterateNext and nsXPathResult::SnapshotItem

Comment 21

16 years ago
The DOM WG has been vague in the past about what happens when a null is passed
in when the API says there cannot be a null.  I think it is best to check it and
throw at the earliest possible time, i.e. when the resolver is created.  Since
we don't actually specify how the null exception is ever thrown, I suspect you
could do a number of things and still be right.  The DOM WG has not been
motivated to clear up null exceptions, but has called them platform-specific. 
At most, they probably should be binding specific.  We cannot resolve the lack
of specificity in the bug.  I will at least ask what people think in the WG.  I
expect the answer is implementation-specific behavior on nulls passed in, which
any reasonable implementation would flush out at the earliest possible time.
(Assignee)

Comment 22

16 years ago
Created attachment 69171 [details] [diff] [review]
Implementation v3
(Assignee)

Updated

16 years ago
Attachment #68873 - Attachment is obsolete: true
nsXPathResult::mDocument should be an nsCOMPtr (sorry, missed that the first 
time). At least if it should be an owning reference (see further down). 
Otherwise an nsIWeakReference or something.

You still observe the document when the result-type is a single-node value. The 
spec doesn't actually say anything on this afaict, however since you don't 
check the invalid-state in nsXPathResult::GetSingleNodeValue there's no point 
in observing either. Actually, you don't need to observe for snapshots either 
(on this the spec is clear).


And then two things we talked on irc that i'm unsure about:

+        xpathResult = new nsXPathResult();
(xpathResult is an nsCOMPtr), is this safe?

does the doc hold strong refs to it's observers?


r=sicking if those are ok
(Assignee)

Comment 24

16 years ago
Sicking:

"The result is a node set as defined by XPath 1.0 and will be accessed as a
single node, which may be null if the node set is empty. Document modification
does not invalidate the node, but may mean that the result node no longer
corresponds to the current document."

So i'll drop observing the document for single node value.
(Assignee)

Comment 25

16 years ago
Created attachment 69374 [details] [diff] [review]
Implementation v4

Should address all sicking's comments. I'll let him mark it with r=.
Attachment #69171 - Attachment is obsolete: true
+    mElements->Count(&count);
+    if (mElements && aIndex >= 0 && aIndex < count) {

A little late to check mElements...
sorry, missed this the last round :(((


+            if (mResultType != UNORDERED_NODE_ITERATOR_TYPE &&
+                mResultType != ORDERED_NODE_ITERATOR_TYPE) {
However i'm not the only one that keep failing ;-)
(the condition is inversed)
Comment on attachment 69374 [details] [diff] [review]
Implementation v4

r=sicking with those two things changed
Attachment #69374 - Flags: review+
(Assignee)

Comment 28

16 years ago
Created attachment 69857 [details] [diff] [review]
Implementation v5 (sigh)
Attachment #69374 - Attachment is obsolete: true
+    mElements->Count(&count);
+    if (mElements && aIndex >= 0 && aIndex < count) {

Mind fixing _all_ these? ;-)
(Assignee)

Comment 30

16 years ago
Created attachment 70099 [details] [diff] [review]
Implementation v5bis
Attachment #69857 - Attachment is obsolete: true
Comment on attachment 70099 [details] [diff] [review]
Implementation v5bis

r=sicking!
Attachment #70099 - Flags: review+
Comment on attachment 70099 [details] [diff] [review]
Implementation v5bis

Shouldn't you change the lines starting with 'dnl' in Makefile.in to '#', or
what is the deal here?
(Assignee)

Comment 33

16 years ago
I used the license boilerplate for .in files from
http://www.mozilla.org/MPL/boilerplate-1.1/.
(Assignee)

Comment 34

16 years ago
Created attachment 72420 [details] [diff] [review]
Implementation v6

Corrected makefile license
Post-DOMCI landing
Return UNORDERED_... for ANY_TYPE and nodelist (that's what the spec says)
Attachment #70099 - Attachment is obsolete: true
(Assignee)

Comment 35

16 years ago
Oh, and NSResolver can now be a function.
Comment on attachment 72420 [details] [diff] [review]
Implementation v6

- Add "(original author)" to all new comment blocks.

- In nsIDOMXPathResult.idl:

+  readonly attribute nsIDOMNode       singleNodeValue;
+				       // raises(XPathException) on retrieval
+
+  readonly attribute boolean	      invalidIteratorState;
+  readonly attribute unsigned long   snapshotLength;
+					 // raises(XPathException) on retrieval

jst fix indentation of // raises...

- In nsXPathEvaluator::CreateExpression():

+    return CallQueryInterface(xpathExpression, aResult);

Do you really need a QI here? Isn't an implicit cast and a NS_ADDREF() enough?

- In nsXPathEvaluator::CreateNSResolver(), same as above...

- In nsXPathExpression::Evaluate():

+	     PRUint32 textLength;
+	     textNode->GetLength(&textLength);
+	     if (textLength <= 0)

Pointless < check, textLength is unsigned.

+	 // XXX Need to get logical XPath text node for CDATASection
+	 //	and Text nodes.

Do we have bugs on comments of this type?

+	     case ExprResult::TREE_FRAGMENT:
+		 NS_ASSERTION(0, "Can't return a tree fragment!");
+		 resultType = nsIDOMXPathResult::ORDERED_NODE_ITERATOR_TYPE;

Uh, you assert and yet set the type here?

... and at the end of the method, why QI? Why not just assign and addref?

- nsXPathNamespace, why do we need it? I don't see it being used anywhere. And
does the spec say to throw exceptions on all but one accessors?

- In nsXPathResult::SnapshotItem():

+    if (mElements && aIndex >= 0) {

Pointless >= 0 check, aIndex is unsigned so the check will always
be true.

- In the declaration of the class nsXPathResult:

+    PRUint16 mResultType;
+    PRBool mInvalidIteratorState;

Use PRPackedBool to save a few bytes.

Other than that, sr=jst (and transfering r= too)
Attachment #72420 - Flags: superreview+
Attachment #72420 - Flags: review+
yep, r=sicking sticks
(Assignee)

Comment 38

16 years ago
Created attachment 72793 [details] [diff] [review]
Implementation v6bis
Attachment #72420 - Attachment is obsolete: true
(Assignee)

Comment 39

16 years ago
Comment on attachment 72793 [details] [diff] [review]
Implementation v6bis

carrying forward r=sicking and sr=jst
Attachment #72793 - Flags: superreview+
Attachment #72793 - Flags: review+
(Assignee)

Comment 40

16 years ago
Created attachment 72801 [details] [diff] [review]
Add dom_xpath.xpt to packages files

This one still needs reviews.
Comment on attachment 72801 [details] [diff] [review]
Add dom_xpath.xpt to packages files

Don't forget embedding, didn't you listen to Adam in Brussels? ;-)
(Assignee)

Comment 42

16 years ago
embedding doesn't have Transformiix.
(Assignee)

Comment 43

16 years ago
Ok, numbers:

Without the patch:
Ts: 2358 (10 runs)
Txul: 677
Tp: 1023/1029/670/3258

With the patch:
Ts: 2367 (10 runs)
Txul: 645
Tp: 1027/1035/668/3369

I haven't been able to get reliable leak numbers, but none of the leaks that I
get look related to my code. At one point, I saw 70k of leaks *without* my patch
against 12k with my patch. It is highly unlikely that I've fixed those leaks
however. In general, I saw leaks vary between 9k and 12k, with the same classes
getting leaked with or without the patch.
Comment on attachment 72793 [details] [diff] [review]
Implementation v6bis

a=dbaron for trunk checkin
Attachment #72793 - Flags: approval+
(Assignee)

Comment 45

16 years ago
Checked in, thanks everyone.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 46

16 years ago
yes indeed, this eagle landed
Status: RESOLVED → VERIFIED
I'm pretty sure this caused bug 348466; not sure if that bug is a real bug or just now needs to be an evang bug against Netflix?
Man, sorry; commented in the wrong bug: I meant that bug 346362 caused bug 348466.  Sorry for the double spam.
You need to log in before you can comment on or make changes to this bug.