Adding regex support to search bar in dev tools console

Adding regex support to search bar in dev tools console

This patch extends dev tools console search with support for regular 
expressions. UI wise I added a checkbox labeled "Regex" after the console 
search input field, which toggles regex support when searching and also 
triggers the search. When regex is enabled, console search translates the query
string to a standard JavaScript RegExp.

I implemented the regex checkbox by adding a boolean parameter to
WebInspector.SearchableView that determines whether the view should feature 
such a checkbox. I'm not 100% sure if this was the best way of doing this, 
since it's only relevant to the ConsoleView ATM, but it was the simplest way.

I'm concerned about two empty table cells between the search input field
and the Regex checkbox, but in fairness, these were already present prior to 
my changes.

Finally, I'm not sure how to write tests for my changes, even though I'd like to.

BUG=418406

[eustas, 2014-10-06 08:16:06]

eustas@chromium.org changed reviewers:
+ eustas@chromium.org

[eustas, 2014-10-06 08:16:07]

https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/co...
File Source/devtools/front_end/components/SearchableView.js (right):

https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/components/SearchableView.js:36: * @param {!boolean}
supportRegex
If you make this parameter optional, then you can remove changes in all
panels/views except console.

Like this:
@param {?boolean=} supportRegex

https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/components/SearchableView.js:40: var self = this;
We usually don't do that way.

