1216532 - Request Location permission at runtime

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

This bug tracks the work needed to request the camera permission at location on Android 6+.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1216532 - GeckoAppShell.enableLocation(): Check and request location permission. r?nalexander

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

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1216532 - StumblerService: Do not run if location permission has not been granted. r?nalexander

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

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1216532 - GeckoPreferences: Request location permission if stumbler setting is changed. r?nalexander

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

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=814bfe765383

Comment 5

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8703565 [details]
MozReview Request: Bug 1216532 - GeckoAppShell.enableLocation(): Check and request location permission. r?nalexander

https://reviewboard.mozilla.org/r/29393/#review26305

wfm modulo clarifications.

::: mobile/android/base/java/org/mozilla/gecko/GeckoAppShell.java:519
(Diff revision 1)
> +                .run(new Runnable() {

Just to be sure I'm clear on the expectations here: the user will see the two prompts on first use -- one for the system permission, and one for the Gecko site-specific permission.

What order do they occur?  Can we arrange to make the system-permission happen first?  (That seems better to me, but I'm not confident how antlam et al. want this to flow.)

::: mobile/android/base/java/org/mozilla/gecko/GeckoAppShell.java:552
(Diff revision 1)
> -                        lm.requestLocationUpdates(provider, 100, (float).5, getGeckoInterface().getLocationListener(), l);
> +                            lm.requestLocationUpdates(provider, 100, (float) .5, getGeckoInterface().getLocationListener(), l);

Meh, this whitespace change is superfluous.

Comment 6

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8703566 [details]
MozReview Request: Bug 1216532 - StumblerService: Do not run if location permission has not been granted. r?nalexander

https://reviewboard.mozilla.org/r/29391/#review26307

Sure.

Comment 7

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8703567 [details]
MozReview Request: Bug 1216532 - GeckoPreferences: Request location permission if stumbler setting is changed. r?nalexander

https://reviewboard.mozilla.org/r/29395/#review26309

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java:1195
(Diff revision 1)
> -            broadcastStumblerPref(this, (Boolean) newValue);
> +            final boolean isEnabled = (Boolean) newValue;

This appears to ask for permissions even if we're transitioning from `true` to `false`.

This seems wrong to me.  Explain why I'm confused.

Further: some of our channels default to stumbler on.  Your earlier patch will silently short-circuit the stumbler in that situation.  The location permission will only be requested if a website happens to request it, and the user happens to say yes.

How do we prompt users to allow this?  Perhaps we need to disable the stumbler on startup if the location permission isn't granted, and message (snackbar?) about flipping the pref manually?  This might be one for antlam.

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

https://reviewboard.mozilla.org/r/29395/#review26309

> This appears to ask for permissions even if we're transitioning from `true` to `false`.
> 
> This seems wrong to me.  Explain why I'm confused.

You are correct. I'm going to update the patch.

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

https://reviewboard.mozilla.org/r/29393/#review26305

> Just to be sure I'm clear on the expectations here: the user will see the two prompts on first use -- one for the system permission, and one for the Gecko site-specific permission.
> 
> What order do they occur?  Can we arrange to make the system-permission happen first?  (That seems better to me, but I'm not confident how antlam et al. want this to flow.)

Currently this shows the gecko permission prompt first - then the system permission prompt. For doing it the other way around we'll need a much earlier callback from the native side: This part of the code is called after the user has accepted to share the location.

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

https://reviewboard.mozilla.org/r/29395/#review26309

This is a good point. Stumbler on by default is a problem (This is non of our channels as it seems, right?). A snackbar is a good solution. However I was hoping to get a first version done without any new strings (Upliftable if needed).

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8703565 [details]
MozReview Request: Bug 1216532 - GeckoAppShell.enableLocation(): Check and request location permission. r?nalexander

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

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8703566 [details]
MozReview Request: Bug 1216532 - StumblerService: Do not run if location permission has not been granted. r?nalexander

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

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8703567 [details]
MozReview Request: Bug 1216532 - GeckoPreferences: Request location permission if stumbler setting is changed. r?nalexander

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

Comment 14

[Nick Alexander :nalexander [he/him]]

https://reviewboard.mozilla.org/r/29393/#review27333

Comment 15

[Nick Alexander :nalexander [he/him]]

https://reviewboard.mozilla.org/r/29393/#review26305

> Currently this shows the gecko permission prompt first - then the system permission prompt. For doing it the other way around we'll need a much earlier callback from the native side: This part of the code is called after the user has accepted to share the location.

I was thinking further about this and realize that we should definitely show the Gecko permission prompt first -- if we don't, we look a bit like malware.  And the user's context is established by the cascading prompts -- grant location in browser, then grant it to the browser.  We'll see how that works in the wild.

Comment 16

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8703567 [details]
MozReview Request: Bug 1216532 - GeckoPreferences: Request location permission if stumbler setting is changed. r?nalexander

https://reviewboard.mozilla.org/r/29395/#review27335

It looks like you didn't address the issue I raised about asking for permissions even when the pref is being turned off.  Was that intentional?  (I assume not.)

Comment 17

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8703565 [details]
MozReview Request: Bug 1216532 - GeckoAppShell.enableLocation(): Check and request location permission. r?nalexander

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

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8703566 [details]
MozReview Request: Bug 1216532 - StumblerService: Do not run if location permission has not been granted. r?nalexander

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

Comment 19

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8703567 [details]
MozReview Request: Bug 1216532 - GeckoPreferences: Request location permission if stumbler setting is changed. r?nalexander

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

Comment 20

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Nick Alexander :nalexander from comment #16)
> It looks like you didn't address the issue I raised about asking for
> permissions even when the pref is being turned off.  Was that intentional? 
> (I assume not.)

I pushed a new version that handles this case properly. :)

Comment 21

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8703567 [details]
MozReview Request: Bug 1216532 - GeckoPreferences: Request location permission if stumbler setting is changed. r?nalexander

https://reviewboard.mozilla.org/r/29395/#review27735

lgtm.

Comment 22

[Nick Alexander :nalexander [he/him]]

https://reviewboard.mozilla.org/r/29395/#review27737

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java:1206
(Diff revision 3)
> -            broadcastStumblerPref(this, (Boolean) newValue);
> +            if ((Boolean) newValue) {

If the permissions abstraction handles an empty set of permissions cleanly, you could simplify this by making the permission set conditional and always going through the callback flow.

Just food for thought.

Comment 23

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/fx-team/rev/6d7b14c256fbd2142307f35f2a60fe6dcc72c8df
Bug 1216532 - GeckoAppShell.enableLocation(): Check and request location permission. r=nalexander

https://hg.mozilla.org/integration/fx-team/rev/e4234f88fcfa6f24fbc107ee2e8ee77a85407827
Bug 1216532 - StumblerService: Do not run if location permission has not been granted. r=nalexander

https://hg.mozilla.org/integration/fx-team/rev/36f58398f0574c724eadd777a8a8082d54033c87
Bug 1216532 - GeckoPreferences: Request location permission if stumbler setting is changed. r=nalexander

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6d7b14c256fb
https://hg.mozilla.org/mozilla-central/rev/e4234f88fcfa
https://hg.mozilla.org/mozilla-central/rev/36f58398f057