1235352 - Add JavaScript module to check and request runtime permissions

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

Based on the Java implementation of bug 1235347 we'll need a JS module to check and request permissions from JavaScript land.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1235352 - Add JavaScript module for checking and requesting runtime permissions. r=nalexander

Review commit: https://reviewboard.mozilla.org/r/29387/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29387/

Comment 2

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8703557 [details]
MozReview Request: Bug 1235352 - Add JavaScript module for checking and requesting runtime permissions. r=nalexander

https://reviewboard.mozilla.org/r/29387/#review26299

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java:694
(Diff revision 1)
> +                               callback.sendError(null);

Fail early if `callback == null`, please.  And if `permission == null`.

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java:700
(Diff revision 1)
> +                               callback.sendSuccess(null);

`false` isn't really an error.

Why not always report `true`/`false` and bubble exceptions using `sendError`?

::: mobile/android/modules/RuntimePermissions.jsm:33
(Diff revision 1)
> +      function() { return true; },

This is a bit of anti-pattern IMO: error is not the same thing as false.

Either catch and log the exception here, or have the Java side return true/false and just send it through.  Then exceptions bubble as you'd expect...

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8703557 [details]
MozReview Request: Bug 1235352 - Add JavaScript module for checking and requesting runtime permissions. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29387/diff/1-2/

Comment 4

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8703557 [details]
MozReview Request: Bug 1235352 - Add JavaScript module for checking and requesting runtime permissions. r=nalexander

https://reviewboard.mozilla.org/r/29387/#review27329

Looks good!

::: mobile/android/modules/RuntimePermissions.jsm:28
(Diff revisions 1 - 2)
> -   * @returns A promise resolving to true if the permission has been granted or false if the permission has been denied.
> +   * @returns A promise resolving to true if the permissions have been granted or false if the permissions have been denied.

nit: "if *all* the permissions have been granted", "if *any of* the permissions have been denied".

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java:687
(Diff revision 2)
> +            String[] permissions = message.getStringArray("permissions");

Can you verify that an empty array returns true, perhaps in the previous commit?  It's the edge case that makes `reduce` and friends possible.

How hard is it to add a little test for this?  I really doubt it's possible, since anything interesting will be prompted for, but if you have any ideas...

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8703557 [details]
MozReview Request: Bug 1235352 - Add JavaScript module for checking and requesting runtime permissions. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29387/diff/2-3/

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8703557 [details]
MozReview Request: Bug 1235352 - Add JavaScript module for checking and requesting runtime permissions. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29387/diff/3-4/

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/fx-team/rev/3ad64475829692ac813f9e7f70db6d08c017dfaa
Bug 1235352 - Add JavaScript module for checking and requesting runtime permissions. r=nalexander

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/fx-team/rev/2fcba613f9fd448442ff07268d480e66587386fa
Bug 1235352 - Disable testRuntimePermissionsAPI. r=me

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3ad644758296
https://hg.mozilla.org/mozilla-central/rev/2fcba613f9fd

Comment 10

[Will Bamberg [:wbamberg]]

I've added a page on this. Please let me know if it looks OK to you.
https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/API/RuntimePermissions.jsm

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Will Bamberg [:wbamberg] from comment #10)
> I've added a page on this. Please let me know if it looks OK to you.
> https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/API/
> RuntimePermissions.jsm

This is looking good. Thank you!

For the constants: This module currently only has constants for the permissions used by Firefox itself. Depending on what the add-on does it might need other permissions. This is the list of Android permissions:
http://developer.android.com/intl/zh-cn/reference/android/Manifest.permission.html

Comment 12

[Will Bamberg [:wbamberg]]

OK, so Sebastian confirmed that add-ons can only request permissions in the Firefox manifest, meaning that it only makes sense to use the constants. I've added a couple of sentences to the waitForPermissions() doc on this, and am marking this dev-doc-complete. But please feel free to let me now if you need anything else here.