[webkit-reviews] review denied: [Bug 93686] Web Inspector: Support breakpoints in CodeMirrorTextEditor : [Attachment 157685] 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: Support breakpoints in CodeMirrorTextEditor
https://bugs.webkit.org/show_bug.cgi?id=93686

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

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


> Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:120
> +	  
this.dispatchEventToListeners(WebInspector.TextEditor.Events.GutterClick, {
lineNumber: lineNumber, shiftKey: false });

Event object is passed as the third parameter here. You should pass it along
with the event instead of this shiftKey modifier.

> Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:137
> +	   this._codeMirror.setMarker(lineNumber, marker.outerHTML); // FIXME
in upstream codemirror

This is no better than just a string as long as it is still going through the
innerHTML =.

> Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:145
> +	   this._codeMirror.clearMarker(lineNumber);

I don't think this is proactive: messages are going to be based on the same
technology, right? I guess you already need to make things based on handles
(model attributes)

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

The line number might have changed since then if say text was inserted above.
You need to use handle to the line.

> Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:169
> +    {

Please include // FIXME: Implement ... here.

> Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:194
> +	   this._codeMirror.setLineClass(lineNumber, "", "cm-highlight");

Why not to use the way it is implemented in the default text editor? We should
provide the same dimming css animation here. If we want to switch to this
editor, it should provide the complete feature parity. We are not in a hurry,
we need to do things right.

> Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:-342
> -	       console.log(xhr.responseText);

Thanks!

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:195
> +	   if (typeof this._executionLineNumber === "number") {

As I can see, we already suffer from this bug. You can maintain bug parity
then...