Currently when selecting go to definition it is necessary to highlight the whole word to get that word into the search box. Would be ideal if the current word that the cursor is in would get copied up to the box.

Hi, I am ready to work on this bug. As this is my very first try to conribute to Firefox I need a little help. I have managed to download source code from repo, compile sources and run my first build. I have found that code responsible for Debugger is in /fx-team/browser/devtools/debugger/debugger-toolbar.js. I have discovered that method responsible for "searching for function definition" is called `_doSearch()`. What is more, I have found that current caret position can be get with following method: `DebuggerView.editor.getCaretPosition()`. But not always caret is in the same position as right click. So could you tell me if there is any built-in method which can get position of right click? Or maybe I should write it from scratch? Or maybe there is a built-in method which will return a whole word on which the right click was fired? As this bug is marked as "good-first-bug" and "mentioned-bug", I am hoping that my questions are not stupid ;-) Any help is much appreciated.

Thanks for offering to work on this bu, it's much appreciated! The description of the issue mentions pre-selecting the word the cursor is in, so there is no need to consider right clicks. You only need to make sure that the right text is copied in the search box when search for definition is selected. See bug 852931 for some code that could be reused. Come to think of it, Victor, do you think we should mark that bug as blocking this one and finish that one first? If only a test is missing, this should be very easier to do now with your recent improvements to the debugger tests.

