Comment 88 for bug 1100326

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.