[webkit-reviews] review granted: [Bug 195940] [GTK][WPE] Add API to provide geolocation information : [Attachment 365142] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at igalia.com> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 195940: [GTK][WPE] Add API to provide geolocation information
https://bugs.webkit.org/show_bug.cgi?id=195940

Attachment 365142: Patch

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




--- Comment #6 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 365142
  --> https://bugs.webkit.org/attachment.cgi?id=365142
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.