Last Comment Bug 762160 - Find Implementors of a function in the Debugger
: Find Implementors of a function in the Debugger
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: unspecified
: All All
: P3 normal (vote)
: Firefox 22
Assigned To: Victor Porof [:vporof][:vp]
:
Mentors:
: 732442 762825 (view as bug list)
Depends on: 785704 732442 759837 802546 812083 897104 918017
Blocks: 786069 800858 798330 852931 853181
  Show dependency treegraph
 
Reported: 2012-06-06 11:31 PDT by Rob Campbell [:rc] (:robcee)
Modified: 2013-09-18 12:41 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (103.13 KB, patch)
2012-12-23 09:12 PST, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v2 (111.85 KB, patch)
2012-12-23 16:14 PST, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v3 (113.87 KB, patch)
2012-12-24 00:09 PST, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v4 (123.14 KB, patch)
2012-12-24 13:32 PST, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
looks like this (178.98 KB, image/png)
2012-12-24 13:47 PST, Victor Porof [:vporof][:vp]
no flags Details
v5 (138.67 KB, patch)
2012-12-26 14:18 PST, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v5.1 (138.77 KB, patch)
2012-12-26 14:31 PST, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v5.2 (146.46 KB, patch)
2012-12-27 04:14 PST, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v5.3 (149.19 KB, patch)
2013-01-11 08:22 PST, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v6 (62.66 KB, patch)
2013-03-02 02:20 PST, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v7 (167.06 KB, patch)
2013-03-09 22:52 PST, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v7.1 (167.09 KB, patch)
2013-03-10 14:37 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v7.2 (168.90 KB, patch)
2013-03-12 09:50 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v7.3 (168.52 KB, patch)
2013-03-12 09:57 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v8 (165.69 KB, patch)
2013-03-20 05:59 PDT, Victor Porof [:vporof][:vp]
past: review+
Details | Diff | Splinter Review
v8.1 (170.08 KB, patch)
2013-03-25 06:54 PDT, Victor Porof [:vporof][:vp]
vporof: review+
Details | Diff | Splinter Review

