[webkit-reviews] review denied: [Bug 41453] HTMLTreeBuilder needs an adoption agency : [Attachment 60451] fix cr-linux

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Eric Seidel <eric at webkit.org>'s
request for review:
Bug 41453: HTMLTreeBuilder needs an adoption agency
https://bugs.webkit.org/show_bug.cgi?id=41453

Attachment 60451: fix cr-linux
https://bugs.webkit.org/attachment.cgi?id=60451&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
WebCore/html/HTMLElementStack.cpp:167
 +	    recordAbove->next()->element()->beginParsingChildren(); // FIXME:
Is this right?
I think so.

WebCore/html/HTMLElementStack.cpp:245
 +  #if ENABLE(SVG_FOREIGN_OBJECT)
We need to remove this ifdef eventually.

WebCore/html/HTMLElementStack.h:55
 +	    ElementRecord* next() const { return m_next.get(); }
Blank line below.

WebCore/html/HTMLElementStack.cpp:229
 +	return find(element);
!!find ?

WebCore/html/HTMLFormattingElementList.cpp:106
 +	    // This is somewhat of a hack, and is why this method can't be
const.
Yeah, I'd just use a loop here.  It's not like Vector::find is any smarter, and
then we wouldn't need operator==, etc.

WebCore/html/HTMLTreeBuilder.cpp:810
 +	    // !phrasing && !formatting == special || scoping
Nit: The comment is in a different order than the code.

WebCore/html/HTMLTreeBuilder.cpp:831
 +	    fosterParentElement = lastTableElementRecord->next()->element();
This code seems strange.  Do we really prefer the lastTableElement's actual
parent in the DOM?  Keep in mind that DOM nodes can be moved around during
parsing.  Usually we refer only to our own stack of open elements.

WebCore/html/HTMLTreeBuilder.cpp:846
 +	    newParent->appendChild(child, ec);
No, this is no good.  You need to collect these up into a vector and hold
references to them while you twiddle the DOM.  This fires mutation events,
which can do all manner of bad things to you here.