Bug 1331172 - Current permission state should use the past tense. r=johannh,Paolo draft
authorDão Gottwald <dao@mozilla.com>
Mon, 16 Jan 2017 20:33:00 +0100
changeset 464095 1dd675b427fbbd298a46fdfc68b2bebd950deac1
parent 464075 aa3e49299a3aa5cb0db570532e3df9e75d30c2d1
child 542854 4aacf177fbddbf285294ee9eb81cd6d7e3e28b4a
push id42269
push userbmo:jhofmann@mozilla.com
push dateFri, 20 Jan 2017 10:42:42 +0000
reviewersjohannh, Paolo
bugs1331172
milestone53.0a1
Bug 1331172 - Current permission state should use the past tense. r=johannh,Paolo MozReview-Commit-ID: 7XqHa0xrfsh
browser/base/content/browser.js
browser/base/content/pageinfo/permissions.js
browser/locales/en-US/chrome/browser/sitePermissions.properties
browser/modules/SitePermissions.jsm
browser/modules/test/browser_SitePermissions.js
browser/modules/test/xpcshell/test_SitePermissions.js
--- a/browser/base/content/browser.js
+++ b/browser/base/content/browser.js
@@ -7407,20 +7407,22 @@ var gIdentityHandler = {
             found = true;
             permission.inUse = true;
             break;
           }
           if (!found) {
             // If the permission item we were looking for doesn't exist,
             // the user has temporarily allowed sharing and we need to add
             // an item in the permissions array to reflect this.
-            let permission =
-              SitePermissions.getPermissionDetails(id, SitePermissions.SCOPE_REQUEST);
-            permission.inUse = true;
-            permissions.push(permission);
+            permissions.push({
+              id,
+              state: SitePermissions.ALLOW,
+              scope: SitePermissions.SCOPE_REQUEST,
+              inUse: true,
+            });
           }
         }
       }
     }
     for (let permission of permissions) {
       let item = this._createPermissionItem(permission);
       this._permissionList.appendChild(item);
     }
