[webkit-reviews] review granted: [Bug 129094] Web Inspector: create a CodeMirrorEditingController superclass : [Attachment 224743] Patch

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at apple.com> has granted Antoine Quint
<graouts at webkit.org>'s request for review:
Bug 129094: Web Inspector: create a CodeMirrorEditingController superclass
https://bugs.webkit.org/show_bug.cgi?id=129094

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

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