Bug 1339952 Sort order of permission prompts draft
authorAndrew Swan <aswan@mozilla.com>
Thu, 23 Feb 2017 18:27:34 -0800
changeset 490572 55145a9f8357a5562112eefdb5ccd7344be999c8
parent 490268 30187c04e172c7090fe2c05f37427f6f2c9b46e4
child 547300 c8286426d41618a1c19cfdc9c5577269f4444b2e
push id47143
push useraswan@mozilla.com
push dateTue, 28 Feb 2017 20:49:13 +0000
bugs1339952
milestone54.0a1
Bug 1339952 Sort order of permission prompts MozReview-Commit-ID: 6ngylPGJ5EE
browser/base/content/test/webextensions/head.js
browser/modules/ExtensionsUI.jsm
--- a/browser/base/content/test/webextensions/head.js
+++ b/browser/base/content/test/webextensions/head.js
@@ -114,16 +114,47 @@ function is_visible(element) {
   // Hiding a parent element will hide all its children
   if (element.parentNode != element.ownerDocument)
     return is_visible(element.parentNode);
 
   return true;
 }
 
 /**
+ * Check the contents of an individual permission string.
+ * This function is fairly specific to the use here and probably not
+ * suitable for re-use elsewhere...
+ *
+ * @param {string} string
+ *        The string value to check (i.e., pulled from the DOM)
+ * @param {string} key
+ *        The key in browser.properties for the localized string to
+ *        compare with.
+ * @param {string|null} param
+ *        Optional string to substitute for %S in the localized string.
+ * @param {string} msg
+ *        The message to be emitted as part of the actual test.
+ */
+function checkPermissionString(string, key, param, msg) {
+  let localizedString = param ?
+                        gBrowserBundle.formatStringFromName(key, [param], 1) :
+                        gBrowserBundle.GetStringFromName(key);
+
+  // If this is a parameterized string and the parameter isn't given,
+  // just do a simple comparison of the text before and after the %S
+  if (localizedString.includes("%S")) {
+    let i = localizedString.indexOf("%S");
+    ok(string.startsWith(localizedString.slice(0, i)), msg);
+    ok(string.endsWith(localizedString.slice(i + 2)), msg);
+  } else {
+    is(string, localizedString, msg);
+  }
+}
+
+/**
  * Test that install-time permission prompts work for a given
  * installation method.
  *
  * @param {Function} installFn
  *        Callable that takes the name of an xpi file to install and
  *        starts to install it.  Should return a Promise that resolves
  *        when the install is finished or rejects if the install is canceled.
  *
@@ -191,17 +222,34 @@ async function testInstallMethod(install
       // The icon should come from the extension, don't bother with the precise
       // path, just make sure we've got a jar url pointing to the right path
       // inside the jar.
       ok(icon.startsWith("jar:file://"), "Icon is a jar url");
       ok(icon.endsWith("/icon.png"), "Icon is icon.png inside a jar");
 
       is(header.getAttribute("hidden"), "", "Permission list header is visible");
       is(ul.childElementCount, 5, "Permissions list has 5 entries");
-      // Real checking of the contents here is deferred until bug 1316996 lands
+
+      checkPermissionString(ul.children[0].textContent,
+                            "webextPerms.hostDescription.wildcard",
+                            "wildcard.domain",
+                            "First permission is domain permission");
+      checkPermissionString(ul.children[1].textContent,
+                            "webextPerms.hostDescription.oneSite",
+                            "singlehost.domain",
+                            "Second permission is single host permission");
+      checkPermissionString(ul.children[2].textContent,
+                            "webextPerms.description.nativeMessaging", null,
+                            "Third permission is nativeMessaging");
+      checkPermissionString(ul.children[3].textContent,
+                            "webextPerms.description.tabs", null,
+                            "Fourth permission is tabs");
+      checkPermissionString(ul.children[4].textContent,
+                            "webextPerms.description.history", null,
+                            "Fifth permission is history");
     } else if (filename == NO_PERMS_XPI) {
       // This extension has no icon, it should have the default
       ok(isDefaultIcon(icon), "Icon is the default extension icon");
 
       is(header.getAttribute("hidden"), "true", "Permission list header is hidden");
       is(ul.childElementCount, 0, "Permissions list has 0 entries");
     }
 
--- a/browser/modules/ExtensionsUI.jsm
+++ b/browser/modules/ExtensionsUI.jsm
@@ -215,33 +215,17 @@ this.ExtensionsUI = {
       result.header = "";
       result.text = bundle.formatStringFromName("webextPerms.updateText", [addonName], 1);
       result.acceptText = bundle.GetStringFromName("webextPerms.updateAccept.label");
       result.acceptKey = bundle.GetStringFromName("webextPerms.updateAccept.accessKey");
     }
 
     let perms = info.permissions || {hosts: [], permissions: []};
 
-    result.msgs = [];
-    for (let permission of perms.permissions) {
-      let key = `webextPerms.description.${permission}`;
-      if (permission == "nativeMessaging") {
-        let brandBundle = Services.strings.createBundle(BRAND_PROPERTIES);
-        let appName = brandBundle.GetStringFromName("brandShortName");
-        result.msgs.push(bundle.formatStringFromName(key, [appName], 1));
-      } else {
-        try {
-          result.msgs.push(bundle.GetStringFromName(key));
-        } catch (err) {
-          // We deliberately do not include all permissions in the prompt.
-          // So if we don't find one then just skip it.
-        }
-      }
-    }
-
+    // First classify our host permissions
     let allUrls = false, wildcards = [], sites = [];
     for (let permission of perms.hosts) {
       if (permission == "<all_urls>") {
         allUrls = true;
         break;
       }
       let match = /^[htps*]+:\/\/([^/]+)\//.exec(permission);
       if (!match) {
@@ -251,16 +235,20 @@ this.ExtensionsUI = {
         allUrls = true;
       } else if (match[1].startsWith("*.")) {
         wildcards.push(match[1].slice(2));
       } else {
         sites.push(match[1]);
       }
     }
 
+    // Format the host permissions.  If we have a wildcard for all urls,
+    // a single string will suffice.  Otherwise, show domain wildcards
+    // first, then individual host permissions.
+    result.msgs = [];
     if (allUrls) {
       result.msgs.push(bundle.GetStringFromName("webextPerms.hostDescription.allUrls"));
     } else {
       // Formats a list of host permissions.  If we have 4 or fewer, display
       // them all, otherwise display the first 3 followed by an item that
       // says "...plus N others"
       function format(list, itemKey, moreKey) {
         function formatItems(items) {
@@ -278,16 +266,40 @@ this.ExtensionsUI = {
       }
 
       format(wildcards, "webextPerms.hostDescription.wildcard",
              "webextPerms.hostDescription.tooManyWildcards");
       format(sites, "webextPerms.hostDescription.oneSite",
              "webextPerms.hostDescription.tooManySites");
     }
 
+    let permissionKey = perm => `webextPerms.description.${perm}`;
+
+    // Next, show the native messaging permission if it is present.
+    const NATIVE_MSG_PERM = "nativeMessaging";
+    if (perms.permissions.includes(NATIVE_MSG_PERM)) {
+      let brandBundle = Services.strings.createBundle(BRAND_PROPERTIES);
+      let appName = brandBundle.GetStringFromName("brandShortName");
+      result.msgs.push(bundle.formatStringFromName(permissionKey(NATIVE_MSG_PERM), [appName], 1));
+    }
+
+    // Finally, show remaining permissions, in any order.
+    for (let permission of perms.permissions) {
+      // Handled above
+      if (permission == "nativeMessaging") {
+        continue;
+      }
+      try {
+        result.msgs.push(bundle.GetStringFromName(permissionKey(permission)));
+      } catch (err) {
+        // We deliberately do not include all permissions in the prompt.
+        // So if we don't find one then just skip it.
+      }
+    }
+
     return result;
   },
 
   showPermissionsPrompt(browser, strings, icon) {
     function eventCallback(topic) {
       if (topic == "showing") {
         let doc = this.browser.ownerDocument;
         doc.getElementById("addon-webext-perm-header").innerHTML = strings.header;