Comment 80 for bug 1100326

Comment on attachment 225775
Patch proposal

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

The CMake parts look okay. Instead of adding the source files to the list conditionally, I would add them both unconditionally and guard them with a preprocessor define. I prefer GEOCLUE_API_VERSION_2 to be named WTF_USE_GEOCLUE2. The GTK+ version one is not a great example to follow. The last general comment I have is that we avoid polluting the command-line flags any further, if we can avoid it. Can you try to make this option available in the config.h file. I can show you how to do it for CMake.

> Source/Platform/GNUmakefile.am:109
> +if GEOCLUE_API_VERSION_2
> +libPlatform_la_CPPFLAGS += \
> + $(GEOCLUE2_CFLAGS)
> +else
> +libPlatform_la_CPPFLAGS += \
> + $(GEOCLUE_CFLAGS)
> +endif

You can use GEOCLUE_CFLAGS for both GeoClue1 and GeoClue2 and avoid this change.

> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue.h:37
> +#if defined(GEOCLUE_API_VERSION_2)
> +#include "Geoclue2Interface.h"
> +#else
> #include <geoclue/geoclue-master.h>
> #include <geoclue/geoclue-position.h>
> +#endif
> #include <wtf/gobject/GRefPtr.h>
> +#if defined(GEOCLUE_API_VERSION_2)
> +#include <wtf/text/WTFString.h>
> +#endif

You should split off the conditional includes at the end of the list instead of trying to keep them alphabetical. This will make it a bit simpler here. :)

> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue2.cpp:36
> +#define GEOCLUE_BUS_NAME "org.freedesktop.GeoClue2"
> +#define GEOCLUE_MANAGER_PATH "/org/freedesktop/GeoClue2/Manager"

Please use static const char* for these.

> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue2.cpp:44
> + GClueAccuracyLevelCountry = 1,
> + GClueAccuracyLevelCity = 4,
> + GClueAccuracyLevelStreet = 6,
> + GClueAccuracyLevelExact = 8,

GClue -> GeoClue.