Bug 124364 – Web Inspector: allow editing of colors in CSS resources

• Component: Web Inspector
• Status: RESOLVED FIXED

[Antoine Quint]

Right now the color picker only gets shown when editing in the Style sidebar. We should also allow colors in a CSS resource source code to be edited using a color picker. See https://bugs.webkit.org/show_bug.cgi?id=124363 for a potential solution for this.

[Radar WebKit Bug Importer]

<rdar://problem/15470062>

[Antoine Quint]

Created attachment 218440 [details]
Screen recording showing color editing in a CSS resource in action

[Antoine Quint]

Created attachment 218518 [details]
Patch

[WebKit Commit Bot]

Attachment 218518 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebInspectorUI/ChangeLog', u'Source/WebInspectorUI/UserInterface/CSSStyleDeclarationTextEditor.js', u'Source/WebInspectorUI/UserInterface/CodeMirrorAdditions.js', u'Source/WebInspectorUI/UserInterface/CodeMirrorTokenTrackingController.js', u'Source/WebInspectorUI/UserInterface/ColorMarkerEditingController.js', u'Source/WebInspectorUI/UserInterface/Images/ColorIcon.png', u'Source/WebInspectorUI/UserInterface/Images/[e-mail]', u'Source/WebInspectorUI/UserInterface/Main.html', u'Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.css', u'Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js', u'Source/WebInspectorUI/UserInterface/TextEditor.js', '--commit-queue']" exit_code: 1
Traceback (most recent call last):
  File "Tools/Scripts/check-webkit-style", line 48, in <module>
    sys.exit(CheckWebKitStyle().main())
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/main.py", line 154, in main
    patch_checker.check(patch)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/patchreader.py", line 71, in check
    self._text_file_reader.process_file(file_path=path, line_numbers=None)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/filereader.py", line 118, in process_file
    lines = self._read_lines(file_path)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/filereader.py", line 86, in _read_lines
    contents = file.read()
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/codecs.py", line 671, in read
    return self.reader.read(size)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/codecs.py", line 477, in read
    newchars, decodedbytes = self.decode(data, self.errors)
UnicodeDecodeError: 'utf8' codec can't decode byte 0x89 in position 0: invalid start byte


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Comment on attachment 218518 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=218518&action=review

This patch looks really clean! But I think Timothy Hatcher would say that TextEditor should not expose CodeMirror objects / types outside of it. TextEditor is an abstraction of an editor, and exposing it's internal "CodeMirror" stuff to its clients is a layering violation (e.g. TextEditor.contentDidChange). I'd like to see what he has to say about that aspect of this patch.

> Source/WebInspectorUI/ChangeLog:16
> +        Remove the code that goes through the lines of the CodeMirror editor to look for
> +        color strings and replace it with a call to the .createColorMarkers() CodeMirror
> +        extension in which the code was refactored. The callback passed to
> +        .createColorMarkers() handles the CSSStyleDeclarationTextEditor-specific creation

Awesome

> Source/WebInspectorUI/ChangeLog:56
> +        For the moment, use the same behavior as the existing "MarkedTokens" mode where

Incomplete sentence.

> Source/WebInspectorUI/UserInterface/CodeMirrorTokenTrackingController.js:43
> +    this.hoveredMarker = null;

I think we should stick to getters / setters for properties for now as that is the convention. So this would be this._hoveredMarker.

