Bug 93686 – Web Inspector: Render breakpoint gutter markers and execution line in CodeMirrorTextEditor

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Jan Keromnes]

Currently, breakpoints are not supported by the experimental editor using CodeMirror. This can be fixed by refactoring `TextEditor.addDecoration` and wiring up breakpoints with CodeMirrorTextEditor.

[Jan Keromnes]

Created attachment 157651 [details]
Patch

[Pavel Feldman]

Comment on attachment 157651 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157651&action=review

Looks good, couple of nits inline.

> 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.

> 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.

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

Empty if.

[Pavel Feldman]

Comment on attachment 157651 [details]
Patch

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.

[Pavel Feldman]

Comment on attachment 157651 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157651&action=review

>>> 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.

Or actually, I would do the following: 
var className = "cm-breakpoint";
if (disabled)
    className += "cm-breakpoint-disabled";
if (conditional)
    className += "cm-breakpoint-conditional";
setMarker(lineNumber, "<div className'" + className +"'>%N%</div>");

and use no absolute positioning, refer to border image from css only.

[Jan Keromnes]

Created attachment 157685 [details]
Patch

[Jan Keromnes]

New patch addressing all your concerns except the fragment marker that needs to be fixed in upstream codemirror.

[Pavel Feldman]

Comment on attachment 157685 [details]
Patch

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...

[Jan Keromnes]

Created attachment 157914 [details]
Patch

[Jan Keromnes]

The new patch addresses most of the remarks from comment #7.

> > 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 =.

Fixed and upstreamed.

> > 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)

A CodeMirror marker is a unique gutter decoration the replaces a line number in the gutter. Messages and popovers will use different API calls.

> > 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.

Now uses handle instead of line number.

> > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:169
> > +    {
> 
> Please include // FIXME: Implement ... here.

I couldn't decide how to formulate the FIXME message so I implemented the feature instead.

> > 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.

Agreed. The new patch uses the same styling as the DefaultTextEditor.

> > 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...

Fixed as a side-effect of the line handle change.

[Pavel Feldman]

Comment on attachment 157914 [details]
Patch

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.

[Pavel Feldman]

> Fixed and upstreamed.

It is not yet upstreamed (and for good since it does not really make much sense the way it is implemented to me). Please update codemirror.js only in separate changes, only to the actual Marijnh's master, specifying the list of changes happened upstream. Otherwise we get into a mess of merging.

> A CodeMirror marker is a unique gutter decoration the replaces a line number in the gutter. Messages and popovers will use different API calls.

Sounds good, thanks!

[Jan Keromnes]

> Missing pieces:
> - I don't see conditional breakpoints handling code. Try calling "Edit breakpoint" in the default editor's gutter context menu.

Yes, context menus aren't implemented yet because they are probably going to need some refactoring. I'll work on them in a follow-up patch.

> - Please provide the screenshots for the message bubbles as well as execution line

Sure, I'll upload some screenshots after the next patch.

> - What happens when you start editing the code that has execution line decoration? In the default editor, the decoration is hidden upon editing

It is also hidden upon editing with CodeMirror.

> - While on a breakpoint, I don't think that _getPopoverAnchor will properly show hover evaluation. Probably deserves a separate bug.

Indeed, I didn't work on the hover part for this patch.

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

I think context menus and hover messages deserve separate bugs. My goal for this patch was supporting gutter clicks that set breakpoints, and it already does more than that. I'll rename the bug.

> 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.

I'll pass the mouse down event inside the gutter click event if you want, but the context menu will have to wait for now.

> > 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.

this._codeMirror.setMarker(lineNumber, null, className) did the trick.

> > 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).

I'm including those examples in the screenshots I'll upload after the patch.

> > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:179
> > +        element.parent.removeChild(element);
> 
> What is parent? I don't think you were testing this.

Thanks for catching this! Turns out removeDecoration() is rarely called now that breakpoints and execution line stopped using it.

> > 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.

Even worse, not clearing the class makes the yellow flash happen every time you change a character in the line. Let's reset the class after the 2s.

> > 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.

Done.

[Jan Keromnes]

Created attachment 158119 [details]
Patch

[Jan Keromnes]

Created attachment 158125 [details]
A breakpoint was set and the corresponding line was highlighted.

[Jan Keromnes]

Created attachment 158126 [details]
The breakpoint was deactivated.

[Jan Keromnes]

Created attachment 158128 [details]
We stopped on a breakpoint and stepped over (execution line).

[Jan Keromnes]

Created attachment 158130 [details]
Then we decided to edit the line: execution line lost its highlighting as expected.

[Jan Keromnes]

Created attachment 158131 [details]
A warning message bubble.

[Jan Keromnes]

Created attachment 158132 [details]
An error message bubble.

[Jan Keromnes]

Created attachment 158135 [details]
Several error message bubbles.

[Jan Keromnes]

Created attachment 158141 [details]
Patch

[Jan Keromnes]

Last patch reverts codemirror.js to upstream.

[Pavel Feldman]

Comment on attachment 158141 [details]
Patch

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.

[Jan Keromnes]

Created attachment 158234 [details]
Patch

[Jan Keromnes]

(In reply to comment #23)
> > 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.

I don't think that temporary decorations above the text are bad, but maybe eclipse-like decorations could be nice. Reverted and added TODO.

> > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:190
> > +        this._codeMirror.setLineClass(lineNumber, null, null);
> 
> Why do we need this line now?

When we highlight we need to clear the previously highlighted line, so that it has the same behaviour as DefaultTextEditor (i.e. flashes again when we click again before the 2s timeout).

> > 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.

Fixed.

> > 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.

Fixed.

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

Fixed.

> > 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.

Reverted codemirror.js to previous version.

[Pavel Feldman]

Comment on attachment 158234 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158234&action=review

> Source/WebCore/ChangeLog:25
> +        (WebInspector.CodeMirrorTextEditor.prototype._loadLibraries):

Why do you have these lines in the changelog? That code has not changed.

> Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:153
> +        if (this._executionLine)

What if this is the first line (0)?

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

This will crash since you did not append the element above.

> Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:202
> +        if (this._highlightedLine)

What is this is a first line (i.e. 0)? You should check for typeof this._highlightedLine.

[Jan Keromnes]

Created attachment 158386 [details]
Patch

[Jan Keromnes]

(In reply to comment #26)
> > Source/WebCore/ChangeLog:25
> > +        (WebInspector.CodeMirrorTextEditor.prototype._loadLibraries):
> 
> Why do you have these lines in the changelog? That code has not changed.

It was a glitch, fixed.

> What if this is the first line (0)?
> What is this is a first line (i.e. 0)? You should check for typeof this._highlightedLine.

Now using typeof.

> > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:173
> > +        element.parentNode.removeChild(element);
> 
> This will crash since you did not append the element above.

Also reverted.

[Pavel Feldman]

Comment on attachment 158386 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158386&action=review

> Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:164
> +        // TODO implement so that it doesn't hide context code

WebKit uses FIXME, not TODO.

[WebKit Review Bot]

Comment on attachment 158386 [details]
Patch

Clearing flags on attachment: 158386

Committed r125599: <http://trac.webkit.org/changeset/125599>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.