Scratchpad already uses CodeMirror, time for the debugger!

Oh boy.

Just to start the movement. 7d2d96c Replace Orion with CodeMirror; everything's broken

Syntax highlighting works now. JavaScript and CSS is now highlighted alongside HTML thanks to htmlmixed mode!

^_^

Breakpoints UI kind of works now.

Try: https://tbpl.mozilla.org/?tree=Try&rev=f7427c716397 (this push is too see how many tests fail without actually running them on my computer) So what I'm doing is converting all DebuggerView.editor.* calls from files other than debugger-view.js into DebuggerView methods. This way we will have only one view object that interacts with the editor component which is better for future maintenance. Question to Rob, Victor or anyone else who has an opinion: What should we do with the search UI? Scratchpad and StyleEditor have (or will have soon) the same unified user interface for search/replace but the Debugger has its own toolbar search thingy. Maybe we can leave toolbar for the project search only and move in-file search to the same UI shared with other tools? This way our users will have an expected UI when performing a search in any of our editors. Plus we won't be reimplementing find/findNext in the debugger.

I expect us to eventually end up with a single Sources panel, as discussed in the authoring tools thread, so consistency will be obtained in the end no matter which road we take. But I think in that particular end game, the current search functionality offered by the debugger toolbar (which largely follows Sublime Text's example) will be far superior to a plain file search. Combining search modifiers gives power users a very efficient way to minimize keystrokes when searching for anything and we have yet to effectively communicate this feature to our users.

I agree with Panos here. The long term plan is to unify those tools, in which case the consistency problem will be fixed anyway. In the meantime, I'd still argue that the debugger mandates a more performant search mechanism, so having a different UI than the Scratchpad, for example, is understandable. As far as code structure and difficulty goes, I think it would't be hard to implement the previous source editor's findPrev and findNext. It makes sense now to have a debugger EditorView singleton now, just like all the other *View objects in the frontend, which takes care of all the things you added, stores, methods and whatnot. Simply strapping all of the in DebuggerView will bloat it a bit.

Yeah, I'm with Panos and Victor. I think the search capabilities of the Debugger are going to be more powerful than what CodeMirror gives us. Unification will happen when we consolidate source views across tools.

nice try push!

Ok, so the consensus is to preserve current search functionality. (In reply to Victor Porof [:vp] from comment #8) > > As far as code structure and difficulty goes, I think it would't be hard to > implement the previous source editor's findPrev and findNext. It makes sense > now to have a debugger EditorView singleton now, just like all the other > *View objects in the frontend, which takes care of all the things you added, > stores, methods and whatnot. Simply strapping all of the in DebuggerView > will bloat it a bit. I will try to re-use searchcursor addon we ship with CodeMirror. I don't think that adding another singleton is a good idea since having multiple interdependent files grows the module's propagation cost. I'd rather vote for converting current singletons into modules that don't depend on the DebuggerView and having the DebuggerView use them. But for this bug I will see how much I can reuse from searchcursor (hopefully all of it) and then see if it makes sense to make an addon module for the Editor that adds debugging capabilities.

(In reply to Anton Kovalyov (:anton) from comment #12) > Ok, so the consensus is to preserve current search functionality. > > (In reply to Victor Porof [:vp] from comment #8) > > > > As far as code structure and difficulty goes, I think it would't be hard to > > implement the previous source editor's findPrev and findNext. It makes sense > > now to have a debugger EditorView singleton now, just like all the other > > *View objects in the frontend, which takes care of all the things you added, > > stores, methods and whatnot. Simply strapping all of the in DebuggerView > > will bloat it a bit. > > I'd rather vote for converting current singletons into modules that don't > depend on the DebuggerView and having the DebuggerView use them. > That's what I was actually suggesting :) Perhaps I wasn't clear enough: There are multiple singletons in the debugger frontend that handle different parts of the logic and the view. There's a SourcesView that handles sources, a WatchExpressionsView, a ToolbarView that handles those, and so on. Similarly, we didn't have a DebuggerView singleton in the debugger frontend until now because, theoretically, we didn't need it. The initial design decisions (when we shipped Orion) for switching to a different editor were that new editors need to expose an identical API. In this case, wrapping the source editor again in the debugger frontend didn't make much sense, because it was in itself a standalone module. Right now, with your patch, there are quite a bit of new editor related methods in DebuggerView, which bloats it a bit and, as you said (if I understood you correctly) this will hurt maintainability in the long run. I'm not at all suggesting we add a different file or module, just taking all of those functions and putting them into a dedicated EditorView singleton, instead of ad-hoc-ly strapping all of them to DebuggerView (which mostly only handles initialization of all the other view components).

Search somewhat works now. There are still things to fix. Try (will fail): https://tbpl.mozilla.org/?tree=Try&rev=ae12d16fb5a1

Lots of test fixes. Try: https://tbpl.mozilla.org/?tree=Try&rev=269dc0ff90f1

Down to 7 failing tests. Try: https://tbpl.mozilla.org/?tree=Try&rev=341325391418

I think I fixed all the tests. Try: https://tbpl.mozilla.org/?tree=Try&rev=de6281bd6f4f Still needed to be done: * Re-enable CSS mode tests * Highlight current line * Visualize debug location

Another attempt at a green Try: https://tbpl.mozilla.org/?tree=Try&rev=04bc5f9943f8

Highlight current line. Try push is the same, not much changed.

Okay, added comments and stuff. Ready for review. Another full Try, just in case: https://tbpl.mozilla.org/?tree=Try&rev=aae4d38fe116

Comment on attachment 818704 [details] [diff] [review] WIP 11 Review of attachment 818704 [details] [diff] [review]: ----------------------------------------------------------------- This looks surprisingly smooth and clean, great work. I do have some questions and comments, please address them and ask me again for r?. ::: browser/devtools/debugger/debugger-controller.js @@ +1461,3 @@ > * Event handler for new breakpoints that come from the editor. > * > + * @param num aLine Nit: number, not num @@ +1484,5 @@ > > /** > * Event handler for breakpoints that are removed from the editor. > * > + * @param num aLine Ditto. ::: browser/devtools/debugger/debugger-panes.js @@ +476,5 @@ > */ > _hideConditionalPopup: function() { > this._cbPanel.hidden = true; > + if (this._cbPanel.hidePopup) > + this._cbPanel.hidePopup(); Please add curly braces for if/else with a single statement, here and everywhere else, for consistency. @@ +643,4 @@ > * The load listener for the source editor. > */ > _onEditorLoad: function(aName, aEditor) { > + aEditor.on("cursorActivity", this._onEditorSelection); Might want to also rename _onEditorSelection to _onEditorCursorActivity, since the name is not entirely accurate now. @@ +888,2 @@ > } > // Avoid placing breakpoints incorrectly when using key shortcuts. Nit: add a newline here as well, since you added it in the function above. ::: browser/devtools/debugger/debugger-toolbar.js @@ +865,5 @@ > return; > } > + > + aLine = aLine > DebuggerView.editor.lineCount() ? 0 : aLine - 1; > + DebuggerView.editor.setCursor({ line: aLine, ch: 0 }); What when you ask CodeMirror to set a line that exceeds that lineCount? Doesn't it clamp the value by default? These checks might be redundant. ::: browser/devtools/debugger/debugger-view.js @@ +5,4 @@ > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > "use strict"; > > +Cu.import("resource://gre/modules/Services.jsm"); // For Telemetry This is already imported in debugger-controller.js. Remove it. @@ +31,5 @@ > const SEARCH_LINE_FLAG = ":"; > const SEARCH_VARIABLE_FLAG = "*"; > + > +const events = require("devtools/shared/event-emitter"); > +const EditorDebugger = require("devtools/sourceeditor/debugger.js"); Please move all requires in debugger-controller.js. Also, EventEmitter was already imported. Nit: DebuggerEditor sounds better to me, can we use that instead? @@ +68,4 @@ > this._initializeEditor(deferred.resolve); > document.title = L10N.getStr("DebuggerWindowTitle"); > > + events.decorate(this); Only emit public debugger events from the debugger panel window, which is already an event emitter, as suggested by the EVENTS literal in debugger-controller.js. However, why decorate the DebuggerView object? It doesn't look like you're emitting anything from it. "breakpointAdded" and "breakpointRemoved" are emitted from the editor instance, not DebuggerView, and I don't see any other new events here. @@ +187,5 @@ > _initializeEditor: function(aCallback) { > dumpn("Initializing the DebuggerView editor"); > > + let extraKeys = {}; > + extraKeys[(Services.appinfo.OS == "Darwin" ? "Cmd-" : "Ctrl-") + "F"] = (cm) => { I don't like this. It should be localized and added via a command/key combo in XUL. Can you get this information from the DOM instead of hard-coding in JS? @@ +310,4 @@ > * The source text content. > */ > _setEditorMode: function(aUrl, aContentType = "", aTextContent = "") { > + // Avoid setting the editor mode for very large files. (TODO: is this still necessary?) File a bug for the TODO and add the bug number here. @@ +311,5 @@ > */ > _setEditorMode: function(aUrl, aContentType = "", aTextContent = "") { > + // Avoid setting the editor mode for very large files. (TODO: is this still necessary?) > + if (aTextContent.length >= SOURCE_SYNTAX_HIGHLIGHT_MAX_FILE_SIZE) > + return void this.editor.setMode(Editor.modes.text); Nit: curly braces, here and everywhere else. @@ +322,5 @@ > // &lt;, regardless of extension. > + if (aTextContent.match(/^\s*</)) > + return void this.editor.setMode(Editor.modes.html); > + > + // Unknown source code, use text. Nit: unknown language sounds more accurate. The source code *is* known :) @@ +430,4 @@ > let sourceItem = this.Sources.getItemByValue(aUrl); > let sourceForm = sourceItem.attachment.source; > > + this.editor.clearDebugLocation(); Why is this here? Please remove it or add a comment specifying why. ::: browser/devtools/debugger/debugger.css @@ +25,4 @@ > > .devtools-toolbarbutton:not([label]) > .toolbarbutton-text { > display: none; > +} \ No newline at end of file Nit: I think you removed a trailing newline here or something. ::: browser/devtools/debugger/debugger.xul @@ +30,5 @@ > + <script type="application/javascript"> > + function goUpdateSourceEditorMenuItems() { > + goUpdateGlobalEditMenuItems(); > + let commands = ['cmd_undo', 'cmd_redo', 'cmd_cut', 'cmd_paste', 'cmd_delete', 'cmd_find']; > + commands.forEach(goUpdateCommand); Nit: I would combine the declaration and forEach into a single statement. @@ +38,4 @@ > <commandset id="editMenuCommands"/> > + > + <commandset id="sourceEditorCommands"> > + <command id="cmd_gotoLine" oncommand="goDoCommand('cmd_gotoLine')"/> Why is cmd_gotoLine special? Is this needed? We're already using a special command in the debugger for this in tandem with the search box. @@ +139,4 @@ > label="&debuggerUI.searchGoToLine;" > accesskey="&debuggerUI.searchGoToLine.key;" > key="lineSearchKey" > + command="cmd_gotoLine"/> Again, we're using a special operator for this and a designated keyboard shortcut. ::: browser/devtools/debugger/test/browser_dbg_editor-contextmenu.js @@ +59,3 @@ > > + ok(!document.getElementById("cmd_gotoLine").hasAttribute("disabled"), > + "gotoLine is enabled"); You probably don't need this anymore (see my comments for debugger.xul). ::: browser/devtools/debugger/test/browser_dbg_sources-labels.js @@ +85,4 @@ > let url = href + leaf; > let label = gUtils.getSourceLabel(url); > let trimmedLabel = gUtils.trimUrlLength(label); > + gSources.push([trimmedLabel, url], { attachment: {}}); The { attachment: {} } shouldn't be necessary, was the test failing without it? ::: browser/devtools/sourceeditor/codemirror/css.js @@ +1,1 @@ > +CodeMirror.defineMode("css", function(config) { I assume this is the default CSS syntax rules file for CodeMirror? Does it need to go through a proper style/security review? Did CodeMirror ever go through such a review? ::: browser/devtools/sourceeditor/codemirror/htmlmixed.js @@ +1,1 @@ > +CodeMirror.defineMode("htmlmixed", function(config, parserConfig) { Same question. ::: browser/devtools/sourceeditor/codemirror/mozilla.css @@ +2,5 @@ > + width: 16px; > +} > + > +.breakpoint, .debugLocation, .breakpoint-debugLocation { > + display: inline-block; Don't we need to split these rules between content/theme CSS? See https://wiki.mozilla.org/DevTools/CSSTips @@ +12,5 @@ > + background-size: 12px; > +} > + > +.breakpoint { > + background-image: url("chrome://browser/skin/devtools/orion-breakpoint.png"); Nit: rename those files from orion-* to editor-*. ::: browser/devtools/sourceeditor/debugger.js @@ +56,5 @@ > + return cm.getSearchCursor(query, pos, > + typeof query == "string" && query == query.toLowerCase()); > +} > + > +function doSearch(cm, rev, query) { Nit: add comments for all these functions, describing their behavior. @@ +227,5 @@ > + */ > +function getPositionFromCoords(ctx, left, top) { > + let { cm } = ctx; > + return cm.coordsChar({ left: left, top: top }); > +} Please make getPositionFromCoords (coordsChar) and extendSelection available to all Editor instances. I'll need them for the ShaderEditor, and I imagine other consumers will make use of them as well, since they're not characteristic to a "debugger" per-se. ::: browser/devtools/sourceeditor/editor.js @@ +209,5 @@ > + cm.on("gutterClick", (cm, line) => this.emit("gutterClick", line)); > + > + cm.on("cursorActivity", (cm) => { > + this.emit("cursorActivity"); > + if (cm.somethingSelected()) this.emit("somethingSelected"); Wouldn't it be better it simply have cursorActivity as an event and somethingSelected as a public method, instead of two events which in many cases mean the same thing?

I also found this usability awkwardness that should be fixed IMHO: 1. Go to http://todomvc.com/architecture-examples/backbone/ 2. Add a breakpoint on line 31 in app-view.js ("this.allCheckbox ...") 3. Reload and wait for the breakpoint to be hit 4. Step in The focused line is now at the bottom of the editor, and will remain so as you keep stepping in. If you step out and end up back to app-view.js, the next line (32) will also be at the bottom of the editor. Orion would try to keep it somewhere in the middle, so you'd get as much context as possible both before and after the current stack frame line. With the current approach, you'll always have to scroll down to see what's coming, which is a usability minus.

(In reply to Victor Porof [:vp] from comment #22) > I also found this usability awkwardness that should be fixed IMHO: > > 1. Go to http://todomvc.com/architecture-examples/backbone/ > 2. Add a breakpoint on line 31 in app-view.js ("this.allCheckbox ...") > 3. Reload and wait for the breakpoint to be hit > 4. Step in > > The focused line is now at the bottom of the editor, and will remain so as > you keep stepping in. If you step out and end up back to app-view.js, the > next line (32) will also be at the bottom of the editor. > > Orion would try to keep it somewhere in the middle, so you'd get as much > context as possible both before and after the current stack frame line. With > the current approach, you'll always have to scroll down to see what's > coming, which is a usability minus. I am fixing this in bug 919978 by making .setCursor default to "top" (as in make the line at top of the view with some 3 line margin from top if the line was not visible initially). "center" and "bottom" are also supported.

Thanks Victor! Optimizer's patch (which is already r+ and waiting for this bug to be landed) fixes the usability issue. I'll fix all other comments except for the CSS one: we have a bug about this and either me or bgrins will move files to themes/ as a followup. Also, CodeMirror didn't go through the secreview yet but will when all components have it in Nightly.

> What when you ask CodeMirror to set a line that exceeds that lineCount? Doesn't it > clamp the value by default? These checks might be redundant. CodeMirror defaults to the last line which is different from our logic here (first line). > The { attachment: {} } shouldn't be necessary, was the test failing without it? It was failing but it was printing an exception to the console. > Nit: rename those files from orion-* to editor-*. I plan to do these when I convert SourceEditor component and remove all Orion-related code. > Wouldn't it be better it simply have cursorActivity as an event and somethingSelected as a public method, instead of two events which in many cases mean the same thing? I feel like having a special 'somethingSelected' method is better for public API since it abstracts the way CodeMirror treats selections and cursors (i.e. everything is a selection and "no selection" is when start and end of a selection is the same).

> I don't like this. It should be localized and added via a command/key combo in XUL. Can you get this information from the DOM instead of hard-coding in JS? I agree but I'm not sure how to convert information from the DOM to the CodeMirror compatible format. Since this is relevant not only for the Debugger but also for the StyleEditor and the Editor itself, I think I'm going to do this as a follow up. See bug 929234.

Try: https://tbpl.mozilla.org/?tree=Try&rev=0669b1a25340

(In reply to Anton Kovalyov (:anton) from comment #26) > > I don't like this. It should be localized and added via a command/key combo in XUL. Can you get this information from the DOM instead of hard-coding in JS? > > I agree but I'm not sure how to convert information from the DOM to the > CodeMirror compatible format. Since this is relevant not only for the > Debugger but also for the StyleEditor and the Editor itself, I think I'm > going to do this as a follow up. See bug 929234. If you don't do this at all and leave the key/command defined only in xul, does it get eaten up by the code mirror iframe? Or is codemirror specifically preventing propagation? It used to work with Orion, which was also living in an iframe and didn't eat keyboard events.

CodeMirror re-defines some shortcuts and eats propagation if I'm not mistaken.

(In reply to Anton Kovalyov (:anton) from comment #29) > CodeMirror re-defines some shortcuts and eats propagation if I'm not > mistaken. In that case, how about adding an option to Editor.js to stop propagation for certain events/shortcuts? That looks the most elegant way of fixing this if I'm not mistaken. With the current approach, apart from the localization issue (consider whether other locales have different mapped keys, in which case F might clash), you'll also need to have the editor focused for the shortcut key to work. Secondly, having a debugger shortcut going through the editor logic and back up to the debugger js logic again is awkward.

> In that case, how about adding an option to Editor.js to stop propagation for certain events/shortcuts? That looks the most elegant way of fixing this if I'm not mistaken. I will have to investigate this and see how it'll work not only with the Debugger but also with Scratchpad and StyleEditor.

Try is green!

(In reply to Anton Kovalyov (:anton) from comment #32) > Try is green! \o/

LAND ITTtt!11

Rob, it's still r? Victor, did you want me to do the shortcuts changes in the scope of this bug? I'd rather do that as a followup (bug 929234) since it affects other components as well.

Comment on attachment 820071 [details] [diff] [review] WIP 12 Review of attachment 820071 [details] [diff] [review]: ----------------------------------------------------------------- ++! All my likes, with a few additional small comments. ::: browser/devtools/debugger/debugger-panes.js @@ +477,5 @@ > _hideConditionalPopup: function() { > this._cbPanel.hidden = true; > + if (this._cbPanel.hidePopup) { > + this._cbPanel.hidePopup(); > + } Nit: did this start failing? Any idea why? Care to add a comment? @@ +678,4 @@ > * The context menu listener for the source editor. > */ > _onEditorContextMenu: function({ x, y }) { > + this._editorContextMenuLineNumber = DebuggerView.editor.getPositionFromCoords(x, y); The getPositionFromCoords returns an object if I'm not mistaken. In this case, the `this._editorContextMenuLineNumber >= 0` checks and `{ line: this._editorContextMenuLineNumber, ch: 0 }` assignments below won't work properly. ::: browser/devtools/debugger/debugger-toolbar.js @@ +865,5 @@ > return; > } > + > + aLine = aLine > DebuggerView.editor.lineCount() ? 0 : aLine - 1; > + DebuggerView.editor.setCursor({ line: aLine, ch: 0 }); Come to think about it, if I remember correctly, it doesn't really matter in this particular case if the cursor overflows, or even where the cursor jumps when aLine is out of bounds. This is already handled in _onKeyPress for UP/DOWN navigation, and if a user searches for a line number that's too large, it will actually make more sense to jump to the bottom, not to the top. Just do `DebuggerView.editor.setCursor({ line: aLine, ch: 0 });` @@ +1043,5 @@ > > // Jump to the next/previous instance of the currently searched token. > if (isTokenSearch) { > + let methods = { selectNext: "findNext", selectPrev: "findPrev" }; > + DebuggerView.editor[methods[actionToPerform]](); Uber nit: do you think naming the editor functions "selectNext" and "selectPrev" instead of "find*" would be better? In that case, we can bypass the needless "methods" object. ::: browser/devtools/debugger/debugger-view.js @@ +189,5 @@ > > + let extraKeys = {}; > + extraKeys[(Services.appinfo.OS == "Darwin" ? "Cmd-" : "Ctrl-") + "F"] = (cm) => { > + DebuggerView.Filtering._doTokenSearch(); > + }; Assuming comment 31 doesn't work out, add a comment with the followup bug number. @@ +441,5 @@ > // update the source editor's caret position and debug location. > return this._setEditorSource(sourceForm, aFlags).then(() => { > // Line numbers in the source editor should start from 1. If invalid > // or not specified, then don't do anything. > + Uber nit: the comment above is directly related to the if function below, so I would remove this newline. ::: browser/devtools/debugger/debugger.xul @@ +30,5 @@ > + <script type="application/javascript"> > + function goUpdateSourceEditorMenuItems() { > + goUpdateGlobalEditMenuItems(); > + ['cmd_undo', 'cmd_redo', 'cmd_cut', > + 'cmd_paste', 'cmd_delete', 'cmd_find'].forEach(goUpdateCommand); If I think about this more, I don't think we need any goUpdateCommand calls at all, since none of those menu items (undo, redo, cut, paste, delete, find) are present in the context menu. So remove them. The goUpdateGlobalEditMenuItems call is required though, because we have a "copy" menu item. ::: browser/devtools/sourceeditor/editor.js @@ +69,4 @@ > "redo", > "clearHistory", > "posFromIndex", > + "indexFromPos", Are posFromIndex and indexFromPos necessary, since we have the getPosition and getOffset functions? Having all four as public seems a bit redundant to me. @@ +209,5 @@ > + cm.on("gutterClick", (cm, line) => this.emit("gutterClick", line)); > + > + cm.on("cursorActivity", (cm) => { > + this.emit("cursorActivity"); > + if (cm.somethingSelected()) this.emit("somethingSelected"); It doesn't look like you're using the "somethingSelected" event anywhere. Can you remove it? Is it required for a different bug? Nit: If not, you also said that > I feel like having a special 'somethingSelected' method is better for public > API since it abstracts the way CodeMirror treats selections and cursors (i.e. > everything is a selection and "no selection" is when start and end of a > selection is the same). which is exactly what I was suggesting. "somethingSelected" should be a method, not an event, because we already have the "cursorActivity" event. But this discussion is probably moot now since this stuff isn't used anywhere :)

(In reply to Anton Kovalyov (:anton) from comment #35) > Victor, did you want me to do the shortcuts changes in the scope of this > bug? I'd rather do that as a followup (bug 929234) since it affects other > components as well. That's entirely fine by me, but I'd make that bug at least a P2, because the localization concerns in comment 30.

Thanks Victor, good catch on posFromIndex, somethingSelected, etc!

Updated patch per Victor's feedback.

Comment on attachment 820579 [details] [diff] [review] WIP 13 Review of attachment 820579 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/debugger/debugger-panes.js @@ +684,4 @@ > * The context menu listener for the source editor. > */ > _onEditorContextMenu: function({ x, y }) { > + this._editorContextMenuLineNumber = DebuggerView.editor.getPositionFromCoords(x, y).line; I found one more thing wrong here :) The first param sent to this listener is the event name, not the location. Thus, x and y are undefined. However (!), this doesn't matter and is redundant, that's why the tests passed. Before, when right clicking, Orion didn't change the selected line. CodeMirror does, so all of these shenanigans aren't necessary anymore. I'll file a followup and remove them.

(In reply to Victor Porof [:vp] from comment #41) > Comment on attachment 820579 [details] [diff] [review] > WIP 13 Bug 929888.