@@ -7471,17 +7473,17 @@ var gIdentityHandler = {
     stateLabel.setAttribute("class", "identity-popup-permission-state-label");
     let {state, scope} = aPermission;
     // If the user did not permanently allow this device but it is currently
     // used, set the variables to display a "temporarily allowed" info.
     if (state != SitePermissions.ALLOW && aPermission.inUse) {
       state = SitePermissions.ALLOW;
       scope = SitePermissions.SCOPE_REQUEST;
     }
-    stateLabel.textContent = SitePermissions.getStateLabel(state, scope);
+    stateLabel.textContent = SitePermissions.getCurrentStateLabel(state, scope);
 
     let button = document.createElement("button");
     button.setAttribute("class", "identity-popup-permission-remove-button");
     let tooltiptext = gNavigatorBundle.getString("permissions.remove.tooltip");
     button.setAttribute("tooltiptext", tooltiptext);
     button.addEventListener("command", () => {
 	  let browser = gBrowser.selectedBrowser;
       // Only resize the window if the reload hint was previously hidden.
--- a/browser/base/content/pageinfo/permissions.js
+++ b/browser/base/content/pageinfo/permissions.js
@@ -130,17 +130,17 @@ function createRow(aPartId) {
   controls.appendChild(spacer);
 
   let radiogroup = document.createElement("radiogroup");
   radiogroup.setAttribute("id", radiogroupId);
   radiogroup.setAttribute("orient", "horizontal");
   for (let state of SitePermissions.getAvailableStates(aPartId)) {
     let radio = document.createElement("radio");
     radio.setAttribute("id", aPartId + "#" + state);
-    radio.setAttribute("label", SitePermissions.getStateLabel(state));
+    radio.setAttribute("label", SitePermissions.getMultichoiceStateLabel(state));
     radio.setAttribute("command", commandId);
     radiogroup.appendChild(radio);
   }
   controls.appendChild(radiogroup);
 
   row.appendChild(controls);
 
   document.getElementById("permList").appendChild(row);
--- a/browser/locales/en-US/chrome/browser/sitePermissions.properties
+++ b/browser/locales/en-US/chrome/browser/sitePermissions.properties
@@ -1,18 +1,34 @@
 # 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/.
 
-allow = Allow
-allowForSession = Allow for Session
-allowTemporarily = Allow Temporarily
-block = Block
-blockTemporarily = Block Temporarily
-alwaysAsk = Always Ask
+# LOCALIZATION NOTE (state.current.allowed,
+#                    state.current.allowedForSession,
+#                    state.current.allowedTemporarily,
+#                    state.current.blockedTemporarily,
+#                    state.current.blocked):
+# This label is used to display active permission states in the site
+# identity popup (which does not have a lot of screen space).
+state.current.allowed = Allowed
+state.current.allowedForSession = Allowed for Session
+state.current.allowedTemporarily = Allowed Temporarily
+state.current.blockedTemporarily = Blocked Temporarily
+state.current.blocked = Blocked
+
+# LOCALIZATION NOTE (state.multichoice.alwaysAsk,
+#                    state.multichoice.allow,
+#                    state.multichoice.allowForSession,
+#                    state.multichoice.block):
+# Used to label permission state checkboxes in the page info dialog.
+state.multichoice.alwaysAsk = Always Ask
+state.multichoice.allow = Allow
+state.multichoice.allowForSession = Allow for Session
+state.multichoice.block = Block
 
 permission.cookie.label = Set Cookies
 permission.desktop-notification2.label = Receive Notifications
 permission.image.label = Load Images
 permission.camera.label = Use the Camera
 permission.microphone.label = Use the Microphone
 permission.screen.label = Share the Screen
 permission.install.label = Install Add-ons
--- a/browser/modules/SitePermissions.jsm
+++ b/browser/modules/SitePermissions.jsm
@@ -179,44 +179,16 @@ this.SitePermissions = {
         });
       }
     }
 
     return result;
   },
 
   /**
-   * Returns detailed information on the specified permission.
-   *
-   * @param {String} id
-   *        The permissionID of the permission.
-   * @param {SitePermissions scope} scope
-   *        The current scope of the permission.
-   * @param {SitePermissions state} state (optional)
-   *        The current state of the permission.
-   *        Will default to the default state if omitted.
-   *
-   * @return {Object} an object with the keys:
-   *           - id: the permissionID of the permission
-   *           - label: the localized label
-   *           - state: the passed in state argument
-   *           - scope: the passed in scope argument
-   *           - availableStates: an array of all available states for that permission,
-   *             represented as objects with the keys:
-   *             - id: the state constant
-   *             - label: the translated label of that state
-   */
-  getPermissionDetails(id, scope, state = this.getDefault(id)) {
-    let availableStates = this.getAvailableStates(id).map(val => {
-      return { id: val, label: this.getStateLabel(val) };
-    });
-    return {id, label: this.getPermissionLabel(id), state, scope, availableStates};
-  },
-
-  /**
    * Returns all custom permissions for a given browser.
    *
    * To receive a more detailed, albeit less performant listing see
    * SitePermissions.getAllPermissionDetailsForBrowser().
    *
    * @param {Browser} browser
    *        The browser to fetch permission for.
    *
@@ -244,21 +216,27 @@ this.SitePermissions = {
 
   /**
    * Returns a list of objects with detailed information on all permissions
    * that are currently set for the given browser.
    *
    * @param {Browser} browser
    *        The browser to fetch permission for.
    *
-   * @return {Array} a list of objects. See getPermissionDetails for the content of each object.
+   * @return {Array<Object>} a list of objects with the keys:
+   *           - id: the permissionID of the permission
+   *           - state: a constant representing the current permission state
+   *             (e.g. SitePermissions.ALLOW)
+   *           - scope: a constant representing how long the permission will
+   *             be kept.
+   *           - label: the localized label
    */
   getAllPermissionDetailsForBrowser(browser) {
     return this.getAllForBrowser(browser).map(({id, scope, state}) =>
-      this.getPermissionDetails(id, scope, state));
+      ({id, scope, state, label: this.getPermissionLabel(id)}));
   },
 
   /**
    * Checks whether a UI for managing permissions should be exposed for a given
    * URI. This excludes file URIs, for instance, as they don't have a host,
    * even though nsIPermissionManager can still handle them.
    *
    * @param {nsIURI} uri
@@ -484,39 +462,62 @@ this.SitePermissions = {
   },
 
   /**
    * Returns the localized label for the given permission state, to be used in
    * a UI for managing permissions.
    *
    * @param {SitePermissions state} state
    *        The state to get the label for.
+   *
+   * @return {String|null} the localized label or null if an
+   *         unknown state was passed.
+   */
+  getMultichoiceStateLabel(state) {
+    switch (state) {
+      case this.UNKNOWN:
+        return gStringBundle.GetStringFromName("state.multichoice.alwaysAsk");
+      case this.ALLOW:
+        return gStringBundle.GetStringFromName("state.multichoice.allow");
+      case this.ALLOW_COOKIES_FOR_SESSION:
+        return gStringBundle.GetStringFromName("state.multichoice.allowForSession");
+      case this.BLOCK:
+        return gStringBundle.GetStringFromName("state.multichoice.block");
+      default:
+        return null;
+    }
+  },
+
+  /**
+   * Returns the localized label for a permission's current state.
+   *
+   * @param {SitePermissions state} state
+   *        The state to get the label for.
    * @param {SitePermissions scope} scope (optional)
    *        The scope to get the label for.
    *
-   * @return {String} the localized label.
+   * @return {String|null} the localized label or null if an
+   *         unknown state was passed.
    */
