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

[bugzilla-daemon at webkit.org]

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
https://bugs.webkit.org/show_bug.cgi?id=120185

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

------- 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
trunk.

> 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
> +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
$(GEOCLUE_DBUS_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,
G_DBUS_PROXY_FLAGS_NONE, GEOCLUE_BUS_NAME, GEOCLUE_MANAGER_PATH,
> +	   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: ") +
error->message);

String::format()

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:148
> +    gclue_client_proxy_new_for_bus(G_BUS_TYPE_SYSTEM,
G_DBUS_PROXY_FLAGS_NONE, GEOCLUE_BUS_NAME, path.get(),
> +	   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,
this);

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: ") +
error->message);

String::format

> 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,
G_DBUS_PROXY_FLAGS_NONE, GEOCLUE_BUS_NAME, newPath,
> +	   0, onLocationProxyReady, self);

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

> 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: ") +
error->message);

String::format

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

String::format

> 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
optional:

  "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*,
void*);
> +    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;
  #else
  typedef GeolocationProviderGeoclue2Client GeolocationProviderClient;
  #endif

  }

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