[webkit-reviews] review denied: [Bug 120185] Port to Geoclue2 : [Attachment 225392] Patch Proposal

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Mario Sanchez Prada
<mario at webkit.org>'s request for review:
Bug 120185: Port to Geoclue2
https://bugs.webkit.org/show_bug.cgi?id=120185

Attachment 225392: Patch Proposal
https://bugs.webkit.org/attachment.cgi?id=225392&action=review

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
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?