[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





--- Comment #7 from Vsevolod Vlasov <vsevik at chromium.org>  2011-03-30 10:31:17 PST ---
(From update of attachment 87393)
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.

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