Navigate between individual search matches in DevTools console

Source/devtools/front_end/console/ConsoleView.js

Index: Source/devtools/front_end/console/ConsoleView.js
diff --git a/Source/devtools/front_end/console/ConsoleView.js b/Source/devtools/front_end/console/ConsoleView.js
index 908af5059261eaf8247a8edd19d09d2ae619d6c1..fa735eca1ad3638bf04680b0fafc0c5737291448 100644
--- a/Source/devtools/front_end/console/ConsoleView.js
+++ b/Source/devtools/front_end/console/ConsoleView.js
@@ -48,6 +48,8 @@ WebInspector.ConsoleView = function()
     this._visibleViewMessages = [];
     this._urlToMessageCount = {};
     this._hiddenByFilterCount = 0;
+    this._regexMatchRanges = [];

[lushnikov, 2014/11/07 16:27:16]

please annotate this with the type of elemens in the array. 

You might want to typedef your classname for regexMatch object - please, take a
look at the CSSParser.js file for an example.

[aknudsen, 2014/11/08 11:49:50]

Acknowledged.

[aknudsen, 2014/11/09 00:02:12]

Done.

+
 
     this._clearConsoleButton = new WebInspector.StatusBarButton(WebInspector.UIString("Clear console log."), "clear-status-bar-item");
     this._clearConsoleButton.addEventListener("click", this._requestClearMessages, this);
@@ -574,10 +576,8 @@ WebInspector.ConsoleView.prototype = {
 
             this._visibleViewMessages.push(viewMessage);
 
-            if (this._searchRegex && viewMessage.matchesRegex(this._searchRegex)) {
-                this._searchResults.push(viewMessage);
-                this._searchableView.updateSearchMatchesCount(this._searchResults.length);
-            }
+            if (this._searchRegex && this._searchMessage(this._visibleViewMessages.length - 1))
+                this._searchableView.updateSearchMatchesCount(this._regexMatchRanges.length);
         }
 
         if (viewMessage.consoleMessage().isGroupStartMessage())
