Last Comment Bug 212218 - Implement DOM Level 3 Document properties/methods
: Implement DOM Level 3 Document properties/methods
Status: RESOLVED FIXED
: meta
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: P4 normal (vote)
: ---
Assigned To: Christopher Aillon (sabbatical, not receiving bugmail)
:
Mentors:
Depends on: 155749 212225 245476 288513
Blocks:
  Show dependency treegraph
 
Reported: 2003-07-09 13:31 PDT by Christopher Aillon (sabbatical, not receiving bugmail)
Modified: 2011-10-09 08:55 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
First pass patch (14.49 KB, patch)
2003-07-09 23:52 PDT, Christopher Aillon (sabbatical, not receiving bugmail)
jst: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
Updated to comments (20.76 KB, patch)
2003-07-11 15:25 PDT, Christopher Aillon (sabbatical, not receiving bugmail)
no flags Details | Diff | Splinter Review

Description Christopher Aillon (sabbatical, not receiving bugmail) 2003-07-09 13:31:16 PDT
 
Comment 1 Christopher Aillon (sabbatical, not receiving bugmail) 2003-07-09 23:52:05 PDT
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.  :-)
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2003-07-10 00:16:16 PDT
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 3 Boris Zbarsky [:bz] (still a bit busy) 2003-07-10 01:22:28 PDT
Comment on attachment 127417 [details] [diff] [review]
First pass patch

sr=me with those nits fixed.
Comment 4 Johnny Stenback (:jst, jst@mozilla.com) 2003-07-10 11:24:10 PDT
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
Comment 5 Christopher Aillon (sabbatical, not receiving bugmail) 2003-07-11 15:25:05 PDT
Created attachment 127581 [details] [diff] [review]
Updated to comments
Comment 6 Christopher Aillon (sabbatical, not receiving bugmail) 2003-07-11 15:34:54 PDT
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.
Comment 7 :Ms2ger (⌚ UTC+1/+2) 2011-10-09 08:55:26 PDT
Marking fixed; everything we should implement from DOM3 Document, and rather much more we shouldn't have, has been implemented.

Note You need to log in before you can comment on or make changes to this bug.