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 This si already included by the header. > Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:34 > +#include 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(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(self); Ditto. > Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:82 > + GeolocationProviderGeoclue2* provider = static_cast(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 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?