https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/components/SearchableView.js:127:
self._regexCheckboxElement.addEventListener("change", function (event) {
Don't inline function and bind it to this.

[aknudsen, 2014-10-06 08:18:41]

On 2014/10/06 08:16:07, eustas wrote:
>
https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/co...
> File Source/devtools/front_end/components/SearchableView.js (right):
> 
>
https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/co...
> Source/devtools/front_end/components/SearchableView.js:36: * @param {!boolean}
> supportRegex
> If you make this parameter optional, then you can remove changes in all
> panels/views except console.
> 
> Like this:
> @param {?boolean=} supportRegex
Ah, thank you.
  
>
https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/co...
> Source/devtools/front_end/components/SearchableView.js:127:
> self._regexCheckboxElement.addEventListener("change", function (event) {
> Don't inline function and bind it to this.
How do I bind the function to 'this'?

[vsevik, 2014-10-06 08:26:26]

vsevik@chromium.org changed reviewers:
+ vsevik@chromium.org

[robwu, 2014-10-06 08:30:20]

rob@robwu.nl changed reviewers:
+ rob@robwu.nl

[robwu, 2014-10-06 08:30:22]

https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/co...
File Source/devtools/front_end/components/SearchableView.js (right):

https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/components/SearchableView.js:36: * @param {!boolean}
supportRegex
After looking over the rest, I'm not sure if this parameter belongs here. I
think that you should default to plain text, and save the regex preference when
the user changes the checkbox value. When the view is rendered again, you use
the saved preference to render the correct checkbox.

https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/components/SearchableView.js:40: var self = this;
Remove this and use "this" instead of "self".

https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/components/SearchableView.js:130: }, false);
Move the contents of this function to a separate member of this object with an
apprioiate underscore-prefixed name, and use .addEventListener("change",
this._someName.bind(this), false); to bind the "this" context (this eliminates
the need for "var self = this;").

https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/components/SearchableView.js:134:
self._regexElement.setAttribute("for", regexCheckboxId);
Change to

this._regexLabelElement.setAttribute("for", regexCheckboxId);

https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/co...
File Source/devtools/front_end/console/ConsoleView.js (right):

https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/console/ConsoleView.js:42: this._searchableView = new
WebInspector.SearchableView(this, true);
Why did you use true as the default setting? Plaintext search should be used by
default. See my earlier comment, I think that you should remove this new
parameter.

https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/console/ConsoleView.js:919: else {
} else { must be on a single line.

https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/console/ConsoleView.js:920: this._searchRegex = new
RegExp(query, "gi");
Use a separate function or method to construct the regex, and make sure that you
properly handle invalid regular expressions. Currently, when you pass in an
invalid regex, a SyntaxError will be thrown.

The devtools has already implemented regex filtering for console messages. You
could locate this code and see whether it can be re-used for this purpose.

[aknudsen, 2014-10-06 08:38:23]

On 2014/10/06 08:30:22, robwu wrote:
>
https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/co...
> File Source/devtools/front_end/components/SearchableView.js (right):
> 
>
https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/co...
> Source/devtools/front_end/components/SearchableView.js:36: * @param {!boolean}
> supportRegex
> After looking over the rest, I'm not sure if this parameter belongs here. I
> think that you should default to plain text, and save the regex preference
when
> the user changes the checkbox value. When the view is rendered again, you use
> the saved preference to render the correct checkbox.
This parameter doesn't say if regex search should be enabled, but whether the
view should have facilities for this at all (i.e. the checkbox).
  
>
https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/co...
> Source/devtools/front_end/components/SearchableView.js:130: }, false);
> Move the contents of this function to a separate member of this object with an
> apprioiate underscore-prefixed name, and use .addEventListener("change",
> this._someName.bind(this), false); to bind the "this" context (this eliminates
> the need for "var self = this;").
Thanks.

>
https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/co...
> Source/devtools/front_end/components/SearchableView.js:134:
> self._regexElement.setAttribute("for", regexCheckboxId);
> Change to
> 
> this._regexLabelElement.setAttribute("for", regexCheckboxId);
Oops, thanks.
 
>
https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/co...
> File Source/devtools/front_end/console/ConsoleView.js (right):
> 
>
https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/co...
> Source/devtools/front_end/console/ConsoleView.js:42: this._searchableView =
new
> WebInspector.SearchableView(this, true);
> Why did you use true as the default setting? Plaintext search should be used
by
> default. See my earlier comment, I think that you should remove this new
> parameter.
The true parameter only says to include regex search facilities in this view.
Plaintext is in fact default.
 
>
https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/co...
> Source/devtools/front_end/console/ConsoleView.js:919: else {
> } else { must be on a single line.
Noted.

>
https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/co...
> Source/devtools/front_end/console/ConsoleView.js:920: this._searchRegex = new
> RegExp(query, "gi");
> Use a separate function or method to construct the regex, and make sure that
you
> properly handle invalid regular expressions. Currently, when you pass in an
> invalid regex, a SyntaxError will be thrown.
OK, thanks.
 
> The devtools has already implemented regex filtering for console messages. You
> could locate this code and see whether it can be re-used for this purpose.
Thanks, will do.

[aknudsen, 2014-10-07 19:39:57]

I've uploaded a revised patch, please review. I also added a CSS file
corresponding to SearchableView, in order to be able to show that a regex is
invalid.

https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/co...
File Source/devtools/front_end/components/SearchableView.js (right):

https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/components/SearchableView.js:36: * @param {!boolean}
supportRegex
On 2014/10/06 08:16:07, eustas wrote:
> If you make this parameter optional, then you can remove changes in all
> panels/views except console.
> 
> Like this:
> @param {?boolean=} supportRegex

Done.

https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/components/SearchableView.js:40: var self = this;
On 2014/10/06 08:16:07, eustas wrote:
> We usually don't do that way.

Done.

https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/components/SearchableView.js:40: var self = this;
On 2014/10/06 08:30:21, robwu wrote:
> Remove this and use "this" instead of "self".

Done.

https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/components/SearchableView.js:127:
self._regexCheckboxElement.addEventListener("change", function (event) {
On 2014/10/06 08:16:07, eustas wrote:
> Don't inline function and bind it to this.

Done.

https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/components/SearchableView.js:130: }, false);
On 2014/10/06 08:30:21, robwu wrote:
> Move the contents of this function to a separate member of this object with an
> apprioiate underscore-prefixed name, and use .addEventListener("change",
> this._someName.bind(this), false); to bind the "this" context (this eliminates
> the need for "var self = this;").
> 

Done.

https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/components/SearchableView.js:134:
self._regexElement.setAttribute("for", regexCheckboxId);
On 2014/10/06 08:30:21, robwu wrote:
> Change to
> 
> this._regexLabelElement.setAttribute("for", regexCheckboxId);

Done.

https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/co...
File Source/devtools/front_end/console/ConsoleView.js (right):

https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/console/ConsoleView.js:919: else {
On 2014/10/06 08:30:21, robwu wrote:
> } else { must be on a single line.

Done.

https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/console/ConsoleView.js:920: this._searchRegex = new
RegExp(query, "gi");
On 2014/10/06 08:30:22, robwu wrote:
> Use a separate function or method to construct the regex, and make sure that
you
> properly handle invalid regular expressions. Currently, when you pass in an
> invalid regex, a SyntaxError will be thrown.
> 
> The devtools has already implemented regex filtering for console messages. You
> could locate this code and see whether it can be re-used for this purpose.

Acknowledged.

[aknudsen, 2014-10-07 20:50:12]

I've refactored regex creation into a separate method, which detects if it's
invalid.

https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/co...
File Source/devtools/front_end/console/ConsoleView.js (right):

https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/console/ConsoleView.js:920: this._searchRegex = new
RegExp(query, "gi");
On 2014/10/06 08:30:22, robwu wrote:
> Use a separate function or method to construct the regex, and make sure that
you
> properly handle invalid regular expressions. Currently, when you pass in an
> invalid regex, a SyntaxError will be thrown.
> 
> The devtools has already implemented regex filtering for console messages. You
> could locate this code and see whether it can be re-used for this purpose.

Done.

[aknudsen, 2014-10-07 21:07:52]

I am seeing a couple of problems, which I don't understand enough to rectify.

1. Searching for the regex '.' will mark whole lines as matching.
2. Searching for the regex '.*' makes the Chrome tab unresponsive and CPU usage
hit 100%.

[robwu, 2014-10-07 22:42:15]

On 2014/10/07 21:07:52, aknudsen wrote:
> I am seeing a couple of problems, which I don't understand enough to rectify.
> 
> 1. Searching for the regex '.' will mark whole lines as matching.

A dot (.) in JavaScript regular expressions matches any non-newline character.
Your regex has the global g flag, so all non-newline characters - basically all
lines - are matched.

> 2. Searching for the regex '.*' makes the Chrome tab unresponsive and CPU
usage
> hit 100%.

Weird. What's your test case? To debug the issue, you could try to set
breakpoints in the developer tools to debug the developer tools. Visit
chrome://inspect/#other to open the devtools for your devtools instance.

[aknudsen, 2014-10-08 06:47:11]

On 2014/10/07 22:42:15, robwu wrote:
> On 2014/10/07 21:07:52, aknudsen wrote:
> > I am seeing a couple of problems, which I don't understand enough to
rectify.
> > 
> > 1. Searching for the regex '.' will mark whole lines as matching.
> 
> A dot (.) in JavaScript regular expressions matches any non-newline character.
> Your regex has the global g flag, so all non-newline characters - basically
all
> lines - are matched.
OK, will have to rethink the global flag.
 
> > 2. Searching for the regex '.*' makes the Chrome tab unresponsive and CPU
> usage
> > hit 100%.
> 
> Weird. What's your test case? To debug the issue, you could try to set
> breakpoints in the developer tools to debug the developer tools. Visit
> chrome://inspect/#other to open the devtools for your devtools instance.
All I did was to evaluate an expression in the console (type text + Enter), then
search for '.*' with regex enabled. Is there some framework in place for (debug)
logging in the DevTools btw? I always find that immensely useful while
debugging.

[aknudsen, 2014-10-12 21:12:14]

In my latest patch set (#4), I've reimplemented regex search in order to make it
work the way it does in editors like Sublime. I think it works pretty well,
although there are probably a few bugs lurking in there. However, I'm very
interested in early feedback from the rest of you, on how you think this works
especially as it diverges strongly from how console search currently works.

The main difference lies in that regex search now detects matching text
individually (independent of containing messages), and potentially across
multiple lines, whereas plain text search just finds whole messages that contain
matching text. The plain text search model didn't fit regex search very well I
thought, and the behaviour I chose corresponds to e.g. Sublime AFAICT.

I also introduced a CSS class 'current-search-result' in order to visualize the
current search result, since we now distinguish between just search results and
the *current* search result. However, I strongly urge that my current design be
revised by a designer as I merely came up with something I could work with! :)

There is one bug that I am currently aware of: The current (regex) search isn't
updated when a message is appended to the console.

[aknudsen, 2014-10-12 22:00:05]

Btw, the closure compiler generates a number of warnings for patch set #4, but
I'm not sure how to best address them. Hoping you guys can provide some guidance
in that regard.

[vsevik, 2014-10-20 08:49:33]

I don't think we should add yet another checkbox to UI.

In fact we have a different view on the way this UI should look like:
https://codereview.chromium.org/658403002/

This CL is not landed yet, but when it does land the console regex search
support should be based on the same machinery.

Also I don't think we could allow our console search implementation to be this
complex. We should rather sacrifice multiline search.

[aknudsen, 2014-10-21 08:25:31]

On 2014/10/20 08:49:33, vsevik wrote:
> I don't think we should add yet another checkbox to UI.
> 
> In fact we have a different view on the way this UI should look like:
> https://codereview.chromium.org/658403002/
> 
> This CL is not landed yet, but when it does land the console regex search
> support should be based on the same machinery.
Aha exciting news, I'll have a look at it.

> Also I don't think we could allow our console search implementation to be this
> complex. We should rather sacrifice multiline search.
How do you envision that regex search behave in practice in the console? Why is
this level of complexity a problem in practice, do you have performance
considerations in mind? I thought the current search behaviour was rather weird
when I first understood it properly, I find typical text editor style search
more natural, i.e. navigate from match to match, not from line to line.

It'd be a real shame to lose multiline search as I find it really useful
especially to dig through logs, leading to my original motivation for this
patch. Could it eventually be made available through an extension?

[vsevik, 2014-10-21 09:11:09]

> I find typical text editor style search
> more natural, i.e. navigate from match to match, not from line to line.
I agree, I didn't realized we only have message to message search, not match to
match.
I think, however, we'd better split this patch into two - one adding regex
support and another one switching to match-to-match search.
The first one should be pretty straight forward now, while the second one will
require careful review.

Here is a link to the only test for console search that we currently have

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
You could create another one based on it. The way inspector tests work is that
the test() function is run in the devtools context and InspectorTest object
could be used there for various test harness utilities.
Feel free to ask me (vsevik@chromium.org) or lushnikov@chromium.org if you have
any questions while working on this.

[aknudsen, 2014-10-22 07:31:43]

On 2014/10/21 09:11:09, vsevik wrote:
> > I find typical text editor style search
> > more natural, i.e. navigate from match to match, not from line to line.
> I agree, I didn't realized we only have message to message search, not match
to
> match.
> I think, however, we'd better split this patch into two - one adding regex
> support and another one switching to match-to-match search.
> The first one should be pretty straight forward now, while the second one will
> require careful review.
OK, so what you're saying is I should write a patch to base console regex search
on https://codereview.chromium.org/658403002/? And then write a second patch to
introduce match-by-match search?

> Here is a link to the only test for console search that we currently have
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> You could create another one based on it. The way inspector tests work is that
> the test() function is run in the devtools context and InspectorTest object
> could be used there for various test harness utilities.
OK thanks, I'll keep this in mind.

[vsevik, 2014-10-22 08:03:31]

> OK, so what you're saying is I should write a patch to base console regex
search
> on https://codereview.chromium.org/658403002/ And then write a second patch to
> introduce match-by-match search?
Yes, or in reverse order if you wish so :)

[eustas, 2014-10-27 08:08:47]

eustas@chromium.org changed reviewers:
- eustas@chromium.org