[webkit-reviews] review denied: [Bug 93686] Web Inspector: Support breakpoints in CodeMirrorTextEditor : [Attachment 157651] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied  review:
Bug 93686: Web Inspector: Support breakpoints in CodeMirrorTextEditor
https://bugs.webkit.org/show_bug.cgi?id=93686

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

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


>> Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:121
>> +	    if (info.markerText)
> 
> There can be different markers (messages, etc). Editor should have no way of
knowing whether there is a breakpoint. It should notify GutterClicked, not
BreakpointAdded. The only place where editor it breakpoint-aware is
decorations.

Or you can leave it as is for now.

>> Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:150
>> +	    this._codeMirror.setMarker(lineNumber, "<span style=\"color: white;
position: absolute; left: 0; right: -6px; border-width: 0 8px 0px 2px;
-webkit-border-image: url(Images/breakpointBorder.png) 1 14 1
2;\">%N%</span>x");
> 
> Nit: We need to be able to pass Element / DocumentFragment to the codemirror.
You should also style things using CSS. Worst case, create element and get its
innerHTML here.

This can be landed as is, but needs to be fixed upstream.

> Source/WebCore/inspector/front-end/JavaScriptSourceFrame.js:55
> +   
this.textEditor.addEventListener(WebInspector.TextEditor.Events.BreakpointAdded
, this._handleSetBreakpoint.bind(this), this);

I don't see where DefaultTextEditor fires it. You should make a proper
refactoring there.