we need namespace support from the DOM

VERIFIED FIXED in mozilla0.9.6

Status

()

Core
XSLT
P3
normal
VERIFIED FIXED
17 years ago
16 years ago

People

(Reporter: sicking, Assigned: Axel Hecht)

Tracking

Trunk
mozilla0.9.6
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 19 obsolete attachments)

487 bytes, text/xml
Details
394 bytes, text/xml
Details
60.97 KB, patch
sicking
: review+
Details | Diff | Splinter Review
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...
Status: NEW → ASSIGNED
Keywords: review
Created attachment 30918 [details] [diff] [review]
here goes...
trying to get this in for 0.9
Target Milestone: --- → mozilla0.9
(Assignee)

Comment 3

17 years ago
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 :(
(Assignee)

Comment 6

17 years ago
> 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?
Target Milestone: mozilla0.9 → mozilla0.9.1
(Assignee)

Comment 9

17 years ago
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
Created attachment 31824 [details] [diff] [review]
patch ver 2
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.
Created attachment 32599 [details] [diff] [review]
small beautification of nsIAtom* handling. Now using a nsCOMPtr
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
Target Milestone: mozilla0.9.1 → mozilla0.9.2
Created attachment 37365 [details] [diff] [review]
diff v4
Created attachment 37366 [details] [diff] [review]
txAtom.cpp
Created attachment 37368 [details]
txAtom.h
Created attachment 37369 [details]
txNamespaceManager.cpp
Created attachment 37370 [details]
txNamespaceManager.h
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...
Created attachment 37876 [details] [diff] [review]
diff ver 5
Created attachment 37877 [details]
txAtom.cpp ver 2
Created attachment 37878 [details]
txAtom.h ver 2
Created attachment 37879 [details]
txNamespaceManager.cpp ver 2
Created attachment 37880 [details]
txNamespaceManager.h ver 2
pushing
Target Milestone: mozilla0.9.2 → mozilla0.9.3
pushing
Target Milestone: mozilla0.9.3 → mozilla0.9.4
(Assignee)

Updated

17 years ago
Priority: -- → P5
(Assignee)

Updated

17 years ago
Blocks: 92634
(Assignee)

Comment 28

16 years ago
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
(Assignee)

Comment 29

16 years ago
Created attachment 44920 [details] [diff] [review]
diff -u -N, contains txAtom.h and txNamespaceManager.h, plus the diff, lot's of changes
(Assignee)

Updated

16 years ago
Priority: P5 → P3
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)
Blocks: 94471
Over to Pike since he's the one doing the work here now...
Assignee: sicking → axel
Status: ASSIGNED → NEW
(Assignee)

Updated

16 years ago
Blocks: 96410
(Assignee)

Comment 32

16 years ago
got new code, but that needs a few tweaks concerning bookkeeping of atoms.
pushing in favor of bug 96647
Target Milestone: mozilla0.9.4 → mozilla0.9.5
(Assignee)

Comment 33

16 years ago
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
(Assignee)

Updated

16 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 34

16 years ago
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
(Assignee)

Updated

16 years ago
Depends on: 98704
(Assignee)

Updated

16 years ago
No longer blocks: 94471
No longer depends on: 98704
(Assignee)

Updated

16 years ago
Depends on: 98704
(Assignee)

Updated

16 years ago
Depends on: 98815
(Assignee)

Comment 35

16 years ago
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?
(Assignee)

Comment 36

16 years ago
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
(Assignee)

Comment 37

16 years ago
Created attachment 51082 [details] [diff] [review]
Current version of patch, feedback required
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...
(Assignee)

Comment 39

16 years ago
nsdoom again. Doing salt'n'peppa
Target Milestone: mozilla0.9.5 → mozilla0.9.6
(Assignee)

Comment 40

16 years ago
Created attachment 52215 [details]
stylesheet for namespace-uri() test
(Assignee)

Comment 41

16 years ago
Created attachment 52216 [details]
xml file for namespace-uri() test
(Assignee)

Comment 42

16 years ago
Created attachment 52217 [details] [diff] [review]
new patch, with fix for xpath namespace-uri(), we do need a testcase
(Assignee)

Comment 43

16 years ago
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
(Assignee)

Updated

16 years ago
Attachment #30918 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #31824 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #32599 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #37365 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #37366 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #37368 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #37369 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #37370 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #37876 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #37877 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #37878 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #37879 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #37880 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #44920 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #51082 - Attachment is obsolete: true
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?


Blocks: 104052
(Assignee)

Comment 45

16 years ago
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
(Assignee)

Comment 46

16 years ago
Created attachment 53083 [details] [diff] [review]
patch (monty, why doesn't this python get smaller with age?)
(Assignee)

Updated

16 years ago
Attachment #52217 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
No longer blocks: 96410
(Assignee)

Updated

16 years ago
Blocks: 96478
(Assignee)

Comment 47

16 years ago
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
Attachment #53083 - Attachment is obsolete: true
(Assignee)

Comment 48

16 years ago
Created attachment 53606 [details] [diff] [review]
another patch, please compile (and test) on all you have. Don't let C++ trick me again.
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
Attachment #53606 - Flags: review+
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.
(Assignee)

Comment 51

16 years ago
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
(Assignee)

Comment 52

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

r=peterv.
Attachment #54047 - Flags: review+
(Assignee)

Updated

16 years ago
Attachment #53606 - Attachment is obsolete: true
(Assignee)

Comment 54

16 years ago
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.
Attachment #54047 - Attachment is obsolete: true
Attachment #54047 - Flags: review+
(Assignee)

Comment 55

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

Runs like a charm!

r=sicking
Attachment #54066 - Flags: review+
-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.

Updated

16 years ago
Attachment #54066 - Flags: superreview+
(Assignee)

Comment 58

16 years ago
yesyesyes. Marking fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
(Assignee)

Comment 59

16 years ago
bug 98815 didn't really block us.
No longer depends on: 98815
this is in
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.