[webkit-reviews] review denied: [Bug 41337] Teach HTML5TreeBuilder how to merge attributes from extra html/body elements : [Attachment 59997] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
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
More information about the webkit-reviews
mailing list