XML Viewer: declared namespaces are not rendered.

[Pavel Feldman]

Alan, could you attach test cases for this please?

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.

[Vsevolod Vlasov]

Created attachment 87393 [details]
Patch

[Pavel Feldman]

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

[Alexey Proskuryakov]

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

[Pavel Feldman]

(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.

[Vsevolod Vlasov]

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.

[Vsevolod Vlasov]

Created attachment 87568 [details]
Patch with fixes

[WebKit Review Bot]

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.

[Vsevolod Vlasov]

Created attachment 87570 [details]
Patch with style fixed

[Pavel Feldman]

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.

[WebKit Commit Bot]

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

[Vsevolod Vlasov]

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

[WebKit Commit Bot]

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

[Vsevolod Vlasov]

Created attachment 87698 [details]
merge fix

[Pavel Feldman]

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

[WebKit Review Bot]

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