[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

Michael Catanzaro <mcatanzaro at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mcatanzaro at igalia.com
 Attachment #365142|review?                     |review+
              Flags|                            |

--- 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.

-- 
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/20190320/e643d0fe/attachment-0001.html>