DevTools: Optimize source color picker to analyze changed lines

third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js

Index: third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js
diff --git a/third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js b/third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js
index 49bbec382d2ec6e39063ba4c245880cc2393b990..3b0b52ec957e0cba9317f79b9b13b9efee3858df 100644
--- a/third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js
+++ b/third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js
@@ -38,14 +38,10 @@ WebInspector.CSSSourceFrame = function(uiSourceCode)
     WebInspector.UISourceCodeFrame.call(this, uiSourceCode);
     this.textEditor.setAutocompleteDelegate(new WebInspector.CSSSourceFrame.AutocompleteDelegate());
     this._registerShortcuts();
-    this._colorBookmarks = [];
     this._swatchPopoverHelper = new WebInspector.SwatchPopoverHelper();
     this._muteColorProcessing = false;
 }
 
-/** @type {number} */
-WebInspector.CSSSourceFrame.UpdateTimeout = 200;
-
 WebInspector.CSSSourceFrame.prototype = {
     _registerShortcuts: function()
     {
@@ -103,82 +99,81 @@ WebInspector.CSSSourceFrame.prototype = {
         return true;
     },
 
-    _updateColorSwatches: function()
+    /**
+     * @param {number} startLine
+     * @param {number} endLine
+     */
+    _updateColorSwatches: function(startLine, endLine)
     {
-        if (this._updateTimeout)
-            clearTimeout(this._updateTimeout);
-        this._updateTimeout = null;
-
-        var colorPositions = this._extractColorPositions(this.textEditor.text());
-        this.textEditor.operation(this._putColorSwatchesInline.bind(this, colorPositions));
+        var colorPositions = this._extractColorPositions(startLine, endLine);
+        this.textEditor.operation(this._putColorSwatchesInline.bind(this, startLine, endLine, colorPositions));
     },
 
     /**
-     * @param {string} content
-     * @return {?Array<!WebInspector.CSSSourceFrame.ColorPosition>}
+     * @param {number} startLine
+     * @param {number} endLine
+     * @return {!Array<!WebInspector.CSSSourceFrame.ColorPosition>}
      */
-    _extractColorPositions: function(content)
+    _extractColorPositions: function(startLine, endLine)

[lushnikov, 2016/07/25 23:15:36]

Now we have _updateColorSwatches, _extractColorPositions and
_putColorSwatchesInline, which call
each other and basically do the same thing. Let's simplify this!

[flandy, 2016/07/26 01:16:46]

I've combined these three functions into _updateColorSwatches.

     {
-        if (!content)
-            return null;
-
         var colorPositions = [];
-        var text = new WebInspector.Text(content);
-        var numberOfLines = text.lineCount();
         var colorRegex = /[\s:;,(){}]((?:rgb|hsl)a?\([^)]+\)|#[0-9a-f]{8}|#[0-9a-f]{6}|#[0-9a-f]{3,4}|[a-z]+)(?=[\s;,(){}])/gi;
-        for (var lineNumber = 0; lineNumber < numberOfLines; lineNumber++) {
-            var line = text.lineAt(lineNumber) + "\n";
+        for (var lineNumber = startLine; lineNumber <= endLine; lineNumber++) {
+            var line = this.textEditor.line(lineNumber) + "\n";
             var match;
             while ((match = colorRegex.exec(line)) !== null) {
                 if (match.length < 2)
                     continue;
-
                 var colorText = match[1];
                 var color = WebInspector.Color.parse(colorText);
                 if (color)
                     colorPositions.push(new WebInspector.CSSSourceFrame.ColorPosition(color, lineNumber, match.index + 1, colorText.length));
             }
         }
-
         return colorPositions;
     },
 
     /**
-     * @param {?Array<!WebInspector.CSSSourceFrame.ColorPosition>} colorPositions
+     * @param {number} startLine
+     * @param {number} endLine
+     * @param {!Array<!WebInspector.CSSSourceFrame.ColorPosition>} colorPositions
      */
-    _putColorSwatchesInline: function(colorPositions)
+    _putColorSwatchesInline: function(startLine, endLine, colorPositions)

[lushnikov, 2016/07/25 23:15:36]

let's not pass colorPositions from the outside

[flandy, 2016/07/26 01:16:46]

Done.

     {
-        this._clearColorBookmarks();
-        if (!colorPositions)
-            return;
+        this._clearBookmarks(startLine, endLine);
 
         for (var i = 0; i < colorPositions.length; i++) {
             var colorPosition = colorPositions[i];
-
             var swatch = WebInspector.ColorSwatch.create();
             swatch.setColorText(colorPosition.color.asString(WebInspector.Color.Format.Original));
             swatch.iconElement().title = WebInspector.UIString("Open color picker.");
-            swatch.iconElement().addEventListener("click", this._showSpectrum.bind(this, swatch, colorPosition), true);
             swatch.hideText(true);
-
             var bookmark = this.textEditor.addBookmark(colorPosition.textRange.startLine, colorPosition.textRange.startColumn, swatch);
-            this._colorBookmarks.push(bookmark);
+            swatch.iconElement().addEventListener("click", this._showSpectrum.bind(this, swatch, bookmark), true);
         }
     },
 
-    _clearColorBookmarks: function()
+    /**
+     * @param {number} startLine
+     * @param {number} endLine
+     */
+    _clearBookmarks: function(startLine, endLine)
     {
-        for (var i = 0; i < this._colorBookmarks.length; i++)
-            this._colorBookmarks[i].clear();
-        this._colorBookmarks = [];
+        var range = new WebInspector.TextRange(startLine, 0, endLine, this.textEditor.line(endLine).length);
+        var markers = this.textEditor.bookmarks(range);
+        for (var i = 0; i < markers.length; i++) {
+            var marker = markers[i];
+            if (marker.type === "bookmark")

[lushnikov, 2016/07/25 23:15:36]

what other type could there be? Shouldn't we hide this detail inside
CodeMirrorTextEditor?

[flandy, 2016/07/26 01:16:46]

Another type is "range", for example added with doc.markText()
https://codemirror.net/doc/manual.html#api_marker. Yes, let's move this to
CodeMirrorTextEditor.

+                marker.clear();
+        }
     },
 
     /**
      * @param {!WebInspector.ColorSwatch} swatch
-     * @param {!WebInspector.CSSSourceFrame.ColorPosition} colorPosition
+     * @param {!CodeMirror.TextMarker} bookmark
      * @param {!Event} event
      */
-    _showSpectrum: function(swatch, colorPosition, event)
+    _showSpectrum: function(swatch, bookmark, event)
     {
         event.consume(true);
         if (this._swatchPopoverHelper.isShowing()) {
@@ -186,9 +181,11 @@ WebInspector.CSSSourceFrame.prototype = {
             return;
         }
         this._hadSpectrumChange = false;
+        var position = bookmark.find();
+        var colorText = swatch.color().asString(WebInspector.Color.Format.Original);
+        this._currentTextRange = new WebInspector.TextRange(position.line, position.ch, position.line, position.ch + colorText.length);

[lushnikov, 2016/07/25 23:15:36]

this._currentTextRange is not descriptive enough (and even tricks reader into a
confusion that this is a cursor position, for example)

[flandy, 2016/07/26 01:16:46]

How about _currentColorTextRange

         this._currentSwatch = swatch;
-        this._currentColorPosition = colorPosition;
-        this.textEditor.setSelection(WebInspector.TextRange.createFromLocation(colorPosition.textRange.startLine, colorPosition.textRange.startColumn));
+        this.textEditor.setSelection(WebInspector.TextRange.createFromLocation(position.line, position.ch));
         this._spectrum = new WebInspector.Spectrum();
         this._spectrum.setColor(swatch.color(), swatch.format());
         this._spectrum.addEventListener(WebInspector.Spectrum.Events.SizeChanged, this._spectrumResized, this);
@@ -213,9 +210,8 @@ WebInspector.CSSSourceFrame.prototype = {
         this._hadSpectrumChange = true;
         var colorString = /** @type {string} */ (event.data);
         this._currentSwatch.setColorText(colorString);
-        this.textEditor.editRange(this._currentColorPosition.textRange, colorString, "*color-text-changed");
-        this._currentColorPosition.color = WebInspector.Color.parse(colorString);
-        this._currentColorPosition.textRange.endColumn = this._currentColorPosition.textRange.startColumn + colorString.length;
+        this._textEditor.editRange(this._currentTextRange, colorString, "*color-text-changed");
+        this._currentTextRange.endColumn = this._currentTextRange.startColumn + colorString.length;
     },
 
     /**
@@ -229,9 +225,7 @@ WebInspector.CSSSourceFrame.prototype = {
             this.textEditor.undo();
         delete this._spectrum;
         delete this._currentSwatch;

[lushnikov, 2016/07/25 23:15:36]

for some reason you don't delete this._currentTextRange here, but delete
currentSwatch. Is there a good reason for that?

[flandy, 2016/07/26 01:16:46]

We can delete them all or leave them all. It should not make a difference.

-        delete this._currentColorPosition;
         this._muteColorProcessing = false;
-        this._updateColorSwatches();
     },
 
     /**
@@ -241,7 +235,7 @@ WebInspector.CSSSourceFrame.prototype = {
     {
         WebInspector.UISourceCodeFrame.prototype.onTextEditorContentSet.call(this);
         if (!this._muteColorProcessing && Runtime.experiments.isEnabled("sourceColorPicker"))
-            this._updateColorSwatches();
+            this._updateColorSwatches(0, this.textEditor.linesCount - 1);
     },
 
     /**
@@ -252,11 +246,8 @@ WebInspector.CSSSourceFrame.prototype = {
     onTextChanged: function(oldRange, newRange)
     {
         WebInspector.UISourceCodeFrame.prototype.onTextChanged.call(this, oldRange, newRange);
-        if (!this._muteColorProcessing && Runtime.experiments.isEnabled("sourceColorPicker")) {
-            if (this._updateTimeout)
-                clearTimeout(this._updateTimeout);
-            this._updateTimeout = setTimeout(this._updateColorSwatches.bind(this), WebInspector.CSSSourceFrame.UpdateTimeout);
-        }
+        if (!this._muteColorProcessing && Runtime.experiments.isEnabled("sourceColorPicker"))
+            this._updateColorSwatches(newRange.startLine, newRange.endLine);
     },
 
     /**