1250922 - Add gpsd location provider

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Thomas Zimmermann [:tzimmermann] [:tdz]]

The program gpsd provides support for a number of GPS receivers on Linux. The position information are more precise than with MLS.

Gpsd support was originally added in bug 492328 and later removed in bug 866893. We should re-add optional support for gpsd.

Comment 1

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 1250922: Add gpsd geolocation provider on Linux

Comment 2

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 1250922: Use gpsd for Geolocation

Comment 3

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 1250922: Add MLS as fallback for gpsd

Comment 4

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Doug,

This is the gpsd patch set; hopefully as discussed via email. I own a Pharos GPS 500 receiver, which works nicely with gpsd. The returned position is correct, even though I'm indoors. MLS's results are off by ~50 meters.

Patches [01] and [02] implement for gpsd. It's only used with GTK+. QT already seems to come with it's own Geolocation support.

Patch [03] adds MLS support for cases where gpsd is not supported or slow to return a valid location. I tested various corner cases, like 'gpsd disabled,' 'no receiver attached,' or 'no GPS fixes yet.' In all cases MLS takes over after a few seconds. Once there's a valid GPS position, it's used instead of MLS.

One thing I don't understand is why all providers implement their own MLS fallback. It seems like this should be handled automatically by |nsGeolocationService|.

Comment 5

[Hanno Schlichting]

I spoke with the QT geolocation folks last year at a hackathon. IIRC QT includes a geolocation provider based on the geoclue 1 library. There's a total API-incompatible rewrite of that library geoclue 2 and they were looking at adopting the newer library instead. I don't know if that work happened.

There's also bug https://bugzilla.mozilla.org/show_bug.cgi?id=1063572 which tracks a potential implementation of a Firefox provider using said geoclue 2 library. But so far nobody wrote the code to make that happen.

Comment 6

[Thomas Zimmermann [:tzimmermann] [:tdz]]

In preparation of this patch set, I looked at the GeoClue source code. They also use MLS, and they don't support gpsd. (Apparently because the maintainer doesn't like it.) I'm curious what the benefits of a GeoClue-based provider would be.

Comment 7

[Hanno Schlichting]

@Thomas: I think working code wins trumps all. For Linux, the real question is what distributions ship which what libraries installed by default. Or how many users actually have one library vs. the other (or all the dependencies for it).

I haven't actually looked at that. My impression was that geoclue 2 might ship with Gnome by default, and that would potentially lead to a larger install base. But I could be totally wrong on this. My information is from hearsay, as I'm not a Linux user myself.

Comment 8

[Hanno Schlichting]

Comment on attachment 8723049 [details] [diff] [review]
[02] Bug 1250922: Use gpsd for Geolocation

Review of attachment 8723049 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/geolocation/moz.build
@@ +57,1 @@
>  

This looks like you accidentally removed the "elif ... == 'windows'" block.

Comment 9

[Hanno Schlichting]

Comment on attachment 8723048 [details] [diff] [review]
[01] Bug 1250922: Add gpsd geolocation provider on Linux

Review of attachment 8723048 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/system/linux/GpsdLocationProvider.cpp
@@ +300,5 @@
> +          continue; // There's no useful data in this fix; continue.
> +      }
> +
> +      timestamp = gpsData.fix.time * 1000; // convert to milliseconds
> +

This looks like it is using the GPS time and forwards it, so it is exposed as the DOM timestamp.

