Closed Bug 76070 Opened 23 years ago Closed 23 years ago

we need namespace support from the DOM

Categories

(Core :: XSLT, defect, P3)

Product:
Core
Component:
XSLT
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.6

People

(Reporter: sicking, Assigned: axel)

References

Details

Attachments

(3 files, 19 obsolete files)

stylesheet for namespace-uri() test
487 bytes, text/xml
Details
xml file for namespace-uri() test
394 bytes, text/xml
Details
mac friendly patch, friend -> friend class
60.97 KB, patch
sicking
: review+
jst
: superreview+
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
trying to get this in for 0.9
Target Milestone: --- → mozilla0.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?
Target Milestone: mozilla0.9 → mozilla0.9.1
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
Target Milestone: mozilla0.9.1 → mozilla0.9.2
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
Target Milestone: mozilla0.9.2 → mozilla0.9.3
pushing
Target Milestone: mozilla0.9.3 → mozilla0.9.4
Priority: -- → P5
Blocks: 92634
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
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)
Over to Pike since he's the one doing the work here now...
Assignee: sicking → axel
Status: ASSIGNED → NEW
Blocks: 96410
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
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
Status: NEW → ASSIGNED
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
Depends on: 98704
No longer blocks: 94471
No longer depends on: 98704
Depends on: 98704
Depends on: 98815
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
Target Milestone: mozilla0.9.5 → mozilla0.9.6

    
    

    
  


  

    
    

    
  


  

    
    

    
  
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

    
  

        Attachment #30918 -
        Attachment is obsolete: true

    
  

        Attachment #31824 -
        Attachment is obsolete: true

    
  

        Attachment #32599 -
        Attachment is obsolete: true

    
  

        Attachment #37365 -
        Attachment is obsolete: true

    
  

        Attachment #37366 -
        Attachment is obsolete: true

    
  

        Attachment #37368 -
        Attachment is obsolete: true

    
  

        Attachment #37369 -
        Attachment is obsolete: true

    
  

        Attachment #37370 -
        Attachment is obsolete: true

    
  

        Attachment #37876 -
        Attachment is obsolete: true

    
  

        Attachment #37877 -
        Attachment is obsolete: true

    
  

        Attachment #37878 -
        Attachment is obsolete: true

    
  

        Attachment #37879 -
        Attachment is obsolete: true

    
  

        Attachment #37880 -
        Attachment is obsolete: true

    
  

        Attachment #44920 -
        Attachment is obsolete: true

    
  

        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?




    
    

    
  
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

    
  

        Attachment #52217 -
        Attachment is obsolete: true

    
  
No longer blocks: 96410

    
  
Blocks: 96478

    
    

    
  
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

    
    

    
  
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.

    
    

    
  
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.

        Attachment #54047 -
        Flags: review+

    
  

        Attachment #53606 -
        Attachment is obsolete: true

    
    

    
  
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+

    
    

    
  
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.

        Attachment #54066 -
        Flags: superreview+

    
    

    
  
yesyesyes. Marking fixed.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED

    
    

    
  
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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: