Bug 120529 - Simplify DocumentType handling.
Summary: Simplify DocumentType handling.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-08-30 08:11 PDT by Andreas Kling
Modified: 2013-09-02 11:33 PDT (History)
7 users (show)

See Also:


Attachments
Patch (3.76 KB, patch)
2013-08-30 08:12 PDT, Andreas Kling
koivisto: review+
Details | Formatted Diff | Diff
Less crappy patch (7.05 KB, patch)
2013-09-02 09:04 PDT, Andreas Kling
koivisto: review+
Details | Formatted Diff | Diff
Lander (7.20 KB, patch)
2013-09-02 09:27 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2013-08-30 08:11:56 PDT
Clean out some DocumentType gunk now that there's always a document().
Comment 1 Andreas Kling 2013-08-30 08:12:25 PDT
Created attachment 210110 [details]
Patch
Comment 2 Darin Adler 2013-08-30 09:35:01 PDT
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?
Comment 3 Andreas Kling 2013-09-02 09:04:18 PDT
Created attachment 210292 [details]
Less crappy patch
Comment 4 Antti Koivisto 2013-09-02 09:14:32 PDT
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.
Comment 5 Andreas Kling 2013-09-02 09:27:08 PDT
Created attachment 210294 [details]
Lander
Comment 6 WebKit Commit Bot 2013-09-02 10:02:58 PDT
The commit-queue encountered the following flaky tests while processing attachment 210294 [details]:

fast/workers/termination-with-port-messages.html bug 119980 (authors: dimich@chromium.org and rniwa@webkit.org)
The commit-queue is continuing to process your patch.
Comment 7 WebKit Commit Bot 2013-09-02 10:04:18 PDT
Comment on attachment 210294 [details]
Lander

Clearing flags on attachment: 210294

Committed r154961: <http://trac.webkit.org/changeset/154961>
Comment 8 WebKit Commit Bot 2013-09-02 10:04:21 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2013-09-02 10:39:19 PDT
<rdar://problem/14892699>
Comment 10 Darin Adler 2013-09-02 11:28:07 PDT
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?
Comment 11 Andreas Kling 2013-09-02 11:33:00 PDT
(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.