STR: 1. Cmd+F to focus the search box with "#" operator 2. type something you know is in the debugged script multiple times (var/let/etc...) 3. Focus the source editor (maybe you did this because are adding breakpoints or just want to click and scroll around or whatever) 4. Cmd+F to focus the search box again Expected: The search box has "#|<thing you queried in step 2>|" where text in between "|"s is selected. Now you can continue cycling through your matching results with ENTER or Cmd+G some more, or you can start searching for new things in the page without accidentally removing the "#" operator. Actual: The search box's previous input was cleared and "#" is in the search box. -------------------------------------------------------------------------------- I think as long as you are using the same search operator, we should maintain your previous query.

Makes sense.

I could try to resolve this bug if no one is looking into it. So far my findings are: debugger-toolbar.js Looks like it is the problem with |FilterView| class (line 685) |_doSearch| function will clear the input on line 1070: this._searchbox.value = ""; // Need to clear value beforehand. Bug 779738. It seems that |aText| parameter is not passed into the function from |_doTokenSearch| (line 1126) This parameter is an odd one since |_doSearch| does not even have a comment explaining usage of the |aText| parameter and its meaning. (functions _doFileSearch, _doGlobalSearch, _doFunctionSearch, _doLineSearch, _doVariableSearch neither take an advantage of the aText parameter as well) There are 2 ways of solving the problem: A) modify |_doSearch| function's logic by removing |aText|. Basically keep in memory existing input's text and then make a decision if to use it or to use (for example) selected text. B) modify function |_doTokenSearch| which do call |_doSearch| to take advantage of the |aText| parameter.

I made an attempt of solving this bug. Please have a look.

Comment on attachment 8477865 [details] [diff] [review] bug908956.patch Review of attachment 8477865 [details] [diff] [review]: ----------------------------------------------------------------- This change will probably break a lot of tests. Please make sure you fix them before asking for review.

Hi Kushagra. I noticed you wrote a patch for this bug. Are you still interested in landing it? If so, what can we do to help you?

Hi Eddy. It seems that you have flagged the wrong kushagra. I'll forward your query to the correct kushagra. ----Kushagra---- Hey !! If you are interested in landing your patch then please do get in touch with Eddy [:ejpbruel]

This bug looks like it's been abandoned, but I've made a patch that should work. It covers cases where: * There is text selected in the debugger panel - use the selected text * There is no text selected, the command requested the same search type as before, there is existing text in the search box - use the existing search text * There is no text selected, the command requested a different search type - use a blank search with the operator character There is also the code block at https://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-toolbar.js#1117 I couldn't figure out what case this was testing for, so I left it in.

Comment on attachment 8580040 [details] [diff] [review] bug908956-debuggerSearchBoxClearPrevention.diff Review of attachment 8580040 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/debugger/debugger-toolbar.js @@ +1103,5 @@ > * The operator to use for filtering. > */ > _doSearch: function(aOperator = "", aText = "") { > this._searchbox.focus(); > + aText = this._searchbox.value; // Get existing value, for re-use. Bug 908956. You're always overriding aText here. In this case, having this extra argument is useless. A more appropriate fix would be to go through all the call sites and make sure the correct text is passed in. Otherwise, remove this extra argument, and make sure the existing tests still pass. Let me know if you need help with anything.

I've removed the argument, and everything is building correctly. However, I'm unsure as to which tests to run, or how to run the tests.

(In reply to Léon McGregor from comment #9) > I've removed the argument, and everything is building correctly. However, > I'm unsure as to which tests to run, or how to run the tests. See https://wiki.mozilla.org/DevTools/Hacking#DevTools_Mochitests for info on devtools tests. In this case, you would do this: ./mach mochitest-devtools browser/debugger

I wasn't sure I was doing it right, but this throws an error > UnicodeDecodeError: 'ascii' codec can't decode byte 0x82 in position 17: ordinal not in range(128) It's not as if a test is failing, but that the script to run the tests is failing. I can't see why.

(In reply to Léon McGregor from comment #11) > I wasn't sure I was doing it right, but this throws an error > > UnicodeDecodeError: 'ascii' codec can't decode byte 0x82 in position 17: ordinal not in range(128) > It's not as if a test is failing, but that the script to run the tests is > failing. I can't see why. What version of python do you have?

I'm debugging this using windows, so I assume it is whatever is packaged with the mozilla build environment installer. Running "python" gives: > Python 2.7.8 (default, Jun 30 2014, 16:03:49) [MSC v.1500 32 bit (Intel)] on win32

(In reply to Léon McGregor from comment #13) > I'm debugging this using windows, so I assume it is whatever is packaged > with the mozilla build environment installer. Running "python" gives: > > Python 2.7.8 (default, Jun 30 2014, 16:03:49) [MSC v.1500 32 bit (Intel)] on win32 Ah, okay, seems the same version as me, though I'm on a Mac. I would suggest asking in the #build channel on Mozilla IRC about this issue.

The tests are running now. There are a couple that failed. Should I re-write the failing parts of the tests so they work or try a different solution to the patch?

If the tests don't make sense anymore, they should me modified. Otherwise, the approach should be different in order to make them pass.

I've changed the patch to now use the argument that wasn't used before, and the tests all pass fine. This also refactors some repeated code into a new method. I didn't apply the suggested change to the _doFileSearch function, because in my mind you would only use it once and then it would take you to the file you searched for. ( Currently, the debugger doesn't retain this type of search once you make it, so I didn't think it made sense to change anything. )

Comment on attachment 8582576 [details] [diff] [review] bug908956-passExistingTextAsArgumentOnSearch.diff Review of attachment 8582576 [details] [diff] [review]: ----------------------------------------------------------------- Nice and clean! Thanks a bunch! ::: browser/devtools/debugger/debugger-toolbar.js @@ +1129,5 @@ > } > } > this._searchbox.value = aOperator; > }, > + Nit: whitespace at the end of the line here.

Could be worth adding a new test covering this specific use case.

I've never written a test for the mozilla codebase before. Do you have any tips on how to go about this?

I've added tests that should cover all of the cases relating to this.

Comment on attachment 8586162 [details] [diff] [review] bug908956-passExistingTextAsArgumentOnSearch.diff Review of attachment 8586162 [details] [diff] [review]: ----------------------------------------------------------------- Awesome!

This patch is ready - right? Bug wasn't touched since March it seems. Did it land? If not, why not?

Shouldn've been landed by now. Hopefully this doesn't need rebasing.

Please run this through Try first.