[webkit-reviews] review granted: [Bug 65803] Initial pass at a new XML tree builder : [Attachment 103324] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Jeffrey Pfau <jeffrey at endrift.com>'s
request for review:
Bug 65803: Initial pass at a new XML tree builder
https://bugs.webkit.org/show_bug.cgi?id=65803

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=103324&action=review


I'm slightly worried about re-entrancy problems.  That was the hardest part
about getting the HTML parser correct.	I don't want to hold you up, because
ironing out all those issues can be tricky, but it's something to keep in mind
that we might need to return to at a later date.

> Source/WebCore/xml/parser/XMLTreeBuilder.cpp:93
> +    m_currentNodeStack.append(m_currentStackItem);

For a later patch, you might want to experiment with using a Vector and using a
linked list.  We found that a linked list was faster for the HTML parser, but
the HTML parser doesn't have as strong a stack discipline.

> Source/WebCore/xml/parser/XMLTreeBuilder.cpp:101
> +    m_currentNodeStack.removeLast();

Is there a reason to keep m_currentNodeStack separate?	It seems like you can
always just use m_currentNodeStack.last().

> Source/WebCore/xml/parser/XMLTreeBuilder.cpp:102
> +    m_currentStackItem = m_currentNodeStack.last();

Does this actually mutate m_currentStackItem?

> Source/WebCore/xml/parser/XMLTreeBuilder.cpp:111
> +    RefPtr<ProcessingInstruction> pi =
ProcessingInstruction::create(m_document, token.target(), token.data());
> +    add(pi);

I would just combine these lines.

> Source/WebCore/xml/parser/XMLTreeBuilder.cpp:171
> +    RefPtr<Element> newElement = m_document->createElement(qName, true);
> +    if (!newElement) {

How can this fail?

> Source/WebCore/xml/parser/XMLTreeBuilder.cpp:186
> +    if (token.selfClosing())
> +	   newElement->finishParsingChildren();
> +    else
> +	   pushCurrentNode(newElement.get());
> +
> +    m_currentStackItem = m_currentNodeStack.last();

I'm still worried about getting re-entered via finishParsingChildren().  We
need to make sure we're in a consistent state before calling out to arbitrary
JavaScript.

> Source/WebCore/xml/parser/XMLTreeBuilder.cpp:218
> +    RefPtr<CDATASection> cdata = CDATASection::create(m_document,
token.data());
> +    add(cdata);

I'd combine these lines.

> Source/WebCore/xml/parser/XMLTreeBuilder.cpp:226
> +    RefPtr<Comment> comment = Comment::create(m_document, token.comment());
> +    add(comment);

These too.

> Source/WebCore/xml/parser/XMLTreeBuilder.cpp:314
> +	  appendToText(reinterpret_cast<UChar*>(&entityValue), 1);

Bad indent.

> Source/WebCore/xml/parser/XMLTreeBuilder.cpp:343
> +    if (!m_sawFirstElement) {
> +	   // FIXME: ensure it's just whitespace
> +	   return;
> +    }

Is this a parse error?	I guess that's what the FIXME comment is about.

> Source/WebCore/xml/parser/XMLTreeBuilder.cpp:354
> +    RefPtr<Text> text = Text::create(m_document, m_leafText->toString());
> +    add(text);

I'd combine these lines.

> Source/WebCore/xml/parser/XMLTreeBuilder.cpp:375
> +	   m_namespace = parent->m_namespace;
> +	   m_scopedNamespaces = parent->m_scopedNamespaces;

Bad indent.

> Source/WebCore/xml/parser/XMLTreeBuilder.h:107
> +    NodeStackItem m_currentStackItem;
> +    Vector<NodeStackItem> m_currentNodeStack;

I'm not sure whether m_currentStackItem is really adding any value given that
m_currentNodeStack knows what the current item is anyway.