Bug 1345294 - add an eslint rule to reject usage of {get,set}ComplexValue for nsISupportsString and suggest {get,set}StringPref instead, and make it pass, r=Mossop.
authorFlorian Quèze <florian@queze.net>
Thu, 16 Mar 2017 19:26:02 +0100
changeset 348174 79bacf0ea46664a09a1f09996e70d0d3a4af042d
parent 348173 5a8192a650e92565aa2e85721569dad58cc1922c
child 348175 3b2c7853ae71bd4ec36eb0c8be1a5a4200ade4f8
push id39092
push userkwierso@gmail.com
push dateFri, 17 Mar 2017 18:14:05 +0000
treeherderautoland@88576fd417e7 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersMossop
bugs1345294
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1345294 - add an eslint rule to reject usage of {get,set}ComplexValue for nsISupportsString and suggest {get,set}StringPref instead, and make it pass, r=Mossop.
.eslintrc.js
browser/base/content/aboutDialog.js
browser/components/nsBrowserGlue.js
browser/modules/SocialService.jsm
browser/modules/test/unit/social/test_SocialServiceMigration21.js
browser/modules/test/unit/social/test_SocialServiceMigration22.js
devtools/shared/gcli/commands/cmd.js
toolkit/components/exthelper/extApplication.js
toolkit/content/widgets/preferences.xml
tools/lint/docs/linters/eslint-plugin-mozilla.rst
tools/lint/eslint/eslint-plugin-mozilla/lib/index.js
tools/lint/eslint/eslint-plugin-mozilla/lib/rules/avoid-nsISupportsString-preferences.js
tools/lint/eslint/eslint-plugin-mozilla/package.json
tools/lint/eslint/eslint-plugin-mozilla/tests/avoid-nsISupportsString-preferences.js
--- a/.eslintrc.js
+++ b/.eslintrc.js
@@ -2,16 +2,17 @@
 
 module.exports = {
   // When adding items to this file please check for effects on sub-directories.
   "plugins": [
     "mozilla"
   ],
   "rules": {
     "mozilla/avoid-removeChild": "error",
+    "mozilla/avoid-nsISupportsString-preferences": "error",
     "mozilla/import-globals": "warn",
     "mozilla/no-import-into-var-and-global": "error",
     "mozilla/no-useless-parameters": "error",
     "mozilla/no-useless-removeEventListener": "error",
     "mozilla/use-default-preference-values": "error",
     "mozilla/use-ownerGlobal": "error",
 
     // No (!foo in bar) or (!object instanceof Class)
--- a/browser/base/content/aboutDialog.js
+++ b/browser/base/content/aboutDialog.js
@@ -7,39 +7,30 @@
 // Services = object with smart getters for common XPCOM services
 Components.utils.import("resource://gre/modules/Services.jsm");
 Components.utils.import("resource://gre/modules/AppConstants.jsm");
 
 function init(aEvent) {
   if (aEvent.target != document)
     return;
 
-  try {
-    var distroId = Services.prefs.getCharPref("distribution.id");
-    if (distroId) {
-      var distroVersion = Services.prefs.getCharPref("distribution.version");
-
-      var distroIdField = document.getElementById("distributionId");
-      distroIdField.value = distroId + " - " + distroVersion;
-      distroIdField.style.display = "block";
+  var distroId = Services.prefs.getCharPref("distribution.id", "");
+  if (distroId) {
+    var distroVersion = Services.prefs.getCharPref("distribution.version");
 
-      try {
-        // This is in its own try catch due to bug 895473 and bug 900925.
-        var distroAbout = Services.prefs.getComplexValue("distribution.about",
-          Components.interfaces.nsISupportsString);
-        var distroField = document.getElementById("distribution");
-        distroField.value = distroAbout;
-        distroField.style.display = "block";
-      } catch (ex) {
-        // Pref is unset
-        Components.utils.reportError(ex);
-      }
+    var distroIdField = document.getElementById("distributionId");
+    distroIdField.value = distroId + " - " + distroVersion;
+    distroIdField.style.display = "block";
+
+    var distroAbout = Services.prefs.getStringPref("distribution.about", "");
+    if (distroAbout) {
+      var distroField = document.getElementById("distribution");
+      distroField.value = distroAbout;
+      distroField.style.display = "block";
     }
-  } catch (e) {
-    // Pref is unset
   }
 
   // Include the build ID and display warning if this is an "a#" (nightly or aurora) build
   let versionField = document.getElementById("version");
   let version = Services.appinfo.version;
   if (/a\d+$/.test(version)) {
     let buildID = Services.appinfo.appBuildID;
     let year = buildID.slice(0, 4);
--- a/browser/components/nsBrowserGlue.js
+++ b/browser/components/nsBrowserGlue.js
@@ -1833,29 +1833,26 @@ BrowserGlue.prototype = {
     if (currentUIVersion < 24) {
       // Reset homepage pref for users who have it set to start.mozilla.org
       // or google.com/firefox.
       const HOMEPAGE_PREF = "browser.startup.homepage";
       if (Services.prefs.prefHasUserValue(HOMEPAGE_PREF)) {
         const DEFAULT =
           Services.prefs.getDefaultBranch(HOMEPAGE_PREF)
                         .getComplexValue("", Ci.nsIPrefLocalizedString).data;
-        let value =
-          Services.prefs.getComplexValue(HOMEPAGE_PREF, Ci.nsISupportsString);
+        let value = Services.prefs.getStringPref(HOMEPAGE_PREF);
         let updated =
-          value.data.replace(/https?:\/\/start\.mozilla\.org[^|]*/i, DEFAULT)
-                    .replace(/https?:\/\/(www\.)?google\.[a-z.]+\/firefox[^|]*/i,
-                             DEFAULT);
-        if (updated != value.data) {
+          value.replace(/https?:\/\/start\.mozilla\.org[^|]*/i, DEFAULT)
+               .replace(/https?:\/\/(www\.)?google\.[a-z.]+\/firefox[^|]*/i,
+                        DEFAULT);
+        if (updated != value) {
           if (updated == DEFAULT) {
             Services.prefs.clearUserPref(HOMEPAGE_PREF);
           } else {
-            value.data = updated;
-            Services.prefs.setComplexValue(HOMEPAGE_PREF,
-                                           Ci.nsISupportsString, value);
+            Services.prefs.setStringPref(HOMEPAGE_PREF, updated);
           }
         }
       }
     }
 
     if (currentUIVersion < 25) {
       // Make sure the doNotTrack value conforms to the conversion from
       // three-state to two-state. (This reverts a setting of "please track me"
--- a/browser/modules/SocialService.jsm
+++ b/browser/modules/SocialService.jsm
@@ -165,18 +165,17 @@ function getOriginActivationType(origin)
   return "foreign";
 }
 
 var ActiveProviders = {
   get _providers() {
     delete this._providers;
     this._providers = {};
     try {
-      let pref = Services.prefs.getComplexValue("social.activeProviders",
-                                                Ci.nsISupportsString);
+      let pref = Services.prefs.getStringPref("social.activeProviders");
       this._providers = JSON.parse(pref);
     } catch (ex) {}
     return this._providers;
   },
 
   has(origin) {
     return (origin in this._providers);
   },
--- a/browser/modules/test/unit/social/test_SocialServiceMigration21.js
+++ b/browser/modules/test/unit/social/test_SocialServiceMigration21.js
@@ -34,20 +34,18 @@ function* testMigration(manifest, next) 
   // look at social.activeProviders, we should have migrated into that, and
   // we should be set as a user level pref after migration
   do_check_false(MANIFEST_PREFS.prefHasUserValue(manifest.origin));
   // we need to access the providers for everything to initialize
   yield SocialService.getProviderList(next);
   do_check_true(SocialService.enabled);
   do_check_true(Services.prefs.prefHasUserValue("social.activeProviders"));
 
-  let activeProviders;
-  let pref = Services.prefs.getComplexValue("social.activeProviders",
-                                            Ci.nsISupportsString);
-  activeProviders = JSON.parse(pref);
+  let activeProviders =
+    JSON.parse(Services.prefs.getStringPref("social.activeProviders"));
   do_check_true(activeProviders[manifest.origin]);
   do_check_true(MANIFEST_PREFS.prefHasUserValue(manifest.origin));
   do_check_true(JSON.parse(DEFAULT_PREFS.getCharPref(manifest.origin)).builtin);
 
   let userPref = JSON.parse(MANIFEST_PREFS.getCharPref(manifest.origin));
   do_check_true(parseInt(userPref.updateDate) > 0);
   // migrated providers wont have an installDate
   do_check_true(userPref.installDate === 0);
--- a/browser/modules/test/unit/social/test_SocialServiceMigration22.js
+++ b/browser/modules/test/unit/social/test_SocialServiceMigration22.js
@@ -41,20 +41,18 @@ function* testMigration(manifest, next) 
   // look at social.activeProviders, we should have migrated into that, and
   // we should be set as a user level pref after migration
   do_check_false(MANIFEST_PREFS.prefHasUserValue(manifest.origin));
   // we need to access the providers for everything to initialize
   yield SocialService.getProviderList(next);
   do_check_true(SocialService.enabled);
   do_check_true(Services.prefs.prefHasUserValue("social.activeProviders"));
 
-  let activeProviders;
-  let pref = Services.prefs.getComplexValue("social.activeProviders",
-                                            Ci.nsISupportsString);
-  activeProviders = JSON.parse(pref);
+  let activeProviders =
+    JSON.parse(Services.prefs.getStringPref("social.activeProviders"));
   do_check_true(activeProviders[manifest.origin]);
   do_check_true(MANIFEST_PREFS.prefHasUserValue(manifest.origin));
   do_check_true(JSON.parse(DEFAULT_PREFS.getCharPref(manifest.origin)).builtin);
 
   let userPref = JSON.parse(MANIFEST_PREFS.getCharPref(manifest.origin));
   do_check_true(parseInt(userPref.updateDate) > 0);
   // migrated providers wont have an installDate
   do_check_true(userPref.installDate === 0);
--- a/devtools/shared/gcli/commands/cmd.js
+++ b/devtools/shared/gcli/commands/cmd.js
@@ -12,20 +12,16 @@ const gcli = require("gcli/index");
 const l10n = require("gcli/l10n");
 
 loader.lazyGetter(this, "prefBranch", function () {
   let prefService = Cc["@mozilla.org/preferences-service;1"]
                       .getService(Ci.nsIPrefService);
   return prefService.getBranch(null).QueryInterface(Ci.nsIPrefBranch2);
 });
 
-loader.lazyGetter(this, "supportsString", function () {
-  return Cc["@mozilla.org/supports-string;1"].createInstance(Ci.nsISupportsString);
-});
-
 loader.lazyImporter(this, "NetUtil", "resource://gre/modules/NetUtil.jsm");
 
 const PREF_DIR = "devtools.commands.dir";
 
 /**
  * Load all the .mozcmd files in the directory pointed to by PREF_DIR
  * @return A promise of an array of items suitable for gcli.addItems or
  * using in gcli.addItemsByModule
@@ -165,17 +161,16 @@ exports.items = [
       }
     ],
     returnType: "string",
     get hidden() {
       // !prefBranch.prefHasUserValue(PREF_DIR);
       return true;
     },
     exec: function (args, context) {
-      supportsString.data = args.directory;
-      prefBranch.setComplexValue(PREF_DIR, Ci.nsISupportsString, supportsString);
+      prefBranch.setStringPref(PREF_DIR, args.directory);
 
       gcli.load();
 
       return l10n.lookupFormat("cmdStatus3", [ args.directory ]);
     }
   }
 ];
--- a/toolkit/components/exthelper/extApplication.js
+++ b/toolkit/components/exthelper/extApplication.js
@@ -280,20 +280,17 @@ PreferenceBranch.prototype = {
     return aValue;
   },
 
   setValue: function prefs_sv(aName, aValue) {
     var type = aValue != null ? aValue.constructor.name : "";
 
     switch (type) {
       case "String":
-        var str = Components.classes["@mozilla.org/supports-string;1"]
-                            .createInstance(Ci.nsISupportsString);
-        str.data = aValue;
-        this._prefs.setComplexValue(aName, Ci.nsISupportsString, str);
+        this._prefs.setStringPref(aName, aValue);
         break;
       case "Boolean":
         this._prefs.setBoolPref(aName, aValue);
         break;
       case "Number":
         this._prefs.setIntPref(aName, aValue);
         break;
       default:
--- a/toolkit/content/widgets/preferences.xml
+++ b/toolkit/content/widgets/preferences.xml
@@ -379,21 +379,17 @@
                                 .createInstance(Components.interfaces.nsIPrefLocalizedString);
             pls.data = val;
             this.preferences.rootBranch
                 .setComplexValue(this.name, Components.interfaces.nsIPrefLocalizedString, pls);
             break;
           case "string":
           case "unichar":
           case "fontname":
-            var iss = Components.classes["@mozilla.org/supports-string;1"]
-                                .createInstance(Components.interfaces.nsISupportsString);
-            iss.data = val;
-            this.preferences.rootBranch
-                .setComplexValue(this.name, Components.interfaces.nsISupportsString, iss);
+            this.preferences.rootBranch.setStringPref(this.name, val);
             break;
           case "file":
             var lf;
             if (typeof(val) == "string") {
               lf = Components.classes["@mozilla.org/file/local;1"]
                              .createInstance(Components.interfaces.nsILocalFile);
               lf.persistentDescriptor = val;
               if (!lf.exists())
--- a/tools/lint/docs/linters/eslint-plugin-mozilla.rst
+++ b/tools/lint/docs/linters/eslint-plugin-mozilla.rst
@@ -39,16 +39,21 @@ Rules
 =====
 
 avoid-removeChild
 -----------------
 
 Rejects using element.parentNode.removeChild(element) when element.remove()
 can be used instead.
 
+avoid-nsISupportsString-preferences
+-----------------------------------
+
+Rejects using getComplexValue and setComplexValue with nsISupportsString.
+
 balanced-listeners
 ------------------
 
 Checks that for every occurence of 'addEventListener' or 'on' there is an
 occurence of 'removeEventListener' or 'off' with the same event name.
 
 
 import-globals
--- a/tools/lint/eslint/eslint-plugin-mozilla/lib/index.js
+++ b/tools/lint/eslint/eslint-plugin-mozilla/lib/index.js
@@ -20,16 +20,18 @@ module.exports = {
     "simpletest": require("../lib/environments/simpletest.js")
   },
   processors: {
     ".xml": require("../lib/processors/xbl-bindings"),
     ".js": require("../lib/processors/self-hosted")
   },
   rules: {
     "avoid-removeChild": require("../lib/rules/avoid-removeChild"),
+    "avoid-nsISupportsString-preferences":
+      require("../lib/rules/avoid-nsISupportsString-preferences"),
     "balanced-listeners": require("../lib/rules/balanced-listeners"),
     "import-globals": require("../lib/rules/import-globals"),
     "import-headjs-globals": require("../lib/rules/import-headjs-globals"),
     "mark-test-function-used": require("../lib/rules/mark-test-function-used"),
     "no-aArgs": require("../lib/rules/no-aArgs"),
     "no-cpows-in-tests": require("../lib/rules/no-cpows-in-tests"),
     "no-single-arg-cu-import": require("../lib/rules/no-single-arg-cu-import"),
     "no-import-into-var-and-global":
@@ -42,16 +44,17 @@ module.exports = {
     "reject-some-requires": require("../lib/rules/reject-some-requires"),
     "use-default-preference-values":
       require("../lib/rules/use-default-preference-values"),
     "use-ownerGlobal": require("../lib/rules/use-ownerGlobal"),
     "var-only-at-top-level": require("../lib/rules/var-only-at-top-level")
   },
   rulesConfig: {
     "avoid-removeChild": 0,
+    "avoid-nsISupportsString-preferences": 0,
     "balanced-listeners": 0,
     "import-globals": 0,
     "import-headjs-globals": 0,
     "mark-test-function-used": 0,
     "no-aArgs": 0,
     "no-cpows-in-tests": 0,
     "no-single-arg-cu-import": 0,
     "no-import-into-var-and-global": 0,
new file mode 100644
--- /dev/null
+++ b/tools/lint/eslint/eslint-plugin-mozilla/lib/rules/avoid-nsISupportsString-preferences.js
@@ -0,0 +1,51 @@
+/**
+ * @fileoverview Rejects using getComplexValue and setComplexValue with
+ *               nsISupportsString.
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+
+"use strict";
+
+// -----------------------------------------------------------------------------
+// Rule Definition
+// -----------------------------------------------------------------------------
+
+var helpers = require("../helpers");
+
+function isNsISupportsString(arg) {
+  let isNsISupportsStringIdentifier = obj =>
+    obj.type == "Identifier" && obj.name == "nsISupportsString";
+  return isNsISupportsStringIdentifier(arg) ||
+         (arg.type == "MemberExpression" &&
+          isNsISupportsStringIdentifier(arg.property));
+}
+
+module.exports = function(context) {
+
+  // ---------------------------------------------------------------------------
+  // Public
+  //  --------------------------------------------------------------------------
+
+  return {
+    "CallExpression": function(node) {
+      let callee = node.callee;
+      if (callee.type !== "MemberExpression" ||
+          callee.property.type !== "Identifier" ||
+          node.arguments.length < 2 ||
+          !isNsISupportsString(node.arguments[1])) {
+        return;
+      }
+
+      if (callee.property.name == "getComplexValue") {
+        context.report(node, "use getStringPref instead of " +
+                             "getComplexValue with nsISupportsString");
+      } else if (callee.property.name == "setComplexValue") {
+        context.report(node, "use setStringPref instead of " +
+                             "setComplexValue with nsISupportsString");
+      }
+    }
+  };
+};
--- a/tools/lint/eslint/eslint-plugin-mozilla/package.json
+++ b/tools/lint/eslint/eslint-plugin-mozilla/package.json
@@ -1,11 +1,11 @@
 {
   "name": "eslint-plugin-mozilla",
-  "version": "0.2.29",
+  "version": "0.2.30",
   "description": "A collection of rules that help enforce JavaScript coding standard in the Mozilla project.",
   "keywords": [
     "eslint",
     "eslintplugin",
     "eslint-plugin",
     "mozilla",
     "firefox"
   ],
new file mode 100644
--- /dev/null
+++ b/tools/lint/eslint/eslint-plugin-mozilla/tests/avoid-nsISupportsString-preferences.js
@@ -0,0 +1,40 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+//------------------------------------------------------------------------------
+// Requirements
+//------------------------------------------------------------------------------
+
+var rule = require("../lib/rules/avoid-nsISupportsString-preferences");
+
+//------------------------------------------------------------------------------
+// Tests
+//------------------------------------------------------------------------------
+
+function invalidCode(code, accessType = "get") {
+  let message = "use " + accessType + "StringPref instead of " +
+                accessType + "ComplexValue with nsISupportsString";
+  return {code: code, errors: [{message: message, type: "CallExpression"}]};
+}
+
+exports.runTest = function(ruleTester) {
+  ruleTester.run("no-useless-removeEventListener", rule, {
+    valid: [
+      "branch.getStringPref('name');",
+      "branch.getComplexValue('name', Ci.nsIPrefLocalizedString);",
+      "branch.setStringPref('name', 'blah');",
+      "branch.setComplexValue('name', Ci.nsIPrefLocalizedString, pref);"
+    ],
+    invalid: [
+      invalidCode("branch.getComplexValue('name', Ci.nsISupportsString);"),
+      invalidCode("branch.getComplexValue('name', nsISupportsString);"),
+      invalidCode("branch.getComplexValue('name', Ci.nsISupportsString).data;"),
+      invalidCode("branch.setComplexValue('name', Ci.nsISupportsString, str);",
+                  "set"),
+      invalidCode("branch.setComplexValue('name', nsISupportsString, str);",
+                  "set")
+    ]
+  });
+};