76070 - we need namespace support from the DOM

Status: VERIFIED FIXED

(Core :: XSLT, defect, P3)

Description

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

We are in bad need of namespace support from the DOM. Will attach a patch that 
does the following:

1. Implement Node::localName, Node::namespaceURI and
   Node::lookupNamespaceURI(prefix) (from DOM3 WD) in both DOMs

2. Implement Attr::ownerElement in both DOMs since I needed it for
   lookupNamespaceURI and I had no access to a DOMHelper to get owning element
   from.

3. Remove some old getBaseURI code that was left behind

I have not yet moved the current code over to use the new namespace code since 
I'm planning to do some restructureing of that code anyway...

Comment 1

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

Attachment: here goes...

Comment 2

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

trying to get this in for 0.9

Comment 3

[Axel Hecht]

shouldn't the moz impl for lookupNamespaceURI use the mozilla 
nsINameSpace::FindNameSpace ?
Was there more? Ugh, my brain just died :-(

Axel

Comment 4

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

Been looking at how I would do this but I cant find a generic way of getting 
such an interface. The different DOM implementations seems to inherit 
compleatly different interfaces and classes.

Also, I doubt that the nsINameSpace implementators are updated/generated 
correctly for dynamically generated/edited stylesheets.

I'll try to make an implementation that assumes that the supplied element is an 
nsXMLElement

Comment 5

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

grmbl... I looked for the wrong interface...

As far as I can see only XUL uses nsINameSpace so I don't think we can use it :(

Comment 6

[Axel Hecht]

> how to implement lookupNamespaceURI(prefix) for mozilla module

IIRC, we need to QI to a nsIXMLContent, then
GetContainingNameSpace(nsINameSpace*& aNameSpace), and use 
FindNameSpace(nsIAtom* aPrefix, nsINameSpace*& aNameSpace) to resolve the 
prefix.
Attributes need to use the ownerElement?

CCing jst for a sane answer. Which Nodes implement nsIXMLContent, any gotchas
for attributes?

Axel

Comment 7

[Johnny Stenback (:jst)]

Don't use nsIXMLContent::GetContainingNameSpace(), it's going away. Use some
other mechanism to do the lookup.

Comment 8

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

As far as I can see only XUL elements implement nsXMLContent...

jst: is there some other interface I can use to get map prefix->namespace? Or 
is the only way to walk up the tree looking for xmlns:prefix attributes?

Comment 9

[Axel Hecht]

Just chatted with jst, 
<jst> Pike: we use that method in *one* place
... and that's the reason it's going to die. And the reason for no replacement.

So we have to crawl up the parents and check the attributes. But we should
use atoms for doing that, to make that crawl more efficient.
nsIContent::GetAttribute looks like a good signature to me. I don't know if
we need the prefixed version or not, though.

XXX: this is for the source doc only. Which is ok for XPath parsing, evaluation.
We can't use this in the result doc. Off to file another bug.

Axel

Comment 10

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

Attachment: patch ver 2

Comment 11

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

Version two uses the nsIContent::GetAttribute function to get the attributes 
while crawling up the tree.

I also put back the getBaseURI code that I removed in the first patch since 
Node::GetbaseURI() will be removed from mozilla shortly (see bug 76641). So now 
I don't touch the baseURI code at all.

Comment 12

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

Attachment: small beautification of nsIAtom* handling. Now using a nsCOMPtr

Comment 13

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

the only thing different in this patch should be that I now use an 
nsCOMPtr<nsIAtom> to hold the prefix atom in Node::lookupNamspaceURI. I've also 
synced to latest tip

Comment 14

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

humhum

Comment 15

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

Attachment: diff v4

Comment 16

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

Attachment: txAtom.cpp

Comment 17

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

Attachment: txAtom.h

Comment 18

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

Attachment: txNamespaceManager.cpp

Comment 19

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

Attachment: txNamespaceManager.h

Comment 20

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

Diff ver 4 and the new files implements the followind additions to DOM nodes 
(both module and standalone)

txAtom* Node::getLocalName()
Int32   Node::getNamespaceID()
Node*   Node::getXPathParent()
Int32   Node::lookupNamespaceID(txAtom* prefix)

oh, and remove the patch to XSLTProcessor.cpp (last in the diff) that sliped 
through, sorry about that...

Comment 21

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

Attachment: diff ver 5

Comment 22

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

Attachment: txAtom.cpp ver 2

Comment 23

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

Attachment: txAtom.h ver 2

Comment 24

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

Attachment: txNamespaceManager.cpp ver 2

Comment 25

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

Attachment: txNamespaceManager.h ver 2

Comment 26

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

pushing

Comment 27

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

pushing

Comment 28

[Axel Hecht]

Great beating of sickings code.
made this work without further object files
moved txNamespaceManager.h to xml/dom/standalone (it's only used there)
added getNamespaceURI() method
made statics static pointers, converted the methods to static methods in
txAtomService and txNamespaceManager
a chunk of comment cleanup in the dom code
shutdown a leak in sickings code (dont_Addref was missing)
hooked on xml: to http://www.w3.org/XML/1998/namespace

patch is coming up

Comment 29

[Axel Hecht]

Attachment: diff -u -N, contains txAtom.h and txNamespaceManager.h, plus the diff, lot's of changes

Comment 30

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

A few things:

* We need to "hardwire" the xmlns namespace too
* The hardwireing of xml/xmlns namespaces needs to be done for module too (I
  think)
* The hardwireing of xml returns 0 but should return 1
* I don't like that txAtomService and txNamespaceManager is static for two
  reasons: First of all you don't release the atoms-map and the namespace-list.
  Second, IMHO the code will be nicer if we have global pointers to an
  atomservice and a namespacemanager. (We'd still need the txInitDOM() function)

Comment 31

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

Over to Pike since he's the one doing the work here now...

Comment 32

[Axel Hecht]

got new code, but that needs a few tweaks concerning bookkeeping of atoms.
pushing in favor of bug 96647

Comment 33

[Axel Hecht]

I need 
#define kNameSpaceID_Unknown -1
#define kNameSpaceID_None     0
#define kNameSpaceID_XMLNS    1 // not really a namespace, but it needs to play 
the game
#define kNameSpaceID_XML      2

and 
#define kNameSpaceID_XSLT     6
(3 for standalone)

Use the constants, instead of hardcoding the numbers.

Axel

Comment 34

[Axel Hecht]

note to self, see
http://lxr.mozilla.org/seamonkey/source/layout/build/nsLayoutModule.cpp#319
for use of NS_IMPL_NSGETMODULE_WITH_CTOR_DTOR instead of
NS_IMPL_NSGETMODULE, defined in
http://lxr.mozilla.org/seamonkey/source/xpcom/components/nsIGenericFactory.h#160

Comment 35

[Axel Hecht]

Peter, Jonas,
in standalone we don't have refcounting, so in general I wouldn't need to
TX_RELEASE_IF_ATOM. But this looks like crappy style. So in general I tried
to add them (even if they don't do anything).
Should I RELEASE in the destructors of Element, Attr and ProcessingInstruction?
It's a bit tricky, as those are done in a getter, so I had to really addref
the copy that I return.

Votes?

Comment 36

[Axel Hecht]

ok, there is a new patch coming up.
New stuff:
- no more txNamespaceManager.h, it's moved into standalone/dom.h
- redone init stuff completely
- created constructor/destructor for module
- fixed ::advance in the list iterator
- moved the late getting of atoms in standalone to constructors, in the end we
should have mostly atomized content in standalone dom, so that's the right
thing todo right now, IMHO. Should mLocalName move to NodeDefinition? The code
in Element, Attr and ProcessingInstruction is completely the same. This somehow
depends on how we do atoms for the node name in the end. They prolly should be
different, so I'd vote for moving them in.
- don't store localName for module, should mNamespaceID die too?
- new license plate for txAtom.h, Jonas, do you agree on that one?

I guess that pretty much wraps it up.

Axel

Comment 37

[Axel Hecht]

Attachment: Current version of patch, feedback required

Comment 38

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

For module:
Why have the namespace looked up in the ctor, but the localname looked up in 
the getter? Not saying that it's wrong, just curious why.

In Node::lookupNamespaceID you need to make sure that elem->GetAttr actually 
returns a value since rv will be a successvalue even when no attribute was 
found. I think replacing

+        rv = elem->GetAttr(kNameSpaceID_XMLNS, prefix, uri.getNSString());
+        if (NS_SUCCEEDED(rv)) {

with

+        rv = elem->GetAttr(kNameSpaceID_XMLNS, prefix, uri.getNSString());
+        NS_ENSURE_SUCCESS(rv, -1);
+        if (rv != NS_CONTENT_ATTR_NOT_THERE) {

would work.



For standalone:
Looking up the namespace in the ctor unfortuantly doesn't work, since the node 
isn't inserted in the tree at that time and therefor can't walk up the tree to 
find xmlns-attributes. The RightThing to do is to make our xml-parser ns-aware, 
but until then i think you should just init the namespace at the first call to 
getNamespaceID.

Couldn't you make NodeDefinition::getNamespaceURI into a generic implementation 
that called getNamespaceID() (since txNamespaceManager::getNamespaceURI(0) 
should return "")

Please make txNamespaceManager::getNamespaceURI (and perhaps 
Node::getNamespaceURI) return a |const String&| to save some stringcopying, and 
let it return NULL_STRING if it can't find the looked up namespace

For other:
How about renaming txXMLAtoms to something else (txAtomTable) so that we can 
stick other atoms in there later. It'd be great if we could replace our static 
strings with atoms instead...

Comment 39

[Axel Hecht]

nsdoom again. Doing salt'n'peppa

Comment 40

[Axel Hecht]

Attachment: stylesheet for namespace-uri() test

Comment 41

[Axel Hecht]

Attachment: xml file for namespace-uri() test

Comment 42

[Axel Hecht]

Attachment: new patch, with fix for xpath namespace-uri(), we do need a testcase

Comment 43

[Axel Hecht]

new patch, this one incorporates a fix for namespace-uri(), so we can at least
test parts of the functionality used here.
I added a new method to Element, MBool getAttr(), taking local name atom and
ns ID, this should be used all over in our code.
A great deal of changes.
jst, could you look at Node::lookupNamespaceID for module? The question is, can
we factor the code in nsIDOM3Node with this one? Is this the right code?
(How brittle is this code concerning the xhtml attribute namespace bah?)

Axel

Comment 44

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

comments on attachment 52217 [details] [diff] [review]:

+    String ns;
+    aAttr->GetNamespaceURI(ns.getNSString());
+    aOwner->nsNSManager->GetNameSpaceID(ns.getConstNSString(),
+                                        namespaceID);

feel free to make ns an nsAutoString


+    NSI_FROM_TX(Element)
+    nsCOMPtr<nsIContent> cont(do_QueryInterface(nsElement));

please QI direct to nsIContent instead of through nsIDOMElement 
(Element::getAttr)


You could use the cached nsNSManager in lookupNamespaceID for module if you 
want, i don't really care either way...


in Attr::Attr when |idx == NOT_FOUND|

+    else if (mLocalName == txXMLAtoms::XMLPrefix)
+      mNamespaceID = kNameSpaceID_XML;

is wrong. AFAICT the only mention on unprefixed attributes i can find is that 
they belong to the null namespace (only exception is xmlns attributes). NS-spec 
says "The *prefix* xml is by definition bound to the namespace name 
http://www.w3.org/XML/1998/namespace", no mention of special handling of 
attributes *named* xml.


The localName atoms arn't "released" for standalone.


I'm not sure i like adding the entire txNamespaceManager to dom.h, it makes a 
too big .h even bigger IMHO. Please break it out into txNamespaceManager.h


How about doing the AttrMap::clear stuff in the AttrMap destructor?


in NodeDefinition.cpp

-} // getBaseURI
+} //-- getBaseURI

was this really intentional?

Comment 45

[Axel Hecht]

fixed comments by Jonas, up to the txNamespaceManager.h thingie, we agreed on
moving that to the bottom of dom.h.
Attaching new patch, this time you get the full monty, that is the atom init
foo in build/ too. (*yac*, I'm shupid)

Axel

Comment 46

[Axel Hecht]

Attachment: patch (monty, why doesn't this python get smaller with age?)

Comment 47

[Axel Hecht]

Comment on attachment 53083 [details] [diff] [review]
patch (monty, why doesn't this python get smaller with age?)

Jonas found a compile problem in 
Element::getAttr, we need
AttrMap::ListItem* item = mAttributes.firstItem;

And then of course I had a hang in
NodeDefinition::lookupNamespaceID
'cause getAttr is calling getNamespaceID.
if I have a attribute foo:some, this got 
infinite.
I moved that code to getAttributeNode.

Axel

Comment 48

[Axel Hecht]

Attachment: another patch, please compile (and test) on all you have. Don't let C++ trick me again.

Comment 49

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

Comment on attachment 53606 [details] [diff] [review]
another patch, please compile (and test) on all you have. Don't let C++ trick me again.

r=sicking

/me cheers

Comment 50

[Peter Van der Beken [:peterv]]

Comment on attachment 53606 [details] [diff] [review]
another patch, please compile (and test) on all you have. Don't let C++ trick me again.

Great patch!

> Attr::Attr(nsIDOMAttr* aAttr, Document* aOwner) : Node(aAttr, aOwner)
> {
>+    nsAutoString ns;
>+    aAttr->GetNamespaceURI(ns);
>+    aOwner->nsNSManager->GetNameSpaceID(ns, namespaceID);
> }

Need null-checks on aAttr and aOwner?

>+    nsCOMPtr<nsIDocument> doc(do_QueryInterface(document));

I think you need if (doc) here.

>+    doc->GetNameSpaceManager(*getter_AddRefs(nsNSManager));


>+    nsCOMPtr<nsIDocument> doc(do_QueryInterface(aDocument));

Same here.

>+    doc->GetNameSpaceManager(*getter_AddRefs(nsNSManager));


>+    cont->GetNameSpaceID(namespaceID);

Null check on cont.

>+    rv = cont->GetAttr(aNSID, aLocalName, aValue.getNSString());

Same.

>+    if (rv != NS_CONTENT_ATTR_NOT_THERE)
>+        return MB_TRUE;
>+    else
>+        return MB_FALSE;

Don't else after return.

>+    NSI_FROM_TX(Element)
>+    nsCOMPtr<nsIContent> cont(do_QueryInterface(nsElement));
>+    NS_ASSERTION(cont, "Element doesn't implement nsIContent");
>+    cont->GetTag(*aLocalName);

Go directly to nsIContent from NSObject. Null-check on cont.

>+PRInt32 Node::lookupNamespaceID(txAtom* prefix)

Comment that we use our own implementation until the Mozilla DOM gives us a nice method for this.

>+        String uri;

I think you can use an nsAutoString here. (sorry, i cut away too much context)

>+        NS_ENSURE_SUCCESS(rv, -1);
...
>+        NS_ENSURE_SUCCESS(rv, -1);
...
>+    return -1;

kNameSpaceID_Unknown

>+MBool ProcessingInstruction::getLocalName(txAtom** aLocalName)
>+{
>+    if (!aLocalName)
>+        return MB_FALSE;

Use NS_ENSURE_TRUE? (also in other places)

>+  if (mNamespaceID>=0)

Spaces around >=.

>+        aNSID==attrNode->getNamespaceID() &&
>+        aLocalName==localName) {

Spaces around ==.

>+  String name("xmlns:");
...
>+    if ((xmlns = ((Element*)node)->getAttributeNode(name))) {
...
>+       * xmlns = "" resolves to 0 (null Namespace)

But won't we end up looking for xmlns: = ""?

>+    ~Element();
...
>+    ~Attr();
...

You'll probably need to make these virtual, but i'm not sure.

>+        uri = new String(aURI);
>+        mNamespaces->add(uri);

Need an out-of-memory check.

Comment 51

[Axel Hecht]

New patch coming up, addressing petervs comments.
I got some new comments by mail:

> 1) I'm not sure making you use NS_ENSURE_TRUE was a good idea :/.
> NS_ENSURE_TRUE asserts, and you already asserted most of the time, so
> we'll get double assertions. Sorry.

I cross checked, and there is just one occassion where I was double asserting.
I removed the NS_ENSURE in favor of getting a richer assertion message. The
other NS_ENSUREs are ok.

> Surely you mean if (!aAttr || !aOwner) ;)
nice catch, thanx. (from a pre-version per mail, all /. bugzilla.)

Axel

Comment 52

[Axel Hecht]

Attachment: I sure do see this getting somewhere. Build-and-test-food

Comment 53

[Peter Van der Beken [:peterv]]

Comment on attachment 54047 [details] [diff] [review]
I sure do see this getting somewhere. Build-and-test-food

r=peterv.

Comment 54

[Axel Hecht]

Comment on attachment 54047 [details] [diff] [review]
I sure do see this getting somewhere. Build-and-test-food

this doesn't build on mac, new more friendly patch
is next.

Comment 55

[Axel Hecht]

Attachment: mac friendly patch, friend -> friend class

Comment 56

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

Comment on attachment 54066 [details] [diff] [review]
mac friendly patch, friend -> friend class

Runs like a charm!

r=sicking

Comment 57

[Johnny Stenback (:jst)]

-In Initializr(), null check che return value from NS_NewAtom().

- In List.cpp, inconsistent useage of space-before-and-after-operator, I see
both |i > 0| and |i<0|, be consistent.

- In Attr::getLocalName(), no null check after NS_NewAtom(), same thing in
ProcessingInstruction::getLocalName().

sr=jst with that fixed.

Comment 58

[Axel Hecht]

yesyesyes. Marking fixed.

Comment 59

[Axel Hecht]

bug 98815 didn't really block us.

Comment 60

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

this is in