832411 - a> nsIGeolocationProvider::SetHighAccuracy does not get called from nsgeolocation service for all cases.

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Stephen Li]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0
Build ID: 20130104151925

Steps to reproduce:

In custom nsIGeolocationProvider implementation want to be able to take some decisions based on whether high/low accuracy fix is requested.




Actual results:

nsIGeolocationProvider::setHighAccuracy does not get called in all cases.
At http://mxr.mozilla.org/mozilla-beta/source/dom/src/geolocation/nsGeolocation.cpp#441, nsgeolocationService::SetHigherAccuracy gets called only if enableHighAccuracy is true. Here switching becomes an issue, i.e. if at first you have high accuracy selected and then we switch back to low. 


Expected results:

Need the accuracy call for every session requested  be it high/low.

Comment 1

[Josh Matthews [:jdm]]

Attachment: Track geolocation requests' use of high accuracy and ensure the providers are update when necessary.

Comment 2

[Josh Matthews [:jdm]]

Here's the design: in a process, the service is notified of the most recent high accuracy state for each new request. It scans all existing requests to see if any of them require high accuracy, and if there's any change (ie. it's now needed or not needed), it updates the providers. When multiple processes are involved, it's more complicated - the service behaves as before, but passes the required high accuracy status for that process to the parent. The parent process removes any existing geolocation request for that process unconditionally and recreates it with the desired accuracy. This causes the providers to be updated if necessary with the new accuracy status.

Comment 3

[Josh Matthews [:jdm]]

Attachment: Track geolocation requests' use of high accuracy and ensure the providers are update when necessary.

Now with fewer copy/paste errors.

Comment 4

[Doug Turner (:dougt)]

Comment on attachment 705440 [details] [diff] [review]
Track geolocation requests' use of high accuracy and ensure the providers are update when necessary.

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

::: dom/ipc/ContentParent.cpp
@@ +2131,5 @@
> +AddGeolocationListener(nsIDOMGeoPositionCallback* watcher, bool highAccuracy)
> +{
> +    nsCOMPtr<nsIDOMGeoGeolocation> geo = do_GetService("@mozilla.org/geolocation;1");
> +    if (!geo) {
> +        return -1;

Is -1 a valid watch id?

@@ +2134,5 @@
> +    if (!geo) {
> +        return -1;
> +    }
> +
> +    nsRefPtr<nsGeolocation> geosvc = static_cast<nsGeolocation*>(geo.get());

You really should have a nsIGeolocation and do the xpcom thing here... and if you did that, you wouldn't have to fuck around with jsval.  Just create a new method that fits your need.

::: dom/src/geolocation/nsGeolocation.cpp
@@ +1224,5 @@
>    return mPendingCallbacks.Length() != 0;
>  }
>  
> +bool
> +nsGeolocation::HighAccuracyRequired()

I'd prefer the name HighAccuracyRequested()

Comment 5

[Josh Matthews [:jdm]]

Attachment: Track geolocation requests' use of high accuracy and ensure the providers are update when necessary.

Look, here are some tests as well. Also -1 is a value that indicates that we don't have a listener.

Comment 6

[Doug Turner (:dougt)]

Comment on attachment 709649 [details] [diff] [review]
Track geolocation requests' use of high accuracy and ensure the providers are update when necessary.

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

lgtm

::: dom/ipc/ContentParent.cpp
@@ +2142,5 @@
> +AddGeolocationListener(nsIDOMGeoPositionCallback* watcher, bool highAccuracy)
> +{
> +    nsCOMPtr<nsIGeolocation> geo = do_GetService("@mozilla.org/geolocation;1");
> +    if (!geo) {
> +        return -1;

4 spaces or 2 in this file?

@@ +2145,5 @@
> +    if (!geo) {
> +        return -1;
> +    }
> +
> +    mozilla::dom::GeoPositionOptions* options = new mozilla::dom::GeoPositionOptions();

Can't you just use GeoPositionOptions here?  I think we are using mozilla::dom in this file.

@@ +2146,5 @@
> +        return -1;
> +    }
> +
> +    mozilla::dom::GeoPositionOptions* options = new mozilla::dom::GeoPositionOptions();
> +    options->enableHighAccuracy = highAccuracy;

OOC, Are there setters for these sorts of things, or do we just assign like this?

@@ +2147,5 @@
> +    }
> +
> +    mozilla::dom::GeoPositionOptions* options = new mozilla::dom::GeoPositionOptions();
> +    options->enableHighAccuracy = highAccuracy;
> +    int32_t retval;

set to -1 or..

@@ +2148,5 @@
> +
> +    mozilla::dom::GeoPositionOptions* options = new mozilla::dom::GeoPositionOptions();
> +    options->enableHighAccuracy = highAccuracy;
> +    int32_t retval;
> +    geo->WatchPosition(watcher, nullptr, options, &retval);

You'll want to test for an error result here.

Comment 7

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e195e29b4d6
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5ec301ad6c0

Comment 8

[Josh Matthews [:jdm]]

Attachment: B2G18 - Track geolocation requests' use of high accuracy and ensure the providers are update when necessary.

b2gv18-applicable patch.

Comment 9

[Josh Matthews [:jdm]]

Attachment: Track geolocation requests' use of high accuracy and ensure the providers are update when necessary.

Now actually building.

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6e195e29b4d6
https://hg.mozilla.org/mozilla-central/rev/b5ec301ad6c0

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/430d15b56dd2