[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