1043136 - Add device storage API support to permissions prompt

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

Right now the permissions prompt fails when using device storage API

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Handle access in permission prompt (v1)

Requests with access property use a permission that's a combination of the type and the access.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8464015 [details] [diff] [review]
Handle access in permission prompt (v1)

I assume this won't break other permission prompts coming into this code path?

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Only prompt for certain permissions when action is unknown (v1)

Similar to b2g [1], when a permission has unknown action, we should only prompt the user for certain permissions. For permissions like contacts and device-storage, we should default to deny and not prompt. This only applies to web content. For web apps, all unknown permissions are denied by default.

[1] http://mxr.mozilla.org/mozilla-central/source/b2g/components/ContentPermissionPrompt.js#16

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add permission prompt for device storage API (v1)

Add permission strings for device storage. Asking Ian to review the string wording.

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Mark Finkle (:mfinkle) from comment #2)
> Comment on attachment 8464015 [details] [diff] [review]
> Handle access in permission prompt (v1)
> 
> I assume this won't break other permission prompts coming into this code
> path?

Right. Existing permissions like geolocation don't have the access property and are not affected.

Comment 6

[Ian Barlow (:ibarlow)]

Jim, can you please provide a little more context about what this is, why we're doing it, and when/how a user might see such a prompt?

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8464016 [details] [diff] [review]
Only prompt for certain permissions when action is unknown (v1)

># HG changeset patch
># User Jim Chen <[e-mail]>
>
>Bug 1043136 - Only prompt for certain permissions when action is unknown;
>
>diff --git a/mobile/android/components/ContentPermissionPrompt.js b/mobile/android/components/ContentPermissionPrompt.js

>-    if (isApp && (result == Ci.nsIPermissionManager.UNKNOWN_ACTION && !!kEntities[type])) {
>+    if (denyUnknown && result == Ci.nsIPermissionManager.UNKNOWN_ACTION) {

I was worried about this change, but as you say, "contacts" is already defaulting to DENY and I assume somewhere "device-storage" is as well. That leaves "desktop-notification" and "geolocation", which you handle with the PROMPT_FOR_UNKNOWN check.

Comment 8

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Ian Barlow (:ibarlow) from comment #6)
> Jim, can you please provide a little more context about what this is, why
> we're doing it, and when/how a user might see such a prompt?

This is for the Device Storage API, which is a web API we're enabling to have parity with B2G. The purpose of this API is to let web apps access the user's pictures, music, videos, etc. For example, a web app can display the user's existing pictures, or it can save pictures to the user's pictures directory.

When a web app uses this API, the user will be prompted for permission, similar to geolocation, etc. The strings added here will be what's shown to the user.

Comment 9

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Screenshot_2014-07-29-11-15-40.png

Here is a screenshot of a web app requesting access to device storage.

Comment 10

[(inactive) Jim Chen [:jchen] [:darchons]]

Ian, see comment 8 and comment 9. Just wanted to make sure the wording of the prompts in attachment 8464017 [details] [diff] [review] looks okay.

Comment 11

[Ian Barlow (:ibarlow)]

Oh hey Jim, thanks for the nudge. Lost this bug in my inbox :P

This looks good overall. One question / comment: 

What do you think about saying "SD card" instead of "external storage"? Currently it seems a bit unspecific to me, I'm curious if we can be clearer about what the website is asking for?

Comment 12

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Ian Barlow (:ibarlow) from comment #11)
> Oh hey Jim, thanks for the nudge. Lost this bug in my inbox :P
> 
> This looks good overall. One question / comment: 
> 
> What do you think about saying "SD card" instead of "external storage"?
> Currently it seems a bit unspecific to me, I'm curious if we can be clearer
> about what the website is asking for?

Thanks! My only concern is many devices don't use SD cards and use non-removable storage instead. "External storage" seems to be the Android lingo for "either the SD card or the non-removable storage".

Comment 13

[Ian Barlow (:ibarlow)]

Oh, ok. If that's pretty standard terminology then i would be cool to just go with that then. Especially if "SD card" wouldn't be a universally accurate phrase.

Comment 14

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8464017 [details] [diff] [review]
Add permission prompt for device storage API (v1)

Thanks Ian!

Comment 15

[(inactive) Jim Chen [:jchen] [:darchons]]

Try, https://tbpl.mozilla.org/?tree=Try&rev=477925f9f8ab

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/1f3a810aed3e
https://hg.mozilla.org/integration/fx-team/rev/44055cccb7b8
https://hg.mozilla.org/integration/fx-team/rev/9879b02bd7c4

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1f3a810aed3e
https://hg.mozilla.org/mozilla-central/rev/44055cccb7b8
https://hg.mozilla.org/mozilla-central/rev/9879b02bd7c4