Description Rob Campbell [:rc] (:robcee) 2012-06-06 11:31:06 PDT
Implement a search feature to "Find Implementors" of a function in the Debugger. Feature will highlight the definition of the function in whichever file loaded in the page.
Comment 1 Victor Porof [:vporof][:vp] 2012-06-14 09:23:00 PDT
*** Bug 762825 has been marked as a duplicate of this bug. ***
Comment 2 Rob Campbell [:rc] (:robcee) 2012-06-14 09:23:14 PDT
This will require parser support implemented in bug 759837.
Comment 3 Victor Porof [:vporof][:vp] 2012-06-14 09:23:36 PDT
And we'll use "@" file filtering to search for functions.
Comment 4 Victor Porof [:vporof][:vp] 2012-08-26 02:51:44 PDT
(In reply to Rob Campbell [:rc] (:robcee) from comment #2)
> This will require parser support implemented in bug 759837.

Reflect.jsm is the cool kid on the block.
Comment 5 Victor Porof [:vporof][:vp] 2012-08-28 08:37:14 PDT
You know what we could do besides a magical "@" search operator?
Ctrl+click to jump to definition.
Comment 6 Panos Astithas [:past] 2012-08-28 10:05:53 PDT
(In reply to Victor Porof [:vp] from comment #5)
> You know what we could do besides a magical "@" search operator?
> Ctrl+click to jump to definition.

Want!
Comment 7 Rob Campbell [:rc] (:robcee) 2012-09-04 10:41:25 PDT
ctrl-click on mac is context menu (equivalent to right click). I guess cmd-click on mac?

(mac, y u gotta be so difficult?)
Comment 8 Victor Porof [:vporof][:vp] 2012-12-11 14:48:46 PST
Should searching for functions be a global operation, or local (in the currently displayed source)? I'm thinking that a symbol is likely be defined in a different file, and since we're jumping to definitions, global searches would make sense. I'm not sure how Reflect.parse would cope with 10 minified jquerys and githubs at once, but I have hopes it wouldn't be that bad.
Comment 9 Panos Astithas [:past] 2012-12-12 02:52:57 PST
Global would be best, since we have complete knowledge of the runtime, but I could live with the local case, if performance proves terrible.
Comment 10 Victor Porof [:vporof][:vp] 2012-12-23 09:12:12 PST
Created attachment 695316 [details] [diff] [review]
v1

Pfew...
This patch pretty much works. Lacks keyboard navigation and some other details, but the logic is all there.
Comment 11 Victor Porof [:vporof][:vp] 2012-12-23 09:21:15 PST
*** Bug 732442 has been marked as a duplicate of this bug. ***
Comment 12 Victor Porof [:vporof][:vp] 2012-12-23 16:14:21 PST
Created attachment 695373 [details] [diff] [review]
v2

Works! Just needs a test.

There were some pretty terrifying race conditions happening when rapidly trying to switch between sources and some of them weren't fetched yet, which this patch fixes.

Most of the parsing is done in a ChromeWorker. The mechanism itself should be fairly reusable in implementing a bunch of other helpers, like a getTokenAtOffset() or findAllSymbols(), which should help bug 762164 a lot.

I haven't yet stumbled upon any websites that cause significant slowdowns due to very large scripts (see comment 8).
Comment 13 Victor Porof [:vporof][:vp] 2012-12-24 00:09:35 PST
Created attachment 695418 [details] [diff] [review]
v3

Added the ability to infer function names from both ObjectExpression and VariableDeclarator blocks. So
> { foo: function(){} }
and
> var bar = function(){}
will yield the anonymous function expressions as actually "foo" and "bar". When facing named function expressions, both the actual and inferred names are shown in the results.
Comment 14 Victor Porof [:vporof][:vp] 2012-12-24 13:32:10 PST
Created attachment 695529 [details] [diff] [review]
v4

Since definitions may be part of a larger assignment expression stack, this patch extends support for function expressions which can be defined in assignment expressions, e.g.
> ... = foo = function(){}
or
> ... = foo.bar = function() {},
in which case it is possible to infer the assignee name.

Thus, the parser can now recognize, for example, that jquery === $ in a minified script.

This patch is also very close to having accel+click on symbols jumping to function definitions (of course, not entirely amazingly smart, but good enough to know if we're hovering over a new or property/call expression, and in which case suggesting the place where that function is defined). Implementation should be more than sufficient for bug 725235. Two birds with one stone :)
Comment 15 Victor Porof [:vporof][:vp] 2012-12-24 13:47:56 PST
Created attachment 695534 [details]
looks like this
Comment 16 Victor Porof [:vporof][:vp] 2012-12-26 14:18:24 PST
Created attachment 695836 [details] [diff] [review]
v5

Accel+click jumps to function definitions, as long as you're hovering a function call or function as an argument.
Comment 17 Victor Porof [:vporof][:vp] 2012-12-26 14:31:46 PST
Created attachment 695840 [details] [diff] [review]
v5.1

Fix all the type errors!
Comment 18 Victor Porof [:vporof][:vp] 2012-12-27 04:14:44 PST
Created attachment 695997 [details] [diff] [review]
v5.2

Ermahgerd optimizations!

Parsing now makes sure that the sought node is part of a branch guaranteed to satisfy certain conditions. Otherwise, abruptly halt walking the corresponding syntax tree branch.
Comment 19 Victor Porof [:vporof][:vp] 2013-01-11 08:19:58 PST
Honza, regarding your question about handling {x, y} coords and also getting the text under the cursor: there are a few API methods in the SourceEditor that expose this functionality already.

1. You'll just need to add a MouseMove event on the instance:
editor.addEventListener("MouseMove", myCallback, false);

2. When the callback is called, the mouse event itself, and the x and y coordinates relative to the editor text area are passed as params [0]

3. Once you have the x and y coords, you can calculate the (approximate) line and column hovered:

> let editorOffset = editor.getOffsetAtLocation(x, y);
> let editorLine = editor.getLineAtOffset(editorOffset);
> let editorLineStart = editor.getLineStart(editorLine);
> let editorColumn = editorOffset - editorLineStart;

4. Getting a region of text under the cursor should be fairly easy at this point. This patch relies on stuff that the reflection API [1] spits out, but there are simpler ways of getting text at offsets if you don't care about AST nodes [2].

5. To display a popup, use convertCoordinates to get the coordinates relative to the entire document (iframe) holding the source editor [3]. You can use these coordinates and the editor.editorElement as an anchor for positioning.

[0] http://mxr.mozilla.org/mozilla-central/source/browser/devtools/sourceeditor/source-editor.jsm#237 
[1] https://developer.mozilla.org/en-US/docs/SpiderMonkey/Parser_API
[2] http://mxr.mozilla.org/mozilla-central/source/browser/devtools/sourceeditor/source-editor-orion.jsm#1531
[3] http://mxr.mozilla.org/mozilla-central/source/browser/devtools/sourceeditor/source-editor-orion.jsm#2018
Comment 20 Victor Porof [:vporof][:vp] 2013-01-11 08:22:13 PST
Created attachment 701138 [details] [diff] [review]
v5.3

Rebased.
Comment 21 Jan Honza Odvarko [:Honza] 2013-01-15 07:52:05 PST
(In reply to Victor Porof [:vp] from comment #19)
> Honza, regarding your question about handling {x, y} coords and also getting
> the text under the cursor: there are a few API methods in the SourceEditor
> that expose this functionality already.
Excellent, thanks for the info!

> 1. You'll just need to add a MouseMove event on the instance:
> editor.addEventListener("MouseMove", myCallback, false);

1) The "addEventListener" doesn't have the third parameter useCapture (as e.g. window.addEventListener does) so, it should not be there, correct? (the same for removeEventListener).

Note that you are also using it in your page in _onEditorLoad and _onEditorUnload

2) Should I be using constants defined in SourceEditor.EVENTS or "MouseMove" string is just fine?

Honza
Comment 22 Victor Porof [:vporof][:vp] 2013-01-15 08:07:44 PST
(In reply to Jan Honza Odvarko from comment #21)
> 
> 1) The "addEventListener" doesn't have the third parameter useCapture (as
> e.g. window.addEventListener does) so, it should not be there, correct? (the
> same for removeEventListener).
> 

You're right, I thought the source editor also exposed this flag, but it seems it doesn't. I need to fix this across the debugger frontend code, thanks!

> Note that you are also using it in your page in _onEditorLoad and
> _onEditorUnload
> 
> 2) Should I be using constants defined in SourceEditor.EVENTS or "MouseMove"
> string is just fine?
> 
> Honza

