741328 - Add a search input to easily/incrementally find scripts => with live buffer switching

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

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

We also may need to make the script list fetching faster in order to enable live buffer switching, but this could be fixed by the patch in bug 728926.

Comment 1

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

Attachment: v1

Works. Needs more features.

Comment 2

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

Attachment: v2

Smarter. Needs a test.

You can search for
* :n (line number in the current file)
* regex in the script url
* regex in the menuitem label
* regex:n (same story + line number in the first found file)

Rob, how does if feel?

Comment 3

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

Attachment: v2.1

Now even smarter! But still needs a test.

Additional search criteria
* @token (search for something in the current script)
* regex@token (something in a certain file)

Automatically jumps the caret to the found token.

ENTER to find next (wraps around)
ESCAPE to focus to the editor

@ has precedence

For example, if you search "myfile:12@func", the line number will be ignored. If you search for "myfile@func:12" you get a search for "func:12" in the file.

I thought about doing per line token searches (make ":12@func" search for "func" on line 12), but that seems like something no one will ever use.

You don't need the full filename to be searched to be able to goto line or token automatically. 

You can have searches like "/myregex/:42" which will jump to the line 42 in the first found file that matches the regex. 
Similarly, "/myregex/@token" will search for a token in the first file that matches the regex.

I like it.

Comment 4

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

Attachment: v3

Has a test!

Comment 5

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

