I swear we have a bug open for this but I couldn't find one. Feel free to mark as dupe if you can find it. Anyways, we should autocomplete CSS properties in the rule view.

Yup, totally!

Trying to give it a shot!

This works. Needs tests plus a question: Should we show some popup kind of thing with 5 suggestions instead of autocompleting to the first one in-place. If popup is too much, may be we can allow the user to press TAB or UP/DOWN to cycle through other suggestions. What chrome does is to show a popup with remaining suggestions and allow user to cycle through them using UP/DOWN.

Does tab/shift+tab cycle through suggestions in chrome? I think we should land the initial stuff as fast as possible and then push further improvements into follow ups.

(In reply to Nick Fitzgerald [:fitzgen] from comment #4) > Does tab/shift+tab cycle through suggestions in chrome? No, it jumps to the value input box completing the property that was selected in the popup atm. > I think we should land the initial stuff as fast as possible and then push > further improvements into follow ups. I can include the popup in this patch in no time, if that is the concern ;)

This adds popup. Currently clicking the popup to select a completion does not work as as soon as you mousedown on popup, the input box gets blurred and thus destroys itself. You can cycle suggestion via UP/DOWN anyways. Next up will be tests. I will add the click support in a followup as it requires logical change in inplaceeditor object.

This patch: 1. Takes care of all edge cases which were throwing exceptions in previous patch. 2. Changes the behavior of autocomplete popup to cycle through to other corner when reaching one corner item. (This has already been discussed and agreed upon with Mihai). Asking Mihai for review for web console related changes. 3. Adds 2 tests to check if property names are completed correctly. try : https://tbpl.mozilla.org/?tree=Try&rev=f613cd8e1cd2

> + ["d", "direction", 0, 3], If you can, I highly suggest you make "d" complete to "display" instead. (And similarly o -> overflow, a -> animation, c -> color, etc. Chrome's inspector should have nice list I believe.)

(In reply to Simon Lindholm from comment #8) > > + ["d", "direction", 0, 3], > If you can, I highly suggest you make "d" complete to "display" instead. > (And similarly o -> overflow, a -> animation, c -> color, etc. Chrome's > inspector should have nice list I believe.) Well, we can bring prediction later on in follow ups :). Right now, the list is alphabetically sorted. May be (in a follow up), we can keep track of what user is actually selecting for each entry, like when he pressed "d", he pressed DOWN to select "display". And in this manner suggest the most selected entry itself.

Previous patch had test failures as there is no END key on Mac. I also added a 50ms time gap before I check for completed value and popup in the tests so as to make sure that the entries will be there [As both of these oprations are async, they can cause race conditions if simple executeSoon is used]. I also made searching in the list faster a bit.

new try : https://tbpl.mozilla.org/?tree=Try&rev=d1bc5c7aa761

try is green this time.

(In reply to Simon Lindholm from comment #8) > > + ["d", "direction", 0, 3], > If you can, I highly suggest you make "d" complete to "display" instead. > (And similarly o -> overflow, a -> animation, c -> color, etc. Chrome's > inspector should have nice list I believe.) Filed 896969. Chrome also do something like that and they have a sample dictionary for WebInspector.

-1 for blocking this from landing with bug 896969. That should just be a follow up after this lands. "Perfect is the enemy of good" and all that.

I also dont see a point in waiting for bug 896969 to land. Thus a *separate* bug :). I just wanted to link the 2 bugs and this is the correct linking.

(In reply to Girish Sharma [:Optimizer] from comment #15) > I also dont see a point in waiting for bug 896969 to land. Thus a *separate* > bug :). > > I just wanted to link the 2 bugs and this is the correct linking. In that case this bug should block bug 896969 instead of depending on it.

Comment on attachment 778873 [details] [diff] [review] patch v1.1 (more reviewers = faster review) (amirite?)

Comment on attachment 778873 [details] [diff] [review] patch v1.1 Mihai's review is still required for the Autocomplete Popup changes. :)

Comment on attachment 778873 [details] [diff] [review] patch v1.1 Review of attachment 778873 [details] [diff] [review]: ----------------------------------------------------------------- Awesome work, just a few things to address. ::: browser/devtools/shared/AutocompletePopup.jsm @@ +54,5 @@ > + theme = Services.prefs.getCharPref("devtools.theme"); > + this.autoThemeEnabled = true; > + // Setup theme change listener. > + this.handleThemeChange = this.handleThemeChange.bind(this); > + Cu.import("resource:///modules/devtools/gDevTools.jsm"); Use this at the top of the file otherwise it looks like it is scoped to the constructor but it is referenced by the destructor. You could also use a lazy getter. let {gDevTools} = Cu.import("resource:///modules/devtools/gDevTools.jsm", {}); @@ +509,5 @@ > > return this.__scrollbarWidth; > }, > + > + handleThemeChange: function AP_handleThemeChange(aEvent, aData) The JS guys are trying to remove all function names because the JS engine is better at naming them than we are. Please use this instead: handleThemeChange: function(aEvent, aData) We also no longer have to prefix params with an "a". ::: browser/devtools/shared/inplace-editor.js @@ +753,5 @@ > prevent = true; > + } else if (increment && this.popup && this.popup.isOpen) { > + cycling = true; > + prevent = true; > + increment > 0 ? this.popup.selectPreviousItem() Use an if statement as it is easier to pick out at a glance. @@ +763,5 @@ > + if (aEvent.keyCode === Ci.nsIDOMKeyEvent.DOM_VK_BACK_SPACE || > + aEvent.keyCode === Ci.nsIDOMKeyEvent.DOM_VK_DELETE || > + aEvent.keyCode === Ci.nsIDOMKeyEvent.DOM_VK_LEFT || > + aEvent.keyCode === Ci.nsIDOMKeyEvent.DOM_VK_RIGHT) { > + this.popup && this.popup.isOpen && this.popup.hidePopup(); Same again, use an if statement here. @@ +904,5 @@ > + if (!this.popup) { > + return; > + } > + let finalList = [], i = 0, count = 0, length = list.length; > + for (; i < length && count < MAX_POPUP_ENTRIES; i++) { Multiple lets on a single line, please use: let finalList = []; let length = list.length; for (let i = 0, count = 0; i < length && count < MAX_POPUP_ENTRIES; i++) { ::: browser/devtools/styleinspector/rule-view.js @@ +1886,5 @@ > return Cc["@mozilla.org/inspector/dom-utils;1"].getService(Ci.inIDOMUtils); > }); > + > +let _autocompletePopup = null; > +function getAutocompletePopup(doc) { Please move this method inside the CssRuleView prototype, make it a getter and change _autocompletePopup to this._autocompletePopup. You should then destroy it from the destroy method. ::: browser/devtools/styleinspector/test/browser_bug893965_css_property_completion_new_property.js @@ +129,5 @@ > + doc.title = "Rule View Test"; > + waitForFocus(openRuleView, content); > + }, true); > + > + content.location = "data:text/html,<h1 style='border: 1px solid red'>Some header text</h1>"; Just a nit but it is usually good to include the test filename in the webpage because then if the tab fails to close for any reason it is easy to see where it came from.

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #19) > Comment on attachment 778873 [details] [diff] [review] > patch v1.1 > > Review of attachment 778873 [details] [diff] [review]: > ----------------------------------------------------------------- > > Awesome work, just a few things to address. > > ::: browser/devtools/shared/AutocompletePopup.jsm > @@ +54,5 @@ > > + theme = Services.prefs.getCharPref("devtools.theme"); > > + this.autoThemeEnabled = true; > > + // Setup theme change listener. > > + this.handleThemeChange = this.handleThemeChange.bind(this); > > + Cu.import("resource:///modules/devtools/gDevTools.jsm"); > > Use this at the top of the file otherwise it looks like it is scoped to the > constructor but it is referenced by the destructor. You could also use a > lazy getter. I have it here so that the import is done only in the case of auto theme. In the destructor, I check if auto theme is enables, then only remove the handler. > let {gDevTools} = Cu.import("resource:///modules/devtools/gDevTools.jsm", > {}); > > @@ +509,5 @@ > > > > return this.__scrollbarWidth; > > }, > > + > > + handleThemeChange: function AP_handleThemeChange(aEvent, aData) > > The JS guys are trying to remove all function names because the JS engine is > better at naming them than we are. Please use this instead: > handleThemeChange: function(aEvent, aData) Just going along with the flow of the file. > We also no longer have to prefix params with an "a". ditto. Also, that is just personal style preference :) > @@ +904,5 @@ > > + if (!this.popup) { > > + return; > > + } > > + let finalList = [], i = 0, count = 0, length = list.length; > > + for (; i < length && count < MAX_POPUP_ENTRIES; i++) { > > Multiple lets on a single line, please use: > let finalList = []; > let length = list.length; > for (let i = 0, count = 0; i < length && count < MAX_POPUP_ENTRIES; i++) { > > ::: browser/devtools/styleinspector/rule-view.js > @@ +1886,5 @@ > > return Cc["@mozilla.org/inspector/dom-utils;1"].getService(Ci.inIDOMUtils); > > }); > > + > > +let _autocompletePopup = null; > > +function getAutocompletePopup(doc) { > > Please move this method inside the CssRuleView prototype, make it a getter > and > change _autocompletePopup to this._autocompletePopup. You should then destroy > it from the destroy method. I cannot make this method any object specific here because the popup is required by many places. Passing the reference will mean addition of an extra argument to many methods. I also wanted to maintain one single popup so made the method global. > ::: > browser/devtools/styleinspector/test/ > browser_bug893965_css_property_completion_new_property.js > @@ +129,5 @@ > > + doc.title = "Rule View Test"; > > + waitForFocus(openRuleView, content); > > + }, true); > > + > > + content.location = "data:text/html,<h1 style='border: 1px solid red'>Some header text</h1>"; > > Just a nit but it is usually good to include the test filename in the > webpage because then if the tab fails to close for any reason it is easy to > see where it came from. So instead of "Some header text", something like "Test for bug ######" ?

(In reply to Girish Sharma [:Optimizer] from comment #20) > (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #19) > > Comment on attachment 778873 [details] [diff] [review] > > patch v1.1 > > > > Review of attachment 778873 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Awesome work, just a few things to address. > > > > ::: browser/devtools/shared/AutocompletePopup.jsm > > @@ +54,5 @@ > > > + theme = Services.prefs.getCharPref("devtools.theme"); > > > + this.autoThemeEnabled = true; > > > + // Setup theme change listener. > > > + this.handleThemeChange = this.handleThemeChange.bind(this); > > > + Cu.import("resource:///modules/devtools/gDevTools.jsm"); > > > > Use this at the top of the file otherwise it looks like it is scoped to the > > constructor but it is referenced by the destructor. You could also use a > > lazy getter. > > I have it here so that the import is done only in the case of auto theme. In > the destructor, I check if auto theme is enables, then only remove the > handler. > > This is exactly the situation where you should be using a lazy getter. > > let {gDevTools} = Cu.import("resource:///modules/devtools/gDevTools.jsm", > > {}); > > > > @@ +509,5 @@ > > > > > > return this.__scrollbarWidth; > > > }, > > > + > > > + handleThemeChange: function AP_handleThemeChange(aEvent, aData) > > > > The JS guys are trying to remove all function names because the JS engine is > > better at naming them than we are. Please use this instead: > > handleThemeChange: function(aEvent, aData) > > Just going along with the flow of the file. > > > We also no longer have to prefix params with an "a". > > ditto. Also, that is just personal style preference :) > > > > > @@ +904,5 @@ > > > + if (!this.popup) { > > > + return; > > > + } > > > + let finalList = [], i = 0, count = 0, length = list.length; > > > + for (; i < length && count < MAX_POPUP_ENTRIES; i++) { > > > > Multiple lets on a single line, please use: > > let finalList = []; > > let length = list.length; > > for (let i = 0, count = 0; i < length && count < MAX_POPUP_ENTRIES; i++) { > > > > ::: browser/devtools/styleinspector/rule-view.js > > @@ +1886,5 @@ > > > return Cc["@mozilla.org/inspector/dom-utils;1"].getService(Ci.inIDOMUtils); > > > }); > > > + > > > +let _autocompletePopup = null; > > > +function getAutocompletePopup(doc) { > > > > Please move this method inside the CssRuleView prototype, make it a getter > > and > > change _autocompletePopup to this._autocompletePopup. You should then destroy > > it from the destroy method. > > I cannot make this method any object specific here because the popup is > required by many places. Passing the reference will mean addition of an > extra argument to many methods. I also wanted to maintain one single popup > so made the method global. > It still makes no sense to duplicate the same method in each file. Why not move it to AutocompletePopup.jsm? > > ::: > > browser/devtools/styleinspector/test/ > > browser_bug893965_css_property_completion_new_property.js > > @@ +129,5 @@ > > > + doc.title = "Rule View Test"; > > > + waitForFocus(openRuleView, content); > > > + }, true); > > > + > > > + content.location = "data:text/html,<h1 style='border: 1px solid red'>Some header text</h1>"; > > > > Just a nit but it is usually good to include the test filename in the > > webpage because then if the tab fails to close for any reason it is easy to > > see where it came from. > > So instead of "Some header text", something like "Test for bug ######" ? Yes, exactly.

Addresses review comments. this brings yet another change in AutocompletePopup.jsm. Now there are static methods to get and destroy popup instances based on String keys. This was needed as I want a single popup per rule view, markup panel and style editor; each.

Forgot the XUL document check.

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

Comment on attachment 780644 [details] [diff] [review] patch v2.1 Review of attachment 780644 [details] [diff] [review]: ----------------------------------------------------------------- Much better. A couple of tiny nits and a couple of issues with the tests that may also apply to your other bugs: ::: browser/devtools/shared/AutocompletePopup.jsm @@ +546,5 @@ > + * @param Object aOptions > + * The options object. See AutocompletePopup constructor. > + */ > +AutocompletePopup.getInstance = function AP_getInstance(aKey, aDoc, aOptions) { > + if (!(aDoc instanceof Ci.nsIDOMXULDocument)) { trailing space @@ +548,5 @@ > + */ > +AutocompletePopup.getInstance = function AP_getInstance(aKey, aDoc, aOptions) { > + if (!(aDoc instanceof Ci.nsIDOMXULDocument)) { > + // XUL panel and richlist box will not function in non XUL documents. > + return null; trailing space ::: browser/devtools/styleinspector/test/browser_bug893965_css_property_completion_existing_property.js @@ +77,5 @@ > + let brace = ruleViewWindow.document.querySelector(".ruleview-propertyname"); > + > + waitForEditorFocus(brace.parentNode, function onNewElement(aEditor) { > + editor = aEditor; > + editor.input.addEventListener("keypress", checkState, false); You need to remove this event in the destructor or debug builds will be orange. Also, have you tried using "keyup" or "input" events here? That way you may not need the setTimeout in checkState. These comments also apply to the other test. @@ +99,5 @@ > + EventUtils.synthesizeKey(key, {}, ruleViewWindow); > +} > + > +function checkState(event) { > + window.setTimeout(function() { Please add a comment explaining why you need to use setTimeout here. Using setTimeout often leads to intermittent oranges so it should always be a last choice.

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #25) > browser/devtools/styleinspector/test/ > browser_bug893965_css_property_completion_existing_property.js > @@ +77,5 @@ > > + let brace = ruleViewWindow.document.querySelector(".ruleview-propertyname"); > > + > > + waitForEditorFocus(brace.parentNode, function onNewElement(aEditor) { > > + editor = aEditor; > > + editor.input.addEventListener("keypress", checkState, false); > > You need to remove this event in the destructor or debug builds will be > orange. its not possible as before the test ends, the input is blurred and thus destroyed itself. I don't see any orange because of leaks in the debug builds too. I think since the node is destroyed, it doesn't matter ? (but I am not sure) > Also, have you tried using "keyup" or "input" events here? That way you may > not need the setTimeout in checkState. The issue here is that my checking is done asynchronously to the main feature. So when a keypress happens, I call the mayBeSuggestAutocomplete method asynchronously to get the correct input value. Then I have to filter a long array to get all the matches (though, I can optimize more on the searching part) and then fill those values in the input box and the popup. This also includes opening up of the popup. All this can take some time on a debug build as they have to spew a lot of things and are thus slow. That is why I put a timeout as there is no event to listen to to know when the actual processing has been done and it is ready to be tested. > These comments also apply to the other test. > > @@ +99,5 @@ > > + EventUtils.synthesizeKey(key, {}, ruleViewWindow); > > +} > > + > > +function checkState(event) { > > + window.setTimeout(function() { > > Please add a comment explaining why you need to use setTimeout here. Using > setTimeout often leads to intermittent oranges so it should always be a last > choice. Should I comment something similar to what I wrote above ?

Comment on attachment 780644 [details] [diff] [review] patch v2.1 Review of attachment 780644 [details] [diff] [review]: ----------------------------------------------------------------- I do not want the AutocompletePopup to manage its own instances. Rule view seems to use only one instance for all of its editors. Please make changes in rule view to accommodate such needs, not inside the autocomplete popup code. Thank you! I am willing to be swayed to accept these changes if you bring really good arguments as to why the rule view really *needs* them. ::: browser/devtools/shared/AutocompletePopup.jsm @@ +58,5 @@ > + if (theme == "auto") { > + theme = Services.prefs.getCharPref("devtools.theme"); > + this.autoThemeEnabled = true; > + // Setup theme change listener. > + this.handleThemeChange = this.handleThemeChange.bind(this); Please make handleThemeChange() a private method.

(In reply to Mihai Sucan [:msucan] from comment #27) > Comment on attachment 780644 [details] [diff] [review] > patch v2.1 > > Review of attachment 780644 [details] [diff] [review]: > ----------------------------------------------------------------- > > I do not want the AutocompletePopup to manage its own instances. Rule view > seems to use only one instance for all of its editors. Please make changes > in rule view to accommodate such needs, not inside the autocomplete popup > code. Thank you! > > I am willing to be swayed to accept these changes if you bring really good > arguments as to why the rule view really *needs* them. Its not just the rule view, in bug 896181, it is the markup view and in the future style editor bugs, it would be the style editor and what not. This was specially difficult to manage because if it is handled outside of autocompletepopup.jsm, then the same code is being repeated in each file which needs a unique instance per document. > ::: browser/devtools/shared/AutocompletePopup.jsm > @@ +58,5 @@ > > + if (theme == "auto") { > > + theme = Services.prefs.getCharPref("devtools.theme"); > > + this.autoThemeEnabled = true; > > + // Setup theme change listener. > > + this.handleThemeChange = this.handleThemeChange.bind(this); > > Please make handleThemeChange() a private method. Sure.

(In reply to Girish Sharma [:Optimizer] from comment #26) > (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #25) > > Also, have you tried using "keyup" or "input" events here? That way you may > > not need the setTimeout in checkState. > > The issue here is that my checking is done asynchronously to the main > feature. So when a keypress happens, I call the mayBeSuggestAutocomplete > method asynchronously to get the correct input value. Then I have to filter > a long array to get all the matches (though, I can optimize more on the > searching part) and then fill those values in the input box and the popup. > This also includes opening up of the popup. All this can take some time on a > debug build as they have to spew a lot of things and are thus slow. That is > why I put a timeout as there is no event to listen to to know when the > actual processing has been done and it is ready to be tested. > > > > These comments also apply to the other test. > > > > @@ +99,5 @@ > > > + EventUtils.synthesizeKey(key, {}, ruleViewWindow); > > > +} > > > + > > > +function checkState(event) { > > > + window.setTimeout(function() { > > > > Please add a comment explaining why you need to use setTimeout here. Using > > setTimeout often leads to intermittent oranges so it should always be a last > > choice. > > Should I comment something similar to what I wrote above ? Yes, but shorter, we only need something like "The keypress handler is async and can take some time so we need to use setTimeout here." You may want to increase the timeout a little as that will make oranges less likely when the test servers are under load (unless you are have already done so).

(In reply to Mihai Sucan [:msucan] from comment #27) > > I do not want the AutocompletePopup to manage its own instances. Rule view > seems to use only one instance for all of its editors. Please make changes > in rule view to accommodate such needs, not inside the autocomplete popup > code. Thank you! > I strongly agree with Mihai on this one, the AutocompletePopup.* static methods don't seem quite pertinent because they currently only handle extremely simple logic (for example, destroyInstance doesn't anything other than calling a destroy() method, which you can call yourself anyway because you already have a reference to the instance). As far as I can tell, a consumer would now have to deal with extra cognitive overhead given that instances are managed in two places (their code + the key/value instance references on the AutocompletePopup itself). This seems bad to me.

(In reply to Victor Porof [:vp] from comment #30) > (In reply to Mihai Sucan [:msucan] from comment #27) > > > > I do not want the AutocompletePopup to manage its own instances. Rule view > > seems to use only one instance for all of its editors. Please make changes > > in rule view to accommodate such needs, not inside the autocomplete popup > > code. Thank you! > > > > I strongly agree with Mihai on this one, the AutocompletePopup.* static > methods don't seem quite pertinent because they currently only handle > extremely simple logic (for example, destroyInstance doesn't anything other > than calling a destroy() method, which you can call yourself anyway because > you already have a reference to the instance). I am perfectly okay with copying over this static method [0] to 3 different files, thus repeating the code. It was Mike's suggestion and it felt the right thing to avoid duplicate code across the code base thus I moved it into Autocompletepopup.jsm. > As far as I can tell, a consumer would now have to deal with extra cognitive > overhead given that instances are managed in two places (their code + the > key/value instance references on the AutocompletePopup itself). This seems > bad to me. As in ? Do you see any extra overhead in the patch ? I don't see any. Its just keeping reference to one constant string. [0] https://bugzilla.mozilla.org/attachment.cgi?id=778873&action=diff#a/browser/devtools/styleinspector/rule-view.js_sec6

(In reply to Girish Sharma [:Optimizer] from comment #31) > > As in ? Do you see any extra overhead in the patch ? I don't see any. Its > just keeping reference to one constant string. I said *cognitive* overhead :)

Girish: sorry, but the argument does not hold for me. You only need to do once |new AutocompletePopup()| in rule view, then reuse the same instance object inside each editor. This is perfectly fine inside the markup view and inside the style editor. This change in the patch does not look like it is any kind of code reuse, it looks like a indirection for no obvious reason. I would appreciate having only one way to use the popup. The review comment still holds. Thank you for your understanding.

Did as asked by Mihai. (I wanted to avoid this chained property reference introduced in this patch and next ones) Addressed other review comments.

try push : https://tbpl.mozilla.org/?tree=Try&rev=8af93aa97b87

try is green wrt this bug.

Comment on attachment 781281 [details] [diff] [review] patch v3.0 Review of attachment 781281 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me.

Comment on attachment 781281 [details] [diff] [review] patch v3.0 Review of attachment 781281 [details] [diff] [review]: ----------------------------------------------------------------- Patch looks good. Thanks for the update. r+ with the comments below addressed. (do note I only reviewed the popup and webconsole-related code) ::: browser/devtools/shared/AutocompletePopup.jsm @@ +59,5 @@ > + theme = Services.prefs.getCharPref("devtools.theme"); > + this.autoThemeEnabled = true; > + // Setup theme change listener. > + this._handleThemeChange = this._handleThemeChange.bind(this); > + gDevTools.on("pref-changed", this.__handleThemeChange); There's an extra _ here. @@ +518,5 @@ > + * Manages theme switching for the popup based on the devtools.theme pref. > + * > + * @private > + */ > + _handleThemeChange: function AP__handleThemeChange(aEvent, aData) nit: jsdoc for the arguments. ::: browser/devtools/webconsole/test/browser_webconsole_bug_585991_autocomplete_popup.js @@ +68,2 @@ > > + // Was previously : is(popup.selectedIndex, -1, "no index is selected"); Please remove this comment. It only causes confusion.

landed in fx-team: https://hg.mozilla.org/integration/fx-team/rev/d8e9483d9436

Patch that landed.

(In reply to Mihai Sucan [:msucan] from comment #38) > Comment on attachment 781281 [details] [diff] [review] > patch v3.0 > > Review of attachment 781281 [details] [diff] [review]: > ----------------------------------------------------------------- > > Patch looks good. Thanks for the update. r+ with the comments below > addressed. (do note I only reviewed the popup and webconsole-related code) Thanks for the review :) > ::: browser/devtools/shared/AutocompletePopup.jsm > @@ +59,5 @@ > > + theme = Services.prefs.getCharPref("devtools.theme"); > > + this.autoThemeEnabled = true; > > + // Setup theme change listener. > > + this._handleThemeChange = this._handleThemeChange.bind(this); > > + gDevTools.on("pref-changed", this.__handleThemeChange); > > There's an extra _ here. Nice catch! > @@ +518,5 @@ > > + * Manages theme switching for the popup based on the devtools.theme pref. > > + * > > + * @private > > + */ > > + _handleThemeChange: function AP__handleThemeChange(aEvent, aData) > > nit: jsdoc for the arguments. done. > ::: > browser/devtools/webconsole/test/ > browser_webconsole_bug_585991_autocomplete_popup.js > @@ +68,2 @@ > > > > + // Was previously : is(popup.selectedIndex, -1, "no index is selected"); > > Please remove this comment. It only causes confusion. done.