> Source/WebInspectorUI/UserInterface/CodeMirrorTokenTrackingController.js:297
> +            if (this.hoveredMarker && token && this._delegate && typeof this._delegate.tokenTrackingControllerMouseOutOfHoveredMarker === "function") {

Is the "&& token" part here necessary, or is it an optimization?

> Source/WebInspectorUI/UserInterface/ColorMarkerEditingController.js:34
> +    this.marker = marker;
> +    this._delegate = delegate;
> +
> +    this.range = marker.find();

Same here, this._marker and this._range with getters and optional setters. Unless I missed a decision we made to move to raw public properties.

> Source/WebInspectorUI/UserInterface/ColorMarkerEditingController.js:67
> +        this._setColor(this._originalColor);
> +        this._popover.dismiss();

Would it be necessary here to stopPropagation / preventDefault for this handling of Esc?

> Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:1030
> +        for (var i = 0; i < markers.length; ++i) {
> +            if (markers[i].__color)
> +                return true;
> +        }

You could use the new for..of loop!

    for (var marker of markers) {
        if (marker.__color)
            return true;
    }

> Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:1317
> +        markers.some(function(marker) {
> +            if (!marker.__markedColor)
> +                return false;
> +
> +            colorMarker = marker;
> +            return true;
> +        });

Shrug. A simple for..of loop with a break seems simpler then Array.prototype.some, especially because you're ignoring the result of some().

    for (var marker of markers) {
        if (marker.__markedColor) {
            colorMarker = marker;
            break;
        }
    }

> Source/WebInspectorUI/UserInterface/TextEditor.js:597
> +    contentDidChange: function(codeMirror, change)
> +    {
> +        // Implemented by subclasses.
> +    },

This is the layering violation I'm thinking of.

[Antoine Quint]

(In reply to comment #5)
> (From update of attachment 218518 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218518&action=review
> 
> This patch looks really clean! But I think Timothy Hatcher would say that TextEditor should not expose CodeMirror objects / types outside of it. TextEditor is an abstraction of an editor, and exposing it's internal "CodeMirror" stuff to its clients is a layering violation (e.g. TextEditor.contentDidChange). I'd like to see what he has to say about that aspect of this patch.

Me too!

> > Source/WebInspectorUI/ChangeLog:56
> > +        For the moment, use the same behavior as the existing "MarkedTokens" mode where
> 
> Incomplete sentence.

I think there should be no "where" and we should just end the sentence after "mode".

> > Source/WebInspectorUI/UserInterface/CodeMirrorTokenTrackingController.js:43
> > +    this.hoveredMarker = null;
> 
> I think we should stick to getters / setters for properties for now as that is the convention. So this would be this._hoveredMarker.

OK.

> > Source/WebInspectorUI/UserInterface/CodeMirrorTokenTrackingController.js:297
> > +            if (this.hoveredMarker && token && this._delegate && typeof this._delegate.tokenTrackingControllerMouseOutOfHoveredMarker === "function") {
> 
> Is the "&& token" part here necessary, or is it an optimization?

I seem to recall that there were situations where there was no token returned by calling this._codeMirror.getTokenAt(position) and it was necessary to check for this since we later get token.start. 

> > Source/WebInspectorUI/UserInterface/ColorMarkerEditingController.js:34
> > +    this.marker = marker;
> > +    this._delegate = delegate;
> > +
> > +    this.range = marker.find();
> 
> Same here, this._marker and this._range with getters and optional setters. Unless I missed a decision we made to move to raw public properties.

OK.

> > Source/WebInspectorUI/UserInterface/ColorMarkerEditingController.js:67
> > +        this._setColor(this._originalColor);
> > +        this._popover.dismiss();
> 
> Would it be necessary here to stopPropagation / preventDefault for this handling of Esc?

It didn't seem like it during my testing (ie. the quick console didn't show) but it sounds like we ought to.

> > Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:1030
> > +        for (var i = 0; i < markers.length; ++i) {
> > +            if (markers[i].__color)
> > +                return true;
> > +        }
> 
> You could use the new for..of loop!
> 
>     for (var marker of markers) {
>         if (marker.__color)
>             return true;
>     }

Hmm-hmm!

> > Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:1317
> > +        markers.some(function(marker) {
> > +            if (!marker.__markedColor)
> > +                return false;
> > +
> > +            colorMarker = marker;
> > +            return true;
> > +        });
> 
> Shrug. A simple for..of loop with a break seems simpler then Array.prototype.some, especially because you're ignoring the result of some().
> 
>     for (var marker of markers) {
>         if (marker.__markedColor) {
>             colorMarker = marker;
>             break;
>         }
>     }

Agreed.

> > Source/WebInspectorUI/UserInterface/TextEditor.js:597
> > +    contentDidChange: function(codeMirror, change)
> > +    {
> > +        // Implemented by subclasses.
> > +    },
> 
> This is the layering violation I'm thinking of.

Yeah, I wasn't sure about this. There is a WebInspector.TextEditor.Event.ContentDidChange triggered by TextEditor but it felt weird for a subclass to consume events of its superclass and it lacked the "change" object necessary to inspect the editing change. I'm sure Tim will have an opinion on the best way to implement this.

[Timothy Hatcher]

Comment on attachment 218518 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=218518&action=review

This is heading in the right direction.

> Source/WebInspectorUI/UserInterface/CodeMirrorAdditions.js:552
> +WebInspector.codeMirrorRangesOverlap = function(aRangeStart, aRangeEnd, bRangeStart, bRangeEnd)
> +{
> +    function isPositionInRange(position, rangeStart, rangeEnd)
> +    {
> +        return position.line >= rangeStart.line && position.ch >= rangeStart.ch &&
> +               position.line <= rangeEnd.line && position.ch <= rangeEnd.ch;
> +    }
> +
> +    return isPositionInRange(aRangeStart, bRangeStart, bRangeEnd) || isPositionInRange(aRangeEnd, bRangeStart, bRangeEnd) ||
> +           isPositionInRange(bRangeStart, aRangeStart, aRangeEnd) || isPositionInRange(bRangeEnd, aRangeStart, aRangeEnd);
> +};

Would be better as an extension in CodeMirrorAdditions.js. We try not to leak CodeMirror knowledge out into other general Inspector files beyond CodeMirror specific classes — incase we ever adopt something else.

>>> Source/WebInspectorUI/UserInterface/CodeMirrorTokenTrackingController.js:297
>>> +            if (this.hoveredMarker && token && this._delegate && typeof this._delegate.tokenTrackingControllerMouseOutOfHoveredMarker === "function") {
>> 
>> Is the "&& token" part here necessary, or is it an optimization?
> 
> I seem to recall that there were situations where there was no token returned by calling this._codeMirror.getTokenAt(position) and it was necessary to check for this since we later get token.start.

Then can/should you use position instead of token.start?

> Source/WebInspectorUI/UserInterface/ColorMarkerEditingController.js:26
> +WebInspector.ColorMarkerEditingController = function(codeMirror, marker, delegate)

We try to put CodeMirror in the names of classes that work with CodeMirror. So this should be CodeMirrorColorMarkerEditingController or maybe just CodeMirrorColorEditingController.

> Source/WebInspectorUI/UserInterface/ColorMarkerEditingController.js:64
> +    handleEvent: function(event)
> +    {
> +        if (event.keyCode !== WebInspector.KeyboardShortcut.Key.Escape.keyCode || !this._popover.visible)
> +            return;

You should use WebInspector.KeyboardShortcut instead of manual keydown listening. That will avoid the conflict with the quick console too.

> Source/WebInspectorUI/UserInterface/ColorMarkerEditingController.js:70
> +    hoverMenuWasActivated: function(hoverMenu)

This callback sounds more like the hover menu was shown, not the button on the hover menu was pressed. Perhaps rename this? hoverMenuButtonWasPressed?

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

This is the first use of CodeMirror directly in this class. TextEditor should be the only user of CodeMirror.

> Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:236
> +        codeMirror.getAllMarks().forEach(function(marker) {
> +            var position = marker.find();
> +            if (WebInspector.codeMirrorRangesOverlap(position.from, position.to, change.from, change.to))
> +                marker.clear();
> +        });

This is expensive. contentDidChange is fired often and needs to be fast and localized. codeMirror.getAllMarks will get marks for the whole document, and for large color heavy resources than can be a lot and marker.find() is not cheap in CodeMirror. You also might end up clearing markers that should not be cleared. Also find() can return null if the marker isn't in the document anymore, so you need to null check position.

Also a CodeMirror change can contain multiple changes, with `change` being a linked list (having a change.next), so you need to walk that list to get all changes.

With all that said, why is this needed? CodeMirror will clear markers if an edit deletes them. If we need to update stale colors, createColorMarkers might be the better place to do that.

> Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:243
> +        if (change.text.some(notEmpty)) {

Again, change is a linked list and might have multiple changes. Don't you also need to call _updateTokenTrackingControllerState here if this is the first color added to change from None mode to MarkedTokens mode?

> Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:246
> +            var endLine = startLine + change.text.length;

You should use change.to.line instead of change.text.length.

> Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:250
> +    },

With all that said, I think processing the CodeMirror change objects and trying to be smart can be very very hard to get right. I even got it wrong in WebInspector.CSSStyleDeclarationTextEditor._contentChanged by only using change.from and ignoring change.next and change.to.

We should iterate over the change linked list, call _updateColorMarkers for all the lines between change.from and change.to. _updateColorMarkers should clear or reuse color markers for the line as needed. At the end of that change linked list loop, call _updateTokenTrackingControllerState and maybe _dismissColorMarkerEditingController if needed. Trying to infer from removed or text what to do seems ripe for odd edge case bugs.

Finding a way to move this into TextEditor with more localized callbacks per changed line or text range would be best.

> Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:988
> +        var modes = WebInspector.CodeMirrorTokenTrackingController.Mode;

We try no to use shortcuts for constants so find and replace can find active uses of them. This makes it hard to find them in the future.

> Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:1301
> +        var colorMarkers = this._codeMirror.createColorMarkers(lineNumber, function(marker, color, colorString) {

This uses this._codeMirror, which is private to TextEditor. CodeMirrorColorEditingController should do this work.

>>> Source/WebInspectorUI/UserInterface/TextEditor.js:597
>>> +    },
>> 
>> This is the layering violation I'm thinking of.
> 
> Yeah, I wasn't sure about this. There is a WebInspector.TextEditor.Event.ContentDidChange triggered by TextEditor but it felt weird for a subclass to consume events of its superclass and it lacked the "change" object necessary to inspect the editing change. I'm sure Tim will have an opinion on the best way to implement this.

Yeah, we do try to leak CodeMirror outside of TextEditor. The function is right, just not the objects passed to it.

I would have TextEditor do the change linked list walk I mentioned above and generate a WebInspector.TextRange or array of WebInspector.TextRanges. Then call this.contentDidChange() with that range. The subclass only cares about the lines that need updated by CodeMirrorColorEditingController.

[Antoine Quint]

(In reply to comment #7)
> (From update of attachment 218518 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218518&action=review
> 
> This is heading in the right direction.
> 
> > Source/WebInspectorUI/UserInterface/CodeMirrorAdditions.js:552
> > +WebInspector.codeMirrorRangesOverlap = function(aRangeStart, aRangeEnd, bRangeStart, bRangeEnd)
> > +{
> > +    function isPositionInRange(position, rangeStart, rangeEnd)
> > +    {
> > +        return position.line >= rangeStart.line && position.ch >= rangeStart.ch &&
> > +               position.line <= rangeEnd.line && position.ch <= rangeEnd.ch;
> > +    }
> > +
> > +    return isPositionInRange(aRangeStart, bRangeStart, bRangeEnd) || isPositionInRange(aRangeEnd, bRangeStart, bRangeEnd) ||
> > +           isPositionInRange(bRangeStart, aRangeStart, aRangeEnd) || isPositionInRange(bRangeEnd, aRangeStart, aRangeEnd);
> > +};
> 
> Would be better as an extension in CodeMirrorAdditions.js. We try not to leak CodeMirror knowledge out into other general Inspector files beyond CodeMirror specific classes — incase we ever adopt something else.

OK, that makes sense.

> >>> Source/WebInspectorUI/UserInterface/CodeMirrorTokenTrackingController.js:297
> >>> +            if (this.hoveredMarker && token && this._delegate && typeof this._delegate.tokenTrackingControllerMouseOutOfHoveredMarker === "function") {
> >> 
> >> Is the "&& token" part here necessary, or is it an optimization?
> > 
> > I seem to recall that there were situations where there was no token returned by calling this._codeMirror.getTokenAt(position) and it was necessary to check for this since we later get token.start.
> 
> Then can/should you use position instead of token.start?

Probably! I'll try that.

> > Source/WebInspectorUI/UserInterface/ColorMarkerEditingController.js:26
> > +WebInspector.ColorMarkerEditingController = function(codeMirror, marker, delegate)
> 
> We try to put CodeMirror in the names of classes that work with CodeMirror. So this should be CodeMirrorColorMarkerEditingController or maybe just CodeMirrorColorEditingController.

CodeMirrorColorEditingController it is.

> > Source/WebInspectorUI/UserInterface/ColorMarkerEditingController.js:64
> > +    handleEvent: function(event)
> > +    {
> > +        if (event.keyCode !== WebInspector.KeyboardShortcut.Key.Escape.keyCode || !this._popover.visible)
> > +            return;
> 
> You should use WebInspector.KeyboardShortcut instead of manual keydown listening. That will avoid the conflict with the quick console too.

Cool, will try that.

> > Source/WebInspectorUI/UserInterface/ColorMarkerEditingController.js:70
> > +    hoverMenuWasActivated: function(hoverMenu)
> 
> This callback sounds more like the hover menu was shown, not the button on the hover menu was pressed. Perhaps rename this? hoverMenuButtonWasPressed?

Will do.

> > Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:220
> > +    contentDidChange: function(codeMirror, change)
> 
> This is the first use of CodeMirror directly in this class. TextEditor should be the only user of CodeMirror.
> 
> > Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:236
> > +        codeMirror.getAllMarks().forEach(function(marker) {
> > +            var position = marker.find();
> > +            if (WebInspector.codeMirrorRangesOverlap(position.from, position.to, change.from, change.to))
> > +                marker.clear();
> > +        });
> 
> This is expensive. contentDidChange is fired often and needs to be fast and localized. codeMirror.getAllMarks will get marks for the whole document, and for large color heavy resources than can be a lot and marker.find() is not cheap in CodeMirror. You also might end up clearing markers that should not be cleared. Also find() can return null if the marker isn't in the document anymore, so you need to null check position.
> 
> Also a CodeMirror change can contain multiple changes, with `change` being a linked list (having a change.next), so you need to walk that list to get all changes.
> 
> With all that said, why is this needed? CodeMirror will clear markers if an edit deletes them. If we need to update stale colors, createColorMarkers might be the better place to do that.

Yeah, I think I'll add the ability to remove stale color markers in createColorMarkers.

> > Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:243
> > +        if (change.text.some(notEmpty)) {
> 
> Again, change is a linked list and might have multiple changes. Don't you also need to call _updateTokenTrackingControllerState here if this is the first color added to change from None mode to MarkedTokens mode?

The call to _updateColorMarkers will handle the call to _updateTokenTrackingControllerState.

> > Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:246
> > +            var endLine = startLine + change.text.length;
> 
> You should use change.to.line instead of change.text.length.

Will do.

> > Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:250
> > +    },
> 
> With all that said, I think processing the CodeMirror change objects and trying to be smart can be very very hard to get right. I even got it wrong in WebInspector.CSSStyleDeclarationTextEditor._contentChanged by only using change.from and ignoring change.next and change.to.
> 
> We should iterate over the change linked list, call _updateColorMarkers for all the lines between change.from and change.to. _updateColorMarkers should clear or reuse color markers for the line as needed. At the end of that change linked list loop, call _updateTokenTrackingControllerState and maybe _dismissColorMarkerEditingController if needed. Trying to infer from removed or text what to do seems ripe for odd edge case bugs.
> 
> Finding a way to move this into TextEditor with more localized callbacks per changed line or text range would be best.

OK, I will give that a shot.

> > Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:988
> > +        var modes = WebInspector.CodeMirrorTokenTrackingController.Mode;
> 
> We try no to use shortcuts for constants so find and replace can find active uses of them. This makes it hard to find them in the future.

Good point, will change that.

> > Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:1301
> > +        var colorMarkers = this._codeMirror.createColorMarkers(lineNumber, function(marker, color, colorString) {
> 
> This uses this._codeMirror, which is private to TextEditor. CodeMirrorColorEditingController should do this work.

OK. I suppose it'll be OK for CodeMirrorColorEditingController to be passed a reference to this._codeMirror, right?

> >>> Source/WebInspectorUI/UserInterface/TextEditor.js:597
> >>> +    },
> >> 
> >> This is the layering violation I'm thinking of.
> > 
> > Yeah, I wasn't sure about this. There is a WebInspector.TextEditor.Event.ContentDidChange triggered by TextEditor but it felt weird for a subclass to consume events of its superclass and it lacked the "change" object necessary to inspect the editing change. I'm sure Tim will have an opinion on the best way to implement this.
> 
> Yeah, we do try to leak CodeMirror outside of TextEditor. The function is right, just not the objects passed to it.
> 
> I would have TextEditor do the change linked list walk I mentioned above and generate a WebInspector.TextRange or array of WebInspector.TextRanges. Then call this.contentDidChange() with that range. The subclass only cares about the lines that need updated by CodeMirrorColorEditingController.

Good point, I will look into this.

[Antoine Quint]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 218518 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=218518&action=review
> > 
> > This is heading in the right direction.
> > 
> > > Source/WebInspectorUI/UserInterface/CodeMirrorAdditions.js:552
> > > +WebInspector.codeMirrorRangesOverlap = function(aRangeStart, aRangeEnd, bRangeStart, bRangeEnd)
> > > +{
> > > +    function isPositionInRange(position, rangeStart, rangeEnd)
> > > +    {
> > > +        return position.line >= rangeStart.line && position.ch >= rangeStart.ch &&
> > > +               position.line <= rangeEnd.line && position.ch <= rangeEnd.ch;
> > > +    }
> > > +
> > > +    return isPositionInRange(aRangeStart, bRangeStart, bRangeEnd) || isPositionInRange(aRangeEnd, bRangeStart, bRangeEnd) ||
> > > +           isPositionInRange(bRangeStart, aRangeStart, aRangeEnd) || isPositionInRange(bRangeEnd, aRangeStart, aRangeEnd);
> > > +};
> > 
> > Would be better as an extension in CodeMirrorAdditions.js. We try not to leak CodeMirror knowledge out into other general Inspector files beyond CodeMirror specific classes — incase we ever adopt something else.
> 
> OK, that makes sense.

Actually, this is already in CodeMirrorAdditions.js as WebInspector.codeMirrorRangesOverlap. Or did you mean an actual extension method on CodeMirror instances? I think this makes more sense as a static method since it makes no use of a CodeMirror instance.

[Antoine Quint]

> > > Source/WebInspectorUI/UserInterface/ColorMarkerEditingController.js:64
> > > +    handleEvent: function(event)
> > > +    {
> > > +        if (event.keyCode !== WebInspector.KeyboardShortcut.Key.Escape.keyCode || !this._popover.visible)
> > > +            return;
> > 
> > You should use WebInspector.KeyboardShortcut instead of manual keydown listening. That will avoid the conflict with the quick console too.
> 
> Cool, will try that.

I might be missing something, but this isn't working for me:

    this._keyboardShortcutEsc = new WebInspector.KeyboardShortcut(null, WebInspector.KeyboardShortcut.Key.Escape, this._escapeKeyWasPressed.bind(this), window);

This doesn't prevent the quick console from showing up while my previous technique does.

[Antoine Quint]

> > > Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:236
> > > +        codeMirror.getAllMarks().forEach(function(marker) {
> > > +            var position = marker.find();
> > > +            if (WebInspector.codeMirrorRangesOverlap(position.from, position.to, change.from, change.to))
> > > +                marker.clear();
> > > +        });
> > 
> > This is expensive. contentDidChange is fired often and needs to be fast and localized. codeMirror.getAllMarks will get marks for the whole document, and for large color heavy resources than can be a lot and marker.find() is not cheap in CodeMirror. You also might end up clearing markers that should not be cleared. Also find() can return null if the marker isn't in the document anymore, so you need to null check position.
> > 
> > Also a CodeMirror change can contain multiple changes, with `change` being a linked list (having a change.next), so you need to walk that list to get all changes.
> > 
> > With all that said, why is this needed? CodeMirror will clear markers if an edit deletes them. If we need to update stale colors, createColorMarkers might be the better place to do that.
> 
> Yeah, I think I'll add the ability to remove stale color markers in createColorMarkers.

Actually, I'm not sure how to efficiently detect stale color markers. Wouldn't I still need to work through getAllMarks()? Or I guess I could keep track of color markers myself and see if they're still valid when createColorMarkers() is called. The idea is that if the author is manually editing a color with text input and making it so that it's no longer a valid color, we remove the text marker.

[Timothy Hatcher]

Comment on attachment 218518 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=218518&action=review

This is heading in the right direction.

>>>> Source/WebInspectorUI/UserInterface/CodeMirrorAdditions.js:552
>>>> +WebInspector.codeMirrorRangesOverlap = function(aRangeStart, aRangeEnd, bRangeStart, bRangeEnd)
>>>> +{
>>>> +    function isPositionInRange(position, rangeStart, rangeEnd)
>>>> +    {
>>>> +        return position.line >= rangeStart.line && position.ch >= rangeStart.ch &&
>>>> +               position.line <= rangeEnd.line && position.ch <= rangeEnd.ch;
>>>> +    }
>>>> +
>>>> +    return isPositionInRange(aRangeStart, bRangeStart, bRangeEnd) || isPositionInRange(aRangeEnd, bRangeStart, bRangeEnd) ||
>>>> +           isPositionInRange(bRangeStart, aRangeStart, aRangeEnd) || isPositionInRange(bRangeEnd, aRangeStart, aRangeEnd);
>>>> +};
>>> 
>>> Would be better as an extension in CodeMirrorAdditions.js. We try not to leak CodeMirror knowledge out into other general Inspector files beyond CodeMirror specific classes — incase we ever adopt something else.
>> 
>> OK, that makes sense.
> 
> Actually, this is already in CodeMirrorAdditions.js as WebInspector.codeMirrorRangesOverlap. Or did you mean an actual extension method on CodeMirror instances? I think this makes more sense as a static method since it makes no use of a CodeMirror instance.

Would be better as an extension in CodeMirrorAdditions.js. We try not to leak CodeMirror knowledge out into other general Inspector files beyond CodeMirror specific classes — incase we ever adopt something else.

>>>>> Source/WebInspectorUI/UserInterface/CodeMirrorTokenTrackingController.js:297
>>>>> +            if (this.hoveredMarker && token && this._delegate && typeof this._delegate.tokenTrackingControllerMouseOutOfHoveredMarker === "function") {
>>>> 
>>>> Is the "&& token" part here necessary, or is it an optimization?
>>> 
>>> I seem to recall that there were situations where there was no token returned by calling this._codeMirror.getTokenAt(position) and it was necessary to check for this since we later get token.start.
>> 
>> Then can/should you use position instead of token.start?
> 
> Probably! I'll try that.

Then can/should you use position instead of token.start?

>>> Source/WebInspectorUI/UserInterface/ColorMarkerEditingController.js:26
>>> +WebInspector.ColorMarkerEditingController = function(codeMirror, marker, delegate)
>> 
>> We try to put CodeMirror in the names of classes that work with CodeMirror. So this should be CodeMirrorColorMarkerEditingController or maybe just CodeMirrorColorEditingController.
> 
> CodeMirrorColorEditingController it is.

We try to put CodeMirror in the names of classes that work with CodeMirror. So this should be CodeMirrorColorMarkerEditingController or maybe just CodeMirrorColorEditingController.

>>> Source/WebInspectorUI/UserInterface/ColorMarkerEditingController.js:64
>>> +    handleEvent: function(event)
>>> +    {
>>> +        if (event.keyCode !== WebInspector.KeyboardShortcut.Key.Escape.keyCode || !this._popover.visible)
>>> +            return;
>> 
>> You should use WebInspector.KeyboardShortcut instead of manual keydown listening. That will avoid the conflict with the quick console too.
> 
> Cool, will try that.

You should use WebInspector.KeyboardShortcut instead of manual keydown listening. That will avoid the conflict with the quick console too.

>>> Source/WebInspectorUI/UserInterface/ColorMarkerEditingController.js:70
>>> +    hoverMenuWasActivated: function(hoverMenu)
>> 
>> This callback sounds more like the hover menu was shown, not the button on the hover menu was pressed. Perhaps rename this? hoverMenuButtonWasPressed?
> 
> Will do.

This callback sounds more like the hover menu was shown, not the button on the hover menu was pressed. Perhaps rename this? hoverMenuButtonWasPressed?

>>> Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:220
>>> +    contentDidChange: function(codeMirror, change)
>> 
>> This is the first use of CodeMirror directly in this class. TextEditor should be the only user of CodeMirror.
> 
> Yeah, I think I'll add the ability to remove stale color markers in createColorMarkers.

This is the first use of CodeMirror directly in this class. TextEditor should be the only user of CodeMirror.

>> Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:236
>> +        codeMirror.getAllMarks().forEach(function(marker) {
>> +            var position = marker.find();
>> +            if (WebInspector.codeMirrorRangesOverlap(position.from, position.to, change.from, change.to))
>> +                marker.clear();
>> +        });
> 
> This is expensive. contentDidChange is fired often and needs to be fast and localized. codeMirror.getAllMarks will get marks for the whole document, and for large color heavy resources than can be a lot and marker.find() is not cheap in CodeMirror. You also might end up clearing markers that should not be cleared. Also find() can return null if the marker isn't in the document anymore, so you need to null check position.
> 
> Also a CodeMirror change can contain multiple changes, with `change` being a linked list (having a change.next), so you need to walk that list to get all changes.
> 
> With all that said, why is this needed? CodeMirror will clear markers if an edit deletes them. If we need to update stale colors, createColorMarkers might be the better place to do that.

This is expensive. contentDidChange is fired often and needs to be fast and localized. codeMirror.getAllMarks will get marks for the whole document, and for large color heavy resources than can be a lot and marker.find() is not cheap in CodeMirror. You also might end up clearing markers that should not be cleared. Also find() can return null if the marker isn't in the document anymore, so you need to null check position.

Also a CodeMirror change can contain multiple changes, with `change` being a linked list (having a change.next), so you need to walk that list to get all changes.

With all that said, why is this needed? CodeMirror will clear markers if an edit deletes them. If we need to update stale colors, createColorMarkers might be the better place to do that.

>>> Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:243
>>> +        if (change.text.some(notEmpty)) {
>> 
>> Again, change is a linked list and might have multiple changes. Don't you also need to call _updateTokenTrackingControllerState here if this is the first color added to change from None mode to MarkedTokens mode?
> 
> The call to _updateColorMarkers will handle the call to _updateTokenTrackingControllerState.

Again, change is a linked list and might have multiple changes. Don't you also need to call _updateTokenTrackingControllerState here if this is the first color added to change from None mode to MarkedTokens mode?

>>> Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:246
>>> +            var endLine = startLine + change.text.length;
>> 
>> You should use change.to.line instead of change.text.length.
> 
> Will do.

You should use change.to.line instead of change.text.length.

>>> Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:250
>>> +    },
>> 
>> With all that said, I think processing the CodeMirror change objects and trying to be smart can be very very hard to get right. I even got it wrong in WebInspector.CSSStyleDeclarationTextEditor._contentChanged by only using change.from and ignoring change.next and change.to.
>> 
>> We should iterate over the change linked list, call _updateColorMarkers for all the lines between change.from and change.to. _updateColorMarkers should clear or reuse color markers for the line as needed. At the end of that change linked list loop, call _updateTokenTrackingControllerState and maybe _dismissColorMarkerEditingController if needed. Trying to infer from removed or text what to do seems ripe for odd edge case bugs.
>> 
>> Finding a way to move this into TextEditor with more localized callbacks per changed line or text range would be best.
> 
> OK, I will give that a shot.

With all that said, I think processing the CodeMirror change objects and trying to be smart can be very very hard to get right. I even got it wrong in WebInspector.CSSStyleDeclarationTextEditor._contentChanged by only using change.from and ignoring change.next and change.to.

We should iterate over the change linked list, call _updateColorMarkers for all the lines between change.from and change.to. _updateColorMarkers should clear or reuse color markers for the line as needed. At the end of that change linked list loop, call _updateTokenTrackingControllerState and maybe _dismissColorMarkerEditingController if needed. Trying to infer from removed or text what to do seems ripe for odd edge case bugs.

Finding a way to move this into TextEditor with more localized callbacks per changed line or text range would be best.

>>> Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:988
>>> +        var modes = WebInspector.CodeMirrorTokenTrackingController.Mode;
>> 
>> We try no to use shortcuts for constants so find and replace can find active uses of them. This makes it hard to find them in the future.
> 
> Good point, will change that.

We try no to use shortcuts for constants so find and replace can find active uses of them. This makes it hard to find them in the future.

>>> Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:1301
>>> +        var colorMarkers = this._codeMirror.createColorMarkers(lineNumber, function(marker, color, colorString) {
>> 
>> This uses this._codeMirror, which is private to TextEditor. CodeMirrorColorEditingController should do this work.
> 
> OK. I suppose it'll be OK for CodeMirrorColorEditingController to be passed a reference to this._codeMirror, right?

Yes, that is fine and matches other CodeMirror*Controller classes we have.

>>>>> Source/WebInspectorUI/UserInterface/TextEditor.js:597
>>>>> +    },
>>>> 
>>>> This is the layering violation I'm thinking of.
>>> 
>>> Yeah, I wasn't sure about this. There is a WebInspector.TextEditor.Event.ContentDidChange triggered by TextEditor but it felt weird for a subclass to consume events of its superclass and it lacked the "change" object necessary to inspect the editing change. I'm sure Tim will have an opinion on the best way to implement this.
>> 
>> Yeah, we do try to leak CodeMirror outside of TextEditor. The function is right, just not the objects passed to it.
>> 
>> I would have TextEditor do the change linked list walk I mentioned above and generate a WebInspector.TextRange or array of WebInspector.TextRanges. Then call this.contentDidChange() with that range. The subclass only cares about the lines that need updated by CodeMirrorColorEditingController.
> 
> Good point, I will look into this.

Yeah, we do try to leak CodeMirror outside of TextEditor. The function is right, just not the objects passed to it.

I would have TextEditor do the change linked list walk I mentioned above and generate a WebInspector.TextRange or array of WebInspector.TextRanges. Then call this.contentDidChange() with that range. The subclass only cares about the lines that need updated by CodeMirrorColorEditingController.

[Timothy Hatcher]

Bah, the review tool sent all my feed back twice. I only meant to say:

Yes, that is fine and matches other CodeMirror*Controller classes we have.

[Timothy Hatcher]

Comment on attachment 218518 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=218518&action=review

>>>>> Source/WebInspectorUI/UserInterface/CodeMirrorAdditions.js:552
>>>>> +};
>>>> 
>>>> Would be better as an extension in CodeMirrorAdditions.js. We try not to leak CodeMirror knowledge out into other general Inspector files beyond CodeMirror specific classes — incase we ever adopt something else.
>>> 
>>> OK, that makes sense.
>> 
>> Actually, this is already in CodeMirrorAdditions.js as WebInspector.codeMirrorRangesOverlap. Or did you mean an actual extension method on CodeMirror instances? I think this makes more sense as a static method since it makes no use of a CodeMirror instance.
> 
> Would be better as an extension in CodeMirrorAdditions.js. We try not to leak CodeMirror knowledge out into other general Inspector files beyond CodeMirror specific classes — incase we ever adopt something else.

I only see WebInspector.compareCodeMirrorPositions. Having it standalone is fine.

[Timothy Hatcher]

(In reply to comment #10)
> > > > Source/WebInspectorUI/UserInterface/ColorMarkerEditingController.js:64
> > > > +    handleEvent: function(event)
> > > > +    {
> > > > +        if (event.keyCode !== WebInspector.KeyboardShortcut.Key.Escape.keyCode || !this._popover.visible)
> > > > +            return;
> > > 
> > > You should use WebInspector.KeyboardShortcut instead of manual keydown listening. That will avoid the conflict with the quick console too.
> > 
> > Cool, will try that.
> 
> I might be missing something, but this isn't working for me:
> 
>     this._keyboardShortcutEsc = new WebInspector.KeyboardShortcut(null, WebInspector.KeyboardShortcut.Key.Escape, this._escapeKeyWasPressed.bind(this), window);
> 
> This doesn't prevent the quick console from showing up while my previous technique does.

this._keyboardShortcutEsc = new WebInspector.KeyboardShortcut(null, WebInspector.KeyboardShortcut.Key.Escape);

Should be all you need. I think window was making it not work. Leaving it out will implicitly use document, just like QuickConsole. But QuickConsole registers first, so QuickConsole will likely get first crack at it. We might need to add a priority concept to WebInspector.KeyboardShortcut that WebInspector.KeyboardShortcut._handleKeyDown honors. Or make WebInspector.KeyboardShortcut._handleKeyDown loop over targetElement._keyboardShortcuts in reverse.

[Timothy Hatcher]

(In reply to comment #11)
> > > > Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:236
> > > > +        codeMirror.getAllMarks().forEach(function(marker) {
> > > > +            var position = marker.find();
> > > > +            if (WebInspector.codeMirrorRangesOverlap(position.from, position.to, change.from, change.to))
> > > > +                marker.clear();
> > > > +        });
> > > 
> > > This is expensive. contentDidChange is fired often and needs to be fast and localized. codeMirror.getAllMarks will get marks for the whole document, and for large color heavy resources than can be a lot and marker.find() is not cheap in CodeMirror. You also might end up clearing markers that should not be cleared. Also find() can return null if the marker isn't in the document anymore, so you need to null check position.
> > > 
> > > Also a CodeMirror change can contain multiple changes, with `change` being a linked list (having a change.next), so you need to walk that list to get all changes.
> > > 
> > > With all that said, why is this needed? CodeMirror will clear markers if an edit deletes them. If we need to update stale colors, createColorMarkers might be the better place to do that.
> > 
> > Yeah, I think I'll add the ability to remove stale color markers in createColorMarkers.
> 
> Actually, I'm not sure how to efficiently detect stale color markers. Wouldn't I still need to work through getAllMarks()? Or I guess I could keep track of color markers myself and see if they're still valid when createColorMarkers() is called. The idea is that if the author is manually editing a color with text input and making it so that it's no longer a valid color, we remove the text marker.

Yeah, thinking about it more there really is not an efficient way.

Maybe the best option is to not worry about stale color markers. If the user hovers a marker, check again if the text in the marker is still a color before showing the hover menu. If the color marker is not valid anymore, clear it. If it is a color continue showing the hover menu.

Basically only eagerly mark colors, but lazily clear them.

I think that will still work with CSSStyleDeclarationTextEditor, and not cause multiple swatches.

[Antoine Quint]

(In reply to comment #16)
> (In reply to comment #11)
> > > > > Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:236
> > > > > +        codeMirror.getAllMarks().forEach(function(marker) {
> > > > > +            var position = marker.find();
> > > > > +            if (WebInspector.codeMirrorRangesOverlap(position.from, position.to, change.from, change.to))
> > > > > +                marker.clear();
> > > > > +        });
> > > > 
> > > > This is expensive. contentDidChange is fired often and needs to be fast and localized. codeMirror.getAllMarks will get marks for the whole document, and for large color heavy resources than can be a lot and marker.find() is not cheap in CodeMirror. You also might end up clearing markers that should not be cleared. Also find() can return null if the marker isn't in the document anymore, so you need to null check position.
> > > > 
> > > > Also a CodeMirror change can contain multiple changes, with `change` being a linked list (having a change.next), so you need to walk that list to get all changes.
> > > > 
> > > > With all that said, why is this needed? CodeMirror will clear markers if an edit deletes them. If we need to update stale colors, createColorMarkers might be the better place to do that.
> > > 
> > > Yeah, I think I'll add the ability to remove stale color markers in createColorMarkers.
> > 
> > Actually, I'm not sure how to efficiently detect stale color markers. Wouldn't I still need to work through getAllMarks()? Or I guess I could keep track of color markers myself and see if they're still valid when createColorMarkers() is called. The idea is that if the author is manually editing a color with text input and making it so that it's no longer a valid color, we remove the text marker.
> 
> Yeah, thinking about it more there really is not an efficient way.
> 
> Maybe the best option is to not worry about stale color markers. If the user hovers a marker, check again if the text in the marker is still a color before showing the hover menu. If the color marker is not valid anymore, clear it. If it is a color continue showing the hover menu.
> 
> Basically only eagerly mark colors, but lazily clear them.

Sounds good.

> I think that will still work with CSSStyleDeclarationTextEditor, and not cause multiple swatches.

So we definitely want to keep swatches in CSSStyleDeclarationTextEditor? I kinda think the hover menu approach is better and equally discoverable… if you're going to edit a color, you'll hover it and find out, won't you? That way we have a unique system, and we'll use more and more of the hover menu approach.

[Timothy Hatcher]

> > I think that will still work with CSSStyleDeclarationTextEditor, and not cause multiple swatches.
> 
> So we definitely want to keep swatches in CSSStyleDeclarationTextEditor? I kinda think the hover menu approach is better and equally discoverable… if you're going to edit a color, you'll hover it and find out, won't you? That way we have a unique system, and we'll use more and more of the hover menu approach.

I think the swatches help users preview/spot colors and are expected at this point. If we remove them we likely will get developer push back. Clicking the swatch to get the picker might be fine to remove, but it also provides a way to toggle the format.

[Antoine Quint]

Created attachment 218998 [details]
Patch

[WebKit Commit Bot]

Attachment 218998 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebInspectorUI/ChangeLog', u'Source/WebInspectorUI/UserInterface/CSSStyleDeclarationTextEditor.js', u'Source/WebInspectorUI/UserInterface/CodeMirrorAdditions.js', u'Source/WebInspectorUI/UserInterface/CodeMirrorColorEditingController.js', u'Source/WebInspectorUI/UserInterface/CodeMirrorTokenTrackingController.js', u'Source/WebInspectorUI/UserInterface/Color.js', u'Source/WebInspectorUI/UserInterface/HoverMenu.js', u'Source/WebInspectorUI/UserInterface/Images/ColorIcon.png', u'Source/WebInspectorUI/UserInterface/Images/[e-mail]', u'Source/WebInspectorUI/UserInterface/Main.html', u'Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.css', u'Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js', u'Source/WebInspectorUI/UserInterface/TextEditor.js', '--commit-queue']" exit_code: 1
Traceback (most recent call last):
  File "Tools/Scripts/check-webkit-style", line 48, in <module>
    sys.exit(CheckWebKitStyle().main())
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/main.py", line 154, in main
    patch_checker.check(patch)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/patchreader.py", line 71, in check
    self._text_file_reader.process_file(file_path=path, line_numbers=None)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/filereader.py", line 118, in process_file
    lines = self._read_lines(file_path)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/filereader.py", line 86, in _read_lines
    contents = file.read()
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/codecs.py", line 671, in read
    return self.reader.read(size)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/codecs.py", line 477, in read
    newchars, decodedbytes = self.decode(data, self.errors)
UnicodeDecodeError: 'utf8' codec can't decode byte 0x89 in position 0: invalid start byte


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antoine Quint]

This new patch removes all use of CodeMirror in SourceCodeTextEditor and instead adds some new public APIs on TextEditor to expose some CodeMirror functionality to its subclasses. Additionally, contentDidChange now passes an array of WebInspector.TextRange objects and there's no mention of CodeMirror. I've also made all the changes suggested by Joe.

[Timothy Hatcher]

Comment on attachment 218998 [details]
Patch

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?

[Antoine Quint]

(In reply to comment #22)
> (From update of attachment 218998 [details])
> 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!

Good!

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

Will fix in commit.

> > 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.

That's true. I was wondering if we ought to have a CSS-specific SourceCodeTextEditor subclass, and a specific one for JS.

> > 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.

Absolutely, I'll tackle that in a followup patch.

> > 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?

Will do in the commit and deal with it ASAP.

[Antoine Quint]

(In reply to comment #22)
> I'd make getMarkers a getter too — get markers().

I think we might want to add a system where we can get markers only for a range or line, but until we do so, it'd be cleaner as a getter. I'll fix that in the commit.

[Antoine Quint]

Created attachment 219059 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 219059 [details]
Patch for landing

Clearing flags on attachment: 219059

Committed r160483: <http://trac.webkit.org/changeset/160483>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Timothy Hatcher]

Comment on attachment 218998 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=218998&action=review

>>> Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:986
>>> +            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.
> 
> That's true. I was wondering if we ought to have a CSS-specific SourceCodeTextEditor subclass, and a specific one for JS.

It does not need to be a subclass. It can be based on the mode CodeMirror is using.

[Antoine Quint]

Followup to add a TextMarker abstraction is https://bugs.webkit.org/show_bug.cgi?id=125695.