[webkit-reviews] review denied: [Bug 43586] Use the HTML5 TreeBuilder for Fragment Parsing : [Attachment 63903] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Eric Seidel <eric at webkit.org>'s
request for review:
Bug 43586: Use the HTML5 TreeBuilder for Fragment Parsing
https://bugs.webkit.org/show_bug.cgi?id=43586

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
WebCore/html/HTMLTreeBuilder.cpp:824
 +  bool isFragmentCaseWithoutBody(const HTMLElementStack* elementStack)
This is wrong.	You probably want to cache this information.  Basically, you're
not covering the case when there are more than three elements on the stack.

WebCore/html/HTMLTreeBuilder.cpp:2902
 +	    m_document->finishedParsing();
I would use a return here instead of an else.  The else is uglies.

WebCore/html/HTMLTreeBuilder.cpp:2900
 +	// Warning, this may delete the parser, so don't try to do anything
else after this.
Please move this comment closer to m_document->finishedParsing().

WebCore/html/HTMLTreeBuilder.cpp:2914
 +		RefPtr<Node> newChild = fragment->document()->adoptNode(child,
ec);
This is dangerous.  We need a version of adoptNode that doesn't run JavaScript.
 Also, you should grab references to all the children first and then move them
to their new home.

WebCore/html/HTMLTreeBuilder.h:212
 +	    RefPtr<Document> m_dummyDocumentForFragmentParse;
m_dummyDocumentForFragmentParse -> m_dummyDocumentForFragmentParsing ?

WebCore/html/HTMLTreeBuilder.h:221
 +	FragmentParsingContext m_fragmentContext;
Insert blank line above.