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

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at apple.com> has denied 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 218518: Patch
https://bugs.webkit.org/attachment.cgi?id=218518&action=review

------- Additional Comments from Timothy Hatcher <timothy at apple.com>
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.