Make DocumentTypeNode to use NodeInfoManager of the document

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

({dev-doc-complete})

Trunk
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Updated

6 years ago
Summary: Make DocumentTypeNode to use NodeInfoManager of some document → Make DocumentTypeNode to use NodeInfoManager of the document
I'd be just as happy to not special case doctype.ownerDocument, but that doesn't necessarily need to happen in this bug.
OS: Linux → All
Hardware: x86_64 → All
Version: unspecified → Trunk
Indeed, DOM Core could simply define a sensible ownerDocument for newly created doctypes.
(Assignee)

Updated

6 years ago
Assignee: nobody → Olli.Pettay
(Assignee)

Updated

6 years ago
Blocks: 682420
(Assignee)

Comment 3

6 years ago
Created attachment 556149 [details] [diff] [review]
patch

Uploaded to try
(Assignee)

Updated

6 years ago
Attachment #556149 - Flags: review?(jonas)

Updated

6 years ago
Blocks: 682651
(Assignee)

Updated

6 years ago
Blocks: 675107
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?
Holy shit! I didn't realize that bug 335998 was fixed! That's awesome!
I'm still not understanding the changes from using "OwnerDoc" to using "OwnerDocument" in a bunch of places?
(Assignee)

Comment 7

6 years ago
(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.
(Assignee)

Comment 8

6 years ago
Created attachment 556855 [details] [diff] [review]
patch

uploaded to tryserver.
Before landing this, acid3 needs to be changed.
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.
Attachment #556855 - Flags: review+
(Assignee)

Comment 10

6 years ago
(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.
(Assignee)

Comment 11

6 years ago
Created attachment 556919 [details] [diff] [review]
patch
(Assignee)

Updated

6 years ago
Attachment #556149 - Flags: review?(jonas)
(Assignee)

Comment 12

6 years ago
Created attachment 560664 [details] [diff] [review]
uptodate
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?
(Assignee)

Comment 14

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

If that change is wanted, please file a followup.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Keywords: dev-doc-needed
(Assignee)

Comment 15

6 years ago
dev-doc-needed since this changed docType.ownerDocument handling.
(Not sure if that is documented in MDC)
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.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.