Closed Bug 803451 Opened 7 years ago Closed 7 years ago

GPS code prompts even when GPS is disabled

Categories

(Firefox OS Graveyard :: Gaia, defect, P2)

defect

Tracking

(blocking-basecamp:+, firefox18 fixed)

VERIFIED FIXED
blocking-basecamp +
Tracking Status
firefox18 --- fixed

People

(Reporter: mossop, Assigned: baku)

Details

(Keywords: b2g-testdriver, unagi)

Attachments

(1 file, 3 obsolete files)

Settings shows GPS is toggled off, yet opening Maps still asks you to allow geolocation and it appears in the status bar as if it is trying to locate you. I'm not outside so I don't know if it will get a fix in this state or not.
blocking-basecamp: --- → ?
Note that the GPS is probably not actually on. We just go through the security checks before we even try to run any actual GPS code. So the fix here is likely to just return early when GPS is disabled.
Summary: GPS setting claims to be off but is on → GPS code prompts even when GPS is disabled
Assignee: nobody → amarchesini
OS: Windows 7 → All
Hardware: x86_64 → All
Currently we do not check the mozSettings at the startup of the nsGeolocationService. We just observe for changes. The mozSettings API is asynchronous but GetCurrentPosition and WatchPosition are not, so probably we have to change the logic of the nsGeolocationService in order to keep them synchronously.
Attached patch patch 1 (obsolete) — Splinter Review
Waiting for green on try.
Attachment #674122 - Flags: review?(jonas)
Comment on attachment 674122 [details] [diff] [review]
patch 1

I have some timeout on try. Before asking for a review I want to fix them.
Attachment #674122 - Flags: review?(jonas)
Attached patch patch 1b (obsolete) — Splinter Review
This should be green on try soon :)
Attachment #674122 - Attachment is obsolete: true
Attachment #674198 - Flags: review?(jonas)
What is actually the expected behaviour and what does this patch do?

"Allow geolocation" isn't necessarily the same as "turn on GPS" as there are other methods of geolocation. Once this patch is applied will an app which needs geolocation simply not work, without giving any feedback to the user?

If I use the turn by turn directions app on Android it prompts me to turn on geolocation, because otherwise it can't work properly.

Do we need some UX input on this?
Flags: needinfo?(jcarpenter)
Attached patch patch 1c (obsolete) — Splinter Review
For b2g we have an additional settings service. As far as I understood this settings service manages 'global' properties. One of them is geolocation.enabled.

This patch does:
1. if the settings service exists,
2. it retrieves the geolocation.enabled property
3. then it processes any pending requests.
Attachment #674198 - Attachment is obsolete: true
Attachment #674198 - Flags: review?(jonas)
Attachment #674255 - Flags: review?(jonas)
Attachment #674255 - Flags: review?(jonas) → review?(doug.turner)
Sorry I'm still not clear on what the expected behaviour should be if an app tries to request geolocation access when GPS is turned off in the settings.

