Last Comment Bug 741328 - Add a search input to easily/incrementally find scripts => with live buffer switching
: Add a search input to easily/incrementally find scripts => with live buffer s...
Status: RESOLVED FIXED
[chrome-debug]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: 12 Branch
: All All
: P3 normal (vote)
: Firefox 15
Assigned To: Victor Porof [:vporof][:vp]
:
: James Long (:jlongster)
Mentors:
Depends on: 741325
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-02 03:05 PDT by Victor Porof [:vporof][:vp]
Modified: 2012-05-04 07:27 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (3.10 KB, patch)
2012-04-13 07:51 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v2 (4.36 KB, patch)
2012-04-13 09:04 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v2.1 (4.98 KB, patch)
2012-04-14 11:50 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v3 (15.43 KB, patch)
2012-04-15 05:31 PDT, Victor Porof [:vporof][:vp]
rcampbell: review+
Details | Diff | Splinter Review
v3.1 (16.11 KB, patch)
2012-04-17 04:52 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v3.1.1 (16.11 KB, patch)
2012-04-17 11:18 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v3.1.2 (18.29 KB, patch)
2012-04-19 04:58 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review

Description Victor Porof [:vporof][:vp] 2012-04-02 03:05:49 PDT
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] 2012-04-13 07:51:44 PDT
Created attachment 614781 [details] [diff] [review]
v1

Works. Needs more features.
Comment 2 Victor Porof [:vporof][:vp] 2012-04-13 09:04:42 PDT
Created attachment 614817 [details] [diff] [review]
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] 2012-04-14 11:50:17 PDT
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.
Comment 4 Victor Porof [:vporof][:vp] 2012-04-15 05:31:22 PDT
Created attachment 615139 [details] [diff] [review]
v3

Has a test!
Comment 5 Victor Porof [:vporof][:vp] 2012-04-15 08:24:06 PDT
(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) 2012-04-16 03:21:34 PDT
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: gavin@gavinsharp.com] 2012-04-16 11:20:48 PDT
Would it make sense to use a type="search" textbox here?
Comment 8 Panos Astithas [:past] 2012-04-17 03:50:45 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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 [:past] 2012-04-17 03:55:23 PDT
(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] 2012-04-17 04:44:28 PDT
(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] 2012-04-17 04:44:42 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #7)
> Would it make sense to use a type="search" textbox here?

Yup.
Comment 12 Victor Porof [:vporof][:vp] 2012-04-17 04:52:13 PDT
Created attachment 615669 [details] [diff] [review]
v3.1

Addressed comments. Type=search.
Comment 13 Victor Porof [:vporof][:vp] 2012-04-17 05:15:10 PDT
Maybe we could also use a Ctrl/Cmd+P that focuses the search? (+prevent default)
:)
Comment 14 Rob Campbell [:rc] (:robcee) 2012-04-17 09:59:07 PDT
(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) 2012-04-17 10:02:39 PDT
(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] 2012-04-17 11:06:24 PDT
(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] 2012-04-17 11:18:58 PDT
Created attachment 615800 [details] [diff] [review]
v3.1.1

Making sure this is the latest patch.
Comment 18 Victor Porof [:vporof][:vp] 2012-04-19 04:58:21 PDT
Created attachment 616519 [details] [diff] [review]
v3.1.2

Separated events to keyup and input.
https://tbpl.mozilla.org/?tree=Try&rev=a53e2ccea347
Comment 19 Rob Campbell [:rc] (:robcee) 2012-04-19 08:03:09 PDT
(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) 2012-04-23 09:04:43 PDT
https://hg.mozilla.org/integration/fx-team/rev/d636e92961fa
Comment 21 Rob Campbell [:rc] (:robcee) 2012-04-23 15:27:39 PDT
https://hg.mozilla.org/integration/fx-team/rev/f849003d9864
Comment 22 Rob Campbell [:rc] (:robcee) 2012-04-25 08:23:11 PDT
https://hg.mozilla.org/integration/fx-team/rev/c7523bf07e12
Comment 23 Tim Taubert [:ttaubert] 2012-04-27 05:49:49 PDT
https://hg.mozilla.org/mozilla-central/rev/c7523bf07e12
Comment 24 :Ehsan Akhgari 2012-05-02 13:25:09 PDT
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) 2012-05-03 05:55:58 PDT
https://hg.mozilla.org/integration/fx-team/rev/5642725c88a3
Comment 26 Tim Taubert [:ttaubert] 2012-05-04 07:27:57 PDT
https://hg.mozilla.org/mozilla-central/rev/5642725c88a3

Note You need to log in before you can comment on or make changes to this bug.