Bug 22022 - [GTK] add a GeoLocation implementation
: [GTK] add a GeoLocation implementation
Status: RESOLVED FIXED
: WebKit
WebKit Gtk
: 528+ (Nightly build)
: PC Linux
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2008-10-31 17:08 PST by
Modified: 2009-02-02 05:45 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2008-10-31 17:08:14 PST
At least for Linux/*BSD/freedesktop.org we need a Geolocation implementation. The candidates include gpsd, gypsy and GeoClue.
------- Comment #1 From 2008-10-31 17:21:29 PST -------
Created an attachment (id=24824) [details]
Add configure.ac support for geo location
------- Comment #2 From 2008-10-31 17:23:30 PST -------
Created an attachment (id=24825) [details]
Add stubs for GeolocationServiceGtk

Stubs and makefile changes
------- Comment #3 From 2008-10-31 17:25:58 PST -------
Created an attachment (id=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 From 2008-11-01 09:03:58 PST -------
Created an attachment (id=24833) [details]
configure.ac changes for Geolocation support

Add a new configure option for geolocation
------- Comment #5 From 2008-11-01 09:05:38 PST -------
Created an attachment (id=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 From 2008-11-01 09:08:25 PST -------
Created an attachment (id=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 From 2008-11-02 08:30:16 PST -------
Created an attachment (id=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 From 2008-11-25 06:18:56 PST -------
(From update of attachment 24833 [details])
Looks sane.  r=me

Please add a ChangeLog entry before committing.
------- Comment #9 From 2008-11-25 06:21:37 PST -------
(From update of attachment 24834 [details])
Looks good.  r=me

Please add a ChangeLog entry before committing.
------- Comment #10 From 2008-11-25 15:18:19 PST -------
(From update of attachment 24833 [details])
Clearing the review flag. The patch was landed.
------- Comment #11 From 2008-11-25 15:19:53 PST -------
(From update of attachment 24834 [details])
Clearing the review flag. The patch was landed.
------- Comment #12 From 2008-11-25 16:26:00 PST -------
(From update of attachment 24843 [details])
Looks sane. r=me.
------- Comment #13 From 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 From 2008-12-12 07:19:12 PST -------
Created an attachment (id=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 From 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 From 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 From 2008-12-12 12:33:55 PST -------
Created an attachment (id=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 From 2008-12-13 08:27:23 PST -------
(From update of attachment 25984 [details])
> 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 From 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 From 2008-12-16 07:50:10 PST -------
Created an attachment (id=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 From 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 From 2009-02-02 04:21:07 PST -------
I did land the original patch and the delta in r40441 and 40442.