[webkit-reviews] review denied: [Bug 9596] [Drosera] add a function popup to the source pane : [Attachment 11317] Patch

[bugzilla-request-daemon at macosforge.org]

Timothy Hatcher <timothy at hatcher.name> has denied Timothy Hatcher
<timothy at hatcher.name>'s request for review:
Bug 9596: [Drosera] add a function popup to the source pane
http://bugs.webkit.org/show_bug.cgi?id=9596

Attachment 11317: Patch
http://bugs.webkit.org/attachment.cgi?id=11317&action=edit

------- Additional Comments from Timothy Hatcher <timothy at hatcher.name>
This is looking good. There are a few things that need addressed.

Many elese statments do not meet our style guidlines.

+		     }
+		     else

Place the else on the same line as the end brace.

+		     } else

+		     result += "<span class=\"keyword\">" + "<a
name=\"function-" 

These first two strings can be combined into one, eliminating a concatination.

+    var functionNames = new Array();

Why have a local functionNames when you just use file. functionNames?

+		     var counter = 8;

Rename that to functionKeywordOffset or something meaningful. The current name
is confusing.

Please use "<No selected symbol>", like Xcode. Note the missing spaces and the
capital No.

removeChildrenFromElement should be a prototype function on Element and just
called removeChildren. There are a couple of functions we add to element for
adding and removing class names.

I am not big fan of the red highlight around the function name, lets just
remove that. Xcode dosen't do that.