98815 - implement DOM3 Core namespace methods

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[vidur (gone)]

Implement the Node interface methods:

 // Introduced in DOM Level 3:
  DOMString          lookupNamespacePrefix(in DOMString namespaceURI);
  // Introduced in DOM Level 3:
  DOMString          lookupNamespaceURI(in DOMString prefix);

Comment 1

[vidur (gone)]

Attachment: Proposed implementation using the existing implementation of the nsINamespace interface

Comment 2

[Johnny Stenback (:jst)]

- I don't see why we'd want to throw NS_DOM_NOT_SUPPORTED_ERR exceptions when
lookupNamespacePrefix() or lookupNamespaceURI() is called on nodes where you
can't resolve them, simpy returning an empty string and no exception should be
enough. The DOM spec doesn't say what to do yet, so we can only guess, but IMO
throwing an exception doesn't make sense.

- In nsGenericDOMDataNode::LookupNamespacePrefix(), and ::LookupNamespacePrefix,
there's no need for the "if mParent" check, do_QueryInterface() is null safe.

- In nsNode3Tearoff::LookupNamespacePrefix(), use nsINodeInfo to get to the
namespace manager, only if there's no nodeinfo use the document. This way it'll
work even if the element is not currently in a document. I.e. something like this:

+  nsCOMPtr<nsINameSpaceManager> manager;
+  nsCOMPtr<nsINodeInfo> ni;
+
+  mContent->GetNodeInfo(*getter_AddRefs(ni));
+
+  if (ni) {
+    nsCOMPtr<nsINodeInfoManager> nimgr;
+
+    ni->GetNodeInfoManager(*getter_AddRefs(nimgr));
+    NS_ENSURE_TRUE(nimgr, NS_ERROR_UNEXPECTED);
+
+    nimgr->GetNameSpaceManager(*getter_AddRefs(manager));
+  }
+
+  if (!manager) {
+    get manager from document
+  }

- This should be changed:

+  if (manager == nsnull) {
+    return NS_ERROR_FAILURE;
+  }

to:

+  if (!manager) {
+    return NS_ERROR_FAILURE;
+  }

to guarantee that it compiles on all platforms (manager is a nsCOMPtr<...>)

sr=jst with these fixes.

Comment 3

[vidur (gone)]

Attachment: patch v1.1 with suggested changes

Comment 4

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

Comment on attachment 48906 [details] [diff] [review]
patch v1.1 with suggested changes

r=heikki, but I'd prefer you return the error value you received from functions
instead of translating that to your own error code, unless there is a good
reason for this:

>+NS_IMETHODIMP
>+nsNode3Tearoff::LookupNamespacePrefix(const nsAReadableString& aNamespaceURI,
>+                                      nsAWritableString& aPrefix)

>+        nsresult rv = ns->FindNameSpacePrefix(namespaceId, 
>+                                              *getter_AddRefs(prefix));
>+        if (NS_FAILED(rv)) {
>+          return NS_ERROR_FAILURE;
>+        }

>+NS_IMETHODIMP
>+nsNode3Tearoff::LookupNamespaceURI(const nsAReadableString& aNamespacePrefix,
>+                                   nsAWritableString& aNamespaceURI)

>+        nsresult rv = ns->FindNameSpace(prefix, *getter_AddRefs(foundNS));
>+        if (NS_FAILED(rv)) {
>+          return NS_ERROR_FAILURE;
>+        }


Also, since you seem to be returning FAILURE for unexpected errors, maybe you should 
change the one UNEXPECTED to FAILURE.

Comment 5

[vidur (gone)]

Actually, the two cases that you cite are failures and not unexpected results.
Specifically, in both cases, the lookup failed. 

Currently, I return NS_ERROR_FAILURE in lieu of a DOM error code - the spec
doesn't yet specify what to return on failure - but anticipate changing the code
when one is specified. Again, it both cited cases, it doesn't make sense to
return the error code returned by the called function.

Comment 6

[Axel Hecht]

I personally like the UNEXPECTED. And the FAILURE looks reasonable, too.

Adding a depend for transformiix, we should use the code for parsing and 
evaluating XPath expressions.

Axel

Comment 7

[Axel Hecht]

I have been looking at the patch abit more closely.
Two things, first, this implementation only get the URI string, but the at least
for a descent XPath engine, we need the NameSpaceID.
Does it make sense to have the code for getting the nsINameSpace for the 
prefix exposed, factored?
This patch uses nsINameSpace::FindNameSpace, jst, I recall you issued a
"I wanna kill that". Is that still true?

Axel

Comment 8

[vidur (gone)]

Fixed a while ago. My original patch was modified by [e-mail] when he
removed the original namespace structures.

Comment 9

[Sivakiran Tummala]

verified fixed