DevTools: Optimize source color picker to analyze changed lines

DevTools: Optimize source color picker to analyze changed lines

-Analyze the new lines from onTextChanged when updating the color swatches.
(CodeMirror handles the bookmarks for any old lines that have been deleted)
-Use bookmark find() method to keep track of swatch positions.
-Remove the timeout so that swatches are updated immediately.
-Only analyze a set number of columns (currently 300) to improve loading
of minified style sheets.

R=lushnikov

Committed: https://crrev.com/959d01eae15091152be1e8b88489ecc451a83e8f
Cr-Commit-Position: refs/heads/master@{#408022}

[flandy, 2016-07-22 16:56:34]

Please take a look :)

[lushnikov, 2016-07-22 18:40:59]

looks like it's time to have a few tests for this

Let's add two tests which:
- makes sure color swatches are at least rendered
- makes sure that editing swatch properly updates the rest

[flandy, 2016-07-25 22:43:27]

On 2016/07/22 18:40:59, lushnikov wrote:
> looks like it's time to have a few tests for this
> 
> Let's add two tests which:
> - makes sure color swatches are at least rendered
> - makes sure that editing swatch properly updates the rest

I've added two tests. Please take another look

[lushnikov, 2016-07-25 23:15:37]

https://codereview.chromium.org/2173913002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/inspector/sources/color-swatch-position.html
(right):

https://codereview.chromium.org/2173913002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/sources/color-swatch-position.html:45:
}
lets add a few more tests here that test some corner cases

https://codereview.chromium.org/2173913002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/sources/color-swatch-position.html:48:
function addSwatchPositions()
dumpSwatchPositions

https://codereview.chromium.org/2173913002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/inspector/sources/color-swatch.html (right):

https://codereview.chromium.org/2173913002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/sources/color-swatch.html:3: 
I'd kill this test - it doesn't test much and if fully covered with
color-swatch-position.html

https://codereview.chromium.org/2173913002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js
(right):

https://codereview.chromium.org/2173913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:117:
_extractColorPositions: function(startLine, endLine)
Now we have _updateColorSwatches, _extractColorPositions and
_putColorSwatchesInline, which call
each other and basically do the same thing. Let's simplify this!

https://codereview.chromium.org/2173913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:141:
_putColorSwatchesInline: function(startLine, endLine, colorPositions)
let's not pass colorPositions from the outside

https://codereview.chromium.org/2173913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:166: if
(marker.type === "bookmark")
what other type could there be? Shouldn't we hide this detail inside
CodeMirrorTextEditor?

https://codereview.chromium.org/2173913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:186:
this._currentTextRange = new WebInspector.TextRange(position.line, position.ch,
position.line, position.ch + colorText.length);
this._currentTextRange is not descriptive enough (and even tricks reader into a
confusion that this is a cursor position, for example)

https://codereview.chromium.org/2173913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:227:
delete this._currentSwatch;
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]

Please take another look.

https://codereview.chromium.org/2173913002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/inspector/sources/color-swatch-position.html
(right):

https://codereview.chromium.org/2173913002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/sources/color-swatch-position.html:45:
}
On 2016/07/25 23:15:36, lushnikov wrote:
> lets add a few more tests here that test some corner cases

I've added two more cases. Do you have any ideas for corner cases?

https://codereview.chromium.org/2173913002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/sources/color-swatch-position.html:48:
function addSwatchPositions()
On 2016/07/25 23:15:36, lushnikov wrote:
> dumpSwatchPositions

Done.

https://codereview.chromium.org/2173913002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/inspector/sources/color-swatch.html (right):

https://codereview.chromium.org/2173913002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/sources/color-swatch.html:3: 
On 2016/07/25 23:15:36, lushnikov wrote:
> I'd kill this test - it doesn't test much and if fully covered with
> color-swatch-position.html

Done.

https://codereview.chromium.org/2173913002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js
(right):

https://codereview.chromium.org/2173913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:117:
_extractColorPositions: function(startLine, endLine)
On 2016/07/25 23:15:36, lushnikov wrote:
> Now we have _updateColorSwatches, _extractColorPositions and
> _putColorSwatchesInline, which call
> each other and basically do the same thing. Let's simplify this!

I've combined these three functions into _updateColorSwatches.

https://codereview.chromium.org/2173913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:141:
_putColorSwatchesInline: function(startLine, endLine, colorPositions)
On 2016/07/25 23:15:36, lushnikov wrote:
> let's not pass colorPositions from the outside

Done.

https://codereview.chromium.org/2173913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:166: if
(marker.type === "bookmark")
On 2016/07/25 23:15:36, lushnikov wrote:
> what other type could there be? Shouldn't we hide this detail inside
> CodeMirrorTextEditor?

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.

