Comment 56 for bug 1100326

Comment on attachment 212180
Patch

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.