Closed Bug 485472 Opened 15 years ago Closed 10 years ago

Use Geoclue1 Location provider

Categories

(Core :: DOM: Geolocation, enhancement)

Product:
Core
Component:
DOM: Geolocation
Platform:
All
Linux
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: bugzilla, Unassigned)

References

Details

Attachments

(1 file, 2 obsolete files)

mozilla-geoclue-for-location-3.patch
15.37 KB, patch
dougt
: review-
Details | Diff | Splinter Review 
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
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.
Flags: wanted1.9.1?
(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.
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 :)
Attachment #369611 - Attachment is obsolete: true

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?
Status: UNCONFIRMED → NEW
Ever confirmed: true
(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.
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.
Attachment #369792 - Attachment is obsolete: true
Could I have a review on the existing code please?
Comment on attachment 370177 [details] [diff] [review]
mozilla-geoclue-for-location-3.patch

needs to be marked for review.  I will try to get to it this week.
Attachment #370177 - Flags: review?(doug.turner)
Comment on attachment 370177 [details] [diff] [review]
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?
Attachment #370177 - Flags: review?(doug.turner) → review-
(In reply to comment #11)
> (From update of attachment 370177 [details] [diff] [review])
> 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.
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 added the following comment to Launchpad bug report 1100326:

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

-- 
http://launchpad.net/bugs/1100326
(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.
(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. :)
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.
Flags: wanted1.9.1?
Hardware: x86 → All
Zeeshan says this patch to use geoclue1 is obsolete. We should integrate geoclue2 (in a new bug).
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → WONTFIX
Summary: Geoclue Location provider → Use Geoclue1 Location provider
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: