[webkit-reviews] review granted: [Bug 124364] Web Inspector: allow editing of colors in CSS resources : [Attachment 218998] Patch

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at apple.com> has granted Antoine Quint
<graouts at apple.com>'s request for review:
Bug 124364: Web Inspector: allow editing of colors in CSS resources
https://bugs.webkit.org/show_bug.cgi?id=124364

Attachment 218998: Patch
https://bugs.webkit.org/attachment.cgi?id=218998&action=review

------- Additional Comments from Timothy Hatcher <timothy at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=218998&action=review


> Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:220
> +    contentDidChange: function(replacedRanges, newRanges)

I like this!

> Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:228
> +	   var lines = new Set();

No ().

> Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:229
> +	   for (var range of newRanges) {

No more forEach()!

> Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:986
> +	   if (WebInspector.debuggerManager.paused)
> +	       mode =
WebInspector.CodeMirrorTokenTrackingController.Mode.JavaScriptExpression;
> +	   else if (this._hasColorMarkers())
> +	       mode =
WebInspector.CodeMirrorTokenTrackingController.Mode.MarkedTokens;

This might need some work. If the resource is a CSS file, it should not matter
if the debugger is paused, we should still be able to edit colors. Also colors
should likely not match in anything but CSS resources.

> Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:1323
> +	       colorMarker.clear();

You do use .clear() here, which is a CodeMirror function. It is fine. We just
need to consider a TextMarker abstraction at some point.

> Source/WebInspectorUI/UserInterface/TextEditor.js:612
> +    getMarkers: function()
> +    {
> +	   return this._codeMirror.getAllMarks();
> +    },
> +
> +    findMarkersAtPosition: function(position)
> +    {
> +	   return this._codeMirror.findMarksAt(position);
> +    },

A little bit risky here since the callers might, and do use, CodeMirror
properties on the result objects — specifically clear. We might want our own
TextMarker classes used here someday. I'd make getMarkers a getter too — get
markers(). Exposing the CodeMirror objects here makes it harder to detangle
uses later. Can you add a FIXME here?