https://codereview.chromium.org/2173913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:186:
this._currentTextRange = new WebInspector.TextRange(position.line, position.ch,
position.line, position.ch + colorText.length);
On 2016/07/25 23:15:36, lushnikov wrote:
> this._currentTextRange is not descriptive enough (and even tricks reader into
a
> confusion that this is a cursor position, for example)

How about _currentColorTextRange

https://codereview.chromium.org/2173913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:227:
delete this._currentSwatch;
On 2016/07/25 23:15:36, lushnikov wrote:
> for some reason you don't delete this._currentTextRange here, but delete
> currentSwatch. Is there a good reason for that?

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

[lushnikov, 2016-07-26 22:41:26]

lgtm, thanks

Please make sure this works fine with multiple cursors

https://codereview.chromium.org/2173913002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js
(right):

https://codereview.chromium.org/2173913002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:111: var
maxColumnsSupported = 300;
let's extract this
WebInspector.CSSSourceFrame.maxSwatchProcessingLength = 300

[flandy, 2016-07-26 23:23:31]

Description was changed from

==========
DevTools: Optimize source color picker to analyze changed lines

-Analyze the new lines from onTextChanged when updating the color swatches.
(CodeMirror handles the bookmarks for any old lines that have been deleted)
-Use bookmark find() method to keep track of swatch positions.
-Remove the timeout so that swatches are updated immediately.

R=lushnikov
==========

to

==========
DevTools: Optimize source color picker to analyze changed lines

-Analyze the new lines from onTextChanged when updating the color swatches.
(CodeMirror handles the bookmarks for any old lines that have been deleted)
-Use bookmark find() method to keep track of swatch positions.
-Remove the timeout so that swatches are updated immediately.
-Only analyze a set number of columns (currently 300) to improve loading
of minified style sheets.

R=lushnikov
==========

[flandy, 2016-07-26 23:24:15]

It works fine with multiple cursors.

https://codereview.chromium.org/2173913002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js
(right):

https://codereview.chromium.org/2173913002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:111: var
maxColumnsSupported = 300;
On 2016/07/26 22:41:25, lushnikov wrote:
> let's extract this
> WebInspector.CSSSourceFrame.maxSwatchProcessingLength = 300

Done.

[flandy, 2016-07-26 23:25:19]

The CQ bit was checked by flandy@google.com

[flandy, 2016-07-26 23:25:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
lushnikov@chromium.org
Link to the patchset: https://codereview.chromium.org/2173913002/#ps80001
(title: "Address last comment")

[commit-bot: I haz the power, 2016-07-26 23:25:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-27 02:47:17]

Description was changed from

==========
DevTools: Optimize source color picker to analyze changed lines

-Analyze the new lines from onTextChanged when updating the color swatches.
(CodeMirror handles the bookmarks for any old lines that have been deleted)
-Use bookmark find() method to keep track of swatch positions.
-Remove the timeout so that swatches are updated immediately.
-Only analyze a set number of columns (currently 300) to improve loading
of minified style sheets.

R=lushnikov
==========

to

==========
DevTools: Optimize source color picker to analyze changed lines

-Analyze the new lines from onTextChanged when updating the color swatches.
(CodeMirror handles the bookmarks for any old lines that have been deleted)
-Use bookmark find() method to keep track of swatch positions.
-Remove the timeout so that swatches are updated immediately.
-Only analyze a set number of columns (currently 300) to improve loading
of minified style sheets.

R=lushnikov
==========

[commit-bot: I haz the power, 2016-07-27 02:47:18]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-27 02:49:20]

Description was changed from

==========
DevTools: Optimize source color picker to analyze changed lines

-Analyze the new lines from onTextChanged when updating the color swatches.
(CodeMirror handles the bookmarks for any old lines that have been deleted)
-Use bookmark find() method to keep track of swatch positions.
-Remove the timeout so that swatches are updated immediately.
-Only analyze a set number of columns (currently 300) to improve loading
of minified style sheets.

R=lushnikov
==========

to

==========
DevTools: Optimize source color picker to analyze changed lines

-Analyze the new lines from onTextChanged when updating the color swatches.
(CodeMirror handles the bookmarks for any old lines that have been deleted)
-Use bookmark find() method to keep track of swatch positions.
-Remove the timeout so that swatches are updated immediately.
-Only analyze a set number of columns (currently 300) to improve loading
of minified style sheets.

R=lushnikov

Committed: https://crrev.com/959d01eae15091152be1e8b88489ecc451a83e8f
Cr-Commit-Position: refs/heads/master@{#408022}
==========

[commit-bot: I haz the power, 2016-07-27 02:49:21]

Patchset 5 (id:??) landed as
https://crrev.com/959d01eae15091152be1e8b88489ecc451a83e8f
Cr-Commit-Position: refs/heads/master@{#408022}