[webkit-reviews] review denied: [Bug 13807] XML without style should render as syntax-highlighted source : [Attachment 83838] Patch with some more fixes

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Feb 25 14:20:06 PST 2011


Pavel Feldman <pfeldman at chromium.org> has denied Vsevolod Vlasov
<vsevik at chromium.org>'s request for review:
Bug 13807: XML without style should render as syntax-highlighted source
https://bugs.webkit.org/show_bug.cgi?id=13807

Attachment 83838: Patch with some more fixes
https://bugs.webkit.org/attachment.cgi?id=83838&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=83838&action=review

Looks great. Please fix the JavaScript nits and we can land this.

> Source/WebCore/xml/XMLViewer.xsl:268
> +		   function onload() {

should be on the next line. Can be fixed while landing.

> Source/WebCore/xml/XMLViewer.xsl:269
> +		     drawArrows();

Four spaces indent. Ditto.

> Source/WebCore/xml/XMLViewer.xsl:301
> +		   function getElementsByClassName(classname, node, recursive) 
{

Too many spaces before { (that should be on the next line)

> Source/WebCore/xml/XMLViewer.xsl:305
> +		       if (recursive) {

No need for { and } for one-liners.

> Source/WebCore/xml/XMLViewer.xsl:311
> +			   if(re.test(els[i].className)) {

Ditto

> Source/WebCore/xml/XMLViewer.xsl:319
> +		       getElementsByClassName('expanded',
button.parentNode.parentNode.parentNode, false)[0].className = 'expanded';

what is button.parentNode.parentNode.parentNode? You should not walk DOM
structure like that - it is better to store a link to the node you need into
the button object itself.
Please use document.querySelectorAll() instead of this getElementsByClassName

> Source/WebCore/xml/XMLViewer.xsl:324
> +		       getElementsByClassName('collapsed',
button.parentNode.parentNode.parentNode, false)[0].className = 'collapsed';

ditto


More information about the webkit-reviews mailing list