Thanks for the review. See my comments below...

(In reply to comment #29)
> (From update of attachment 225392 [details])
> 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. 

I thought you said you wanted this to be optional, that's why Anton added that flag (and I kept it). But I can change that of course.

> As martin said, we need the cmake support too.

Couldn't this be added as a separate patch? I'm not very fond on CMake to be honest and so I feel like this would delay this patch a bit, which should build fine anyway if autotools support is kept for a while.

> 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.

Ok. I'll try to do that change.

> > 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?

I think having that generated code is handy because it hides all the D-Bus complexity from WebCore code. Besides, it's not a big deal to generate it either (and it's following the suggested way from Geoclue2, as far as I know from Anton).

About the name, I will change it to DerivedSources/WebCore/geoclue2.

> > 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.

Ok.

> > Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:31
> > +#include <wtf/gobject/GRefPtr.h>
> 
> This si already included by the header.
> 

Ok. I'll remove it.

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

Ok too.

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

This is to avoid defining static functions in the implementation file. As far as I understand, it's the recommended alternative technique to using static functions in C++, so that's why I used it. 

But if you prefer, I can remove that unnamed namespace and make all those callbacks static, as in plain C.

> > 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

That's right. I'll change it.

> > 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.

Ok.

> > Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:60
> > +    provider->setGeoclueManagerProxyAndGetClient(managerProxy);
> 
> Add a comment here that the provider takes the ownership.

Ok.

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

Ok.

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

Ok.

> 
> > Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:87
> > +    provider->setGeoclueClientProxyAndStart(clientProxy);
> 
> Add a comment here about the ownership transfer too.

Ok.

> 
> > 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.

Oops! That was my fault. Sorry, I'll fix it.

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

Ok.

> > Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:138
> > +    m_clientProxy.clear();
> > +    m_managerProxy.clear();
> 
> These are going to be deleted now
> 

...therefore there are not needed. Ok.

> > 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
> 

Ok.

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

Ok.

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

Ok.

> > Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:163
> > +    m_locationProxy.clear();
> 
> Where is this set?
> 

When a new position is received. But according to your comment of having a possible leak, I need to revisit this again.

> > 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?
> 

We can do that, sure.

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

Ok.

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

Ok.

> 
> > 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?
> 
Good point. I'll do that.

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

Ok.

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

Then we can remove the private attributes and just use local variables from the stack to call notifyPositionChanged(). I'll do that

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

Ok.

> 
> > 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?
> 

That was what Anton originally did and I changed it (together with refactoring some logic), thinking it was the normal way to do it as I could see it from other source files. But I can change it back to static members, sure.

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

Ok.

> > 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
> 

That's right, I didn't notice it. Will refactor that too.

> > 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?
> 

I'll try to refactor all that in the next patch and then this should be gone, yes.

> > 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?

Same here