711300 - Add GPS support for gonk

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(Possible dep on bug 714355.)

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 4

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Add GPS support for gonk

Comment 5

[Josh Matthews [:jdm]]

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 6

[Michael Wu [:mwu]]

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;

Comment 7

[Kan-Ru Chen [:kanru] (UTC+9)]

(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.

Comment 8

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Add GPS support for gonk, v2

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

Comment 9

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Add GPS support for gonk, v2

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

Comment 10

[Josh Matthews [:jdm]]

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/

Comment 11

[Kan-Ru Chen [:kanru] (UTC+9)]

(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.

Comment 12

[Josh Matthews [:jdm]]

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

Comment 13

[Kan-Ru Chen [:kanru] (UTC+9)]

(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 14

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Bug 711300 - Add GPS support for gonk, v3

Comment 15

[Josh Matthews [:jdm]]

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.

Comment 16

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Bug 711300 - Add GPS support for gonk, v4

Comment 17

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/173c06d3dd71

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/173c06d3dd71