Bug 195940 – [GTK][WPE] Add API to provide geolocation information

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

WebKitGTK currently uses GeoClue to provide a default implementation for geolocation, but it's not possible for application to provide their own implementation. We should add API for that and keep the GeoClue as the default implementation in case the users doesn't provide their own.

[Carlos Garcia Campos]

Created attachment 365140 [details]
Patch

[Carlos Garcia Campos]

Created attachment 365142 [details]
Patch

[Adrian Perez]

Comment on attachment 365142 [details]
Patch

I did a quick skimming over the new API and it looks quite nice
to me. Just a quick question: Is there any reason why the WPE port
cannot use GeoClue if available? As far as I understand, GeoClue
itself does not depend on GTK. Of course, we can also enable using
it as an option in the WPE port with a follow-up patch :-)

[Carlos Garcia Campos]

(In reply to Adrian Perez from comment #3)
> Comment on attachment 365142 [details]
> Patch
> 
> I did a quick skimming over the new API and it looks quite nice
> to me. Just a quick question: Is there any reason why the WPE port
> cannot use GeoClue if available? As far as I understand, GeoClue
> itself does not depend on GTK. Of course, we can also enable using
> it as an option in the WPE port with a follow-up patch :-)

No reason. I didn't add support for geoclue in this patch to WPE because I'm working on a patch to remove the build dependency on geoclue. We depend on geoclue only to generate code to talk to the DBus service. The code is simple enough to not need to auto-generate it, so it's better to simply use GDBus to talk to the Geoclue DBus service (if available). The patch will not have any build flag, so WPE will gain Geoclue support automatically.

[Adrian Perez]

