454490 - RFE: Support multiple geolocation providers.

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Doug Turner (:dougt)]

Currently there can only be one geolocation provider in the system.  It registers itself with the following contract id:

http://mxr.mozilla.org/mozilla-central/source/dom/public/idl/geolocation/nsIGeolocationProvider.idl#140

This prevents multiple geolocation providers from exists -- there is only one.  We should change this at some point.

Comment 1

[Martin McNickle]

Looks like this is going to have be sooner than later. With the latest nightlies coming with a built in provider, any providers via extensions are ignored which isn't ideal.

I'm willing to help out on this as I have developed such an extension. What needs to be done to add a "Geolocation Provider Manager"?

-- Martin

Comment 2

[Martin McNickle]

So I guess the starting point for this would be to be able to list the available providers, and provide a preference option for turning them off and on.

With that in place, we could create a GUI for it.

So is there anyway to inspect which components are implemementing geolocationprovider through XPCOM or does this have to be built?

-- Martin

Comment 3

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 381125 [details] [diff] [review]
patch v.1

>   // if NS_MAEMO_LOCATION, see if we should try the MAEMO location provider
> #ifdef NS_MAEMO_LOCATION
>-  if (!mProvider)
>-    mProvider = new MaemoLocationProvider();
>+  provider = new MaemoLocationProvider();
>+  if (provider)
>+    mProviders.AppendObject(provider);
> #endif
> 
>   // if WINCE, see if we should try the WINCE location provider
> #ifdef WINCE_WINDOWS_MOBILE
>-  if (!mProvider){
>-    mProvider = new WinMobileLocationProvider();
>-  }
>+  provider = new WinMobileLocationProvider();
>+  if (provider)
>+    mProviders.AppendObject(provider);
> #endif
I wonder if also MaemoLocationProvider and WinMobileLocationProvider should use "geolocation-provider" category. Could be a follow-up bug.


>+nsGeolocationService::IsBetterPosition(nsIDOMGeoPosition *aSomewhere)
>+{
>+  if (!aSomewhere)
>+    return PR_FALSE;
>+
>+  PRBool betterPosition;
Unused variable?

>+
>+  nsRefPtr<nsGeolocationService> geoService = nsGeolocationService::GetInstance();
>+  nsCOMPtr<nsIDOMGeoPosition> lastPosition = geoService->GetCachedPosition();
>+
>+  if (!lastPosition)
>+    return PR_TRUE;
>+
>+  nsCOMPtr<nsIDOMGeoPositionCoords> coords;
>+  lastPosition->GetCoords(getter_AddRefs(coords));
Add a null check for coords.

>+
>+  double oldAccuracy;
>+  coords->GetAccuracy(&oldAccuracy);
>+  
>+  aSomewhere->GetCoords(getter_AddRefs(coords));
Add a null check, and perhaps you should use a different
variable than coords.

>+  double newAccuracy;
>+  coords->GetAccuracy(&newAccuracy);
>+
>+  return newAccuracy < oldAccuracy;

This should probably be "return newAccuracy <= oldAccuracy"
If the accuracy is the same, better to use the latest possible location, right?

What happens if some provider is very slow-to-update but accurate and some other is fast-to-update but inaccurate. I mean, I could manually provide
very accurate location (0.001m accuracy, or something), but then any GPS provider
couldn't override that, ever. 
I think the best option would be to try to figure out if the new position
is out of the possible old position (old position +- accuracy).


> nsGeolocationService::HasGeolocationProvider()
> {
>-  return (mProvider != nsnull);
>+  return mProviders.Count() > 0;
> }
Please add some comment to .h that the method returns true if there are
1 or more providers.

r- because the accuracy/update-speed problem needs to be solved or at least
documented somewhere.

Comment 5

[Doug Turner (:dougt)]

Attachment: patch v.2

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 383793 [details] [diff] [review]
patch v.2

>+  double oldAccuracy;
>+  coords->GetAccuracy(&oldAccuracy);
I think you should check the rv value of the method.
Same thing also elsewhere. You never know how geolocation providers
are implemented. So perhaps add few NS_ENSURE_SUCCESS(rv, PR_FALSE);

>+  // Convert to meters. 1 second of arc of latitude (or longitude at the
>+  // equator) is 1 nautical mile or 1852m.
>+  delta *= 60 * 1852;
I trust you here :)

>+  // check to see if the aSomewhere position has a better timestamp.
>+  if (newTime > oldTime + 5000)
>+    return PR_TRUE;
Is this needed at all? If the position hasn't changed at all or the more accurate
provider hasn't done any updates, why should the not so accurate provider win,
even if it gives some very wrong location?

Comment 7

[Doug Turner (:dougt)]

i will add some 'NS_ENSURE_SUCCESS(rv, PR_FALSE)'.  good idea

the time check is basically to throttle provider(s).

Comment 8

[Doug Turner (:dougt)]

Attachment: patch v.3

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #7)
> the time check is basically to throttle provider(s).
I still don't quite understand the reason. Because of the time check,
we may not always use the most accurate data.

Comment 10

[Doug Turner (:dougt)]

the check before the timestamp check will return true if the new position has a better accuracy than the current/last position.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 383856 [details] [diff] [review]
patch v.3

>+  // check to see if the aSomewhere position is more accurate
>+  if (oldAccuracy > newAccuracy)
>+    return PR_TRUE;
I think this could use >=

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #10)
> the check before the timestamp check will return true if the new position has a
> better accuracy than the current/last position.
But what if the old position has better accuracy; the timestamp check
causes geolocation to use less accurate position, even if the new position
is very inaccurate.

