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

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


Missing pieces:
- I don't see conditional breakpoints handling code. Try calling "Edit
breakpoint" in the default editor's gutter context menu.
- Please provide the screenshots for the message bubbles as well as execution
line
- What happens when you start editing the code that has execution line
decoration? In the default editor, the decoration is hidden upon editing
- While on a breakpoint, I don't think that _getPopoverAnchor will properly
show hover evaluation. Probably deserves a separate bug.

I don't think this change is too large, so you should probably bake conditional
breakpoints and fixes I mention above in.

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

I don't see a call to populateLineGutterContextMenu called from within this
editor. Now that you have this nice event, I think you should pass "event"
object into it instead of the shiftKey flag and get rid of the
populateLineGuggerContextMenu. Let JavaScriptSourceFrame create context menu
for itself.

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

I think you could so this._codeMirror.setMarker(lineNumber, className) and that
would result in the same behavior.

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

So you handle messages now? Is there a sreenshot for that? I'm interested in
multiline error message (several messages on the same line + a breakpoint on
that line).

> Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:179
> +	   element.parent.removeChild(element);

What is parent? I don't think you were testing this.

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

Given that you have a viewport rendering, actual dom element will be re-created
each time you scroll into the range containing this line. As a result, your
highlight animation will trigger upon every scrolling. You don't want that.

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:147
> +	  
this.dispatchEventToListeners(WebInspector.TextEditor.Events.GutterClick, {
lineNumber: target.lineNumber, shiftKey: event.shiftKey });

I think you need to pass the event object here.