Since the editor component will be available for add-on developers, I don't expose any private information not even as underscored properties. Take a look at a patch in bug 919709: 'editors' WeakMap stores objects that we use to access private variables (see 'let { cm, meta } = editors.get(this)') in the patch for that bug.

This patch works. Some of the existing feature might not work though. (Specially keybindings)

(In reply to Girish Sharma [:Optimizer] from comment #2) > Created attachment 814337 [details] [diff] [review] > WIP 1 > > This patch works. Some of the existing feature might not work though. > (Specially keybindings) Which ones? Search?

Apparently, search works. I got this error while closing the Style Editor: > TypeError: this.selectedEditor.sourceEditor.getCaretPosition is not a function (from StyleEditorUI.jsm:154)

Comment on attachment 814337 [details] [diff] [review] WIP 1 Review of attachment 814337 [details] [diff] [review]: ----------------------------------------------------------------- I understand that this patch is still WIP so this is not a review. Just a couple of comments after I read the patch (otherwise I will forget them). Glad to see it's working well without too much hassle! ::: browser/devtools/sourceeditor/editor.js @@ +33,5 @@ > "chrome://browser/content/devtools/codemirror/dialog.js", > "chrome://browser/content/devtools/codemirror/searchcursor.js", > "chrome://browser/content/devtools/codemirror/search.js", > "chrome://browser/content/devtools/codemirror/matchbrackets.js", > + "chrome://browser/content/devtools/codemirror/closebrackets.js", You might need to put a reference to this file to our licensing file but don't worry about that until later. @@ +309,5 @@ > > /** > + * Selects contents of the text area. > + */ > + setSelection: function (start, end) { setSelection is already there. See CM_MAPPING constant at the top of the file for all functions that we bind from CodeMirror instance.

This is almost complete patch. Everything works (which used to work previously using Orion) and all related tests pass except one. the loop in http://mxr.mozilla.org/mozilla-central/source/browser/devtools/styleeditor/test/browser_styleeditor_new.js#107 simulates typing of "body{background:red;" and as soon as "{" is typed, "}" should be added magically. it works when I type by hand, or run the same script using scratchpad browser mode but fails in the test and the final text remains "body{}" Once I get this test fixed, I will refactor things and put it up for review.

- All tests pass now. (At least the ones related to style editor.) - Added the closebrackets.js file name in README - try push : https://tbpl.mozilla.org/?tree=Try&rev=9b2fc6682e09 Since Gecko provides us a full list of CSS property names, values and color names.. I wanted to override the list that is present in codemirror/css.js file. I know how to do it, and the code is also present (commented) in editor.js starting from line 505 . The only problem is that the CodeMirror global object is only available in the onopen method of Editor.prototype.appendTo method. But I do not want to change the mime types on each appendTo call . @Anton, what approach should I follow here .. ?

Comment on attachment 815513 [details] [diff] [review] patch v0.1 Review of attachment 815513 [details] [diff] [review]: ----------------------------------------------------------------- This is looking really nice. It is a rather swift transition to codemirror. Thank you! ::: browser/devtools/sourceeditor/editor.js @@ +12,5 @@ > const L10N_BUNDLE = "chrome://browser/locale/devtools/sourceeditor.properties"; > const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"; > +/** > + * Maximum allowed vertical offset for the line index when you call > + * SourceEditor.setCaretPosition(). Is this accurate? @@ +507,5 @@ > +// one provided by Gecko. > + > +// XXX : Does not work since CodeMirror global is not available outside of the > +// Editor.prototype.CodeMirror object, and I want to avoid doing all the below > +// things on every editor creation. You can do all this in the CodeMirror instance and cache the information in a global variable.

Forgot to mention: the r+ is given with the provision that the XXX-commented out code is updated to work properly. I made a suggestion, but I see you asked Anton for feedback, so you two can figure it out.

- Fixes the inaccurate comment pointed out by Mihai. - Moves the defineMIME call to appendTo method and properly replaces the property names, values and color names used by CodeMirror with the ones provided by Gecko. - try : https://tbpl.mozilla.org/?tree=Try&rev=f89966b3eedc

Comment on attachment 816002 [details] [diff] [review] patch v0.2 Review of attachment 816002 [details] [diff] [review]: ----------------------------------------------------------------- Nicely done. As you'll see below, I have some comments and questions. To set the topic for this discussion, my goal is to keep editor.js as simple as possible. This means clearing up a few things and moving non-essential functions to the pluggable files. Thanks! ::: browser/devtools/sourceeditor/editor.js @@ +15,5 @@ > + * Maximum allowed vertical offset for the line index when you call > + * Editor.setCursorPosition(). > + * > + * @type number > + */ You don't need this giant comment here. Rename this to MAX_VERTICAL_OFFSET and add a one-line comment describing what it is. Also (just a general remark, I assume you copied this constant from SourceEditor), "Maximum allowed vertical offset" and MAX_VERTICAL_OFFSET mean the same thing and don't help at all. I still don't understand what it is. Is it padding? Also, since you assign a number to a constant it is quite obvious that this is a number and will always stay a number. So no need for a @type number here. @@ +83,5 @@ > + "charCoords", > + "coordsChar", > + "scrollTo", > + "lineCount", > + "markClean", This method is deprecated. We should use changeGeneration instead. @@ +95,5 @@ > > const editors = new WeakMap(); > > +// Globals to be computed once and used in all imports of this file. > +let gCSSPropertyList = []; It is not clear where these values are computed. Here's what I suggest you change. Make the IIFE below and actual function that returns an object with three properties cssprops, cssvalues and csscolors. Then here call this function: const { cssprops, cssvalues, csscolors } = getCSSData(); Making them constants makes sure nobody will ever change the reference (they still can mutate actual objects but oh well we work with JavaScript here). I changed the names here but you can keep yours. Although in the first case List in the name is redundant and in all other cases misleading (those are objects). @@ +429,5 @@ > + * > + * @return number > + * The line number, counting from 0. > + */ > + getTopIndex: function () { You don't return an index here, you return a line. Rename it to something like getFirstVisibleLine. Bonus points if you come up with shorter name. With this more descriptive name you won't need @return part of your comment. No need to state the obvious. Also, why do you need this method and not lineAtHeight(0, "local")? @@ +439,5 @@ > + * > + * @param {number} index > + * The line number, counting from 0. > + */ > + setTopIndex: function (index) { This method doesn't set an index, it scrolls to the line. So it should be called scrollToLine or something along these lines. @@ +447,5 @@ > + > + /** > + * Set the cursor position: line and column. > + * > + * @param number line If you change the signature per my suggestions below you won't need JavaDoc comments since it is possible to describe the signature in one sentence: "this function accepts a position object {line, ch} and a string that accepts the following values: ..." This file doesn't have @params and @returns and I'd like to keep it that way. By the way, what would really be useful is a description on how and where this method is being used or can be used. @@ +458,5 @@ > + * Editor.VERTICAL_ALIGN.TOP target line at top of view. > + * Editor.VERTICAL_ALIGN.CENTER target line at center of view. > + * Editor.VERTICAL_ALIGN.BOTTOM target line at bottom of view. > + */ > + setCursorPosition: function (line, column, align) { Name setCursorPosition is too ambiguous and its not clear what this function does, what is view and so on. Please use { line, ch } object instead of first two parameters to be consistent with the rest of methods and CodeMirror API. Along these lines, to be consistent with CodeMirror/Editor align should take a string and Editor.VERTICAL_* should be removed. Constants such as VERTICAL_ALIGN.TOP make sense when the underlying value can be changed in future, but they can and should be replaced by strings when the use case is a simple conditional (in your case it is a switch statement). For example, the VERTICAL_OFFSET in this file makes sense since it abstracts away the actual value that might be changed in future. VERTICAL_ALIGN.TOP, on the other hand, corresponds to an arbitrary number (zero) and doesn't abstract anything. So I'd like to see the signature of this method changed to (pos, align) where pos is a {line, ch} object and align is a string that accepts three values: "top", "center" and "bottom". The bigger issue here is whether this function useful for everyone or for StyleEditor only. If it's the latter, I'd like this function to be moved into a separate pluggable file. Since this patch is based on my Debugger patch (bug 919709) I suggest looking into it. There's a sourceeditor/debugger.js file that defines extensions to the Editor. @@ +476,5 @@ > + // VERTICAL_OFFSET is the maximum allowed value. > + let offset = Math.min(halfVisible, VERTICAL_OFFSET); > + > + let topIndex; > + switch (aAlign) { aAlign? Shouldn't it be align? Since tests didn't catch that, is this being used at all? Also, it could be rewritten as (I renamed topIndex to top): let top = { center: Math.max(line - halfVisible, 0), bottom: Math.max(line - linesVisible + offset, 0), top: Math.max(line - offset, 0) }[align]; @@ +514,5 @@ > }); > > +// Allowed vertical alignment options for the line index > +// when you call SourceEditor.setCaretPosition(). > +Editor.VERTICAL_ALIGN = { This is not needed, see above why. @@ +525,5 @@ > +// names, values and color names, we compute them so that they can replace > +// the ones used in CodeMirror while initiating an editor object. This is done > +// here instead of the file codemirror/css.js so as to leave that file untouched > +// and easily upgradable. > +(function() { This should be a non-IIFE function per my comments above. ::: browser/devtools/styleeditor/StyleSheetEditor.jsm @@ +394,2 @@ > > + bindings[ctrl(_("saveStyleSheet.commandkey"))] = () => { Is there really an underscore function?

Comment on attachment 816002 [details] [diff] [review] patch v0.2 Review of attachment 816002 [details] [diff] [review]: ----------------------------------------------------------------- Oops, forgot to clear the review.

I just went through your patch again and I don't think the 'align' argument in the setCursorPosition method is used anywhere at all. If it's not being used, kill it.

(In reply to Anton Kovalyov (:anton) from comment #13) > I just went through your patch again and I don't think the 'align' argument > in the setCursorPosition method is used anywhere at all. If it's not being > used, kill it. The CENTER and BOTTOM values are not being used anywhere. Ideally, I assume that the search feature of the editor should use the center method when the matching token is not visible on the screen. (That is the behavior of Firefox too)

(In reply to Anton Kovalyov (:anton) from comment #11) > Comment on attachment 816002 [details] [diff] [review] > patch v0.2 > > Review of attachment 816002 [details] [diff] [review]: > ----------------------------------------------------------------- > > Nicely done. As you'll see below, I have some comments and questions. To set > the topic for this discussion, my goal is to keep editor.js as simple as > possible. This means clearing up a few things and moving non-essential > functions to the pluggable files. > > Thanks! > > ::: browser/devtools/sourceeditor/editor.js > @@ +15,5 @@ > > + * Maximum allowed vertical offset for the line index when you call > > + * Editor.setCursorPosition(). > > + * > > + * @type number > > + */ > > You don't need this giant comment here. Rename this to MAX_VERTICAL_OFFSET > and add a one-line comment describing what it is. Also (just a general > remark, I assume you copied this constant from SourceEditor), "Maximum > allowed vertical offset" and MAX_VERTICAL_OFFSET mean the same thing and > don't help at all. I still don't understand what it is. Is it padding? > > Also, since you assign a number to a constant it is quite obvious that this > is a number and will always stay a number. So no need for a @type number > here. This is coming from the Style Editor world, where the functions are heavily documented :) As for what it is : Suppose you want to move to line number 123 and it is not even visible on the screen right now. Now if you choose the bottom or top options of the setCursorPosition, then instead of making line 123 as the first (or last) visible line, it will make it as the 4th (or 4th last) visible line, given that the total height of the editor permits it. > @@ +95,5 @@ > > > > const editors = new WeakMap(); > > > > +// Globals to be computed once and used in all imports of this file. > > +let gCSSPropertyList = []; > > It is not clear where these values are computed. Here's what I suggest you > change. Make the IIFE below and actual function that returns an object with > three properties cssprops, cssvalues and csscolors. Then here call this > function: > > const { cssprops, cssvalues, csscolors } = getCSSData(); > > Making them constants makes sure nobody will ever change the reference (they > still can mutate actual objects but oh well we work with JavaScript here). I > changed the names here but you can keep yours. Although in the first case > List in the name is redundant and in all other cases misleading (those are > objects). const here is kind of useless (as you also mentioned) but it wouldn't hurt anyone :) > @@ +429,5 @@ > > + * > > + * @return number > > + * The line number, counting from 0. > > + */ > > + getTopIndex: function () { > > You don't return an index here, you return a line. Rename it to something In Orion, offset was the term equivalent of the index in CM. > like getFirstVisibleLine. Bonus points if you come up with shorter name. Agreed. Not sure if smaller name will be this meaningful. > With this more descriptive name you won't need @return part of your comment. > No need to state the obvious. > > Also, why do you need this method and not lineAtHeight(0, "local")? lineAtHeight(0, "local") seems a bit Codemirror specific and private, but yeah, I should use this method inside the getTopIndex call. > @@ +447,5 @@ > > + > > + /** > > + * Set the cursor position: line and column. > > + * > > + * @param number line > > If you change the signature per my suggestions below you won't need JavaDoc > comments since it is possible to describe the signature in one sentence: > "this function accepts a position object {line, ch} and a string that > accepts the following values: ..." > > This file doesn't have @params and @returns and I'd like to keep it that way. > > By the way, what would really be useful is a description on how and where > this method is being used or can be used. > > @@ +458,5 @@ > > + * Editor.VERTICAL_ALIGN.TOP target line at top of view. > > + * Editor.VERTICAL_ALIGN.CENTER target line at center of view. > > + * Editor.VERTICAL_ALIGN.BOTTOM target line at bottom of view. > > + */ > > + setCursorPosition: function (line, column, align) { > > Name setCursorPosition is too ambiguous and its not clear what this function > does, what is view and so on. > > Please use { line, ch } object instead of first two parameters to be > consistent with the rest of methods and CodeMirror API. > > Along these lines, to be consistent with CodeMirror/Editor align should take > a string and Editor.VERTICAL_* should be removed. Constants such as > VERTICAL_ALIGN.TOP make sense when the underlying value can be changed in > future, but they can and should be replaced by strings when the use case is > a simple conditional (in your case it is a switch statement). > > For example, the VERTICAL_OFFSET in this file makes sense since it abstracts > away the actual value that might be changed in future. VERTICAL_ALIGN.TOP, > on the other hand, corresponds to an arbitrary number (zero) and doesn't > abstract anything. > > So I'd like to see the signature of this method changed to (pos, align) > where pos is a {line, ch} object and align is a string that accepts three > values: "top", "center" and "bottom". > > The bigger issue here is whether this function useful for everyone or for > StyleEditor only. If it's the latter, I'd like this function to be moved > into a separate pluggable file. Since this patch is based on my Debugger > patch (bug 919709) I suggest looking into it. There's a > sourceeditor/debugger.js file that defines extensions to the Editor. This is a general purpose method that was earlier present in SourceEditor files. Ideally, "every switching to a source editor line" should use the approach of having an option to vertically place the line too. (Search in debugger, scratchpad et. al. can use the CENTER option, like Firefox search works). More over I think the initial idea for this was that "it is ergonomically better to place the switched line (that was not in the view earlier) to be placed at a little offset (of say 2 lines) from the top, instead of the topmost line" I was thinking of merging this method into setCursor method. What do you think ? > @@ +476,5 @@ > > + // VERTICAL_OFFSET is the maximum allowed value. > > + let offset = Math.min(halfVisible, VERTICAL_OFFSET); > > + > > + let topIndex; > > + switch (aAlign) { > > aAlign? Shouldn't it be align? Since tests didn't catch that, is this being > used at all? Umm, last I remember, style inspector to style editor switch was using this, and also console to style editor switch. > Also, it could be rewritten as (I renamed topIndex to top): > let top = { > center: Math.max(line - halfVisible, 0), > bottom: Math.max(line - linesVisible + offset, 0), > top: Math.max(line - offset, 0) > }[align]; I like it. > @@ +514,5 @@ > > }); > > > > +// Allowed vertical alignment options for the line index > > +// when you call SourceEditor.setCaretPosition(). > > +Editor.VERTICAL_ALIGN = { > > This is not needed, see above why. Although I agree that the hardcoded values for align ("top", "center" and "bottom") are pretty obvious, but are we sure to hardcode these values, instead of letting them be like an enum .. > @@ +525,5 @@ > > +// names, values and color names, we compute them so that they can replace > > +// the ones used in CodeMirror while initiating an editor object. This is done > > +// here instead of the file codemirror/css.js so as to leave that file untouched > > +// and easily upgradable. > > +(function() { > > This should be a non-IIFE function per my comments above. > > ::: browser/devtools/styleeditor/StyleSheetEditor.jsm > @@ +394,2 @@ > > > > + bindings[ctrl(_("saveStyleSheet.commandkey"))] = () => { > > Is there really an underscore function? Yup, in StyleEditorUtil.jsm . It is the l10n method.

Need info is for the setCursorPosition and setCursor methods merge suggestion . Anton, do you think it makes sense to merge the two methods and make the default behavior as "top" (with 3 lines offset) (where we intelligently move the screen only when the line to be viewed is not visible) ? FWIW, this is what is currently happening when you jump to debugger or style editor from external tools (console to debugger, console to style editor, rule view to style editor)

(In reply to Girish Sharma [:Optimizer] from comment #16) > Need info is for the setCursorPosition and setCursor methods merge > suggestion . Anton, do you think it makes sense to merge the two methods and > make the default behavior as "top" (with 3 lines offset) (where we > intelligently move the screen only when the line to be viewed is not > visible) ? > > FWIW, this is what is currently happening when you jump to debugger or style > editor from external tools (console to debugger, console to style editor, > rule view to style editor) Yeah, merging these methods sounds good. Hopefully it won't break a lot of tests. :)

(In reply to Girish Sharma [:Optimizer] from comment #15) > > > @@ +476,5 @@ > > > + // VERTICAL_OFFSET is the maximum allowed value. > > > + let offset = Math.min(halfVisible, VERTICAL_OFFSET); > > > + > > > + let topIndex; > > > + switch (aAlign) { > > > > aAlign? Shouldn't it be align? Since tests didn't catch that, is this being > > used at all? > > Umm, last I remember, style inspector to style editor switch was using this, > and also console to style editor switch. In this case, we need more test (not necessarily in this patch) because nothing caught an obviously undefined variable. > > > @@ +514,5 @@ > > > }); > > > > > > +// Allowed vertical alignment options for the line index > > > +// when you call SourceEditor.setCaretPosition(). > > > +Editor.VERTICAL_ALIGN = { > > > > This is not needed, see above why. > > Although I agree that the hardcoded values for align ("top", "center" and > "bottom") are pretty obvious, but are we sure to hardcode these values, > instead of letting them be like an enum .. Yeah, this is consistent with CodeMirror API (and Editor is simply a thin wrapper around it). Plus this enum is also hardcoded. You will never ever change values (0, 1, 2) because they don't mean anything. So if you will need to change the key (VERTICAL_ALIGN, etc.) you will have to change it everywhere.

Addressed all comments. Tests are as green as the try of the debugger patch :)

Comment on attachment 818335 [details] [diff] [review] patch v0.3 Review of attachment 818335 [details] [diff] [review]: ----------------------------------------------------------------- Nicely done. I assume you'll land this after bug 919709 is landed? ::: browser/devtools/sourceeditor/editor.js @@ +12,5 @@ > const L10N_BUNDLE = "chrome://browser/locale/devtools/sourceeditor.properties"; > const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"; > > +// Maximum allowed margin (in number of lines) from top or bottom of the editor > +// while shifting to a line which was initially out of view. Very good! @@ +460,5 @@ > + }[align || "top"] || offset; > + > + // Bringing down the topLine to total lines in the editor if exceeding. > + topLine = Math.min(topLine, this.lineCount()); > + this.setFirstVisibleLine(topLine); I'd combine these two lines but it's a nitpick.

Yeah, I need a full green try before I can land it. And definetely we want bug 919709 in the tree. I will attach the rebased patch after you have a r+ and green try for the patch in bug 919709.

Turns out that the aligning feature was indeed not working in CM and thus the aAlign is undefined exception was not thrown because codeflow was not even reaching there. There were two things wrong: - cm.getViewPort() was not giving the top and the bottom visible lines, instead it gives the currently in DOM lines (as CM keeps on certain lines in DOM apart from the ones that are visible) - cm.setCursor() itself makes the selected line as the last visible line, thus the aligning part of the code was not touching the line as it was already visible. Fixed the above two issues. Asking for re-review as I have introduced some change sin debugger.js to allow centering of the searched token's line

Comment on attachment 819582 [details] [diff] [review] patch v0.4 Review of attachment 819582 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/debugger/debugger-toolbar.js @@ +865,5 @@ > return; > } > > aLine = aLine > DebuggerView.editor.lineCount() ? 0 : aLine - 1; > + DebuggerView.editor.setCursor({ line: aLine, ch: 0 }, "center"); Can't "center" be default?

(In reply to Victor Porof [:vp] from comment #23) > Can't "center" be default? I am impartial towards both as I think that both have equal amount of usage throughout our tools. But if the question is that do we want to "center" the line instead of "top" for the cases in which currently the line is placed at "top", (cases being, step-in/out, jump to debugger from console and profiler, jump to style editor from console and inspector) then I think we need more discussion :) . (We might need some UR opinion here.)

I'd prefer to keep the current default to ship this sooner. I'll review this patch later today.

Comment on attachment 819582 [details] [diff] [review] patch v0.4 Review of attachment 819582 [details] [diff] [review]: ----------------------------------------------------------------- r+ but you will need to re-base and do a Try run before landing, there were changes in the debugger patch before I landed it. ::: browser/devtools/sourceeditor/editor.js @@ +469,5 @@ > + * Aligns the provided line to either "top", "center" or "bottom" of the > + * editor view with a maximum margin of MAX_VERTICAL_OFFSET lines from top or > + * bottom. > + */ > + alignLine: function(line, align) { I'm not sure about the name 'alignLine' but I don't have anything better from the top of my head. Also, add a space after 'function'.

Rebased myself to save you time, there were trivial conflicts that I fixed without testing locally. Here's a Try push: https://tbpl.mozilla.org/?tree=Try&rev=75605f3b2cdb

orange because the "posFromIndex" got removed from CM_MAPPINGS in the rebased patch. I will post updated patch once I get back to my machine.

Rebased. Fixed the test orange. try: https://tbpl.mozilla.org/?tree=Try&rev=8766d0398a03

Damnit! Missed to convert indexFromPos to getOffset try again https://tbpl.mozilla.org/?tree=Try&rev=29b872ade935

Comment on attachment 821160 [details] [diff] [review] patch v0.6 Review of attachment 821160 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/sourceeditor/editor.js @@ +77,5 @@ > + "lineCount", > + "scrollTo", > + "charCoords", > + "coordsChar", > + "lineAtHeight", Why would you expose any of these as public especially since they're not used outside of this module? Please ask anton again for review.

(In reply to Victor Porof [:vp] from comment #31) > Comment on attachment 821160 [details] [diff] [review] > patch v0.6 > > Review of attachment 821160 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/devtools/sourceeditor/editor.js > @@ +77,5 @@ > > + "lineCount", > > + "scrollTo", > > + "charCoords", > > + "coordsChar", > > + "lineAtHeight", > > Why would you expose any of these as public especially since they're not > used outside of this module? Please ask anton again for review. The last patch r+'ed by Anton had these. So basically I have not added anything extra after the r+.

Mechanical change from this.xxx to cm.xxx

Comment on attachment 821281 [details] [diff] [review] patch v0.7 Review of attachment 821281 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.

green try with the latest patch : https://tbpl.mozilla.org/?tree=Try&rev=45ece0e7b82d landed in fx-team : https://tbpl.mozilla.org/?tree=Fx-Team&rev=d6018de7fe82