I used "MouseMove", but I believe SourceEditor.EVENTS should be safer. Mihai?
Comment 23 Mihai Sucan [:msucan] 2013-01-15 08:17:59 PST
(In reply to Victor Porof [:vp] from comment #22)
> > 2) Should I be using constants defined in SourceEditor.EVENTS or "MouseMove"
> > string is just fine?
> > 
> > Honza
> 
> I used "MouseMove", but I believe SourceEditor.EVENTS should be safer. Mihai?

Correct. The idea was that these "constants" allow the source editor to be more flexible in terms of which component is used (orion or something else).
Comment 24 Victor Porof [:vporof][:vp] 2013-03-02 02:20:14 PST
Created attachment 720261 [details] [diff] [review]
v6

This was madly bitrotten. Rebased. All tests pass again.
Comment 25 Victor Porof [:vporof][:vp] 2013-03-09 22:52:23 PST
Created attachment 723159 [details] [diff] [review]
v7

Got bored on the plane and made this thing know about function contexts (in object expression chains, like when defining prototypes, or assignment chains, dummy inheritance like with Object.create() etc.) and added those to the output panel.

I'm worried that the parser will start developing its own intelligence so I'll stop here. We don't want a malefic self aware parser.

Anyway, I also wrote some tests. I'd still like to write a couple more to make sure the ctrl/cmd+click functionality works as expected. Mihai, if you want to start taking a look at this then please feel free. It's not high priority, so no rush.
Comment 26 Victor Porof [:vporof][:vp] 2013-03-10 14:37:00 PDT
Created attachment 723276 [details] [diff] [review]
v7.1

