We of course want geolocation support in gonk. To get that, we need to add (at least) a backend for geolocation that talks to the gps HW. There's a reasonable and usable GPS HAL in android (hardware/libhardware/include/hardware/gps.h), but I haven't investigated how it's used in higher levels of the system. We might be able to use a simpler wrapper around the HAL.

The logic is spread across frameworks/base/services/jni/com_android_server_location_GpsLocationProvider.cpp and frameworks/base/services/java/com/android/server/location/GpsLocationProvider.java. This depends on fairly accurate (ntp) time support, and full a-gps and ni-gps support have other dependencies. Let's focus this bug on just GPS support.

(Possible dep on bug 714355.)

Kan-Ru, if you're currently working on this, can you assign it to yourself?

Comment on attachment 587968 [details] [diff] [review] Add GPS support for gonk Review of attachment 587968 [details] [diff] [review]: ----------------------------------------------------------------- This looks fairly sensible, but I'd like to see another version with my comments addressed. ::: dom/src/geolocation/nsGeolocation.cpp @@ -87,4 +87,5 @@ > > #ifdef MOZ_WIDGET_ANDROID > > #include "AndroidLocationProvider.h" > > #endif > > > > +#include "GonkGPSGeolocationProvider.h" We'll want #ifdef goop here, presumably. @@ -577,3 +579,5 @@ > > if (provider) > > mProviders.AppendObject(provider); > > #endif > > + > > + provider = new GonkGPSGeolocationProvider(); And here as well. ::: dom/system/b2g/GonkGPSGeolocationProvider.cpp @@ +45,5 @@ > + > +NS_IMPL_ISUPPORTS1(GonkGPSGeolocationProvider, nsIGeolocationProvider) > + > +static nsIGeolocationUpdate* gLocationCallback = NULL; > +static const GpsInterface* gGpsInterface = NULL; There's no particular reason these need to be static globals. Let's move them inside the provider and add a public getter for the callback. The provider can be the only static global in this file. @@ +71,5 @@ > + pthread_t thread; > + pthread_attr_t attr; > + > + pthread_attr_init(&attr); > + pthread_create(&thread, &attr, (void*(*)(void*))start, arg); This cast weirds me out. Why is it necessary? @@ +85,5 @@ > + const GpsInterface* interface = NULL; > + > + err = hw_get_module(GPS_HARDWARE_MODULE_ID, (hw_module_t const**)&module); > + > + if (!err) { Invert the control flow and return NULL if hw_get_module fails. @@ +88,5 @@ > + > + if (!err) { > + hw_device_t* device; > + err = module->methods->open(module, GPS_HARDWARE_MODULE_ID, &device); > + if (!err) { Same thing here. @@ +114,5 @@ > +} > + > +GonkGPSGeolocationProvider::~GonkGPSGeolocationProvider() > +{ > + NS_IF_RELEASE(gLocationCallback); If gLocationCallback becomes an nsCOMPtr member, this can go away. @@ +121,5 @@ > +NS_IMETHODIMP > +GonkGPSGeolocationProvider::Startup() > +{ > + if (!gGpsInterface) { > + gGpsInterface = GetGPSInterface(); Why not move this into the constructor? @@ +140,5 @@ > +GonkGPSGeolocationProvider::Watch(nsIGeolocationUpdate* aCallback) > +{ > + NS_IF_RELEASE(gLocationCallback); > + gLocationCallback = aCallback; > + NS_IF_ADDREF(gLocationCallback); If the callback becomes an nsCOMPtr, the NS_IF goop can go away.

