Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 632573002: Adding regex support to search bar in dev tools console (Closed)

Created:
3 years, 1 month ago by aknudsen
Modified:
2 years, 9 months ago
Reviewers:
vsevik, robwu
CC:
blink-reviews, caseq+blink_chromium.org, loislo+blink_chromium.org, eustas+blink_chromium.org, malch+blink_chromium.org, yurys+blink_chromium.org, lushnikov+blink_chromium.org, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, paulirish+reviews_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, sergeyv+blink_chromium.org, aandrey+blink_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/blink@master
Project:
blink
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 19

Patch Set 2 : Revision 1 #

Patch Set 3 : Clear regex error state when search input is cleared #

Patch Set 4 : Implement regex search similar to Sublime Text #

Unified diffs Side-by-side diffs Delta from patch set Stats (+247 lines, -32 lines) Patch
M Source/devtools/front_end/components/SearchableView.js View 1 2 3 6 chunks +56 lines, -12 lines 0 comments Download
M Source/devtools/front_end/console/ConsoleView.js View 1 2 3 5 chunks +158 lines, -16 lines 0 comments Download
M Source/devtools/front_end/console/ConsoleViewMessage.js View 1 2 3 1 chunk +18 lines, -0 lines 0 comments Download
M Source/devtools/front_end/inspectorCommon.css View 1 2 3 1 chunk +8 lines, -0 lines 0 comments Download
A + Source/devtools/front_end/searchable.css View 1 1 chunk +3 lines, -2 lines 0 comments Download
M Source/devtools/front_end/ui/UIUtils.js View 1 2 3 1 chunk +4 lines, -2 lines 0 comments Download
Trybot results:

Messages

Total messages: 20 (4 generated)
eustas
https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/components/SearchableView.js File Source/devtools/front_end/components/SearchableView.js (right): https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/components/SearchableView.js#newcode36 Source/devtools/front_end/components/SearchableView.js:36: * @param {!boolean} supportRegex If you make this parameter ...
3 years, 1 month ago (2014-10-06 08:16:07 UTC) #2
aknudsen
On 2014/10/06 08:16:07, eustas wrote: > https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/components/SearchableView.js > File Source/devtools/front_end/components/SearchableView.js (right): > > https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/components/SearchableView.js#newcode36 > ...
3 years, 1 month ago (2014-10-06 08:18:41 UTC) #3
robwu
https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/components/SearchableView.js File Source/devtools/front_end/components/SearchableView.js (right): https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/components/SearchableView.js#newcode36 Source/devtools/front_end/components/SearchableView.js:36: * @param {!boolean} supportRegex After looking over the rest, ...
3 years, 1 month ago (2014-10-06 08:30:22 UTC) #6
aknudsen
On 2014/10/06 08:30:22, robwu wrote: > https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/components/SearchableView.js > File Source/devtools/front_end/components/SearchableView.js (right): > > https://codereview.chromium.org/632573002/diff/1/Source/devtools/front_end/components/SearchableView.js#newcode36 > ...
3 years, 1 month ago (2014-10-06 08:38:23 UTC) #7
aknudsen
I've uploaded a revised patch, please review. I also added a CSS file corresponding to ...
3 years, 1 month ago (2014-10-07 19:39:57 UTC) #8
aknudsen
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/console/ConsoleView.js File ...
3 years, 1 month ago (2014-10-07 20:50:12 UTC) #9
aknudsen
I am seeing a couple of problems, which I don't understand enough to rectify. 1. ...
3 years, 1 month ago (2014-10-07 21:07:52 UTC) #10
robwu
On 2014/10/07 21:07:52, aknudsen wrote: > I am seeing a couple of problems, which I ...
3 years, 1 month ago (2014-10-07 22:42:15 UTC) #11
aknudsen
On 2014/10/07 22:42:15, robwu wrote: > On 2014/10/07 21:07:52, aknudsen wrote: > > I am ...
3 years, 1 month ago (2014-10-08 06:47:11 UTC) #12
aknudsen
In my latest patch set (#4), I've reimplemented regex search in order to make it ...
3 years, 1 month ago (2014-10-12 21:12:14 UTC) #13
aknudsen
Btw, the closure compiler generates a number of warnings for patch set #4, but I'm ...
3 years, 1 month ago (2014-10-12 22:00:05 UTC) #14
vsevik
I don't think we should add yet another checkbox to UI. In fact we have ...
3 years ago (2014-10-20 08:49:33 UTC) #15
aknudsen
On 2014/10/20 08:49:33, vsevik wrote: > I don't think we should add yet another checkbox ...
3 years ago (2014-10-21 08:25:31 UTC) #16
vsevik
> I find typical text editor style search > more natural, i.e. navigate from match ...
3 years ago (2014-10-21 09:11:09 UTC) #17
aknudsen
On 2014/10/21 09:11:09, vsevik wrote: > > I find typical text editor style search > ...
3 years ago (2014-10-22 07:31:43 UTC) #18
vsevik
3 years ago (2014-10-22 08:03:31 UTC) #19
> 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 :)

Powered by Google App Engine
This is Rietveld efc10ee0f