Location requested by websites should be able to use GPS/mobile positioning

User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en; rv:1.9.0.7) Gecko/20080528 Fedora/2.24.3-3.fc10 Epiphany/2.22 Firefox/3.0
Build Identifier:

Untested patch below

Reproducible: Always

Created attachment 369611
mozilla-geoclue-for-location-1.patch

Comments welcome

Looks like I need to use the GeoclueMaster and GeoclueMasterClient APIs instead of hard-coding Gypsy, as WebKit did (https://bugs.webkit.org/show_bug.cgi?id=22022)

Bastien, is this ready for review? Sorry, I don't understand your second comment here.

(In reply to comment #3)
> Bastien, is this ready for review? Sorry, I don't understand your second
> comment here.

The current code will only work with GPS devices, and it's hard-coded. I need to update the patch to use a different API that will hide the providers (so it also works with hostip, etc.).

So it's not ready for review just yet, though I'd appreciate any comments somebody could have about the current code.

Created attachment 369792
mozilla-geoclue-for-location-2.patch

Patch that (nearly) compiles, same as -1. Can't get past that error though:
../../staticlib/components/libgklayout.a(GeoclueLocationProvider.o): In function `GeoclueLocationProvider::Startup()':
GeoclueLocationProvider.cpp:(.text+0x194): undefined reference to `geoclue_position_new'
../../staticlib/components/libgklayout.a(GeoclueLocationProvider.o): In function `location_changed(_GeocluePosition*, int, int, double, double, double, void*, void*)':
GeoclueLocationProvider.cpp:(.text+0x216): undefined reference to `geoclue_accuracy_get_details'
/usr/bin/ld: libxul.so: hidden symbol `geoclue_position_new' isn't defined
/usr/bin/ld: final link failed: Nonrepresentable section on output
collect2: ld returned 1 exit status

-lgeoclue _is_ listed in the extra dso, but I believe something is missing for me to be able to finish the linking. I also don't believe the Maemo/liblocation backend would even compile, so not a great example to follow :)

Marking new since support for geoclue is reasonable. However, what do you think of dynamically linking to geoclue so that we can build on systems that do not have geoclue?

(In reply to comment #6)
>
> Marking new since support for geoclue is reasonable. However, what do you
> think of dynamically linking to geoclue so that we can build on systems that do
> not have geoclue?

I think it might be a bit early to do that. I don't think that Geoclue's got any guarantees of API stability just yet, and I would expect problems if it were to changes (which could be causing crashes in the front-ends).

For now, I'd just fancy any hints to make it link properly, so that I can rework and test the code as mentioned in comment #2. Then using GModule it should be pretty straight forward to build without geoclue, but the support to still be enabled.

Created attachment 370177
mozilla-geoclue-for-location-3.patch

Ready for a first round of review. Note that you need to "rm dist/bin/components/TestGeolocationProvider.js" if you want to test this, otherwise you'll be in SF.

I couldn't get geoclue to report anything interesting to me I'm afraid, though Gypsy (the GPS daemon) itself did work.

If you want the code to be made run-time loadable before merging, please point me to the nspr way of doing this, otherwise I'll use GModule.

Could I have a review on the existing code please?

Comment on attachment 370177
mozilla-geoclue-for-location-3.patch

needs to be marked for review. I will try to get to it this week.

Comment on attachment 370177
mozilla-geoclue-for-location-3.patch

I would have thought that bug 454490 would have landed by now. :-( And, i would have thought that i would have reviewed this bug already.

Generally looks okay. How do I install geoclue on my ubuntu 9.04 box?

separate out the build related stuff so that Ted can review it independently.

white space is off. no tabs please.

extra blank line after the return:
+ return;
+ if (!(fields & GEOCLUE_POSITION_FIELDS_LATITUDE && fields & GEOCLUE_POSITION_FIELDS_LONGITUDE))

local variables shouldn't be prefixed with "a". That convention is for in/inout vars

If a location from geoclue doesn't have an accuracy, should the value given to the web really be 0?

Also, i am unfamiliar with the geoclue api. I am making assumptions about your usage being correct. What is the best API documentation for geoclue?

(In reply to comment #11)
> (From update of attachment 370177 [details])
> I would have thought that bug 454490 would have landed by now. :-( And, i
> would have thought that i would have reviewed this bug already.
>
> Generally looks okay. How do I install geoclue on my ubuntu 9.04 box?

There's some packages available:
http://packages.ubuntu.com/karmic/geoclue

It should be pretty straight forward to recompile the package on your version of Ubuntu. I don't think the package is very up-to-date, so compiling from git is probably a good idea (it lives on freedesktop.org).

> separate out the build related stuff so that Ted can review it independently.

Will do.

> white space is off. no tabs please.
>
> extra blank line after the return:
> + return;
> + if (!(fields & GEOCLUE_POSITION_FIELDS_LATITUDE && fields &
> GEOCLUE_POSITION_FIELDS_LONGITUDE))

Will fix.

> local variables shouldn't be prefixed with "a". That convention is for
> in/inout vars

OK.

> If a location from geoclue doesn't have an accuracy, should the value given to
> the web really be 0?

To be honest, I don't know how the backend within Mozilla will behave in those cases. Accuracy should always be available when using most of the position providers, but I wouldn't be able to tell you whether it's the right thing to do. What does the HTML5 API tell us we should be using?

> Also, i am unfamiliar with the geoclue api. I am making assumptions about your
> usage being correct. What is the best API documentation for geoclue?

I had some Geoclue people look at the code, and they seemed happy with it. The API docs are available in geoclue itself, built from gtk-doc. Usually this lives in the devel package.

FYI, I'm at GCDS and we're having a Geoclue BoF this afternoon, so we should be able to do some more work on the backend, and I'll ask people about the accuracy problem.

if you say that accuracy is zero the Mozilla implementation will assume that this is the exact point that you are at and it will override everything. Maybe that is good for a user defined position.

(In reply to comment #6)
>
> Marking new since support for geoclue is reasonable. However, what do you
> think of dynamically linking to geoclue so that we can build on systems that do
> not have geoclue?

Just a thought - instead of dynamically linking to libgeoclue, could it not just call the DBUS methods directly? Might work out a bit cleaner that way.

dumb question -- does geoclue provide info to gpsd?

(In reply to comment #15)
> dumb question -- does geoclue provide info to gpsd?

No. Geoclue is an abstraction on top of gpsd, so really, gpsd provides info to geoclue. The point of Geoclue is that an application can ask for the present location, and not care if it came from cell tower triangulation, manual user input, gpsd, or something else entirely.

(In reply to comment #14)
> Just a thought - instead of dynamically linking to libgeoclue, could it not
> just call the DBUS methods directly? Might work out a bit cleaner that way.

GeoClue provide more than D-Bus.
"Geoclue is a modular geoinformation service built on top of the D-Bus messaging system. The goal of the Geoclue project is to make creating location-aware applications as simple as possible. "

Yes, calling the DBus methods directly would be possible, libgeoclue is basically a convenience wrapper that makes the DBus calls.

FWIW, I'm waiting until I have time to clean up geoclue itself before updating this patch. If somebody wants to do it in the meantime, feel free to go ahead.

The current ModemManager backend for gsmloc uses the old Location interface.

The Location interface, along with all the other ModemManager interfaces, has been updated for MM >= 0.7.x; see:

http://cgit.freedesktop.org/ModemManager/ModemManager/tree/introspection/org.freedesktop.ModemManager1.Modem.Location.xml

In particular, location sources need to be enabled now in ModemManager via the Setup() call. 3GPP LAC/CI and CDMA Base Station location are by default enabled, but not the GPS location, which is also now implemented for several modems.

Also see the HTML documentation of the interface:

http://www.lanedo.com/~aleksander/modem-manager/api-20121005/api/gdbus-org.freedesktop.ModemManager1.Modem.Location.html

We're currently working on geoclue2, a trimmed down, more secure version of geoclue. See:
http://www.hadess.net/2013/04/geocluing-desktop-slowly.html
for some details.

I'll try and update the code here if it hasn't changed too much since, when geoclue2 is more featureful.

See also https://bugs.freedesktop.org/show_bug.cgi?id=58952

(In reply to Launchpad from comment #21)
> Marius B. Kotsbak added the following comment to Launchpad bug report
> 1100326:
>
> See also https://bugs.freedesktop.org/show_bug.cgi?id=58952

Unrelated. It's never going to get merged, as Geoclue 1 is dead.

Geoclue is being re-written, with emphasis on simplicity (API and implementation) and privacy (user's location should not be shared w/o asking for user's consent). Obviously, all apps that use geoclue need to port to new D-Bus interface: http://cgit.freedesktop.org/geoclue/tree/src/geoclue-interface.xml . Sorry no docs yet but there is two examples at least:

http://cgit.freedesktop.org/geoclue/tree/demo/where-am-i.c
https://git.gnome.org/browse/gnome-maps/tree/src/geoclue.js

Regarding privacy, I had a discussion with one of WebKit devs (can't recall the name) about the issue of browser itself asking for user's consent and hence duplication of user's input required and hence us annoying the hell out of the user. Thing is that currently this privacy is not implemented and when it is, there is certainly going to be whitelisting of applications. Browsers and GNOME Maps are the first ones to be on that list so there really shouldn't be any issue.

I see that the new API doesn't provide the altitude and the altitude accuracy information as compared to the old API - that's a shame, but any other information apart from latitude, longitude and the accuracy of those two is optional per the W3C spec[1], so that's OK really.

If you ever intend to expand the Geoclue interface and the API, we could also make use of the heading and speed information in addition to the altitude and the altitude accuracy :>

[1] http://www.w3.org/TR/geolocation-API/

(In reply to comment #1)
> I see that the new API doesn't provide the altitude and the altitude accuracy information as compared to the old API - that's a shame, but any other information apart from latitude, longitude and the accuracy of those two is optional per the W3C spec[1], so that's OK really.

Oh the only reason we don't yet support those is that currently we only have IP-based geolocation and the only open geoip DB we know, doens't provide altitude info. :( We'll add those when we have the most accurate source: GPS(A).

> If you ever intend to expand the Geoclue interface and the API, we could also make use of the heading and speed information in addition to the altitude and the altitude accuracy :>

'heading' is already on the todo and 'speed; should be very much possible when we have heading but those only make sense for more precise/portable sources than geoip.

> [1] http://www.w3.org/TR/geolocation-API/

(In reply to comment #2)
> (In reply to comment #1)
> > I see that the new API doesn't provide the altitude and the altitude accuracy information as compared to the old API - that's a shame, but any other information apart from latitude, longitude and the accuracy of those two is optional per the W3C spec[1], so that's OK really.
>
> Oh the only reason we don't yet support those is that currently we only have IP-based geolocation and the only open geoip DB we know, doens't provide altitude info. :( We'll add those when we have the most accurate source: GPS(A).

This makes much sense, thanks for clearing it up.

The geoclue 1.x codebase is obsolete now, and replaced by geoclue 2.x. I'll close this bug as we won't be updating that codebase.

Any help that you can give Zeeshan with implementing GPS support for mobile broadband modems would be greatly appreciated.

So where should a new bug for the 2.x codebase be filed instead?

(In reply to comment #3)
> So where should a new bug for the 2.x codebase be filed instead?

Right here on this bz, just that older bugs don't apply to new code.

Started working on geoclue2 provider.

(In reply to comment #4)
> Started working on geoclue2 provider.

Awesome. Would be really nice if all apps have already ported in 3.10 so distros don't have to keep shipping the old unmaintained code.

BTW, if you get annoyed by interface not being as simple as it should be, there is hope :)

https://bugs.freedesktop.org/show_bug.cgi?id=68658

(In reply to comment #5)
> (In reply to comment #4)
> > Started working on geoclue2 provider.
>
> Awesome. Would be really nice if all apps have already ported in 3.10 so distros don't have to keep shipping the old unmaintained code.

To be clear, I meant GNOME 3.10. :)

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > Started working on geoclue2 provider.
> >
> > Awesome. Would be really nice if all apps have already ported in 3.10 so distros don't have to keep shipping the old unmaintained code.
>
> To be clear, I meant GNOME 3.10. :)

While implementing it got few questions for you:
1) About accuracy - are you planning to add API to set accuracy level of the latitude and longitude coordinates? I mean high/low accuracy. Or may be I can set it via DistanceThreshold property. In this case what would be "high" accuracy threshold?
2) Same about timestamps - there is no way to obtain them now.
3) I am going to use gdbus-codegen to generate wrapper for the API like they do it for Empathy. if you had client lib for the API it would be a bit easier but anyway I can live with the generated interface.
4) And last question for now :) where can I find geoclue2 ip location provider to try out my changes?

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > (In reply to comment #4)
> > > > Started working on geoclue2 provider.
> > >
> > > Awesome. Would be really nice if all apps have already ported in 3.10 so distros don't have to keep shipping the old unmaintained code.
> >
> > To be clear, I meant GNOME 3.10. :)
>
> While implementing it got few questions for you:
> 1) About accuracy - are you planning to add API to set accuracy level of the latitude and longitude coordinates?

If I understood your question correctly, no. This info is given from geoclue to app together with coordinates. Its accuracy of the location fix in meters. Currently its city-level at best. The app-writable 'DistanceThreshold' property is to specify the minimum distance changed before geoclue should notify your app.

> 2) Same about timestamps - there is no way to obtain them now.

What kind of timestamps you need?

> 3) I am going to use gdbus-codegen to generate wrapper for the API like they do it for Empathy. if you had client lib for the API it would be a bit easier but anyway I can live with the generated interface.

Sure. I was kind hoping that Empathy guys meant contributing patches for these rather when they said "contribute". :)

> 4) And last question for now :) where can I find geoclue2 ip location provider to try out my changes?

You don't need to do anything other than to just install geoclue2 (in system prefix as that seems to be required of system dbus services) and run your app.

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (In reply to comment #5)
> > > > (In reply to comment #4)
> > > > > Started working on geoclue2 provider.
> > > >
> > > > Awesome. Would be really nice if all apps have already ported in 3.10 so distros don't have to keep shipping the old unmaintained code.
> > >
> > > To be clear, I meant GNOME 3.10. :)
> >
> > While implementing it got few questions for you:
> > 1) About accuracy - are you planning to add API to set accuracy level of the latitude and longitude coordinates?
>
> If I understood your question correctly, no. This info is given from geoclue to app together with coordinates. Its accuracy of the location fix in meters. Currently its city-level at best. The app-writable 'DistanceThreshold' property is to specify the minimum distance changed before geoclue should notify your app.

Just for information you had this API for accuracy in geoclue:
/**
 * GeoclueAccuracyLevel:
 *
 * Enum values used to define the approximate accuracy of
 * Position or Address information. These are ordered in
 * from lowest accuracy possible to highest accuracy possible.
 * geoclue_accuracy_get_details() can be used to get get the
 * current accuracy. It is up to the provider to set the
 * accuracy based on analysis of its queries.
 **/
typedef enum {
 GEOCLUE_ACCURACY_LEVEL_NONE = 0,
 GEOCLUE_ACCURACY_LEVEL_COUNTRY,
 GEOCLUE_ACCURACY_LEVEL_REGION,
 GEOCLUE_ACCURACY_LEVEL_LOCALITY,
 GEOCLUE_ACCURACY_LEVEL_POSTALCODE,
 GEOCLUE_ACCURACY_LEVEL_STREET,
 GEOCLUE_ACCURACY_LEVEL_DETAILED,
} GeoclueAccuracyLevel;

geoclue_master_client_set_requirements_*(, GeoclueAccuracyLevel, ...);
>
> > 2) Same about timestamps - there is no way to obtain them now.
>
> What kind of timestamps you need?
from the Geolocation spec: The length of time specified by the timeout property has elapsed before the implementation could successfully acquire a new Position.
in geoclue https://developer.gnome.org/geoclue/unstable/GeocluePosition.html#GeocluePositionCallback has timestamp which I guess
is abs time of acquiring the position. In geoclue2 I can calculate timestamps myself on LocationUpdated but it would be a bit in precise. But probably should be OK in most of the cases.

>
> > 3) I am going to use gdbus-codegen to generate wrapper for the API like they do it for Empathy. if you had client lib for the API it would be a bit easier but anyway I can live with the generated interface.
>
> Sure. I was kind hoping that Empathy guys meant contributing patches for these rather when they said "contribute". :)
>
> > 4) And last question for now :) where can I find geoclue2 ip location provider to try out my changes?
>
> You don't need to do anything other than to just install geoclue2 (in system prefix as that seems to be required of system dbus services) and run your app.

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > (In reply to comment #6)
> > > > (In reply to comment #5)
> > > > > (In reply to comment #4)
> > > > > > Started working on geoclue2 provider.
> > > > >
> > > > > Awesome. Would be really nice if all apps have already ported in 3.10 so distros don't have to keep shipping the old unmaintained code.
> > > >
> > > > To be clear, I meant GNOME 3.10. :)
> > >
> > > While implementing it got few questions for you:
> > > 1) About accuracy - are you planning to add API to set accuracy level of the latitude and longitude coordinates?
> >
> > If I understood your question correctly, no. This info is given from geoclue to app together with coordinates. Its accuracy of the location fix in meters. Currently its city-level at best. The app-writable 'DistanceThreshold' property is to specify the minimum distance changed before geoclue should notify your app.
>
> Just for information you had this API for accuracy in geoclue:
> /**
> * GeoclueAccuracyLevel:
> *
> * Enum values used to define the approximate accuracy of
> * Position or Address information. These are ordered in
> * from lowest accuracy possible to highest accuracy possible.
> * geoclue_accuracy_get_details() can be used to get get the
> * current accuracy. It is up to the provider to set the
> * accuracy based on analysis of its queries.
> **/
> typedef enum {
> GEOCLUE_ACCURACY_LEVEL_NONE = 0,
> GEOCLUE_ACCURACY_LEVEL_COUNTRY,
> GEOCLUE_ACCURACY_LEVEL_REGION,
> GEOCLUE_ACCURACY_LEVEL_LOCALITY,
> GEOCLUE_ACCURACY_LEVEL_POSTALCODE,
> GEOCLUE_ACCURACY_LEVEL_STREET,
> GEOCLUE_ACCURACY_LEVEL_DETAILED,
> } GeoclueAccuracyLevel;
>
> geoclue_master_client_set_requirements_*(, GeoclueAccuracyLevel, ...);

Ah! Its there in the design:

   enum RequestedAccuracy: accuracy level requested by the application

I can look into adding it today if you really need it?

> > > 2) Same about timestamps - there is no way to obtain them now.
> >
> > What kind of timestamps you need?
> from the Geolocation spec: The length of time specified by the timeout property has elapsed before the implementation could successfully acquire a new Position.
> in geoclue https://developer.gnome.org/geoclue/unstable/GeocluePosition.html#GeocluePositionCallback has timestamp which I guess
> is abs time of acquiring the position. In geoclue2 I can calculate timestamps myself on LocationUpdated but it would be a bit in precise. But probably should be OK in most of the cases.

Ah ok but I'm still interested to find out if this info is actual useful for apps? If its for measuring speed, we have in the design to add 'speed' in geoclue2 so app won't have to do that.

I think it is good to (In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > (In reply to comment #7)
> > > > (In reply to comment #6)
> > > > > (In reply to comment #5)
> > > > > > (In reply to comment #4)
> > > > > > > Started working on geoclue2 provider.
> > > > > >
> > > > > > Awesome. Would be really nice if all apps have already ported in 3.10 so distros don't have to keep shipping the old unmaintained code.
> > > > >
> > > > > To be clear, I meant GNOME 3.10. :)
> > > >
> > > > While implementing it got few questions for you:
> > > > 1) About accuracy - are you planning to add API to set accuracy level of the latitude and longitude coordinates?
> > >
> > > If I understood your question correctly, no. This info is given from geoclue to app together with coordinates. Its accuracy of the location fix in meters. Currently its city-level at best. The app-writable 'DistanceThreshold' property is to specify the minimum distance changed before geoclue should notify your app.
> >
> > Just for information you had this API for accuracy in geoclue:
> > /**
> > * GeoclueAccuracyLevel:
> > *
> > * Enum values used to define the approximate accuracy of
> > * Position or Address information. These are ordered in
> > * from lowest accuracy possible to highest accuracy possible.
> > * geoclue_accuracy_get_details() can be used to get get the
> > * current accuracy. It is up to the provider to set the
> > * accuracy based on analysis of its queries.
> > **/
> > typedef enum {
> > GEOCLUE_ACCURACY_LEVEL_NONE = 0,
> > GEOCLUE_ACCURACY_LEVEL_COUNTRY,
> > GEOCLUE_ACCURACY_LEVEL_REGION,
> > GEOCLUE_ACCURACY_LEVEL_LOCALITY,
> > GEOCLUE_ACCURACY_LEVEL_POSTALCODE,
> > GEOCLUE_ACCURACY_LEVEL_STREET,
> > GEOCLUE_ACCURACY_LEVEL_DETAILED,
> > } GeoclueAccuracyLevel;
> >
> > geoclue_master_client_set_requirements_*(, GeoclueAccuracyLevel, ...);
>
> Ah! Its there in the design:
>
> enum RequestedAccuracy: accuracy level requested by the application
>
> I can look into adding it today if you really need it?
>
Well it is not urgent but would be good to have for testing - I hope to get my change working by the end of this week.
> > > > 2) Same about timestamps - there is no way to obtain them now.
> > >
> > > What kind of timestamps you need?
> > from the Geolocation spec: The length of time specified by the timeout property has elapsed before the implementation could successfully acquire a new Position.
> > in geoclue https://developer.gnome.org/geoclue/unstable/GeocluePosition.html#GeocluePositionCallback has timestamp which I guess
> > is abs time of acquiring the position. In geoclue2 I can calculate timestamps myself on LocationUpdated but it would be a bit in precise. But probably should be OK in most of the cases.
>
> Ah ok but I'm still interested to find out if this info is actual useful for apps? If its for measuring speed, we have in the design to add 'speed' in geoclue2 so app won't have to do that.
As for the applications use cases I don't really know myself. May be someone else can comment here. I guess it can be used also to determine quality of the service (in terms of...

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (In reply to comment #5)
> > > > (In reply to comment #4)
> > > > > Started working on geoclue2 provider.
> > > >
> > > > Awesome. Would be really nice if all apps have already ported in 3.10 so distros don't have to keep shipping the old unmaintained code.
> > >
> > > To be clear, I meant GNOME 3.10. :)
> >
> > While implementing it got few questions for you:
> > 1) About accuracy - are you planning to add API to set accuracy level of the latitude and longitude coordinates?
>
> If I understood your question correctly, no. This info is given from geoclue to app together with coordinates. Its accuracy of the location fix in meters. Currently its city-level at best. The app-writable 'DistanceThreshold' property is to specify the minimum distance changed before geoclue should notify your app.
>
> > 2) Same about timestamps - there is no way to obtain them now.
>
> What kind of timestamps you need?
>
> > 3) I am going to use gdbus-codegen to generate wrapper for the API like they do it for Empathy. if you had client lib for the API it would be a bit easier but anyway I can live with the generated interface.
>
> Sure. I was kind hoping that Empathy guys meant contributing patches for these rather when they said "contribute". :)
>
> > 4) And last question for now :) where can I find geoclue2 ip location provider to try out my changes?
>
> You don't need to do anything other than to just install geoclue2 (in system prefix as that seems to be required of system dbus services) and run your app.
I am having problems running geoclue2 on Ubuntu 12.10 - some libs including glib are outdated and there are segmentations faults while running the service. I take it you developed it on Fedora?

What's you IRC account BTW?

(In reply to comment #12)
> (In reply to comment #8)
> > > 4) And last question for now :) where can I find geoclue2 ip location provider to try out my changes?
> >
> > You don't need to do anything other than to just install geoclue2 (in system prefix as that seems to be required of system dbus services) and run your app.
> I am having problems running geoclue2 on Ubuntu 12.10 - some libs including glib are outdated and there are segmentations faults while running the service.

Yikes. :(

> I take it you developed it on Fedora?

Yeah so our requirement of glib might not be correctly set in configure but then again, you shouldn't be able to build it in the first place if we are using too new API. I'm 'zeenix' on IRC. Better come to our channel #gnome-maps on irc.gnome.org.

Oh and I strongly suggest you use git master of geoclue for now. The last release had many potential crashes and issues. We'll roll out another one as soon as we've fixed some more issues.

Created attachment 212180
Patch

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

Comment on attachment 212180
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=212180&action=review

Thanks for the patch, Anton. It looks quite well already, but there's definitely more work to do (e.g. tests) so setting r- for now.

Also, I really think some more people familiar with these part of the code should review it, specially when it comes to how to incorporate this into trunk.

> Source/Platform/ChangeLog:8
> +
> + * GNUmakefile.am:

You need at least a short description for the change here. Same consideration applies to the other ChangeLogs

> Source/WebCore/GNUmakefile.am:375
> +# Geoclue interface
> +DerivedSources/geoclue2/geoclue-interface.c: DerivedSources/geoclue2/geoclue-interface.h
> +DerivedSources/geoclue2/geoclue-interface.h:
> + $(AM_V_GEN)gdbus-codegen --interface-prefix org.freedesktop.GeoClue2. --c-namespace GClue --generate-c-code DerivedSources/geoclue2/geoclue-interface $(GEOCLUE_DBUS_INTERFACE)
> +endif

I'd rather use "Geoclue2" as the generated namespace instead of the cut-down version "GClue".

Anyway, I'm not sure enough about this bit since I feel like I lack some information to provide a proper review. Thus, I'd rather have someone else reviewing the changes in this file.

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:66
> +void GeolocationProviderGeoclue2::start()

I would rename this function to something more descriptive, such as startGeoclueClient or startGeoclueSubsystem

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:72
> + if (!manager())
> + return;
> +
> + if (!client())
> + return;

You probably can combine this two in one if

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:74
> + // Set the requirement for the client

Missing period.

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:76
> + gclue_client_call_start(m_clientProxy.get(), 0, onStart, this);

What happens here if m_clientProxy is not valid? Shouldn't we control that situation if that's a possibility or just ASSERT for non-null otherwise?

Also, "onStart" looks like to me like a too simple name for the callback. What about something like geoclueClientStartCallback?

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:79
> +void GeolocationProviderGeoclue2::stop()

I would also rename this function to something more descriptive, such as stopGeoclueClient or stopGeoclueSubsystem

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:85
> + m_locationProxy.clear();
> + m_clientProxy.clear();

Shouldn't you better do this on a callback for gclue_client_call_stop?

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:108
> +void GeolocationProviderGeoclue2::onManagerProxyReady(GObject *sourceObject, GAsyncResult *result, void* self)

Wrong placement for *

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:110
> + GOwnPtr<GError> error;

Define this variable right before you need it (after the if)

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:117
> + provider->errorOccurred(String("Failed to create Geoclue manager: ") + e...

Comment on attachment 212180
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=212180&action=review

I was wondering about the testing. I should be able to use Geolocation layout tests. Also I should probably write new API tests. The main problem for me that I am not sure how to setup system D-BUS GeoClue2 service for the testing. May be I can develop some fake GeoClue2 D-BUS interface. How do you usually handle testing requiring specific D-BUS services?

>> Source/Platform/ChangeLog:8
>> + * GNUmakefile.am:
>
> You need at least a short description for the change here. Same consideration applies to the other ChangeLogs

yes

>> Source/WebCore/GNUmakefile.am:375
>> +endif
>
> I'd rather use "Geoclue2" as the generated namespace instead of the cut-down version "GClue".
>
> Anyway, I'm not sure enough about this bit since I feel like I lack some information to provide a proper review. Thus, I'd rather have someone else reviewing the changes in this file.

Can be done. I need to check latest version of the API though first.

>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:66
>> +void GeolocationProviderGeoclue2::start()
>
> I would rename this function to something more descriptive, such as startGeoclueClient or startGeoclueSubsystem

ok

>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:72
>> + return;
>
> You probably can combine this two in one if

ok

>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:74
>> + // Set the requirement for the client
>
> Missing period.

ok

>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:76
>> + gclue_client_call_start(m_clientProxy.get(), 0, onStart, this);
>
> What happens here if m_clientProxy is not valid? Shouldn't we control that situation if that's a possibility or just ASSERT for non-null otherwise?
>
> Also, "onStart" looks like to me like a too simple name for the callback. What about something like geoclueClientStartCallback?

It will be valid here since "if (!client()) return statement" will ensure that m_clientProxy is not null. But ASSERT can be added here anyway.

>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:79
>> +void GeolocationProviderGeoclue2::stop()
>
> I would also rename this function to something more descriptive, such as stopGeoclueClient or stopGeoclueSubsystem

ok

>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:85
>> + m_clientProxy.clear();
>
> Shouldn't you better do this on a callback for gclue_client_call_stop?

void GeolocationProviderGeoclue2::stop() is synchronous so I use synchronous gclue_client_call_stop here and I would prefer to do cleanup synchronously since it should reduce possible mess with start/stop calls.

>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:108
>> +void GeolocationProviderGeoclue2::onManagerProxyReady(GObject *sourceObject, GAsyncResult *result, void* self)
>
> Wrong placement for *

ok

>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:110
>> + GOwnPtr<GError> error;
>
> Define this variable right before you need it (after the if)

ok

>> Source...

Just wanted to comment here to remind you nice folks about this. Geoclue now has more sources: wifi, 3GPP and GPS. Also gnome-shell now (3.11) shows an icon in the panel when location services are in use. Would be nice to get this in place in time for 3.12 as well.

(In reply to Bastien Nocera from comment #20)
> We're currently working on geoclue2, a trimmed down, more secure version of
> geoclue. See:
> http://www.hadess.net/2013/04/geocluing-desktop-slowly.html
> for some details.
>
> I'll try and update the code here if it hasn't changed too much since, when
> geoclue2 is more featureful.

Its featureful enough now for firefox to use it. :)

(In reply to comment #20)
> Just wanted to comment here to remind you nice folks about this. Geoclue now has more sources: wifi, 3GPP and GPS. Also gnome-shell now (3.11) shows an icon in the panel when location services are in use. Would be nice to get this in place in time for 3.12 as well.

Hi, thanks for the update, I will keep it in mind, I've been busy recently but should be able to find soon some time to get the patch in.

(In reply to comment #21)
> Hi, thanks for the update, I will keep it in mind, I've been busy recently but should be able to find soon some time to get the patch in.

The 2.4 stable release is approaching (a branch has already been created). I don't know if it's too late to get this in for 2.4, but if not, the sooner a patch is proposed, the better.

It'd be a pitty to miss another stable release, particularly as the old geoclue is being dropped from some distributions and not porting webkit would mean geoclue support has to be disabled.

(In reply to comment #22)
> (In reply to comment #21)
> > Hi, thanks for the update, I will keep it in mind, I've been busy recently but should be able to find soon some time to get the patch in.
>
> The 2.4 stable release is approaching (a branch has already been created). I don't know if it's too late to get this in for 2.4, but if not, the sooner a patch is proposed, the better.
>
> It'd be a pitty to miss another stable release, particularly as the old geoclue is being dropped from some distributions and not porting webkit would mean geoclue support has to be disabled.

As long as the dependency is optional, it doesn't break any test and the patch is low risk I don't mind to add it to the stable branch, but I agree, the sooner the better. I have branched early because of the jsCStack merge that broke the world, but we are not still frozen.

Guys, I got :) Will try to do it as soon as possible - will try to find sometime at the end of the week.

(In reply to comment #24)
> Guys, I got :) Will try to do it as soon as possible - will try to find sometime at the end of the week.

Any luck in finding time for this? We just merged controls for geolocation in gnome-shell so now its even more important that at least all GNOME apps honor those preferences by using geoclue2: https://raw.github.com/gnome-design-team/gnome-mockups/master/shell/system-menu/system-status-menu-geolocation.png

Created attachment 225392
Patch Proposal

(In reply to comment #25)
> (In reply to comment #24)
> > Guys, I got :) Will try to do it as soon as possible - will try to find sometime at the end of the week.
>
> Any luck in finding time for this? We just merged controls for geolocation in gnome-shell so now
> its even more important that at least all GNOME apps honor those preferences by using geoclue2:
> https://raw.github.com/gnome-design-team/gnome-mockups/master/shell/system-menu/system-status-menu-geolocation.png

As I mentioned yesterday on IRC, I agree with Anton to lend him a hand moving this forward, as he's extremely busy these days with other matters and could barely do it on time. So, I picked his patch, rebase it against master and did a few changes (some of them following my own review) and I think I got now a patch that could be a good option already for now since it works :), and adds this as an optional thing (you need to pass --with-geoclue=2.0 at configure time).

Please take a look to it and let me know what you think. I'll try to be as fast and responsive to comments as possible during the following days.

Thanks!

(In reply to comment #26)
> patch that could be a good option already for now since it works :), and adds
> this as an optional thing (you need to pass --with-geoclue=2.0 at configure
> time).

I think it would be better to automatically choose Geoclue 2 when it is available on the system and avoid adding a new configuration option. This patch will also probably need CMake support, as it is possible that the autotools build will be gone by the time it lands.

(In reply to comment #27)
> (In reply to comment #26)
> > patch that could be a good option already for now since it works :), and adds
> > this as an optional thing (you need to pass --with-geoclue=2.0 at configure
> > time).
>
> I think it would be better to automatically choose Geoclue 2 when it is available on the system and avoid adding a new configuration option. This patch will also probably need CMake support, as it is possible that the autotools build will be gone by the time it lands.

Please, don't remove the autotools build, let's switch the bots to build with cmake by default, but keep the auotools build for a while, at least until I can make a successful release with cmake

Comment on attachment 225392
Patch Proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=225392&action=review

Thanks for the patch. I prefer if the geoclue version is detected automatically instead of having to pass an option to configure, if possible. As martin said, we need the cmake support too. I think we should use a common class name and header files, and different implementation (cpp files only) for geoclue 1 and 2. That way we don't even change anything in WebKit layer, because the provider api used by the api layer is the same.

> Source/Platform/GNUmakefile.am:15
> + -I$(top_builddir)/DerivedSources/geoclue2

This should probably be something like DerivedSources/WebCore/geoclue2. Do we really need generated code for this?

> Source/WebCore/GNUmakefile.am:389
> +# Geoclue interface

I think it's pretty obvious already :-)

> Source/WebCore/GNUmakefile.am:391
> +DerivedSources/geoclue2/geoclue-interface.c: DerivedSources/geoclue2/geoclue-interface.h
> +DerivedSources/geoclue2/geoclue-interface.h:

We should probably name the generated files GeoClue2Interface.h and GeoClue2Interface.c for consistency.

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:31
> +#include <wtf/gobject/GRefPtr.h>

This si already included by the header.

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:34
> +#include <wtf/text/WTFString.h>

Ditto.

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:41
> +using namespace WebCore;
> +
> +namespace {

Why?

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:48
> +typedef enum {
> + GCLUE_ACCURACY_LEVEL_COUNTRY = 1,
> + GCLUE_ACCURACY_LEVEL_CITY = 4,
> + GCLUE_ACCURACY_LEVEL_STREET = 6,
> + GCLUE_ACCURACY_LEVEL_EXACT = 8,
> +} GClueAccuracyLevel;

This should follow webkit coding style since it's private

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:55
> + GeolocationProviderGeoclue2* provider = static_cast<GeolocationProviderGeoclue2*>(self);

You could probably cast the callback and use GeolocationProviderGeoclue2* provider as parameter instead of void* self.

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:60
> + provider->setGeoclueManagerProxyAndGetClient(managerProxy);

Add a comment here that the provider takes the ownership.

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:68
> + GeolocationProviderGeoclue2* provider = static_cast<GeolocationProviderGeoclue2*>(self);

Ditto.

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:82
> + GeolocationProviderGeoclue2* provider = static_cast<GeolocationProviderGeoclue2*>(self);

Ditto.

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:87
> + provider->setGeoclueClientProxyAndStart(clientProxy);

Add a comment here about the ownership transfer too.

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:101
> + provider->updateLocation(locationProxy);

And here we are leaking the new proxy? updateLocation doesn't seem to take the ownership, it simply gets info from the location proxy. Maybe we could pass a GRefPtr to make it more explicit and we don...

Thanks for the review. See my comments below...

(In reply to comment #29)
> (From update of attachment 225392 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225392&action=review
>
> Thanks for the patch. I prefer if the geoclue version is detected automatically instead of having to pass an option to configure, if possible.

I thought you said you wanted this to be optional, that's why Anton added that flag (and I kept it). But I can change that of course.

> As martin said, we need the cmake support too.

Couldn't this be added as a separate patch? I'm not very fond on CMake to be honest and so I feel like this would delay this patch a bit, which should build fine anyway if autotools support is kept for a while.

> I think we should use a common class name and header files, and different implementation (cpp files only) for geoclue 1 and 2. That way we don't even change anything in WebKit layer, because the provider api used by the api layer is the same.

Ok. I'll try to do that change.

> > Source/Platform/GNUmakefile.am:15
> > + -I$(top_builddir)/DerivedSources/geoclue2
>
> This should probably be something like DerivedSources/WebCore/geoclue2. Do we really need generated code for this?

I think having that generated code is handy because it hides all the D-Bus complexity from WebCore code. Besides, it's not a big deal to generate it either (and it's following the suggested way from Geoclue2, as far as I know from Anton).

About the name, I will change it to DerivedSources/WebCore/geoclue2.

> > Source/WebCore/GNUmakefile.am:389
> > +# Geoclue interface
>
> I think it's pretty obvious already :-)

:)

> > Source/WebCore/GNUmakefile.am:391
> > +DerivedSources/geoclue2/geoclue-interface.c: DerivedSources/geoclue2/geoclue-interface.h
> > +DerivedSources/geoclue2/geoclue-interface.h:
>
> We should probably name the generated files GeoClue2Interface.h and GeoClue2Interface.c for consistency.

Ok.

> > Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:31
> > +#include <wtf/gobject/GRefPtr.h>
>
> This si already included by the header.
>

Ok. I'll remove it.

> > Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:34
> > +#include <wtf/text/WTFString.h>
>
> Ditto.
>

Ok too.

> > Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:41
> > +using namespace WebCore;
> > +
> > +namespace {
>
> Why?

This is to avoid defining static functions in the implementation file. As far as I understand, it's the recommended alternative technique to using static functions in C++, so that's why I used it.

But if you prefer, I can remove that unnamed namespace and make all those callbacks static, as in plain C.

> > Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:48
> > +typedef enum {
> > + GCLUE_ACCURACY_LEVEL_COUNTRY = 1,
> > + GCLUE_ACCURACY_LEVEL_CITY = 4,
> > + GCLUE_ACCURACY_LEVEL_STREET = 6,
> > + GCLUE_ACCURACY_LEVEL_EXACT = 8,
> > +} GClueAccuracyLevel;
>
> This should follow webkit coding style since it's private

That's right. I'll change it.

> > Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:55
> > + GeolocationProviderGeocl...

(In reply to comment #30)
> Thanks for the review. See my comments below...
>
> (In reply to comment #29)
> > (From update of attachment 225392 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=225392&action=review
> >
> > Thanks for the patch. I prefer if the geoclue version is detected automatically instead of having to pass an option to configure, if possible.
>
> I thought you said you wanted this to be optional, that's why Anton added that flag (and I kept it). But I can change that of course.

And I want it to be optional, that's orthogonal to be automatically detected. What I don't want is to bump the min requirements.

> > As martin said, we need the cmake support too.
>
> Couldn't this be added as a separate patch? I'm not very fond on CMake to be honest and so I feel like this would delay this patch a bit, which should build fine anyway if autotools support is kept for a while.

I'm sure martin can help us with this.

> > > Source/Platform/GNUmakefile.am:15
> > > + -I$(top_builddir)/DerivedSources/geoclue2
> >
> > This should probably be something like DerivedSources/WebCore/geoclue2. Do we really need generated code for this?
>
> I think having that generated code is handy because it hides all the D-Bus complexity from WebCore code. Besides, it's not a big deal to generate it either (and it's following the suggested way from Geoclue2, as far as I know from Anton).

As long as you don't break distcheck it's fine with me.

> About the name, I will change it to DerivedSources/WebCore/geoclue2.

Thanks.

> > > Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:41
> > > +using namespace WebCore;
> > > +
> > > +namespace {
> >
> > Why?
>
> This is to avoid defining static functions in the implementation file. As far as I understand, it's the recommended alternative technique to using static functions in C++, so that's why I used it.

I had to go up and down all the time to follow the calls.

> But if you prefer, I can remove that unnamed namespace and make all those callbacks static, as in plain C.

No, I said static members not static as in plain C, keeping the callbacks and the internal methods private in the class.

(In reply to comment #29)
> (From update of attachment 225392 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225392&action=review
>
> Thanks for the patch. I prefer if the geoclue version is detected automatically instead of having to pass an option to configure, if possible. As martin said, we need the cmake support too. I think we should use a common class name and header files, and different implementation (cpp files only) for geoclue 1 and 2. That way we don't even change anything in WebKit layer, because the provider api used by the api layer is the same.

Would it still be possible for webkit to provide the destkop ID of the application then? We require it in geoclue2 to be able to implement secure application authorization when we have means to reliably verify an app's desktop ID.

Created attachment 225475
Patch Proposal

Please see attached a WIP patch proposal addressing all the comments by Carlos. I'm not asking r? yet, because there are a couple of things I need to check first:

 - CMake build for Geoclue1 (I suspect is broken with this patch)
 - CMake build for Geoclue2 (I want to give it a try anyway :)
 - Providing the right desktop ID to geoclue (as it seems the dummy desktop ID included with this patch won't be good enough)

I'll try to do that on Monday, but feel free to inspect this new patch (which I tested locally and works as the previous one) if you want to

Have you all a nice weekend!

Ideas for determining the desktop file name:

 - use the GApplication app-id

   - will be equal in increasing numbers of cases

 - use g_get_prgname()

   - will be equal as long as people follow the
     prgname == wmclass == desktop name convention

at this point in time, I'd go with option #2. That should cover most cases.

One tricky thing to consider, however -- epiphany webapps. They have their own separate desktop files... Do you want to use that desktop ID here, or the main one for epiphany?

(In reply to comment #34)

> One tricky thing to consider, however -- epiphany webapps. They have their own separate desktop files... Do you want to use that desktop ID here, or the main one for epiphany?

Is the application ID used for permissions requests? WebKit will always need it's own permissions request mechanism, because it's per-security domain instead of per-application.

(In reply to comment #35)
> (In reply to comment #34)
>
> > One tricky thing to consider, however -- epiphany webapps. They have their own separate desktop files... Do you want to use that desktop ID here, or the main one for epiphany?
>
> Is the application ID used for permissions requests?

Yes.

> WebKit will always need it's own permissions request mechanism,
> because it's per-security domain instead of per-application.

I know of this special case of web browsers and I already have put at least some of the apps in the whitelist. The application ID will still be needed to be able to identify the app to determine that its whitelisted.

Comment on attachment 225475
Patch Proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=225475&action=review

> Source/WebCore/GNUmakefile.am:392
> +if GEOCLUE_API_VERSION_2
> +DerivedSources/WebCore/geoclue2/Geoclue2Interface.c: DerivedSources/WebCore/geoclue2/Geoclue2Interface.h
> +DerivedSources/WebCore/geoclue2/Geoclue2Interface.h:
> + $(AM_V_GEN)gdbus-codegen --interface-prefix org.freedesktop.GeoClue2. --c-namespace GClue --generate-c-code DerivedSources/WebCore/geoclue2/Geoclue2Interface $(GEOCLUE_DBUS_INTERFACE)
> +endif

It's so strange that there's not an API to geoclue2 beyond the DBus API. Are there plans for a frontend?

(In reply to comment #37)
> (From update of attachment 225475 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225475&action=review
>
> > Source/WebCore/GNUmakefile.am:392
> > +if GEOCLUE_API_VERSION_2
> > +DerivedSources/WebCore/geoclue2/Geoclue2Interface.c: DerivedSources/WebCore/geoclue2/Geoclue2Interface.h
> > +DerivedSources/WebCore/geoclue2/Geoclue2Interface.h:
> > + $(AM_V_GEN)gdbus-codegen --interface-prefix org.freedesktop.GeoClue2. --c-namespace GClue --generate-c-code DerivedSources/WebCore/geoclue2/Geoclue2Interface $(GEOCLUE_DBUS_INTERFACE)
> > +endif
>
> It's so strange that there's not an API to geoclue2 beyond the DBus API.

Keeping in mind that it has only one developer working on it and that he has other projects to maintain and develop, it is not strange at all. :)

> Are there plans for a frontend?

https://bugs.freedesktop.org/show_bug.cgi?id=68658

I had a look at geoclue2 and as far as I can see it's Linux-only so far, right? If so, it will be kind of difficult for me to work on this, as I'm currently on a Mac. I also don't really see myself having enough time for this in the near future, sadly :/

However, I'll still keep an eye on this bug, and I'll be glad to answer questions/give advice on the geolocation bits if you need.

(In reply to Guilherme Gonçalves [:ggp] from comment #24)
> I had a look at geoclue2 and as far as I can see it's Linux-only so far,
> right? If so, it will be kind of difficult for me to work on this, as I'm
> currently on a Mac.

Ryan Lortie recently made it work on the basic level (only IP-based geolocation) on FreeBSD so it might just work for testing and development purposes on Mac too.

Created attachment 225774
Patch proposal

Attaching a new patch that tries to address all the issues mentioned in this thread, in a way or another. See comments below...

(In reply to comment #33)
> Created an attachment (id=225475) [details]
> Patch Proposal
>
> Please see attached a WIP patch proposal addressing all the comments by Carlos. I'm not asking r? yet, because there are a couple of things I need to check first:
> - CMake build for Geoclue1 (I suspect is broken with this patch)

Indeed it was broken, but fixing this was easy. Done :)

> - CMake build for Geoclue2 (I want to give it a try anyway :)

That was not that easy for a CMake newbie, but I think it has been properly addressed as well in this new patch (with a bit of help from Martin. Thanks!)

> - Providing the right desktop ID to geoclue (as it seems the dummy desktop ID included with this patch won't be good enough)

I'm a bit reluctant to do "too many assumptions" from the Geoclue2 provider in terms of which kind of application is embedding WebKit, since it might be an app that does not have a .desktop file or even an app embedding other port different than WebKitGTK+, since this provider is meant to be generic enough to be shared with other platforms (e.g. EFL).

So, instead of trying the original idea commented on IRC of getting the top level window and extracting the app ID from there, in this patch I'm just providing the program name as the ID, which in many cases will match the name of the .desktop file anyway (not for the case of ephy web apps, though). It may not be the perfect solution, but I hope it will be good enough for now. At least, geoclue should have enough information now to add some GNOME apps using webkit (e.g. epiphany) to the white list, for instance.

Anyway, other ideas, comments and even personal attacks are welcome, should you had any :)

> I'll try to do that on Monday, but feel free to inspect this new patch (which I tested locally and works as the previous one) if you want to

It's Tuesday already, hope it's not too late!

Please review, thanks!

Attachment 225774 did not pass style-queue:

ERROR: Source/WebCore/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5]
Total errors found: 1 in 17 files

If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 225775
Patch proposal

New patch fixing style error in the ChangeLog

Comment on attachment 225775
Patch proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=225775&action=review

The CMake parts look okay. Instead of adding the source files to the list conditionally, I would add them both unconditionally and guard them with a preprocessor define. I prefer GEOCLUE_API_VERSION_2 to be named WTF_USE_GEOCLUE2. The GTK+ version one is not a great example to follow. The last general comment I have is that we avoid polluting the command-line flags any further, if we can avoid it. Can you try to make this option available in the config.h file. I can show you how to do it for CMake.

> Source/Platform/GNUmakefile.am:109
> +if GEOCLUE_API_VERSION_2
> +libPlatform_la_CPPFLAGS += \
> + $(GEOCLUE2_CFLAGS)
> +else
> +libPlatform_la_CPPFLAGS += \
> + $(GEOCLUE_CFLAGS)
> +endif

You can use GEOCLUE_CFLAGS for both GeoClue1 and GeoClue2 and avoid this change.

> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue.h:37
> +#if defined(GEOCLUE_API_VERSION_2)
> +#include "Geoclue2Interface.h"
> +#else
> #include <geoclue/geoclue-master.h>
> #include <geoclue/geoclue-position.h>
> +#endif
> #include <wtf/gobject/GRefPtr.h>
> +#if defined(GEOCLUE_API_VERSION_2)
> +#include <wtf/text/WTFString.h>
> +#endif

You should split off the conditional includes at the end of the list instead of trying to keep them alphabetical. This will make it a bit simpler here. :)

> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue2.cpp:36
> +#define GEOCLUE_BUS_NAME "org.freedesktop.GeoClue2"
> +#define GEOCLUE_MANAGER_PATH "/org/freedesktop/GeoClue2/Manager"

Please use static const char* for these.

> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue2.cpp:44
> + GClueAccuracyLevelCountry = 1,
> + GClueAccuracyLevelCity = 4,
> + GClueAccuracyLevelStreet = 6,
> + GClueAccuracyLevelExact = 8,

GClue -> GeoClue.

Comment on attachment 225775
Patch proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=225775&action=review

(In reply to comment #42)
> (From update of attachment 225775 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225775&action=review
>
> The CMake parts look okay.

Thanks!

> Instead of adding the source files to the list conditionally, I would add them
> both unconditionally and guard them with a preprocessor define. I prefer
> GEOCLUE_API_VERSION_2 to be named WTF_USE_GEOCLUE2. The GTK+ version one is
> not a great example to follow.

You mean defining then both GEOCLUE_API_VERSION_1 and GEOCLUE_API_VERSION_2, making them both available to the code (so we can disable one file or the other), right?

> The last general comment I have is that we avoid polluting the command-line
> flags any further, if we can avoid it. Can you try to make this option
> available in the config.h file.

Ok, I'll do

> I can show you how to do it for CMake.

Thanks, that would be very much appreciated :)

>> Source/Platform/GNUmakefile.am:109
>> +endif
>
> You can use GEOCLUE_CFLAGS for both GeoClue1 and GeoClue2 and avoid this change.

ok

>> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue.h:37
>> +#endif
>
> You should split off the conditional includes at the end of the list instead of trying to keep them alphabetical. This will make it a bit simpler here. :)

ok

>> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue2.cpp:36
>> +#define GEOCLUE_MANAGER_PATH "/org/freedesktop/GeoClue2/Manager"
>
> Please use static const char* for these.

No problem

>> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue2.cpp:44
>> + GClueAccuracyLevelExact = 8,
>
> GClue -> GeoClue.

ok

(In reply to comment #43)
> (From update of attachment 225775 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225775&action=review
>

> You mean defining then both GEOCLUE_API_VERSION_1 and GEOCLUE_API_VERSION_2, making them both available to the code (so we can disable one file or the other), right?

I mean to use neither of those, but to define WTF_USE_GEOCLUE2. Before one file you would do something like this:

#if ENABLE(GEOLOCATION) && !USE(GEOCLUE2)

and the other this:

#if ENABLE(GEOLOCATION) && USE(GEOCLUE2)

(In reply to comment #44)
> (In reply to comment #43)
> > (From update of attachment 225775 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=225775&action=review
> >
>
> > You mean defining then both GEOCLUE_API_VERSION_1 and GEOCLUE_API_VERSION_2, making them both available to the code (so we can disable one file or the other), right?
>
> I mean to use neither of those, but to define WTF_USE_GEOCLUE2.

Ok.

Before one file you would do something like this:
>
> #if ENABLE(GEOLOCATION) && !USE(GEOCLUE2)
>
> and the other this:
>
> #if ENABLE(GEOLOCATION) && USE(GEOCLUE2)

Alright

Created attachment 225788
Patch proposal

New patch addressing the issues commented by Martin

Just a quick heads-up to mention that I just finished four full builds from scratch right now, one using autotools and the other using cmake, to check whether I broke any of the Geoclue1 or Geoclue2 building paths, and everything went OK.

Sorry not to have done this yesterday but I was in a hurry. Just mentioning it now so to state that the latest changes in the build systems are properly tested as well.

Comment on attachment 225788
Patch proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=225788&action=review

Looks good to me, in general, but Carlos should probably do a review as well since he's managing the stable branch.

> Source/Platform/GNUmakefile.am:16
> - -I$(top_builddir)/DerivedSources/Platform
> + -I$(top_builddir)/DerivedSources/Platform \
> + -I$(top_builddir)/DerivedSources/WebCore
>

It probably makes sense to generate the files in DerivedSources/Platform for the autotools port.

> Source/WebCore/PlatformGTK.cmake:279
> + COMMAND gdbus-codegen --interface-prefix org.freedesktop.GeoClue2. --c-namespace GClue --generate-c-code ${DERIVED_SOURCES_WEBCORE_DIR}/Geoclue2Interface ${GEOCLUE_DBUS_INTERFACE}

So, if possible it would be could to use GeoClue as the C namespace.

> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue2.cpp:36
> +const char* GeoClueBusName = "org.freedesktop.GeoClue2";
> +const char* GeoClueManagerPath = "/org/freedesktop/GeoClue2/Manager";

These should be named like gGeoClueBusName and gGeoClueManagerPath.

> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue2.cpp:117
> + // The provider will take ownershipt of the managerProxy.

ownershipt->ownership

Comment on attachment 225788
Patch proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=225788&action=review

> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue.h:28
> +#include <wtf/text/WTFString.h>

Why are you adding this? I don't see any String used by this header.

> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue2.cpp:50
> + , m_managerProxy(0)
> + , m_clientProxy(0)

You don't need to initialize GRefPtr, and in case of doing so, use nullptr instead of 0

> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue2.cpp:118
> + provider->setGeoclueManagerProxyAndGetClient(managerProxy);

The idea of using static memebers for callbacks was also to simplify this, and clarify the ownership. I think now you can directly here do

provider->m_managerProxy = adoptGRef(gclue_manager_proxy_new_for_bus_finish(result, &error.outPtr()));
if (error) {
    provider->errorOccurred(error->message);
    return;
}

gclue_manager_call_get_client(provider->m_managerProxy.get(), nullptr, reinterpret_cast<GAsyncReadyCallback>(getGeoclueClientCallback), provider);

no? we make the code simpler and easier to follow, avoiding jumping up and down all the time. Another approach would be the opposite, move all the handling to the object itself, leaving the callbacks with a single line, for example:

provider->handleGeoclueManagerProxyCreated(result);

> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue2.cpp:162
> + // Geoclue2 requires the client to provide a desktop ID for security
> + // reasons, which should identify the application requesting the location.
> + CString programName = getCurrentExecutablePath();
> + gclue_client_set_desktop_id(m_clientProxy.get(), basename(programName.data()));

So, I guess the program name works as desktop file id in the end? or is it just a workaround? Wouldn't it be easier to directly use g_get_prgname() that returns a const char *?

Created attachment 225978
Patch proposal

Thanks Martin and Carlos for the reviews. I'm attaching a new patch addressing all those issues.

See some comments of mine below...

(In reply to comment #48)
> (From update of attachment 225788 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225788&action=review
>
> Looks good to me, in general, but Carlos should probably do a review as well since he's managing the stable branch.

Sure.

> > Source/Platform/GNUmakefile.am:16
> > - -I$(top_builddir)/DerivedSources/Platform
> > + -I$(top_builddir)/DerivedSources/Platform \
> > + -I$(top_builddir)/DerivedSources/WebCore
> >
>
> It probably makes sense to generate the files in DerivedSources/Platform for the autotools port.
>
Done.

> > Source/WebCore/PlatformGTK.cmake:279
> > + COMMAND gdbus-codegen --interface-prefix org.freedesktop.GeoClue2. --c-namespace GClue --generate-c-code ${DERIVED_SOURCES_WEBCORE_DIR}/Geoclue2Interface ${GEOCLUE_DBUS_INTERFACE}
>
> So, if possible it would be could to use GeoClue as the C namespace.
>
Might I suggest changing "GClue" to "Geoclue" (lower case "c") instead of to "GeoClue" here?

The reason why I suggest that is to keep the signature of the generated methods more similar to the Geoclue1 counterpart, having things like geoclue_location_get_latitude() instead of geo_clue_location_get_latitude().

In the new patch I did that ("Geoclue"), but I can change it to GeoClue if the naming of the generated methods is not an issue.

> > Source/WebCore/platform/geoclue/GeolocationProviderGeoclue2.cpp:36
> > +const char* GeoClueBusName = "org.freedesktop.GeoClue2";
> > +const char* GeoClueManagerPath = "/org/freedesktop/GeoClue2/Manager";
>
> These should be named like gGeoClueBusName and gGeoClueManagerPath.
>
> > Source/WebCore/platform/geoclue/GeolocationProviderGeoclue2.cpp:117
> > + // The provider will take ownershipt of the managerProxy.
>
> ownershipt->ownership

Done.

(In reply to comment #49)
> (From update of attachment 225788 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225788&action=review
>
> > Source/WebCore/platform/geoclue/GeolocationProviderGeoclue.h:28
> > +#include <wtf/text/WTFString.h>
>
> Why are you adding this? I don't see any String used by this header.
>
It was in the original patch from Anton and forgot to remove it.

Now removed.

> > Source/WebCore/platform/geoclue/GeolocationProviderGeoclue2.cpp:50
> > + , m_managerProxy(0)
> > + , m_clientProxy(0)
>
> You don't need to initialize GRefPtr, and in case of doing so, use nullptr instead of 0
>

Removed those 2 lines.

> > Source/WebCore/platform/geoclue/GeolocationProviderGeoclue2.cpp:118
> > + provider->setGeoclueManagerProxyAndGetClient(managerProxy);
>
> The idea of using static memebers for callbacks was also to simplify this, and clarify the ownership. I think now you can directly here do
>
> provider->m_managerProxy = adoptGRef(gclue_manager_proxy_new_for_bus_finish(result, &error.outPtr()));
> if (error) {
> provider->errorOccurred(error->message);
> return;
> }
>
> gclue_manager_call_get_client(provider->m_managerProxy.get(), nullptr, reinter...

Created attachment 226005
Patch proposal

New patch fixing a mistake in Source/WebCore/GNUmakefile.list.am that was causing trouble when building with build-webkit --gtk (it was fine using ./autogen.sh && make).

AS much as I hate rushing people for reviews, I would appreciate if you (Martin & Carlos) could take another look soon, since tomorrow is Friday and next release is approaching :)

Thanks!

Comment on attachment 226005
Patch proposal

Attachment 226005 did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6437091126804480

New failing tests:
fast/text/selection-rect-line-height-too-big.html
editing/selection/inline-closest-leaf-child.html
fast/text/selection-rect-line-height-too-small.html
fast/inline-block/14498-positionForCoordinates.html

Created attachment 226010
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5

Comment on attachment 226005
Patch proposal

Attachment 226005 did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5006350815854592

New failing tests:
fast/text/selection-rect-line-height-too-big.html
editing/selection/inline-closest-leaf-child.html
fast/text/selection-rect-line-height-too-small.html
fast/inline-block/14498-positionForCoordinates.html

Created attachment 226016
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03 Port: mac-mountainlion Platform: Mac OS X 10.8.5

Comment on attachment 226005
Patch proposal

Attachment 226005 did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5914514671271936

New failing tests:
fast/text/selection-rect-line-height-too-big.html
editing/selection/inline-closest-leaf-child.html
fast/text/selection-rect-line-height-too-small.html
fast/inline-block/14498-positionForCoordinates.html

Created attachment 226024
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02 Port: mac-mountainlion Platform: Mac OS X 10.8.5

Created attachment 226084
Patch proposal

(In reply to comment #51)
> Created an attachment (id=226005) [details]
> Patch proposal
>
> New patch fixing a mistake in Source/WebCore/GNUmakefile.list.am that was causing trouble when building with build-webkit --gtk (it was fine using ./autogen.sh && make).
>
> AS much as I hate rushing people for reviews, I would appreciate if you (Martin & Carlos) could take another look soon, since tomorrow is Friday and next release is approaching :)
>
> Thanks!

Argh.. It seems I uploaded the wrong patch by mistake. Now uploading the right one, sorry for the hassle.

Comment on attachment 226084
Patch proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=226084&action=review

> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue.h:28
> +#include <wtf/text/WTFString.h>

Please, read my previous review, or am I wrong and String is actually used by this header?

> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue.h:55
> + void setGeoclueManagerProxyAndGetClient(GeoclueManager*);
> + void setGeoclueClientProxyAndStart(GeoclueClient*);

Are these methods implemented somewhere?

> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue2.cpp:142
> + // Geoclue2 requires the client to provide a desktop ID for security
> + // reasons, which should identify the application requesting the location.
> + geoclue_client_set_desktop_id(provider->m_clientProxy.get(), g_get_prgname());

I still don't know whether using the program name is the right thing in the end or a workaround. If it's a workaround, please file a bug report and add a FIXME here pointing to the bug report. Because at the moment your comment is confusing here, it says geoclue requires a desktop ID, but we are providing an application name

(In reply to comment #59)
> (From update of attachment 226084 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226084&action=review
>
> > Source/WebCore/platform/geoclue/GeolocationProviderGeoclue.h:28
> > +#include <wtf/text/WTFString.h>
>

I'd swear I removed those, but must forgot to add the change to the commit.

> Please, read my previous review, or am I wrong and String is actually used by this header?
>
> > Source/WebCore/platform/geoclue/GeolocationProviderGeoclue.h:55
> > + void setGeoclueManagerProxyAndGetClient(GeoclueManager*);
> > + void setGeoclueClientProxyAndStart(GeoclueClient*);
>
> Are these methods implemented somewhere?
>
Nope, probably the same mistake creating the patch.

> > Source/WebCore/platform/geoclue/GeolocationProviderGeoclue2.cpp:142
> > + // Geoclue2 requires the client to provide a desktop ID for security
> > + // reasons, which should identify the application requesting the location.
> > + geoclue_client_set_desktop_id(provider->m_clientProxy.get(), g_get_prgname());
>
> I still don't know whether using the program name is the right thing in the end or a workaround.
> If it's a workaround, please file a bug report and add a FIXME here pointing to the bug report.
> Because at the moment your comment is confusing here, it says geoclue requires a desktop ID,
> but we are providing an application name

In theory we should provide the "application name" as the desktop ID for geoclue to be able to handle, where "application name" is typically the name of the .desktop file (if any), or what you get from calling g_application_get_application_id().

The problem is that knowing that information from this point seems to be tricky. We couldgo the route of finding the top level GtkWindow and extracting the application ID from there, but then you would be introducing a dependency on GTK here, which I'm not sure that's what we want as, for instance, this provider might be used from other platforms too (e.g EFL).

Other option could be to provide specific API (not now) for the application to specify an "application ID" that webkit might use for different purposes, if present, such as to pass it through geoclue, or even to let the application willing to use WebKit with geolocation capabilities to talk directly to Geoclue to let it know the application ID.

Unfortunately, it is not clear to me which one would be the best option, but introducing the dependency on GTK seems to be the weirdest one, so in the end I preferred to go with g_get_prgname() for now because (1) in many cases it will provide the same string than the GNOME application name and (2) because it should be enough, at least for now, for geoclue to decide whether to whitelist some trusted apps known to use WebKitGTK+, such as epiphany.

So, in a nutshell, whether we should file a bug or not is still not clear to me, but sure I can at list put a FIXME in the code to reflect this situation, so we don't forget about it and file a bug once we agree on whether it's needed or not.

I'll update the patch now and upload a new version soon. Thanks for the review

Created attachment 226115
Patch proposal

Removed the include and the unused functions

(In reply to comment #60)
> > I still don't know whether using the program name is the right thing in the end or a workaround.
> > If it's a workaround, please file a bug report and add a FIXME here pointing to the bug report.
> > Because at the moment your comment is confusing here, it says geoclue requires a desktop ID,
> > but we are providing an application name
>
> In theory we should provide the "application name" as the desktop ID for geoclue to be able to handle, where "application name" is typically the name of the .desktop file (if any), or what you get from calling g_application_get_application_id().

typically

> The problem is that knowing that information from this point seems to be tricky. We couldgo the route of finding the top level GtkWindow and extracting the application ID from there, but then you would be introducing a dependency on GTK here, which I'm not sure that's what we want as, for instance, this provider might be used from other platforms too (e.g EFL).

Why would that introduce any dependency? We could have a method in WebCore platform to get the application name that GTK+ and EFL can implement. But in any case, I'm not concerned about the actual solution at the moment, I'm just asking whether this is a workaround to have a bug filed and a FIXME comment here so that we don't forget about this.

(In reply to comment #62)
> [...]
> > The problem is that knowing that information from this point seems to be tricky. We couldgo the route of finding the top level GtkWindow and extracting the application ID from there, but then you would be introducing a dependency on GTK here, which I'm not sure that's what we want as, for instance, this provider might be used from other platforms too (e.g EFL).
>
> Why would that introduce any dependency? We could have a method in WebCore platform to get the application name that GTK+ and EFL can implement.

Yes, we could do that. That's right.

> But in any case, I'm not concerned about the actual solution at the moment, I'm just asking whether this is a workaround to have a bug filed and a FIXME comment here so that we don't forget about this.

As I said before, I can not really answer this question because I'm not 100% sure whether that proposal of using g_application_get_application_id() for the desktop ID would be good enough for Geoclue. I think we need Zeeshan input once again at this point to clarify that. If he confirms that would be Ok, I'd be happy to file a bug and put the link in the FIXME here.

(In reply to comment #63)
> (In reply to comment #62)
> > [...]
> > > The problem is that knowing that information from this point seems to be tricky. We couldgo the route of finding the top level GtkWindow and extracting the application ID from there, but then you would be introducing a dependency on GTK here, which I'm not sure that's what we want as, for instance, this provider might be used from other platforms too (e.g EFL).
> >
> > Why would that introduce any dependency? We could have a method in WebCore platform to get the application name that GTK+ and EFL can implement.
>
> Yes, we could do that. That's right.
>
> > But in any case, I'm not concerned about the actual solution at the moment, I'm just asking whether this is a workaround to have a bug filed and a FIXME comment here so that we don't forget about this.
>
> As I said before, I can not really answer this question because I'm not 100% sure whether that proposal of using g_application_get_application_id() for the desktop ID would be good enough for Geoclue. I think we need Zeeshan input once again at this point to clarify that. If he confirms that would be Ok, I'd be happy to file a bug and put the link in the FIXME here.

So here is the thing: Geoclue will expect you (the app) to provide the desktop ID: i-e the name of your desktop file without the '.desktop' extension. Now both get_prgname() and g_application_get_application_id() are fine as long as you get the same value from them as the desktop ID from all your apps.

Having said that, I think g_application_get_application_id() is the better choice here. AFAIK, that is what most modern glib apps set correctly and in fact set this to same value as their desktop file name. So instead of having to add new webkit api, you can simply document that apps should ensure that they set this and that it should be the same as their desktop file. AFAIK existing apps wont' even have to change anything, right?

(In reply to comment #64)
> (In reply to comment #63)
> > (In reply to comment #62)
> > > [...]
> > > > The problem is that knowing that information from this point seems to be tricky. We couldgo the route of finding the top level GtkWindow and extracting the application ID from there, but then you would be introducing a dependency on GTK here, which I'm not sure that's what we want as, for instance, this provider might be used from other platforms too (e.g EFL).
> > >
> > > Why would that introduce any dependency? We could have a method in WebCore platform to get the application name that GTK+ and EFL can implement.
> >
> > Yes, we could do that. That's right.
> >
> > > But in any case, I'm not concerned about the actual solution at the moment, I'm just asking whether this is a workaround to have a bug filed and a FIXME comment here so that we don't forget about this.
> >
> > As I said before, I can not really answer this question because I'm not 100% sure whether that proposal of using g_application_get_application_id() for the desktop ID would be good enough for Geoclue. I think we need Zeeshan input once again at this point to clarify that. If he confirms that would be Ok, I'd be happy to file a bug and put the link in the FIXME here.
>
> So here is the thing: Geoclue will expect you (the app)

This is the main problem we are not an app.

> to provide the desktop ID: i-e the name of your desktop file without the '.desktop' extension. Now both get_prgname() and g_application_get_application_id() are fine as long as you get the same value from them as the desktop ID from all your apps.

So, it's indeed a workaround.

> Having said that, I think g_application_get_application_id() is the better choice here. AFAIK, that is what most modern glib apps set correctly and in fact set this to same value as their desktop file name. So instead of having to add new webkit api, you can simply document that apps should ensure that they set this and that it should be the same as their desktop file. AFAIK existing apps wont' even have to change anything, right?

We can't expect all apps using webkit being a GApplication. We could use the program name as fallback when the toplevel window is not a GtkApplicationWindow, of course, but this still sounds to me like a workaround.

Created attachment 226123
Patch proposal

New patch adding the newly reported bug together with the FIXME

(In reply to comment #65)
> (In reply to comment #64)
> > (In reply to comment #63)
> > > (In reply to comment #62)
> > > > [...]
> > > > > The problem is that knowing that information from this point seems to be tricky. We couldgo the route of finding the top level GtkWindow and extracting the application ID from there, but then you would be introducing a dependency on GTK here, which I'm not sure that's what we want as, for instance, this provider might be used from other platforms too (e.g EFL).
> > > >
> > > > Why would that introduce any dependency? We could have a method in WebCore platform to get the application name that GTK+ and EFL can implement.
> > >
> > > Yes, we could do that. That's right.
> > >
> > > > But in any case, I'm not concerned about the actual solution at the moment, I'm just asking whether this is a workaround to have a bug filed and a FIXME comment here so that we don't forget about this.
> > >
> > > As I said before, I can not really answer this question because I'm not 100% sure whether that proposal of using g_application_get_application_id() for the desktop ID would be good enough for Geoclue. I think we need Zeeshan input once again at this point to clarify that. If he confirms that would be Ok, I'd be happy to file a bug and put the link in the FIXME here.
> >
> > So here is the thing: Geoclue will expect you (the app)
>
> This is the main problem we are not an app.

I know that. :)

> > to provide the desktop ID: i-e the name of your desktop file without the '.desktop' extension. Now both get_prgname() and g_application_get_application_id() are fine as long as you get the same value from them as the desktop ID from all your apps.
>
> So, it's indeed a workaround.

If you don't mandate your apps to set_prgname() or g_application_set_application_id() to desktop id, indeed it is.

> > Having said that, I think g_application_get_application_id() is the better choice here. AFAIK, that is what most modern glib apps set correctly and in fact set this to same value as their desktop file name. So instead of having to add new webkit api, you can simply document that apps should ensure that they set this and that it should be the same as their desktop file. AFAIK existing apps wont' even have to change anything, right?
>
> We can't expect all apps using webkit being a GApplication. We could use the program name as fallback when the toplevel window is not a GtkApplicationWindow, of course, but this still sounds to me like a workaround.

You can give them a choice, if they don't use GApplication, they just have to use g_set_prgname() instead. I don't think that is too much to ask.

Created attachment 226260
Patch proposal

New patch fixing an issue in FindGeoClue2.cmake

Comment on attachment 226260
Patch proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=226260&action=review

> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue1.cpp:79
> + , m_geoclueClient(0)
> + , m_geocluePosition(0)

Remove these ones here too, or use nullptr.

Created attachment 226296
Patch proposal

New patch addressing the last comments from Carlos

Comment on attachment 226296
Patch proposal

Again

Comment on attachment 226296
Patch proposal

Thanks for the review

Comment on attachment 226296
Patch proposal

Clearing flags on attachment: 226296

Committed r165418: <http://trac.webkit.org/changeset/165418>

All reviewed patches have been landed. Closing bug.

Zeeshan says this patch to use geoclue1 is obsolete. We should integrate geoclue2 (in a new bug).

+++ This bug was initially created as a clone of Bug #485472 +++

Geoclue2 is a D-Bus service that provides geolocation, and uses MLS for network location:
https://developer.gnome.org/platform-overview/stable/tech-geoclue2.html.en

As D-Bus service, the provider can be queried at runtime, and if not present, fallback to the default provider.

Geoclue2 bug 1063572

Seems like this is fixed in GeoClue2 codebase. I see that it is using libmm library, and see the use of MM_MODEM_LOCATION_SOURCE_3GPP_LAC_CI in "src/gclue-modem-source.c" as pointed to in comment #1.

(In reply to marius from comment #5)
> Seems like this is fixed in GeoClue2 codebase. I see that it is using libmm
> library, and see the use of MM_MODEM_LOCATION_SOURCE_3GPP_LAC_CI in
> "src/gclue-modem-source.c" as pointed to in comment #1.

That is correct.

FYI, it's now easier to interface with Geoclue2 if you don't mind a dep on glib/gio:

http://zee-nix.blogspot.co.uk/2015/10/geoclue-convenience-library-just-got.html
http://www.freedesktop.org/software/geoclue/docs/libgeoclue/GClueSimple.html

Marking inactive due to the length of inactivity here, feel free to re-open if you wish this to remain active.

Doesn't look like I have permission to re-open, but I'd love to see this or some alternatively to allow integrating with any system geolocation methods I have set up, such as a Bluetooth GPS.

I do have a Geoclue2 geolocation provider for gecko-dev right now and have been using it in Firefox for a few weeks now.

This provider is purely (G)D-Bus based, so it doesn't need any additional external library as a dependency.
I envision it as a system-wide geolocation source, much like the ones that Firefox already uses on Windows, Mac OS or Android platforms.

Unfortunately, it's unlikely that I will be able to find enough time to go through a formal review process with this patch before this year winter holiday break.

Created attachment 9275622
Bug 1063572 - Geoclue2 geolocation provider. r=emilio

Add a Geoclue (version 2) geolocation provider.

This way Firefox can make use of multiple location sources present in the
system, from GNSS provided by a cellular modem or the current network to
location based on visible WiFi networks and 3G tower data, all while
sharing them with other applications.

This is a pure D-Bus-based implementation using a proper state machine, it
does not require any additional dependencies.

By the way, WebKitGTK got Geoclue2 support already.

How does this compare to the portal service that was recently added for Flatpak and so on? Do we really need both?

(In reply to Emilio Cobos Álvarez (:emilio) from comment #7)
> How does this compare to the portal service that was recently added for Flatpak and so on? Do we really need both?

Portal service is a limited API for Flatpak containers that internally forwards to Geoclue. Without Flatpak runtime that API is not available.

In contrast to the portal provider this is a complete, native Geoclue API implementation on a state machine (for handling corner cases, etc.) for use in a standalone Firefox.

Pushed by <email address hidden>:
https://hg.mozilla.org/integration/autoland/rev/39b20dac431b
Geoclue2 geolocation provider. r=emilio

Backed out for causing bustages at GeoclueLocationProvider.

Backout link: https://hg.mozilla.org/integration/autoland/rev/7f6113218f5598eafb379c8d0968fd3976c746f9

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=R_GVMys9T1OFOzXxE1ci_g.0&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=39b20dac431b40d84718ec2080563084506df19d

Failure log: https://treeherder.mozilla.org/logviewer?job_id=378119283&repo=autoland&lineNumber=178368

Sylvestre, do you know why reviewbot didn't complain about the static analysis issue in the phab revision? I'm pretty sure it used to do that in the past.

Marco and Andi should know better now

Pushed by <email address hidden>:
https://hg.mozilla.org/integration/autoland/rev/7357f3be9f82
Geoclue2 geolocation provider. r=emilio

https://hg.mozilla.org/mozilla-central/rev/7357f3be9f82

There is an intermittent issue with code-review events, and it doesn't notify Phabricator, we are tracking this in github.