Comment 13

[Doug Turner (:dougt)]

how about:

if (newAccuracy < oldAccuracy)
  return PR_TRUE

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

So I think '>=' should be used, and perhaps there should be some timer
started if the previous update happened less than 5s ago. Then when the timer
fires, the cached location is used when JS is called.
And should the SetCachedPosition(aSomewhere) call be in nsGeolocationService::Update, not in  nsGeolocation::Update?

Comment 15

[Doug Turner (:dougt)]

Attachment: patch v.4

I am not sure we actually need a timer yet.  i have been thinking about changing the way providers notify us.  Instead of being this push model, we are going to go to a poll model where mozilla will ask each provider for their best location periodically.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 384497 [details] [diff] [review]
patch v.4

I guess you were going to ask a review

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 384497 [details] [diff] [review]
patch v.4

>diff --git a/dom/src/geolocation/nsGeolocation.cpp b/dom/src/geolocation/nsGeolocation.cpp
>--- a/dom/src/geolocation/nsGeolocation.cpp
>+++ b/dom/src/geolocation/nsGeolocation.cpp
>@@ -35,16 +35,18 @@
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "nsGeolocation.h"
> #include "nsAutoPtr.h"
> #include "nsCOMPtr.h"
> #include "nsIDOMWindow.h"
> #include "nsDOMClassInfo.h"
> #include "nsComponentManagerUtils.h"
>+#include "nsICategoryManager.h"
>+#include "nsISupportsPrimitives.h"
> #include "nsServiceManagerUtils.h"
> #include "nsContentUtils.h"
> #include "nsIURI.h"
> #include "nsIPermissionManager.h"
> #include "nsIObserverService.h"
> #include "nsIPrefService.h"
> #include "nsIPrefBranch2.h"
> #include "nsIJSContextStack.h"
>@@ -374,29 +376,61 @@ nsGeolocationService::nsGeolocationServi
>                                        GeoEnabledChangedCallback,
>                                        nsnull);
> 
>   GeoEnabledChangedCallback("geo.enabled", nsnull);
> 
>   if (sGeoEnabled == PR_FALSE)
>     return;
> 
>-  mProvider = do_GetService(NS_GEOLOCATION_PROVIDER_CONTRACTID);
>-  
>+  nsCOMPtr<nsIGeolocationProvider> provider = do_GetService(NS_GEOLOCATION_PROVIDER_CONTRACTID);
>+  if (provider)
>+    mProviders.AppendObject(provider);
>+
>+
>+  // look up any providers that were registered via the category manager
>+  nsCOMPtr<nsICategoryManager> catMan(do_GetService("@mozilla.org/categorymanager;1"));
>+  if (!catMan)
>+    return;
>+
>+  nsCOMPtr<nsISimpleEnumerator> geoproviders;
>+  catMan->EnumerateCategory("geolocation-provider", getter_AddRefs(geoproviders));
>+  if (geoproviders) {
>+
>+    PRBool hasMore;
>+    while (NS_SUCCEEDED(geoproviders->HasMoreElements(&hasMore)) && hasMore) {
>+      nsCOMPtr<nsISupports> elem;
>+      geoproviders->GetNext(getter_AddRefs(elem));
>+
>+      nsCOMPtr<nsISupportsCString> elemString = do_QueryInterface(elem);
>+      
>+      nsCAutoString name;
>+      elemString->GetData(name);
>+
>+      nsXPIDLCString spec;
>+      catMan->GetCategoryEntry("geolocation-provider", name.get(), getter_Copies(spec));
>+
>+      provider = do_GetService(spec);
>+      if (provider)
>+        mProviders.AppendObject(provider);
>+    }
>+  }
>+
I just found today that we have a nice helper nsCategoryCache. That could be used here, I think. See http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsCategoryCache.h
Once maemo provider and wince provider use the same registration mechanism,
nsCOMArray<nsIGeolocationProvider> mProviders could be changed to nsCategoryCache<nsIGeolocationProvider> mProviders and all this enumeration can be removed.

> NS_IMETHODIMP
> nsGeolocationService::Update(nsIDOMGeoPosition *aSomewhere)
> {
>+  // here we have to determine this aSomewhere is a "better"
>+  // position than any previously recv'ed.
>+
>+  if (!IsBetterPosition(aSomewhere))
>+    return NS_OK;
>+
>+
>   for (PRUint32 i = 0; i< mGeolocators.Length(); i++)
>     mGeolocators[i]->Update(aSomewhere);
>   return NS_OK;
> }
Could you file a bug to call SetCachedPosition(aSomewhere)  in
nsGeolocationService::Update, not in  nsGeolocation::Update.

Comment 18

[Doug Turner (:dougt)]

Bug 501431 filed to use the cache.
Bug 501433 filed to move geolocation providers outside of dom/src (and probably make them use this category stuff).

Comment 19

[Doug Turner (:dougt)]

Attachment: patch v.5

Comment 20

[Johnny Stenback (:jst)]

Comment on attachment 386068 [details] [diff] [review]
patch v.5

sr=jst

Comment 21

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/8cb1c6c93a31

Comment 22

[Daniel Veditz [:dveditz]]

Comment on attachment 386068 [details] [diff] [review]
patch v.5

Not taking new features on stable branches unless absolutely necessary. 3.6 will be out soon enough.