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

[bugzilla-daemon at webkit.org]

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





--- Comment #2 from Carlos Garcia Campos <cgarcia at igalia.com>  2012-05-30 01:11:53 PST ---
(From update of attachment 144664)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.