Last Comment Bug 774788 - free text search across all scripts
: free text search across all scripts
Status: RESOLVED FIXED
[fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: unspecified
: All All
: P2 normal (vote)
: Firefox 17
Assigned To: Victor Porof [:vporof][:vp]
:
Mentors:
Depends on: 780631 781102 783393 786156 793444
Blocks: 766054
  Show dependency treegraph
 
Reported: 2012-07-17 11:44 PDT by Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
Modified: 2012-09-22 14:17 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (12.51 KB, patch)
2012-08-08 01:30 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v2 (21.68 KB, patch)
2012-08-08 08:27 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
looks like this (250.76 KB, image/png)
2012-08-08 09:07 PDT, Victor Porof [:vporof][:vp]
no flags Details
v2.1 (21.70 KB, patch)
2012-08-08 10:12 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v2.2 (29.67 KB, patch)
2012-08-08 17:20 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v3 (34.79 KB, patch)
2012-08-09 06:04 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
ui (329.26 KB, image/png)
2012-08-09 06:05 PDT, Victor Porof [:vporof][:vp]
no flags Details
v4 (37.78 KB, patch)
2012-08-09 09:52 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v4.1 (40.22 KB, patch)
2012-08-10 04:32 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v4.2 (41.76 KB, patch)
2012-08-10 09:01 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v5 (94.21 KB, patch)
2012-08-17 07:28 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v5.1 (98.35 KB, patch)
2012-08-17 12:29 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
5.2 (98.84 KB, patch)
2012-08-17 14:08 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v5.3 (99.00 KB, patch)
2012-08-23 01:10 PDT, Victor Porof [:vporof][:vp]
rcampbell: review+
Details | Diff | Splinter Review
v5.4 (99.00 KB, patch)
2012-08-24 13:10 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review

Description Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-07-17 11:44:28 PDT
It's possible to search within the currently selected file with a magical operator, but I have no idea which file I need to scope my search due to files being combined with nondescript filenames.

IRC discussion leaned towards a new operator, but the operator is really undiscoverable and we can do better. I think we could maybe surface this by having a dropdown radio menu off the magnifier to specify which search type, which would also get reflected in the placeholder text.

* filename
* search current file
* search all files
Comment 1 Victor Porof [:vporof][:vp] 2012-07-30 23:24:27 PDT
We could start with the operator first and add a dropdown menu for all the search methods in a followup. Does that sound ok?
Comment 2 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-07-31 11:42:58 PDT
Sounds fine to me, just wanted to get all my thoughts out so they didn't get lost. Thanks Victor!
Comment 3 Victor Porof [:vporof][:vp] 2012-08-01 23:46:17 PDT
Filed bug 779732.
Comment 4 Victor Porof [:vporof][:vp] 2012-08-08 01:30:43 PDT
Created attachment 649992 [details] [diff] [review]
v1

WIP.
Comment 5 Victor Porof [:vporof][:vp] 2012-08-08 08:27:49 PDT
Created attachment 650134 [details] [diff] [review]
v2

Basically works. Needs more functionality. I like it.
Comment 6 Victor Porof [:vporof][:vp] 2012-08-08 09:07:58 PDT
Created attachment 650159 [details]
looks like this
Comment 7 Victor Porof [:vporof][:vp] 2012-08-08 10:12:08 PDT
Created attachment 650197 [details] [diff] [review]
v2.1

Rebased, fixed failing tests, monospace all the things.
Comment 8 Victor Porof [:vporof][:vp] 2012-08-08 17:20:27 PDT
Created attachment 650382 [details] [diff] [review]
v2.2

Much, much faster.
Comment 9 Victor Porof [:vporof][:vp] 2012-08-09 06:04:45 PDT
Created attachment 650520 [details] [diff] [review]
v3

Created a search job pool to speed things up even more.

Clicking on matches jumps the editor to the appropriate file/line and adds a selection around the match.

Pressing ENTER will automatically jump to the first/next match.

Bounce animations!

Speed-wise, this is as fast as I can get it to be.
I also think this is ready UX/UI-wise. Need to write a test and we're done.

Rob, what do you think?
Comment 10 Victor Porof [:vporof][:vp] 2012-08-09 06:05:20 PDT
Created attachment 650521 [details]
ui
Comment 11 Victor Porof [:vporof][:vp] 2012-08-09 07:31:21 PDT
Heh. Chrome does a trick thing when searching: it only shows matches for a single script, and collapses all the other stuff.

Searching is pretty fast with the current implementation in this patch, however creating the UI takes quite a while and it's the only bottleneck. If I'd also automatically collapse everything except the first results (like Chrome does), then the performance improvement would be quite significant!

I'll give this a shot.
Comment 12 Rob Campbell [:rc] (:robcee) 2012-08-09 07:51:33 PDT
some feedback:

Maybe hold off on search and display until the user's stopped typing. (100ms should be enough).

Search results don't scroll when hitting Enter to go to next search result.

NICE!
Comment 13 Victor Porof [:vporof][:vp] 2012-08-09 09:52:20 PDT
Created attachment 650594 [details] [diff] [review]
v4

Optimized the optimizations yo! Addressed rob's comments regarding delayed search.
Scroll into view needs work.
Comment 14 Victor Porof [:vporof][:vp] 2012-08-10 04:32:04 PDT
Created attachment 650849 [details] [diff] [review]
v4.1

Fixed text failures. Cleaned up. Polished. Shaved.
Rob, is there any beachballing going on?
Comment 15 Victor Porof [:vporof][:vp] 2012-08-10 09:01:53 PDT
Created attachment 650900 [details] [diff] [review]
v4.2

Search results now scroll when hitting Enter to go to next search result.

Also, scrolling now automatically expands any collapsed search results that are brought into view.

Writing tests for all this stuff is going to be FUN.

FUN.
Comment 16 Rob Campbell [:rc] (:robcee) 2012-08-10 09:30:11 PDT
beautiful. I do think we should expand the results by default rather than force a user to click them.

*time passes*

... victor replies with a solution to auto-expand search results when scrolled into view, thereby saving some milliseconds on initial results population...
Comment 17 Victor Porof [:vporof][:vp] 2012-08-17 07:28:18 PDT
Created attachment 652760 [details] [diff] [review]
v5

This is it.
I think I may add another test just to make sure the coverage is super high, but this is definitely reviewable.
Comment 18 Victor Porof [:vporof][:vp] 2012-08-17 12:29:47 PDT
Created attachment 652871 [details] [diff] [review]
v5.1

Moar tests.
Comment 19 Victor Porof [:vporof][:vp] 2012-08-17 14:08:21 PDT
Created attachment 652911 [details] [diff] [review]
5.2

Minor changes, reworded some weirdly named events, fixed a potential race condition between clearing the global search view and the scripts cache in the frontend. Try looks green so far.
Comment 20 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-08-17 23:14:00 PDT
<3

There's more where that came from when some UI comes together :)
Comment 21 Victor Porof [:vporof][:vp] 2012-08-18 01:52:34 PDT
Comment on attachment 650521 [details]
ui

Ping shorlander for feedback.
Video of the thing in action: https://dl.dropbox.com/u/2388316/global-search.webm
Comment 22 Victor Porof [:vporof][:vp] 2012-08-18 01:52:49 PDT
Green on try: https://tbpl.mozilla.org/?tree=Try&rev=0f43a3b906f7
Comment 23 Rob Campbell [:rc] (:robcee) 2012-08-22 14:30:46 PDT
_onScriptsCleared is gone from debugger-controller.js after bug 783393. Do we need to do our cleanup elsewhere now?
Comment 24 Victor Porof [:vporof][:vp] 2012-08-23 01:04:50 PDT
(In reply to Rob Campbell [:rc] (:robcee) from comment #23)
> _onScriptsCleared is gone from debugger-controller.js after bug 783393. Do
> we need to do our cleanup elsewhere now?

No it's not... It's called manually in handleTabNavigation. It has been called manually since forever actually, since the scriptscleared event was never implemented in the prototcol. Development is fun.
Comment 25 Victor Porof [:vporof][:vp] 2012-08-23 01:10:46 PDT
Created attachment 654543 [details] [diff] [review]
v5.3

Qrefed a failing hunk and rebased on top of THE BUG.
Comment 26 Victor Porof [:vporof][:vp] 2012-08-23 05:19:47 PDT
Try: https://tbpl.mozilla.org/?tree=Try&rev=b818c53d3af8
Comment 27 Rob Campbell [:rc] (:robcee) 2012-08-24 12:48:00 PDT
Comment on attachment 654543 [details] [diff] [review]
v5.3

Review of attachment 654543 [details] [diff] [review]:
-----------------------------------------------------------------

alright, not a lot to comment on with this. Works well, looks decent.

::: browser/devtools/debugger/debugger-controller.js
@@ +1191,4 @@
>     *        Additional options for showing the script. Supported options:
>     *        - targetLine: place the editor at the given line number.
>     */
> +  showScript: function SS_showScript(aScript, aOptions = {}) {

You made me look this up. Since Firefox 15?

rewrite everything.

::: browser/devtools/debugger/debugger-view.js
@@ +167,5 @@
> +  this._onFetchScriptsFinished = this._onFetchScriptsFinished.bind(this);
> +  this._onLineClick = this._onLineClick.bind(this);
> +  this._onMatchClick = this._onMatchClick.bind(this);
> +  this._onResultsScroll = this._onResultsScroll.bind(this);
> +  this._startSerach = this._startSerach.bind(this);

TYYYPPPOOOO!!!!

@@ +258,5 @@
> +
> +  /**
> +   * Starts searching for a token in all the scripts.
> +   */
> +  _startSerach: function DVGS__startSearch() {

typo. (oh no, he's doing it again)

@@ +668,5 @@
> +  /**
> +   * Scrolls a match into view.
> +   * @param nsIDOMElement aTarget
> +   */
> +  _scrollMatchIntoViewIfNeeded: function DVGS__scrollMatchIntoViewIfNeeded(aTarget) {

did you try using the version of this in shared/LayoutHelpers.jsm?

This is simpler, and I do like that, but we should reuse code where possible.
Comment 28 Victor Porof [:vporof][:vp] 2012-08-24 13:07:47 PDT
(In reply to Rob Campbell [:rc] (:robcee) from comment #27)
> Comment on attachment 654543 [details] [diff] [review]
> v5.3
> 
> Review of attachment 654543 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> alright, not a lot to comment on with this. Works well, looks decent.
> 
> ::: browser/devtools/debugger/debugger-controller.js
> @@ +1191,4 @@
> >     *        Additional options for showing the script. Supported options:
> >     *        - targetLine: place the editor at the given line number.
> >     */
> > +  showScript: function SS_showScript(aScript, aOptions = {}) {
> 
> You made me look this up. Since Firefox 15?
> 
> rewrite everything.

Can I start with Tilt?

> 
> ::: browser/devtools/debugger/debugger-view.js
> @@ +167,5 @@
> > +  this._onFetchScriptsFinished = this._onFetchScriptsFinished.bind(this);
> > +  this._onLineClick = this._onLineClick.bind(this);
> > +  this._onMatchClick = this._onMatchClick.bind(this);
> > +  this._onResultsScroll = this._onResultsScroll.bind(this);
> > +  this._startSerach = this._startSerach.bind(this);
> 
> TYYYPPPOOOO!!!!

I'll make a Sublime plugin.

> 
> @@ +258,5 @@
> > +
> > +  /**
> > +   * Starts searching for a token in all the scripts.
> > +   */
> > +  _startSerach: function DVGS__startSearch() {
> 
> typo. (oh no, he's doing it again)
> 

:(

> @@ +668,5 @@
> > +  /**
> > +   * Scrolls a match into view.
> > +   * @param nsIDOMElement aTarget
> > +   */
> > +  _scrollMatchIntoViewIfNeeded: function DVGS__scrollMatchIntoViewIfNeeded(aTarget) {
> 
> did you try using the version of this in shared/LayoutHelpers.jsm?
> 

I tried, doesn't work at all.

> This is simpler, and I do like that, but we should reuse code where possible.

I like simple code.
Comment 29 Victor Porof [:vporof][:vp] 2012-08-24 13:10:17 PDT
Created attachment 655118 [details] [diff] [review]
v5.4

Fixed typo.
Comment 30 Victor Porof [:vporof][:vp] 2012-08-25 02:00:12 PDT
https://hg.mozilla.org/integration/fx-team/rev/11a0db7262a0
Comment 31 Dave Camp (:dcamp) 2012-08-25 17:05:52 PDT
https://hg.mozilla.org/mozilla-central/rev/11a0db7262a0

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