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

Bug #1100326 reported by Marius B. Kotsbak on 2013-01-16
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Chromium Browser
Fix Committed
Undecided
Unassigned
GeoClue
Won't Fix
Medium
Mozilla Firefox
Confirmed
Wishlist
WebKit
Fix Released
Medium
chromium-browser (Ubuntu)
Undecided
Unassigned
firefox (Ubuntu)
Undecided
Unassigned
geoclue-2.0 (Ubuntu)
Undecided
Unassigned
geoclue-providers (Ubuntu)
Undecided
Unassigned

Bug Description

It would be nice if location requested by websites could use location found from GPS or GSM/CDMA 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.

affects: geoclue (Ubuntu) → geoclue-providers (Ubuntu)
21 comments hidden view all 115 comments
Marius B. Kotsbak (mariusko) wrote :

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.

Changed in chromium-browser:
status: New → Fix Committed
Changed in firefox:
importance: Unknown → Wishlist
status: Unknown → Confirmed
Changed in geoclue:
importance: Unknown → Medium
status: Unknown → Confirmed
22 comments hidden view all 115 comments

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.

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

11 comments hidden view all 115 comments

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/

12 comments hidden view all 115 comments

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.

Changed in geoclue:
status: Confirmed → Won't Fix

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.

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

45 comments hidden view all 115 comments

(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

44 comments hidden view all 115 comments

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.

44 comments hidden view all 115 comments

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

Download full text (4.5 KiB)

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

Read more...

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.

78 comments hidden view all 115 comments

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

1 comments hidden view all 115 comments

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

Changed in firefox:
status: Confirmed → Won't Fix
Changed in firefox:
importance: Wishlist → Unknown
status: Won't Fix → Unknown
Changed in firefox:
importance: Unknown → Wishlist
status: Unknown → Confirmed
78 comments hidden view all 115 comments

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.

Changed in geoclue-2.0 (Ubuntu):
status: New → Fix Released
Changed in chromium-browser (Ubuntu):
status: New → Fix Released
76 comments hidden view all 115 comments
Marius B. Kotsbak (mariusko) wrote :

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

Changed in chromium-browser (Ubuntu):
status: Fix Released → New
Changed in geoclue-providers (Ubuntu):
status: New → Invalid
Marius B. Kotsbak (mariusko) wrote :

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

Changed in webkit-open-source:
importance: Unknown → Medium
status: Unknown → Fix Released
76 comments hidden view all 115 comments

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

Olivier Tilloy (osomon) wrote :

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

Changed in firefox:
status: Confirmed → Unknown
Changed in firefox:
status: Unknown → Confirmed
Displaying first 40 and last 40 comments. View all 115 comments or add a comment.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.