Created attachment 33594 [details] [review]
patch v1

Ah yes, sorry for silence.

I'm looking through any missed patches to include in the next release, which I'm hoping to do to do today... There are a couple of things that I'm not sure about here:

Default position and address: these might be good ideas but I still don't understand why they should live in localnet. Wouldn't a new "default" provider be better? If this sounds acceptable to you, then we should rename the interface to something that both default and localnet can use...

Next, there is no info on which fields are set (lat, lon, alt) in SetPosition, and most sources will not have altitude... This might be why I ended up with this in my keyfile:
  [00:1d:7e:55:8d:80]
  locality=Helsinki
  country=Finland
  street=Solnantie 24
  latitude=0.000000
  longitude=0.000000
  altitude=0.000000
  horizontal_accuracy=0.000000
when I run the example. Maybe copying GetPosition() and using a "fields" bitfield would work?

Also, the accuracy that should be saved is the level (I wish we only had level, really. I admit this is confusing): the horizontal_accuracy and vertical_accuracy are meant for sources that actually know how accurate they are (gps). 


So, I'm not including this in the release now -- sorry, I know I'm the one who's been slow. But: I'd like to put releases out regularly now, so let's get this in state we can both agree on and release it then...

Created attachment 34507 [details] [review]
patch v2

(In reply to comment #2)

> Default position and address: these might be good ideas but I still don't
> understand why they should live in localnet. Wouldn't a new "default" provider
> be better? If this sounds acceptable to you, then we should rename the
> interface to something that both default and localnet can use...

Ok, I've made a new patch implementing a new provider. I've called it "userset". I don't known if that name is good, but I can change it. It implements getting and setting addresses and positions. What do you think about working on this provider first, and once it's good enough to be included in geoclue, I will work on implementing the same thing in localnet ? 

My biggest concern in having a separate provider is: what will happen when both "userset" (or whatever we call it) and "localnet" are used: which one will "win" ?
I've also another minor concern: maybe there will be some code duplication.

> Next, there is no info on which fields are set (lat, lon, alt) in SetPosition,
> and most sources will not have altitude... This might be why I ended up with
> this in my keyfile:
>   [00:1d:7e:55:8d:80]
>   locality=Helsinki
>   country=Finland
>   street=Solnantie 24
>   latitude=0.000000
>   longitude=0.000000
>   altitude=0.000000
>   horizontal_accuracy=0.000000
> when I run the example. Maybe copying GetPosition() and using a "fields"
> bitfield would work?

great idea. That is really better because I don't have to use ugly hacks such as setting latitude to 200° to mark it as "invalid" :)

> Also, the accuracy that should be saved is the level (I wish we only had level,
> really. I admit this is confusing): the horizontal_accuracy and
> vertical_accuracy are meant for sources that actually know how accurate they
> are (gps). 

I really disagree on this one. I'm not sure for vertical accuracy, but it really makes sense for a user to define a horizontal accuracy. For privacy reason[1], user may wish to tell websites (and other applications): I want to tell you I'm in this country, or this city, or this area, but I don't want to tell you exactly where. So, the only thing I tell you is that I'm not farther than Xmeters or Xkilometers from this place. Now, user is able to get geographical informations in his area without displaying more informations than needed.

This is something possible in firefox with geolocater extension (https://addons.mozilla.org/en-US/firefox/addon/14046) (http://www.youtube.com/watch?v=wSMDQwIRVB8) I hope this becomes possible in WebkitGtk also :)

[1]: see http://pleaserobme.com/ for some of the reason why geolocation privacy matters

> So, I'm not including this in the release now -- sorry, I know I'm the one
> who's been slow. But: I'd like to put releases out regularly now, so let's get
> this in state we can both agree on and release it then...

Here is an updated patch that implements a new default provider. It probably needs attachment #34504 [details] [review] (in bug #27342) to apply correctly.

(In reply to comment #3)
> What do you think about working on this provider first, and once it's good 
> enough to be included in geoclue, I will work on implementing the same thing 
> in localnet ? 

No problem with that.

About the naming and scope: I think we need to be very careful so we don't just assume we're talking about the same thing. I haven't looked at the patch yet so bear with me if I'm talking about irrelevant things...

My assumption was that "default" provider is just a default that would be used if there is no other data, and that localnet is a provider that would be practically always used when it does have data: it is user provided so it should trump other providers (see below for more on this).

Some of your comments seem to go above those definitions, so we might need to keep discussing...

> My biggest concern in having a separate provider is: what will happen when 
> both "userset" (or whatever we call it) and "localnet" are used: which one
> will "win" ?

Yeah, this relates to why I proposed a "priority" earlier on... Well, not really proposed, but realised myself that we might need it.

I guess there are three categories at the moment:
* user defined location providers (only localnet at the moment)
* other providers
* default location provider

and it seems these categories should be used in order: E.g. if there is a location from localnet, other providers should not be asked.

I'm a bit worried about bringing this sort of logic into geoclue though, but maybe it is needed.

> > Also, the accuracy that should be saved is the level (I wish we only had 
> > level,
> > really. I admit this is confusing): the horizontal_accuracy and
> > vertical_accuracy are meant for sources that actually know how accurate they
> > are (gps). 
> 
> I really disagree on this one. I'm not sure for vertical accuracy, but it
> really makes sense for a user to define a horizontal accuracy. 

I think you misunderstood me: I do advocate having accuracy there: that's what accuracylevel is: an approximation of the accuracy the application/user should expect to receive. See geoclue/geoclue-types.h

> For privacy
> reason[1], user may wish to tell websites (and other applications): I want to
> tell you I'm in this country, or this city, or this area, but I don't want to
> tell you exactly where. So, the only thing I tell you is that I'm not farther
> than Xmeters or Xkilometers from this place. Now, user is able to get
> geographical informations in his area without displaying more informations 
> than needed.

I'm not against privacy :)

That said the issue is a lot more complex, not solvable by adding just another provider. I gave this some thought a year ago and in my view the master provider (or possibly even all providers) would have a input parameter that would define the "location context": this could be e.g. 'me', 'friends', 'world' representing who the information would be given to. The provider would then lookup the users settings for this context and set the current location accuracy appropriately (and even fuzz it if needed).

In other words it would be totally up to the application to ask for the correct context, for two reasons: First, this way Firefox could ask for 'me' context when I go to openstreetmap.org but 'world' context when I post to twitter. Second, applications could bypass any artificial restrictions anyway (just by connecting to the GPS as an example).


> Here is an updated patch that implements a new default provider. It probably
> needs attachment #34504 [details] [review] (in bug #27342) to apply correctly.

I'll take a look...

Thanks for your work, btw.

(In reply to comment #4)

> 
> About the naming and scope: I think we need to be very careful so we don't just
> assume we're talking about the same thing. I haven't looked at the patch yet so

> > My biggest concern in having a separate provider is: what will happen when 
> > both "userset" (or whatever we call it) and "localnet" are used: which one
> > will "win" ?
> 
> Yeah, this relates to why I proposed a "priority" earlier on... Well, not
> really proposed, but realised myself that we might need it.
> 
> I guess there are three categories at the moment:
> * user defined location providers (only localnet at the moment)
> * other providers
> * default location provider
> 
> and it seems these categories should be used in order: E.g. if there is a
> location from localnet, other providers should not be asked.

this makes sense

> 
> I'm a bit worried about bringing this sort of logic into geoclue though, but
> maybe it is needed.
> 
> > > Also, the accuracy that should be saved is the level (I wish we only had 
> > > level,
> > > really. I admit this is confusing): the horizontal_accuracy and
> > > vertical_accuracy are meant for sources that actually know how accurate they
> > > are (gps). 
> > 
> > I really disagree on this one. I'm not sure for vertical accuracy, but it
> > really makes sense for a user to define a horizontal accuracy. 
> 
> I think you misunderstood me: I do advocate having accuracy there: that's what
> accuracylevel is: an approximation of the accuracy the application/user should
> expect to receive. See geoclue/geoclue-types.h

If I undestand correctly, you prefere that geoclue to stores accuray expressed as: country/area/street etc instead of meters ?
But html5 geolocation api needs an accuray (and optional altitudeAccuracy) expressed in meters. If geoclue only has discrete values accuracy, it will be more complicated for webbrowser to map those values to the html5 specification. 

> That said the issue is a lot more complex, not solvable by adding just another
> provider. I gave this some thought a year ago and in my view the master
> provider (or possibly even all providers) would have a input parameter that
> would define the "location context": this could be e.g. 'me', 'friends',
> 'world' representing who the information would be given to. The provider would
> then lookup the users settings for this context and set the current location
> accuracy appropriately (and even fuzz it if needed).
> 
> In other words it would be totally up to the application to ask for the correct
> context, for two reasons: First, this way Firefox could ask for 'me' context
> when I go to openstreetmap.org but 'world' context when I post to twitter.
> Second, applications could bypass any artificial restrictions anyway (just by
> connecting to the GPS as an example).

But this would make things more complicated both for the application and for the user (having to define a context for each location aware urls will probably be annoying and/or confusing for some users).

Closing all bugs on old geoclue. If your bug still applies to new geoclue, please do re-open, I really don't have time to go through each and every bug and evaluate separately. :( Apologies for any inconvenience caused by this change.