[webkit-reviews] review granted: [Bug 106491] Template element should parse in XHTML just as it does in HTML : [Attachment 183056] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Rafael Weinstein
<rafaelw at chromium.org>'s request for review:
Bug 106491: Template element should parse in XHTML just as it does in HTML
https://bugs.webkit.org/show_bug.cgi?id=106491

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

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=183056&action=review


> Source/WebCore/ChangeLog:8
> +	  
https://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/templates/index.html#par
sing-xhtml-documents.

It'll be better to refer to a specific revision of the spec. you're
implementing so that we know which revision of the spec you implemented in this
patch should the spec change in the future.

> Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:792
> -    RefPtr<Element> newElement = document()->createElement(qName, true);
> +    RefPtr<Element> newElement =
m_currentNode->document()->createElement(qName, true);

There are a lot more places where document() is referenced like when we
instantiate XMLDocumentParserScope in XMLDocumentParser::doWrite or when we
call executeScript in XMLDocumentParser::endElementNs. It know we don't need to
access document() through m_currentNode in those cases but it might be better
that way for simplicity.

Also, I suppose you can't have DOCTYPE inside a template element?

> Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:826
> +	   pushCurrentNode(newElement.get());

Why don't we just leave it here instead of repeating the same statement twice.

>
LayoutTests/fast/dom/HTMLTemplateElement/xhtml-parsing-and-serialization.xml:15

> +if (window.testRunner) {
> +    testRunner.dumpAsText();
> +}

Nit: No curly brackets around a single statement.

> LayoutTests/fast/xpath/xpath-template-element-expected.txt:3
> +A B

It might be better to hide these once the test had ran in DRT/WTR.

> LayoutTests/fast/xsl/xslt-processor-template-expected.txt:1
> +This tests that XSLT transforms can traverse into XHTML template element
content when applying XSL template. If the test succeeds, the transform will
have swapped the position of the body spans (A and B) with the template content
spans (C and D) and replaced the spans with divs.

This is an interesting behavior. I didn't expect XSLT to work inside the
template. It might be worth mentioning it in the change log
and/or refer to the part of the specification where this behavior is mandated.