[webkit-reviews] review requested: [Bug 120185] [GTK] Add support for Geoclue2 : [Attachment 225978] Patch proposal
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Mar 6 04:08:43 PST 2014
Mario Sanchez Prada <mario at webkit.org> has asked for review:
Bug 120185: [GTK] Add support for Geoclue2
https://bugs.webkit.org/show_bug.cgi?id=120185
Attachment 225978: Patch proposal
https://bugs.webkit.org/attachment.cgi?id=225978&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.
More information about the webkit-reviews
mailing list