Bug 22022 – [GTK] add a GeoLocation implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Holger Freyther]

At least for Linux/*BSD/freedesktop.org we need a Geolocation implementation. The candidates include gpsd, gypsy and GeoClue.

[Holger Freyther]

Created attachment 24824 [details]
Add configure.ac support for geo location

[Holger Freyther]

Created attachment 24825 [details]
Add stubs for GeolocationServiceGtk

Stubs and makefile changes

[Holger Freyther]

Created attachment 24826 [details]
Implementation using the gypsy library

An attempt at using the gypsy library. This mostly compiles and links but is completely untested, it is uploaded for reference. The error handling and mixing of querying the information and listening to signals probably needs debugging.

[Holger Freyther]

Created attachment 24833 [details]
configure.ac changes for Geolocation support

Add a new configure option for geolocation

[Holger Freyther]

Created attachment 24834 [details]
Add stubs and GNUmakefile.am changes

Add stubs for a GeolocationServiceGtk, make the idl generation use ENABLE_GEOLOCATION=1, compile with ENABLE_GEOLOCATION=1.

[Holger Freyther]

Created attachment 24835 [details]
GeolocationServiceGtk implementation using GeoClue

Implement GeolocationServiceGtk with GeoClue. This is mostly untested, the velocity, suspend, resume handling is missing (not implementable ATM), the accuracy information might not be accurate and there is no API for browser developers yet.

[Holger Freyther]

Created attachment 24843 [details]
GelocationServiceGtk implementation using GeoClue

Fixed a couple of crashes in the startUpdating method. PositionOptions* can be null, it can fail to create a GeoClueMasterClient...

[David Kilzer (:ddkilzer)]

Comment on attachment 24833 [details]
configure.ac changes for Geolocation support

Looks sane.  r=me

Please add a ChangeLog entry before committing.

[David Kilzer (:ddkilzer)]

Comment on attachment 24834 [details]
Add stubs and GNUmakefile.am changes

Looks good.  r=me

Please add a ChangeLog entry before committing.

[Holger Freyther]

Comment on attachment 24833 [details]
configure.ac changes for Geolocation support

Clearing the review flag. The patch was landed.

[Holger Freyther]

Comment on attachment 24834 [details]
Add stubs and GNUmakefile.am changes

Clearing the review flag. The patch was landed.

[Nikolas Zimmermann]

Comment on attachment 24843 [details]
GelocationServiceGtk implementation using GeoClue

Looks sane. r=me.

[Jan Alonzo]

Hi Holger,

Just out of curiosity, should we be implementing GeolocationServiceClient instead of having our own GeolocationService impl?

[Aurelian Maga]

Created attachment 25980 [details]
tested patch with geoclue

here is a new version of the patch with some minor bugs fixed (also tested with geoclue)

[Holger Freyther]

Where is the difference to the not landed patch? Could you generate a patch just showing the additional changes?

[Holger Freyther]

One thing that held me back to land the patch is privacy. There is currently no API that can allow/disable access to Geo data...

[Aurelian Maga]

Created attachment 25984 [details]
added only delta between not landed patch and the one tested

the reason I had to use GEOCLUE_ACCURACY_LEVEL_DETAILED as default was because the options object being passed was null even when I created one

[Holger Freyther]

Comment on attachment 25984 [details]
added only delta between not landed patch and the one tested

> diff -Naur a/WebCore/page/PositionError.h b/WebCore/page/PositionError.h

> -        TIMEOUT = 3
> +        TIMEOUT = 3,
> +        LOCATION_PROVIDER_ERROR = 4,
> +        POSITION_NOT_FOUND_ERROR = 5
> +
>      };

This is plain wrong. The enum is matching the PositionError of the w3c geolocation API. You may not just add values to it...

>      
>      static PassRefPtr<PositionError> create(ErrorCode code, const String& message) { return adoptRef(new PositionError(code, message)); }
> diff -Naur a/WebCore/page/PositionError.idl b/WebCore/page/PositionError.idl
> --- a/WebCore/page/PositionError.idl	2008-11-24 08:36:41.000000000 -0500
> +++ b/WebCore/page/PositionError.idl	2008-12-12 15:26:40.000000000 -0500
> @@ -35,6 +35,9 @@
>          const unsigned short PERMISSION_DENIED = 1;
>          const unsigned short POSITION_UNAVAILABLE = 2;
>          const unsigned short TIMEOUT = 3;
> +        const unsigned short LOCATION_PROVIDER_ERROR = 4;
> +        const unsigned short POSITION_NOT_FOUND_ERROR = 5;
> +
>      };

wrong, for the same reason. If you think this should be there then please participate in the W3C geolocation discussion?!

  
>  }
> diff -Naur a/WebCore/platform/gtk/GeolocationServiceGtk.cpp b/WebCore/platform/gtk/GeolocationServiceGtk.cpp
> --- a/WebCore/platform/gtk/GeolocationServiceGtk.cpp	2008-12-12 15:24:56.000000000 -0500
> +++ b/WebCore/platform/gtk/GeolocationServiceGtk.cpp	2008-12-12 15:26:40.000000000 -0500
> @@ -88,7 +88,7 @@
>          return false;
>      }
>  
> -    GeoclueAccuracyLevel accuracyLevel = GEOCLUE_ACCURACY_LEVEL_LOCALITY;
> +    GeoclueAccuracyLevel accuracyLevel = GEOCLUE_ACCURACY_LEVEL_DETAILED;

hmm, could you explain that part? It feels odd to treat a non existing option like a request for a high accuracy...

>  
>  void GeolocationServiceGtk::stopUpdating()
> @@ -168,16 +168,16 @@
>      if (error) {
>          setError(PositionError::POSITION_NOT_FOUND_ERROR, error->message);
>          return;
> -    } else if (fields & GEOCLUE_POSITION_FIELDS_LATITUDE && fields & GEOCLUE_POSITION_FIELDS_LONGITUDE) {
> +    } else if (!(fields & GEOCLUE_POSITION_FIELDS_LATITUDE && fields & GEOCLUE_POSITION_FIELDS_LONGITUDE)) {

oops.


>  
> -
> +    // don't update now, wait for Geoclue to trigger 
>      // The m_altitudeAccuracy accuracy is likely to be wrong
> -    GeoclueAccuracyLevel level;
> -    geoclue_accuracy_get_details(accuracy.get(), &level, &m_accuracy, &m_altitudeAccuracy);
> -    updatePosition();
> +    //GeoclueAccuracyLevel level;
> +    //geoclue_accuracy_get_details(accuracy.get(), &level, &m_accuracy, &m_altitudeAccuracy);
> +    //updatePosition();


oh well, could you point me to the gurantee that we will get a signal? I could not find anything in the GeoClue API doing that. And in general we do not comment out code...we remove it.


> +    if (!(fields & GEOCLUE_POSITION_FIELDS_LATITUDE && fields & GEOCLUE_POSITION_FIELDS_LONGITUDE)) {

yes.

[Greg Bolsinga]

(In reply to comment #18)
> (From update of attachment 25984 [details] [review])
> > diff -Naur a/WebCore/page/PositionError.h b/WebCore/page/PositionError.h
> 
> > -        TIMEOUT = 3
> > +        TIMEOUT = 3,
> > +        LOCATION_PROVIDER_ERROR = 4,
> > +        POSITION_NOT_FOUND_ERROR = 5
> > +
> >      };
> 
> This is plain wrong. The enum is matching the PositionError of the w3c
> geolocation API. You may not just add values to it...

Correct. These are up to the current spec, available at:

http://dev.w3.org/geo/api/spec-source.html

Please do not add these values to the enum.

[Aurelian Maga]

Created attachment 26052 [details]
updated patch

I’m sending this new patch which hopefully addresses Holger’s and Greg comments.

>This is plain wrong. The enum is matching the PositionError of the w3c
>geolocation API. You may not just add values to it...

agreed

>wrong, for the same reason. If you think this should be there then please
>participate in the W3C geolocation discussion?!

agreed. Is there any reason to have PositionError::LOCATION_PROVIDER_ERROR and PositionError::POSITION_NOT_FOUND_ERROR in the first place?

>hmm, could you explain that part? It feels odd to treat a non existing option
>like a request for a high accuracy...

agreed. I was trying to get around bug 22847

>oh well, could you point me to the gurantee that we will get a signal? I could
>not find anything in the GeoClue API doing that. And in general we do not
>comment out code...we remove it

in geoclue/example/master-pos-example.c position_changed_cb is triggered periodically

[Greg Bolsinga]

(In reply to comment #20)
> >wrong, for the same reason. If you think this should be there then please
> >participate in the W3C geolocation discussion?!
> 
> agreed. Is there any reason to have PositionError::LOCATION_PROVIDER_ERROR and
> PositionError::POSITION_NOT_FOUND_ERROR in the first place?

Mostly because the JS client will not know what to do with them. You can find the discussion on the Geolocation mailing list of how these names and values came to be. The Thread Subject is "PositionError Requests".

[Holger Freyther]

I did land the original patch and the delta in r40441 and 40442.