(In reply to Carlos Garcia Campos from comment #4)
> (In reply to Adrian Perez from comment #3)
> > Comment on attachment 365142 [details]
> > Patch
> > 
> > I did a quick skimming over the new API and it looks quite nice
> > to me. Just a quick question: Is there any reason why the WPE port
> > cannot use GeoClue if available? As far as I understand, GeoClue
> > itself does not depend on GTK. Of course, we can also enable using
> > it as an option in the WPE port with a follow-up patch :-)
> 
> No reason. I didn't add support for geoclue in this patch to WPE because I'm
> working on a patch to remove the build dependency on geoclue. We depend on
> geoclue only to generate code to talk to the DBus service. The code is
> simple enough to not need to auto-generate it, so it's better to simply use
> GDBus to talk to the Geoclue DBus service (if available). The patch will not
> have any build flag, so WPE will gain Geoclue support automatically.

That sounds great! No need to make this one patch enable the
support for GeoClue in WPE, then 👍

[Michael Catanzaro]

Comment on attachment 365142 [details]
Patch

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

Well... this actually turned out better than I was expecting. The patch is fine, and the new API is fine for what it does. My main concern is simply that I'm not confident we really need the API. In theory, an application might want to roll its own geolocation and ignore geoclue, sure, but... well, the motivating example in this case is an app that wants to provide a fixed geolocation position at all times, and this seems like overkill for that purpose, right? Probably that application should just start building geoclue and write a fixed-position geoclue plugin, that way we don't need any new API. Or this could be kept downstream (not that I generally advocate for that). If the application you're adding this for was actually trying to do real geolocation, then I would be more enthusiastic.

(I was also going to say that "I don't want to build geoclue" isn't a super-compelling argument either, not unless geoclue actually takes up significant space on constrained embedded devices, but checking my desktop the geoclue daemon is actually using 7.7 MiB of RAM, which is more than I had expected. So I suppose an API to allow geolocation without geoclue might actually be appropriate for embedded.)

Anyway, r=me because it's fine, but this is a "meh, consider whether you really want this in the API forever" r=me rather than an enthusiastic one.

> Source/WebCore/ChangeLog:8
> +        Replace ENABLE(GEOLOCATION) with USE(GEOCLUE).

ENABLE(GEOLOCATION) still exists, of course, for all the WebCore geolocation code, but that's fine.

> Source/WebKit/ChangeLog:8
> +        Add WebKitGeolocationManager public class to handle geaolocation position updates. WebKitGeolocationProvider has

geolocation

> Source/WebKit/UIProcess/API/glib/WebKitGeolocationManager.cpp:153
> + * @timestamp: timestamp in seconds since the Epoch

epoch

> Source/WebKit/UIProcess/API/glib/WebKitGeolocationManager.cpp:162
> +    g_return_if_fail(timestamp);

Don't want to allow passing 0 for "now"?

> Source/WebKit/UIProcess/API/glib/WebKitGeolocationManager.cpp:186
> + * @altitude_accuracy: accuracy of position altitude in meters

It's not descriptive enough. Does it mean the real position may be @altitude_accuracy meters above or below the WebKitGeolocationPosition?

> Source/WebKit/UIProcess/API/glib/WebKitGeolocationManager.cpp:204
> + * Set the @position positive angle between the direction of movement and the North

"Set the @position's heading as a positive angle between the direction of movement and North, clockwise."

> Source/WebKit/UIProcess/API/glib/WebKitGeolocationManager.cpp:367
> +     * WebKitGeolocationManager:enable-high-accuracy:

Not really clear why this property is needed. Is the application responsible for providing low-accuracy data when this is not set, or will WebCore handle lowering the accuracy? Surely the later would be preferable? If the property is really needed, please document the expectations further. But I suspect it's not needed.

> Source/WebKit/UIProcess/API/glib/WebKitGeolocationManager.cpp:369
> +     * Whether high accuracy is enabled. This is a read-only property, that will be

no comma

> Source/WebKit/UIProcess/API/glib/WebKitGeolocationManager.cpp:386
> +     * WebKitGeolocationManager::start:

Maybe "WebKitGeolocationManager::geolocation-request-started"? It's hard to name....

> Source/WebKit/UIProcess/API/glib/WebKitGeolocationManager.cpp:392
> +     * the position changes; or use webkit_gelocation_manager_failed() in case

; -> ,

> Source/WebKit/UIProcess/API/glib/WebKitGeolocationManager.cpp:393
> +     * it couldn't be possible to determine the current position.

couldn't be -> isn't

> Source/WebKit/UIProcess/API/glib/WebKitGeolocationManager.cpp:413
> +     * WebKitGeolocationManager::stop:

WebKitGeolocationManager::geolocation-request-ended?

> Source/WebKit/UIProcess/API/wpe/docs/wpe-docs.sgml:89
> +  <index id="api-index-2-24" role="2.24">
> +    <title>Index of new symbols in 2.24</title>
> +    <xi:include href="xml/api-index-2.24.xml"><xi:fallback /></xi:include>
> +  </index>

If 2.24 has a new API version, I would actually delete the "index of new symbols" prior to 2.26.

Or if it's 2.26 that will have the new API version, you can delete them all.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #6)
> Comment on attachment 365142 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=365142&action=review
> 
> Well... this actually turned out better than I was expecting. The patch is
> fine, and the new API is fine for what it does. My main concern is simply
> that I'm not confident we really need the API. In theory, an application
> might want to roll its own geolocation and ignore geoclue, sure, but...
> well, the motivating example in this case is an app that wants to provide a
> fixed geolocation position at all times, and this seems like overkill for
> that purpose, right?

That was the initial motivation to add this API, but not the final goal. We really want to allow applications to provide geolocation positions using their own location system. That's how most of the GLib API works, there's a sane default, but you can provide your own (file chooser, print dialog, context menu, popup menus, notifications, etc.)

> Probably that application should just start building
> geoclue and write a fixed-position geoclue plugin, that way we don't need
> any new API. Or this could be kept downstream (not that I generally advocate
> for that). If the application you're adding this for was actually trying to
> do real geolocation, then I would be more enthusiastic.
>
> (I was also going to say that "I don't want to build geoclue" isn't a
> super-compelling argument either, not unless geoclue actually takes up
> significant space on constrained embedded devices, but checking my desktop
> the geoclue daemon is actually using 7.7 MiB of RAM, which is more than I
> had expected. So I suppose an API to allow geolocation without geoclue might
> actually be appropriate for embedded.)

Embedders might want to use their own gps system. 
 
> Anyway, r=me because it's fine, but this is a "meh, consider whether you
> really want this in the API forever" r=me rather than an enthusiastic one.

Yes, I don't see any problem.

> > Source/WebCore/ChangeLog:8
> > +        Replace ENABLE(GEOLOCATION) with USE(GEOCLUE).
> 
> ENABLE(GEOLOCATION) still exists, of course, for all the WebCore geolocation
> code, but that's fine.

We were using ENABLE(GEOLOCATION) wrongly for geoclue specific code. This options is removed in bug #195994 anyway.

> > Source/WebKit/ChangeLog:8
> > +        Add WebKitGeolocationManager public class to handle geaolocation position updates. WebKitGeolocationProvider has
> 
> geolocation
> 
> > Source/WebKit/UIProcess/API/glib/WebKitGeolocationManager.cpp:153
> > + * @timestamp: timestamp in seconds since the Epoch
> 
> epoch
> 
> > Source/WebKit/UIProcess/API/glib/WebKitGeolocationManager.cpp:162
> > +    g_return_if_fail(timestamp);
> 
> Don't want to allow passing 0 for "now"?

I guess we could.

> > Source/WebKit/UIProcess/API/glib/WebKitGeolocationManager.cpp:186
> > + * @altitude_accuracy: accuracy of position altitude in meters
> 
> It's not descriptive enough. Does it mean the real position may be
> @altitude_accuracy meters above or below the WebKitGeolocationPosition?

I'm not geolocation expert, I guess it's either, but I'll check the spec.

> > Source/WebKit/UIProcess/API/glib/WebKitGeolocationManager.cpp:204
> > + * Set the @position positive angle between the direction of movement and the North
> 
> "Set the @position's heading as a positive angle between the direction of
> movement and North, clockwise."
> 
> > Source/WebKit/UIProcess/API/glib/WebKitGeolocationManager.cpp:367
> > +     * WebKitGeolocationManager:enable-high-accuracy:
> 
> Not really clear why this property is needed. Is the application responsible
> for providing low-accuracy data when this is not set, or will WebCore handle
> lowering the accuracy? Surely the later would be preferable? If the property
> is really needed, please document the expectations further. But I suspect
> it's not needed.

geolocation API has an option to enable high accuracy, when not enable, it's up to the application whether to provide more or less accurate locations. This is a read-only property that applications need to monitor to switch to a more accurate mode, or ignore it if they always provide high accuracy. But it's needed, yes.

> > Source/WebKit/UIProcess/API/glib/WebKitGeolocationManager.cpp:369
> > +     * Whether high accuracy is enabled. This is a read-only property, that will be
> 
> no comma
> 
> > Source/WebKit/UIProcess/API/glib/WebKitGeolocationManager.cpp:386
> > +     * WebKitGeolocationManager::start:
> 
> Maybe "WebKitGeolocationManager::geolocation-request-started"? It's hard to
> name....

Our convention is to use verbs in past for signals that are notifications, so webkit doesn't expect anything from the application like load-started. In this case the signal is notifying the application that it should start providing location updates. Including geolocation in the signal name is redundant and request is not accurate either, because in case of multiple requests start is emitted only once.

> > Source/WebKit/UIProcess/API/glib/WebKitGeolocationManager.cpp:392
> > +     * the position changes; or use webkit_gelocation_manager_failed() in case
> 
> ; -> ,
> 
> > Source/WebKit/UIProcess/API/glib/WebKitGeolocationManager.cpp:393
> > +     * it couldn't be possible to determine the current position.
> 
> couldn't be -> isn't
> 
> > Source/WebKit/UIProcess/API/glib/WebKitGeolocationManager.cpp:413
> > +     * WebKitGeolocationManager::stop:
> 
> WebKitGeolocationManager::geolocation-request-ended?

No, for the same reasons.

> > Source/WebKit/UIProcess/API/wpe/docs/wpe-docs.sgml:89
> > +  <index id="api-index-2-24" role="2.24">
> > +    <title>Index of new symbols in 2.24</title>
> > +    <xi:include href="xml/api-index-2.24.xml"><xi:fallback /></xi:include>
> > +  </index>
> 
> If 2.24 has a new API version, I would actually delete the "index of new
> symbols" prior to 2.26.
> 
> Or if it's 2.26 that will have the new API version, you can delete them all.

I don't understand.

[Carlos Garcia Campos]

Committed r243285: <https://trac.webkit.org/changeset/243285>