Most IDEs have an outline view of the source code that is very useful for visualizing its overall structure, but also for quickly locating the function or object of interest. Since we already have support for parsing JS, creating an outline view should be now possible. Incidentally Orion is providing such an outline view for JavaScript now. I think a new tab in the instruments pane should work best for this.

I like this idea. It would complement function searching nicely and provide a neat overview of the currently shown source file.

I was looking for a more adventurous feature to implement over the holiday break. Would it be okay if I take this on?

(In reply to Gabriel Luong (:gluong) from comment #2) > I was looking for a more adventurous feature to implement over the holiday > break. Would it be okay if I take this on? That's awesome Gabriel, thanks for giving this a shot. Let me know if I can help you with anything!

Thanks Victor! I will start working on it this Wednesday after my exams.

Quick update - still working on this. I will upload a working patch on Monday or earlier.

sweet! Thanks for the update, Gabriel. Marking this "Assigned".

Ran into a bug where the source outline is no longer displaying. I will post up a WIP once I fix that.

Turns out the bug I ran into was because the Widget methods changed after rebasing. The patch is still WIP. I am working on the test cases and hopefully will have that up by Friday. A lot of the code was reused from the FilteredFunctionsView on searching for function. Overview: On a source selection, the sourceOutlineView fetches the text from the given source and parses for the named functions. The function items are processed (actualLocation, function name) and grouped according to their inferred chain name. (Example: root.print = function() {} would have an inferred chain name of root.) Currently, the source outline panel needs to enabled in the about:config. I decided to add a pref in case we wanted to refine the source outline view prior to release in Nightly. Currently, pretty print can cause problems since the source is not properly parsed.

Sounds like you made some great progress there! Let me know if you need any help or feedback!

Will do!

Mini-update - I am expecting the test cases to be done and patch up for review and feedback by tomorrow. Some of my current TODOs: - Current XULElement values are null. Should add the function name to the value. - Possibly refactor the attachments to the items pushed into the list. There is currently an extra indirection (sourceOutline) to get the item information (ex, actualLocation)

Added test cases. My current approach is looping through expected results and incrementing the selected index of in the source outline panel, and checking the label, group and caret position of the editor. I need feedback on how to go about looping through the expect results and waiting for the caret to be updated prior to checking the caret position. For the test code files (code-x.js), I copied existing test code into their own files in case they were modified in the future.

Comment on attachment 8361074 [details] [diff] [review] 950189.patch [WIP2] Review of attachment 8361074 [details] [diff] [review]: ----------------------------------------------------------------- Alright, so, before talking about tests, let talk a bit about the source outline itself :) The implementation so far looks very good. My only general concern here though is that the view shows only functions (because you're using the getNamedFunctionDefinitions in Parser.jsm), not other symbols and identifiers. This might not prove to be much more helpful than the already available function search capabilities (via the @ operator), which does essentially the same thing but in a popup. The source outline would be tremendously more helpful if it shown everything in the source (from objects to methods), hierarchically, in a tree, just like any other traditional source outline (that I know of). I'm obviously interested in other opinions on this, so I'm asking Panos if he only had functions in mind, or the full hierarchy with other objects as well. Showing everything will involve a bit more work, but adding new methods to Parser.jsm should be fairly easy, because we already have getIdentifierAt which should serve as inspiration. Cloning and modifying that to retrieve *all* identifiers with their respective type, in a tree of ownership, would be a good approach. (In reply to Gabriel L (:gluong) from comment #12) > Created attachment 8361074 [details] [diff] [review] > 950189.patch [WIP2] > > I need feedback on how to go about looping through the expect results and > waiting for the caret to be updated prior to checking the caret position. To wait for the caret being updated, you need to use waitForCaretUpdated (which is defined in head.js) if you're sure the caret will update (and want to verify that it will update) to a different location. Alternatively, you can use ensureCaretAt if you just want to make sure the caret is simply situated somewhere (but this won't guarantee that it was actually updated if it previously was at the same location). > For the test code files (code-x.js), I copied existing test code into their > own files in case they were modified in the future. I don't think this is necessary. If there's ever going to be the need for different test file contents, we'll clone them then. Maybe YAGNI :)

I haven't considered identifiers, but I definitely think it would be helpful. My current implementation was based on a combination of what I saw on Orion and Sublime Text, but I will look into other editors that provide this feature for inspiration.

One concern about the performance aspect of this. How much impact will this outline creation have on displaying the source (or any other debugger related action for that instance) in the debugger ?

Comment on attachment 8361074 [details] [diff] [review] 950189.patch [WIP2] Review of attachment 8361074 [details] [diff] [review]: ----------------------------------------------------------------- This looks like a very promising start to me! I agree with Victor, the real benefit of an outline view is to present a tree of the code structure, not just the flat list that one can get from the search field. Furthermore, outline views are more common in object-oriented environments, so displaying all object properties is important, as glancing at the outline should let one identify the shape and behavior of objects. I would suggest that you ditch the groups from the list and go for a variables-view-like appearance, where top-level functions or objects appear first, containing functions or objects defined in them, etc. The tree generation needs to be smart enough to indicate getters and setters appropriately (they look like duplicate identifiers currently) and to combine constructor and prototype object in a single branch (prototype properties should become children of the constructor). Parsing seems fast enough in my limited testing, but we should evaluate doing that work in a worker thread for better responsiveness (probably in a followup). Since the outline remains valid for the lifetime of the source, you can cache it and only invalidate it when navigation occurs. Might even be a good idea to use a weakmap with the source client as the key, but we need to make sure we don't hang on to those longer than we should. Lastly, please use a short "Outline" label for the tab, horizontal space is at a premium :-)

(In reply to Panos Astithas [:past] from comment #16) > Comment on attachment 8361074 [details] [diff] [review] > Thanks for the analysis Panos! > Parsing seems fast enough in my limited testing, but we should evaluate > doing that work in a worker thread for better responsiveness (probably in a > followup). Using Parser.jsm in a worker was pretty slow last time I checked, transferring the data between the worker and main thread was slower than the actual benefits gained from using a different thread. I'd love it if things changed since then, so it's probably a good idea to file a different bug and investigate. > Since the outline remains valid for the lifetime of the source, > you can cache it and only invalidate it when navigation occurs. Might even > be a good idea to use a weakmap with the source client as the key, but we > need to make sure we don't hang on to those longer than we should. > One would get that for free when adding methods in Parser.jsm, the caching mechanism is already implemented. Gabriel, you get a free pass on this one ;)

Thanks all! I will get started in investigating what I will need to do for the next iteration. (In reply to Victor Porof [:vp] from comment #17) > > Since the outline remains valid for the lifetime of the source, > > you can cache it and only invalidate it when navigation occurs. Might even > > be a good idea to use a weakmap with the source client as the key, but we > > need to make sure we don't hang on to those longer than we should. > > > > One would get that for free when adding methods in Parser.jsm, the caching > mechanism is already implemented. Gabriel, you get a free pass on this one ;) (In reply to Girish Sharma [:Optimizer] from comment #15) > One concern about the performance aspect of this. How much impact will this > outline creation have on displaying the source (or any other debugger > related action for that instance) in the debugger ? I am gonna try to address the performance concerns and caching mechanism. In regards to displaying the source, the editor is set with the source text prior to any other debugger components (breakpoints and outline) are synchronized. During the generation of the source outline, Parser.jsm get function automatically caches the ast of the source with the url as the key. This translates to some performance benefit if the user decides to search the source. Currently, the entire cache is cleared on navigation. So, I will take the free pass on this one ;) Furthermore, I found that with autopretty print enabled we are doing 2 fetches of the ast (one before the autopretty print and one with the pretty printed source). I will try to address this in the next iteration, but this might actually be a separate bug which I will investigate and report as necessary. I suspect we need a check for autopretty print prior to setting the editor text. In addition, there might be some issues generating an ast from the pretty print source, and hence no outline. This seems to occur when the pretty print generates invalid source code. IIRC there is already a bug reported for this issue a week or two ago.

(In reply to Gabriel L (:gluong) from comment #18) > > Furthermore, I found that with autopretty print enabled we are doing 2 > fetches of the ast (one before the autopretty print and one with the pretty > printed source). I will try to address this in the next iteration, but this > might actually be a separate bug which I will investigate and report as > necessary. I suspect we need a check for autopretty print prior to setting > the editor text. That's a good catch! I think it makes sense to file a new bug for it, but if you think it's easier to fix it directly in this bug, feel free. > In addition, there might be some issues generating an ast from the pretty > print source, and hence no outline. This seems to occur when the pretty > print generates invalid source code. IIRC there is already a bug reported > for this issue a week or two ago. That's definitely the pretty printer generating bad code. There was bug 956567, which didn't seem to land. Anyway, please file bugs if you find new problems as you go! We'll triage and close them if dupes, no need to worry too much about it.

(In reply to Gabriel L (:gluong) from comment #18) > > I am gonna try to address the performance concerns and caching mechanism. In > regards to displaying the source, the editor is set with the source text > prior to any other debugger components (breakpoints and outline) are > synchronized. During the generation of the source outline, Parser.jsm get > function automatically caches the ast of the source with the url as the key. > This translates to some performance benefit if the user decides to search > the source. Currently, the entire cache is cleared on navigation. So, I will > take the free pass on this one ;) For an even bigger perf improvement, updating the Source Outline view should be done lazily, only when visible. There's no benefit in populating the view if it's not already shown, immediately after selecting the source.

Quick update! I have been updating Victor on a weekly basis. Current plan is to implement a new widget similar to FastListWidget, but utilizing a tree hierarchy as seen in VariableViews. I didn't have much time to work on the patch this week since I will be heading to SF tomorrow and need to get all my school work done. Hope to have some new developments in the upcoming weeks!

Thanks for the updates! This looks like it's progressing nicely and you're on the right track! As always, please let me know if you need any guidance. o/

I haven't been too available due to school and now exams (sorry!), but I wanted to share my insight to the outline widget. I was looking at the SideMenuWidget and VariableView, and I realized the SideMenuWidget is quite close to what we would want for the outline widget. In SideMenuWidget, there is a concept of Groups and Items. The current implementation does not allow for nested Group within a Group. However, it may be possible to create a generic Group which allows for nesting in SideMenuWidget. To expand further on this idea, we can add a vbox to act as a list in the Group DOM object. Each Group would also know the parent container it needs to be appended to. As we are appending objects to the SideMenuWidget, the item we just appended might later become a Group as well (ie, can have objects nested under it). This is where it gets messy about how to deal with the Item object since it is inflexible (maybe we don't need it and everything can be a Group?). It may be possible to simply make SideMenuWidget super generic allowing for passing in the classes/ids for the styling as well. To take a step back, this is what I will probably need from a tree widget: I should be able to simply push a given item to a particular Group in the tree. The item in this case will contain some attachment that will tell me about its parent, the label for the item, location in the source, and an unique ID to associate the item I am adding. The following is an example run done of how I might append the source items using the generic SideMenuWidget, but should apply for any tree implementation. Consider the given code: >>function test() {} >>test.prototype = { >> sub: function() {...} >>} Iteration 1: label: test group: "" location: { start: 1, end: 1 } - This will add a Group "test" to the root of the tree since the parent group does not have a name provided. - We might have a map object to map the group ID to the DOM object in the tree. In this case, the group id would be "test" Iteration 2: label: prototype group: test - Creates a new Group with the label "prototype" to the "test" Group - Basically, creates a nested tree. - Group ID would be "test.prototype" to ensure uniqueness. Iteration 3: label: sub group: test.prototype - Appends a new Group "sub" to "test.prototype" group. Every time an item is appended to a Group, the Parent of the item will display an arrow, and allows the vbox to be expanded.

Created a quick prototype applying the ideas I presented above on smashing together SideMenuWidget and VariablesView. If you're applying this patch, I added a quick test outline that is displayed in the Outline tab once Firefox loads up. You can edit this in displayTestOutline() for further testing. About the patch: - The widget allows for appending Groups to Groups, which is not currently possible in SideMenuWidget. However, it completely removes the need for an Item object. - Currently, reusing the variables-view classes. - There are still a lot of work that would be needed such as showing and hiding the arrows, and toggling the expanded list and arrow, but this was only meant to be a quick prototype to illustrate the idea. How I got nesting Groups implemented: I added a new vbox to the Group object, which serves as container for any additional Groups that can be nested under it. When elements for the widget are added, the parent Group is assumed to be created already (in particular the root), and in the Map _groupsByName that maps a group name to the Group object. After getting the cached parent Group, the new element to be added is appended to the parent Group, and added to _groupsByName. I am curious on the directions we should go from here on whether or not to pursue a more generic SideMenuWidget. Most of the code is reused from SideMenuWidget with the modifications to allow for nesting Groups. So, I am somewhat confident that it can be done. Currently, I am also looking at the Parser to see how I can get it to return the outline items I would want.

Added a screenshot illustrating the current outline view based on the test input in displayTestOutline().

Gabriel, why don't you use the TreeWidget which is about to land in fx-team soon (today or tomorrow) ? Then you will not have to hack through SideMenuWidget + Variables View to get multiple groups (hierarchy)

(In reply to Girish Sharma [:Optimizer] from comment #26) > Gabriel, why don't you use the TreeWidget which is about to land in fx-team > soon (today or tomorrow) ? Then you will not have to hack through > SideMenuWidget + Variables View to get multiple groups (hierarchy) Currently, I am focusing more on getting the outline items from the Parser. It was worthwhile to see whether a more generic SideMenuWidget would be a good idea since we would get the TreeWidget as a side effect without creating a new widget.

Looking at the current TreeWidget, it is quite closely align to how I would want to push the outline items to the widget. The OutlineWidget was only meant to be a proof of concept to illustrate a more generic SideMenuWidget.

Comment on attachment 8408783 [details] [diff] [review] 950189.patch[WIP3] Review of attachment 8408783 [details] [diff] [review]: ----------------------------------------------------------------- I very much like the direction this is going. As a general comment: code is a liability, so less code is always better. By that I don't mean needlessly DRYing things up as much as possible, but writing just enough code to suit our needs for the time being, especially in this case. The OutlineWidget looks really good, but it does have more functionality than it should, and can handle much more options that what's actually needed by the Source Outline consumer. So I'd suggest removing the unnecessary code-paths, YAGNI etc. If you decide going with the the different Tree implementation (Optimizer's), I'm also fine with that. However, please make sure it suits your needs first, because from my quick glance over the implementation, it doesn't actually respect the Widgets API at all; it's a standalone widget that doesn't rely on a consumer view inheriting from WidgetMethods. This isn't necessarily a bad thing, but make sure it's what you need first. Let me know if you need any help with the actual Parser stuff ;)

Updated patch to work with Optimizer's TreeWidget.

Added a screenshot of how it works currently. It only fetches the functions for now.

Started the parsing the variable declarations.

- WIP on the variable front - Quickly added tern-like icons to the outline

Add parsing for objects and better parsing of variables and functions within a closure/scope.

It would also be pretty sleek if this outline view would be available in scratchpad as well (most prob. in a follow up though)

Rebased the patch. It definitely would be a follow up to make this work for scratchpad, and webide.

(In reply to Gabriel Luong (:gl) from comment #37) > Rebased the patch. It definitely would be a follow up to make this work for > scratchpad, and webide. Gabriel, what's the status on this? I for one would love to see this land!

(In reply to Gabriel Luong (:gl) from comment #37) > Rebased the patch. It definitely would be a follow up to make this work for > scratchpad, and webide. Gabriel, are you still interested in this? What can we do to help you land this?

(In reply to Eddy Bruel [:ejpbruel] from comment #39) > (In reply to Gabriel Luong (:gl) from comment #37) > > Rebased the patch. It definitely would be a follow up to make this work for > > scratchpad, and webide. > > Gabriel, are you still interested in this? What can we do to help you land > this? I will be working on this over the winter break. I think majority of the work that is still needed is to find the edge cases that I need to handle in the parser.

The new debugger UI has an outline view :)

Jason I don't think this is the same thing. You are probably talking about the outline view of the list of files. The bug is about the outline view of one file. Here in Firefox Nightly 57.0a1 (2017-08-13) (64-bit) (list of files outline view) and in SublimeText (source code outline view) two different things.

The new UI will have an outline view for functions that we'll be enabling soon. It's not as complete a other structural views, but it is a start https://user-images.githubusercontent.com/254562/29275580-98ebe868-80d9-11e7-9d2c-8cbda5c18b40.png