[webkit-reviews] review canceled: [Bug 120185] [GTK] Add support for Geoclue2 : [Attachment 225788] Patch proposal

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <mario at webkit.org> has canceled Mario Sanchez Prada
<mario at webkit.org>'s request for review:
Bug 120185: [GTK] Add support for Geoclue2
https://bugs.webkit.org/show_bug.cgi?id=120185

Attachment 225788: Patch proposal
https://bugs.webkit.org/attachment.cgi?id=225788&action=review

------- Additional Comments from Mario Sanchez Prada <mario at webkit.org>
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.