762160 - Find Implementors of a function in the Debugger

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Rob Campbell [:rc] (:robcee)]

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 2

[Rob Campbell [:rc] (:robcee)]

This will require parser support implemented in bug 759837.

Comment 3

[Victor Porof [:vporof][:vp]]

And we'll use "@" file filtering to search for functions.

Comment 4

[Victor Porof [:vporof][:vp]]

(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]]

You know what we could do besides a magical "@" search operator?
Ctrl+click to jump to definition.

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

(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)]

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]]

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 (he/him) [:past] (please ni?)]

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]]

Attachment: v1

Pfew...
This patch pretty much works. Lacks keyboard navigation and some other details, but the logic is all there.

Comment 12

[Victor Porof [:vporof][:vp]]

Attachment: 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]]

Attachment: 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]]

Attachment: 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]]

Attachment: looks like this

Comment 16

[Victor Porof [:vporof][:vp]]

Attachment: 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]]

Attachment: v5.1

Fix all the type errors!

Comment 18

[Victor Porof [:vporof][:vp]]

Attachment: 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]]

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]]

Attachment: v5.3

Rebased.

Comment 21

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(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]]

(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]]

(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]]

Attachment: v6

This was madly bitrotten. Rebased. All tests pass again.

Comment 25

[Victor Porof [:vporof][:vp]]

Attachment: 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]]

Attachment: v7.1

Now supporting ctrl as well.

Comment 27

[Victor Porof [:vporof][:vp]]

Attachment: v7.2

Rebased.

Comment 28

[Victor Porof [:vporof][:vp]]

Attachment: v7.3

So I pulled the merge from fx-team and I had to rebase again...

Comment 29

[Victor Porof [:vporof][:vp]]

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]]

Attachment: 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 (he/him) [:past] (please ni?)]

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]]

(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]]

Attachment: v8.1

Fixed orange: https://tbpl.mozilla.org/?tree=Try&rev=ad00eeee485b

Comment 34

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/integration/fx-team/rev/33abf3e2d27b

Comment 35

[Panos Astithas (he/him) [:past] (please ni?)]

(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] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/33abf3e2d27b