Last Comment Bug 675166 - Make DocumentTypeNode to use NodeInfoManager of the document
: Make DocumentTypeNode to use NodeInfoManager of the document
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Olli Pettay [:smaug] (high review load, please consider other reviewers)
:
Mentors:
Depends on:
Blocks: 675107 682420 682651
  Show dependency treegraph
 
Reported: 2011-07-29 02:13 PDT by Olli Pettay [:smaug] (high review load, please consider other reviewers)
Modified: 2011-11-04 10:12 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (36.53 KB, patch)
2011-08-26 15:33 PDT, Olli Pettay [:smaug] (high review load, please consider other reviewers)
no flags Details | Diff | Review
patch (51.38 KB, patch)
2011-08-30 08:35 PDT, Olli Pettay [:smaug] (high review load, please consider other reviewers)
jonas: review+
Details | Diff | Review
patch (52.47 KB, patch)
2011-08-30 10:56 PDT, Olli Pettay [:smaug] (high review load, please consider other reviewers)
no flags Details | Diff | Review
uptodate (52.46 KB, patch)
2011-09-16 18:46 PDT, Olli Pettay [:smaug] (high review load, please consider other reviewers)
no flags Details | Diff | Review

Description Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-07-29 02:13:18 PDT
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 2011-07-29 03:14:40 PDT
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) PTO Until July 5th 2011-07-29 07:57:59 PDT
Indeed, DOM Core could simply define a sensible ownerDocument for newly created doctypes.
Comment 3 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-08-26 15:33:02 PDT
Created attachment 556149 [details] [diff] [review]
patch

Uploaded to try
Comment 4 Jonas Sicking (:sicking) PTO Until July 5th 2011-08-29 19:14:02 PDT
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) PTO Until July 5th 2011-08-29 19:20:06 PDT
Holy shit! I didn't realize that bug 335998 was fixed! That's awesome!
Comment 6 Jonas Sicking (:sicking) PTO Until July 5th 2011-08-29 19:24:22 PDT
I'm still not understanding the changes from using "OwnerDoc" to using "OwnerDocument" in a bunch of places?
Comment 7 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-08-30 01:50:47 PDT
(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] (high review load, please consider other reviewers) 2011-08-30 08:35:36 PDT
Created attachment 556855 [details] [diff] [review]
patch

uploaded to tryserver.
Before landing this, acid3 needs to be changed.
Comment 9 Jonas Sicking (:sicking) PTO Until July 5th 2011-08-30 08:54:30 PDT
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] (high review load, please consider other reviewers) 2011-08-30 10:07:40 PDT
(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] (high review load, please consider other reviewers) 2011-08-30 10:56:47 PDT
Created attachment 556919 [details] [diff] [review]
patch
Comment 12 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-09-16 18:46:55 PDT
Created attachment 560664 [details] [diff] [review]
uptodate
Comment 13 :Ms2ger 2011-09-17 06:53:30 PDT
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] (high review load, please consider other reviewers) 2011-09-18 09:38:38 PDT
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] (high review load, please consider other reviewers) 2011-09-18 21:32:43 PDT
dev-doc-needed since this changed docType.ownerDocument handling.
(Not sure if that is documented in MDC)
Comment 16 Eric Shepherd [:sheppy] 2011-11-04 09:45:03 PDT
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.

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