Bug 120529 – Simplify DocumentType handling.

• Component: DOM
• Status: RESOLVED FIXED

[Andreas Kling]

Clean out some DocumentType gunk now that there's always a document().

[Andreas Kling]

Created attachment 210110 [details]
Patch

[Darin Adler]

Comment on attachment 210110 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=210110&action=review

> Source/WebCore/dom/Document.cpp:748
> -void Document::setDocType(PassRefPtr<DocumentType> docType)
> +void Document::setDoctype(PassRefPtr<DocumentType> docType)

The reason we did not name this consistently with the public DOM function doctype is that it’s not a public DOM function, and is intended only for internal use. If I was renaming it, I would rename it setDocumentType, but I also think I would put the logic into Document rather than the DocumentType class and get rid of this function entirely. There are very few top level nodes in the document, so we could do this work any time we are manipulating document children.

> Source/WebCore/dom/Document.cpp:750
>      // This should never be called more than once.

This comment is confusingly inaccurate.

> Source/WebCore/dom/Document.h:-888
> -    void setDocType(PassRefPtr<DocumentType>);

I suggest deleting all three of the functions Document::setDocType, DocumentType::insertedInto, and DocumentType::removedFrom and doing all the work inside Document::childrenChanged instead, as we do for the document element. I would also rename m_docType to m_documentType or eliminate it entirely. I am not sure that finding the document type node is so performance-critical that it justifies being stored in a data member.

> Source/WebCore/dom/DocumentType.cpp:69
> +    document().setDoctype(0);

What prevents more than one DocumentType node from being inserted into the document?

[Andreas Kling]

Created attachment 210292 [details]
Less crappy patch

[Antti Koivisto]

Comment on attachment 210292 [details]
Less crappy patch

View in context: https://bugs.webkit.org/attachment.cgi?id=210292&action=review

r=me

> Source/WebCore/ChangeLog:19
> +        Removed the insertedInto()/removedFrom() handlers from DocumentType.
> +
> +        Document no longer keeps a pointer to its doctype node, it was only used for the
> +        document.doctype DOM API, which now just looks through the list of (<=2) children.
> +
> +        The ENABLE(LEGACY_VIEWPORT_ADAPTION) hunk from Document::setDocType() was moved
> +        into Document::childrenChanged().
> +
> +        We no longer clear the style resolver on doctype insertion/removal since it
> +        doesn't actually affect style anyway.
> +
> +        Also made doctype() return a PassRefPtr<DocumentType> instead of a raw pointer.

Would be good to mention that the DOM spec says dynamic doctype changes have no effect, maybe even in a code comment.

[Andreas Kling]

Created attachment 210294 [details]
Lander

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 210294 [details]:

fast/workers/termination-with-port-messages.html bug 119980 (authors: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 210294 [details]
Lander

Clearing flags on attachment: 210294

Committed r154961: <http://trac.webkit.org/changeset/154961>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/14892699>

[Darin Adler]

Comment on attachment 210294 [details]
Lander

View in context: https://bugs.webkit.org/attachment.cgi?id=210294&action=review

> Source/WebCore/ChangeLog:17
> +        We no longer clear the style resolver on doctype insertion/removal since it
> +        doesn't actually affect style anyway.

If it doesn’t affect style, what does it affect?

> Source/WebCore/ChangeLog:19
> +        Also made doctype() return a PassRefPtr<DocumentType> instead of a raw pointer.

Why? Does not make sense to me.

> Source/WebCore/dom/Document.cpp:760
> +    for (Node* node = firstChild(); node; node = node->nextSibling()) {

Not one of the fancy new child iterators?

[Andreas Kling]

(In reply to comment #10)
> (From update of attachment 210294 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=210294&action=review
> 
> > Source/WebCore/ChangeLog:17
> > +        We no longer clear the style resolver on doctype insertion/removal since it
> > +        doesn't actually affect style anyway.
> 
> If it doesn’t affect style, what does it affect?

It doesn't affect anything internally in WebKit. See also <rdar://problem/14892683>

> > Source/WebCore/ChangeLog:19
> > +        Also made doctype() return a PassRefPtr<DocumentType> instead of a raw pointer.
> 
> Why? Does not make sense to me.

Matter of taste I guess. I feel that functions whose primary purpose is DOM API should return PassRefPtrs, since the caller is expected to take a reference. This puts that information in the function definition.

> > Source/WebCore/dom/Document.cpp:760
> > +    for (Node* node = firstChild(); node; node = node->nextSibling()) {
> 
> Not one of the fancy new child iterators?

The fancy new iterators are for Elements only.