(In reply to Panos Astithas [:past] from comment #2) I think these bugs could not depend on each other. They can be made interdependent, but it depends on how you interpret the requirements for this bug. Knowing what the "current identifier" is when searching for function definitions is tricky and requires parser API work, which was done in bug 852931. After that, it becomes a matter of "if something is selected in the editor when starting to search, put it in the searchbox automatically". I would suggest going the easy route here and leave the hard work for bug 852931. This bug should just check if there's any selected text in the editor and put it in the searchbox. Similarly to what bug 861478 did for the # operator. If marcink would then want to contribute and work on adding tests for bug 852931, that would be amazing. However, it may require a small bit of rebasing since the patch is several months old by now.

(In reply to Victor Porof [:vp] from comment #3) > I would suggest going the easy route here and leave the hard work for bug > 852931. This bug should just check if there's any selected text in the > editor and put it in the searchbox. Similarly to what bug 861478 did for the > # operator. This seems to already work for me: selecting a text and hitting Cmd-D fills the search box with that text before searching.

(In reply to Panos Astithas [:past] from comment #2) > The description of the issue mentions pre-selecting the word the cursor is > in, so there is no need to consider right clicks. You only need to make sure > that the right text is copied in the search box when search for definition > is selected. See bug 852931 for some code that could be reused. OK, it was my misinterpretation - the description clearly says, that "word that the cursor is in would get copied up to the search box. That seems clear to me. (In reply to Victor Porof [:vp] from comment #3) > I would suggest going the easy route here and leave the hard work for bug > 852931. This bug should just check if there's any selected text in the > editor and put it in the searchbox. Similarly to what bug 861478 did for the > # operator. Now I'm a little confused again :D Above-cited functionality already works. If there is any *selected* word the "Go to definition" function is called properly.

Hey, I'd like to take this bug, please assign it to me. I've made a patch which works, but I'm not sure if I've done it "the right way", please guide me. Thanks.

Comment on attachment 8358982 [details] [diff] [review] Bug 911916 - Search set to identifier under cursor if no text selected; reviewer=past Review of attachment 8358982 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/debugger/debugger-toolbar.js @@ +1073,5 @@ > + this._searchbox.value += aText; > + else if (DebuggerView.editor.somethingSelected()) > + this._searchbox.value += DebuggerView.editor.getSelection(); > + else { > + let cursor = DebuggerView.editor.getCursor(), Drive-by, hope you don't mind :) It might be easier and maintainable here to just use Parser.jsm's getIdentifierAt(), now that it's implemented. Something like this: let parsedSource = DebuggerController.Parser.get(text, url); let identifierInfo = parsedSource.getIdentifierAt({line, col}); If `identifierInfo` is null, there's no identifier at that line ad column. Otherwise, a `location` property will contain the identifier range (start/end line and columns). Here's some example usage: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-panes.js#1775 Here's Parser.jsm: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/shared/Parser.jsm I'll let Panos decide if it's worth it here though :)

Instead of using the start and end properties of identifier.location, I used identifier.name, it seems to be fine. It's done, but one thing that I'm not sure if it's a problem with this bug or not, is that if Parser.jsm encounters any error parsing the code, there will be no identifier. As an example on http://www.bing.com/: 1) Web Developer Tools -> Debugger 2) Switch to AutoSug.js 3) Try to use Ctrl+D on a word Log: http://pastebin.mozilla.org/3998452 Thanks Victor.

Comment on attachment 8358995 [details] [diff] [review] Bug 911916 - Search set to identifier under cursor if no text selected; reviewer=past,vp Review of attachment 8358995 [details] [diff] [review]: ----------------------------------------------------------------- I like this :) Thank you! There are a few code styling nits that you might want to address, and it'd certainly be nice if there was a test for this new feature! I'll let Panos help you with that, I don't want to steal his mentorship. ::: browser/devtools/debugger/debugger-toolbar.js @@ +1068,5 @@ > this._searchbox.focus(); > this._searchbox.value = ""; // Need to clear value beforehand. Bug 779738. > + > + if (aText) > + this._searchbox.value = aOperator + aText; Small nit: we typically always use curly braces in the debugger's frontend source code, even for conditionals with a single statement. @@ +1076,5 @@ > + let cursor = DebuggerView.editor.getCursor(), > + content = DebuggerView.editor.getText(), > + location = DebuggerView.Sources.selectedValue, > + source = DebuggerController.Parser.get(content, location), > + identifier = source.getIdentifierAt({ line: cursor.line+1, column: cursor.ch }); Small nit: Having a single let declaration on a line is the prevalent coding style in the debugger frontend.

Thanks, I think I have to read the "Coding Style" again, and take a look at the file I'm editing to get an overall style, +exp. I'll take a look at tests to see how they function and hopefully make a test for this feature. About that Parser.jsm problem I said, is that a part of this bug or not? Thanks again!

Comment on attachment 8358999 [details] [diff] [review] Bug 911916 - Search set to identifier under cursor if no text selected; reviewer=past Review of attachment 8358999 [details] [diff] [review]: ----------------------------------------------------------------- I'm happy with Victor reviewing this!

(In reply to Panos Astithas [:past] from comment #12) > Comment on attachment 8358999 [details] [diff] [review] > Bug 911916 - Search set to identifier under cursor if no text selected; > reviewer=past > > Review of attachment 8358999 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'm happy with Victor reviewing this! Hah, okay!

(In reply to Mahdi Dibaiee [:mdibaiee] from comment #11) > Created attachment 8358999 [details] [diff] [review] > > I'll take a look at tests to see how they function and hopefully make a test > for this feature. > Let me know if you need any help with that! > About that Parser.jsm problem I said, is that a part of this bug or not? Definitely not part of this bug. Please file a new one with a link to the problematic page. It's probably the auto-pretty-printer generating faulty javascript. We've had a few bugs like this in the past, nothing to worry about.

Comment on attachment 8358999 [details] [diff] [review] Bug 911916 - Search set to identifier under cursor if no text selected; reviewer=past Review of attachment 8358999 [details] [diff] [review]: ----------------------------------------------------------------- When this gets a test, I'll land it!

Added the test, but I feel like my test is too small. :D I saw that on other files, "gVar"s were used, and cleaned after the test, but I thought that I don't need them (because everything happens in a single function), so I used let, tell me if I have to change it. I'm confused about file namings and not sure if I gave my test the right name, please help me. Thanks!

Comment on attachment 8359651 [details] [diff] [review] Bug 911916 - Search set to identifier under cursor if no text selected; reviewer=vp Review of attachment 8359651 [details] [diff] [review]: ----------------------------------------------------------------- r+ with the small comments below :) Thank you very much for this patch and contributing to Mozilla! ::: browser/devtools/debugger/test/browser_dbg_search-function.js @@ +1,1 @@ > +function test() { The name of this file is perfectly ok. It doesn't really matter too much. Please add a license boilerplate and the beginning comment, just like in the other tests. Also, add a description about what this test does (even if it's small, it's much easier to quickly glance over a comment to be sure what the test should do). Also, don't forget about "use strict". Everything looks good. Once you update this, I'll push your patch to the try servers to see if the test passes on all platforms. It should be ok though from what I'm seeing here.

Thanks! It makes me feel nice when I help Mozilla. One thing, may I get vouched for Mozillians.org, please? I really like this community. I have contributions to Webmaker translation, too; I'm the coordinator. [1] My username is Mahdi in mozillians. [2] Thanks again! [1]: https://www.transifex.com/projects/p/webmaker/language/fa/members/ [2]: https://mozillians.org/en-US/u/Mahdi/

(In reply to Mahdi Dibaiee [:mdibaiee] from comment #18) > > It makes me feel nice when I help Mozilla. Thank *you*! It looks like you're already vouched on Mozillians.

(In reply to Victor Porof [:vp] from comment #20) > Thank *you*! It looks like you're already vouched on Mozillians Yes, thankfully Josh Matthews vouched me.