Unfortunately GPS time is not in UTC, but a different special timezone currently 17 seconds behind UTC (it's not adjusted for leap seconds). The same comment as in https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/GonkGPSGeolocationProvider.cpp#142 applies. We shouldn't forward GPS time to the DOM.

Comment 10

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Thanks Hanno for the feedback. I'll update the patches accordingly.

Comment 11

[Thomas Zimmermann [:tzimmermann] [:tdz]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc91d8fd73ca

Comment 12

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 1250922: Add gpsd geolocation provider on Linux (v2)

Changes since v1:

  - disable gpsd by default in config
  - compute timestamp from PR_Now()

Comment 13

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 1250922: Use gpsd for Geolocation (v2)

Changes since v1:

  - restored Windows branch in moz.build

Comment 14

[Thomas Zimmermann [:tzimmermann] [:tdz]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=65f7e4b284e4

Comment 15

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 1250922: Add gpsd geolocation provider on Linux (v3)

Changes since v2:

  - updated build scripts for latest m-c

Comment 16

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 1250922: Use gpsd for Geolocation (v3)

Changes since v2:

  - fixed typo in comment

Comment 17

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 1250922: Add MLS as fallback for gpsd (v2)

Changes since v2:

  - updated Telemetry for latest m-c
  - deleted removed method |MLSGeolocationUpdate::LocationUpdatePending|

Comment 18

[Thomas Zimmermann [:tzimmermann] [:tdz]]

I tested these patches on Debian 8 with a 'Pharos GPS-500' adapter.

Comment 19

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8745317 [details] [diff] [review]
[03] Bug 1250922: Add MLS as fallback for gpsd (v2)

Telemetry probes require an email address to inform when results strongly vary between versions. I put in my own address for now, but is there anyone in the Geolocation team who would take care of this?

Comment 20

[Josh Matthews [:jdm]]

Comment on attachment 8745314 [details] [diff] [review]
[01] Bug 1250922: Add gpsd geolocation provider on Linux (v3)

Review of attachment 8745314 [details] [diff] [review]:
-----------------------------------------------------------------

A build system peer should sign off on the changes here too. I'd like to see another version that uses nsMainThreadPtrHolder/Handle before this merges.

::: dom/system/linux/GpsdLocationProvider.cpp
@@ +20,5 @@
> +//
> +
> +/**
> + * |GpsdLocationProviderWrapper| implements thread-safe reference
> + * counting around |GpsdLocationProvider|.

This doesn't seem particularly thread safe if the last reference is removed on a different thread than it was created. Can we use nsMainThreadPtrHolder and nsMainThreadPtrHandle instead?

@@ +68,5 @@
> +  NS_IMETHOD Run() override
> +  {
> +    auto locationProvider = mLocationProviderWrapper->Get();
> +
> +    if (locationProvider) {

How could this be null?

@@ +102,5 @@
> +  NS_IMETHOD Run() override
> +  {
> +    auto locationProvider = mLocationProviderWrapper->Get();
> +
> +    if (locationProvider) {

How can this be null?

@@ +123,5 @@
> + * |ReleaseRunnable| releases a location-provider wrapper on the
> + * executing thread. This will release a reference to the location
> + * provider as well.
> + */
> +class GpsdLocationProvider::ReleaseRunnable final : public nsRunnable

No need for this class if we use nsMainThreadPtrHandle.

@@ +264,5 @@
> +      }
> +
> +      switch (gpsData.fix.mode) {
> +        case MODE_3D:
> +          if (!std::isnan(gpsData.fix.altitude)) {

Let's use mozilla::IsNan instead for these std::isnan uses.

@@ +267,5 @@
> +        case MODE_3D:
> +          if (!std::isnan(gpsData.fix.altitude)) {
> +            alt = gpsData.fix.altitude;
> +          }
> +          /* fall through */

MOZ_FALLTHROUGH;

@@ +380,5 @@
> +NS_IMETHODIMP
> +GpsdLocationProvider::Startup()
> +{
> +  if (mPollRunnable) {
> +    return NS_OK; // already running; should this be an error?

Not an error.

::: dom/system/linux/GpsdLocationProvider.h
@@ +6,5 @@
> +
> +#ifndef GpsdLocationProvider_h
> +#define GpsdLocationProvider_h
> +
> +#include "mozilla/LazyIdleThread.h"

This can be a forward declaration.

::: dom/system/linux/moz.build
@@ +16,5 @@
> +    LOCAL_INCLUDES += [
> +        '/dom/geolocation'
> +    ]
> +
> +include('/ipc/chromium/chromium-config.mozbuild')

Is this necessary? I thought it was only for libs that need IPDL.

Comment 21

[Josh Matthews [:jdm]]

Comment on attachment 8745315 [details] [diff] [review]
[02] Bug 1250922: Use gpsd for Geolocation (v3)

Review of attachment 8745315 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/geolocation/moz.build
@@ +56,5 @@
> +        LOCAL_INCLUDES += [
> +            '/dom/system/linux',
> +        ]
> +        CXXFLAGS += CONFIG['MOZ_GPSD_CFLAGS']
> +        DEFINES['MOZ_GPSD'] = True

I'm not sure what the presence of this line in both moz.build files entails. Maybe a build peer should look at this patch too.

Comment 22

[Josh Matthews [:jdm]]

Comment on attachment 8745317 [details] [diff] [review]
[03] Bug 1250922: Add MLS as fallback for gpsd (v2)

Review of attachment 8745317 [details] [diff] [review]:
-----------------------------------------------------------------

On a technical level this series of patches makes sense. However, if this isn't a default provider and (I think?) it's not even compiled by default, who do we expect to use it? Why is it important that we collect data about people using it? The histogram will be empty for the vast majority of our users since the codepath to collect the histogram won't be run, right?

::: dom/system/linux/GpsdLocationProvider.cpp
@@ +6,4 @@
>  
>  #include "GpsdLocationProvider.h"
>  #include <cmath>
> +#include <errno.h>

This looks like it belongs in an earlier patch.

@@ +64,5 @@
> +  if (!coords) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  Telemetry::Accumulate(Telemetry::GEOLOCATION_LINUX_SOURCE_IS_MLS, true);

We never accumulate false values for this flag.

::: toolkit/components/telemetry/Histograms.json
@@ +526,5 @@
>      "description" : "Number of navigator.geolocation.watchPosition() calls (0=other, 1=http, 2=https)"
>    },
> +  "GEOLOCATION_LINUX_SOURCE_IS_MLS": {
> +    "alert_emails": ["[e-mail]"],
> +    "expires_in_version": "default",

I believe we strongly encourage choosing explicit versions rather than default.

Comment 23

[Thomas Zimmermann [:tzimmermann] [:tdz]]

> On a technical level this series of patches makes sense. However, if this
> isn't a default provider and (I think?) it's not even compiled by default,
> who do we expect to use it? Why is it important that we collect data about
> people using it? The histogram will be empty for the vast majority of our
> users since the codepath to collect the histogram won't be run, right?

Do you think it would make sense to build and enable gpsd by default if supported? This would be great. MLS will take over after after a few seconds, so technically it shouldn't be a problem. I was just being conservative when writing the patch set.

Comment 24

[Hanno Schlichting]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #23)
> Do you think it would make sense to build and enable gpsd by default if
> supported? This would be great. MLS will take over after after a few
> seconds, so technically it shouldn't be a problem. I was just being
> conservative when writing the patch set.

Unfortunately we cannot enable any non-Google location providers by default on release builds. Our geolocation contract with Google forbids us from using any other location source by default. Doug is talking with various people to figure out if we could change that, but that's a broader business/legal question.

What we could do is the same we do with other providers like OS X Core Location, and build them by default but disable them by default via a preference. If a user than wants to change their location preferences, they can do so without having to build their own Firefox.

The other thing we can do is to try and enable this by default on non-release builds to get some testing done. That's what we do with MLS and Core Location, which are used by default on desktop nightly and dev edition, but disabled on beta + release.

Comment 25

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Hanno Schlichting [:hannosch] from comment #24)
> (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #23)
> > Do you think it would make sense to build and enable gpsd by default if
> > supported? This would be great. MLS will take over after after a few
> > seconds, so technically it shouldn't be a problem. I was just being
> > conservative when writing the patch set.
> 
> Unfortunately we cannot enable any non-Google location providers by default
> on release builds. Our geolocation contract with Google forbids us from
> using any other location source by default. Doug is talking with various
> people to figure out if we could change that, but that's a broader
> business/legal question.

OMG :(

> 
> What we could do is the same we do with other providers like OS X Core
> Location, and build them by default but disable them by default via a
> preference. If a user than wants to change their location preferences, they
> can do so without having to build their own Firefox.
> 
> The other thing we can do is to try and enable this by default on
> non-release builds to get some testing done. That's what we do with MLS and
> Core Location, which are used by default on desktop nightly and dev edition,
> but disabled on beta + release.

Thanks Hanno for this information. If there's no objection from Josh, I'll try implement this for gpsd as well.

Comment 26

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 1250922: Add gpsd geolocation provider on Linux (v4)

Changes since v3:

  - build gpsd support by default if installed
  - replaced |GpsdLocationProviderWrapper| with |nsMainThreadPtrHandle|
  - replaced |std::isnan| with |mozilla::IsNaN|
  - added MOT_FALLTHROUGH
  - forward-declare |LazyIdleThread| in header
  - include 'errno.h' (from patch [03])
  - removed 'chromium-config.mozbuild' from moz.build
  - minor cleanups

Adding :glandium for review of build-script changes.

Comment 27

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 1250922: Use gpsd for Geolocation (v4)

Changes since v3:

  - enable gpsd by default on non-release builds

Comment 28

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 1250922: Add MLS as fallback for gpsd (v3)

Changes since v2:

  - moved Telemetry support into new patch [04]

Comment 29

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [04] Bug 1250922: Support Telemetry for gpsd location provider

Changes since patch [03](v2):

  - set 'GEOLOCATION_LINUX_SOURCE_IS_MLS' to 'false' when updating the location from gpsd
  - set 'expires_in_version' to 'never'

Comment 30

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8746549 [details] [diff] [review]
[04] Bug 1250922: Support Telemetry for gpsd location provider

Review of attachment 8746549 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/Histograms.json
@@ +529,5 @@
> +    "alert_emails": ["[e-mail]"],
> +    "expires_in_version": "never",
> +    "kind": "boolean",
> +    "bug_numbers": [1250922],
> +    "description": "Geolocation on Linux is either MLS or native"

Is that something that can actually change during a Firefox session?
If this is just recorded once during a Firefox session, a "flag" histogram is a better fit-

Boolean histograms allow recording multiple boolean values, flag histograms only record one:
https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe#Choosing_a_Histogram_Type
Flag histograms will show up nicer on telemetry.mozilla.org too (as X% true vs. Y% false).

Note that you'll also need data collection review:
https://wiki.mozilla.org/Firefox/Data_Collection

Comment 31

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #30)
> Is that something that can actually change during a Firefox session?
> If this is just recorded once during a Firefox session, a "flag" histogram
> is a better fit-

It's not a one-time user choice, or something similar. The value measures whether a location is received from gpsd vs. the MLS-based fallback. This can change at any time. This is the same semantics as for other Geolocation back ends (e.g., Windows).

> Note that you'll also need data collection review:
> https://wiki.mozilla.org/Firefox/Data_Collection

Thanks!

Comment 32

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 1250922: Add gpsd geolocation provider on Linux (v5)

Changes since v4:

  - use '--enable-gpsd' for naming configure option

Comment 33

[Mike Hommey [:glandium]]

Comment on attachment 8747664 [details] [diff] [review]
[01] Bug 1250922: Add gpsd geolocation provider on Linux (v5)

Review of attachment 8747664 [details] [diff] [review]:
-----------------------------------------------------------------

::: old-configure.in
@@ +4457,5 @@
> +MOZ_ARG_ENABLE_BOOL(gpsd,
> +[  --enable-gpsd                Enable gpsd support],
> +   MOZ_GPSD=1,
> +   MOZ_GPSD=,
> +   MOZ_GPSD=1)

Do you really intend this to be enabled by default?

@@ +4461,5 @@
> +   MOZ_GPSD=1)
> +
> +dnl If using Linux, ensure that libgps is available
> +if test -n "$MOZ_GPSD"; then
> +    PKG_CHECK_MODULES(MOZ_GPSD, libgps >= $GPSD_VERSION,,)

We can't do PKG_CHECK_MODULES in python configure yet, but it would still be better to add the option itself to python configure, and have it set MOZ_GPSD for old-configure and keep this check here.

The option looks like it should go in toolkit/moz.configure, you'll find examples there.

::: toolkit/library/moz.build
@@ +235,5 @@
>  if CONFIG['MOZ_ALSA']:
>      OS_LIBS += CONFIG['MOZ_ALSA_LIBS']
>  
> +if CONFIG['MOZ_GPSD']:
> +    OS_LIBS += CONFIG['MOZ_GPSD_LIBS']

This shouldn't be needed (it should be inherited from the OS_LIBS you added in dom/system/linux/moz.build)

Comment 34

[Mike Hommey [:glandium]]

Comment on attachment 8746541 [details] [diff] [review]
[02] Bug 1250922: Use gpsd for Geolocation (v4)

Review of attachment 8746541 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/geolocation/moz.build
@@ +55,5 @@
> +    if CONFIG['MOZ_GPSD']:
> +        LOCAL_INCLUDES += [
> +            '/dom/system/linux',
> +        ]
> +        CXXFLAGS += CONFIG['MOZ_GPSD_CFLAGS']

AFAICT, including GpsdLocationProvider.h doesn't require this.

::: dom/geolocation/nsGeolocation.cpp
@@ +53,4 @@
>  #include "GonkGPSGeolocationProvider.h"
>  #endif
>  
> +#ifdef MOZ_GPSD

shouldn't there be a #ifdef MOZ_WIDGET_GTK here?

Comment 35

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8746549 [details] [diff] [review]
[04] Bug 1250922: Support Telemetry for gpsd location provider

Review of attachment 8746549 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine to me.

Comment 36

[Josh Matthews [:jdm]]

Comment on attachment 8746549 [details] [diff] [review]
[04] Bug 1250922: Support Telemetry for gpsd location provider

Review of attachment 8746549 [details] [diff] [review]:
-----------------------------------------------------------------

I still don't feel like I have a good answer for why we're collecting this data. The population for whom we'll actually collect data is vanishingly small.

::: toolkit/components/telemetry/Histograms.json
@@ +526,5 @@
>      "description" : "Number of navigator.geolocation.watchPosition() calls (0=other, 1=http, 2=https)"
>    },
> +  "GEOLOCATION_LINUX_SOURCE_IS_MLS": {
> +    "alert_emails": ["[e-mail]"],
> +    "expires_in_version": "never",

I believe never is even more discouraged than default... We really need to think about how long it would make sense to collect this data.

Comment 37

[Josh Matthews [:jdm]]

Comment on attachment 8746541 [details] [diff] [review]
[02] Bug 1250922: Use gpsd for Geolocation (v4)

Review of attachment 8746541 [details] [diff] [review]:
-----------------------------------------------------------------

The commit message seems to disagree with the changes.

Comment 38

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Josh Matthews [:jdm] from comment #36)
> 
> I still don't feel like I have a good answer for why we're collecting this
> data. The population for whom we'll actually collect data is vanishingly
> small.

Isn't this the same for the other platforms as well?

We can also leave Telemetry out for now. It should be easy to add it later.

Comment 39

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Mike Hommey [:glandium] from comment #33)
> Comment on attachment 8747664 [details] [diff] [review]
> [01] Bug 1250922: Add gpsd geolocation provider on Linux (v5)
> 
> Review of attachment 8747664 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: old-configure.in
> @@ +4457,5 @@
> > +MOZ_ARG_ENABLE_BOOL(gpsd,
> > +[  --enable-gpsd                Enable gpsd support],
> > +   MOZ_GPSD=1,
> > +   MOZ_GPSD=,
> > +   MOZ_GPSD=1)
> 
> Do you really intend this to be enabled by default?

Yes, after the above discussion. If libgpsd is not installed, the call to 'pkg-config' is supposed to disable it.

> @@ +4461,5 @@
> > +   MOZ_GPSD=1)
> > +
> > +dnl If using Linux, ensure that libgps is available
> > +if test -n "$MOZ_GPSD"; then
> > +    PKG_CHECK_MODULES(MOZ_GPSD, libgps >= $GPSD_VERSION,,)
> 
> We can't do PKG_CHECK_MODULES in python configure yet, but it would still be
> better to add the option itself to python configure, and have it set
> MOZ_GPSD for old-configure and keep this check here.
> 
> The option looks like it should go in toolkit/moz.configure, you'll find
> examples there.

OK

> ::: toolkit/library/moz.build
> @@ +235,5 @@
> >  if CONFIG['MOZ_ALSA']:
> >      OS_LIBS += CONFIG['MOZ_ALSA_LIBS']
> >  
> > +if CONFIG['MOZ_GPSD']:
> > +    OS_LIBS += CONFIG['MOZ_GPSD_LIBS']
> 
> This shouldn't be needed (it should be inherited from the OS_LIBS you added
> in dom/system/linux/moz.build)

OK

Comment 40

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Mike Hommey [:glandium] from comment #34)
> 
> ::: dom/geolocation/nsGeolocation.cpp
> @@ +53,4 @@
> >  #include "GonkGPSGeolocationProvider.h"
> >  #endif
> >  
> > +#ifdef MOZ_GPSD
> 
> shouldn't there be a #ifdef MOZ_WIDGET_GTK here?

I tried to avoid this, because gpsd and GTK+ are unrelated.

Comment 41

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 1250922: Use gpsd for Geolocation (v5)

Changes since v4:

  - don't modify CXXFLAGS in dom/geolocation/moz.build

Comment 42

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 1250922: Add gpsd geolocation provider on Linux (v6)

Changes since v5:

  - moved gpsd configure option to Python script
  - removed additional OS_LIBS from toolkit/library/moz.build

Comment 43

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 1250922: Use gpsd for Geolocation (v6)

Changes since v5:

  - fixed commit message

Comment 44

[Mike Hommey [:glandium]]

Comment on attachment 8751692 [details] [diff] [review]
[01] Bug 1250922: Add gpsd geolocation provider on Linux (v6)

Review of attachment 8751692 [details] [diff] [review]:
-----------------------------------------------------------------

::: old-configure.in
@@ +4455,5 @@
> +dnl ========================================================
> +
> +dnl If using Linux, ensure that libgps is available
> +if test -n "$MOZ_GPSD"; then
> +    PKG_CHECK_MODULES(MOZ_GPSD, libgps >= $GPSD_VERSION,,)

The timing kind of sucks. Would you mind rebasing on top of bug 1269513? Not going to block you on this, however:

> > Do you really intend this to be enabled by default?
>
> Yes, after the above discussion. If libgpsd is not installed, the call to 'pkg-config' is supposed to disable it.

We avoid these kinds of magic behavior. That's the best way to end up with something disabled that we expected to be enabled.

Comment 45

[Mike Hommey [:glandium]]

Comment on attachment 8751694 [details] [diff] [review]
[02] Bug 1250922: Use gpsd for Geolocation (v6)

Review of attachment 8751694 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/app/moz.build
@@ +74,5 @@
>  if CONFIG['GNU_CXX']:
>      CXXFLAGS += ['-Wshadow']
>  
> +if CONFIG['MOZ_GPSD']:
> +    DEFINES['MOZ_GPSD'] = True

You have a set_define in moz.configure, that's not needed. Or, come to think of it, you could remove the set_define in moz.configure, if you keep the defines here and in other moz.builds.

Comment 46

[Mike Hommey [:glandium]]

Comment on attachment 8751692 [details] [diff] [review]
[01] Bug 1250922: Add gpsd geolocation provider on Linux (v6)

Review of attachment 8751692 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/system/linux/moz.build
@@ +19,5 @@
> +
> +FINAL_LIBRARY = 'xul'
> +
> +if CONFIG['GNU_CXX']:
> +    CXXFLAGS += ['-Wshadow']

Note that bug 1272513 is going to make these 2 lines obsolete.

Comment 47

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Mike Hommey [:glandium] from comment #44)
> Comment on attachment 8751692 [details] [diff] [review]
> [01] Bug 1250922: Add gpsd geolocation provider on Linux (v6)
> 
> Review of attachment 8751692 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: old-configure.in
> @@ +4455,5 @@
> > +dnl ========================================================
> > +
> > +dnl If using Linux, ensure that libgps is available
> > +if test -n "$MOZ_GPSD"; then
> > +    PKG_CHECK_MODULES(MOZ_GPSD, libgps >= $GPSD_VERSION,,)
> 
> The timing kind of sucks. Would you mind rebasing on top of bug 1269513? Not
> going to block you on this, however:

Sure, I'll wait for this patch set to land.

Comment 48

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 1250922: Add gpsd geolocation provider on Linux (v7)

Changes since v6:

  - moved GPS configure tests to Python-based modules
  - removed '-Wshadow' parameter from moz.build

Comment 49

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 1250922: Use gpsd for Geolocation (v7)

Changes since v6:

  - rebased onto latest m-c

I kept the DEFINES statement in the moz.build file here, and removed the one on the configure scripts.

Comment 50

[Thomas Zimmermann [:tzimmermann] [:tdz]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e9ce5254bf92

Comment 51

[Mike Hommey [:glandium]]

Comment on attachment 8760244 [details] [diff] [review]
[01] Bug 1250922: Add gpsd geolocation provider on Linux (v7)

Review of attachment 8760244 [details] [diff] [review]:
-----------------------------------------------------------------

::: old-configure.in
@@ +71,5 @@
>  STARTUP_NOTIFICATION_VERSION=0.8
>  DBUS_VERSION=0.60
>  SQLITE_VERSION=3.13.0
>  FONTCONFIG_VERSION=2.7.0
> +GPSD_VERSION=3.11

no need for this here.

::: toolkit/moz.configure
@@ +424,5 @@
>          die('Cannot use MOZ_ANDROID_HISTORY alongside MOZ_PLACES.')
>  
> +# gpsd support
> +# ==============================================================
> +option('--disable-gpsd', env='MOZ_GPSD',

You want an opt-in, here, not an opt-out, because an opt-out means virtually everyone will have to add --disable-gpsd, including automation (starting with all windows and osx builds, and then everyone on linux that doesn't have gpsd).

Comment 52

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 1250922: Add gpsd geolocation provider on Linux (v8)

Changes since v7:

  - switch to opt-in for gpsd support
  - removed GPSD version number from old-configure.in 
  - fixed timeout parameter to use microseconds instead of milliseconds

Comment 53

[Mike Hommey [:glandium]]

Comment on attachment 8761517 [details] [diff] [review]
[01] Bug 1250922: Add gpsd geolocation provider on Linux (v8)

Review of attachment 8761517 [details] [diff] [review]:
-----------------------------------------------------------------

r+ for the build system parts, with the nit below.

::: toolkit/moz.configure
@@ +429,5 @@
> +       help='Enable gpsd support')
> +
> +@depends('--enable-gpsd')
> +def gpsd(value):
> +    if value:

Since this is only used for pkg_check_modules, you can simplify this as "return bool(value)"

Comment 54

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 1250922: Add gpsd geolocation provider on Linux (v9)

Changes since v8:

  - simplified |gpsd| helper function

Comment 55

[Thomas Zimmermann [:tzimmermann] [:tdz]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a910debf60f6

Comment 56

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/76b04196acce
Add gpsd geolocation provider on Linux, r=jdm,glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1a42e8d66e9
Use gpsd for Geolocation, r=jdm,glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/457be633b6cc
Add MLS as fallback for gpsd, r=jdm

Comment 57

[Thomas Zimmermann [:tzimmermann] [:tdz]]

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=457be633b6cc

Comment 58

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/76b04196acce
https://hg.mozilla.org/mozilla-central/rev/a1a42e8d66e9
https://hg.mozilla.org/mozilla-central/rev/457be633b6cc

Comment 59

[m.kurz]

Hi,

if I download the current Nightly (firefox-63.0a1.de.linux-x86_64.tar.bz2) from https://www.mozilla.org/de/firefox/channel/desktop/ is gpsd support compiled in that binary?
Or do I have to compile Firefox myself with the --enable-gpsd flag?

I do have gpsd running and it works fine with xgps, gpxlogger, cgps,... All that apps show me nice data about my location, etc.

I even compiled my own little test C application to test if libgps is working:
https://stackoverflow.com/a/32000487/810109
And yes, it did work.

However, after setting "geo.provider.use_gpsd" to true, Firefox Nightly doesn't use the gpsd location provider it seems... It always gives me the location of my wifi.

I even tried stuff like:
geo.provider.use_mls = false
geo.wifi.scan = false
geo.wifi.uri = <blank>

So for me it seems that gpsd isn't even contained in the Nightly binary?
Am I right? If not, what I am doing wrong? What do I have to do to make Firefox communicate with my gpsd daemon?

Thanks!

Comment 60

[Josh Matthews [:jdm]]

I do not believe that Nightly builds are compiled with gpsd support enabled. To enable it, you would need to do a local build with `ac_add_options --enable-gpsd` in your .mozconfig file.

Comment 61

[m.kurz]

Alright, thanks for the hint.

However wouldn't it make sense to always compile Firefox with gpsd support by default (even for the stable release channel)?

The config "geo.provider.use_gpsd" can still be false by default, so you guys don't break any geolocation contracts you apparently have set up with Google. However users like me, who have set up a running and working gpsd daemon could use it within Firefox by simply switching the "geo.provider.use_gpsd" flag...

For me it's just too much hassle to compile my own Firefox solely to enable that feature. I want to stick with the Firefox upgrades my distro provides me.
IMHO, right now the gpsd support within Firefox is basically useless for anyone if not compiled in by default.

What do you guys think?

Comment 62

[m.kurz]

Actually, the suggestion in my last comment is exactly what Hanno Schlichting suggests in comment #24:

> What we could do is the same we do with other providers like OS X Core
> Location, and build them by default but disable them by default via a
> preference. If a user than wants to change their location preferences, they
> can do so without having to build their own Firefox.

So that should be doable or not?

Comment 63

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi,

I'm long out of Mozilla, so I cannot comment on the current state and policies. But the code is still present [1] and can hopefully be build. I'd really like to see it being used more often. Maybe gpsd is something that your Linux distribution can provide to you. Opening a bug there and asking for it to be included in the build is at least worth trying.

[1] https://hg.mozilla.org/mozilla-central/file/tip/dom/system/linux

Comment 64

[m.kurz]

Alright, here we go, the feature request for Ubuntu:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1783202