Bug 9596 - [Drosera] add a function popup to the source pane
Summary: [Drosera] add a function popup to the source pane
Status: CLOSED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-06-26 04:32 PDT by Timothy Hatcher
Modified: 2008-05-17 09:55 PDT (History)
2 users (show)

See Also:


Attachments
Patch (10.40 KB, patch)
2006-10-31 19:40 PST, Vladimir Olexa (vladinecko)
timothy: review-
Details | Formatted Diff | Diff
Screenshot with file popup active (218.95 KB, image/jpeg)
2006-10-31 19:42 PST, Vladimir Olexa (vladinecko)
no flags Details
Screenshot with file popup closed and selected function highlighted (184.51 KB, image/jpeg)
2006-10-31 19:43 PST, Vladimir Olexa (vladinecko)
no flags Details
Patch Revised (9.77 KB, patch)
2006-11-03 13:06 PST, Vladimir Olexa (vladinecko)
timothy: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Timothy Hatcher 2006-06-26 04:32:41 PDT
Add a function popup that lists all JavaScript functions similar to Xcode's popup.
Comment 1 Vladimir Olexa (vladinecko) 2006-10-31 19:40:44 PST
Created attachment 11317 [details]
Patch

This patch adds the function popup functionality in Drosera. Once we have fixed the parsing functions to get keywords properly, this will work a bit better and potentially faster as many "if" statements can be removed . Also, we'll need to handle special cases such as var foo = function() or foo: function() so that the popup is a bit more useful in those cases. Right now, the popup only gives "function()" in those instances.
Comment 2 Vladimir Olexa (vladinecko) 2006-10-31 19:42:18 PST
Created attachment 11318 [details]
Screenshot with file popup active
Comment 3 Vladimir Olexa (vladinecko) 2006-10-31 19:43:25 PST
Created attachment 11319 [details]
Screenshot with file popup closed and selected function highlighted
Comment 4 Timothy Hatcher 2006-11-01 11:25:27 PST
Comment on attachment 11317 [details]
Patch

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.
Comment 5 Timothy Hatcher 2006-11-02 15:25:23 PST
Xcode selects the function name when you pick one. We should do the same and not the red highlight.

Here is how in WebKit:

        var selection = window.getSelection();
        var element = document.getElementById("test");
        selection.setBaseAndExtent(element, 0, element, 1);
Comment 6 Vladimir Olexa (vladinecko) 2006-11-03 13:06:35 PST
Created attachment 11367 [details]
Patch Revised

- corrected else statements
- removed unnecessary variable
- removed unnecessary concatenation 
- renamed var counter to functionKeywordOffset
- prototyped Element.removeChildren
- renamed placeholder text to <No selected symbol>
- replaced function name highlight with selection
Comment 7 Timothy Hatcher 2006-11-03 16:08:07 PST
Comment on attachment 11367 [details]
Patch Revised

Looks good.
Comment 8 Timothy Hatcher 2006-11-03 16:17:40 PST
Great job. Landed in r17583.
Comment 9 Timothy Hatcher 2008-05-17 09:55:51 PDT
Closing since Drosera has been replaced by the new Web Inspector debugger. Moving to the New Bugs component so the Drosera component can be closed and removed.