Last Comment Bug 823385 - When requesting permissions to a WebAPI that has PROMPT_ACTION, the prompt should show data usage intention in the permission description
: When requesting permissions to a WebAPI that has PROMPT_ACTION, the prompt sh...
Status: NEW
:
Product: Firefox OS
Classification: Client Software
Component: Gaia::System (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: -- normal (vote)
: ---
Assigned To: marta
:
:
Mentors:
: 865687 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-12-19 21:05 PST by Jason Smith [:jsmith]
Modified: 2014-02-09 22:14 PST (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
-


Attachments
Proposed solution to the bug (43 bytes, patch)
2013-07-25 06:41 PDT, marta
no flags Details | Diff | Splinter Review
Gaia pull request (46 bytes, text/x-github-pull-request)
2013-07-26 00:56 PDT, marta
timdream: feedback+
Details | Review | Splinter Review
823385.patch (8.57 KB, patch)
2013-07-26 00:57 PDT, marta
no flags Details | Diff | Splinter Review
823385.patch (1.63 KB, patch)
2013-08-02 07:14 PDT, marta
no flags Details | Diff | Splinter Review
Gaia pull request (46 bytes, text/x-github-pull-request)
2013-08-02 07:15 PDT, marta
etienne: review-
Details | Review | Splinter Review
Exemplary manifest file with the rationals given in several languages. (1.59 KB, text/plain)
2013-08-03 01:38 PDT, marta
no flags Details
823385.patch (11.09 KB, patch)
2013-09-11 14:28 PDT, marta
fabrice: feedback+
Details | Diff | Splinter Review
823385gecko.patch (3.96 KB, patch)
2013-09-20 10:06 PDT, marta
fabrice: review-
Details | Diff | Splinter Review
823385gaia.patch (4.23 KB, patch)
2013-09-20 10:07 PDT, marta
etienne: feedback+
Details | Diff | Splinter Review
Screenshot (642.52 KB, image/jpeg)
2013-10-07 04:21 PDT, Agus
no flags Details

Description Jason Smith [:jsmith] 2012-12-19 21:05:23 PST
Build: B2G 18 12/19/2012
Device: Unagi

Steps:

1. Request access to a WebAPI that requires PROMPT_ACTION with a description provided - example below:

  "permissions": {
    "device-storage:pictures": {
      "access": "readonly",
      "description": "Can read what your pictures are from your device"
    },
    "contacts": {
      "access": "readwrite",
      "description": "Come see my contacts"
    }
  },

Expected:

The permission prompt should show data usage intention for the WebAPI. For example, if I requested access to my pictures storage, I should see in the prompt the "description" provided in the prompt to give clear indication to the user what this app intends to do with my data.

Actual:

No data usage intention is seen in the permission prompt. I originally thought this was a security requirement, but apparently it's not working right now.
Comment 1 Jason Smith [:jsmith] 2012-12-19 21:07:05 PST
Into triage since this was originally a security requirement. Someone from security should comment here to determine if they are or are not comfortable with shipping without this.
Comment 2 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-12-19 22:21:20 PST
We discussed this back and forth a whole lot on the various mailing lists, but I can't remember what the ultimate decision was.

Argument against: It's probably a bad idea to show the description for non-privileged (or at least non-signed) apps since the description is totally unverified. So this would be prime relestate to try to social engineer the user.

Argument for: If someone has reviewed the description, it's silly not to display it since it can provide valuable information to the user. This is one of the prime complaints about the android security model, that authors have no way to motivate why they need a particular privilege.

Argument against: It's inconsistent to only show it for privileged apps, but not for "normal" apps.

Argument for: If we can make it clear in the UI that it's a developer-provided description, then we might be able to always display it.
Comment 3 Joe Cheng [:jcheng] 2012-12-20 03:18:35 PST
Triage: BB- , tracking-b2g+. UI Prompt
Comment 4 Jason Smith [:jsmith] 2012-12-20 06:27:54 PST
(In reply to Joe Cheng from comment #3)
> Triage: BB- , tracking-b2g+. UI Prompt

Irrelevant triage argument. This talking about data usage intention.

This requires a product decision to cut this feature. Back into triage it goes.
Comment 5 Lucas Adamski [:ladamski] 2012-12-20 14:32:49 PST
I'll be sad if we don't have this as its a nice differentiating improvement for our model, but it doesn't actually expose the user to additional risk if we ship without this.  It will likely just makes it harder for the user to be comfortable granting permissions.

In terms of behavior, yes we should show it for privileged apps but not unprivileged (i.e. unreviewed/unsigned) apps.
Comment 6 Jason Smith [:jsmith] 2012-12-20 15:16:30 PST
Sounds like we shouldn't block then. But we should probably track it.
Comment 7 Alex Keybl [:akeybl] 2013-02-01 13:34:09 PST
More appropriate for a v2 given scope, so no need to track.
Comment 8 Jason Smith [:jsmith] 2013-04-25 08:03:19 PDT
*** Bug 865687 has been marked as a duplicate of this bug. ***
Comment 9 Agus 2013-07-25 06:30:17 PDT
Posible solution to this bug:
https://github.com/mozilla-b2g/B2G/pull/261
Comment 10 marta 2013-07-25 06:41:43 PDT
Created attachment 780960 [details] [diff] [review]
Proposed solution to the bug

Pull request posted below by one of my students, but now moving it for review request.
Comment 11 Jason Smith [:jsmith] 2013-07-25 07:19:54 PDT
(In reply to marta from comment #10)
> Created attachment 780960 [details] [diff] [review]
> Proposed solution to the bug
> 
> Pull request posted below by one of my students, but now moving it for
> review request.

Note - this pull request needs to be opened up against Gaia, not B2G.
Comment 12 Jason Smith [:jsmith] 2013-07-25 07:20:59 PDT
(In reply to Jason Smith [:jsmith] from comment #11)
> (In reply to marta from comment #10)
> > Created attachment 780960 [details] [diff] [review]
> > Proposed solution to the bug
> > 
> > Pull request posted below by one of my students, but now moving it for
> > review request.
> 
> Note - this pull request needs to be opened up against Gaia, not B2G.

Or actually - the gaia pieces should have a gaia pull request, where as the gecko pieces should have a diff file included here.
Comment 13 Tim Guan-tin Chien [:timdream] (please needinfo; OOO until Oct 17) 2013-07-25 08:43:41 PDT
Comment on attachment 780960 [details] [diff] [review]
Proposed solution to the bug

Hi, Thank you for the contribution. Unfortunately it's not possible to review the patch as-is; you would have to provide patches separately for Gaia and Gecko.

Please find the Gaia here: https://github.com/mozilla-b2g/gaia and basic documentation on how to submit patch here:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Platform/Gaia/Hacking

For Gecko, here is a link as the starting point: https://developer.mozilla.org/en-US/docs/Developer_Guide
Comment 14 marta 2013-07-26 00:56:07 PDT
Created attachment 781558 [details] [review]
Gaia pull request
Comment 15 marta 2013-07-26 00:57:46 PDT
Created attachment 781560 [details] [diff] [review]
823385.patch
Comment 16 Tim Guan-tin Chien [:timdream] (please needinfo; OOO until Oct 17) 2013-07-26 02:46:34 PDT
Comment on attachment 781560 [details] [diff] [review]
823385.patch

This patch is not prepared correctly.

Also I am not be able to review this part of code -- please find :vingtetun.
Comment 17 Tim Guan-tin Chien [:timdream] (please needinfo; OOO until Oct 17) 2013-07-26 02:48:57 PDT
Comment on attachment 781558 [details] [review]
Gaia pull request

Generally looks good but you need to find another reviewer.

Also, please squash your commits into one with an acceptable commit message.
Comment 18 (inactive after 6/18) Alive Kuo [:alive] 2013-07-26 03:01:19 PDT
The string is not localizable. Is it acceptable to show always english permission description to the user? Does this help non-English user to understand the permission request?
Comment 19 Tim Guan-tin Chien [:timdream] (please needinfo; OOO until Oct 17) 2013-07-26 07:19:18 PDT
BTW, I usually navigates to log page to find who are the proper reviewers of a patch:

https://hg.mozilla.org/mozilla-central/filelog/46d73e889cb4/b2g/components/ContentPermissionPrompt.js
Comment 20 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2013-07-29 07:14:43 PDT
What Alive says in #c18 needs to be answered before going further here. 
What is the way to translate the permission description here?

The usual way to localize a string from the manifest is to use the |locales| field of the manifest. See https://developer.mozilla.org/en-US/docs/Web/Apps/Manifest#locales

Based on http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/AppsUtils.jsm#510 I think that it is supposed to work. Can you verify this?
Comment 21 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2013-07-29 07:26:49 PDT
Comment on attachment 781558 [details] [review]
Gaia pull request

I made some comments on github.
Comment 22 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2013-07-29 07:31:39 PDT
Comment on attachment 781560 [details] [diff] [review]
823385.patch

Can you provide a diff of the change? The current patch is displayed like if you have just created the file - it makes it hard for me to understand what has been change.

I assume that you the biggest part of this patch should be to expose permDescription. First let's rename it descrition since the event is already about a permission. Second look at my comment in github about |isApp| versus |appType|. Third you will have to check about localization and ensure it works.

Thanks for trying to solve this issue though :)

I remove the r? flag. Feel free to ask again once you think the patch is ready for a new review.
Comment 23 marta 2013-08-02 07:14:19 PDT
Created attachment 784988 [details] [diff] [review]
823385.patch
Comment 24 marta 2013-08-02 07:15:38 PDT
Created attachment 784989 [details] [review]
Gaia pull request
Comment 25 marta 2013-08-03 01:37:22 PDT
One question about the translation. How do you expect it to be done? Should the developer supply strings in different languages and than the notification should search for the one that fits to the user settings? (exemplary file attached)
Or maybe it would be better to say that the developer introduces the description only in English and we should check the local settings of the phone and translate it on the run (is it even possible on FxOS)?
Comment 26 marta 2013-08-03 01:38:36 PDT
Created attachment 785358 [details]
Exemplary manifest file with the rationals given in several languages.
Comment 27 (inactive after 6/18) Alive Kuo [:alive] 2013-08-03 01:41:46 PDT
Hi contributor.
About manifest property l10n, see this manifest for reference
https://github.com/mozilla-b2g/gaia/blob/master/apps/music/manifest.webapp#L35

The object of locales[current_language] would replace the original property in whole manifest.
Comment 28 marta 2013-08-03 01:49:41 PDT
Yes, but this does not explain where and how will the rational be given - will it be an additional field to the "locales"? eg.
"en-US": {
      "name": "Music",
      "description": "Gaia Music"
      "rational": "Need access to the music database"
    },
or will it be in the part "permissions" eg.
"permissions": {
    "storage":{ "description": {en: "need to play the music"}}},
Comment 29 Jonas Sicking (:sicking) No longer reading bugmail consistently 2013-08-04 00:17:02 PDT
Manifests have a localization method which is used for all properties. The way you use it is something like:

"permissions": {
  "contacts": {
    "description": "To share highscore with your friends.",
    "access": "readonly"
  }
},
"locales": {
  "se-SV": {
    "permissions": {
      "contacts": {
        "description": "För att dela dina högsta poäng med dina vänner."
      }
    }
  }
}


If we don't already have a utility functions for reading localized data from the manifest, we really should add them.
Comment 30 (inactive after 6/18) Alive Kuo [:alive] 2013-08-09 23:25:08 PDT
What Jonas says :)
Sorry I forgot to explain it in my last comment.

(In reply to marta from comment #28)
> Yes, but this does not explain where and how will the rational be given -
> will it be an additional field to the "locales"? eg.
> "en-US": {
>       "name": "Music",
>       "description": "Gaia Music"
>       "rational": "Need access to the music database"
>     },
> or will it be in the part "permissions" eg.
> "permissions": {
>     "storage":{ "description": {en: "need to play the music"}}},
Comment 31 Etienne Segonzac (:etienne) 2013-08-12 06:04:44 PDT
Comment on attachment 784989 [details] [review]
Gaia pull request

Looks like there is a bit of misunderstanding here.

The gaia system app isn't chrome. So you can't access things like |Ci.nsIPrincipal.APP_STATUS_PRIVILEGED|.
You probably need to bubble the information you want from chrome.

Also, the |Provided by developer: | string needs to be internationalized (a good example of how this work is the permissions names).

Finally we should maybe reuse the "more info box" from bug 901383 here.
Comment 32 Etienne Segonzac (:etienne) 2013-08-12 06:06:03 PDT
Comment on attachment 784988 [details] [diff] [review]
823385.patch

Review of attachment 784988 [details] [diff] [review]:
-----------------------------------------------------------------

::: gecko/b2g/components/ContentPermissionPrompt.js
@@ +193,5 @@
>      let details = {
>        type: "permission-prompt",
>        permission: request.type,
> +      //text to show when permission does not contain any description in the manifest
> +      description: "Description not found",

we probably want this to be undefined when the developer didn't provide a description. Otherwise we would display an english string for everybody.
Comment 33 Etienne Segonzac (:etienne) 2013-08-12 06:11:34 PDT
(In reply to Jonas Sicking (:sicking) from comment #29)
> If we don't already have a utility functions for reading localized data from
> the manifest, we really should add them.

We have [1], but we probably to add some support for the permission description.

[1] https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/AppsUtils.jsm#535
Comment 34 Etienne Segonzac (:etienne) 2013-08-12 06:23:50 PDT
Hey :aalvarezmartinez, lots of new stuff here :)

To sum up:

* We need to follow the manifest localization method mentioned in Comment 29.
* We need to add support for getting a localized permission description in AppUtils.jsm
* ContentPermissionPrompt.js should send a mozChromeEvent with details.description undefined when the developer did not provide a description
* ContentPermissionPrompt.js should send a mozChromeEvent with a flag indicating that the "Provided by the developer" string should be displayed (since the gaia system app wont be able to perform this check)
* The "Provided by the developer" key should be localized on the gaia side

Hope this is clear enough to move forward with this bug, cheers!
Comment 35 marta 2013-09-11 14:28:41 PDT
Created attachment 803291 [details] [diff] [review]
823385.patch
Comment 36 [:fabrice] Fabrice Desré 2013-09-13 01:54:10 PDT
Comment on attachment 803291 [details] [diff] [review]
823385.patch

Review of attachment 803291 [details] [diff] [review]:
-----------------------------------------------------------------

Note: you need to split up the gecko and gaia parts. I only reviewed the gecko parts.

::: gecko/b2g/components/ContentPermissionPrompt.js
@@ +185,4 @@
>  
>      let principal = request.principal;
>      let isApp = principal.appStatus != Ci.nsIPrincipal.APP_STATUS_NOT_INSTALLED;
> +    let isWebApp = principal.appStatus == Ci.nsIPrincipal.APP_STATUS_INSTALLED;

I don't understand why you need this flag. APP_STATUS_INSTALLED means that this is a non privileged app, but it's still different from a web page.

::: gecko/dom/apps/src/AppsUtils.jsm
@@ +511,4 @@
>    let locale = chrome.getSelectedLocale("global").toLowerCase();
>    this._localeRoot = this._manifest;
>  
> +  if (this._manifest.locales) {

Can you add a comment block describing the algorithm you implemented here?

@@ +515,2 @@
>      let lang = locale.split('-')[0];
> +    for (let manLoc in this._manifest.locales){

nit: formatting should be: (...locales) {

@@ +515,3 @@
>      let lang = locale.split('-')[0];
> +    for (let manLoc in this._manifest.locales){
> +      if ( manLoc.toLowerCase() == locale ){

nit: if (manLoc.... == locale) {

@@ +520,5 @@
> +      } else if ( manLoc.split('-')[0] == lang ){
> +        // try with the language part of the locale ("en" for "en-GB") only
> +        if (lang != locale && this._manifest.locales[lang]){
> +          this._localeRoot = this._manifest.locales[lang];
> +        // try with the language part of the locales ("en-US" for "en-GB")

Move this comment in the branch of code it's describing. Also, Start comments with a capital letter and end with a full stop.

@@ +521,5 @@
> +        // try with the language part of the locale ("en" for "en-GB") only
> +        if (lang != locale && this._manifest.locales[lang]){
> +          this._localeRoot = this._manifest.locales[lang];
> +        // try with the language part of the locales ("en-US" for "en-GB")
> +        } else{

nit: } else {

@@ +586,4 @@
>      return {};
>    },
>  
> +  permissionDescription: function(permission){

nit: function(aPermission) {

@@ +589,5 @@
> +  permissionDescription: function(permission){
> +    let permissions = this._localeProp("permissions");
> +    if (!permissions){
> +      permissions = this._manifest.permissions;
> +    }

_localeProp already takes care of the _manifest fallback. So if permissions is null, you must return null.

@@ +590,5 @@
> +    let permissions = this._localeProp("permissions");
> +    if (!permissions){
> +      permissions = this._manifest.permissions;
> +    }
> +    for (let perm in permissions){

nit: ) {

@@ +592,5 @@
> +      permissions = this._manifest.permissions;
> +    }
> +    for (let perm in permissions){
> +      if (perm == permission)
> +        return permissions[perm].description;

Even for single line ifs, use:
if () {
  doThat();
}

@@ +594,5 @@
> +    for (let perm in permissions){
> +      if (perm == permission)
> +        return permissions[perm].description;
> +    }
> +    return {};

Doesn't that return a string? I think you want |return null|
Comment 37 Etienne Segonzac (:etienne) 2013-09-19 07:26:00 PDT
Comment on attachment 803291 [details] [diff] [review]
823385.patch

Review of attachment 803291 [details] [diff] [review]:
-----------------------------------------------------------------

(looked only at the gaia part)

We should probably have a dedicated l10n key for "Description provided by developer: ", this way we could compose the string more easily, keep the l10n diff shorter and we'll be able to add this string _only_ if the developer gave a description.

::: gaia/apps/system/js/permission_manager.js
@@ +58,5 @@
> +      // The app hasn't permission description. This shouldn't happen
> +      // because permission's description is mandatory in manifests
> +      } else { //Avoid of show "undefined" when description is not present
> +        str = _(permissionID + '-appRequest', { 'app': detail.appName });
> +      }

Looks like we can simplify this and make it more readable with something like:

```
var requestType = detail.isWebApp ? 'webAppRequest' : 'appRequest';
str = _(permissionID + '-' + requestType, {app: detail.appName});
if (detail.description) {
  str += detail.description;
}
```
Comment 38 marta 2013-09-20 10:06:33 PDT
Created attachment 807825 [details] [diff] [review]
823385gecko.patch

isWebApp is needed to show a "Description provided by developer:" before the description (localizable in gaia side), for those apps which aren't certified or priviledged.
Comment 39 marta 2013-09-20 10:07:21 PDT
Created attachment 807827 [details] [diff] [review]
823385gaia.patch
Comment 40 [:fabrice] Fabrice Desré 2013-09-20 10:24:00 PDT
Comment on attachment 807825 [details] [diff] [review]
823385gecko.patch

Review of attachment 807825 [details] [diff] [review]:
-----------------------------------------------------------------

::: gecko/b2g/components/ContentPermissionPrompt.js
@@ +185,4 @@
>  
>      let principal = request.principal;
>      let isApp = principal.appStatus != Ci.nsIPrincipal.APP_STATUS_NOT_INSTALLED;
> +    let isWebApp = principal.appStatus == Ci.nsIPrincipal.APP_STATUS_INSTALLED;

isUnprivilegedApp would be clearer, even if it's way longer.

::: gecko/dom/apps/src/AppsUtils.jsm
@@ +596,5 @@
> +      }
> +    }
> +    let permissions = this._manifest.permissions;
> +    // If permission not contains description in manifest locale
> +    // try to get the default permission description.

_localeProp() will do the fallback to the default manifest permissions if there are no locale ones. So you don't need that, but should return null if |permissions| is falsy after this._localeProp("permissions");
Comment 41 Agus 2013-09-20 11:08:07 PDT
(In reply to Fabrice Desré [:fabrice] from comment #40)

> ::: gecko/dom/apps/src/AppsUtils.jsm
> @@ +596,5 @@
> > +      }
> > +    }
> > +    let permissions = this._manifest.permissions;
> > +    // If permission not contains description in manifest locale
> > +    // try to get the default permission description.
> 
> _localeProp() will do the fallback to the default manifest permissions if
> there are no locale ones. So you don't need that, but should return null if
> |permissions| is falsy after this._localeProp("permissions");

That piece of code tries to cover the event in which permission is present in locale, but the description is not contained within it. In this case it tries to get the field by default instead of returning null.

Maybe it would be clearer coded as follows?

  permissionDescription: function(aPermission) {
    let permissions = this._localeProp("permissions");
    for (let perm in permissions) {
      if (perm == aPermission) {
        return permissions[perm].description;
      }
    }
    if (permissions != this._manifest.permissions) {
    // If permission is present in manifest locale
    // but it doesn't contain permission description
    // try to get the default permission description.
      let permissions = this._manifest.permissions;
      for (let perm in permissions) {
        if (perm == aPermission) {
          return permissions[perm].description;
        }
      }
    }
    // If the manifest not contains permission description
    // returns null. This shouldn't happen because permission's
    // description is mandatory in manifests.
    return null;
  },
Comment 42 [:fabrice] Fabrice Desré 2013-09-20 11:44:00 PDT
(In reply to Agus from comment #41)
> (In reply to Fabrice Desré [:fabrice] from comment #40)
> 
> > ::: gecko/dom/apps/src/AppsUtils.jsm
> > @@ +596,5 @@
> > > +      }
> > > +    }
> > > +    let permissions = this._manifest.permissions;
> > > +    // If permission not contains description in manifest locale
> > > +    // try to get the default permission description.
> > 
> > _localeProp() will do the fallback to the default manifest permissions if
> > there are no locale ones. So you don't need that, but should return null if
> > |permissions| is falsy after this._localeProp("permissions");
> 
> That piece of code tries to cover the event in which permission is present
> in locale, but the description is not contained within it. In this case it
> tries to get the field by default instead of returning null.
> 
> Maybe it would be clearer coded as follows?
> 
>   permissionDescription: function(aPermission) {
>     let permissions = this._localeProp("permissions");
>     for (let perm in permissions) {
>       if (perm == aPermission) {
>         return permissions[perm].description;
>       }
>     }
>     if (permissions != this._manifest.permissions) {
>     // If permission is present in manifest locale
>     // but it doesn't contain permission description
>     // try to get the default permission description.
>       let permissions = this._manifest.permissions;
>       for (let perm in permissions) {
>         if (perm == aPermission) {
>           return permissions[perm].description;
>         }
>       }
>     }
>     // If the manifest not contains permission description
>     // returns null. This shouldn't happen because permission's
>     // description is mandatory in manifests.
>     return null;
>   },

Ha, that's not your fault but I really think that duplicating all the permissions objects in locales is terribly error prone.

Anyway, here's what I would do (untested, use at your own risk!) :

permissionDescription:function(aPermission) {
  let permissions = this._localeProp("permissions");
  // Check if we have the description in the localized permission.
  if (permissions[aPermission] && permissions[aPermission].description) {
    return permissions[aPermission].description;
  }

  // Fallback to the non-localized one.
  if (permissions !== this._manifest.permissions &&
      this._manifest.permissions &&
      this._manifest.permissions[aPermission] &&
      this._manifest.permissions[aPermission].description) {
    return this._manifest.permissions[aPermission].description;
  }

  // Nothing found.
  return null;
}

We also need tests for these new features.
Comment 43 Etienne Segonzac (:etienne) 2013-09-23 07:08:20 PDT
Comment on attachment 807827 [details] [diff] [review]
823385gaia.patch

Review of attachment 807827 [details] [diff] [review]:
-----------------------------------------------------------------

The code change looks good.
We just need to add the corresponding test.

Some pointers:
- the tests for this part are in gaia/apps/system/test/unit/permission_manager_test.js
- you will probably want to complete the handlePermissionPrompt suite
- you should be able to check the |str| passed by making |stubReq| a spy [1]

[1] http://sinonjs.org/docs/#spies

::: gaia/shared/locales/permissions/permissions.en-US.properties
@@ +42,5 @@
>  perm-geolocation-appRequest={{app}} would like to know your location.
>  perm-geolocation-webRequest={{site}} would like to know your location.
>  perm-geolocation-more-info = {{brandShortName}} uses data such as GPS, Wi-Fi, and mobile networks near you to help determine your approximate location. Data is sent to Mozilla and other service providers and will be used in the aggregate to improve those location databases.  Data may be stored on your device and data collection may occur even when no apps are running.
> +
> +perm-dev-description=Description provided by developer:

nit: "Description provided by the developer:"

::: gaia/test_apps/geoloc/manifest.webapp
@@ +33,5 @@
>        "name": "Geoloc",
> +      "description": "Enregistrement de géolocalisation",
> +      "permissions": {
> +        "geolocation": {
> +          "description": "Nécessaires pour vous guider."

nit: "Nécessaire pour vous guider" without the S.

@@ +36,5 @@
> +        "geolocation": {
> +          "description": "Nécessaires pour vous guider."
> +        },
> +        "device-storage:sdcard": {
> +          "description": "Nécessaires pour la lecture des cartes."

nit: idem
Comment 44 Paul Theriault [:pauljt] 2013-09-26 00:53:11 PDT
Comment on attachment 807827 [details] [diff] [review]
823385gaia.patch

Review of attachment 807827 [details] [diff] [review]:
-----------------------------------------------------------------

::: gaia/apps/system/js/permission_manager.js
@@ +131,5 @@
>        str = _(permissionID + '-appRequest', { 'app': detail.appName });
> +      if (detail.description) {
> +        if (detail.isWebApp){
> +          str += ' ' + _('perm-dev-description');
> +        }

Why would we only show this for web apps? The description is always provided by the developer, so shouldn't we always show "Description is provider by the developer"? (as per comment 2).

@@ +132,5 @@
> +      if (detail.description) {
> +        if (detail.isWebApp){
> +          str += ' ' + _('perm-dev-description');
> +        }
> +        str += ' ' + detail.description;

We need to be a bit more careful with the description I think. If nothing else, we need to check length, and handle the situation when the description is bigger than the space that is shown. Maybe show it in a scrollable input box or something too. (question for UI people maybe?)
Comment 45 Paul Theriault [:pauljt] 2013-09-26 00:55:34 PDT
Sorry for the drive by comment, but can you explain how we deal with the situation when the permissions specified in the locales section differs from the the permissions attribute in the root of the manifest? I assume that locales permissions is ONLY used for text and differences are just ignored.
Comment 46 Paul Theriault [:pauljt] 2013-09-26 00:57:08 PDT
Not sure who the best person to provide the answer to my above question is, so I will needinfo both of you ;)

PS apart from these two questions, this approach looks good to me. Any chance you could provide a screenshot just to indicate what this looks like?
Comment 47 Agus 2013-10-07 03:32:02 PDT
Sorry for the delay in response.
Text is always obtained from locales if the user language is present in them and contains description for the permission, if permission is not present on the locale part, or it doesn't contains description, the description is extracted from the root of the manifest, just like in the situation in which the user's language is not present.
Comment 48 Agus 2013-10-07 04:21:12 PDT
Created attachment 814062 [details]
Screenshot
Comment 49 Agus 2013-10-07 05:18:41 PDT
(In reply to Paul Theriault [:pauljt] On PTO back 28th Oct from comment #44)
> Comment on attachment 807827 [details] [diff] [review]
> 823385gaia.patch
> 
> Review of attachment 807827 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gaia/apps/system/js/permission_manager.js
> @@ +131,5 @@
> >        str = _(permissionID + '-appRequest', { 'app': detail.appName });
> > +      if (detail.description) {
> > +        if (detail.isWebApp){
> > +          str += ' ' + _('perm-dev-description');
> > +        }
> 
> Why would we only show this for web apps? The description is always provided
> by the developer, so shouldn't we always show "Description is provider by
> the developer"? (as per comment 2).

It's shown only for webapps as a responsability disclaim to indicate that the content of the description is unverified and is provided by the developer.

Anyway, if it's preferred that don't display description por webapps, and/or shows "Description provided by the developer" for all kind of app, or any other change I can easily do it.

> @@ +132,5 @@
> > +      if (detail.description) {
> > +        if (detail.isWebApp){
> > +          str += ' ' + _('perm-dev-description');
> > +        }
> > +        str += ' ' + detail.description;
> 
> We need to be a bit more careful with the description I think. If nothing
> else, we need to check length, and handle the situation when the description
> is bigger than the space that is shown. Maybe show it in a scrollable input
> box or something too. (question for UI people maybe?)

It's already shown in a scrollable box. I have tested it even with a more than one million chars' description and it works properly.

Note You need to log in before you can comment on or make changes to this bug.