Find Implementors of a function in the Debugger

RESOLVED FIXED in Firefox 22

Status

()

Firefox
Developer Tools: Debugger
P3
normal
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: rc, Assigned: vporof)

Tracking

(Blocks: 2 bugs)

unspecified
Firefox 22
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 14 obsolete attachments)

v8
165.69 KB, patch
past
: review+
Details | Diff | Splinter Review
170.08 KB, patch
vporof
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
Duplicate of this bug: 762825
(Reporter)

Comment 2

5 years ago
This will require parser support implemented in bug 759837.
Depends on: 759837
Priority: -- → P3
(Assignee)

Comment 3

5 years ago
And we'll use "@" file filtering to search for functions.
Priority: P3 → --
(Assignee)

Updated

5 years ago
Priority: -- → P3
(Assignee)

Comment 4

5 years ago
(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.
(Assignee)

Updated

5 years ago
Depends on: 732442
(Assignee)

Updated

5 years ago
Blocks: 786069
(Assignee)

Comment 5

5 years ago
You know what we could do besides a magical "@" search operator?
Ctrl+click to jump to definition.
(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!
(Reporter)

Comment 7

5 years ago
ctrl-click on mac is context menu (equivalent to right click). I guess cmd-click on mac?

(mac, y u gotta be so difficult?)
(Assignee)

Updated

5 years ago
Blocks: 800858
(Assignee)

Updated

5 years ago
Assignee: nobody → vporof
Status: NEW → ASSIGNED
(Assignee)

Comment 8

5 years ago
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.
Global would be best, since we have complete knowledge of the runtime, but I could live with the local case, if performance proves terrible.
(Assignee)

Comment 10

5 years ago
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.
(Assignee)

Updated

5 years ago
Depends on: 802546
(Assignee)

Updated

5 years ago
Duplicate of this bug: 732442
(Assignee)

Comment 12

5 years ago
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).
Attachment #695316 - Attachment is obsolete: true
(Assignee)

Comment 13

5 years ago
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.
Attachment #695373 - Attachment is obsolete: true
(Assignee)

Comment 14

5 years ago
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 :)
Attachment #695418 - Attachment is obsolete: true
(Assignee)

Comment 15

5 years ago
Created attachment 695534 [details]
looks like this
(Assignee)

Comment 16

5 years ago
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.
Attachment #695529 - Attachment is obsolete: true
(Assignee)

Comment 17

5 years ago
Created attachment 695840 [details] [diff] [review]
v5.1

Fix all the type errors!
Attachment #695836 - Attachment is obsolete: true
(Assignee)

Comment 18

5 years ago
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.
Attachment #695840 - Attachment is obsolete: true
(Assignee)

Comment 19

5 years ago
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
(Assignee)

Comment 20

5 years ago
Created attachment 701138 [details] [diff] [review]
v5.3

Rebased.
Attachment #695997 - Attachment is obsolete: true
(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
(Assignee)

Comment 22

5 years ago
(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?
Flags: needinfo?(mihai.sucan)
(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).
Flags: needinfo?(mihai.sucan)
(Assignee)

Comment 24

5 years ago
Created attachment 720261 [details] [diff] [review]
v6

This was madly bitrotten. Rebased. All tests pass again.
Attachment #701138 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Depends on: 812083
(Assignee)

Comment 25

5 years ago
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.
Attachment #695534 - Attachment is obsolete: true
Attachment #720261 - Attachment is obsolete: true
Attachment #723159 - Flags: review?(mihai.sucan)
(Assignee)

Comment 26

5 years ago
Created attachment 723276 [details] [diff] [review]
v7.1

Now supporting ctrl as well.
(Assignee)

Updated

5 years ago
Depends on: 785704
(Assignee)

Comment 27

5 years ago
Created attachment 723984 [details] [diff] [review]
v7.2

Rebased.
Attachment #723159 - Attachment is obsolete: true
Attachment #723276 - Attachment is obsolete: true
Attachment #723159 - Flags: review?(mihai.sucan)
(Assignee)

Comment 28

5 years ago
Created attachment 723994 [details] [diff] [review]
v7.3

So I pulled the merge from fx-team and I had to rebase again...
Attachment #723984 - Attachment is obsolete: true
(Assignee)

Comment 29

5 years ago
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.
(Assignee)

Updated

5 years ago
Blocks: 852931
(Assignee)

Comment 30

5 years ago
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.
Attachment #723994 - Attachment is obsolete: true
Attachment #727165 - Flags: review?(past)
(Assignee)

Updated

5 years ago
Blocks: 798330
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?
Attachment #727165 - Flags: review?(past) → review+
(Assignee)

Comment 32

5 years ago
(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.
(Assignee)

Updated

5 years ago
Blocks: 853181
(Assignee)

Comment 33

5 years ago
Created attachment 728955 [details] [diff] [review]
v8.1

Fixed orange: https://tbpl.mozilla.org/?tree=Try&rev=ad00eeee485b
Attachment #728955 - Flags: review+
(Assignee)

Updated

5 years ago
Whiteboard: [land-in-fx-team]
(Assignee)

Comment 34

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/33abf3e2d27b
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
(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.
https://hg.mozilla.org/mozilla-central/rev/33abf3e2d27b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 22

Updated

4 years ago
Depends on: 897104

Updated

4 years ago
Depends on: 918017
You need to log in before you can comment on or make changes to this bug.