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

[Marius B. Kotsbak]

It would be nice if location requested by websites could use location found from GPS or GSM/CDMA positioning.

[Bastien Nocera (hadess-deactivatedaccount)]

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

[Bastien Nocera (hadess-deactivatedaccount)]

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

Comments welcome

[Bastien Nocera (hadess-deactivatedaccount)]

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)

[Blizzard (blizzard)]

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

[Bastien Nocera (hadess-deactivatedaccount)]

(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.

[Bastien Nocera (hadess-deactivatedaccount)]

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 :)

[Doug-turner (doug-turner)]

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?

[Bastien Nocera (hadess-deactivatedaccount)]

(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.

[Bastien Nocera (hadess-deactivatedaccount)]

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.

[Bastien Nocera (hadess-deactivatedaccount)]

Could I have a review on the existing code please?

[Doug-turner (doug-turner)]

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.

[Doug-turner (doug-turner)]

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?

[Bastien Nocera (hadess-deactivatedaccount)]

(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.

[Doug-turner (doug-turner)]

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.

[Neil-uy4g6 (neil-uy4g6)]

(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.

[Doug-turner (doug-turner)]

dumb question -- does geoclue provide info to gpsd?

[Craig (candrews-integralblue)]

(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.

[Sonny Piers (sonnyp)]

(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. "

[Ross Burton (ross)]

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

[Bastien Nocera (hadess-deactivatedaccount)]

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.

[Aleksander Morgado (aleksander-m)]

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.

[Aleksander Morgado (aleksander-m)]

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

[Marius B. Kotsbak (mariusko)]

This indicates that Chromium might have it implemented upstream: http://git.chromium.org/gitweb/?p=external/WebKit_trimmed.git;a=commitdiff;h=7cae51db4cb9e27878e1116b78c637d651f10fa0

Maybe it must be enabled when building it.

[Bastien Nocera (hadess-deactivatedaccount)]

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.

[Marius B. Kotsbak (mariusko)]

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

[Bastien Nocera (hadess-deactivatedaccount)]

(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.

[Zeeshan Ali (zeeshanak)]

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.

[Zan-f (zan-f)]

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/

[Zeeshan Ali (zeeshanak)]

(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/

[Bugzilla-x (bugzilla-x)]

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.

[Marius B. Kotsbak (mariusko)]

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

[Zeeshan Ali (zeenix)]

(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.

[Zeeshan Ali (zeeshanak)]

(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. :)

[Ggoncalves (ggoncalves)]

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.

[Zeeshan Ali (zeenix)]

(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.

[Buildbot-3 (buildbot-3)]

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

[Buildbot-3 (buildbot-3)]

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

[Buildbot-3 (buildbot-3)]

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

[I-mario (i-mario)]

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.

[Cgarcia-f (cgarcia-f)]

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

[I-mario (i-mario)]

(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

[I-mario (i-mario)]

Created attachment 226115
Patch proposal

Removed the include and the unused functions

[Cgarcia-f (cgarcia-f)]

(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.

[I-mario (i-mario)]

(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.

[Zeeshan Ali (zeeshanak)]

(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?

[Cgarcia-f (cgarcia-f)]

(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.

[I-mario (i-mario)]

Created attachment 226123
Patch proposal

New patch adding the newly reported bug together with the FIXME

[Zeeshan Ali (zeeshanak)]

(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.

[I-mario (i-mario)]

Created attachment 226260
Patch proposal

New patch fixing an issue in FindGeoClue2.cmake

[Cgarcia-f (cgarcia-f)]

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.

[I-mario (i-mario)]

Created attachment 226296
Patch proposal

New patch addressing the last comments from Carlos

[Cgarcia-f (cgarcia-f)]

Comment on attachment 226296
Patch proposal

Again

[I-mario (i-mario)]

Comment on attachment 226296
Patch proposal

Thanks for the review

[Commit-queue (commit-queue)]

Comment on attachment 226296
Patch proposal

Clearing flags on attachment: 226296

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

[Commit-queue (commit-queue)]

All reviewed patches have been landed. Closing bug.

[Cpeterson-3 (cpeterson-3)]

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

[Gkeeley (gkeeley)]

+++ 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.

[Cpeterson-3 (cpeterson-3)]

Geoclue2 bug 1063572

[Marius B. Kotsbak (mariusko)]

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.

[Marius B. Kotsbak (mariusko)]

Webkit has got support for Geoclue2 (as geoclue1 won't get MM1 support).

[Marius B. Kotsbak (mariusko)]

geoclue-provider should be set to won't fix because upstream is not going to fix it after geoclue2 is available.

[Zeeshan Ali (zeenix)]

(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.

[Zeeshan Ali (zeeshanak)]

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

[Olivier Tilloy (osomon)]

The default location provider in chromium-browser is network-based, it doesn’t look like it knows how to talk to GPS hardware.

[Gkeeley (gkeeley)]

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

[Launchpad Janitor (janitor)]

Status changed to 'Confirmed' because the bug affects multiple users.

[Erikjensen-l (erikjensen-l)]

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.

[Maciej S. Szmigiero (maciejsszmigiero)]

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.

[Maciej S. Szmigiero (maciejsszmigiero)]

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.

[Maciej S. Szmigiero (maciejsszmigiero)]

By the way, WebKitGTK got Geoclue2 support already.

[Emilio (emiliocobos)]

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

[Maciej S. Szmigiero (maciejsszmigiero)]

(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.

[Pulsebot (pulsebot)]

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

[Abutkovits (abutkovits)]

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

[Emilio (emiliocobos)]

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.

[Sledru (sledru)]

Marco and Andi should know better now

[Pulsebot (pulsebot)]

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

[Csabou (csabou)]

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

[Bpostelnicu (bpostelnicu)]

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