[webkit-reviews] review granted: [Bug 48719] HTML5 TreeBuilder regressed a Peacekeeper DOM test by 40% : [Attachment 82053] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Andy Estes <aestes at apple.com>'s
request for review:
Bug 48719: HTML5 TreeBuilder regressed a Peacekeeper DOM test by 40%
https://bugs.webkit.org/show_bug.cgi?id=48719

Attachment 82053: Patch
https://bugs.webkit.org/attachment.cgi?id=82053&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=82053&action=review

This seems to address everything Eric raised. r=me

> Source/WebCore/dom/ContainerNode.h:180
> +inline ContainerNode* ContainerNode::firstElementChild() const
> +{
> +    Node* child = firstChild();
> +    while (child && !child->isElementNode())
> +	   child = child->nextSibling();
> +    return static_cast<ContainerNode*>(child);
> +}

Could make this return Element* and then move the function body into Element.h.
Not important for this check-in, but I think it would be better.

Generally speaking it’s not good for ContainerNode to have the Element concept
at all, so maybe this should be a function that takes a ContainerNode* rather
than a ContainerNode member function.

> Source/WebCore/html/parser/HTMLConstructionSite.h:138
> +    // This is the root ContainerNode to which the parser attaches all newly

> +    // constructed nodes. It points to a DocumentFragment when parsing
fragments
> +    // and a Document in all other cases.
> +    ContainerNode* m_attachmentRoot;

What guarantees the DocumentFragment will live long enough? I’m surprised we
don’t keep a reference to the document fragment. The only reason we don’t ref
the document is that it would lead to a cycle.