[webkit-reviews] review denied: [Bug 56262] XML Viewer: declared namespaces are not rendered. : [Attachment 87393] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Vsevolod Vlasov
<vsevik at chromium.org>'s request for review:
Bug 56262: XML Viewer: declared namespaces are not rendered.
https://bugs.webkit.org/show_bug.cgi?id=56262

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

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
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