The default bug view has changed. See this FAQ.

Implement XPointer framework and element scheme and extensible element scheme registration

RESOLVED FIXED in mozilla1.4alpha

Status

()

Core
XML
--
enhancement
RESOLVED FIXED
15 years ago
6 years ago

People

(Reporter: Heikki Toivonen (remove -bugzilla when emailing directly), Assigned: Heikki Toivonen (remove -bugzilla when emailing directly))

Tracking

Trunk
mozilla1.4alpha
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixinhand])

Attachments

(3 attachments, 8 obsolete attachments)

52.47 KB, patch
Heikki Toivonen (remove -bugzilla when emailing directly)
: review+
peterv
: superreview+
Details | Diff | Splinter Review
4.66 KB, text/xml
Details
1.36 KB, text/xml
Details
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/
Created attachment 107632 [details]
testcase
Created attachment 107633 [details] [diff] [review]
wip

* Move xpointer idl to content/xml
* Make XPointerResult usable and switch to that from node
* Implement xpath1 sceheme in XSLT code
* ...
Created attachment 109703 [details] [diff] [review]
wip 2
Attachment #107633 - Attachment is obsolete: true
Created attachment 110423 [details] [diff] [review]
wip 3 - beginnings of xpath1 scheme impl
Attachment #109703 - Attachment is obsolete: true
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?
Created attachment 110626 [details] [diff] [review]
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.
Attachment #110423 - Attachment is obsolete: true
Attachment #110626 - Flags: superreview?(peterv)
Attachment #110626 - Flags: review?(bugmail)
Status: NEW → ASSIGNED
Whiteboard: [fixinhand]
Target Milestone: --- → mozilla1.3beta
*** Bug 176519 has been marked as a duplicate of this bug. ***
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);
Target Milestone: mozilla1.3beta → mozilla1.4alpha
Blocks: 193275
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.

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.
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.
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.
Created attachment 116463 [details] [diff] [review]
Address Jonas' comments
Attachment #110626 - Attachment is obsolete: true
Comment on attachment 116463 [details] [diff] [review]
Address Jonas' comments

This addresses all of Jonas's comments. See my resolutions/questions above.
Attachment #116463 - Flags: superreview?(peterv)
Attachment #116463 - Flags: review?(bugmail)
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.
Attachment #110626 - Flags: superreview?(peterv)
Attachment #110626 - Flags: review?(bugmail)
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
Attachment #116463 - Flags: review?(bugmail) → review+
Created attachment 117108 [details] [diff] [review]
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.
Attachment #116463 - Attachment is obsolete: true
Attachment #117108 - Flags: superreview?(peterv)
Attachment #117108 - Flags: review+
Blocks: 191800
Attachment #116463 - Flags: superreview?(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?
Created attachment 117536 [details] [diff] [review]
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.
Attachment #117108 - Attachment is obsolete: true
Comment on attachment 117536 [details] [diff] [review]
Address peterv's comments

Happy now, you whitespace-counting git? ;)
Attachment #117536 - Flags: superreview?(peterv)
Attachment #117536 - Flags: review+
> * ++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.
> * ++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.
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.
Created attachment 117619 [details] [diff] [review]
Another proposal

* Post increments changed to pre-increments
* jst-simulacrum does not complain about whitespace
Attachment #117536 - Attachment is obsolete: true
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.
Attachment #117619 - Flags: superreview?(peterv)
Attachment #117619 - Flags: review+
> * 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).
nsRefPtr works, didn't even know we had such a thing! (/me goes to read what the
heck it does.)
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.
Attachment #117619 - Flags: superreview?(peterv) → superreview+
Attachment #117108 - Flags: superreview?(peterv)
Attachment #117536 - Flags: superreview?(peterv)
Checked in with nsRefPtr.
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
Dare I complain about spelling errors in file/interface names?  It should be
nsIModifiableXPointer, not nsIModifyableXPointer.
        ^                          ^
Ok, I'll fix that filename fu in bug 191800 which will move this stuff to
XMLExtras and do other minor cleanup as well.
Created attachment 119333 [details]
testcase
Attachment #107632 - Attachment is obsolete: true

Updated

14 years ago
Blocks: 201754

Comment 33

13 years ago
Created attachment 142152 [details]
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
(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

7 years ago
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.
> 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.
You need to log in before you can comment on or make changes to this bug.