675166 - Make DocumentTypeNode to use NodeInfoManager of the document

Status: RESOLVED FIXED

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

Description

[Olli Pettay [:smaug][[e-mail]]]

Currently document.implementation.createDocumentType
creates a dtd node which doesn't have an owner document.
Now that nodes own their document, we could change that so that
dtd node would reuse NodeInfoManager of the document.
After that change we could rename GetOwnerDoc() to OwnerDoc() and remove
bunch of null checks.

For DOM API node.ownerDocument, we could special case dtd node like we special case
document node.

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I'd be just as happy to not special case doctype.ownerDocument, but that doesn't necessarily need to happen in this bug.

Comment 2

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

Indeed, DOM Core could simply define a sensible ownerDocument for newly created doctypes.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Uploaded to try

Comment 4

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

Comment on attachment 556149 [details] [diff] [review]
patch

Review of attachment 556149 [details] [diff] [review]:
-----------------------------------------------------------------

DOM-Core now calls for doctypes to always have a owner document just like all other nodes. It seems simpler to me to simply go directly to that state. That way you can remove the need for the new flag and also nuke the special doctype codepaths rather than try to adjust them to the new solution.

::: content/base/public/nsINode.h
@@ +778,5 @@
>     *         same.
>     */
>    PRBool HasSameOwnerDoc(nsINode *aOther)
>    {
> +    return GetOwnerDoc() == aOther->GetOwnerDoc();

This isn't correct if the owner document has been GC'ed, no? I.e. since we don't have strong references from nodes to their owner document yet, don't we have to go through the nodeinfomanager?

@@ +1267,5 @@
> +  }
> +  void ClearNodeDoesNotHaveOwnerDocument()
> +  {
> +    ClearBoolFlag(NodeDoesNotHaveOwnerDocument);
> +  }

Please follow the whitespace convention used by the other flags.

::: content/base/src/nsDOMDocumentType.cpp
@@ +173,5 @@
>  nsDOMDocumentType::BindToTree(nsIDocument *aDocument, nsIContent *aParent,
>                                nsIContent *aBindingParent,
>                                PRBool aCompileEventHandlers)
>  {
> +  if (!NODE_FROM(aParent, aDocument)->HasSameOwnerDocument(this)) {

Why this change? Couldn't you always call ClearNodeDoesNotHaveOwnerDocument()?

::: content/base/src/nsGenericElement.cpp
@@ +3472,5 @@
>    // Do this before checking the child-count since this could cause mutations
>    nsIDocument* doc = GetCurrentDoc();
>    mozAutoDocUpdate updateBatch(doc, UPDATE_CONTENT_MODEL, aNotify);
>  
> +  if (!HasSameOwnerDocument(aKid)) {

Why do all of these need to change?

Comment 5

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

Holy shit! I didn't realize that bug 335998 was fixed! That's awesome!

Comment 6

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

I'm still not understanding the changes from using "OwnerDoc" to using "OwnerDocument" in a bunch of places?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Jonas Sicking (:sicking) from comment #4)
> DOM-Core now calls for doctypes to always have a owner document just like
> all other nodes.
Oh, when was that changed.

> > +  if (!HasSameOwnerDocument(aKid)) {
> 
> Why do all of these need to change?
Because I assumed that documenttype.ownerDocument would be null when not attached to document.
The patch is all wrong if documenttype.ownerDocument should return something always.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

uploaded to tryserver.
Before landing this, acid3 needs to be changed.

Comment 9

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

Comment on attachment 556855 [details] [diff] [review]
patch

Review of attachment 556855 [details] [diff] [review]:
-----------------------------------------------------------------

r=me (assuming you either recreated the same parts that I had in bug 675107, or that there is r=you on it)

::: content/base/src/nsDocument.cpp
@@ +1325,3 @@
>                        nsIURI* aDocumentURI,
>                        nsIURI* aBaseURI,
>                        nsIPrincipal* aPrincipal);

You can remove the aPrincipal argument and just use mOwner->NodePrincipal() instead.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Jonas Sicking (:sicking) from comment #9)
> r=me (assuming you either recreated the same parts that I had in bug 675107,
> or that there is r=you on it)
r=me for those parts. I didn't include the unrelated changes.

> You can remove the aPrincipal argument and just use mOwner->NodePrincipal()
> instead.
Ah, indeed. Removing mPrincipal.
So far try looks ok.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Attachment: uptodate

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

For the dom-level2-core tests, how about listing them in exclusions.js instead?

Also, does NS_NewDOMDocumentType still need to be a friend of nsNodeInfoManager?

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Sorry, noticed the previous comment too late.
https://hg.mozilla.org/mozilla-central/rev/3048de52e24c

If that change is wanted, please file a followup.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

dev-doc-needed since this changed docType.ownerDocument handling.
(Not sure if that is documented in MDC)

Comment 16

[Eric Shepherd [:sheppy]]

Documentation updated:

https://developer.mozilla.org/En/DOM/Node.ownerDocument#Gecko_notes
https://developer.mozilla.org/En/DOM/DOMImplementation.createDocumentType

Also mentioned on Firefox 9 for developers.