Now supporting ctrl as well.
Comment 27 Victor Porof [:vporof][:vp] 2013-03-12 09:50:09 PDT
Created attachment 723984 [details] [diff] [review]
v7.2

Rebased.
Comment 28 Victor Porof [:vporof][:vp] 2013-03-12 09:57:36 PDT
Created attachment 723994 [details] [diff] [review]
v7.3

So I pulled the merge from fx-team and I had to rebase again...
Comment 29 Victor Porof [:vporof][:vp] 2013-03-20 05:56:45 PDT
As discussed with Panos, we're not going to do accel+click here, I'm forking this to bug 852931. Like I said there, the only (substantial) thing remaining is to write tests.
Comment 30 Victor Porof [:vporof][:vp] 2013-03-20 05:59:39 PDT
Created attachment 727165 [details] [diff] [review]
v8

Removed code handling accel+click.

I didn't know who to ask for review since Mihai has a lot on his plate afaik. So I panicked and chose Panos. This is a very straight-forward patch, I promise :)

Now going through try.
Comment 31 Panos Astithas [:past] 2013-03-22 09:55:11 PDT
Comment on attachment 727165 [details] [diff] [review]
v8

Review of attachment 727165 [details] [diff] [review]:
-----------------------------------------------------------------

I'm very happy we can finally land Parser.jsm! This will give people who want to try alternative parsers a chance to provide concrete feedback. I didn't find any reference to chrome workers though. Didn't you mention at some point that parsing was done in a worker thread?

::: browser/devtools/debugger/test/browser_dbg_scripts-searching-files_ui.js
@@ +174,5 @@
>      } else {
>        ok(false, "How did you get here?");
>      }
>    }
> +  write(".-0")

Missing semicolon.

@@ +251,5 @@
>      } else {
>        ok(false, "How did you get here?");
>      }
>    }
> +  write(".-")

Ditto.

