[webkit-reviews] review denied: [Bug 120185] Port to Geoclue2 : [Attachment 212180] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Sep 23 03:09:52 PDT 2013

Mario Sanchez Prada <mario at webkit.org> has denied Anton Obzhirov
<a.obzhirov at samsung.com>'s request for review:
Bug 120185: Port to Geoclue2

Attachment 212180: Patch

------- Additional Comments from Mario Sanchez Prada <mario at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=212180&action=review

Thanks for the patch, Anton. It looks quite well already, but there's
definitely more work to do (e.g. tests) so setting r- for now.

Also, I really think some more people familiar with these part of the code
should review it, specially when it comes to how to incorporate this into

> Source/Platform/ChangeLog:8
> +
> +	   * GNUmakefile.am:

You need at least a short description for the change here. Same consideration
applies to the other ChangeLogs

> Source/WebCore/GNUmakefile.am:375
> +# Geoclue interface
> +DerivedSources/geoclue2/geoclue-interface.c:
> +DerivedSources/geoclue2/geoclue-interface.h:
> +	$(AM_V_GEN)gdbus-codegen --interface-prefix org.freedesktop.GeoClue2.
--c-namespace GClue --generate-c-code DerivedSources/geoclue2/geoclue-interface
> +endif

I'd rather use "Geoclue2" as the generated namespace instead of the cut-down
version "GClue".

Anyway, I'm not sure enough about this bit since I feel like I lack some
information to provide a proper review. Thus, I'd rather have someone else
reviewing the changes in this file.

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:66
> +void GeolocationProviderGeoclue2::start()

I would rename this function to something more descriptive, such as
startGeoclueClient or startGeoclueSubsystem

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:72
> +    if (!manager())
> +	   return;
> +
> +    if (!client())
> +	   return;

You probably can combine this two in one if

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:74
> +    // Set the requirement for the client

Missing period.

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:76
> +    gclue_client_call_start(m_clientProxy.get(), 0, onStart, this);

What happens here if m_clientProxy is not valid? Shouldn't we control that
situation if that's a possibility or just ASSERT for non-null otherwise?

Also, "onStart" looks like to me like a too simple name for the callback. What
about something like geoclueClientStartCallback?

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:79
> +void GeolocationProviderGeoclue2::stop()

I would also rename this function to something more descriptive, such as
stopGeoclueClient or stopGeoclueSubsystem

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:85
> +    m_locationProxy.clear();
> +    m_clientProxy.clear();

Shouldn't you better do this on a callback for gclue_client_call_stop?

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:108
> +void GeolocationProviderGeoclue2::onManagerProxyReady(GObject *sourceObject,
GAsyncResult *result, void* self)

Wrong placement for *

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:110
> +    GOwnPtr<GError> error;

Define this variable right before you need it (after the if)

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:117
> +	   provider->errorOccurred(String("Failed to create Geoclue manager: ")
+ error->message);

You can use String::format() for this, or the StringBuilder class if you're
planning to concatenate many things (which does not seem to be the case)

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:124
> +GClueManager* GeolocationProviderGeoclue2::manager()

What about using geoclueManager() as the name?

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:130
> +    gclue_manager_proxy_new_for_bus(G_BUS_TYPE_SYSTEM,
> +	   0, onManagerProxyReady, this);

You can make this one a single line

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:137
> +    GOwnPtr<GError> error;
> +    GOwnPtr<gchar> path;

