Comment 87 for bug 1100326

Comment on attachment 225788
Patch proposal

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.

> 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

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

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