1240711 - After denying the location permission we might ask again after leaving the website

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

When visiting a website that asks for the location and after the user has clicked on "Share" we prompt for the permission if needed. After denying the permission and leaving the website we might ask for the permission again.

This might be caused by multiple calls into GeckoAppShell that are all guarded by a permission check. In addition to that we seem to ask for the permission even if enableLocation(false) is called.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

There are two things happening here which are annoying for the user:

1) (Sometimes?) two calls to GeckoAppShell.enableLocation(true). The first one enables location with low accuracy (probably faster results) and then sets the accuracy to high and enables the location again:

> GeckoAppShell.enableLocation(true)                <--- User is prompted
> GeckoAppShell.enableLocationHighAccuracy(true)
> GeckoAppShell.enableLocation(true)                <--- If previously denied by the user then we prompt again

I think we can handle this case in our Permissions class: If the user just denied a permission and we have a PermissionBlock with this permission in the queue then just reject it: There's no good reason to instantly ask again just after denying.

2) enableLocation(false) also does a permission check because it will access the LocationManager APIs. We do not want to prompt for enableLocation(false) if we never enabled in the first place.

This is something we could add to PermissionBlock: You do not always want to prompt; Sometimes you just need callbacks for hasPermission and hasNotPermission. This is something I'll need for other bugs as well, so let's just add it here.

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1240711 - (Part 1) Do not prompt for queued permission requests if the permission has just been denied. r?nalexander

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

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1240711 - (Part 2) PermissionBlock: Add option to not prompt the user. r?nalexander

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

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1240711 - (Part 3) GeckoAppShell: Do not prompt for permission if we are disabling the location service. r?nalexander

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

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

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

Comment 6

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8710451 [details]
MozReview Request: Bug 1240711 - (Part 1) Do not prompt for queued permission requests if the permission has just been denied. r?nalexander

https://reviewboard.mozilla.org/r/31767/#review28731

::: mobile/android/base/java/org/mozilla/gecko/permissions/Permissions.java:191
(Diff revision 1)
> +        for (String permission : block.getPermissions()) {

I don't really care, but consider just having a `Set<String>` member.  Although, now I look at the `Set` interface, it's impoverished -- so it's not much better than iteration.

Comment 7

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8710452 [details]
MozReview Request: Bug 1240711 - (Part 2) PermissionBlock: Add option to not prompt the user. r?nalexander

https://reviewboard.mozilla.org/r/31769/#review28735

Consider folding this, the null `Runnable`, and the `Context` instead of `Activity` changes into one patch.

Comment 8

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8710453 [details]
MozReview Request: Bug 1240711 - (Part 3) GeckoAppShell: Do not prompt for permission if we are disabling the location service. r?nalexander

https://reviewboard.mozilla.org/r/31771/#review28737

Nifty.  Does this pattern apply to the stumbler startup you handled earlier as well?

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Nick Alexander :nalexander from comment #6)
> mobile/android/base/java/org/mozilla/gecko/permissions/Permissions.java:191
> (Diff revision 1)
> > +        for (String permission : block.getPermissions()) {
> 
> I don't really care, but consider just having a `Set<String>` member. 
> Although, now I look at the `Set` interface, it's impoverished -- so it's
> not much better than iteration.

Looking around how to filter those lists I came across Java 8 streams. This looks fancy and might be helpful here - but I have to wait. :)


(In reply to Nick Alexander :nalexander from comment #8)
> Nifty.  Does this pattern apply to the stumbler startup you handled earlier
> as well?

Oh, that's a good idea. I'll look at that again.

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/fx-team/rev/c91239ff3c36cee3d1ddae67d0b6d62f65696781
Bug 1240711 - (Part 1) Do not prompt for queued permission requests if the permission has just been denied. r=nalexander

https://hg.mozilla.org/integration/fx-team/rev/ebcfad9074a9ce0aa54d0059a39fa9f18287a36c
Bug 1240711 - (Part 2) PermissionBlock: Add option to not prompt the user. r=nnalexander

https://hg.mozilla.org/integration/fx-team/rev/5ce83db83109db5d75efd22d5c8ff3d65af24183
Bug 1240711 - (Part 3) GeckoAppShell: Do not prompt for permission if we are disabling the location service. r=nalexander

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c91239ff3c36
https://hg.mozilla.org/mozilla-central/rev/ebcfad9074a9
https://hg.mozilla.org/mozilla-central/rev/5ce83db83109