Comment on attachment 587968 [details] [diff] [review] Add GPS support for gonk >+static void >+location_callback(GpsLocation* location) >+{ >+ nsRefPtr<nsGeoPosition> somewhere = new nsGeoPosition(location->latitude, >+ location->longitude, >+ location->altitude, >+ location->accuracy, >+ location->accuracy, >+ location->bearing, >+ location->speed, >+ location->timestamp); >+ if (gLocationCallback) >+ gLocationCallback->Update(somewhere); Something like this at the top of the function would be slightly more efficient in the cases where there is no callback: if (!gLocationCallback) return;

(In reply to Josh Matthews [:jdm] from comment #5) > @@ +71,5 @@ > > + pthread_t thread; > > + pthread_attr_t attr; > > + > > + pthread_attr_init(&attr); > > + pthread_create(&thread, &attr, (void*(*)(void*))start, arg); > > This cast weirds me out. Why is it necessary? Unfortunately pthread_create and the callback disagreed on what start function should return. I can typdef pthread_func and add a comment to it. The create_thread_callback thing was for Android to be able to call java runtime from the callback, that we don't need. I guess we can call start directly, but I still create a pthread, the engine might assume a pthread_t would be returned and do something to it.

-- Attachment #587968 [details] [diff] - Attachment is obsolete: true

-- Attachment #588301 [details] [diff] - Attachment is obsolete: true

Comment on attachment 588324 [details] [diff] [review] Add GPS support for gonk, v2 Review of attachment 588324 [details] [diff] [review]: ----------------------------------------------------------------- This looks good, modulo my comments. If the answer about mStartup is that the check is just for safety, then I'm fine with leaving it in. ::: dom/system/b2g/GonkGPSGeolocationProvider.cpp @@ +107,5 @@ > +} > + > +GonkGPSGeolocationProvider::~GonkGPSGeolocationProvider() > +{ > + this->Shutdown(); No need for this->. @@ +111,5 @@ > + this->Shutdown(); > + sSingleton = NULL; > +} > + > +/* static */ GonkGPSGeolocationProvider* already_addrefed<GonkGPSGeolocationProvider> @@ +117,5 @@ > +{ > + if (!sSingleton) > + sSingleton = new GonkGPSGeolocationProvider(); > + > + NS_IF_ADDREF(sSingleton); NS_ADDREF, since new is infallible. @@ +124,5 @@ > + > +nsCOMPtr<nsIGeolocationUpdate> > +GonkGPSGeolocationProvider::GetLocationCallback() > +{ > + return mLocationCallback; This function is better expressed as already_addrefed<nsIGeolocationUpdate> GonkGPSGeolocationProvider::GetLocationCallback() { nsCOMPtr<nsIGeolocationUpdate> callback = mLocationCallback; return callback.forget(); } @@ +146,5 @@ > + > +NS_IMETHODIMP > +GonkGPSGeolocationProvider::Startup() > +{ > + if (mStarted) Why is this needed now? When do we call Startup multiple times without calling Shutdown first? ::: dom/system/b2g/GonkGPSGeolocationProvider.h @@ +36,5 @@ > + * ***** END LICENSE BLOCK ***** */ > +#ifndef GonkGPSGeolocationProvider_h > +#define GonkGPSGeolocationProvider_h > + > +#include <hardware/gps.h> // for GpsInterface GpsInterface can just be a forward declaration, can't it? @@ +51,5 @@ > + nsCOMPtr<nsIGeolocationUpdate> GetLocationCallback(); > + > +private: > + > + /* Client shoud use GetSingleton() to get the provider instance. */ s/shoud/should/

(In reply to Josh Matthews [:jdm] from comment #10) > This looks good, modulo my comments. If the answer about mStartup is that > the check is just for safety, then I'm fine with leaving it in. Just for safety. If initialized multiple times, it will be locked up. > ::: dom/system/b2g/GonkGPSGeolocationProvider.h > @@ +36,5 @@ > > + * ***** END LICENSE BLOCK ***** */ > > +#ifndef GonkGPSGeolocationProvider_h > > +#define GonkGPSGeolocationProvider_h > > + > > +#include <hardware/gps.h> // for GpsInterface > > GpsInterface can just be a forward declaration, can't it? Nope. It's a anonymous typedef struct.

Can't you just use |struct GpsInterface;| in the header, though?

(In reply to Josh Matthews [:jdm] from comment #12) > Can't you just use |struct GpsInterface;| in the header, though? Sadly I can't. GpsInterface is defined as typedef struct { /* ... */ } GpsInterface; So |struct GpsInterface| is a different type.

Comment on attachment 588812 [details] [diff] [review] Bug 711300 - Add GPS support for gonk, v3 >+ nsCOMPtr<GonkGPSGeolocationProvider> provider = >+ GonkGPSGeolocationProvider::GetSingleton(); This should be nsRefPtr. nsCOMPtr is for abstract interface classes.