Comment 81 for bug 1100326

Comment on attachment 225775
Patch proposal

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

(In reply to comment #42)
> (From update of attachment 225775 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225775&action=review
>
> The CMake parts look okay.

Thanks!

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

You mean defining then both GEOCLUE_API_VERSION_1 and GEOCLUE_API_VERSION_2, making them both available to the code (so we can disable one file or the other), right?

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

Ok, I'll do

> I can show you how to do it for CMake.

Thanks, that would be very much appreciated :)

>> Source/Platform/GNUmakefile.am:109
>> +endif
>
> You can use GEOCLUE_CFLAGS for both GeoClue1 and GeoClue2 and avoid this change.

ok

>> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue.h:37
>> +#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. :)

ok

>> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue2.cpp:36
>> +#define GEOCLUE_MANAGER_PATH "/org/freedesktop/GeoClue2/Manager"
>
> Please use static const char* for these.

No problem

>> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue2.cpp:44
>> + GClueAccuracyLevelExact = 8,
>
> GClue -> GeoClue.

ok