182323 - Implement XPointer framework and element scheme and extensible element scheme registration

Status: RESOLVED FIXED

(Core :: XML, enhancement)

Description

[Heikki Toivonen (remove -bugzilla when emailing directly)]

The XPointer spec has been split into 4 parts: framework, element, namespace and
xpointer. This bugs is about implementing the first two parts, maybe even the
third part, and providing an extensible mechanism so that new functionality can
be achieved just by installing new components.

Framework and element:

http://www.w3.org/TR/2002/PR-xptr-framework-20021113/
http://www.w3.org/TR/2002/PR-xptr-element-20021113/

xpath1, to be implemented to test extensible mechanism
http://www.simonstl.com/ietf/draft-stlaurent-xpath-frag-00.html

xmlns (not yet implemented)
http://www.w3.org/TR/2002/PR-xptr-xmlns-20021113/

Comment 1

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: testcase

Comment 2

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: wip

* Move xpointer idl to content/xml
* Make XPointerResult usable and switch to that from node
* Implement xpath1 sceheme in XSLT code
* ...

Comment 3

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: wip 2

Comment 4

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: wip 3 - beginnings of xpath1 scheme impl

Comment 5

[Heikki Toivonen (remove -bugzilla when emailing directly)]

The xpath1 stuff now gets registered and called, but I wonder if it is
registered properly...

The xpath impl does not seem to be correct, though. Calling Evaluate() I get
assertions, and GetSingleNodeValue() returns null.

Jonas, Peter?

Comment 6

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Proposed fix

Now this might be it. xpath1 scheme works, including namespaces. Multiple range
objects work. Selecting multiple ranges work. Following links work. Script API
works.

Comment 7

[Heikki Toivonen (remove -bugzilla when emailing directly)]

*** Bug 176519 has been marked as a duplicate of this bug. ***

Comment 8

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

Index: dom/public/idl/core/MANIFEST_IDL
This change doesn't seem needed.

+interface nsPIXPointerResult : nsIXPointerResult
hmm.. that name is a bit strange. I'd prefer something like
nsIXPointerResultPrivate or nsIXPointerResultModifyable


+class nsXPointerResult : public nsPIXPointerResult {
+public:
+  nsXPointerResult();
+  virtual ~nsXPointerResult();
+
+  NS_DECL_ISUPPORTS
+
+  NS_DECL_NSIXPOINTERRESULT
+  NS_DECL_NSPIXPOINTERRESULT
+
+private:
+  nsAutoVoidArray mArray;
+};

mArray should be an nsCOMArray<nsIDOMRange>. (don't forget to remove the
NS_ADDREF/NS_RELEASE when you do this)

+nsXPointerResult::AppendRange(nsIDOMRange* aReturn)
+{
+  if (mArray.AppendElement(aReturn)) {
+    NS_ADDREF(aReturn);
+    return NS_OK;
+  }
+  return NS_ERROR_FAILURE;
+}

add an NS_ENSURE_TRUE to protect against the argument being null. Return
NS_ERROR_OUT_OF_MEMORY if the append fails.