::: browser/devtools/debugger/test/browser_dbg_sources-cache.js
@@ +4,5 @@
> +
> +const TAB_URL = EXAMPLE_URL + "browser_dbg_function-search-01.html";
> +
> +/**
> + * Tests if the sources cache works knows how to cache sources when prompted.

s/works//

::: browser/devtools/shared/Parser.jsm
@@ +56,5 @@
> +
> +    // If there are no script matches, send the whole source directly to the
> +    // reflection API to generate the AST nodes.
> +    if (!scriptMatches.length) {
> +      // Reflect.parse throws when encounters a syntax error.

"when it encounters"

@@ +143,5 @@
> +  /**
> +   * Handles a request for all known syntax trees.
> +   *
> +   * @param string aRequest
> +   *        The function to call on the SyntaxTree instances.

Why not aFunction then instead of aRequest?

@@ +149,5 @@
> +   *        Any kind params to pass to the request function.
> +   * @return array
> +   *         The results given by all known syntax trees.
> +   */
> +  _drain: function STP__drain(aRequest, aParams) {

_drain doesn't seem very intuitive to me, how about _apply, _call or something along those lines?

@@ +244,5 @@
> +        }
> +        // Infer the function's name from an enclosing syntax tree node.
> +        if (parent) {
> +          let inferredInfo = ParserHelpers.inferFunctionExpressionInfo(aNode);
> +          inferredName = inferredInfo.name;

Wouldn't it make sense to avoid the inference cost when a name is present? Do we still need to infer the location?

@@ +358,5 @@
> +    let functionIdentifiers = new Set();
> +
> +    for (let { functionName, inferredName } of functionDefinitions) {
> +      functionIdentifiers.add(functionName);
> +      functionIdentifiers.add(inferredName);

Why do we need the inferred name if there is a functionName?

@@ +383,5 @@
> +   *        The line number to check.
> +   * @return boolean
> +   *         True if the line and column is contained in the node's bounds.
> +   */
> +  isWithinLines: function PH_isWithinLines(aNode, aLine) {

This method looks redundant, since isWithinBounds could be rewritten to behave like this when no column is provided.

::: browser/themes/linux/devtools/debugger.css
@@ +170,5 @@
>    text-shadow: 0 1px #fff;
>  }
>  
> +.results-panel-item-pre {
> +  -moz-margin-end: 5px !important;

Is the !important bit necessary?
Comment 32 Victor Porof [:vporof][:vp] 2013-03-22 13:45:36 PDT
(In reply to Panos Astithas [:past] from comment #31)
> 
> I'm very happy we can finally land Parser.jsm! This will give people who
> want to try alternative parsers a chance to provide concrete feedback.

Well, it's not really a *parser* per se, but more of a smart consumer of AST nodes generated by our internal reflection API. That being said, do you think it'll be a good idea to rename it to ReflectionConsumer.jsm or something similar? It's a mouthful, but it may avoid confusion. Can you think of a different name?

> I didn't find any reference to chrome workers though. Didn't you mention at
> some point that parsing was done in a worker thread?
> 

Yes, I found out that the overhead of cloning data passed between workers and the main thread was a bit too much and basically invalidated any gains in speed. It was even a bit slower!

> 
> @@ +244,5 @@
> > +        }
> > +        // Infer the function's name from an enclosing syntax tree node.
> > +        if (parent) {
> > +          let inferredInfo = ParserHelpers.inferFunctionExpressionInfo(aNode);
> > +          inferredName = inferredInfo.name;
> 
> Wouldn't it make sense to avoid the inference cost when a name is present?
> Do we still need to infer the location?
> 

Sometimes it's useful to know both things. For example:
> { foo: BAR_foo() {} }
would yield both "foo" and "BAR_foo" as identifiers, and two separate locations (one for the property definition and one for the function expression).

> 
> ::: browser/themes/linux/devtools/debugger.css
> @@ +170,5 @@
> >    text-shadow: 0 1px #fff;
> >  }
> >  
> > +.results-panel-item-pre {
> > +  -moz-margin-end: 5px !important;
> 
> Is the !important bit necessary?

I believe so, but I can't remember why. I'll check.
Comment 33 Victor Porof [:vporof][:vp] 2013-03-25 06:54:55 PDT
Created attachment 728955 [details] [diff] [review]
v8.1

Fixed orange: https://tbpl.mozilla.org/?tree=Try&rev=ad00eeee485b
Comment 34 Victor Porof [:vporof][:vp] 2013-03-25 11:05:03 PDT
https://hg.mozilla.org/integration/fx-team/rev/33abf3e2d27b
Comment 35 Panos Astithas [:past] 2013-03-26 01:11:47 PDT
(In reply to Victor Porof [:vp] from comment #32)
> (In reply to Panos Astithas [:past] from comment #31)
> > 
> > I'm very happy we can finally land Parser.jsm! This will give people who
> > want to try alternative parsers a chance to provide concrete feedback.
> 
> Well, it's not really a *parser* per se, but more of a smart consumer of AST
> nodes generated by our internal reflection API. That being said, do you
> think it'll be a good idea to rename it to ReflectionConsumer.jsm or
> something similar? It's a mouthful, but it may avoid confusion. Can you
> think of a different name?

I don't really care about the name, to be honest. We know what it does and why it does it, and if there is ever a need to substitute it with a full parser, it will probably be because we will have a need for the extra features.

> > I didn't find any reference to chrome workers though. Didn't you mention at
> > some point that parsing was done in a worker thread?
> > 
> 
> Yes, I found out that the overhead of cloning data passed between workers
> and the main thread was a bit too much and basically invalidated any gains
> in speed. It was even a bit slower!

Ah, OK then. I guess workers may be beneficial when doing full parsing.
Comment 36 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2013-03-26 05:59:07 PDT
https://hg.mozilla.org/mozilla-central/rev/33abf3e2d27b

Note You need to log in before you can comment on or make changes to this bug.