© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
1b1ed1a
(Get the code!)
Comment on attachment 212180
Patch
View in context: https:/
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/
>> + * GNUmakefile.am:
>
> You need at least a short description for the change here. Same consideration applies to the other ChangeLogs
yes
>> Source/
>> +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/
>> +void GeolocationProv
>
> I would rename this function to something more descriptive, such as startGeoclueClient or startGeoclueSub
ok
>> Source/
>> + return;
>
> You probably can combine this two in one if
ok
>> Source/
>> + // Set the requirement for the client
>
> Missing period.
ok
>> Source/
>> + gclue_client_
>
> 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 geoclueClientSt
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/
>> +void GeolocationProv
>
> I would also rename this function to something more descriptive, such as stopGeoclueClient or stopGeoclueSubs
ok
>> Source/
>> + m_clientProxy.
>
> Shouldn't you better do this on a callback for gclue_client_
void GeolocationProv
>> Source/
>> +void GeolocationProv
>
> Wrong placement for *
ok
>> Source/
>> + GOwnPtr<GError> error;
>
> Define this variable right before you need it (after the if)
ok
>> Source/
>> + provider-
>
> 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/
>> +GClueManager* GeolocationProv
>
> What about using geoclueManager() as the name?
ok
>> Source/
>> + GOwnPtr<gchar> path;
>
> Move this definitions down, right before you need it.
ok
>> Source/
>> + provider-
>
> String::format()
ok
>> Source/
>> + 0, onClientProxyReady, self);
>
> One line
ok
>> Source/
>> +GClueClient* GeolocationProv
>
> What about using geoclueClient() as the name?
ok
>> Source/
>> + gclue_manager_
>
> For the sake of consistency, I would call the callback used here this something like geoclueManagerG
ok
>> Source/
>> +void GeolocationProv
>
> Wrong placement for *
ok
>> Source/
>> + GOwnPtr<GError> error;
>
> Move this definition down
ok
>> Source/
>> + provider-
>
> String::format
ok
>> Source/
>> + G_CALLBACK(
>
> One line
ok
>> Source/
>> +void GeolocationProv
>
> Wrong placement for *
ok
>> Source/
>> + 0, onLocationProxy
>
> One line. And again, I would suggest another name for the callback. What about createGeoclueLo
ok
>> Source/
>> + GOwnPtr<GError> error;
>
> Move this down
ok
>> Source/
>> +void GeolocationProv
>
> Wrong placement for *
ok
>> Source/
>> + GOwnPtr<GError> error;
>
> Move this down
ok
>> Source/
>> + provider-
>
> String::format
ok
>> Source/
>> +}
>
> 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_
I need to check the latest version of GeoClue2 API, because the API wasn't available when I wrote this code.
>> Source/
>> +#define GEOCLUE_
>
> You don't need this in the header file, just in the implementation file
ok
>> Source/
>> + GeolocationProv
>
> I would move this attribute declaration down after the private functions, together with the rest of private attributes
ok
>> Source/
>> + static void onLocationProxy
>
> 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/
>> +#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(
> typedef GeolocationProv
> #else
> typedef GeolocationProv
> #endif
>
> }
Let me try.