Bug 106491

Created attachment 181986 [details] Work in progress

Created attachment 182202 [details] Patch

(In reply to comment #2) > Created an attachment (id=182202) [details] > Patch Seems like you probably want to merge this with my patch?

*** Bug 106599 has been marked as a duplicate of this bug. ***

Created attachment 182442 [details] Patch

This is now ready for review

Comment on attachment 182442 [details] Patch Attachment 182442 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15822111 New failing tests: fast/xsl/xslt-xhtml-template.xml fast/xpath/xpath-template-element.html fast/xsl/xslt-processor-template.html

Created attachment 182669 [details] Patch

Comment on attachment 182669 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182669&action=review > Source/WebCore/ChangeLog:12 > + Can we have a spec URL? > Source/WebCore/ChangeLog:20 > + * xml/parser/XMLDocumentParserLibxml2.cpp: No changes to Qt's XML parser? > LayoutTests/fast/xsl/xslt-processor-template-expected.txt:1 > +Body divs: CD, Template content divs: AB This output doesn't help us understand what this test is testing. e.g. I can't tell whether this test is passing or failing without having to read the test code. Please add a description. > LayoutTests/fast/xsl/xslt-xhtml-template-expected.txt:3 > +C > +D > +Body divs: CD, Template content divs: AB Ditto.

Comment on attachment 182669 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182669&action=review >> Source/WebCore/ChangeLog:12 >> + > > Can we have a spec URL? done >> Source/WebCore/ChangeLog:20 >> + * xml/parser/XMLDocumentParserLibxml2.cpp: > > No changes to Qt's XML parser? It'd prefer not to mess with Qt's XML parser, especially since this feature is only enabled for the chromium port. I can open a bug against the template element meta bug to fix the Qt XML parser, if you think that's appropriate. >> LayoutTests/fast/xsl/xslt-processor-template-expected.txt:1 >> +Body divs: CD, Template content divs: AB > > This output doesn't help us understand what this test is testing. > e.g. I can't tell whether this test is passing or failing without having to read the test code. > Please add a description. done >> LayoutTests/fast/xsl/xslt-xhtml-template-expected.txt:3 >> +Body divs: CD, Template content divs: AB > > Ditto. done

Created attachment 183056 [details] Patch

ping?

Comment on attachment 183056 [details] Patch 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#parsing-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.

Comment on attachment 183056 [details] Patch 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#parsing-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. done >> Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:792 >> + 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? Changing how we access document() everywhere seems wrong. And no, DOCTYPE is ignored inside template. >> 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. Done >> LayoutTests/fast/dom/HTMLTemplateElement/xhtml-parsing-and-serialization.xml:15 >> +} > > Nit: No curly brackets around a single statement. done >> 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. done

Created attachment 184073 [details] Patch for landing

Comment on attachment 184073 [details] Patch for landing Rejecting attachment 184073 [details] from commit-queue. New failing tests: css2.1/20110323/background-intrinsic-004.htm css2.1/20110323/background-intrinsic-006.htm css2.1/20110323/replaced-intrinsic-ratio-001.htm css3/selectors3/xhtml/css3-modsel-5.xml css2.1/20110323/background-intrinsic-005.htm compositing/images/direct-svg-image.html css3/images/cross-fade-overflow-position.html css2.1/20110323/replaced-intrinsic-002.htm css3/filters/effect-reference-external.html css3/selectors3/xhtml/css3-modsel-7.xml css3/selectors3/xhtml/css3-modsel-4.xml css2.1/20110323/replaced-intrinsic-005.htm css3/selectors3/xhtml/css3-modsel-1.xml css2.1/20110323/replaced-intrinsic-004.htm accessibility/svg-remote-element.html css2.1/20110323/background-intrinsic-002.htm css3/selectors3/xhtml/css3-modsel-2.xml css2.1/20110323/background-intrinsic-007.htm animations/suspend-resume-animation-events.html css2.1/20110323/background-intrinsic-009.htm css2.1/20110323/replaced-intrinsic-003.htm css2.1/20110323/background-intrinsic-001.htm css3/zoom-coords.xhtml css2.1/20110323/replaced-intrinsic-001.htm css2.1/20110323/background-intrinsic-008.htm css3/selectors3/xhtml/css3-modsel-3a.xml css3/selectors3/xhtml/css3-modsel-3.xml css3/selectors3/xhtml/css3-modsel-6.xml http/tests/inspector/resource-parameters.html css2.1/20110323/background-intrinsic-003.htm Full output: http://queues.webkit.org/results/16033918

Comment on attachment 184073 [details] Patch for landing Attachment 184073 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16079389 New failing tests: css2.1/20110323/background-intrinsic-004.htm css3/selectors3/xhtml/css3-modsel-7.xml css2.1/20110323/replaced-intrinsic-ratio-001.htm css3/selectors3/xhtml/css3-modsel-5.xml css2.1/20110323/background-intrinsic-005.htm compositing/images/direct-svg-image.html http/tests/eventsource/eventsource-cors-with-credentials.html css3/images/cross-fade-overflow-position.html css2.1/20110323/replaced-intrinsic-002.htm css3/filters/effect-reference-external.html css3/selectors3/xhtml/css3-modsel-4.xml css2.1/20110323/replaced-intrinsic-005.htm css3/selectors3/xhtml/css3-modsel-1.xml css2.1/20110323/replaced-intrinsic-004.htm accessibility/svg-remote-element.html css2.1/20110323/background-intrinsic-002.htm css3/selectors3/xhtml/css3-modsel-2.xml css2.1/20110323/background-intrinsic-007.htm css2.1/20110323/background-intrinsic-009.htm css2.1/20110323/background-intrinsic-001.htm css2.1/20110323/background-intrinsic-006.htm css3/zoom-coords.xhtml css2.1/20110323/replaced-intrinsic-001.htm css2.1/20110323/replaced-intrinsic-003.htm css2.1/20110323/background-intrinsic-008.htm css3/selectors3/xhtml/css3-modsel-3a.xml css3/selectors3/xhtml/css3-modsel-3.xml css3/selectors3/xhtml/css3-modsel-6.xml http/tests/inspector/resource-parameters.html css2.1/20110323/background-intrinsic-003.htm

Created attachment 184348 [details] Patch for landing

Comment on attachment 184348 [details] Patch for landing Clearing flags on attachment: 184348 Committed r140631: <http://trac.webkit.org/changeset/140631>

All reviewed patches have been landed. Closing bug.