[webkit-reviews] review denied: [Bug 93686] Web Inspector: Render breakpoint gutter markers and message bubbles in CodeMirrorTextEditor : [Attachment 158141] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Jan Keromnes
<janx at chromium.org>'s request for review:
Bug 93686: Web Inspector: Render breakpoint gutter markers and message bubbles
in CodeMirrorTextEditor
https://bugs.webkit.org/show_bug.cgi?id=93686

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

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=158141&action=review


> Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:164
> +	   this._codeMirror.addWidget({ line: lineNumber, ch: 0 }, element,
true);

These decorations are painted above the text which is unfortunate. Is there a
way to copy existing behavior? If not, we should come up with a gutter
decoration (a-la Eclipse) that would toggle the error message popup. We might
need a design for that. I don't think you'll be able to do this in this change,
so you probably should roll it back and change the title of the bug again to
not mention support for messages.

> Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:190
> +	   this._codeMirror.setLineClass(lineNumber, null, null);

Why do we need this line now?

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:143
> +	       return;

It should be up to the embedder on how to handle these, do not return here.

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:148
> +	   event.preventDefault();

It should be up to the embedder to handle it. If embedder handled it, he should
call event.consume(true/false). You typically don't want to prevent default,
but you always want to stop propagation.

> Source/WebCore/inspector/front-end/TextEditor.js:38
> +    GutterClick: "gutterClick",

Trailing coma

> Source/WebCore/inspector/front-end/cm/codemirror.js:248
> +	 getViewport: function() { return {from: showingFrom, to: showingTo};},


As I mentioned earlier, changes to codemirror.js should only go under separate
bugs where we update it from upstream.