[Webkit-unassigned] [Bug 195940] [GTK][WPE] Add API to provide geolocation information

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=195940

--- Comment #7 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20190321/d2dbde39/attachment-0001.html>