Comment on attachment 212180
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=212180&action=review

I was wondering about the testing. I should be able to use Geolocation layout tests. Also I should probably write new API tests. The main problem for me that I am not sure how to setup system D-BUS GeoClue2 service for the testing. May be I can develop some fake GeoClue2 D-BUS interface. How do you usually handle testing requiring specific D-BUS services?

>> Source/Platform/ChangeLog:8
>> +        * GNUmakefile.am:
> 
> You need at least a short description for the change here. Same consideration applies to the other ChangeLogs

yes

>> Source/WebCore/GNUmakefile.am:375
>> +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.

Can be done. I need to check latest version of the API though first.

>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:66
>> +void GeolocationProviderGeoclue2::start()
> 
> I would rename this function to something more descriptive, such as startGeoclueClient or startGeoclueSubsystem

ok

>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:72
>> +        return;
> 
> You probably can combine this two in one if

ok

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

ok

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

It will be valid here since "if (!client()) return statement" will ensure that m_clientProxy is not null. But ASSERT can be added here anyway.

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

ok

>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:85
>> +    m_clientProxy.clear();
> 
> Shouldn't you better do this on a callback for gclue_client_call_stop?

void GeolocationProviderGeoclue2::stop() is synchronous so I use synchronous gclue_client_call_stop here and I would prefer to do cleanup synchronously since it should reduce possible mess with start/stop calls.

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

ok

>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:110
>> +    GOwnPtr<GError> error;
> 
> Define this variable right before you need it (after the if)

ok

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

ok

>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:124
>> +GClueManager* GeolocationProviderGeoclue2::manager()
> 
> What about using geoclueManager() as the name?

ok

>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:137
>> +    GOwnPtr<gchar> path;
> 
> Move this definitions down, right before you need it.

ok

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

ok

>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:148
>> +        0, onClientProxyReady, self);
> 
> One line

ok

>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:151
>> +GClueClient* GeolocationProviderGeoclue2::client()
> 
> What about using geoclueClient() as the name?

ok

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

ok

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

ok

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

ok

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

ok

>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:176
>> +        G_CALLBACK(onLocationUpdated), self);
> 
> One line

ok

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

ok

>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:188
>> +        0, onLocationProxyReady, self);
> 
> One line. And again, I would suggest another name for the callback. What about createGeoclueLocationProxyCallback?

ok

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

ok

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

ok

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

ok

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

ok

>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:221
>> +}
> 
> 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

I need to check the latest version of GeoClue2 API, because the API wasn't available when I wrote this code.

>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.h:31
>> +#define GEOCLUE_MANAGER_PATH "/org/freedesktop/GeoClue2/Manager"
> 
> You don't need this in the header file, just in the implementation file

ok

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

ok

>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.h:57
>> +    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

Hm, I need to think about it :) Static member function allows access to private attributes of the class.

>> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.h:47
>> +#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
> 
>   }

Let me try.