Comment on attachment 8780864 [details] Bug 1294989 - Enable eslint for browser/components/preferences. https://reviewboard.mozilla.org/r/71444/#review68960 Thanks for doing this. Some of this code is dying to be tidied a bit (like making the manual for (i++) array walking more idiomatic, and the duplication between cookies and permissions' treeviews, and...) but that can be done another day, this patch was enough work to write and review as it was. :-) ::: browser/components/preferences/fonts.js:80 (Diff revision 2) > // Only care in in-content prefs > if (!window.frameElement) { > return true; > } Rip this out while we're here, please. ::: browser/components/preferences/fonts.js:93 (Diff revision 2) > if (!preferences.length) { > - return; > + return false; > } AIUI this should be return true. Just like with "normal" events, returning undefined is not the same as returning false, and the docs say false prevents closing / saving the prefs. ::: browser/components/preferences/in-content/advanced.js:95 (Diff revision 2) > setEventListener("viewSecurityDevicesButton", "command", > gAdvancedPane.showSecurityDevices); > setEventListener("cacheSize", "change", > gAdvancedPane.updateCacheSizePref); > > -#ifdef MOZ_WIDGET_GTK > + if (AppConstants.MOZ_WIDGET_GTK) { This doesn't exist. You want ``` if (AppConstants.MOZ_WIDGET_TOOLKIT.startsWith("gtk")) ``` I expect (dxr shows that on dxr the value is "gtk3" and I guess "gtk2" is also possible and should also pass this -- that or define MOZ_WIDGET_GTK on AppConstants...). ::: browser/components/preferences/in-content/main.js:5 (Diff revision 2) > Components.utils.import("resource://gre/modules/Downloads.jsm"); > Components.utils.import("resource://gre/modules/FileUtils.jsm"); > Components.utils.import("resource://gre/modules/Task.jsm"); > Components.utils.import("resource:///modules/ShellService.jsm"); > Components.utils.import("resource:///modules/TransientPrefs.jsm"); So AppConstants.jsm is not requested here, but it's requested by a number of other files included in preferences.js. Can you file a (good first bug?) followup to make this system more sane and put the common lazy module getters / imports in preferences.js and dedup them? ::: browser/components/preferences/in-content/main.js:338 (Diff revision 2) > useCurrent.label = useCurrent.getAttribute("label2"); > else > useCurrent.label = useCurrent.getAttribute("label1"); > > // In this case, the button's disabled state is set by preferences.xml. > - if (document.getElementById > + if (document.getElementById("pref.browser.homepage.disable_button.current_page").locked) Nit: please put the pref name in a temp var. ::: browser/components/preferences/in-content/sync.js:557 (Diff revision 2) > (event.charCode == KeyEvent.DOM_VK_SPACE || event.keyCode == KeyEvent.DOM_VK_RETURN))) { > return true; > - } else { > - return false; > } > + return false; if (foo) return true; else return false; is almost as bad a pet peeve of mine as else-after-return... Could just: ``` return <insert if condition here> ``` but I guess this keeps more blame and might be more readable according to some? Up to you.

Comment on attachment 8780864 [details] Bug 1294989 - Enable eslint for browser/components/preferences. https://reviewboard.mozilla.org/r/71444/#review68960 Filed bug 1295000 for this cleanup. > So AppConstants.jsm is not requested here, but it's requested by a number of other files included in preferences.js. Can you file a (good first bug?) followup to make this system more sane and put the common lazy module getters / imports in preferences.js and dedup them? Filed bug 1294999.

Pushed by jwein@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/90324e0a64e7 Enable eslint for browser/components/preferences. r=Gijs