1240859 - Improve RuntimePermissions.jsm API

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

My initial implementation of RuntimePermissions.jsm (bug 1235352) had this API:

> RuntimePermissions.waitForPermissions(RuntimePermissions.CAMERA)
>                   .then(() => ..permission granted..,
>                         () => ..permission NOT granted..);

To not mix the "not granted" and error state I refactored this to:

> RuntimePermissions.waitForPermissions(RuntimePermissions.CAMERA)
>                    .then(permissionGranted => ..evaluate boolean..,
>                          e => ..error..);

From bug 1240700 (nalexander):
> I'm spit-balling here, but: there's something very unsatisfying about having
> to chain the result through.  I know I was saying not to make `false` a
> failure, but consider adding a `errorIfNotPermitted` or something so that
> you can do:
> ```
> RuntimePermissions.errorIfNotGranted(PERMISSION)
>   .then(() => ...)
>   .catch((e) => /* ignore */))
> ```
> In the code you have, people will forget the boolean check, possibly
> crashing the App on illegal access.
> 
> In the code I have, we'll see lots of "failures" in the logcat, since folks
> will forget to catch-and-ignore.  And worse, they'll catch-and-ignore real

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

Flagging some people who know JavaScript better than I do: What API would you expect? How do you handle such promises with ternary results? :)

Comment 2

[:Gijs (he/him)]

One crucial bit of information that's missing here: When would the promise be rejected, if not by the permission not being granted? That is, what is the "error state" that you describe? What are "runtime permissions" and their "ternary results" (?), anyway - it's not really clear from this bug or bug 1235352 or bug 1235347, all of which are really generic and don't seem to list the possible outcomes, which makes it hard to answer the question...

Is this web-exposed or just about gecko/android JS? Is it Android-specific? All the bugs seem to be filed in Android, but you're asking Desktop folks for advice so now I'm confused. :-)

In general, if there are more than 2 results as "callbacks", the traditional solution is to use an object that implements several methods, e.g. onPermissionGranted, onPermissionRejected, onPermissionError, etc. Of course, that's not very Promise-y.

An alternative would be doing what Nick suggested and make the promise resolve with an object or value you can interrogate, but then the API should probably not be called "waitForPermission", because it implies that the promise won't be resolved until permission is granted. Maybe something like "fetchPermissionState()" would make more sense in implying that the promise will be resolved even if permission is not actually granted... but it's hard to suggest good wording without understanding what the method is supposed to do and what the error conditions are.

Comment 3

[Matthew N. [:MattN]]

I personally like this version best but also agree with Gijs that "waitForPermission" could imply waiting for permission to be granted meaning that it will be granted or error. The fetchPermissionState suggestion seems better.

> > RuntimePermissions.waitForPermissions(RuntimePermissions.CAMERA)
> >                    .then(permissionGranted => ..evaluate boolean..,
> >                          e => ..error..);

I think it's nicer to look at when using a `yield`:

`
try {
  let permGranted = yield RuntimePermissions.fetchPermissionState(RuntimePermissions.CAMERA);
  if (permGranted) {
    …
  }
} catch (ex) {
  // something
}
`

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Thank your for the input! To answer some of the questions: This is an Android-only module (not web-exposed) to check/request system permissions (access location, camera, ..) at app runtime - a new concept in Android 6.0 [1].

waitForPermission() does in the background:
* Resolves to true if the system permission has already been granted in the past
* Resolves to false if the system permission has been permanently rejected by the user
* If needed prompts the user [2] (that's where the "wait" comes from) and then resolves to true or false depending on the user decision.

So a Fennec developer writing JavaScript code might need to access APIs that require a specific system permission. For example in this patch I used the current RuntimePermissions module handle camera/microphone access for WebRTC:
https://reviewboard.mozilla.org/r/29899/diff/2#index_header

So far my experience with our existing code is that as a developer I care about permission granted or rejected to run different callbacks. An error case is not really defined here and would mean that the app is somehow broken.

I've been asking you desktop folks just because you might have more experience with this kind of module design (and nalexander suggested you). :)

[1] http://developer.android.com/training/permissions/requesting.html
[2] http://developer.android.com/images/training/permissions/request_permission_dialog_2x.png

Comment 5

[:Margaret Leibovic]

(In reply to Sebastian Kaspari (:sebastian) from comment #4)

> So far my experience with our existing code is that as a developer I care
> about permission granted or rejected to run different callbacks. An error
> case is not really defined here and would mean that the app is somehow
> broken.

Given this, I think it would make more sense to resolve the promise when the permission is granted, and reject if it's not granted. 

Given that you're not even catching a rejection in the sample WebRTC check, I feel like it would be better for the code to fall back to the "permission not granted" state if some other error happens. That might make it hard to debug legitimate errors, but at least the app wouldn't get into a busted state.

Comment 6

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.