Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2128093004:
DevTools: Add color swatches to Sources panel (Closed)
|
Created:
4 years, 5 months ago by flandy Modified:
4 years, 5 months ago Reviewers:
lushnikov CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, sergeyv+blink_chromium.org, pfeldman, kozyatinskiy+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: Add color swatches to the Sources panel when editing style sheets.
Color swatches help to visualize the code and will later launch color pickers to edit the colors.
BUG=266326
Committed: https://crrev.com/6a382a152026aa3f48a1e94ae89c9d83f37cd09e
Cr-Commit-Position: refs/heads/master@{#405423}
Patch Set 1 #Patch Set 2 : Inline color swatches #
Total comments: 10
Patch Set 3 : Addressed comments #
Total comments: 34
Patch Set 4 : Addressed further comments #
Total comments: 16
Patch Set 5 : Addressed comments #
Total comments: 6
Patch Set 6 : Addressed further comments #
Total comments: 2
Patch Set 7 : First iteration #
Messages
Total messages: 23 (7 generated)
Description was changed from ========== DevTools: Add color-picker to Sources panel to easily change style sheet colors DevTools: Added color swatches to the gutter of style sheet sources. Color swatches help to visualize the code and will later launch color pickers to edit the colors. Rough implementation. Color swatches are not currently clickable. Do not push! BUG=266326 ========== to ========== DevTools: Added color swatches to the gutter of style sheet sources. Color swatches help to visualize the code and will later launch color pickers to edit the colors. Rough implementation. Color swatches are not currently clickable. Do not push! BUG=266326 ==========
flandy@google.com changed reviewers: + lushnikov@chromium.org
Please take a look and let me know if I'm on the right track. Thanks!
Overall looks reasonably good! Let's try inline version to see how it works out.
On 2016/07/08 18:18:46, lushnikov wrote: > Overall looks reasonably good! Let's try inline version to see how it works out. Here is the initial inline version - Patch Set 2
I played with this, and I really liked it! Let's optimize performance (try recording a timeline and make sure your code doesn't interfere badly with typing inside editor). One thing to do is to use CodeMirror.operation for adding/removing bookmarks, and another is postponing your logic while user is typing in the editor. https://codereview.chromium.org/2128093004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js (right): https://codereview.chromium.org/2128093004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:105: __proto__: WebInspector.UISourceCodeFrame.prototype, nit: __proto__ should be the last thing in the object https://codereview.chromium.org/2128093004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:107: _workingCopyChanged: function(event) let's add jsdoc here https://codereview.chromium.org/2128093004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:157: _putColorSwatchesInline: function(colorPositions) this whole function should be a codemirror operation: http://codemirror.net/doc/manual.html#operation this will save a ton on relayouts https://codereview.chromium.org/2128093004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:160: this._clearColorBookmarks(); i guess you want clear color bookmarks outside of this if https://codereview.chromium.org/2128093004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sources/sourcesView.css (right): https://codereview.chromium.org/2128093004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/sourcesView.css:147: [is="color-swatch"] > span { a better way would be adding an API to the ColorSwatch.js to do this
Thanks again for the feedback! Please take another look. I added the use of code mirror operation and a timeout to improve performance. The timeout also helps to prevent the swatch from appearing when typing things like "blue-ish". https://codereview.chromium.org/2128093004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js (right): https://codereview.chromium.org/2128093004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:105: __proto__: WebInspector.UISourceCodeFrame.prototype, On 2016/07/12 03:48:02, lushnikov wrote: > nit: __proto__ should be the last thing in the object Done. https://codereview.chromium.org/2128093004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:107: _workingCopyChanged: function(event) On 2016/07/12 03:48:02, lushnikov wrote: > let's add jsdoc here Done. https://codereview.chromium.org/2128093004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:157: _putColorSwatchesInline: function(colorPositions) On 2016/07/12 03:48:02, lushnikov wrote: > this whole function should be a codemirror operation: > http://codemirror.net/doc/manual.html#operation > this will save a ton on relayouts Awesome! Done. https://codereview.chromium.org/2128093004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:160: this._clearColorBookmarks(); On 2016/07/12 03:48:02, lushnikov wrote: > i guess you want clear color bookmarks outside of this if Done. https://codereview.chromium.org/2128093004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sources/sourcesView.css (right): https://codereview.chromium.org/2128093004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/sourcesView.css:147: [is="color-swatch"] > span { On 2016/07/12 03:48:02, lushnikov wrote: > a better way would be adding an API to the ColorSwatch.js to do this Done.
https://codereview.chromium.org/2128093004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/source_frame/CodeMirrorTextEditor.js (right): https://codereview.chromium.org/2128093004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/source_frame/CodeMirrorTextEditor.js:1011: * @param {?Element} element can we make it non-nullable? The less nullable types, the better https://codereview.chromium.org/2128093004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/source_frame/CodeMirrorTextEditor.js:1012: * @return {!Object} let's introduce a type for this. (look at externs.js for some type examples) https://codereview.chromium.org/2128093004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js (right): https://codereview.chromium.org/2128093004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:116: updateColorSwatchesWhenPossible: function(){ let inline this method into _workingCopyChanged method https://codereview.chromium.org/2128093004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:122: updateColorSwatchesImmediately: function(){ _updateColorSwatches: https://codereview.chromium.org/2128093004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:142: var lines = content.split(/\r?\n/); can we use WI.Text here? It has methods such as lineAt and lineCount, no need to think about \r's. https://codereview.chromium.org/2128093004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:143: for (var lineNum = 0; lineNum < lines.length; lineNum++) { s/lineNum/lineNumber/g https://codereview.chromium.org/2128093004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:147: var arrResult; var match; https://codereview.chromium.org/2128093004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:150: if (arrResult.length >= 1) { let's use fast-return where possible. The less nesting the better! if (!arrResult.length) continue; Also, I believe you actually want to check for arrResult.length > 1 insetad of >= 1 https://codereview.chromium.org/2128093004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:154: var startIndex = arrResult[0].search(/[a-z]|#/i); why the arrResult.index doesn't work for you? https://codereview.chromium.org/2128093004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:172: if (colorPositions) { let's use fast-return here as well if (!colorPositions) return; https://codereview.chromium.org/2128093004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:175: var colorPos = colorPositions[i]; var colorPosition = ... https://codereview.chromium.org/2128093004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:179: swatch.setFormat(WebInspector.Color.detectColorFormat(colorPos.color)); do you need to set it any format? https://codereview.chromium.org/2128093004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:180: swatch.iconElement().title = WebInspector.UIString("Open color picker"); let's kill this until we have it https://codereview.chromium.org/2128093004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:190: for (var i = 0; i < this._colorBookmarks.length; i++) { remove {} https://codereview.chromium.org/2128093004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:202: if (Runtime.experiments.isEnabled("sourceColorPicker")) { remove {} https://codereview.chromium.org/2128093004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js (right): https://codereview.chromium.org/2128093004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js:48: showText: function(show) let's call it hideText https://codereview.chromium.org/2128093004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js:50: show ? this._colorValueElement.style.display = "initial" : this._colorValueElement.style.display = "none"; nit: let's do if statement here
https://codereview.chromium.org/2128093004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/source_frame/CodeMirrorTextEditor.js (right): https://codereview.chromium.org/2128093004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/source_frame/CodeMirrorTextEditor.js:1011: * @param {?Element} element On 2016/07/12 22:38:45, lushnikov wrote: > can we make it non-nullable? The less nullable types, the better Yes, done. https://codereview.chromium.org/2128093004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/source_frame/CodeMirrorTextEditor.js:1012: * @return {!Object} On 2016/07/12 22:38:45, lushnikov wrote: > let's introduce a type for this. (look at externs.js for some type examples) This should be {!CodeMirror.TextMarker}. I've introduced CodeMirror.TextMarker into externs.js. https://codereview.chromium.org/2128093004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js (right): https://codereview.chromium.org/2128093004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:116: updateColorSwatchesWhenPossible: function(){ On 2016/07/12 22:38:46, lushnikov wrote: > let inline this method into _workingCopyChanged method Done. https://codereview.chromium.org/2128093004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:122: updateColorSwatchesImmediately: function(){ On 2016/07/12 22:38:46, lushnikov wrote: > _updateColorSwatches: Done. https://codereview.chromium.org/2128093004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:142: var lines = content.split(/\r?\n/); On 2016/07/12 22:38:46, lushnikov wrote: > can we use WI.Text here? It has methods such as lineAt and lineCount, no need to > think about \r's. Yes we can. Done. https://codereview.chromium.org/2128093004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:143: for (var lineNum = 0; lineNum < lines.length; lineNum++) { On 2016/07/12 22:38:46, lushnikov wrote: > s/lineNum/lineNumber/g Done. https://codereview.chromium.org/2128093004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:147: var arrResult; On 2016/07/12 22:38:46, lushnikov wrote: > var match; Done. https://codereview.chromium.org/2128093004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:150: if (arrResult.length >= 1) { On 2016/07/12 22:38:45, lushnikov wrote: > let's use fast-return where possible. > > The less nesting the better! > > > if (!arrResult.length) > continue; > > Also, I believe you actually want to check for arrResult.length > 1 insetad of > >= 1 Okay great! Done. Yes, I updated the Regex to use a capturing group, so arrResult.length > 1 is correct! Thanks. In the new patch, this becomes: if (match.length < 2) continue; https://codereview.chromium.org/2128093004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:154: var startIndex = arrResult[0].search(/[a-z]|#/i); On 2016/07/12 22:38:46, lushnikov wrote: > why the arrResult.index doesn't work for you? Since I use a capturing group (in parenthesis) for the Regex, arrResult[1] contains the actual color text, like "red" arrResult[0] contains the color text and the preceding character, like ":red". I use a capturing group to make sure the character before the color is valid (i.e. one of the characters in [\s:;,(){}] ). I use arrResult.index + startIndex to skip past this first character. Since this should always be one character, arrResult.index + 1 is better. https://codereview.chromium.org/2128093004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:172: if (colorPositions) { On 2016/07/12 22:38:46, lushnikov wrote: > let's use fast-return here as well > > if (!colorPositions) > return; Done. https://codereview.chromium.org/2128093004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:175: var colorPos = colorPositions[i]; On 2016/07/12 22:38:46, lushnikov wrote: > var colorPosition = ... Done. https://codereview.chromium.org/2128093004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:179: swatch.setFormat(WebInspector.Color.detectColorFormat(colorPos.color)); On 2016/07/12 22:38:46, lushnikov wrote: > do you need to set it any format? No need. setColorText already sets the swatch's format. Removed. https://codereview.chromium.org/2128093004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:180: swatch.iconElement().title = WebInspector.UIString("Open color picker"); On 2016/07/12 22:38:46, lushnikov wrote: > let's kill this until we have it Done. https://codereview.chromium.org/2128093004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:190: for (var i = 0; i < this._colorBookmarks.length; i++) { On 2016/07/12 22:38:46, lushnikov wrote: > remove {} Done. https://codereview.chromium.org/2128093004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:202: if (Runtime.experiments.isEnabled("sourceColorPicker")) { On 2016/07/12 22:38:46, lushnikov wrote: > remove {} Done. https://codereview.chromium.org/2128093004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js (right): https://codereview.chromium.org/2128093004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js:48: showText: function(show) On 2016/07/12 22:38:46, lushnikov wrote: > let's call it hideText Done. https://codereview.chromium.org/2128093004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js:50: show ? this._colorValueElement.style.display = "initial" : this._colorValueElement.style.display = "none"; On 2016/07/12 22:38:46, lushnikov wrote: > nit: let's do if statement here Done.
https://codereview.chromium.org/2128093004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/externs.js (right): https://codereview.chromium.org/2128093004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/externs.js:589: CodeMirror.TextMarker = function(doc, type) { } shouldn't it have a "clear" method? https://codereview.chromium.org/2128093004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js (right): https://codereview.chromium.org/2128093004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:137: lets kill this new line https://codereview.chromium.org/2128093004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:141: var line = text.lineAt(lineNumber) + "\n"; why do you need "\n" in the end? https://codereview.chromium.org/2128093004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:142: let's remove this new line https://codereview.chromium.org/2128093004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:145: let's remove this new line as well https://codereview.chromium.org/2128093004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:190: onTextEditorContentLoaded: function() let's rather override onTextChanged method from UISourceCodeFrame instead of overriding this method and listening to UISOurceCode events! With this in hand, you will eliminate a race condition between UISourceCode events and TextEditor updates https://codereview.chromium.org/2128093004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:197: dispose: function() you will not need this method as you'll stop listening to UISourceCode! https://codereview.chromium.org/2128093004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js (right): https://codereview.chromium.org/2128093004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js:51: this._colorValueElement.style.display = "none"; Let's rather do: this._colorValueElement.hidden = hide;
https://codereview.chromium.org/2128093004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/externs.js (right): https://codereview.chromium.org/2128093004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/externs.js:589: CodeMirror.TextMarker = function(doc, type) { } On 2016/07/13 21:21:53, lushnikov wrote: > shouldn't it have a "clear" method? Yes. Added https://codereview.chromium.org/2128093004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js (right): https://codereview.chromium.org/2128093004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:137: On 2016/07/13 21:21:53, lushnikov wrote: > lets kill this new line Done. https://codereview.chromium.org/2128093004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:141: var line = text.lineAt(lineNumber) + "\n"; On 2016/07/13 21:21:53, lushnikov wrote: > why do you need "\n" in the end? The Regex also makes sure that the character following the color text is valid, [\s;,(){}]. lineAt removes the newline when it returns the line, so I need to add it back so that the Regex can match the red in "color: red\n" but not the red in "color: red-ly" https://codereview.chromium.org/2128093004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:142: On 2016/07/13 21:21:53, lushnikov wrote: > let's remove this new line Done. https://codereview.chromium.org/2128093004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:145: On 2016/07/13 21:21:53, lushnikov wrote: > let's remove this new line as well Done. https://codereview.chromium.org/2128093004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:190: onTextEditorContentLoaded: function() On 2016/07/13 21:21:53, lushnikov wrote: > let's rather override onTextChanged method from UISourceCodeFrame instead of > overriding this method > and listening to UISOurceCode events! > > With this in hand, you will eliminate a race condition between UISourceCode > events and TextEditor updates Switched to override onTextEditorContentLoaded and onTextChanged, rather than listening to uiSourceCode WorkingCopyChanged. https://codereview.chromium.org/2128093004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:197: dispose: function() On 2016/07/13 21:21:53, lushnikov wrote: > you will not need this method as you'll stop listening to UISourceCode! Done. Removed. It should clean up appropriately without this. https://codereview.chromium.org/2128093004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js (right): https://codereview.chromium.org/2128093004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js:51: this._colorValueElement.style.display = "none"; On 2016/07/13 21:21:53, lushnikov wrote: > Let's rather do: > > this._colorValueElement.hidden = hide; Done.
https://codereview.chromium.org/2128093004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js (right): https://codereview.chromium.org/2128093004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:42: if (Runtime.experiments.isEnabled("sourceColorPicker")) let's not check for experiment here - having empty this._colorBookmarks deals no harm https://codereview.chromium.org/2128093004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:112: var colorPositions = this._extractColorPositions(this._uiSourceCode.workingCopy()); don't go for content of uiSourceCode, use textEditor instead (uiSourceCode might be out of sync with editor) https://codereview.chromium.org/2128093004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:130: var colorRegex = /[\s:;,(){}]((?:rgb|hsl)a?\([^)]+\)|#[0-9a-f]{8}|#[0-9a-f]{6}|#[0-9a-f]{3,4}|[a-z]+)(?=[\s;,(){}])/gi; would the \b work for you instead of "[\s:;,(){}]"? This way you would be able to avoid "\n" as well.
https://codereview.chromium.org/2128093004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js (right): https://codereview.chromium.org/2128093004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:42: if (Runtime.experiments.isEnabled("sourceColorPicker")) On 2016/07/13 23:31:42, lushnikov wrote: > let's not check for experiment here - having empty this._colorBookmarks deals no > harm Done. https://codereview.chromium.org/2128093004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:112: var colorPositions = this._extractColorPositions(this._uiSourceCode.workingCopy()); On 2016/07/13 23:31:42, lushnikov wrote: > don't go for content of uiSourceCode, use textEditor instead (uiSourceCode might > be out of sync with editor) Done. https://codereview.chromium.org/2128093004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:130: var colorRegex = /[\s:;,(){}]((?:rgb|hsl)a?\([^)]+\)|#[0-9a-f]{8}|#[0-9a-f]{6}|#[0-9a-f]{3,4}|[a-z]+)(?=[\s;,(){}])/gi; On 2016/07/13 23:31:42, lushnikov wrote: > would the \b work for you instead of "[\s:;,(){}]"? This way you would be able > to avoid "\n" as well. \b is not strict enough because it would match the "red" in "url(http://www.red.com)" for example.
lgtm On a side note, maybe there's a way to unify your color regex with the one from SSP? Looks like you solve similar problems (in a separate patch) https://codereview.chromium.org/2128093004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js (right): https://codereview.chromium.org/2128093004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:128: var colorRegex = /[\s:;,(){}]((?:rgb|hsl)a?\([^)]+\)|#[0-9a-f]{8}|#[0-9a-f]{6}|#[0-9a-f]{3,4}|[a-z]+)(?=[\s;,(){}])/gi; let's extract this out of the for-loop for now
Description was changed from ========== DevTools: Added color swatches to the gutter of style sheet sources. Color swatches help to visualize the code and will later launch color pickers to edit the colors. Rough implementation. Color swatches are not currently clickable. Do not push! BUG=266326 ========== to ========== DevTools: Add color swatches to the Sources panel when editing style sheets. Color swatches help to visualize the code and will later launch color pickers to edit the colors. BUG=266326 ==========
https://codereview.chromium.org/2128093004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js (right): https://codereview.chromium.org/2128093004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:128: var colorRegex = /[\s:;,(){}]((?:rgb|hsl)a?\([^)]+\)|#[0-9a-f]{8}|#[0-9a-f]{6}|#[0-9a-f]{3,4}|[a-z]+)(?=[\s;,(){}])/gi; On 2016/07/14 01:20:05, lushnikov wrote: > let's extract this out of the for-loop for now Done.
The CQ bit was checked by flandy@google.com
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/2128093004/#ps120001 (title: "First iteration")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== DevTools: Add color swatches to the Sources panel when editing style sheets. Color swatches help to visualize the code and will later launch color pickers to edit the colors. BUG=266326 ========== to ========== DevTools: Add color swatches to the Sources panel when editing style sheets. Color swatches help to visualize the code and will later launch color pickers to edit the colors. BUG=266326 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== DevTools: Add color swatches to the Sources panel when editing style sheets. Color swatches help to visualize the code and will later launch color pickers to edit the colors. BUG=266326 ========== to ========== DevTools: Add color swatches to the Sources panel when editing style sheets. Color swatches help to visualize the code and will later launch color pickers to edit the colors. BUG=266326 Committed: https://crrev.com/6a382a152026aa3f48a1e94ae89c9d83f37cd09e Cr-Commit-Position: refs/heads/master@{#405423} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/6a382a152026aa3f48a1e94ae89c9d83f37cd09e Cr-Commit-Position: refs/heads/master@{#405423} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
