Closed Bug 762160 Opened 9 years ago Closed 9 years ago

Find Implementors of a function in the Debugger

Categories

(DevTools :: Debugger, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 22

People

(Reporter: rcampbell, Assigned: vporof)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 14 obsolete files)

v8
165.69 KB, patch
past
: review+
Details | Diff | Splinter Review
170.08 KB, patch
vporof
: review+
Details | Diff | Splinter Review
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.
Duplicate of this bug: 762825
This will require parser support implemented in bug 759837.
Depends on: 759837
Priority: -- → P3
And we'll use "@" file filtering to search for functions.
Priority: P3 → --
Priority: -- → P3
(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.
Depends on: 732442
Blocks: 786069
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!
ctrl-click on mac is context menu (equivalent to right click). I guess cmd-click on mac?

(mac, y u gotta be so difficult?)
Blocks: 800858
Assignee: nobody → vporof
Status: NEW → ASSIGNED
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.
Attached patch v1 (obsolete) — Splinter Review
Pfew...
This patch pretty much works. Lacks keyboard navigation and some other details, but the logic is all there.
Depends on: 802546
Duplicate of this bug: 732442
Attached patch v2 (obsolete) — Splinter Review
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
Attached patch v3 (obsolete) — Splinter Review
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
Attached patch v4 (obsolete) — Splinter Review
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
Attached image looks like this (obsolete) —
Attached patch v5 (obsolete) — Splinter Review
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
Attached patch v5.1 (obsolete) — Splinter Review
Fix all the type errors!
Attachment #695836 - Attachment is obsolete: true
Attached patch v5.2 (obsolete) — Splinter Review
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
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
Attached patch v5.3 (obsolete) — Splinter Review
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
(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)
Attached patch v6 (obsolete) — Splinter Review
This was madly bitrotten. Rebased. All tests pass again.
Attachment #701138 - Attachment is obsolete: true
Depends on: 812083
Attached patch v7 (obsolete) — Splinter Review
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)
Attached patch v7.1 (obsolete) — Splinter Review
Now supporting ctrl as well.
Depends on: 785704
Attached patch v7.2 (obsolete) — Splinter Review
Rebased.
Attachment #723159 - Attachment is obsolete: true
Attachment #723276 - Attachment is obsolete: true
Attachment #723159 - Flags: review?(mihai.sucan)
Attached patch v7.3 (obsolete) — Splinter Review
So I pulled the merge from fx-team and I had to rebase again...
Attachment #723984 - Attachment is obsolete: true
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.
Blocks: 852931
Attached patch v8Splinter Review
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)
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+
(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.
Blocks: 853181
Whiteboard: [land-in-fx-team]
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
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 22
Depends on: 897104
Depends on: 918017
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.