[Webkit-unassigned] [Bug 56262] XML Viewer: declared namespaces are not rendered.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=56262


Pavel Feldman <pfeldman at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #87393|review?, commit-queue?      |review-
               Flag|                            |




--- Comment #4 from Pavel Feldman <pfeldman at chromium.org>  2011-03-29 22:44:40 PST ---
(From update of attachment 87393)
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.