DevTools: Optimize source color picker to analyze changed lines

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

 /*
  * Copyright (C) 2013 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 20 matching lines @@
 /**
  * @constructor
  * @extends {WebInspector.UISourceCodeFrame}
  * @param {!WebInspector.UISourceCode} uiSourceCode
  */
 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()
     {
         var shortcutKeys = WebInspector.ShortcutsScreen.SourcesPanelShortcuts;
         for (var i = 0; i < shortcutKeys.IncreaseCSSUnitByOne.length; ++i)
             this.addShortcut(shortcutKeys.IncreaseCSSUnitByOne[i].key, this._handleUnitModification.bind(this, 1));
         for (var i = 0; i < shortcutKeys.DecreaseCSSUnitByOne.length; ++i)
             this.addShortcut(shortcutKeys.DecreaseCSSUnitByOne[i].key, this._handleUnitModification.bind(this, -1));
         for (var i = 0; i < shortcutKeys.IncreaseCSSUnitByTen.length; ++i)
             this.addShortcut(shortcutKeys.IncreaseCSSUnitByTen[i].key, this._handleUnitModification.bind(this, 10));
@@ skipping 37 matching lines @@
         var newUnitText = this._modifyUnit(cssUnitText, change);
         if (!newUnitText)
             return false;
         this.textEditor.editRange(cssUnitRange, newUnitText);
         selection.startColumn = token.startColumn;
         selection.endColumn = selection.startColumn + newUnitText.length;
         this.textEditor.setSelection(selection);
         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()) {
             this._swatchPopoverHelper.hide(true);
             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);
         this._spectrum.addEventListener(WebInspector.Spectrum.Events.ColorChanged, this._spectrumChanged, this);
         this._swatchPopoverHelper.show(this._spectrum, swatch.iconElement(), this._spectrumHidden.bind(this));
     },
 
     /**
      * @param {!WebInspector.Event} event
      */
     _spectrumResized: function(event)
     {
         this._swatchPopoverHelper.reposition();
     },
 
     /**
      * @param {!WebInspector.Event} event
      */
     _spectrumChanged: function(event)
     {
         this._muteColorProcessing = true;
         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;
     },
 
     /**
      * @param {boolean} commitEdit
      */
     _spectrumHidden: function(commitEdit)
     {
         this._spectrum.removeEventListener(WebInspector.Spectrum.Events.SizeChanged, this._spectrumResized, this);
         this._spectrum.removeEventListener(WebInspector.Spectrum.Events.ColorChanged, this._spectrumChanged, this);
         if (!commitEdit && this._hadSpectrumChange)
             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();
     },
 
     /**
      * @override
      */
     onTextEditorContentSet: function()
     {
         WebInspector.UISourceCodeFrame.prototype.onTextEditorContentSet.call(this);
         if (!this._muteColorProcessing && Runtime.experiments.isEnabled("sourceColorPicker"))
-            this._updateColorSwatches();
+            this._updateColorSwatches(0, this.textEditor.linesCount - 1);
     },
 
     /**
      * @override
      * @param {!WebInspector.TextRange} oldRange
      * @param {!WebInspector.TextRange} newRange
      */
     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);
     },
 
     /**
      * @override
      * @param {number} lineNumber
      */
     scrollChanged: function(lineNumber)
     {
         WebInspector.UISourceCodeFrame.prototype.scrollChanged.call(this, lineNumber);
         if (this._swatchPopoverHelper.isShowing())
@@ skipping 104 matching lines @@
         var propertyToken = this._backtrackPropertyToken(editor, prefixRange.startLine, prefixRange.startColumn - 1);
         if (!propertyToken)
             return this._simpleDelegate.wordsWithPrefix(editor, prefixRange, substituteRange);
 
         var line = editor.line(prefixRange.startLine);
         var tokenContent = line.substring(propertyToken.startColumn, propertyToken.endColumn);
         var keywords = WebInspector.CSSMetadata.keywordsForProperty(tokenContent);
         return keywords.startsWith(prefix);
     },
 }