DevTools: Find bookmarks at line endpoints when calling _clearBookmarks

DevTools: Find bookmarks at line endpoints when calling _clearBookmarks

This fixes an issue with the source color picker where undo-ing the color text did not
always remove the color swatch. CodeMirror findMarks() is not inclusive, so we also need
to check the line endpoints.

The additional newline added to the string in _updateColorSwatches allows
color text to be matched when it is located at the start of a line.

R=lushnikov

Committed: https://crrev.com/a267f68a32a1ecc8f1c220f621db70c96fe3738c
Cr-Commit-Position: refs/heads/master@{#408758}

[flandy, 2016-07-27 22:26:24]

Description was changed from

==========
DevTools: Find bookmarks at line endpoints when calling _clearBookmarks

This fixes an issue where undo-ing the color text did not always remove the
color swatch. CodeMirror findMarks() is not inclusive, so we also need to
check the line endpoints.

The additional newline added to the string in _updateColorSwatches allows
color text to be matched when it is located at the start of a line.

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

to

==========
DevTools: Find bookmarks at line endpoints when calling _clearBookmarks

This fixes an issue with the source color picker where undo-ing the color text
did not
always remove the color swatch. CodeMirror findMarks() is not inclusive, so we
also need
to check the line endpoints.

The additional newline added to the string in _updateColorSwatches allows
color text to be matched when it is located at the start of a line.

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

[flandy, 2016-07-27 22:28:12]

Please take a look

[lushnikov, 2016-07-27 23:32:02]

I believe findMarks is supposed to be inclusive; maybe we can just fix it
upstream?

https://codereview.chromium.org/2187173002/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/source_frame/CodeMirrorTextEditor.js
(right):

https://codereview.chromium.org/2187173002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/source_frame/CodeMirrorTextEditor.js:1036:
bookmarksAt: function(pos)
let's operate in terms of Ranges

https://codereview.chromium.org/2187173002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js
(right):

https://codereview.chromium.org/2187173002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:154: var
startPos = new CodeMirror.Pos(startLine, 0);
we shouldn't be using CodeMirror.Pos outside of CodeMirrorTextEditor, let's do
ranges right away

[flandy, 2016-07-29 17:25:51]

Please take another look. Marijn confirmed that findMarks is intended to be
exclusive, so we'll have to continue using findMarksAt.

https://codereview.chromium.org/2187173002/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/source_frame/CodeMirrorTextEditor.js
(right):

https://codereview.chromium.org/2187173002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/source_frame/CodeMirrorTextEditor.js:1036:
bookmarksAt: function(pos)
On 2016/07/27 23:32:02, lushnikov wrote:
> let's operate in terms of Ranges

Done.

https://codereview.chromium.org/2187173002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js
(right):

https://codereview.chromium.org/2187173002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:154: var
startPos = new CodeMirror.Pos(startLine, 0);
On 2016/07/27 23:32:02, lushnikov wrote:
> we shouldn't be using CodeMirror.Pos outside of CodeMirrorTextEditor, let's do
> ranges right away

Done.

[lushnikov, 2016-07-29 17:46:25]

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

https://codereview.chromium.org/2187173002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/sources/color-swatch-position.html:63:
function testBookmarksBoundary(next)
testBookmarsAtLineStart

https://codereview.chromium.org/2187173002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/source_frame/CodeMirrorTextEditor.js
(right):

https://codereview.chromium.org/2187173002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/source_frame/CodeMirrorTextEditor.js:1028:
if (pos.start.line === pos.end.line && pos.start.ch === pos.end.ch)
if (range.isEmpty())

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

https://codereview.chromium.org/2187173002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:115: var
line = "\n" + this.textEditor.line(lineNumber).substring(0,
WebInspector.CSSSourceFrame.maxSwatchProcessingLength) + "\n";
could you please remind me why we add newlines here?

[flandy, 2016-07-29 18:31:39]

Addressed the comments :)

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

https://codereview.chromium.org/2187173002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/sources/color-swatch-position.html:63:
function testBookmarksBoundary(next)
On 2016/07/29 17:46:24, lushnikov wrote:
> testBookmarsAtLineStart

Done.

https://codereview.chromium.org/2187173002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/source_frame/CodeMirrorTextEditor.js
(right):

https://codereview.chromium.org/2187173002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/source_frame/CodeMirrorTextEditor.js:1028:
if (pos.start.line === pos.end.line && pos.start.ch === pos.end.ch)
On 2016/07/29 17:46:24, lushnikov wrote:
> if (range.isEmpty())

Done.

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

https://codereview.chromium.org/2187173002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:115: var
line = "\n" + this.textEditor.line(lineNumber).substring(0,
WebInspector.CSSSourceFrame.maxSwatchProcessingLength) + "\n";
On 2016/07/29 17:46:24, lushnikov wrote:
> could you please remind me why we add newlines here?

The textEditor.line() function does not include newlines, but the regular
expression relies on the newlines to tell if the text is a valid color. The
regex checks to see if the color text is bordered by \s.

[lushnikov, 2016-07-29 19:13:48]

lgtm

[flandy, 2016-07-29 19:17:57]

The CQ bit was checked by flandy@google.com

[commit-bot: I haz the power, 2016-07-29 19:18:57]

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-29 21:19:41]

Description was changed from

==========
DevTools: Find bookmarks at line endpoints when calling _clearBookmarks

This fixes an issue with the source color picker where undo-ing the color text
did not
always remove the color swatch. CodeMirror findMarks() is not inclusive, so we
also need
to check the line endpoints.

The additional newline added to the string in _updateColorSwatches allows
color text to be matched when it is located at the start of a line.

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

to

==========
DevTools: Find bookmarks at line endpoints when calling _clearBookmarks

This fixes an issue with the source color picker where undo-ing the color text
did not
always remove the color swatch. CodeMirror findMarks() is not inclusive, so we
also need
to check the line endpoints.

The additional newline added to the string in _updateColorSwatches allows
color text to be matched when it is located at the start of a line.

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

[commit-bot: I haz the power, 2016-07-29 21:19:42]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-29 21:22:17]

Description was changed from

==========
DevTools: Find bookmarks at line endpoints when calling _clearBookmarks

This fixes an issue with the source color picker where undo-ing the color text
did not
always remove the color swatch. CodeMirror findMarks() is not inclusive, so we
also need
to check the line endpoints.

The additional newline added to the string in _updateColorSwatches allows
color text to be matched when it is located at the start of a line.

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

to

==========
DevTools: Find bookmarks at line endpoints when calling _clearBookmarks

This fixes an issue with the source color picker where undo-ing the color text
did not
always remove the color swatch. CodeMirror findMarks() is not inclusive, so we
also need
to check the line endpoints.

The additional newline added to the string in _updateColorSwatches allows
color text to be matched when it is located at the start of a line.

R=lushnikov

Committed: https://crrev.com/a267f68a32a1ecc8f1c220f621db70c96fe3738c
Cr-Commit-Position: refs/heads/master@{#408758}
==========

[commit-bot: I haz the power, 2016-07-29 21:22:18]

Patchset 3 (id:??) landed as
https://crrev.com/a267f68a32a1ecc8f1c220f621db70c96fe3738c
Cr-Commit-Position: refs/heads/master@{#408758}