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...

trying to get this in for 0.9

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

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

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 :(

> 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

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

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?

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

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.

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

humhum

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...

pushing

pushing

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

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)

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

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

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

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

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?

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

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...

nsdoom again. Doing salt'n'peppa

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

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?

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 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 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 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.

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 on attachment 54047 [details] [diff] [review] I sure do see this getting somewhere. Build-and-test-food r=peterv.

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 on attachment 54066 [details] [diff] [review] mac friendly patch, friend -> friend class Runs like a charm! r=sicking

-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.

yesyesyes. Marking fixed.

bug 98815 didn't really block us.

this is in