Comment 67 for bug 1100326

Comment on attachment 225392
Patch Proposal

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

Thanks for the patch. I prefer if the geoclue version is detected automatically instead of having to pass an option to configure, if possible. As martin said, we need the cmake support too. I think we should use a common class name and header files, and different implementation (cpp files only) for geoclue 1 and 2. That way we don't even change anything in WebKit layer, because the provider api used by the api layer is the same.

> Source/Platform/GNUmakefile.am:15
> + -I$(top_builddir)/DerivedSources/geoclue2

This should probably be something like DerivedSources/WebCore/geoclue2. Do we really need generated code for this?

> Source/WebCore/GNUmakefile.am:389
> +# Geoclue interface

I think it's pretty obvious already :-)

> Source/WebCore/GNUmakefile.am:391
> +DerivedSources/geoclue2/geoclue-interface.c: DerivedSources/geoclue2/geoclue-interface.h
> +DerivedSources/geoclue2/geoclue-interface.h:

We should probably name the generated files GeoClue2Interface.h and GeoClue2Interface.c for consistency.

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:31
> +#include <wtf/gobject/GRefPtr.h>

This si already included by the header.

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:34
> +#include <wtf/text/WTFString.h>

Ditto.

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:41
> +using namespace WebCore;
> +
> +namespace {

Why?

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:48
> +typedef enum {
> + GCLUE_ACCURACY_LEVEL_COUNTRY = 1,
> + GCLUE_ACCURACY_LEVEL_CITY = 4,
> + GCLUE_ACCURACY_LEVEL_STREET = 6,
> + GCLUE_ACCURACY_LEVEL_EXACT = 8,
> +} GClueAccuracyLevel;

This should follow webkit coding style since it's private

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:55
> + GeolocationProviderGeoclue2* provider = static_cast<GeolocationProviderGeoclue2*>(self);

You could probably cast the callback and use GeolocationProviderGeoclue2* provider as parameter instead of void* self.

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:60
> + provider->setGeoclueManagerProxyAndGetClient(managerProxy);

Add a comment here that the provider takes the ownership.

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:68
> + GeolocationProviderGeoclue2* provider = static_cast<GeolocationProviderGeoclue2*>(self);

Ditto.

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:82
> + GeolocationProviderGeoclue2* provider = static_cast<GeolocationProviderGeoclue2*>(self);

Ditto.

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:87
> + provider->setGeoclueClientProxyAndStart(clientProxy);

Add a comment here about the ownership transfer too.

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:101
> + provider->updateLocation(locationProxy);

And here we are leaking the new proxy? updateLocation doesn't seem to take the ownership, it simply gets info from the location proxy. Maybe we could pass a GRefPtr to make it more explicit and we don't need comments.

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:107
> + 0, createLocationProxyCallback, self);

0 -> nullptr

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:138
> + m_clientProxy.clear();
> + m_managerProxy.clear();

These are going to be deleted now

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:146
> + gclue_manager_proxy_new_for_bus(G_BUS_TYPE_SYSTEM, G_DBUS_PROXY_FLAGS_NONE, GEOCLUE_BUS_NAME, GEOCLUE_MANAGER_PATH, 0, createGeoclueManagerProxyCallback, this);

0 -> nullptr

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:151
> + gclue_manager_call_get_client(m_managerProxy.get(), 0, getGeoclueClientCallback, this);

0 -> nullptr

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:161
> + gclue_client_call_stop(m_clientProxy.get(), 0, 0, 0);

nullptr

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:163
> + m_locationProxy.clear();

Where is this set?

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:169
> + m_enableHighAccuracy = enable;

Should we check if the value has actually changed and return early if it hasn't?

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:185
> + gclue_client_call_start(m_clientProxy.get(), 0, startClientCallback, this);

nullptr

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:192
> + gclue_manager_call_get_client(m_managerProxy.get(), 0, getGeoclueClientCallback, this);

nullptr

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:197
> + gclue_client_proxy_new_for_bus(G_BUS_TYPE_SYSTEM, G_DBUS_PROXY_FLAGS_NONE, GEOCLUE_BUS_NAME, path,

This doesn't use anything from the object, could we do this directly in the callback that call this, like you are doing for for the location proxy?

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:198
> + 0, createGeoclueClientProxyCallback, this);

nullptr

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:220
> + m_altitude = 0; // altitude is not supported now

This will never change

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:222
> + m_altitudeAccuracy = 0; // altitude accuracy is not supported now

Ditto.

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.h:46
> + // To be used from signal callbacks.
> + void setGeoclueManagerProxyAndGetClient(GClueManager*);
> + void createGeoclueClientProxyAndInitialize(const gchar*);
> + void setGeoclueClientProxyAndStart(GClueClient*);
> + void updateLocation(GClueLocation*);
> + void errorOccurred(const char*);

Can we make the callbacks static members and we don't need all this API to be public?

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.h:55
> + GRefPtr<GClueLocation> m_locationProxy;

This looks unused.

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2Client.h:31
> +class GeolocationProviderGeoclue2Client {
> +public:
> + virtual void notifyPositionChanged(int timestamp, double latitude, double longitude, double altitude, double accuracy, double altitudeAccuracy) = 0;
> + virtual void notifyErrorOccurred(const char* message) = 0;
> +};

Why are we duplicating this? Can't we use the same provider client for both geoclue 1 and 2? This looks exactly the same that GeolocationProviderGeoclueClient.h

> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.h:46
> +#if !defined(GEOCLUE_API_VERSION_2)
> +typedef GeolocationProviderGeoclueClient GeolocationProviderClient;
> +#else
> +typedef GeolocationProviderGeoclue2Client GeolocationProviderClient;
> +#endif

I guess we don't need this, if we use the same client, because it's the same in the end, no?

> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.h:76
> +#if defined(GEOCLUE_API_VERSION_2)
> + WebCore::GeolocationProviderGeoclue2 m_provider;
> +#else
> WebCore::GeolocationProviderGeoclue m_provider;
> +#endif

Since we are compiling one or the other, and the api is the same, why not using the same name and we don't need more #ifdefs here?