72522 - support the baseURI attribute in XML and HTML DOM

Status: RESOLVED FIXED

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

Description

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

The Core DOM L3 attribute baseURI could simplify much code in mozilla a great 
deal. The attribute should support xml:base for XML DOM and <base> for HTML DOM 
(<base> should probobly affect the documents baseURI). I'm not really sure how 
XHTML should be handled but i'm guessing both xml:base and <html:base> affects 
baseURI in XHTML DOM.

This is a tracker so that i'll have someplace to add dependencies

Comment 1

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

This is not just a tracker, this is a real rfe. I had not made this a bug
earlier since I was going to fix this as part of another bug that required this
feature. Since we have more dependences it made sense to open this one; thanks.

Added dependences:

* Bug 48217 - XML Base should affect Open Link in New Window
* Bug 54074 - XML Base should affect :visited etc CSS styles
* Bug 49462 - XML Base should affect PI href

There are some difficulties in implementing DOM Level 3 stuff in Mozilla because
we do not want/cannot extend the existing DOM interfaces. We must create new
interfaces for DOM Level 3, and adopt code to QI for that etc. (will work just
fine for JS because of automatic interface flattening). There are some questions
to be answered, like should DOM Level 3 interfaces inherit from earlier
interfaces, or should they stand on their own (and implementations should
inherit from both). Currently I am leaning towards inheriting from old. Also we
need to choose a naming convention. I'll give the two options using Node as an
example: nsIDOM3Node or nsIDOMNode3.

Once the level 3 interface are in place, we need to figure out a way to hook
them to the implementations. At the moment I do not know how to do this.

Comment 2

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

Uh huh, 54074 should have been 54047:

* Bug 54047 - XML Base should affect :visited etc CSS styles

Comment 3

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

Attachment: First cut fix

Comment 4

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

This is the first implementation that actually compiled, so there are probably
going to be some changes to it. It does seem to work with some of my testcases
in XML at least. I have not implemented this for XUL.

Comment 5

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

Attachment: Proposed fix 1

Comment 6

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

I found one problem with the first patch: it did not check the current element's
baseURI, it checked the parent's. I fixed that in the proposed fix 1. That patch
is also cleaner as I updated my tree today and cleared some junk.

I'll come up with some testcases for this as well.

Comment 7

[Johnny Stenback (:jst)]

A couple of comments about the patch:

First of all, the long term solution for supporting DOM Level 3
interfaces/methods in mozilla should be (if we ever wanto freeze our DOM
interfaces, and I think we really wanto do that) to add interfaces (i.e.
nsIDOMNode3 or nsIDOM3Node) and implement those as tearoffs on the objects that
implement them (unless the object is a document or somesuch where the number of
vtables doesn't matter). But given that I'm about to rewrite the DOM interfaces
in XPIDL I'm ok with checking this in for now, but I will change this later
(i.e. I will create the new nsIDOMNode3, or whatever, interface).

This code:

  aURI.Assign(NS_ConvertASCIItoUCS2(spec));

can be written more efficiently as:

  CopyASCIItoUCS2(nsLiteralCString(spec), aURI);

In nsGenericElement::GetBaseURI()

...
+    nsCOMPtr<nsIContent> 
node(do_QueryInterface(NS_STATIC_CAST(nsIContent*,this)));
^^^^ Reanme this to 'content', when I see 'node' I think of the DOM interface,
not nsIContent.

+    while (node) {
+      nsCOMPtr<nsIXMLContent> xmlContent(do_QueryInterface(node));
+      if (xmlContent) {
+        xmlContent->GetXMLBaseURI(getter_AddRefs(uri));
+        break;
+      }
+      node->GetParent(*getter_AddRefs(node));

This will fail to work on many compilers (null ptr reference since
getter_AddRefs() nulls out 'node'), change this to:

       nsIContent *tmp = node; (or 'content' :-)
       tmp->GetParent(*getter_AddRefs(node));

+    }

Also in nsGenericElement::GetBaseURI():

+  if (!uri && mDocument) {
+    // HTML document or for some reason there was no XML content in XML document
+    mDocument->GetBaseURL(*getter_AddRefs(uri));
+  }

AFAIK there's no guarantee that nsIDocument::GetBaseURL() always returns a url,
I think the base url is only returned if it's actually specified in the
document, check for a null url and call nsIDocument::GetDocumentURL(). Same goes
for nsHTMLDocument::GetBaseURI(), mBaseURI can be null, AFAIK.

The method nsIXMLContent::GetXMLBaseURI() really belongs in nsIContent, we're
trying to eliminate nsIXMLContent so no new methods should be put in that
interface, but since the code for this method already exists in nsXMLElement I'd
accept this for now, it will be moved (and probably renamed) once nsIXMLContent
is eliminated, unless you wanto do it now.

Other than that the changes look good to me, sr=jst with the above changes.

Comment 8

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

Attachment: Proposed fix 2

Comment 9

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

In proposed fix 2 I fixed all the points jst talked about. I do believe that
base URL would always be in a document - if no base tag is present then it would
just be document base. But I bullet proofed this as well as jst noted.

Additionally, I changed nsXMLElement::GetXMLBaseURI() a little so that it also
tries first baseURL from document and if not present, the document URL. Also
because the method was changed to an interface I changed the signature to follow
that, and simplified the null check in the beginning a little. I also added some
baseURI stuff to XUL, which is better than nothing (basically just get document
URL).

The patch also contains some changes to nsIDOMDocument. When the dir property
was added I think they did the changes by hand, not with the xpidlc compiler as
they should have. The changes in the interface were made by the compiler.

Comment 10

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

Attachment: Prposed fix 3, bullet proof nsXULElement GetXMLBaseURI

Comment 11

[harishd]

r=harishd for the above patch.

Comment 12

[Johnny Stenback (:jst)]

sr=jst

Comment 13

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

Fix checked in.

Comment 14

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

Reopening. Had to back out to fix binary compatibility, will reassign to jst
'cos he has the fix on the DOM conversion branch.

Comment 15

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

You have the fix on the DOM conversion branch, reassigning.

Comment 16

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

Look at bug 76641 to see what needs to go back once you fix this.

Comment 17

[Johnny Stenback (:jst)]

Fixed.

Comment 18

[Prashant Desale]

Updating QA contact to Shivakiran Tummala.

Comment 19

[Sivakiran Tummala]

i created a small testcase to set the baseURI .... i was unable to set the
baseURI...there were no exceptions thrown... 
tested on linux and windows..

build id linux 2001-06-07-13-0.9.1
build id windows 2001-06-07-06-0.9.1

i think this can be reproduced on all operating systems....

<script>
document.write(document.documentElement.baseURI);
document.documentElement.setBaseURI("http://localhost");
document.write(document.documentElement.baseURI);
</script>

i am reopening this bug 

Comment 20

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

there is no such method as setBaseURI. baseURI is an readonly attribute so 
first of all you can't set it, and second the syntax would be:

docuent.documentElement.baseURI = "http://foobar.com/"

which should throw an exception.

Comment 21

[Johnny Stenback (:jst)]

Jonas, docuent.documentElement.baseURI = "http://foobar.com/" should *not* throw
an exception. It does so in mozilla, and that won't change, but by the
ECMAScript standard setting readonly properties on an object fails silently. The
DOM ECMAScript bindings doesn't specify otherwise so technically mozilla is
doing the wrong thing.

Comment 22

[Sivakiran Tummala]

verified ...the exception was thrown when trying to set the baseURI

Comment 23

[Sivakiran Tummala]

the exception was thrown when trying to set the baseURI.......the script did not
die silently with out raising exception (johnny can u comment on this  .)

Comment 24

[Sivakiran Tummala]

reopening

Comment 25

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

jst's comment previously was that we know that trying to set baseURI throws in
Mozilla and it won't change. Besides, that is a different bug if you want to
file one. Marking this fixed once more.