Created attachment 225978
Patch proposal

Thanks Martin and Carlos for the reviews. I'm attaching a new patch addressing all those issues.

See some comments of mine below...

(In reply to comment #48)
> (From update of attachment 225788 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225788&action=review
> 
> Looks good to me, in general, but Carlos should probably do a review as well since he's managing the stable branch.

Sure.

> > Source/Platform/GNUmakefile.am:16
> > -	-I$(top_builddir)/DerivedSources/Platform
> > +	-I$(top_builddir)/DerivedSources/Platform \
> > +	-I$(top_builddir)/DerivedSources/WebCore
> >  
> 
> It probably makes sense to generate the files in DerivedSources/Platform for the autotools port.
> 
Done.

> > Source/WebCore/PlatformGTK.cmake:279
> > +         COMMAND gdbus-codegen --interface-prefix org.freedesktop.GeoClue2. --c-namespace GClue --generate-c-code ${DERIVED_SOURCES_WEBCORE_DIR}/Geoclue2Interface ${GEOCLUE_DBUS_INTERFACE}
> 
> So, if possible it would be could to use GeoClue as the C namespace.
> 
Might I suggest changing "GClue" to "Geoclue" (lower case "c") instead of to "GeoClue" here?

The reason why I suggest that is to keep the signature of the generated methods more similar to the Geoclue1 counterpart, having things like geoclue_location_get_latitude() instead of geo_clue_location_get_latitude(). 

In the new patch I did that ("Geoclue"), but I can change it to GeoClue if the naming of the generated methods is not an issue.

> > Source/WebCore/platform/geoclue/GeolocationProviderGeoclue2.cpp:36
> > +const char* GeoClueBusName = "org.freedesktop.GeoClue2";
> > +const char* GeoClueManagerPath = "/org/freedesktop/GeoClue2/Manager";
> 
> These should be named like gGeoClueBusName and gGeoClueManagerPath.
> 
> > Source/WebCore/platform/geoclue/GeolocationProviderGeoclue2.cpp:117
> > +    // The provider will take ownershipt of the managerProxy.
> 
> ownershipt->ownership

Done.

(In reply to comment #49)
> (From update of attachment 225788 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225788&action=review
> 
> > Source/WebCore/platform/geoclue/GeolocationProviderGeoclue.h:28
> > +#include <wtf/text/WTFString.h>
> 
> Why are you adding this? I don't see any String used by this header.
> 
It was in the original patch from Anton and forgot to remove it.

Now removed.

> > Source/WebCore/platform/geoclue/GeolocationProviderGeoclue2.cpp:50
> > +    , m_managerProxy(0)
> > +    , m_clientProxy(0)
> 
> You don't need to initialize GRefPtr, and in case of doing so, use nullptr instead of 0
> 

Removed those 2 lines.

> > Source/WebCore/platform/geoclue/GeolocationProviderGeoclue2.cpp:118
> > +    provider->setGeoclueManagerProxyAndGetClient(managerProxy);
> 
> The idea of using static memebers for callbacks was also to simplify this, and clarify the ownership. I think now you can directly here do 
> 
> provider->m_managerProxy = adoptGRef(gclue_manager_proxy_new_for_bus_finish(result, &error.outPtr()));
> if (error) {
>     provider->errorOccurred(error->message);
>     return;
> }
> 
> gclue_manager_call_get_client(provider->m_managerProxy.get(), nullptr, reinterpret_cast<GAsyncReadyCallback>(getGeoclueClientCallback), provider);
> 
> no? we make the code simpler and easier to follow, avoiding jumping up and down all the time. 

Done (changed the code to implement the approach you described here)

> Another approach would be the opposite, move all the handling to the object itself, leaving the callbacks with a single line, for example:
> 
> provider->handleGeoclueManagerProxyCreated(result);
> 

Nah. The other approach sounds good to me as well (and avoids polluting the header file)

> > Source/WebCore/platform/geoclue/GeolocationProviderGeoclue2.cpp:162
> > +    // Geoclue2 requires the client to provide a desktop ID for security
> > +    // reasons, which should identify the application requesting the location.
> > +    CString programName = getCurrentExecutablePath();
> > +    gclue_client_set_desktop_id(m_clientProxy.get(), basename(programName.data()));
> 
> So, I guess the program name works as desktop file id in the end? or is it just a workaround? Wouldn't it be easier to directly use g_get_prgname() that returns a const char *?

For some reason the compiler complained last time I tried to use that one and so I ended up using this getCurrentExecutablePath() from WTF. But I've tried now again and got no issue at all (I might have misread the error output previously), so I replaced those two lines with g_get_prgname(). Done.