Bug 87800 – [GTK] Add a new and reusable Geoclue-based geolocation provider in WebCore

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Mario Sanchez Prada]

The idea is to have a class in WebCore that we can use both for WebKit and WebKit2, which would take care of dealing with Geoclue when using the geolocation module.

This new class would allow simplifying code in WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp and would ease a lot the implementation of a new client-based geolocation provider for WebKit2GTK (see bug 83877)

[Mario Sanchez Prada]

Created attachment 144664 [details]
Patch proposal

[Carlos Garcia Campos]

Comment on attachment 144664 [details]
Patch proposal

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

I know this code has just been moved from wk layer, but I think we can take advantage to improve or fix it. Most of the geoclue api includes sync and async versions of the methods, and we are using sync in some cases and async in others, so I wonder whether we could make this fully non-blocking  by always using the async methods.

> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue.cpp:81
> +    GOwnPtr<GError> error;
> +    GRefPtr<GeoclueMasterClient> client = adoptGRef(geoclue_master_create_client(master.get(), 0, &error.outPtr()));
> +    if (!client) {
> +        errorOccured(error->message);
> +        return;
> +    }

Could we use geoclue_master_create_client_async() instead?

> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue.cpp:88
> +    GeoclueAccuracyLevel accuracyLevel = m_enableHighAccuracy ? GEOCLUE_ACCURACY_LEVEL_DETAILED : GEOCLUE_ACCURACY_LEVEL_LOCALITY;
> +    if (!geoclue_master_client_set_requirements(client.get(), accuracyLevel, 0,
> +                                                false, GEOCLUE_RESOURCE_ALL, &error.outPtr())) {
> +        errorOccured(error->message);
> +        return;
> +    }

Could we use geoclue_master_client_set_requirements_async() instead?. We could move this to a helper since it's used twice in this file.

> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue.cpp:94
> +    m_geocluePosition = adoptGRef(geoclue_master_client_create_position(client.get(), &error.outPtr()));
> +    if (!m_geocluePosition) {
> +        errorOccured(error->message);
> +        return;
> +    }

geoclue_master_client_create_position_async()?

> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue.cpp:99
> +    g_signal_connect(G_OBJECT(m_geocluePosition.get()), "position-changed",
> +                     G_CALLBACK(positionChangedCallback), this);

g_signal_connect expects a gpointer not a GObject so we don't need to cast here. This could probably be a single line,

> Source/WebCore/platform/geoclue/GeolocationProviderGeoclueListener.h:26
> +#include <geoclue/geoclue-master.h>
> +#include <geoclue/geoclue-position.h>

Do you really need these headers here?

[Mario Sanchez Prada]

Created attachment 145031 [details]
Patch proposal

New patch, addressing issues raised by Carlos

[Carlos Garcia Campos]

Comment on attachment 145031 [details]
Patch proposal

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

> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue.cpp:39
> +        g_error_free(error);

We are freeing the error here but not in the other callbacks, it always looked weird to me that we would have to free the error in the callback, but I assumed geoclue worked this way. If this is the case we should free the error in all other callbacks too.

> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue.cpp:60
> +    provider->setGeocluePosition(position);
> +
> +    geoclue_position_get_position_async(position, reinterpret_cast<GeocluePositionCallback>(getPositionCallback), provider);
> +    g_signal_connect(position, "position-changed", G_CALLBACK(positionChangedCallback), provider);

In these cases where we have to use a static callback I prefer to do the minimum things in the callback, what do you think about moving these two lines to setGeocluePosition?

> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue.cpp:82
> +    provider->setGeoclueClient(client);
> +    provider->updateClientRequirements();
> +
> +    geoclue_master_client_create_position_async(client, createPositionCallback, provider);

Same here, we could updateClientRequirements and create the position in setGeoclueClient

> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue.cpp:143
> +    m_isUpdating = true;

It looks a bit weird to set isUpdating here, is it a problem if it's set in startUpdating()?

> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue.cpp:172
> +void GeolocationProviderGeoclue::errorOccured(const char* message)
> +{
> +    m_listener->notifyErrorOcurred(message);
> +}

This is funny, one method is errorOccured (double c in ocurred and only one r) and the other is notifyErrorOcurred (only on c and double r), I'm not English expert, but google says the right word id occurred (double c and r) :-)

[Mario Sanchez Prada]

Created attachment 145321 [details]
Patch proposal

New patch addressing issues raised by Carlos.

I also renamed GeolocationProviderGeoclueListener to GeolocationProviderGeoclueClient.

[Carlos Garcia Campos]

Comment on attachment 145321 [details]
Patch proposal

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

Please consider changing the return in geoclueClientSetRequirementsCallback before landing.

> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue.cpp:66
> +    if (error) {
> +        provider->errorOccurred(error->message);
> +        g_error_free(error);
> +        return;
> +    }

This could be an early return, or remove the return, because there's nothing to do if there isn't an error

[Mario Sanchez Prada]

Committed r119249: <http://trac.webkit.org/changeset/119249>