Bug 56262

Created attachment 85639 [details] Sample XML with namespaces that are prefixed and not prefixed. Also contains a CDATA section.

The attached test case should give you plenty for testing namespaces. It also includes a CDATA section element, which will also be problematic. Do we want to include the CDATA issue in with the namespace issue? I believe they are both related to the XSL implementation.

Created attachment 87393 [details] Patch

Comment on attachment 87393 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87393&action=review Overall looks good. A handful of nits need to be fixed prior to landing. > Source/WebCore/ChangeLog:21 > + * xml/XMLViewer.css: Added. You can remove internals of this file from the ChangeLog. > Source/WebCore/ChangeLog:31 > + * xml/XMLViewer.js: Added. Ditto > Source/WebCore/xml/XMLTreeViewer.cpp:77 > + frame->script()->evaluate("addCSS('" + cssString.replace("\n", " ").replace("'", "\"") + "');"); I wonder if adding a style tag to the document's element is simpler. Alternatively, you should do escape() here and unescape in the JavaScript. > Source/WebCore/xml/XMLViewer.js:44 > + while (document.childNodes.length != 0) { while (var childNode = document.firstChild) { ... } > Source/WebCore/xml/XMLViewer.js:50 > + document.appendChild(html); When you remove head/body from the document, it gets them back. It might work for html documents, not xml though. Anyways, worth checking if don't have them duped. > Source/WebCore/xml/XMLViewer.js:76 > + if (!onWebKitXMLViewerSourceXMLLoaded) This will fail as undefined symbol, should be typeof window.onWebKitXMLViewerSourceXMLLoaded === "function". I'd also make it: if (typeof window.onWebKitXMLViewerSourceXMLLoaded === "function") { // Let extensions override xml viewing onWebKitXMLViewerSourceXMLLoaded(); } else sourceXMLLoaded(); It reads better. Also, content scripts don't actually have access to the page's scripts, so they won't see / be able to define your function. You should only interact with content scripts using DOM. > Source/WebCore/xml/XMLViewer.js:97 > + processNode(nodeParentPairs[i].parentElement, nodeParentPairs[i].node); Why breadth first, but not depth first? You would not need these pairs in DFS, right? > Source/WebCore/xml/XMLViewer.js:107 > + switch (node.nodeType) { Sounds like you could put handler functions into a map and do automatic dispatch in place of this switch. > Source/WebCore/xml/XMLViewer.js:130 > + if (node.childNodes.length == 0) { This is weird, but you don't need to have { } here. if / else in WebKit does not need to have consistent {} > Source/WebCore/xml/XMLViewer.js:134 > + for (var i = 0; i < node.childNodes.length; i++) { You could do: for (var child = node.firstChild; child; child = child.nextSibling) { } > Source/WebCore/xml/XMLViewer.js:263 > +function createHTMLElement(elementName) Can you specify default namespace instead? > Source/WebCore/xml/XMLViewer.js:268 > +function createCollapsable() createCollapsible (a->i) > Source/WebCore/xml/XMLViewer.js:270 > + var collapsable = createHTMLElement('div'); ditto

Just so that it's written down somewhere: why JS, and not C++?

(In reply to comment #5) > Just so that it's written down somewhere: why JS, and not C++? I guess it just sounds easier to do that in JavaScript + we could receive patches with improvements from the third-parties that currently own browser extensions rendering XML. But overall, I see your point - the part that converts XML to the XHTML DOM could be done in C++. It would leave some dynamics in JavaScript though.

Comment on attachment 87393 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87393&action=review >> Source/WebCore/ChangeLog:21 >> + * xml/XMLViewer.css: Added. > > You can remove internals of this file from the ChangeLog. Done. >> Source/WebCore/ChangeLog:31 >> + * xml/XMLViewer.js: Added. > > Ditto Done. >> Source/WebCore/xml/XMLTreeViewer.cpp:77 >> + frame->script()->evaluate("addCSS('" + cssString.replace("\n", " ").replace("'", "\"") + "');"); > > I wonder if adding a style tag to the document's element is simpler. Alternatively, you should do escape() here and unescape in the JavaScript. Done. Style element is created from native code directly now. >> Source/WebCore/xml/XMLViewer.js:44 >> + while (document.childNodes.length != 0) { > > while (var childNode = document.firstChild) { > ... > } Done except you can not put 'var' in while loop condition. var child; while (child = document.firstChild) { ... } >> Source/WebCore/xml/XMLViewer.js:50 >> + document.appendChild(html); > > When you remove head/body from the document, it gets them back. It might work for html documents, not xml though. Anyways, worth checking if don't have them duped. I've checked that nothing gets duplicated. >> Source/WebCore/xml/XMLViewer.js:76 >> + if (!onWebKitXMLViewerSourceXMLLoaded) > > This will fail as undefined symbol, should be typeof window.onWebKitXMLViewerSourceXMLLoaded === "function". I'd also make it: > > if (typeof window.onWebKitXMLViewerSourceXMLLoaded === "function") { > // Let extensions override xml viewing > onWebKitXMLViewerSourceXMLLoaded(); > } else > sourceXMLLoaded(); > > It reads better. > > > Also, content scripts don't actually have access to the page's scripts, so they won't see / be able to define your function. You should only interact with content scripts using DOM. Removed this section completely. Content script are supposed to look for element with id 'webkit-xml-viewer-source-xml' now. I'll comment in corresponding bug once we finish with this and opener issue. >> Source/WebCore/xml/XMLViewer.js:97 >> + processNode(nodeParentPairs[i].parentElement, nodeParentPairs[i].node); > > Why breadth first, but not depth first? You would not need these pairs in DFS, right? To avoid recursion. I feel it would be better. >> Source/WebCore/xml/XMLViewer.js:107 >> + switch (node.nodeType) { > > Sounds like you could put handler functions into a map and do automatic dispatch in place of this switch. Done. >> Source/WebCore/xml/XMLViewer.js:130 >> + if (node.childNodes.length == 0) { > > This is weird, but you don't need to have { } here. if / else in WebKit does not need to have consistent {} Done. >> Source/WebCore/xml/XMLViewer.js:134 >> + for (var i = 0; i < node.childNodes.length; i++) { > > You could do: > > for (var child = node.firstChild; child; child = child.nextSibling) { > } Done. >> Source/WebCore/xml/XMLViewer.js:263 >> +function createHTMLElement(elementName) > > Can you specify default namespace instead? No, default namespace is probably already used by XML document. >> Source/WebCore/xml/XMLViewer.js:268 >> +function createCollapsable() > > createCollapsible (a->i) Done. >> Source/WebCore/xml/XMLViewer.js:270 >> + var collapsable = createHTMLElement('div'); > > ditto Done.

Created attachment 87568 [details] Patch with fixes

Attachment 87568 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1 Source/WebCore/xml/XMLTreeViewer.cpp:45: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 87570 [details] Patch with style fixed

Comment on attachment 87570 [details] Patch with style fixed View in context: https://bugs.webkit.org/attachment.cgi?id=87570&action=review > Source/WebCore/xml/XMLTreeViewer.cpp:75 > + String cssString(reinterpret_cast<const char*>(XMLViewer_css), sizeof(XMLViewer_css)); Nit: define closer to the usage.

Comment on attachment 87570 [details] Patch with style fixed Rejecting attachment 87570 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'apply-..." exit_code: 2 Last 500 characters of output: d at 25442 (offset 8 lines). patching file Source/WebCore/dom/XMLDocumentParserLibxml2.cpp patching file Source/WebCore/xml/XMLTreeViewer.cpp patching file Source/WebCore/xml/XMLViewer.css patching file Source/WebCore/xml/XMLViewer.js patching file Source/WebCore/xml/XMLViewer.xsl rm 'Source/WebCore/xml/XMLViewer.xsl' patching file Source/WebCore/xml/XSLStyleSheet.h Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Pavel Feldman', u'--fo..." exit_code: 1 Full output: http://queues.webkit.org/results/8281950

Created attachment 87682 [details] Patch with nits and merge fixed.

Comment on attachment 87682 [details] Patch with nits and merge fixed. Rejecting attachment 87682 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'apply-..." exit_code: 2 Last 500 characters of output: re.xcodeproj/project.pbxproj patching file Source/WebCore/dom/XMLDocumentParserLibxml2.cpp patching file Source/WebCore/xml/XMLTreeViewer.cpp patching file Source/WebCore/xml/XMLViewer.css patching file Source/WebCore/xml/XMLViewer.js patching file Source/WebCore/xml/XMLViewer.xsl rm 'Source/WebCore/xml/XMLViewer.xsl' patching file Source/WebCore/xml/XSLStyleSheet.h Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Pavel Feldman', u'--fo..." exit_code: 1 Full output: http://queues.webkit.org/results/8311251

Created attachment 87698 [details] merge fix

Committed r82562: <http://trac.webkit.org/changeset/82562>

http://trac.webkit.org/changeset/82562 might have broken GTK Linux 32-bit Release The following tests are not passing: fast/css/dumpAsText/xml-stylesheet-pi-not-in-prolog.xml svg/hixie/error/dumpAsText/005.xml