Bug 129094 – Web Inspector: create a CodeMirrorEditingController superclass

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Antoine Quint]

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.

[Radar WebKit Bug Importer]

<rdar://problem/16119910>

[Antoine Quint]

Created attachment 224743 [details]
Patch

[Timothy Hatcher]

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?

[Antoine Quint]

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

[Timothy Hatcher]

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.

[Antoine Quint]

Created attachment 224761 [details]
Patch for landing

[WebKit Commit Bot]

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

[Antoine Quint]

Created attachment 224766 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 224766 [details]
Patch for landing

Clearing flags on attachment: 224766

Committed r164436: <http://trac.webkit.org/changeset/164436>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.