Casey or Lucas, can you confirm?
ben, my expectation is that apps can NOT use geolocation when it is disabled in settings.
(In reply to Doug Turner (:dougt) from comment #11)
> ben, my expectation is that apps can NOT use geolocation when it is disabled
> in settings.

AFAIK you can't disable geolocation in the Settings app, you can disable GPS, which isn't necessarily the only geolocation method.

But anyway, that isn't really my question. My question is about what feedback is given to the user in this case and whether this patch will cause apps to silently fail? Or is this up to the app to handle?

As a user ideally I would like to be given the opportunity to turn GPS on if an app wants to use geolocation. I was just wondering what the expected UX was in this situation.
I don't think we use any geolocating services on the device besides the GPS, at least I've never gotten it to return a location if I didn't have a clear GPS signal.  So in this case I think GPS off === geolocation off.  But if we add a geolocation service that would no longer be the case obviously.
This went through Gaia triage because it's in the Gaia component and got judged as blocking-basecamp+ on the basis the user seems to be given misleading information about whether GPS is turned on or not.

I would still like to follow up on expected behaviour. My concern is that if GPS is turned off by default (which is the sensible option for battery life), users will never see a geolocation permission prompt unless they explicity turn on GPS and apps will silently fail. I think other OSs might still prompt the user in these situations and give the user the opportunity to turn on GPS.
blocking-basecamp: ? → +
My feedback is that if the user disables the GPS, he/she doesn't want to use the geolocation service and he/she doesn't want to see the permission prompt.

A nice feature to have could be: the map application checks if the GPS is disabled and asks if the user wants to enable it or not.
Comment on attachment 674255 [details] [diff] [review]
patch 1c

Review of attachment 674255 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/src/geolocation/nsGeolocation.cpp
@@ +68,5 @@
>  // that a window can make.
>  #define MAX_GEO_REQUESTS_PER_WINDOW  1500
>  
> +// The settings key.
> +#define GEO_SETINGS_ENABLED          "geolocation.enabled"

I think there is another place we use this string.  Please convert them to use this #define

@@ +716,5 @@
> +
> +void
> +nsGeolocationService::HandleMozsettingValue(const bool aValue)
> +{
> +    if (aValue == false) {

!value

@@ +734,5 @@
> +      for (uint32_t i = 0, length = mGeolocators.Length(); i < length; ++i) {
> +        mGeolocators[i]->ServiceReady();
> +      }
> +
> +      sGeoInitPending = false;

shouldn't this be reset back to true in the case aValue == false?

@@ +1112,5 @@
>      NS_DispatchToMainThread(ev);
>      return NS_OK;
>    }
>  
> +  if (!mOwner && !nsContentUtils::IsCallerChrome()) {

i understand why we are throwing if we don't have a mOwner.  But why the test for nsContentUtils::IsCallerChrome()

::: dom/src/geolocation/nsGeolocation.h
@@ +195,5 @@
>  
>    // Check to see if the widnow still exists
>    bool WindowOwnerStillExists();
>  
> +  // Notification from the server:

Server -> service

@@ +206,5 @@
>    bool RegisterRequestWithPrompt(nsGeolocationRequest* request);
>  
> +  // Methods for the service when it's ready to process requests:
> +  NS_IMETHOD GetCurrentPositionReady(nsGeolocationRequest* aRequest);
> +  NS_IMETHOD WatchPositionReady(nsGeolocationRequest* aRequest);

These should just be declared as nsresult, not NS_IMETHOD.

@@ +226,5 @@
>    // owning back pointer.
>    nsRefPtr<nsGeolocationService> mService;
> +
> +  // Watch ID
> +  unsigned int mLastWatchId;

should this be a int32_t ?

@@ +229,5 @@
> +  // Watch ID
> +  unsigned int mLastWatchId;
> +
> +  // Pending requests are used when the service is not ready:
> +  struct PendingRequest

C++, lets use classes.
Attachment #674255 - Flags: review?(doug.turner) → review-
Thank for the review! I'm going to upload a new patch. Here few comments:

> I think there is another place we use this string.  Please convert them to
> use this #define

I think it's already done.

> @@ +734,5 @@
> > +      for (uint32_t i = 0, length = mGeolocators.Length(); i < length; ++i) {
> > +        mGeolocators[i]->ServiceReady();
> > +      }
> > +
> > +      sGeoInitPending = false;
> 
> shouldn't this be reset back to true in the case aValue == false?

No. The GeoInitPending should be used just once to get the initial value.

> > +  if (!mOwner && !nsContentUtils::IsCallerChrome()) {
> 
> i understand why we are throwing if we don't have a mOwner.  But why the
> test for nsContentUtils::IsCallerChrome()

The reason why I wrote this is because these are the condition for having NS_OK in GetCurrentPositionReady (and in the previous implementation):

  if (mOwner) {
    if (!RegisterRequestWithPrompt(aRequest)) {
      return NS_ERROR_NOT_AVAILABLE;
    }
    return NS_OK;  
  }
  
  if (!nsContentUtils::IsCallerChrome()) {
    return NS_ERROR_FAILURE;  
  }
Attached patch patch 2Splinter Review
Attachment #674255 - Attachment is obsolete: true
Attachment #676196 - Flags: review?(doug.turner)
Priority: -- → P2
Comment on attachment 676196 [details] [diff] [review]
patch 2

Review of attachment 676196 [details] [diff] [review]:
-----------------------------------------------------------------

make sure that this passes try before landing.
Attachment #676196 - Flags: review?(doug.turner) → review+
Sorry for delayed UX input, am on PTO. Good catch here. I had not considered this scenario.

> My feedback is that if the user disables the GPS, he/she doesn't want to use the 
> geolocation service and he/she doesn't want to see the permission prompt.

> A nice feature to have could be: the map application checks if the GPS is disabled > and asks if the user wants to enable it or not.

I agree with both ideas. I assume the first patch will address the first. What about the second?

> My concern is that if GPS is turned off by default (which is the sensible option for battery life)

GPS will be enabled by default. Battery life is important, absolutely, but ensuring that apps "just work" right out of the box is more important. And location-awareness in particular is vital to a mobile device. Power users who want to eke out every drop of battery life have options such as toggling the Power Saver mode from the Utility Tray, or manually disabling off GPS from Settings.
Flags: needinfo?(jcarpenter)
try: https://tbpl.mozilla.org/?tree=Try&rev=af6928587bcd
Waiting for results.

About the map application checking if the GPS is enabled/disabled, we should file a bug.
I have no experience about map app, but I would love to help :)
Keywords: checkin-needed
> > A nice feature to have could be: the map application checks if the GPS is disabled > and asks if the user wants to enable it or not.
> 
> I agree with both ideas. I assume the first patch will address the first.
> What about the second?

If we need the second scenario, the change should be done in gaia instead of gecko.
Gaia still needs information about permission asking, then check if the geolocation settings is on or off, to determine to show 'turn on geolocation' prompt or 'ask geolocation permission' prompt.
System app/Permission Manager could change the settings of geolocation if the user would like so.

I am not sure what pending request means...if I have 3 map apps opened but the setting of geolocation is turned off, then I goto settings app to turn on geolocation, would I get 3 prompts? That's bad :(
https://hg.mozilla.org/mozilla-central/rev/86e5da1a0a52
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
> If we need the second scenario, the change should be done in gaia instead of gecko.
Gaia still needs information about permission asking, then check if the geolocation settings is on or off, to determine to show 'turn on geolocation' prompt or 'ask geolocation permission' prompt.
System app/Permission Manager could change the settings of geolocation if the user would like so.

I'll file a bug and indicate non-blocking for v1. This prompt would be a nice to have.

> I am not sure what pending request means...if I have 3 map apps opened but the setting of geolocation is turned off, then I goto settings app to turn on geolocation, would I get 3 prompts? That's bad :(

Good catch. We present permission prompts to the user "in situ". That is, only once the user has the app open, viewing content that requires the permission (the "Map" tab of a Yelp app, for example). We never present permission prompts outside the app. 

Andrea, is your patch from a week ago consistent with that approach?
Created related bug for the prompt:

Prompt user to turn on Geolocation when needed by open app
https://bugzilla.mozilla.org/show_bug.cgi?id=807939
verified Unagi Build ID: 20130102070202
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.