@@ -606,7 +606,7 @@ WebInspector.ConsoleView.prototype = {
 
     _consoleCleared: function()
     {
-        this._clearCurrentSearchResultHighlight();
+        this._clearSearchResultHighlights();
         this._consoleMessages = [];
         this._updateMessageList();
 
@@ -684,7 +684,7 @@ WebInspector.ConsoleView.prototype = {
     {
         this._topGroup = WebInspector.ConsoleGroup.createTopGroup();
         this._currentGroup = this._topGroup;
-        this._searchResults = [];
+        this._regexMatchRanges = [];
         this._hiddenByFilterCount = 0;
         for (var i = 0; i < this._visibleViewMessages.length; ++i) {
             this._visibleViewMessages[i].resetCloseGroupDecorationCount();
@@ -898,8 +898,8 @@ WebInspector.ConsoleView.prototype = {
 
     searchCanceled: function()
     {
-        this._clearCurrentSearchResultHighlight();
-        delete this._searchResults;
+        this._clearSearchResultHighlights();
+        this._regexMatchRanges = [];
         delete this._searchRegex;
         this._viewport.refresh();
     },
@@ -916,31 +916,24 @@ WebInspector.ConsoleView.prototype = {
         this._searchableView.updateSearchMatchesCount(0);
         this._searchRegex = createPlainTextSearchRegex(query, "gi");
 
-        /** @type {!Array.<number>} */
-        this._searchResults = [];
-        for (var i = 0; i < this._visibleViewMessages.length; i++) {
-            if (this._visibleViewMessages[i].matchesRegex(this._searchRegex))
-                this._searchResults.push(i);
-        }
-        this._searchableView.updateSearchMatchesCount(this._searchResults.length);
-        this._currentSearchResultIndex = -1;
-        if (shouldJump && this._searchResults.length)
-            this._jumpToSearchResult(jumpBackwards ? -1 : 0);
+        this._regexMatchRanges = [];
+        this._currentMatchRangeIndex = -1;
+        for (var i = 0; i < this._visibleViewMessages.length; i++)
+            this._searchMessage(i);
+        this._searchableView.updateSearchMatchesCount(this._regexMatchRanges.length);
+        if (shouldJump)
+            this._jumpToMatch(jumpBackwards ? -1 : 0);
         this._viewport.refresh();
     },
 
     jumpToNextSearchResult: function()
     {
-        if (!this._searchResults || !this._searchResults.length)
-            return;
-        this._jumpToSearchResult(this._currentSearchResultIndex + 1);
+        this._jumpToMatch(this._currentMatchRangeIndex + 1);
     },
 
     jumpToPreviousSearchResult: function()
     {
-        if (!this._searchResults || !this._searchResults.length)
-            return;
-        this._jumpToSearchResult(this._currentSearchResultIndex - 1);
+        this._jumpToMatch(this._currentMatchRangeIndex - 1);
     },
 
     /**
@@ -959,26 +952,64 @@ WebInspector.ConsoleView.prototype = {
         return false;
     },
 
-    _clearCurrentSearchResultHighlight: function()
+    _clearSearchResultHighlights: function()
     {
-        if (!this._searchResults)
-            return;
-
-        var highlightedViewMessage = this._visibleViewMessages[this._searchResults[this._currentSearchResultIndex]];
-        if (highlightedViewMessage)
-            highlightedViewMessage.clearHighlight();
-        this._currentSearchResultIndex = -1;
+        for (var i = 0; i < this._regexMatchRanges.length; ++i) {
+            var matchRange = this._regexMatchRanges[i];
+            var message = this._visibleViewMessages[

[lushnikov, 2014/11/07 16:27:17]

please no wrapping in the code

[aknudsen, 2014/11/08 11:49:50]

Acknowledged.

Out of curiousity though, how do you guys deal with long lines, by enabling
soft-wrap in the editor? I'm totally used to a certain maximum line length,
which is useful in avoiding having to side scroll when diffing for instance.

[aknudsen, 2014/11/09 00:02:12]

Done.

[lushnikov, 2014/11/19 06:55:47]

It depends, one enables line-wrapping, one gets larger display.. All in all, its
a styleguide we all follow.

+                matchRange.messageIndex];
+            if (message)
+                message.clearHighlights();
+        }
+        this._currentMatchRangeIndex = -1;
     },
 
-    _jumpToSearchResult: function(index)
+    _jumpToMatch: function(index)

[lushnikov, 2014/11/07 16:27:16]

annotate

[aknudsen, 2014/11/08 11:49:51]

Acknowledged.

[aknudsen, 2014/11/09 00:02:13]

Done.

     {
-        index = mod(index, this._searchResults.length);
-        this._clearCurrentSearchResultHighlight();
-        this._currentSearchResultIndex = index;
-        this._searchableView.updateCurrentMatchIndex(this._currentSearchResultIndex);
-        var currentViewMessageIndex = this._searchResults[index];
-        this._viewport.scrollItemIntoView(currentViewMessageIndex);
-        this._visibleViewMessages[currentViewMessageIndex].highlightSearchResults(this._searchRegex);
+        if (!this._regexMatchRanges.length)
+            return;
+
+        index = mod(index, this._regexMatchRanges.length);

[lushnikov, 2014/11/07 16:27:17]

this line could be moved after the "if" 

[aknudsen, 2014/11/08 11:49:51]

Acknowledged.

[aknudsen, 2014/11/09 00:02:13]

Done.

+        var matchRange;
+        if (this._currentMatchRangeIndex >= 0) {
+            // Convert current match highlight to regular match highlight

[lushnikov, 2014/11/07 16:27:17]

remove comment

[aknudsen, 2014/11/08 11:49:50]

Acknowledged.

[aknudsen, 2014/11/09 00:02:13]

Done.

+            matchRange = this._regexMatchRanges[this._currentMatchRangeIndex];
+            matchRange.highlightNode.classList.remove(WebInspector.currentHighlightedSearchResultClassName);
+            matchRange.highlightNode.classList.add(WebInspector.highlightedSearchResultClassName);

[lushnikov, 2014/11/07 16:27:16]

We can make this simpler. This line could be deleted together with line #984. 

After that, your rule for highlighting current-search-result
should have a selector with bigger specificity, e.g. 

.highlighted-search-result.current-search-result { 
...
}

[aknudsen, 2014/11/08 11:49:50]

Acknowledged.

[aknudsen, 2014/11/09 00:02:13]

Done.

+        }
+
+        this._currentMatchRangeIndex = index;
+        this._searchableView.updateCurrentMatchIndex(index);
+        matchRange = this._regexMatchRanges[index];
+        matchRange.highlightNode.classList.remove(WebInspector.highlightedSearchResultClassName);

[lushnikov, 2014/11/07 16:27:16]

this is not needed as per previous comment

[aknudsen, 2014/11/08 11:49:50]

Acknowledged.

[aknudsen, 2014/11/09 00:02:13]

Done.

+        matchRange.highlightNode.classList.add(WebInspector.currentHighlightedSearchResultClassName);
+        // Scroll the message itself into view

[lushnikov, 2014/11/07 16:27:17]

lets remove these comments regarding scroll and add a test instead to verify
that every match is actually shown to the user.

[aknudsen, 2014/11/08 11:49:51]

Why are the comments problematic though? I had no idea initially, for example,
that the message had to be scrolled into view before
matchRange.highlightNode.scrollIntoViewIfNeeded() would have any effect. I agree
that there should be a test in any case.

[lushnikov, 2014/11/19 06:55:47]

In this case, the comment doesn't add any new information to the line it
comments (viewport.scrollItemIntoView is verbose
and clear itself).
In general, the policy is to write as little comments as possible and prefer
self-describing code.

+        this._viewport.scrollItemIntoView(matchRange.messageIndex);
+        // In case the highlight node is in a tree element, it must be scrolled into view, 
+        // the message itself must first be scrolled into view though
+        matchRange.highlightNode.scrollIntoViewIfNeeded();
+    },
+
+    _searchMessage: function(index)

[lushnikov, 2014/11/07 16:27:16]

please annotate
also, please move it closer to the performSearch method to improve ease of read

[aknudsen, 2014/11/08 11:49:50]

Acknowledged.

[aknudsen, 2014/11/09 00:02:13]

Done.

+    {
+        // Reset regex wrt. global search

[lushnikov, 2014/11/07 16:27:16]

please finish the comment with a period

[aknudsen, 2014/11/08 11:49:51]

Acknowledged.

Is this a common style guideline?

[aknudsen, 2014/11/09 00:02:12]

Done.

[lushnikov, 2014/11/19 06:55:47]

Yes, http://www.chromium.org/blink/coding-style#TOC-Comments

+        this._searchRegex.lastIndex = 0;
+
+        var message = this._visibleViewMessages[index];
+        var text = message.getText();
+        var match;
+        var matchRange;
+        while ((match = this._searchRegex.exec(text)) && match[0]) {
+            matchRange = {
+                messageIndex: index,
+                start: match.index,
+                end: match.index + match[0].length - 1,

[lushnikov, 2014/11/07 16:27:16]

you compute "end" here, and afterward couple of layers below (in highlightMatch
function) compute range length again. Lets store length here?

[aknudsen, 2014/11/08 11:49:50]

Acknowledged.

[aknudsen, 2014/11/09 00:02:13]

I simplified, by sending an array of SourceRanges to message.highlightMatches
instead.

+            };
+            matchRange.highlightNode = message.highlightMatch(matchRange.start, matchRange.end)[0];

[lushnikov, 2014/11/07 16:27:16]

Bad news: this calls WebInspector.highlightSearchResults for every match range. 
And this is inefficient, as it is iterating through the DOM every time.

Good news: WebInspector.highlightSearchResults accepts an array of ranges to be
highlighted, so this is easily fixable.

[aknudsen, 2014/11/08 11:49:50]

Acknowledged. Thanks.

[aknudsen, 2014/11/09 00:02:12]

Done.

+            this._regexMatchRanges.push(matchRange);
+        }
+
+        return !!matchRange;
     },
 
     __proto__: WebInspector.VBox.prototype
@@ -1132,7 +1163,7 @@ WebInspector.ConsoleCommand = function(message, linkifier, nestingLevel)
 }
 
 WebInspector.ConsoleCommand.prototype = {
-    clearHighlight: function()
+    clearHighlights: function()
     {
         var highlightedMessage = this._formattedCommand;
         delete this._formattedCommand;
@@ -1140,6 +1171,7 @@ WebInspector.ConsoleCommand.prototype = {
         this._element.replaceChild(this._formattedCommand, highlightedMessage);
     },
 
+    // XXX: Can we remove this?

[lushnikov, 2014/11/07 16:27:17]

Yes! 

[aknudsen, 2014/11/08 11:49:51]

Acknowledged. :)

[aknudsen, 2014/11/09 00:02:13]

Done.

     /**
      * @param {!RegExp} regexObject
      */
@@ -1187,6 +1219,28 @@ WebInspector.ConsoleCommand.prototype = {
         this._formattedCommand.textContent = this.text;
     },
 
+    /**
+    * @return {!string}

[lushnikov, 2014/11/07 16:27:16]

wrong indent for comments (asterisks should form a column)

[apavlov, 2014/11/07 16:30:07]

Non-object types should not have ! in front of them ({!Object} but not {!string}
or {!function()}), as they are non-nullable by default (as opposed to object
types, which are nullable by default).

[aknudsen, 2014/11/08 11:49:50]

Acknowledged.

[aknudsen, 2014/11/08 11:49:51]

Acknowledged.

[aknudsen, 2014/11/09 00:02:12]

Done.

[aknudsen, 2014/11/09 00:02:13]

Done.

+    */
+    getText: function()
+    {
+        return this.text;
+    },
+
+    /**
+    * @return {!Array.<!Element>}
+    */
+    highlightMatch: function(start, end)

[lushnikov, 2014/11/07 16:27:17]

arguments should be annotated as well

[aknudsen, 2014/11/08 11:49:51]

Acknowledged.

[aknudsen, 2014/11/09 00:02:13]

Done.

+    {
+        var highlightNodes = [];
+        if (this._formattedCommand) {
+            highlightNodes = WebInspector.highlightSearchResults(this._formattedCommand,

[lushnikov, 2014/11/07 16:27:16]

please no wrap

[aknudsen, 2014/11/08 11:49:50]

Acknowledged.

[aknudsen, 2014/11/09 00:02:13]

Done.

+                [new WebInspector.SourceRange(start, end - start + 1)]);
+            this._element.scrollIntoViewIfNeeded();

[lushnikov, 2014/11/07 16:27:17]

Why do you need this call? It should not be needed.

[aknudsen, 2014/11/09 00:02:12]

Done.

+        }
+        return highlightNodes;
+    },
+
     __proto__: WebInspector.ConsoleViewMessage.prototype
 }