We need to provide a way to reload the option for autoclose brackets, kind of like resetIndentUnit does for whitespace-related properties. This would read the devtools.editor.autoclosebrackets preference and remember the original config that was passed in (it currently resets that value if autoclose was false on initialization). See https://bugzilla.mozilla.org/show_bug.cgi?id=1029511#c4 and https://bug964356.bugzilla.mozilla.org/attachment.cgi?id=8428318 for more information.

This exposes a function that reloads all of the editor prefs, and automatically calls it when any of them change. It also adds a test to this effect.

Initial patch forgot to call clearUserPref at the end of the test

Hah, wrong patch. I think this one will be right.

Comment on attachment 8449457 [details] [diff] [review] editor-reload-prefs.patch Review of attachment 8449457 [details] [diff] [review]: ----------------------------------------------------------------- Nice! ::: browser/devtools/sourceeditor/autocomplete.js @@ +340,5 @@ > +/** > + * Returns whether autocompletion is enabled for this editor. > + * Used for testing > + */ > +function isAutocompletionEnabled({ ed }) { I would rename this function, since, afaict, `privates` can store pretty much anything, not just autocompletion prefs. ::: browser/devtools/sourceeditor/editor.js @@ +28,5 @@ > const RE_JUMP_TO_LINE = /^(\d+):?(\d+)?/; > > const {Promise: promise} = Cu.import("resource://gre/modules/Promise.jsm", {}); > const events = require("devtools/toolkit/event-emitter"); > +const { PrefObserver } = require("devtools/styleeditor/utils"); We should really make this a shared util, maybe it'd go along nicely with ViewHelpers.Prefs inside ViewHelpers.jsm? Followup material for sure. @@ +413,5 @@ > + useAutoClose ? this.config.autoCloseBracketsSaved : false); > + > + // If alternative keymap is provided, use it. > + const keyMap = Services.prefs.getCharPref(KEYMAP); > + if (keyMap === "emacs" || keyMap === "vim" || keyMap === "sublime") Nit: a const VALID_KEYMAPS = new Set(["emacs", "vim", "sublime]); and if (VALID_KEYMAPS.has(keyMap)) ... would be nicer here imo.

(In reply to Victor Porof [:vporof][:vp] from comment #4) > Comment on attachment 8449457 [details] [diff] [review] > editor-reload-prefs.patch > > Review of attachment 8449457 [details] [diff] [review]: > ----------------------------------------------------------------- > > Nice! > > ::: browser/devtools/sourceeditor/autocomplete.js > @@ +340,5 @@ > > +/** > > + * Returns whether autocompletion is enabled for this editor. > > + * Used for testing > > + */ > > +function isAutocompletionEnabled({ ed }) { > > I would rename this function, since, afaict, `privates` can store pretty > much anything, not just autocompletion prefs. I'm not sure I follow - privates is a WeakMap declared inside of this file. The only time is a key is set is inside initializeAutoCompletion and the only time a key is deleted is inside of the destroy callbacks.

(In reply to Victor Porof [:vporof][:vp] from comment #4) > ::: browser/devtools/sourceeditor/editor.js > @@ +28,5 @@ > > const RE_JUMP_TO_LINE = /^(\d+):?(\d+)?/; > > > > const {Promise: promise} = Cu.import("resource://gre/modules/Promise.jsm", {}); > > const events = require("devtools/toolkit/event-emitter"); > > +const { PrefObserver } = require("devtools/styleeditor/utils"); > > We should really make this a shared util, maybe it'd go along nicely with > ViewHelpers.Prefs inside ViewHelpers.jsm? Followup material for sure. I agree - the PrefObserver would be great functionality to share. We could move it into ViewHelpers.Prefs as you suggest, or make a new ViewHelpers.PrefObserver to prevent any overlap with the API. Heather, what do you think? As a note, we also currently have a 'pref-changed' event emit gDevTools that is kind of confusing because it fires only when prefs are changed through the options panel. We could also make gDevTools use this utility and observe on the "devtools." branch to it will emit more consistently.

(In reply to Brian Grinstead [:bgrins] from comment #6) > I agree - the PrefObserver would be great functionality to share. We could > move it into ViewHelpers.Prefs as you suggest, or make a new > ViewHelpers.PrefObserver to prevent any overlap with the API. Heather, what > do you think? > > As a note, we also currently have a 'pref-changed' event emit gDevTools that > is kind of confusing because it fires only when prefs are changed through > the options panel. We could also make gDevTools use this utility and > observe on the "devtools." branch to it will emit more consistently. I like all of this. ViewHelpers doesn't sound like the right file, maybe something more generic.

OK so when running the tests I realized there were a lot of places that were not properly destroying editors and were waiting to leak for something like this. I'm a bit surprised that they weren't already leaking with the iframe inside of the editor. This new version updates netmonitor, shader editor, markup view, and scratchpad to make sure things are being cleaned up. It also removes any external calls to setupAutoCompletion and instead introduces autocompleteOpts for use with the style editor (so that it gets initialized with the correct options). This is actually nicer, because before you'd have to do something like `editor({autocomplete:true}); editor.appendTo(foo).then(() => {editor.setupAutoCompletion(); });` and now you can just do `editor({autocomplete: true}); editor.appendTo(foo);`.

Comment on attachment 8450145 [details] [diff] [review] editor-reload-prefs.patch Review of attachment 8450145 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, modulo the addition of _editors maps. See below. ::: browser/devtools/netmonitor/netmonitor-view.js @@ +129,5 @@ > + > + for (let ed of this._editors.values()) { > + ed.destroy(); > + } > + this._editors.clear(); I don't understand why we need an extra _editors map? Why not just: _destroyPanes: Task.async(function*() { ... for (let promise of this._editorPromises) { let editor = yield promise; editor.clear(); } }), ::: browser/devtools/shadereditor/shadereditor.js @@ +364,5 @@ > this._toggleListeners("off"); > + for (let ed of this._editors.values()) { > + ed.destroy(); > + } > + this._editors.clear(); Same here.

(In reply to Victor Porof [:vporof][:vp] from comment #9) > I don't understand why we need an extra _editors map? Why not just: > > _destroyPanes: Task.async(function*() { > ... > for (let promise of this._editorPromises) { > let editor = yield promise; > editor.clear(); > } > }), I was assuming async destroy functions would be an antipattern - if the toolbox is closing, then it is closing whether we want it to wait or not, right? Or does it wait for async destroys to finish?

OK, this version seems to fix all the leaks: https://tbpl.mozilla.org/?tree=Try&rev=b4e40098a291. Heather, I had to make some changes to StyleSheetEditor.jsm to prevent leaking tests. this.sourceEditor is only set once it is appended and ready, which is what we want. However, this can cause leaky tests if the test ends before it has been appended. So the change is setting a `this._sourceEditor` property immediately after instantiating it so we have a reference to clean it up. Also, I'm binding the editor events to named functions instead of anonymous so I can clean those up in destroy as well. Let me know what you think. Victor, for shadereditor.js simply putting the destroy in a yield didn't work, because in browser_se_first-run.js the call to _toggleListeners in destroy is the first call to _getEditor, so it actually instantiates new editors that never get appended (since it is in the middle of the destroy function). The fix I put in place it keeping a bit to track whether it is destroyed, and if so then resolve immediately in _getEditor. Another option would be do what the previous patch did (keep track of the pre-appended editors in a map).

Comment on attachment 8451625 [details] [diff] [review] editor-reload-prefs.patch Review of attachment 8451625 [details] [diff] [review]: ----------------------------------------------------------------- This is fine.

Comment on attachment 8451625 [details] [diff] [review] editor-reload-prefs.patch Review of attachment 8451625 [details] [diff] [review]: ----------------------------------------------------------------- Style Editor change looks good.

OK, rebased and updated commit message