Bug 22022 - [GTK] add a GeoLocation implementation
Summary: [GTK] add a GeoLocation implementation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-10-31 17:08 PDT by Holger Freyther
Modified: 2009-02-02 05:45 PST (History)
2 users (show)

See Also:


Attachments
Add configure.ac support for geo location (2.14 KB, patch)
2008-10-31 17:21 PDT, Holger Freyther
no flags Details | Formatted Diff | Diff
Add stubs for GeolocationServiceGtk (4.79 KB, patch)
2008-10-31 17:23 PDT, Holger Freyther
no flags Details | Formatted Diff | Diff
Implementation using the gypsy library (13.09 KB, patch)
2008-10-31 17:25 PDT, Holger Freyther
no flags Details | Formatted Diff | Diff
configure.ac changes for Geolocation support (2.14 KB, patch)
2008-11-01 09:03 PDT, Holger Freyther
no flags Details | Formatted Diff | Diff
Add stubs and GNUmakefile.am changes (4.72 KB, patch)
2008-11-01 09:05 PDT, Holger Freyther
no flags Details | Formatted Diff | Diff
GeolocationServiceGtk implementation using GeoClue (9.16 KB, patch)
2008-11-01 09:08 PDT, Holger Freyther
no flags Details | Formatted Diff | Diff
GelocationServiceGtk implementation using GeoClue (9.51 KB, patch)
2008-11-02 08:30 PST, Holger Freyther
no flags Details | Formatted Diff | Diff
tested patch with geoclue (10.04 KB, patch)
2008-12-12 07:19 PST, Aurelian Maga
no flags Details | Formatted Diff | Diff
added only delta between not landed patch and the one tested (3.39 KB, patch)
2008-12-12 12:33 PST, Aurelian Maga
no flags Details | Formatted Diff | Diff
updated patch (3.12 KB, patch)
2008-12-16 07:50 PST, Aurelian Maga
zecke: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Holger Freyther 2008-10-31 17:08:14 PDT
At least for Linux/*BSD/freedesktop.org we need a Geolocation implementation. The candidates include gpsd, gypsy and GeoClue.
Comment 1 Holger Freyther 2008-10-31 17:21:29 PDT
Created attachment 24824 [details]
Add configure.ac support for geo location
Comment 2 Holger Freyther 2008-10-31 17:23:30 PDT
Created attachment 24825 [details]
Add stubs for GeolocationServiceGtk

Stubs and makefile changes
Comment 3 Holger Freyther 2008-10-31 17:25:58 PDT
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.
Comment 4 Holger Freyther 2008-11-01 09:03:58 PDT
Created attachment 24833 [details]
configure.ac changes for Geolocation support

Add a new configure option for geolocation
Comment 5 Holger Freyther 2008-11-01 09:05:38 PDT
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.
Comment 6 Holger Freyther 2008-11-01 09:08:25 PDT
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.
Comment 7 Holger Freyther 2008-11-02 08:30:16 PST
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...
Comment 8 David Kilzer (:ddkilzer) 2008-11-25 06:18:56 PST
Comment on attachment 24833 [details]
configure.ac changes for Geolocation support

Looks sane.  r=me

Please add a ChangeLog entry before committing.
Comment 9 David Kilzer (:ddkilzer) 2008-11-25 06:21:37 PST
Comment on attachment 24834 [details]
Add stubs and GNUmakefile.am changes

Looks good.  r=me

Please add a ChangeLog entry before committing.
Comment 10 Holger Freyther 2008-11-25 15:18:19 PST
Comment on attachment 24833 [details]
configure.ac changes for Geolocation support

Clearing the review flag. The patch was landed.
Comment 11 Holger Freyther 2008-11-25 15:19:53 PST
Comment on attachment 24834 [details]
Add stubs and GNUmakefile.am changes

Clearing the review flag. The patch was landed.
Comment 12 Nikolas Zimmermann 2008-11-25 16:26:00 PST
Comment on attachment 24843 [details]
GelocationServiceGtk implementation using GeoClue

Looks sane. r=me.
Comment 13 Jan Alonzo 2008-11-26 03:59:53 PST
Hi Holger,

Just out of curiosity, should we be implementing GeolocationServiceClient instead of having our own GeolocationService impl?
Comment 14 Aurelian Maga 2008-12-12 07:19:12 PST
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)
Comment 15 Holger Freyther 2008-12-12 10:05:33 PST
Where is the difference to the not landed patch? Could you generate a patch just showing the additional changes?
Comment 16 Holger Freyther 2008-12-12 10:06:59 PST
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...
Comment 17 Aurelian Maga 2008-12-12 12:33:55 PST
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
Comment 18 Holger Freyther 2008-12-13 08:27:23 PST
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.
Comment 19 Greg Bolsinga 2008-12-13 14:56:50 PST
(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.
Comment 20 Aurelian Maga 2008-12-16 07:50:10 PST
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
Comment 21 Greg Bolsinga 2008-12-19 15:40:32 PST
(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".
Comment 22 Holger Freyther 2009-02-02 04:21:07 PST
I did land the original patch and the delta in r40441 and 40442.