-  getStateLabel(state, scope = null) {
+  getCurrentStateLabel(state, scope = null) {
     switch (state) {
-      case this.UNKNOWN:
-        return gStringBundle.GetStringFromName("alwaysAsk");
       case this.ALLOW:
         if (scope && scope != this.SCOPE_PERSISTENT)
-          return gStringBundle.GetStringFromName("allowTemporarily");
-        return gStringBundle.GetStringFromName("allow");
+          return gStringBundle.GetStringFromName("state.current.allowedTemporarily");
+        return gStringBundle.GetStringFromName("state.current.allowed");
       case this.ALLOW_COOKIES_FOR_SESSION:
-        return gStringBundle.GetStringFromName("allowForSession");
+        return gStringBundle.GetStringFromName("state.current.allowedForSession");
       case this.BLOCK:
         if (scope && scope != this.SCOPE_PERSISTENT)
-          return gStringBundle.GetStringFromName("blockTemporarily");
-        return gStringBundle.GetStringFromName("block");
+          return gStringBundle.GetStringFromName("state.current.blockedTemporarily");
+        return gStringBundle.GetStringFromName("state.current.blocked");
       default:
         return null;
     }
-  }
+  },
 };
 
 var gPermissionObject = {
   /* Holds permission ID => options pairs.
    *
    * Supported options:
    *
    *  - exactHostMatch
--- a/browser/modules/test/browser_SitePermissions.js
+++ b/browser/modules/test/browser_SitePermissions.js
@@ -33,68 +33,47 @@ add_task(function* testGetAllPermissionD
   let permissions = SitePermissions.getAllPermissionDetailsForBrowser(tab.linkedBrowser);
 
   let camera = permissions.find(({id}) => id === "camera");
   Assert.deepEqual(camera, {
     id: "camera",
     label: "Use the Camera",
     state: SitePermissions.ALLOW,
     scope: SitePermissions.SCOPE_PERSISTENT,
-    availableStates: [
-      { id: SitePermissions.UNKNOWN, label: "Always Ask" },
-      { id: SitePermissions.ALLOW, label: "Allow" },
-      { id: SitePermissions.BLOCK, label: "Block" },
-    ]
   });
 
-  // check that removed permissions (State.UNKNOWN) are skipped
+  // Check that removed permissions (State.UNKNOWN) are skipped.
   SitePermissions.remove(uri, "camera");
   permissions = SitePermissions.getAllPermissionDetailsForBrowser(tab.linkedBrowser);
 
   camera = permissions.find(({id}) => id === "camera");
   Assert.equal(camera, undefined);
 
-  // check that different available state values are represented
-
   let cookie = permissions.find(({id}) => id === "cookie");
   Assert.deepEqual(cookie, {
     id: "cookie",
     label: "Set Cookies",
     state: SitePermissions.ALLOW_COOKIES_FOR_SESSION,
     scope: SitePermissions.SCOPE_PERSISTENT,
-    availableStates: [
-      { id: SitePermissions.ALLOW, label: "Allow" },
-      { id: SitePermissions.ALLOW_COOKIES_FOR_SESSION, label: "Allow for Session" },
-      { id: SitePermissions.BLOCK, label: "Block" },
-    ]
   });
 
   let popup = permissions.find(({id}) => id === "popup");
   Assert.deepEqual(popup, {
     id: "popup",
     label: "Open Pop-up Windows",
     state: SitePermissions.BLOCK,
     scope: SitePermissions.SCOPE_PERSISTENT,
-    availableStates: [
-      { id: SitePermissions.ALLOW, label: "Allow" },
-      { id: SitePermissions.BLOCK, label: "Block" },
-    ]
   });
 
   let geo = permissions.find(({id}) => id === "geo");
   Assert.deepEqual(geo, {
     id: "geo",
     label: "Access Your Location",
     state: SitePermissions.ALLOW,
     scope: SitePermissions.SCOPE_SESSION,
-    availableStates: [
-      { id: SitePermissions.UNKNOWN, label: "Always Ask" },
-      { id: SitePermissions.ALLOW, label: "Allow" },
-      { id: SitePermissions.BLOCK, label: "Block" },
-    ]
   });
 
   SitePermissions.remove(uri, "cookie");
   SitePermissions.remove(uri, "popup");
   SitePermissions.remove(uri, "geo");
 
   yield BrowserTestUtils.removeTab(gBrowser.selectedTab);
 });
--- a/browser/modules/test/xpcshell/test_SitePermissions.js
+++ b/browser/modules/test/xpcshell/test_SitePermissions.js
@@ -46,8 +46,24 @@ add_task(function* testGetAllByURI() {
   SitePermissions.remove(uri, "desktop-notification");
   Assert.deepEqual(SitePermissions.getAllByURI(uri), []);
 
   // XXX Bug 1303108 - Control Center should only show non-default permissions
   SitePermissions.set(uri, "addon", SitePermissions.BLOCK);
   Assert.deepEqual(SitePermissions.getAllByURI(uri), []);
   SitePermissions.remove(uri, "addon");
 });
+
+add_task(function* testGetAvailableStates() {
+  Assert.deepEqual(SitePermissions.getAvailableStates("camera"),
+                   [ SitePermissions.UNKNOWN,
+                     SitePermissions.ALLOW,
+                     SitePermissions.BLOCK ]);
+
+  Assert.deepEqual(SitePermissions.getAvailableStates("cookie"),
+                   [ SitePermissions.ALLOW,
+                     SitePermissions.ALLOW_COOKIES_FOR_SESSION,
+                     SitePermissions.BLOCK ]);
+
+  Assert.deepEqual(SitePermissions.getAvailableStates("popup"),
+                   [ SitePermissions.ALLOW,
+                     SitePermissions.BLOCK ]);
+});