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.