The default bug view has changed. See this FAQ.

Implement DOM Level 3 Document properties/methods

RESOLVED FIXED

Status

()

Core
DOM: Core & HTML
P4
normal
RESOLVED FIXED
14 years ago
6 years ago

People

(Reporter: Christopher Aillon (sabbatical, not receiving bugmail), Assigned: Christopher Aillon (sabbatical, not receiving bugmail))

Tracking

({meta})

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 obsolete attachments)

 
Depends on: 212225
Created attachment 127417 [details] [diff] [review]
First pass patch

Still leaves a few things unimplemented for various reasons, either because
they entail some more risky work (renameNode), because of potential security
reasons, or just because.  :-)
Attachment #127417 - Flags: superreview?(bzbarsky)
Attachment #127417 - Flags: review?(jst)
Comment on attachment 127417 [details] [diff] [review]
First pass patch

>Index: dom/public/idl/core/Makefile.in
> 	nsIDOM3Node.idl				\
>+	nsIDOM3Document.idl				\
> 	nsIDOMNSDocument.idl			\
> 	nsIDOMXMLDocument.idl			\
> 	nsIDOMUserDataHandler.idl				\
>+	nsIDOMDOMConfiguration.idl				\

What's with the weird spacing of the '\' at the end?  Could you just line the
up, please?

>Index: dom/public/idl/core/nsIDOM3Document.idl

>+interface nsIDOM3Document : nsISupports

Should this be inheriting from nsIDOM3Node or something like that?  I guess
that's jst's call...

Add a pointer to the relevant spec to the comments here, please.

>Index: dom/public/idl/core/nsIDOMDOMConfiguration.idl

Same here.

>Index: content/base/src/nsDocument.cpp

> nsDocument::Normalize()
>     if (node) {
>       return node->Normalize();
>     }

I'd think you want something more like:

nsresult rv;
if (node) {
  if (NS_FAILED(rv = node->Normalize())) {
    return rv;
  }
}

or something like that.

>+nsDocument::GetXmlEncoding(nsAString& aXmlEncoding)
>+{
>+  SetDOMStringToNull(aXmlEncoding);
>+
>+  if (mXMLDeclarationBits & XML_DECLARATION_BITS_DECLARATION_EXISTS &&
>+      mXMLDeclarationBits & XML_DECLARATION_BITS_ENCODING_EXISTS) {
>+    // XXX We don't store the encoding given in the xml declaration.
>+    // For now, just output the actualEncoding which we do store.
>+    GetActualEncoding(aXmlEncoding);
>+  }

How about calling SeDOMStringToNull in the else clause?

>+nsDocument::GetXmlStandalone(PRBool *aXmlStandalone)
>+{
>+  *aXmlStandalone = PR_FALSE;
>+
>+  if (mXMLDeclarationBits & XML_DECLARATION_BITS_DECLARATION_EXISTS &&
>+      mXMLDeclarationBits & XML_DECLARATION_BITS_STANDALONE_EXISTS &&
>+      mXMLDeclarationBits & XML_DECLARATION_BITS_STANDALONE_YES) {
>+    *aXmlStandalone = PR_TRUE;
>+  }

How about:

*aXMLStandalone = 
    mXMLDeclarationBits & XML_DECLARATION_BITS_DECLARATION_EXISTS &&
    mXMLDeclarationBits & XML_DECLARATION_BITS_STANDALONE_EXISTS &&
    mXMLDeclarationBits & XML_DECLARATION_BITS_STANDALONE_YES;
return NS_OK;

>+nsDocument::GetDocumentURI(nsAString& aDocumentURI)
>+{
>+  SetDOMStringToNull(aDocumentURI);
>+
>+  if (mDocumentURL) {
>+    nsCAutoString uri;
>+    mDocumentURL->GetSpec(uri);
>+    CopyUTF8toUTF16(uri, aDocumentURI);
>+  }

Again, put the SetDOMStringToNull in the else clause of that if.

>+nsDocument::AdoptNode(nsIDOMNode *source, nsIDOMNode **aReturn)
>+{
>+  return NS_ERROR_NOT_IMPLEMENTED;
>+}

Please add a comment that there are security implications to adopting that
would need to be considered, I think....
(and maybe a comment that this basically consists of giving the node a new
nodeinfo).
Comment on attachment 127417 [details] [diff] [review]
First pass patch

sr=me with those nits fixed.
Attachment #127417 - Flags: superreview?(bzbarsky) → superreview+
Comment on attachment 127417 [details] [diff] [review]
First pass patch

In addition to what bz already said:

- In nsDocument::SetXmlStandalone():

+{
+    return NS_ERROR_NOT_IMPLEMENTED;
+}

Use two-space indentation here too, dude :-)

- In nsDocument.h:

@@ -227,16 +228,17 @@ class nsDocument : public nsIDocument,
...
		    public nsIDOM3Node,
+		    public nsIDOM3Document,

I think, after considering this for some time, that we want to make
nsIDOM3Document inherit nsIDOM3Node. Not that it matters for nsIDOM3Document,
but it kinda does matter for nsIDOM3Element (once that comes along) where we'll
hopefully be able to save a vtable slot on all DOM 3 tearoffs if nsIDOM3Element
inherits nsIDOM3Node. So, for consistency's sake, nsIDOM3Document should
inherit nsIDOM3Node...

r/sr=jst
Attachment #127417 - Flags: review?(jst) → review+
Created attachment 127581 [details] [diff] [review]
Updated to comments
Attachment #127417 - Attachment is obsolete: true
Comment on attachment 127581 [details] [diff] [review]
Updated to comments

Patch checked in.  Leaving bug open for now to track other work here in the
dependency list.
Attachment #127581 - Attachment is obsolete: true
Keywords: meta
QA Contact: desale → ian
Priority: -- → P4

Updated

9 years ago
Component: DOM: Core → DOM: Core & HTML
QA Contact: ian → general

Updated

7 years ago
Depends on: 288513

Updated

7 years ago
Depends on: 245476

Updated

7 years ago
Depends on: 155749
Marking fixed; everything we should implement from DOM3 Document, and rather much more we shouldn't have, has been implemented.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.