Move this definitions down, right before you need it.

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:143
> +	   provider->errorOccurred(String("Failed to get client: ") +


> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:148
> +    gclue_client_proxy_new_for_bus(G_BUS_TYPE_SYSTEM,
> +	   0, onClientProxyReady, self);

One line

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:151
> +GClueClient* GeolocationProviderGeoclue2::client()

What about using geoclueClient() as the name?

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:158
> +    gclue_manager_call_get_client(m_managerProxy.get(), 0, onGetClientReady,

For the sake of consistency, I would call the callback used here this something
like geoclueManagerGetClientCallback

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:162
> +void GeolocationProviderGeoclue2::onClientProxyReady(GObject *sourceObject,
GAsyncResult *result, void* self)

Wrong placement for *

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:164
> +    GOwnPtr<GError> error;

Move this definition down

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:171
> +	   provider->errorOccurred(String("Failed to get client proxy: ") +


> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:176
> +    g_signal_connect(provider->m_clientProxy.get(), "location-updated",
> +	   G_CALLBACK(onLocationUpdated), self);

One line

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:181
> +void GeolocationProviderGeoclue2::onLocationUpdated(GClueClient *client,
const gchar* oldPath, const gchar* newPath, void* self)

Wrong placement for *

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:188
> +    gclue_location_proxy_new_for_bus(G_BUS_TYPE_SYSTEM,
> +	   0, onLocationProxyReady, self);

One line. And again, I would suggest another name for the callback. What about

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:193
> +    GOwnPtr<GError> error;

Move this down

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:199
> +	   provider->errorOccurred(String("Failed to start: ") +


> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:202
> +void GeolocationProviderGeoclue2::onLocationProxyReady(GObject
*sourceObject, GAsyncResult *result, void* self)

Wrong placement for *

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:204
> +    GOwnPtr<GError> error;

Move this down

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:211
> +	   provider->errorOccurred(String("Failed to create location proxy: ")
+ error->message);


> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:221
> +void GeolocationProviderGeoclue2::updateClientRequirements()
> +{
> +    // FIXME: implement when accuracy API is available
> +}

If this is is an optional feature, I'd rather leave this function out of the
patch. However, according to the W3C spec, accuracy does not seem like

  "6.2.2 The Geolocation API must provide information about the accuracy of the
retrieved location data."

In any case, later in this patch you use a call to
gclue_location_get_accuracy() so I'm not entirely sure about what you mean with
this FIXME here

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.h:31
> +#define GEOCLUE_BUS_NAME "org.freedesktop.GeoClue2"
> +#define GEOCLUE_MANAGER_PATH "/org/freedesktop/GeoClue2/Manager"

You don't need this in the header file, just in the implementation file

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.h:50
> +    GeolocationProviderGeoclue2Client* m_client;

I would move this attribute declaration down after the private functions,
together with the rest of private attributes

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.h:57
> +    static void onManagerProxyReady(GObject*, GAsyncResult*, void*);
> +    static void onGetClientReady(GObject*, GAsyncResult*, void*);
> +    static void onClientProxyReady(GObject*, GAsyncResult*, void*);
> +    static void onStart(GObject*, GAsyncResult*, void*);
> +    static void onStop(GObject*, GAsyncResult*, void*);
> +    static void onLocationUpdated(GClueClient*, const gchar*, const gchar*,
> +    static void onLocationProxyReady(GObject*, GAsyncResult*, void*);

You do not need to make this callbacks static members of this class, just
static functions in the implementation file (C-style) or, even better, private
functions in the implementation file members of an unnamed namespace

> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.h:47
> +#if !defined(GEOCLUE_API_VERSION_2)
>  namespace WebCore {
>  class Geolocation;
> +typedef GeolocationProviderGeoclueClient GeolocationProviderClient;
> +}
> +#else
> +namespace WebCore {
> +typedef GeolocationProviderGeoclue2Client GeolocationProviderClient;
>  }
> +#endif

I think you still want the forward class declaration of Geolocation if using
geoclue2, meaning that the only difference is the typedef.

What about something like this:

  namespace WebCore {
  class Geolocation;

  #if !defined(GEOCLUE_API_VERSION_2)
  typedef GeolocationProviderGeoclueClient GeolocationProviderClient;
  typedef GeolocationProviderGeoclue2Client GeolocationProviderClient;


> Tools/gtk/jhbuild.modules:33
> +	 <dep package="json-glib"/>
> +	 <dep package="geoclue"/>

This should be moved to the list of optional modules, at least while it's meant
to be an experimental thing.

More information about the webkit-reviews mailing list