[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 #4 from Carlos Garcia Campos <cgarcia at igalia.com>  2012-05-31 08:56:53 PST ---
(From update of attachment 145031)
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) :-)

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