At the moment, the CodeMirrorColorEditingController encapsulates code both specific to editing of a color and generic to any editing of a value in a CodeMirror editor. In order to support editing of gradients (http://webkit.org/b/119686), we should create a new CodeMirrorEditingController superclass that splits out the generic code from CodeMirrorColorEditingController and reduce CodeMirrorColorEditingController to contain color-editing-specific code only. We should also take this opportunity to enhance the new CodeMirrorEditingController with new features such as being able to deal with text markers spread across multiple lines and more gracefully deal with Esc. key presses, which have been issues so far.
<rdar://problem/16119910>
Created attachment 224743 [details] Patch
Comment on attachment 224743 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=224743&action=review > Source/WebInspectorUI/UserInterface/CodeMirrorEditingController.js:106 > + get cssClassName() > + { > + return ""; > + }, Should this have a // Implemented by subclasses. comment? > Source/WebInspectorUI/UserInterface/Geometry.js:104 > + for (var i = 1; i < rects.length; ++i) for (... of ...) > Source/WebInspectorUI/UserInterface/HoverMenu.js:36 > + this._svg = this._element.appendChild(document.createElementNS("http://www.w3.org/2000/svg", "svg")); _svg is pretty generic. Can this have a better name? _outlineElement? > Source/WebInspectorUI/UserInterface/HoverMenu.js:166 > + _drawTwoNonOverlappingLines: function(rects) Crazy town! But I like it. > Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:1312 > + if (marker.type === WebInspector.TextMarker.Type.Color) { > + var color = this._editingController.value; > + if (!color || !color.valid) { Most of this code is now generic (editableMarker), but this is specific to color. Does this not apply to all editable markers?
(In reply to comment #3) > (From update of attachment 224743 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=224743&action=review > > > Source/WebInspectorUI/UserInterface/CodeMirrorEditingController.js:106 > > + get cssClassName() > > + { > > + return ""; > > + }, > > Should this have a // Implemented by subclasses. comment? Good point, I'll add it. > > Source/WebInspectorUI/UserInterface/Geometry.js:104 > > + for (var i = 1; i < rects.length; ++i) > > for (... of ...) We start at i = 1 in this case. > > Source/WebInspectorUI/UserInterface/HoverMenu.js:36 > > + this._svg = this._element.appendChild(document.createElementNS("http://www.w3.org/2000/svg", "svg")); > > _svg is pretty generic. Can this have a better name? _outlineElement? Sure! > > Source/WebInspectorUI/UserInterface/HoverMenu.js:166 > > + _drawTwoNonOverlappingLines: function(rects) > > Crazy town! But I like it. This matches DataDetectors UI on OS X and generally looks pretty good. > > Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:1312 > > + if (marker.type === WebInspector.TextMarker.Type.Color) { > > + var color = this._editingController.value; > > + if (!color || !color.valid) { > > Most of this code is now generic (editableMarker), but this is specific to color. Does this not apply to all editable markers? I suppose we ought to make it so that a CodeMirrorEditingController value ought to implement a .valid getter to check that its value is valid so that this code could be generic. I'll rework that in an updated patch if that sounds like a good idea to you.
Comment on attachment 224743 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=224743&action=review >>> Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:1312 >>> + if (!color || !color.valid) { >> >> Most of this code is now generic (editableMarker), but this is specific to color. Does this not apply to all editable markers? > > I suppose we ought to make it so that a CodeMirrorEditingController value ought to implement a .valid getter to check that its value is valid so that this code could be generic. I'll rework that in an updated patch if that sounds like a good idea to you. This is fine as-is. We can change it later of other editors need it.
Created attachment 224761 [details] Patch for landing
Comment on attachment 224761 [details] Patch for landing Rejecting attachment 224761 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 224761, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: nk #1 succeeded at 445 (offset 1 line). patching file Source/WebInspectorUI/UserInterface/Main.js patching file Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.css patching file Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js patching file Source/WebInspectorUI/UserInterface/TextEditor.js patching file Source/WebInspectorUI/UserInterface/TextMarker.js Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.appspot.com/results/4763882363027456
Created attachment 224766 [details] Patch for landing
Comment on attachment 224766 [details] Patch for landing Clearing flags on attachment: 224766 Committed r164436: <http://trac.webkit.org/changeset/164436>
All reviewed patches have been landed. Closing bug.