(In reply to Victor Porof from comment #4)
> Created attachment 615139 [details] [diff] [review]
> v3
> 
> Has a test!

https://tbpl.mozilla.org/?tree=Try&rev=e4a920b2e92f

Comment 6

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

Comment on attachment 615139 [details] [diff] [review]
v3

+   * The keyup listener for the scripts search box.
+   */
+  _onScriptsSearch: function DVS__onScriptsSearch(e) {
+    if (e.keyCode === e.DOM_VK_ESCAPE) {

"e" is presumably a @param event?

+  _onScriptsSearch: function DVS__onScriptsSearch(e) {
...
+    let fileEnd = lastColon != -1 ? lastColon : lastAt != -1 ? lastAt : rawLength;

That's getting a tad lengthy.

+
+    // Presume we won't find anything.
+    scripts.selectedItem = this._preferredScript;

such a pessimist.

+    if (!file) {
+      for (let i = 0, l = scripts.itemCount; i < l; i++) {
+        scripts.getItemAtIndex(i).hidden = false;
+      }

you could

Array.forEach(scripts, function(script) {
  script.hidden = false;
});

if you wanted to do away with the variables.

same in the else clause.

test/browser_dbg_script-switching.js

 function testSwitchRunning()
 {
+  dump("Debugger editor text:\n" + gDebugger.editor.getText() + "\n");
+

should remove that.

+    if (scriptShown && framesAdded) {
+      Services.tm.currentThread.dispatch({ run: testScriptSearching }, 0);
+    }

did you just sneakily include a setTimeout(0) on me?

+    ok(editor.getCaretPosition().line == 11 &&
+       editor.getCaretPosition().col == 0,
+      "The editor didn't jump to the correct line.");
+
+
+    write("@debugger");

extra newline.

yup.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Would it make sense to use a type="search" textbox here?

Comment 8

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

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #7)
> Would it make sense to use a type="search" textbox here?

I think so, too. As a matter of fact I had done so in remote-debug:

https://hg.mozilla.org/users/rcampbell_mozilla.com/remote-debug/rev/fc1d4a37a696#l2.22

shorlander's new mockups concur as well.

Comment 9

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

(In reply to Victor Porof from comment #3)
> Created attachment 615072 [details] [diff] [review]
> v2.1
> 
> Now even smarter! But still needs a test.
> 
> Additional search criteria
> * @token (search for something in the current script)
> * regex@token (something in a certain file)
> 
> Automatically jumps the caret to the found token.
> 
> ENTER to find next (wraps around)
> ESCAPE to focus to the editor
> 
> @ has precedence
> 
> For example, if you search "myfile:12@func", the line number will be
> ignored. If you search for "myfile@func:12" you get a search for "func:12"
> in the file.
> 
> I thought about doing per line token searches (make ":12@func" search for
> "func" on line 12), but that seems like something no one will ever use.
> 
> You don't need the full filename to be searched to be able to goto line or
> token automatically. 
> 
> You can have searches like "/myregex/:42" which will jump to the line 42 in
> the first found file that matches the regex. 
> Similarly, "/myregex/@token" will search for a token in the first file that
> matches the regex.
> 
> I like it.

I haven't played with it yet, but I think this is an awesome feature set that will make power users happy. Even if some of the features are not very discoverable or feel weird (mostly concerned about ENTER & ESC), I think it makes sense to ship first and iterate later.

Comment 10

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

(In reply to Rob Campbell [:rc] (:robcee) from comment #6)
> Comment on attachment 615139 [details] [diff] [review]
> v3
> 
> +   * The keyup listener for the scripts search box.
> +   */
> +  _onScriptsSearch: function DVS__onScriptsSearch(e) {
> +    if (e.keyCode === e.DOM_VK_ESCAPE) {
> 
> "e" is presumably a @param event?
> 

I've never seen events documented. It's pretty much self implied. But ok.

> +  _onScriptsSearch: function DVS__onScriptsSearch(e) {
> ...
> +    let fileEnd = lastColon != -1 ? lastColon : lastAt != -1 ? lastAt :
> rawLength;
> 
> That's getting a tad lengthy.
> 

Meh.

> +
> +    // Presume we won't find anything.
> +    scripts.selectedItem = this._preferredScript;
> 
> such a pessimist.
> 
> +    if (!file) {
> +      for (let i = 0, l = scripts.itemCount; i < l; i++) {
> +        scripts.getItemAtIndex(i).hidden = false;
> +      }
> 
> you could
> 
> Array.forEach(scripts, function(script) {
>   script.hidden = false;
> });
> 
> if you wanted to do away with the variables.
> 
> same in the else clause.
> 

Again, that won't work because what we're iterating on is a menulist, not an array-like object. It doesn't have a .length
Same for everything in bug 741325

> +    if (scriptShown && framesAdded) {
> +      Services.tm.currentThread.dispatch({ run: testScriptSearching }, 0);
> +    }
> 
> did you just sneakily include a setTimeout(0) on me?
> 

I'd never do that :)
It's in all of the test! I wouldn't want to be the one breaking the rules here, now, would I?

> +    ok(editor.getCaretPosition().line == 11 &&
> +       editor.getCaretPosition().col == 0,
> +      "The editor didn't jump to the correct line.");
> +
> +
> +    write("@debugger");
> 
> extra newline.
> 
> yup.

Comment 11

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

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #7)
> Would it make sense to use a type="search" textbox here?

Yup.

Comment 12

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

Attachment: v3.1

Addressed comments. Type=search.

Comment 13

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

Maybe we could also use a Ctrl/Cmd+P that focuses the search? (+prevent default)
:)

Comment 14

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

(In reply to Victor Porof from comment #13)
> Maybe we could also use a Ctrl/Cmd+P that focuses the search? (+prevent
> default)
> :)

that'd likely coincide with Print. I'm not sure what the policies are on overriding well known shortcuts in specific cases, but that seems a little obnoxious.

(see also: Ctrl/Cmd-T).

WTB: extra keys.

Comment 15

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

(In reply to Victor Porof from comment #10)
> (In reply to Rob Campbell [:rc] (:robcee) from comment #6)
> > Comment on attachment 615139 [details] [diff] [review]
> > v3
> > 
> > +   * The keyup listener for the scripts search box.
> > +   */
> > +  _onScriptsSearch: function DVS__onScriptsSearch(e) {
> > +    if (e.keyCode === e.DOM_VK_ESCAPE) {
> > 
> > "e" is presumably a @param event?
> 
> I've never seen events documented. It's pretty much self implied. But ok.

http://mxr.mozilla.org/mozilla-central/source/browser/devtools/highlighter/inspector.jsm#997

> > +  _onScriptsSearch: function DVS__onScriptsSearch(e) {
> > ...
> > +    let fileEnd = lastColon != -1 ? lastColon : lastAt != -1 ? lastAt :
> > rawLength;
> > 
> > That's getting a tad lengthy.
> 
> Meh.

max(80)!

> > +
> > +    // Presume we won't find anything.
> > +    scripts.selectedItem = this._preferredScript;
> > 
> > such a pessimist.
> > 
> > +    if (!file) {
> > +      for (let i = 0, l = scripts.itemCount; i < l; i++) {
> > +        scripts.getItemAtIndex(i).hidden = false;
> > +      }
> > 
> > you could
> > 
> > Array.forEach(scripts, function(script) {
> >   script.hidden = false;
> > });
> > 
> > if you wanted to do away with the variables.
> > 
> > same in the else clause.
> 
> Again, that won't work because what we're iterating on is a menulist, not an
> array-like object. It doesn't have a .length
> Same for everything in bug 741325

yeah, I missed that we were dealing with the actual script items in that bug.

Array.forEach(items, function) is a pretty common pattern for iterating DOM objects in a nicer way throughout the firefox codebase though. Totally works!

Comment 16

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

(In reply to Rob Campbell [:rc] (:robcee) from comment #15)
> 
> Array.forEach(items, function) is a pretty common pattern for iterating DOM
> objects in a nicer way throughout the firefox codebase though. Totally works!

I wonder why it failed when I tried it.. :(
*off topic*

Comment 17

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

Attachment: v3.1.1

Making sure this is the latest patch.

Comment 18

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

Attachment: v3.1.2

Separated events to keyup and input.
https://tbpl.mozilla.org/?tree=Try&rev=a53e2ccea347

Comment 19

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

(In reply to Victor Porof from comment #16)
> (In reply to Rob Campbell [:rc] (:robcee) from comment #15)
> > 
> > Array.forEach(items, function) is a pretty common pattern for iterating DOM
> > objects in a nicer way throughout the firefox codebase though. Totally works!
> 
> I wonder why it failed when I tried it.. :(
> *off topic*

not sure. And it really isn't that important anyway. I checked and it's not used as often as I thought. Mostly in tests.

Comment 20

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

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

Comment 21

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

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

Comment 22

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

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

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/c7523bf07e12

Comment 24

[(no longer active)]

This patch was in a range which caused a Ts regression, so I backed out the whole range:

https://hg.mozilla.org/mozilla-central/rev/24a6a53c714a

Please reland after investigating and fixing the regression.

Comment 25

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

https://hg.mozilla.org/integration/fx-team/rev/5642725c88a3

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/5642725c88a3