[webkit-reviews] review denied: [Bug 41337] Teach HTML5TreeBuilder how to merge attributes from extra html/body elements : [Attachment 59997] Patch

Tue Jun 29 09:55:18 PDT 2010

Adam Barth <abarth at webkit.org> has denied Eric Seidel <eric at webkit.org>'s
request for review:
Bug 41337: Teach HTML5TreeBuilder how to merge attributes from extra html/body
elements
https://bugs.webkit.org/show_bug.cgi?id=41337

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
Nice test progression.	Here are a bunch of comments, mostly about naming and
asserts.  I'd like to see this one more time.  You might have written it while
tired.	:)

WebCore/html/HTMLTreeBuilder.cpp:286
 +	m_openElements.pushHtml(attach(m_document, element.release()));
How do we decide if HTML should be Html?

WebCore/html/HTMLTreeBuilder.cpp:331
 +		insertHead(token);
insertHeadElement

WebCore/html/HTMLTreeBuilder.cpp:351
 +		insertBody(token);
insertBodyElement

WebCore/html/HTMLTreeBuilder.cpp:958
 +  void HTMLTreeBuilder::insertHtml(AtomicHTMLToken& token)
ugg.  Naming problems...

WebCore/html/HTMLTreeBuilder.cpp: 
 +	ASSERT(token.type() == HTMLToken::StartTag);
Why did you remove these asserts?

WebCore/html/HTMLTreeBuilder.cpp: 
 +	ASSERT(token.type() == HTMLToken::StartTag);
ditto

WebCore/html/HTMLTreeBuilder.cpp:963
 +  void HTMLTreeBuilder::insertHead(AtomicHTMLToken& token)
Please ASSERT that the token is a StartTag with a name of HEAD

WebCore/html/HTMLTreeBuilder.cpp:969
 +  void HTMLTreeBuilder::insertBody(AtomicHTMLToken& token)
please add that ASSERT to all these cases.

WebCore/html/HTMLTreeBuilder.h:133
 +		ASSERT(m_headElement == m_headElement);
This assert seems vacuous.

WebCore/html/HTMLTreeBuilder.h:169
 +		ASSERT(m_htmlElement);
Please assert that element doesn't have a tag name of HTML, Body, or Head.

WebCore/html/HTMLTreeBuilder.h:251
 +	    // We remember <html>, <head> and <body> as their pushed.  Their
their -> they are