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

Issue 676193002: Navigate between individual search matches in DevTools console (Closed)

Created:
3 years ago by aknudsen
Modified:
2 years, 10 months ago
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
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Navigate between individual search matches in DevTools console Rather than the original DevTools console search behaviour, which is to navigate between lines containing search matches, highlight all matches and navigate between individual matches irrespective of containing lines. Please note that the .current-search-result CSS class should be revised by a designer. BUG=426366 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=188234

Patch Set 1 #

Total comments: 22

Patch Set 2 : Revision #1 #

Patch Set 3 : Add test; fix scrolling to current match #

Patch Set 4 : Mark current search results by modifying CSS classes #

Patch Set 5 : Scroll tree elements into view #

Patch Set 6 : Add tests #

Total comments: 102

Patch Set 7 : Highlight all matches per message at once #

Patch Set 8 : Rebase on origin/master #

Patch Set 9 : Consolidate test suites #

Total comments: 8

Patch Set 10 : Improve test #

Total comments: 7

Patch Set 11 : Rebase on origin/master #

Patch Set 12 : Call clearHighlights once per message #

Total comments: 2

Patch Set 13 : Remove comments #

Patch Set 14 : Synchronize with origin #

Messages

Total messages: 66 (11 generated)
aknudsen
3 years ago (2014-10-25 22:29:47 UTC) #2
robwu
I've reviewed your code and glanced over the logic, but I haven't looked at the ...
3 years ago (2014-10-25 23:13:00 UTC) #3
aknudsen
On 2014/10/25 23:13:00, robwu wrote: > I've reviewed your code and glanced over the logic, ...
3 years ago (2014-10-26 10:58:38 UTC) #4
aknudsen
https://codereview.chromium.org/676193002/diff/1/Source/devtools/front_end/console/ConsoleView.js File Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/676193002/diff/1/Source/devtools/front_end/console/ConsoleView.js#newcode579 Source/devtools/front_end/console/ConsoleView.js:579: if (this._searchRegex && this._searchMessage(this._visibleViewMessages.length-1)) { On 2014/10/25 23:12:59, robwu ...
3 years ago (2014-10-26 11:22:41 UTC) #7
aknudsen
On 2014/10/25 23:13:00, robwu wrote: > And these features surely need some test coverage. Would ...
3 years ago (2014-10-26 18:43:21 UTC) #8
robwu
On 2014/10/26 18:43:21, aknudsen wrote: > On 2014/10/25 23:13:00, robwu wrote: > > And these ...
3 years ago (2014-10-26 19:03:02 UTC) #9
aknudsen
On 2014/10/26 19:03:02, robwu wrote: > 1. Get Chromium, see > http://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/docs/html/depot_tools_tutorial.html Can I execute ...
3 years ago (2014-10-26 19:36:34 UTC) #10
robwu
On 2014/10/26 19:36:34, aknudsen wrote: > On 2014/10/26 19:03:02, robwu wrote: > > 1. Get ...
3 years ago (2014-10-26 20:05:56 UTC) #11
aknudsen
On 2014/10/26 20:05:56, robwu wrote: > On 2014/10/26 19:36:34, aknudsen wrote: > > On 2014/10/26 ...
3 years ago (2014-10-26 21:24:10 UTC) #12
robwu
On 2014/10/26 21:24:10, aknudsen wrote: > On 2014/10/26 20:05:56, robwu wrote: > > On 2014/10/26 ...
3 years ago (2014-10-26 21:30:19 UTC) #13
lushnikov
I've applied the patch and played with it. It's a great start! However, there are ...
3 years ago (2014-10-27 08:16:37 UTC) #15
lushnikov
Regarding the console-search-reveals-messages.html test, here's an explanation how things work https://gist.github.com/aslushnikov/f1bf449ebc955435c38a Hope this helps.
3 years ago (2014-10-27 09:14:40 UTC) #16
aknudsen
On 2014/10/26 19:03:02, robwu wrote: > On 2014/10/26 18:43:21, aknudsen wrote: > > Would you ...
3 years ago (2014-10-27 17:37:43 UTC) #17
aknudsen
On 2014/10/27 08:16:37, lushnikov wrote: > I've applied the patch and played with it. It's ...
3 years ago (2014-10-27 22:20:22 UTC) #18
lushnikov
On 2014/10/27 17:37:43, aknudsen wrote: > On 2014/10/26 19:03:02, robwu wrote: > > On 2014/10/26 ...
3 years ago (2014-10-28 04:43:55 UTC) #19
aknudsen
On 2014/10/28 04:43:55, lushnikov wrote: > On 2014/10/27 17:37:43, aknudsen wrote: > > On 2014/10/26 ...
3 years ago (2014-10-28 11:18:04 UTC) #20
aknudsen
On 2014/10/27 09:14:40, lushnikov wrote: > Regarding the console-search-reveals-messages.html test, here's an explanation > how ...
3 years ago (2014-10-29 22:08:22 UTC) #21
aknudsen
On 2014/10/27 08:16:37, lushnikov wrote: > I've applied the patch and played with it. It's ...
3 years ago (2014-10-29 22:09:41 UTC) #22
lushnikov
On 2014/10/29 22:09:41, aknudsen wrote: > On 2014/10/27 08:16:37, lushnikov wrote: > > I've applied ...
3 years ago (2014-10-30 06:09:35 UTC) #23
aknudsen
On 2014/10/30 06:09:35, lushnikov wrote: > On 2014/10/29 22:09:41, aknudsen wrote: > > On 2014/10/27 ...
3 years ago (2014-10-30 09:03:54 UTC) #24
lushnikov
On 2014/10/30 09:03:54, aknudsen wrote: > On 2014/10/30 06:09:35, lushnikov wrote: > > On 2014/10/29 ...
3 years ago (2014-10-30 09:20:53 UTC) #25
aknudsen
On 2014/10/30 09:20:53, lushnikov wrote: > On 2014/10/30 09:03:54, aknudsen wrote: > > On 2014/10/30 ...
3 years ago (2014-10-30 09:29:48 UTC) #26
lushnikov
I've applied your patch one more time. There's still a lag when i search for ...
3 years ago (2014-11-07 16:27:19 UTC) #27
apavlov
https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_end/console/ConsoleView.js File Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_end/console/ConsoleView.js#newcode1223 Source/devtools/front_end/console/ConsoleView.js:1223: * @return {!string} Non-object types should not have ! ...
3 years ago (2014-11-07 16:30:07 UTC) #29
aknudsen
https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_end/console/ConsoleView.js File Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_end/console/ConsoleView.js#newcode51 Source/devtools/front_end/console/ConsoleView.js:51: this._regexMatchRanges = []; On 2014/11/07 16:27:16, lushnikov wrote: > ...
3 years ago (2014-11-08 11:49:51 UTC) #30
aknudsen
https://codereview.chromium.org/676193002/diff/140001/LayoutTests/inspector/console/console-search-can-highlight-current-match.html File LayoutTests/inspector/console/console-search-can-highlight-current-match.html (right): https://codereview.chromium.org/676193002/diff/140001/LayoutTests/inspector/console/console-search-can-highlight-current-match.html#newcode22 LayoutTests/inspector/console/console-search-can-highlight-current-match.html:22: setTimeout(InspectorTest.completeTest.bind(InspectorTest), 2000); On 2014/11/07 16:27:16, lushnikov wrote: > We ...
3 years ago (2014-11-08 12:00:53 UTC) #31
aknudsen
https://codereview.chromium.org/676193002/diff/140001/LayoutTests/inspector/console/console-search-can-highlight-current-match.html File LayoutTests/inspector/console/console-search-can-highlight-current-match.html (right): https://codereview.chromium.org/676193002/diff/140001/LayoutTests/inspector/console/console-search-can-highlight-current-match.html#newcode22 LayoutTests/inspector/console/console-search-can-highlight-current-match.html:22: setTimeout(InspectorTest.completeTest.bind(InspectorTest), 2000); On 2014/11/07 16:27:16, lushnikov wrote: > We ...
3 years ago (2014-11-09 00:02:14 UTC) #32
aknudsen
https://codereview.chromium.org/676193002/diff/140001/LayoutTests/inspector/console/console-search-can-jump-backward-between-matches.html File LayoutTests/inspector/console/console-search-can-jump-backward-between-matches.html (right): https://codereview.chromium.org/676193002/diff/140001/LayoutTests/inspector/console/console-search-can-jump-backward-between-matches.html#newcode51 LayoutTests/inspector/console/console-search-can-jump-backward-between-matches.html:51: function testFindMatches(next) On 2014/11/07 16:27:16, lushnikov wrote: > Could ...
3 years ago (2014-11-09 00:09:46 UTC) #33
lushnikov
https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_end/console/ConsoleView.js File Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/676193002/diff/140001/Source/devtools/front_end/console/ConsoleView.js#newcode959 Source/devtools/front_end/console/ConsoleView.js:959: var message = this._visibleViewMessages[ On 2014/11/08 11:49:50, aknudsen wrote: ...
2 years, 12 months ago (2014-11-19 06:55:47 UTC) #34
lushnikov
> When considering merging tests, I thought about how they differ in setup logic. > ...
2 years, 12 months ago (2014-11-19 06:57:00 UTC) #35
aknudsen
On 2014/11/19 06:57:00, lushnikov wrote: > > When considering merging tests, I thought about how ...
2 years, 11 months ago (2014-11-24 08:07:44 UTC) #36
aknudsen
On 2014/11/24 08:07:44, aknudsen wrote: > On 2014/11/19 06:57:00, lushnikov wrote: > > > When ...
2 years, 11 months ago (2014-12-02 22:56:42 UTC) #37
lushnikov
Thank you for the effort! Just a few last nits before we can have this ...
2 years, 11 months ago (2014-12-04 16:07:34 UTC) #38
aknudsen
https://codereview.chromium.org/676193002/diff/200001/LayoutTests/inspector/console/console-search.html File LayoutTests/inspector/console/console-search.html (right): https://codereview.chromium.org/676193002/diff/200001/LayoutTests/inspector/console/console-search.html#newcode62 LayoutTests/inspector/console/console-search.html:62: // Find first match On 2014/12/04 16:07:34, lushnikov wrote: ...
2 years, 11 months ago (2014-12-05 22:30:02 UTC) #39
aknudsen
I haven't tested my latest changes, since the current Chromium head isn't buildable :( I ...
2 years, 11 months ago (2014-12-05 22:36:49 UTC) #40
robwu
On 2014/12/05 22:36:49, aknudsen wrote: > I haven't tested my latest changes, since the current ...
2 years, 11 months ago (2014-12-05 23:05:11 UTC) #41
aknudsen
On Sat, Dec 6, 2014 at 12:05 AM, <rob@robwu.nl> wrote: > On 2014/12/05 22:36:49, aknudsen ...
2 years, 11 months ago (2014-12-05 23:52:21 UTC) #42
aknudsen
Nah, still fails: ./../third_party/WebKit/Source/core/inspector/ScriptProfile.cpp:70:32: error: no member named 'GetHitLineCount' in 'v8::CpuProfileNode'; did you mean 'GetHitCount'? ...
2 years, 11 months ago (2014-12-06 00:21:46 UTC) #43
lushnikov
First of all, sorry for the long delay, and thank you for all the work ...
2 years, 11 months ago (2014-12-17 13:21:23 UTC) #44
aknudsen
https://codereview.chromium.org/676193002/diff/220001/Source/devtools/front_end/console/ConsoleView.js File Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/676193002/diff/220001/Source/devtools/front_end/console/ConsoleView.js#newcode1023 Source/devtools/front_end/console/ConsoleView.js:1023: var message = this._visibleViewMessages[matchRange.messageIndex]; On 2014/12/17 13:21:23, lushnikov wrote: ...
2 years, 11 months ago (2014-12-21 17:58:24 UTC) #45
aknudsen
https://codereview.chromium.org/676193002/diff/220001/Source/devtools/front_end/console/ConsoleView.js File Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/676193002/diff/220001/Source/devtools/front_end/console/ConsoleView.js#newcode1023 Source/devtools/front_end/console/ConsoleView.js:1023: var message = this._visibleViewMessages[matchRange.messageIndex]; On 2014/12/17 13:21:23, lushnikov wrote: ...
2 years, 11 months ago (2014-12-21 20:00:09 UTC) #46
aknudsen
Btw guys, I'm not able to test in Canary at this point. I get an ...
2 years, 11 months ago (2014-12-21 20:04:40 UTC) #47
lushnikov
On 2014/12/21 20:04:40, aknudsen wrote: > Btw guys, I'm not able to test in Canary ...
2 years, 11 months ago (2014-12-23 13:41:10 UTC) #48
lushnikov
lgtm given the last nit fixed yay! :) https://codereview.chromium.org/676193002/diff/220001/Source/devtools/front_end/console/ConsoleView.js File Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/676193002/diff/220001/Source/devtools/front_end/console/ConsoleView.js#newcode1051 Source/devtools/front_end/console/ConsoleView.js:1051: // ...
2 years, 11 months ago (2014-12-23 13:43:33 UTC) #49
robwu
One more nit from me. https://codereview.chromium.org/676193002/diff/260001/Source/devtools/front_end/inspectorCommon.css File Source/devtools/front_end/inspectorCommon.css (right): https://codereview.chromium.org/676193002/diff/260001/Source/devtools/front_end/inspectorCommon.css#newcode154 Source/devtools/front_end/inspectorCommon.css:154: Undo this unnecessary space ...
2 years, 11 months ago (2014-12-23 13:53:35 UTC) #50
aknudsen
On 2014/12/23 13:41:10, lushnikov wrote: > On 2014/12/21 20:04:40, aknudsen wrote: > > Btw guys, ...
2 years, 10 months ago (2014-12-24 10:52:20 UTC) #51
aknudsen
All done I think. Merry Christmas! https://codereview.chromium.org/676193002/diff/220001/Source/devtools/front_end/console/ConsoleView.js File Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/676193002/diff/220001/Source/devtools/front_end/console/ConsoleView.js#newcode1051 Source/devtools/front_end/console/ConsoleView.js:1051: // Scroll the ...
2 years, 10 months ago (2014-12-24 11:08:07 UTC) #52
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/676193002/280001
2 years, 10 months ago (2014-12-24 22:42:42 UTC) #54
commit-bot: I haz the power
Try jobs failed on following builders: blink_presubmit on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/23038) win_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/builds/30681) win_gpu ...
2 years, 10 months ago (2014-12-24 22:47:40 UTC) #56
robwu
apavlov: Could you put your LGTM on this CL? Lushnikov has already LGed this CL, ...
2 years, 10 months ago (2014-12-24 22:51:05 UTC) #57
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/676193002/280001
2 years, 10 months ago (2014-12-25 22:14:12 UTC) #59
commit-bot: I haz the power
Try jobs failed on following builders: blink_presubmit on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/23076)
2 years, 10 months ago (2014-12-25 22:20:03 UTC) #61
pfeldman
lgtm
2 years, 10 months ago (2015-01-12 13:24:29 UTC) #63
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/676193002/300001
2 years, 10 months ago (2015-01-12 14:45:15 UTC) #65
commit-bot: I haz the power
2 years, 10 months ago (2015-01-12 17:28:29 UTC) #66
Message was sent while issue was closed.
Committed patchset #14 (id:300001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188234

Powered by Google App Engine
This is Rietveld efc10ee0f