+NS_IMETHODIMP
+nsXPointerResult::Item(PRUint32 aIndex, nsIDOMRange** aReturn)
+{
+  NS_ENSURE_ARG_POINTER(aReturn);

asserting that aReturn is non-null is enough, the scriptengine will never call
this with a null-argument, and neither should any internal clients. Same in
::GetLength


+NS_IMETHODIMP
+nsXPointerResult::GetLength(PRUint32* aLength)
+{
+  NS_ENSURE_ARG_POINTER(aLength);
+
+  PRInt32 count = mArray.Count();
+  *aLength = count > 0 ? count : 0;

mArray.Count() will never return a negative value, so just assign straight into
*aLength.


+static nsresult NS_NewXPointerResult(nsIDOMRange *aRange, nsIXPointerResult
**aResult)
+{
+  if (!aRange)
+    return NS_OK;

hmm.. seems a bit unexpected to return null and NS_OK if a range isn't supplied.
Please create an empty result or return an error (I would prefer the former).
Same for NS_NewXPointerResult(nsIDOMNode *aNode, ...)


+static nsresult NS_NewXPointerResult(nsIDOMNode *aNode, nsIXPointerResult
**aResult)
+{
+  if (!aNode)
+    return NS_OK;
+
+  nsCOMPtr<nsIDOMRange> range(do_CreateInstance(kRangeCID));
+  if (!range)
+    return NS_ERROR_OUT_OF_MEMORY;
+
+  nsresult rv = range->SelectNode(aNode);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }

please use NS_ENSURE_SUCCESS(rv, rv);


+NS_INTERFACE_MAP_BEGIN(nsXPointerSchemeContext)
+  NS_INTERFACE_MAP_ENTRY(nsISupports)
+  NS_INTERFACE_MAP_ENTRY(nsIXPointerSchemeContext)
+NS_INTERFACE_MAP_END
+
+NS_IMPL_ADDREF(nsXPointerSchemeContext)
+NS_IMPL_RELEASE(nsXPointerSchemeContext)

just do NS_IMPL_ISUPPORTS1(nsXPointerSchemeContext, nsIXPointerSchemeContext);
instead. Same for nsXPath1SchemeNSResolver and nsXPath1SchemeProcessor


+  aScheme = Substring(aExpression, 0, lp);
+  aScheme.CompressWhitespace();
+  if (aScheme.FindCharInSet(" \t\r\n") > 0)
+    return NS_ERROR_FAILURE; // scheme name can't contain...

CompressWhitespace(PR_TRUE, PR_FALSE) is more correct since whitespace isn't
allowed between the sceme and the '('.


+  PRBool escapeOn = PR_FALSE;
+  PRInt32 balance = 1;
+  for (; i < len; i++) {
+    // Circumflex is the escape character that can precede ^, ( and ) only
+    if (aExpression[i] == '^') {

You should really use string-iterators. [] is slow. Not really a requirement
though since this code isn't perf-critical.


+  // Keep trying the schemes from left to right until one finds a subresource
+  while (!expression.IsEmpty()) {
+    rv = GetNextSchemeNameAndData(expression, scheme, data);
+    if (NS_FAILED(rv))
+      break;

couldn't you just do |NS_ENSURE_SUCCESS(rv, rv);| seems like you're not doing
anything at the end of the function that you need to execute on failure. Same at
a few other places in this function.


+      // We implement element scheme by using the FIXptr processor.
+      // Check there are no parenthesis (legal in FIXptr data).
+      if (data.FindChar('(') < 0) {

Shouldn't you abort parsing if you do find a parenthesis here?


+        rv = nsFIXptr::Evaluate(aDocument, data, getter_AddRefs(range));
+        if (NS_FAILED(rv))
+          break;
+        if (range) {
+          return NS_NewXPointerResult(range, aResult);
+        }

Is it intential that you continue processing if no range is returned? Seems like
you (or actually nsFIXptr::Evaluate) should return an error if this happens? Or
does a null-result mean that evaluation didn't produce a range? If so, you
should stop processing and return null.
Same both in element() and fixptr().


+    if (xmldoc) {
+      xmldoc->EvaluateXPointer(aAnchorName, getter_AddRefs(xpointerResult));
+      if (xpointerResult) {
+        xpointerResult->Item(0, getter_AddRefs(jumpToRange));
+        if (jumpToRange) {

Looking at the implementations of the xpointer-types they return null if no
range is found. If that is the design then ->Item(0, ...) not returning a value
should be a fatal error and not just a fall-through. (i.e. return an errorcode
instead of just continue looking for other pointer-types).


+          jumpToRange->GetStartContainer(getter_AddRefs(node));
+          if (node) {
+            node->QueryInterface(NS_GET_IID(nsIContent),getter_AddRefs(content));

use CallQueryInterface




+  NS_INIT_ISUPPORTS();

NS_INIT_ISUPPORTS is deprecated, no need to do anything these days.


+  nsXPath1SchemeNSResolver(); // no impl
no need to do this. Since you specify another ctor no default empty one will be
created.


+  // a few scheme + data pairs (and only some of those will be xmlns
+  // schemes), and typical XPath expressions only have a few prefixes
+  // as well, so we'll see if we can manage without...
+
+  if (!mContext)
+    return NS_OK;

Shouldn't this return an error?


+      if (sep > 0) {
+        if (aPrefix.Equals( Substring(data, 0, sep) )) {
+          aURI.Assign( Substring(data, sep + 1, data.Length() - sep - 1) );

Please combine the two ifs into one. And remove the extra space after '(' and
before ')'.


+  nsXPath1SchemeNSResolver nsresolver(aContext);
+  nsCOMPtr<nsIDOMXPathResult> result;
+  nsXPathEvaluator e;

since nsXPath1SchemeNSResolver and nsXPathEvaluator are refcounted you shouldn't
create them on the stack. This is especially dangerous since you don't bump the
refcount to 1 so if a client would addref and then release them they would get
deleted.


+  nsresult rv = e.Evaluate(aData, 
+                           aDocument, 
+                           &nsresolver, 
+                           nsIDOMXPathResult::ORDERED_NODE_ITERATOR_TYPE, 
+                           nsnull, 
+                           getter_AddRefs(result));
+  if (NS_FAILED(rv)) {
+    return rv;
+  }

all |if (NS_FAILED(rv))| in this function should be replaced with
NS_ENSURE_SUCCESS(rv, rv);

Comment 9

[Heikki Toivonen (remove -bugzilla when emailing directly)]

I am working on this... progress as of now:

No MANIFEST_IDL change.

Changed to nsIModifyableXPointerResult.

Changed to nsCOMArray.

Fixed AppendRange().

I prefer checking ARG_POINTER for returns, so I did not change that.

Fixed GetLength().

TODO.

I don't like to use NS_ENSURE macros everywhere 'cos its easier to debug.

Fixed some NS_IMPL_ISUPPORTS macro usages - I tend to like the longer variable,
but...

Made CompressWhitespace() change.

Didn't change to iterators yet, as this is not yet perf critical. I want to
"prove that the code works" before I go on optimizing it.

I prefer fewer returns from a function, which is why I tend to use breaks.

TODO.

If no range is found, it is not a fatal error, just that resource was not found.
In that case try next pointer part. This is per the spec.

I can't bail early, because I don't know if the epxression really was an
XPointer. If it returned a range I know it is, otherwise I need to try every
anchor resolving method available.

The function already uses QueryInterface so I'd rather not make this different.

Removed the no-ops.

I thought default constructor will always be created unless you stop it like
this. However, when I tested, it made no difference to have this or not, and it
seems like the compiler always creates the ctor anyway. Dunno what is going on,
but I left it as is.

TODO.

TODO.

Whey, remnants from fast testing... Fixed.

Again style issue, didn't change.

Comment 10

[Heikki Toivonen (remove -bugzilla when emailing directly)]

The TODO items:

TODO: NS_NewXPointerResult

TODO: Shouldn't you abort parsing

Return error: I believe we would hit this code if there were no xmlns pointer
parts before this currently processing xpath1 pointer part, and this current
pointer part contained an expression with a namespace. What should happen is
that the pointer part will fail to return a result but we should continue with
next pointer part. I fear that if I return error, the XPath engine will
propagate it back to my code and I will terminate too early. On the other hand,
if I just return NS_OK I realized might signify that the namespaceURI for the
prefix is "". What should I be doing?

Combine ifs etc. done.

Comment 11

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Shouldn't you abort parsing: No, because it is only this pointer part that is in
error, and I need to continue with the next pointer part.

Comment 12

[Heikki Toivonen (remove -bugzilla when emailing directly)]

NS_NewXPointerResult: This was actually ok because of the usage pattern, but I
made them and callers follow the typical norm.

Will attach an updated patch soon.

Comment 13

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Address Jonas' comments

Comment 14

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 116463 [details] [diff] [review]
Address Jonas' comments

This addresses all of Jonas's comments. See my resolutions/questions above.

Comment 15

[Heikki Toivonen (remove -bugzilla when emailing directly)]

After some discussion with Jonas, I have made the following changes to
nsXPath1Scheme.cpp:

- make LookupNamespaceURI return NS_ERROR_DOM_NAMESPACE_ERR if no mContext or no
namespace found (doesn't matter currently 'cos the XPath engine ignores the
return values)
- change the following 3 return values from Evaluate to NS_OK because an error
in pointer part must not terminate whole XPointer:
NS_ERROR_DOM_INVALID_EXPRESSION_ERR, NS_ERROR_DOM_NAMESPACE_ERR,
NS_ERROR_DOM_TYPE_ERR

Please review attached patch and consider these mental additions :) I'll attach
a new patch if there are more changes.

Comment 16

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

Comment on attachment 116463 [details] [diff] [review]
Address Jonas' comments

I can't find where in the spec it says that if evaluation of a supported part
_fails_ that the failure should be ignored and processing should contiue with
the next part?


the dom/public/idl/core/MANIFEST_IDL change is still there


> > +    if (xmldoc) {
> > +      xmldoc->EvaluateXPointer(aAnchorName, getter_AddRefs(xpointerResult));
> > +      if (xpointerResult) {
> > +        xpointerResult->Item(0, getter_AddRefs(jumpToRange));
> > +        if (jumpToRange) {
> >
> > Looking at the implementations of the xpointer-types they return null if no
> > range is found. If that is the design then ->Item(0, ...) not returning a
> > 
> > valueshould be a fatal error and not just a fall-through. (i.e. return an
> > errorcode instead of just continue looking for other pointer-types).

This still applies, IMHO it unneccesary to consider both no xpointerResult and
an empty xpointerResult as not returning a resource.

> > +  nsXPath1SchemeNSResolver(); // no impl
> > no need to do this. Since you specify another ctor no default empty one will
> > be created.
> 
> I thought default constructor will always be created unless you stop it like
> this. However, when I tested, it made no difference to have this or not, and it
> seems like the compiler always creates the ctor anyway. Dunno what is going on,
> but I left it as is.

No, a default empty ctor is only created if no ctor is specified.

> +  nsXPath1SchemeNSResolver *nsresolver = new nsXPath1SchemeNSResolver(aCont...
> +  if (!nsresolver)
> +    return NS_ERROR_OUT_OF_MEMORY;
> +  NS_ADDREF(nsresolver);
> +  nsXPathEvaluator *e = new nsXPathEvaluator();

use nsCOMPtr<>s for nsresolver and e, that way you won't have to explicitly
NS_ADDREF/NS_RELEASE


> - make LookupNamespaceURI return NS_ERROR_DOM_NAMESPACE_ERR if no mContext or no
> namespace found (doesn't matter currently 'cos the XPath engine ignores the
> return values)

Checked this in the spec and the XPathNSResolver should not throw an
DOM_NAMESPACE_ERR if the namespace isn't found. It's the job of the XPath
engine to throw that error. Sorry :(
However if there is no context you should probably throw NS_ERROR_FAILURE or
NS_ERROR_UNEXPECTED or some such.

r=sicking with this fixed

Comment 17

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Latest

Hmm, I think it is undefined what happens if a pointer part processor fails. So
I'll leave it as is, because I prefer it this way.

This time there REALLY is no change in MANIFEST_IDL :)

I made GoToAnchor() return early if no item 0 in xpointer result.

Removed the empty ctor.

Changed to nsCOMPtr (I thought I would not be able to use them because I
thought I was using non-IDL methods, but I wasn't).

Yeah, according to DOM XPath that should not throw, so I made it to return
NS_OK always.

Comment 18

[Peter Van der Beken [:peterv]]

Comment on attachment 117108 [details] [diff] [review]
Latest

>Index: content/base/src/nsDocument.cpp
>===================================================================

>+NS_IMETHODIMP
>+nsDocument::EvaluateXPointer(const nsAString& aExpression, nsIXPointerResult **aResult)

Long line.

>Index: content/xml/document/public/nsIModifyableXPointer.idl
>===================================================================
>
>+ * Version: NPL 1.1/GPL 2.0/LGPL 2.1

Use MPL/GPL/LGPL for all new files. This applies to other files as well.

>+ * The Initial Developer of the Original Code is 

Nit: remove the trailing space. This applies to other files as well.

>+ * Netscape Communications Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 2002

Nit: the copyright should be for 2003. This applies to other files as well.

>Index: content/xml/document/public/nsIXPointer.idl
>===================================================================

>+  void getSchemeData(in unsigned long index, out DOMString scheme, out DOMString data);

Long line.

>+ * nsIXPointerSchemeProcessor implementations must be registered with the below
>+ * progid, to which the scheme name the processor implements is appended to.

Nit: appended with the scheme name that the processor implements?

>+  nsIXPointerResult evaluate(in nsIDOMDocument aDocument, 

Remove the trailing space.

>Index: content/xml/document/src/nsXMLDocument.cpp
>===================================================================

>+nsXMLDocument::EvaluateXPointer(const nsAString& aExpression, nsIXPointerResult **aResult)

Long line.

>Index: content/xml/document/src/nsXPointer.cpp
>===================================================================

>+/**
>+ * Implementation for the XPointer family of specifications, practically the
>+ * XPointer Processor. The processor can call optional modules that implement
>+ * some XPointer schemes that were not implemented in this file. Please note
>+ * that implementation of the xmlns scheme is left for the optional scheme

Nit: left to

>+ * Additionally this module implements 'fixptr' scheme for the FIXptr
>+ * W3C NOTE.

Have an URL for that?

>+NS_IMETHODIMP
>+nsXPointerResult::AppendRange(nsIDOMRange* aRange)

>+  if (!mArray.AppendObject(aRange))
>+    return NS_ERROR_OUT_OF_MEMORY;

Nit: braces around one-line if's. Also applies to other places in the patch.

>+static nsresult NS_NewXPointerResult(nsIDOMRange *aRange, nsIXPointerResult **aResult)

Long line.

>+{
>+  NS_ENSURE_ARG(aRange);
>+  NS_ENSURE_ARG_POINTER(aResult);
>+
>+  nsXPointerResult *result = new nsXPointerResult();

Could you make this an nsCOMPtr?

>+static nsresult NS_NewXPointerResult(nsIDOMNode *aNode, nsIXPointerResult **aResult)

Long line.

>+class nsXPointerSchemeContext : public nsIXPointerSchemeContext {

Move the brace to the next line.

>+private:  

Remove the trailing spaces.

>+nsresult
>+nsXPointerSchemeContext::Append(const nsAString &aScheme, const nsAString &aData)

Long line.

>+{
>+  if (!mSchemes.AppendString(aScheme))
>+    return NS_ERROR_OUT_OF_MEMORY;
>+
>+  if (!mDatas.AppendString(aData)) {
>+    mSchemes.RemoveStringAt(mSchemes.Count() - 1); // Keep mDatas and mSchemes in sync

You could have just one array for schemes and data. Up to you.

>+nsXPointerSchemeContext::GetSchemeData(PRUint32 aIndex, nsAString &aScheme, nsAString &aData)

Long line.

>+static nsresult GetNextSchemeNameAndData(nsString& aExpression, 
>+                                         nsString &aScheme, 
>+                                         nsString& aData)

Remove trailing space.

Iterators would be nice here :-).

>+  PRBool escapeOn = PR_FALSE;
>+  PRInt32 balance = 1;
>+  for (; i < len; i++) {

++i

>+nsresult 
>+nsXPointer::Evaluate(nsIDOMDocument *aDocument,
>+                     const nsAString& aExpression,
>+                     nsIXPointerResult **aResult)

>+    if (element) {
>+      rv = NS_NewXPointerResult(element, aResult);
>+    }
>+    return rv;

Just do:

    if (element) {
      return NS_NewXPointerResult(element, aResult);
    }
    return NS_OK;

>+  nsXPointerSchemeContext *contextSchemeDataArray = new nsXPointerSchemeContext();

Long line.

>+  if (!contextSchemeDataArray)
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  nsCOMPtr<nsXPointerSchemeContext> autoRefCnt(contextSchemeDataArray);

Why have a regular pointer and a  nsCOMPtr?

  nsCOMPtr<nsXPointerSchemeContext> contextSchemeDataArray =
     new nsXPointerSchemeContext();
  if (!contextSchemeDataArray) {
    return NS_ERROR_OUT_OF_MEMORY;
  }

>+      nsCOMPtr<nsIDOMRange> range;
>+      rv = nsFIXptr::Evaluate(aDocument, data, getter_AddRefs(range));
>+      if (NS_FAILED(rv))
>+        break;
>+      if (range) {
>+        return NS_NewXPointerResult(range, aResult);
>+      }
>+    } else {  

Remove the trailing whitespace.

>+    if (NS_FAILED(rv))
>+      break;
>+
>+  }
>+
>+  return rv;

There's a lot of breaks that should be returns imho. You do return early on
success.

>Index: content/xml/document/src/nsXPointer.h
>===================================================================

>+  static nsresult Evaluate(nsIDOMDocument *aDocument,
>+                           const nsAString& aExpression,
>+                           nsIXPointerResult **aResult);  

Remove trailing whitespace.

>Index: layout/html/base/src/nsPresShell.cpp
>===================================================================

>+        nsCOMPtr<nsIDOMNode> node;
>+        jumpToRange->GetStartContainer(getter_AddRefs(node));
>+        if (node) {
>+          node->QueryInterface(NS_GET_IID(nsIContent),getter_AddRefs(content));

Use do_QueryInterface.

>+          nsCOMPtr<nsISelection> sel;
>+          if (NS_SUCCEEDED(GetSelection(nsISelectionController::SELECTION_NORMAL, getter_AddRefs(sel))) 
>+              && sel) {

Long line. Also move the && to the end of the previous line.

>Index: extensions/transformiix/source/xpath/nsXPath1Scheme.cpp
>===================================================================

>+  nsXPath1SchemeNSResolver(nsIXPointerSchemeContext *aContext) { 

Remove trailing whitespace

>+    mContext = aContext;
>+  }
>+  virtual ~nsXPath1SchemeNSResolver() {}

Make this:

  nsXPath1SchemeNSResolver(nsIXPointerSchemeContext *aContext) :
mContext(aContext)
  {
  }
  virtual ~nsXPath1SchemeNSResolver()
  {
  }

>+//DOMString          lookupNamespaceURI(in DOMString prefix);
>+NS_IMETHODIMP
>+nsXPath1SchemeNSResolver::LookupNamespaceURI(const nsAString &aPrefix, nsAString &aURI)

Long line.

>+    if (scheme.Equals(xmlns)) {
>+      PRInt32 sep = data.FindChar('=');
>+      if (sep > 0 && aPrefix.Equals(Substring(data, 0, sep))) {
>+        aURI.Assign( Substring(data, sep + 1, data.Length() - sep - 1) );

Remove the space after the opening parenthesis and before the closing one.

>+nsXPath1SchemeProcessor::~nsXPath1SchemeProcessor()
>+{
>+
>+}

Remove the empty line.

>+
>+nsXPath1SchemeProcessor::Evaluate(nsIDOMDocument *aDocument, 

Remove the trailing whitespace.

>+                         nsIXPointerSchemeContext *aContext,
>+                         const nsAString &aData,
>+                         nsIXPointerResult **aResult)
>+{
>+  NS_ENSURE_ARG_POINTER(aDocument);
>+  NS_ENSURE_ARG_POINTER(aContext);
>+  NS_ENSURE_ARG_POINTER(aResult);
>+  *aResult = nsnull;  
>+ 
>+  // Resolve expression
>+  nsCOMPtr<nsIDOMXPathNSResolver> nsresolver(do_QueryInterface(new nsXPath1SchemeNSResolver(aContext)));

Do you need to QI?

>+  nsCOMPtr<nsIDOMXPathEvaluator> e(do_QueryInterface(NS_STATIC_CAST(nsIDOMXPathEvaluator*, new nsXPathEvaluator())));

Make this nsCOMPtr<nsXPathEvaluator>. Casting and QI'ing is too much here.

>+  if (NS_FAILED(rv)) {
>+    if ((rv == NS_ERROR_DOM_INVALID_EXPRESSION_ERR) ||
>+        (rv == NS_ERROR_DOM_NAMESPACE_ERR) ||
>+        (rv == NS_ERROR_DOM_TYPE_ERR)) {

Drop all the parenthesis.

>+  nsCOMPtr<nsPIXPointerResult> privatePointerResult(do_QueryInterface(xpointerResult));

This should be nsIModifyableXPointerResult?

>+  PRUint32 count;
>+  xpointerResult->GetLength(&count);
>+  if (NS_SUCCEEDED(rv) && (count > 0)) {

Drop the inner set of parenthesis.

I wonder if it wouldn't be better to make a nsXPathXPointerResult that holds on
to a nsXPathResult and created the ranges on demand. Maybe an idea for later?

Comment 19

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Address peterv's comments

This patch fixes all of peterv's issues except as follows:

* Not sure what you meant by one array.
* Iterators can be introduced later if perf becomes an issue.
* ++i makes no difference when i is int.
* NS_NewXPointerResult and returns, my way we have one less return.
* With breaks I can set a single break at end to see if function failed.
* nsCOMPtr<nsXPathEvaluator> won't work without cast, ambiguous.
* I like grouping parenthesis.
* Result building on demand -> future perf work if needed.

Comment 20

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 117536 [details] [diff] [review]
Address peterv's comments

Happy now, you whitespace-counting git? ;)

Comment 21

[Peter Van der Beken [:peterv]]

> * ++i makes no difference when i is int.

Please read bug 78032, comment 1.

> Happy now, you whitespace-counting git? ;)

No, it's actually gotten worse with the new patch. I'd rather have you fix these
(http://www.johnkeiser.com/cgi-bin/jst-review-cgi.pl?patch_url=http%3A%2F%2Fbugzilla.mozilla.org%2Fattachment.cgi%3Fid%3D117536&patch_text=&reason_type=W)
before checkin than having to review another huge content/ patch by jst where
most of the fixes are whitespace-only changes someone else forgot to do before
checkin.

Comment 22

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

> * ++i makes no difference when i is int.

I think that some compilers actually produce more code for i++ then for ++i,
even when i is an integer.

Comment 23

[Heikki Toivonen (remove -bugzilla when emailing directly)]

I am well aware that pre-increment works faster for complex objects. I have
never seen proof that this is the case for ints or other simple types.

Comment 24

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Another proposal

* Post increments changed to pre-increments
* jst-simulacrum does not complain about whitespace

Comment 25

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 117619 [details] [diff] [review]
Another proposal

You could save everyone's time by saying "fix these and r/sr" when only trivial
things remain, but that is of course up to you.

Comment 26

[Peter Van der Beken [:peterv]]

> * nsCOMPtr<nsXPathEvaluator> won't work without cast, ambiguous.

Ah, right. Can you try with an nsRefPtr? (No need to attach new patch yet, I'll
have my final comments tomorrow).

Comment 27

[Heikki Toivonen (remove -bugzilla when emailing directly)]

nsRefPtr works, didn't even know we had such a thing! (/me goes to read what the
heck it does.)

Comment 28

[Peter Van der Beken [:peterv]]

Comment on attachment 117619 [details] [diff] [review]
Another proposal

sr=peterv with the nsRefPtr. With one array I meant that since you always seem
to have a schem/data pair, you could store the scheme at index 2 * i and the
data at index (2 * i) + 1 in the same array.

Comment 29

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Checked in with nsRefPtr.

Comment 30

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

Dare I complain about spelling errors in file/interface names?  It should be
nsIModifiableXPointer, not nsIModifyableXPointer.
        ^                          ^

Comment 31

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Ok, I'll fix that filename fu in bug 191800 which will move this stuff to
XMLExtras and do other minor cleanup as well.

Comment 32

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: testcase

Comment 33

[Xanthor Sccas]

Attachment: testcase for Sibling nodes

It seems that there is a regression :
the element scheme doesn't work with sibiling elements :
If we have a document with : <doc><e id="e1">1</e><e id="e2">2</e></doc> and we
try to access the e2 element with #element(e2), it's the e1 element which will
be pointed...

Tested with : Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040207
Firefox/0.8
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040122 Epiphany/1.0.7
and Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7a) Gecko/20040218

Comment 34

[Peter Van der Beken [:peterv]]

(In reply to comment #33)
> It seems that there is a regression :
> the element scheme doesn't work with sibiling elements :
> If we have a document with : <doc><e id="e1">1</e><e id="e2">2</e></doc> and we
> try to access the e2 element with #element(e2), it's the e1 element which will
> be pointed...

I filed bug 281654 on that.

Comment 35

[Matthijs Wensveen]

Could it be that this functionality is removed from later versions of mozilla / firefox / gecko?
I'm currently running firefox 3.6.2 and all test cases except https://bug182323.bugzilla.mozilla.org/attachment.cgi?id=119333#id1 fail. The JS button fails for every testcase.

This would be very useful to have. But there might be valid reasons why one would not want this, although I can't think of one.

Comment 36

[Boris Zbarsky [:bzbarsky]]

> Could it be that this functionality is removed from later versions 

xpointer was removed (possibly accidentally) back in 2007 or so when importing into mercurial.