[GTK] Add support for Geoclue2

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Zeeshan Ali]

Geoclue is being re-written, with emphasis on simplicity (API and implementation) and privacy (user's location should not be shared w/o asking for user's consent). Obviously, all apps that use geoclue need to port to new D-Bus interface: http://cgit.freedesktop.org/geoclue/tree/src/geoclue-interface.xml . Sorry no docs yet but there is two examples at least:

http://cgit.freedesktop.org/geoclue/tree/demo/where-am-i.c
https://git.gnome.org/browse/gnome-maps/tree/src/geoclue.js

Regarding privacy, I had a discussion with one of WebKit devs (can't recall the name) about the issue of browser itself asking for user's consent and hence duplication of user's input required and hence us annoying the hell out of the user. Thing is that currently this privacy is not implemented and when it is, there is certainly going to be whitelisting of applications. Browsers and GNOME Maps are the first ones to be on that list so there really shouldn't be any issue.

[Zan Dobersek]

I see that the new API doesn't provide the altitude and the altitude accuracy information as compared to the old API - that's a shame, but any other information apart from latitude, longitude and the accuracy of those two is optional per the W3C spec[1], so that's OK really.

If you ever intend to expand the Geoclue interface and the API, we could also make use of the heading and speed information in addition to the altitude and the altitude accuracy :>

[1] http://www.w3.org/TR/geolocation-API/

[Zeeshan Ali]

(In reply to comment #1)
> I see that the new API doesn't provide the altitude and the altitude accuracy information as compared to the old API - that's a shame, but any other information apart from latitude, longitude and the accuracy of those two is optional per the W3C spec[1], so that's OK really.

Oh the only reason we don't yet support those is that currently we only have IP-based geolocation and the only open geoip DB we know, doens't provide altitude info. :( We'll add those when we have the most accurate source: GPS(A).
 
> If you ever intend to expand the Geoclue interface and the API, we could also make use of the heading and speed information in addition to the altitude and the altitude accuracy :>

'heading' is already on the todo and 'speed; should be very much possible when we have heading but those only make sense for more precise/portable sources than geoip.
 
> [1] http://www.w3.org/TR/geolocation-API/

[Zan Dobersek]

(In reply to comment #2)
> (In reply to comment #1)
> > I see that the new API doesn't provide the altitude and the altitude accuracy information as compared to the old API - that's a shame, but any other information apart from latitude, longitude and the accuracy of those two is optional per the W3C spec[1], so that's OK really.
> 
> Oh the only reason we don't yet support those is that currently we only have IP-based geolocation and the only open geoip DB we know, doens't provide altitude info. :( We'll add those when we have the most accurate source: GPS(A).

This makes much sense, thanks for clearing it up.

[Anton Obzhirov]

Started working on geoclue2 provider.

[Zeeshan Ali]

(In reply to comment #4)
> Started working on geoclue2 provider.

Awesome. Would be really nice if all apps have already ported in 3.10 so distros don't have to keep shipping the old unmaintained code.

BTW, if you get annoyed by interface not being as simple as it should be, there is hope :)

https://bugs.freedesktop.org/show_bug.cgi?id=68658

[Zeeshan Ali]

(In reply to comment #5)
> (In reply to comment #4)
> > Started working on geoclue2 provider.
> 
> Awesome. Would be really nice if all apps have already ported in 3.10 so distros don't have to keep shipping the old unmaintained code.

To be clear, I meant GNOME 3.10. :)

[Anton Obzhirov]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > Started working on geoclue2 provider.
> > 
> > Awesome. Would be really nice if all apps have already ported in 3.10 so distros don't have to keep shipping the old unmaintained code.
> 
> To be clear, I meant GNOME 3.10. :)

While implementing it got few questions for you:
1) About accuracy - are you planning to add API to set accuracy level of the latitude and longitude coordinates? I mean high/low accuracy. Or may be I can set it via DistanceThreshold property. In this case what would be "high" accuracy threshold?
2) Same about timestamps - there is no way to obtain them now.
3) I am going to use gdbus-codegen to generate wrapper for the API like they do it for Empathy. if you had client lib for the API it would be a bit easier but anyway I can live with the generated interface.
4) And last question for now :) where can I find geoclue2 ip location provider to try out my changes?

[Zeeshan Ali]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > (In reply to comment #4)
> > > > Started working on geoclue2 provider.
> > > 
> > > Awesome. Would be really nice if all apps have already ported in 3.10 so distros don't have to keep shipping the old unmaintained code.
> > 
> > To be clear, I meant GNOME 3.10. :)
> 
> While implementing it got few questions for you:
> 1) About accuracy - are you planning to add API to set accuracy level of the latitude and longitude coordinates?

If I understood your question correctly, no. This info is given from geoclue to app together with coordinates. Its accuracy of the location fix in meters. Currently its city-level at best. The app-writable 'DistanceThreshold' property is to specify the minimum distance changed before geoclue should notify your app.

> 2) Same about timestamps - there is no way to obtain them now.

What kind of timestamps you need?

> 3) I am going to use gdbus-codegen to generate wrapper for the API like they do it for Empathy. if you had client lib for the API it would be a bit easier but anyway I can live with the generated interface.

Sure. I was kind hoping that Empathy guys meant contributing patches for these rather when they said "contribute". :)

> 4) And last question for now :) where can I find geoclue2 ip location provider to try out my changes?

You don't need to do anything other than to just install geoclue2 (in system prefix as that seems to be required of system dbus services) and run your app.

[Anton Obzhirov]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (In reply to comment #5)
> > > > (In reply to comment #4)
> > > > > Started working on geoclue2 provider.
> > > > 
> > > > Awesome. Would be really nice if all apps have already ported in 3.10 so distros don't have to keep shipping the old unmaintained code.
> > > 
> > > To be clear, I meant GNOME 3.10. :)
> > 
> > While implementing it got few questions for you:
> > 1) About accuracy - are you planning to add API to set accuracy level of the latitude and longitude coordinates?
> 
> If I understood your question correctly, no. This info is given from geoclue to app together with coordinates. Its accuracy of the location fix in meters. Currently its city-level at best. The app-writable 'DistanceThreshold' property is to specify the minimum distance changed before geoclue should notify your app.

Just for information you had this API for accuracy in geoclue:
/**
 * GeoclueAccuracyLevel:
 *
 * Enum values used to define the approximate accuracy of 
 * Position or Address information. These are ordered in
 * from lowest accuracy possible to highest accuracy possible.
 * geoclue_accuracy_get_details() can be used to get get the
 * current accuracy. It is up to the provider to set the
 * accuracy based on analysis of its queries.
 **/
typedef enum {
	GEOCLUE_ACCURACY_LEVEL_NONE = 0,
	GEOCLUE_ACCURACY_LEVEL_COUNTRY,
	GEOCLUE_ACCURACY_LEVEL_REGION,
	GEOCLUE_ACCURACY_LEVEL_LOCALITY,
	GEOCLUE_ACCURACY_LEVEL_POSTALCODE,
	GEOCLUE_ACCURACY_LEVEL_STREET,
	GEOCLUE_ACCURACY_LEVEL_DETAILED,
} GeoclueAccuracyLevel;

geoclue_master_client_set_requirements_*(, GeoclueAccuracyLevel, ...);
> 
> > 2) Same about timestamps - there is no way to obtain them now.
> 
> What kind of timestamps you need?
from the Geolocation spec: The length of time specified by the timeout property has elapsed before the implementation could successfully acquire a new Position. 
in geoclue https://developer.gnome.org/geoclue/unstable/GeocluePosition.html#GeocluePositionCallback has timestamp which I guess 
is abs time of acquiring the position. In geoclue2 I can calculate timestamps myself on LocationUpdated but it would be a bit in precise. But probably should be OK in most of the cases.

> 
> > 3) I am going to use gdbus-codegen to generate wrapper for the API like they do it for Empathy. if you had client lib for the API it would be a bit easier but anyway I can live with the generated interface.
> 
> Sure. I was kind hoping that Empathy guys meant contributing patches for these rather when they said "contribute". :)
> 
> > 4) And last question for now :) where can I find geoclue2 ip location provider to try out my changes?
> 
> You don't need to do anything other than to just install geoclue2 (in system prefix as that seems to be required of system dbus services) and run your app.

[Zeeshan Ali]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > (In reply to comment #6)
> > > > (In reply to comment #5)
> > > > > (In reply to comment #4)
> > > > > > Started working on geoclue2 provider.
> > > > > 
> > > > > Awesome. Would be really nice if all apps have already ported in 3.10 so distros don't have to keep shipping the old unmaintained code.
> > > > 
> > > > To be clear, I meant GNOME 3.10. :)
> > > 
> > > While implementing it got few questions for you:
> > > 1) About accuracy - are you planning to add API to set accuracy level of the latitude and longitude coordinates?
> > 
> > If I understood your question correctly, no. This info is given from geoclue to app together with coordinates. Its accuracy of the location fix in meters. Currently its city-level at best. The app-writable 'DistanceThreshold' property is to specify the minimum distance changed before geoclue should notify your app.
> 
> Just for information you had this API for accuracy in geoclue:
> /**
>  * GeoclueAccuracyLevel:
>  *
>  * Enum values used to define the approximate accuracy of 
>  * Position or Address information. These are ordered in
>  * from lowest accuracy possible to highest accuracy possible.
>  * geoclue_accuracy_get_details() can be used to get get the
>  * current accuracy. It is up to the provider to set the
>  * accuracy based on analysis of its queries.
>  **/
> typedef enum {
>     GEOCLUE_ACCURACY_LEVEL_NONE = 0,
>     GEOCLUE_ACCURACY_LEVEL_COUNTRY,
>     GEOCLUE_ACCURACY_LEVEL_REGION,
>     GEOCLUE_ACCURACY_LEVEL_LOCALITY,
>     GEOCLUE_ACCURACY_LEVEL_POSTALCODE,
>     GEOCLUE_ACCURACY_LEVEL_STREET,
>     GEOCLUE_ACCURACY_LEVEL_DETAILED,
> } GeoclueAccuracyLevel;
> 
> geoclue_master_client_set_requirements_*(, GeoclueAccuracyLevel, ...);

Ah! Its there in the design:

   enum RequestedAccuracy: accuracy level requested by the application

I can look into adding it today if you really need it?

> > > 2) Same about timestamps - there is no way to obtain them now.
> > 
> > What kind of timestamps you need?
> from the Geolocation spec: The length of time specified by the timeout property has elapsed before the implementation could successfully acquire a new Position. 
> in geoclue https://developer.gnome.org/geoclue/unstable/GeocluePosition.html#GeocluePositionCallback has timestamp which I guess 
> is abs time of acquiring the position. In geoclue2 I can calculate timestamps myself on LocationUpdated but it would be a bit in precise. But probably should be OK in most of the cases.

Ah ok but I'm still interested to find out if this info is actual useful for apps? If its for measuring speed, we have in the design to add 'speed' in geoclue2 so app won't have to do that.

[Anton Obzhirov]

I think it is good to (In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > (In reply to comment #7)
> > > > (In reply to comment #6)
> > > > > (In reply to comment #5)
> > > > > > (In reply to comment #4)
> > > > > > > Started working on geoclue2 provider.
> > > > > > 
> > > > > > Awesome. Would be really nice if all apps have already ported in 3.10 so distros don't have to keep shipping the old unmaintained code.
> > > > > 
> > > > > To be clear, I meant GNOME 3.10. :)
> > > > 
> > > > While implementing it got few questions for you:
> > > > 1) About accuracy - are you planning to add API to set accuracy level of the latitude and longitude coordinates?
> > > 
> > > If I understood your question correctly, no. This info is given from geoclue to app together with coordinates. Its accuracy of the location fix in meters. Currently its city-level at best. The app-writable 'DistanceThreshold' property is to specify the minimum distance changed before geoclue should notify your app.
> > 
> > Just for information you had this API for accuracy in geoclue:
> > /**
> >  * GeoclueAccuracyLevel:
> >  *
> >  * Enum values used to define the approximate accuracy of 
> >  * Position or Address information. These are ordered in
> >  * from lowest accuracy possible to highest accuracy possible.
> >  * geoclue_accuracy_get_details() can be used to get get the
> >  * current accuracy. It is up to the provider to set the
> >  * accuracy based on analysis of its queries.
> >  **/
> > typedef enum {
> >     GEOCLUE_ACCURACY_LEVEL_NONE = 0,
> >     GEOCLUE_ACCURACY_LEVEL_COUNTRY,
> >     GEOCLUE_ACCURACY_LEVEL_REGION,
> >     GEOCLUE_ACCURACY_LEVEL_LOCALITY,
> >     GEOCLUE_ACCURACY_LEVEL_POSTALCODE,
> >     GEOCLUE_ACCURACY_LEVEL_STREET,
> >     GEOCLUE_ACCURACY_LEVEL_DETAILED,
> > } GeoclueAccuracyLevel;
> > 
> > geoclue_master_client_set_requirements_*(, GeoclueAccuracyLevel, ...);
> 
> Ah! Its there in the design:
> 
>    enum RequestedAccuracy: accuracy level requested by the application
> 
> I can look into adding it today if you really need it?
>
Well it is not urgent but would be good to have for testing - I hope to get my change working by the end of this week.
> > > > 2) Same about timestamps - there is no way to obtain them now.
> > > 
> > > What kind of timestamps you need?
> > from the Geolocation spec: The length of time specified by the timeout property has elapsed before the implementation could successfully acquire a new Position. 
> > in geoclue https://developer.gnome.org/geoclue/unstable/GeocluePosition.html#GeocluePositionCallback has timestamp which I guess 
> > is abs time of acquiring the position. In geoclue2 I can calculate timestamps myself on LocationUpdated but it would be a bit in precise. But probably should be OK in most of the cases.
> 
> Ah ok but I'm still interested to find out if this info is actual useful for apps? If its for measuring speed, we have in the design to add 'speed' in geoclue2 so app won't have to do that.
As for the applications use cases I don't really know myself. May be someone else can comment here. I guess it can be used also to determine quality of the service (in terms of delays and so on) but again it is just my guess.

[Anton Obzhirov]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (In reply to comment #5)
> > > > (In reply to comment #4)
> > > > > Started working on geoclue2 provider.
> > > > 
> > > > Awesome. Would be really nice if all apps have already ported in 3.10 so distros don't have to keep shipping the old unmaintained code.
> > > 
> > > To be clear, I meant GNOME 3.10. :)
> > 
> > While implementing it got few questions for you:
> > 1) About accuracy - are you planning to add API to set accuracy level of the latitude and longitude coordinates?
> 
> If I understood your question correctly, no. This info is given from geoclue to app together with coordinates. Its accuracy of the location fix in meters. Currently its city-level at best. The app-writable 'DistanceThreshold' property is to specify the minimum distance changed before geoclue should notify your app.
> 
> > 2) Same about timestamps - there is no way to obtain them now.
> 
> What kind of timestamps you need?
> 
> > 3) I am going to use gdbus-codegen to generate wrapper for the API like they do it for Empathy. if you had client lib for the API it would be a bit easier but anyway I can live with the generated interface.
> 
> Sure. I was kind hoping that Empathy guys meant contributing patches for these rather when they said "contribute". :)
> 
> > 4) And last question for now :) where can I find geoclue2 ip location provider to try out my changes?
> 
> You don't need to do anything other than to just install geoclue2 (in system prefix as that seems to be required of system dbus services) and run your app.
I am having problems running geoclue2 on Ubuntu 12.10 - some libs including glib are outdated and there are segmentations faults while running the service. I take it you developed it on Fedora?

[Anton Obzhirov]

What's you IRC account BTW?

[Zeeshan Ali]

(In reply to comment #12)
> (In reply to comment #8)
> > > 4) And last question for now :) where can I find geoclue2 ip location provider to try out my changes?
> > 
> > You don't need to do anything other than to just install geoclue2 (in system prefix as that seems to be required of system dbus services) and run your app.
> I am having problems running geoclue2 on Ubuntu 12.10 - some libs including glib are outdated and there are segmentations faults while running the service.

Yikes. :(

> I take it you developed it on Fedora?

Yeah so our requirement of glib might not be correctly set in configure but then again, you shouldn't be able to build it in the first place if we are using too new API. I'm 'zeenix' on IRC. Better come to our channel #gnome-maps on irc.gnome.org.

[Zeeshan Ali]

Oh and I strongly suggest you use git master of geoclue for now. The last release had many potential crashes and issues. We'll roll out another one as soon as we've fixed some more issues.

[Anton Obzhirov]

Created attachment 212180 [details]
Patch

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Mario Sanchez Prada]

Comment on attachment 212180 [details]
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.

[Anton Obzhirov]

Comment on attachment 212180 [details]
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.

[Zeeshan Ali]

Just wanted to comment here to remind you nice folks about this. Geoclue now has more sources: wifi, 3GPP and GPS. Also gnome-shell now (3.11) shows an icon in the panel when location services are in use. Would be nice to get this in place in time for 3.12 as well.

[Anton Obzhirov]

(In reply to comment #20)
> Just wanted to comment here to remind you nice folks about this. Geoclue now has more sources: wifi, 3GPP and GPS. Also gnome-shell now (3.11) shows an icon in the panel when location services are in use. Would be nice to get this in place in time for 3.12 as well.

Hi, thanks for the update, I will keep it in mind, I've been busy recently but should be able to find soon some time to get the patch in.

[Emilio Pozuelo Monfort]

(In reply to comment #21)
> Hi, thanks for the update, I will keep it in mind, I've been busy recently but should be able to find soon some time to get the patch in.

The 2.4 stable release is approaching (a branch has already been created). I don't know if it's too late to get this in for 2.4, but if not, the sooner a patch is proposed, the better.

It'd be a pitty to miss another stable release, particularly as the old geoclue is being dropped from some distributions and not porting webkit would mean geoclue support has to be disabled.

[Carlos Garcia Campos]

(In reply to comment #22)
> (In reply to comment #21)
> > Hi, thanks for the update, I will keep it in mind, I've been busy recently but should be able to find soon some time to get the patch in.
> 
> The 2.4 stable release is approaching (a branch has already been created). I don't know if it's too late to get this in for 2.4, but if not, the sooner a patch is proposed, the better.
> 
> It'd be a pitty to miss another stable release, particularly as the old geoclue is being dropped from some distributions and not porting webkit would mean geoclue support has to be disabled.

As long as the dependency is optional, it doesn't break any test and the patch is low risk I don't mind to add it to the stable branch, but I agree, the sooner the better. I have branched early because of the jsCStack merge that broke the world, but we are not still frozen.

[Anton Obzhirov]

Guys, I got :) Will try to do it as soon as possible - will try to find sometime at the end of the week.

[Zeeshan Ali]

(In reply to comment #24)
> Guys, I got :) Will try to do it as soon as possible - will try to find sometime at the end of the week.

Any luck in finding time for this? We just merged controls for geolocation in gnome-shell so now its even more important that at least all GNOME apps honor those preferences by using geoclue2: https://raw.github.com/gnome-design-team/gnome-mockups/master/shell/system-menu/system-status-menu-geolocation.png

[Mario Sanchez Prada]

Created attachment 225392 [details]
Patch Proposal

(In reply to comment #25)
> (In reply to comment #24)
> > Guys, I got :) Will try to do it as soon as possible - will try to find sometime at the end of the week.
> 
> Any luck in finding time for this? We just merged controls for geolocation in gnome-shell so now
> its even more important that at least all GNOME apps honor those preferences by using geoclue2: 
> https://raw.github.com/gnome-design-team/gnome-mockups/master/shell/system-menu/system-status-menu-geolocation.png

As I mentioned yesterday on IRC, I agree with Anton to lend him a hand moving this forward, as he's extremely busy these days with other matters and could barely do it on time. So, I picked his patch, rebase it against master and did a few changes (some of them following my own review) and I think I got now a patch that could be a good option already for now since it works :), and adds this as an optional thing (you need to pass --with-geoclue=2.0 at configure time).

Please take a look to it and let me know what you think. I'll try to be as fast and responsive to comments as possible during the following days.

Thanks!

[Martin Robinson]

(In reply to comment #26)
> patch that could be a good option already for now since it works :), and adds 
> this as an optional thing (you need to pass --with-geoclue=2.0 at configure 
> time).

I think it would be better to automatically choose Geoclue 2 when it is available on the system and avoid adding a new configuration option. This patch will also probably need CMake support, as it is possible that the autotools build will be gone by the time it lands.

[Carlos Garcia Campos]

(In reply to comment #27)
> (In reply to comment #26)
> > patch that could be a good option already for now since it works :), and adds 
> > this as an optional thing (you need to pass --with-geoclue=2.0 at configure 
> > time).
> 
> I think it would be better to automatically choose Geoclue 2 when it is available on the system and avoid adding a new configuration option. This patch will also probably need CMake support, as it is possible that the autotools build will be gone by the time it lands.

Please, don't remove the autotools build, let's switch the bots to build with cmake by default, but keep the auotools build for a while, at least until I can make a successful release with cmake

[Carlos Garcia Campos]

Comment on attachment 225392 [details]
Patch Proposal

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

Thanks for the patch. I prefer if the geoclue version is detected automatically instead of having to pass an option to configure, if possible. As martin said, we need the cmake support too. I think we should use a common class name and header files, and different implementation (cpp files only) for geoclue 1 and 2. That way we don't even change anything in WebKit layer, because the provider api used by the api layer is the same.

> Source/Platform/GNUmakefile.am:15
> +	-I$(top_builddir)/DerivedSources/geoclue2

This should probably be something like DerivedSources/WebCore/geoclue2. Do we really need generated code for this?

> Source/WebCore/GNUmakefile.am:389
> +# Geoclue interface

I think it's pretty obvious already :-)

> Source/WebCore/GNUmakefile.am:391
> +DerivedSources/geoclue2/geoclue-interface.c: DerivedSources/geoclue2/geoclue-interface.h
> +DerivedSources/geoclue2/geoclue-interface.h:

We should probably name the generated files GeoClue2Interface.h and GeoClue2Interface.c for consistency.

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:31
> +#include <wtf/gobject/GRefPtr.h>

This si already included by the header.

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:34
> +#include <wtf/text/WTFString.h>

Ditto.

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:41
> +using namespace WebCore;
> +
> +namespace {

Why?

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:48
> +typedef enum {
> +    GCLUE_ACCURACY_LEVEL_COUNTRY = 1,
> +    GCLUE_ACCURACY_LEVEL_CITY = 4,
> +    GCLUE_ACCURACY_LEVEL_STREET = 6,
> +    GCLUE_ACCURACY_LEVEL_EXACT = 8,
> +} GClueAccuracyLevel;

This should follow webkit coding style since it's private

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:55
> +    GeolocationProviderGeoclue2* provider = static_cast<GeolocationProviderGeoclue2*>(self);

You could probably cast the callback and use GeolocationProviderGeoclue2* provider as parameter instead of void* self.

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:60
> +    provider->setGeoclueManagerProxyAndGetClient(managerProxy);

Add a comment here that the provider takes the ownership.

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:68
> +    GeolocationProviderGeoclue2* provider = static_cast<GeolocationProviderGeoclue2*>(self);

Ditto.

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:82
> +    GeolocationProviderGeoclue2* provider = static_cast<GeolocationProviderGeoclue2*>(self);

Ditto.

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:87
> +    provider->setGeoclueClientProxyAndStart(clientProxy);

Add a comment here about the ownership transfer too.

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:101
> +    provider->updateLocation(locationProxy);

And here we are leaking the new proxy? updateLocation doesn't seem to take the ownership, it simply gets info from the location proxy. Maybe we could pass a GRefPtr to make it more explicit and we don't need comments.

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:107
> +        0, createLocationProxyCallback, self);

0 -> nullptr

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

These are going to be deleted now

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:146
> +        gclue_manager_proxy_new_for_bus(G_BUS_TYPE_SYSTEM, G_DBUS_PROXY_FLAGS_NONE, GEOCLUE_BUS_NAME, GEOCLUE_MANAGER_PATH, 0, createGeoclueManagerProxyCallback, this);

0 -> nullptr

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:151
> +        gclue_manager_call_get_client(m_managerProxy.get(), 0, getGeoclueClientCallback, this);

0 -> nullptr

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:161
> +        gclue_client_call_stop(m_clientProxy.get(), 0, 0, 0);

nullptr

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

Where is this set?

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:169
> +    m_enableHighAccuracy = enable;

Should we check if the value has actually changed and return early if it hasn't?

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

nullptr

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:192
> +    gclue_manager_call_get_client(m_managerProxy.get(), 0, getGeoclueClientCallback, this);

nullptr

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:197
> +    gclue_client_proxy_new_for_bus(G_BUS_TYPE_SYSTEM, G_DBUS_PROXY_FLAGS_NONE, GEOCLUE_BUS_NAME, path,

This doesn't use anything from the object, could we do this directly in the callback that call this, like you are doing for for the location proxy?

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:198
> +        0, createGeoclueClientProxyCallback, this);

nullptr

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:220
> +    m_altitude = 0; // altitude is not supported now

This will never change

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:222
> +    m_altitudeAccuracy = 0; // altitude accuracy is not supported now

Ditto.

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.h:46
> +    // To be used from signal callbacks.
> +    void setGeoclueManagerProxyAndGetClient(GClueManager*);
> +    void createGeoclueClientProxyAndInitialize(const gchar*);
> +    void setGeoclueClientProxyAndStart(GClueClient*);
> +    void updateLocation(GClueLocation*);
> +    void errorOccurred(const char*);

Can we make the callbacks static members and we don't need all this API to be public?

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.h:55
> +    GRefPtr<GClueLocation> m_locationProxy;

This looks unused.

> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2Client.h:31
> +class GeolocationProviderGeoclue2Client {
> +public:
> +    virtual void notifyPositionChanged(int timestamp, double latitude, double longitude, double altitude, double accuracy, double altitudeAccuracy) = 0;
> +    virtual void notifyErrorOccurred(const char* message) = 0;
> +};

Why are we duplicating this? Can't we use the same provider client for both geoclue 1 and 2? This looks exactly the same that GeolocationProviderGeoclueClient.h

> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.h:46
> +#if !defined(GEOCLUE_API_VERSION_2)
> +typedef GeolocationProviderGeoclueClient GeolocationProviderClient;
> +#else
> +typedef GeolocationProviderGeoclue2Client GeolocationProviderClient;
> +#endif

I guess we don't need this, if we use the same client, because it's the same in the end, no?

> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.h:76
> +#if defined(GEOCLUE_API_VERSION_2)
> +    WebCore::GeolocationProviderGeoclue2 m_provider;
> +#else
>      WebCore::GeolocationProviderGeoclue m_provider;
> +#endif

Since we are compiling one or the other, and the api is the same, why not using the same name and we don't need more #ifdefs here?

[Mario Sanchez Prada]

Thanks for the review. See my comments below...

(In reply to comment #29)
> (From update of attachment 225392 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225392&action=review
> 
> Thanks for the patch. I prefer if the geoclue version is detected automatically instead of having to pass an option to configure, if possible. 

I thought you said you wanted this to be optional, that's why Anton added that flag (and I kept it). But I can change that of course.

> As martin said, we need the cmake support too.

Couldn't this be added as a separate patch? I'm not very fond on CMake to be honest and so I feel like this would delay this patch a bit, which should build fine anyway if autotools support is kept for a while.

> I think we should use a common class name and header files, and different implementation (cpp files only) for geoclue 1 and 2. That way we don't even change anything in WebKit layer, because the provider api used by the api layer is the same.

Ok. I'll try to do that change.

> > Source/Platform/GNUmakefile.am:15
> > +	-I$(top_builddir)/DerivedSources/geoclue2
> 
> This should probably be something like DerivedSources/WebCore/geoclue2. Do we really need generated code for this?

I think having that generated code is handy because it hides all the D-Bus complexity from WebCore code. Besides, it's not a big deal to generate it either (and it's following the suggested way from Geoclue2, as far as I know from Anton).

About the name, I will change it to DerivedSources/WebCore/geoclue2.

> > Source/WebCore/GNUmakefile.am:389
> > +# Geoclue interface
> 
> I think it's pretty obvious already :-)

:)

> > Source/WebCore/GNUmakefile.am:391
> > +DerivedSources/geoclue2/geoclue-interface.c: DerivedSources/geoclue2/geoclue-interface.h
> > +DerivedSources/geoclue2/geoclue-interface.h:
> 
> We should probably name the generated files GeoClue2Interface.h and GeoClue2Interface.c for consistency.

Ok.

> > Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:31
> > +#include <wtf/gobject/GRefPtr.h>
> 
> This si already included by the header.
> 

Ok. I'll remove it.

> > Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:34
> > +#include <wtf/text/WTFString.h>
> 
> Ditto.
> 

Ok too.

> > Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:41
> > +using namespace WebCore;
> > +
> > +namespace {
> 
> Why?

This is to avoid defining static functions in the implementation file. As far as I understand, it's the recommended alternative technique to using static functions in C++, so that's why I used it. 

But if you prefer, I can remove that unnamed namespace and make all those callbacks static, as in plain C.

> > Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:48
> > +typedef enum {
> > +    GCLUE_ACCURACY_LEVEL_COUNTRY = 1,
> > +    GCLUE_ACCURACY_LEVEL_CITY = 4,
> > +    GCLUE_ACCURACY_LEVEL_STREET = 6,
> > +    GCLUE_ACCURACY_LEVEL_EXACT = 8,
> > +} GClueAccuracyLevel;
> 
> This should follow webkit coding style since it's private

That's right. I'll change it.

> > Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:55
> > +    GeolocationProviderGeoclue2* provider = static_cast<GeolocationProviderGeoclue2*>(self);
> 
> You could probably cast the callback and use GeolocationProviderGeoclue2* provider as parameter instead of void* self.

Ok.

> > Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:60
> > +    provider->setGeoclueManagerProxyAndGetClient(managerProxy);
> 
> Add a comment here that the provider takes the ownership.

Ok.

> > Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:68
> > +    GeolocationProviderGeoclue2* provider = static_cast<GeolocationProviderGeoclue2*>(self);
> 
> Ditto.

Ok.

> > Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:82
> > +    GeolocationProviderGeoclue2* provider = static_cast<GeolocationProviderGeoclue2*>(self);
> 
> Ditto.

Ok.

> 
> > Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:87
> > +    provider->setGeoclueClientProxyAndStart(clientProxy);
> 
> Add a comment here about the ownership transfer too.

Ok.

> 
> > Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:101
> > +    provider->updateLocation(locationProxy);
> 
> And here we are leaking the new proxy? updateLocation doesn't seem to take the ownership, it simply gets info from the location proxy. Maybe we could pass a GRefPtr to make it more explicit and we don't need comments.

Oops! That was my fault. Sorry, I'll fix it.

> 
> > Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:107
> > +        0, createLocationProxyCallback, self);
> 
> 0 -> nullptr
> 

Ok.

> > Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:138
> > +    m_clientProxy.clear();
> > +    m_managerProxy.clear();
> 
> These are going to be deleted now
> 

...therefore there are not needed. Ok.

> > Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:146
> > +        gclue_manager_proxy_new_for_bus(G_BUS_TYPE_SYSTEM, G_DBUS_PROXY_FLAGS_NONE, GEOCLUE_BUS_NAME, GEOCLUE_MANAGER_PATH, 0, createGeoclueManagerProxyCallback, this);
> 
> 0 -> nullptr
> 

Ok.

> > Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:151
> > +        gclue_manager_call_get_client(m_managerProxy.get(), 0, getGeoclueClientCallback, this);
> 
> 0 -> nullptr
> 

Ok.

> > Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:161
> > +        gclue_client_call_stop(m_clientProxy.get(), 0, 0, 0);
> 
> nullptr
> 

Ok.

> > Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:163
> > +    m_locationProxy.clear();
> 
> Where is this set?
> 

When a new position is received. But according to your comment of having a possible leak, I need to revisit this again.

> > Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:169
> > +    m_enableHighAccuracy = enable;
> 
> Should we check if the value has actually changed and return early if it hasn't?
> 

We can do that, sure.

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

Ok.

> 
> > Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:192
> > +    gclue_manager_call_get_client(m_managerProxy.get(), 0, getGeoclueClientCallback, this);
> 
> nullptr

Ok.

> 
> > Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:197
> > +    gclue_client_proxy_new_for_bus(G_BUS_TYPE_SYSTEM, G_DBUS_PROXY_FLAGS_NONE, GEOCLUE_BUS_NAME, path,
> 
> This doesn't use anything from the object, could we do this directly in the callback that call this, like you are doing for for the location proxy?
> 
Good point. I'll do that.

> > Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:198
> > +        0, createGeoclueClientProxyCallback, this);
> 
> nullptr

Ok.

> 
> > Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:220
> > +    m_altitude = 0; // altitude is not supported now
> 
> This will never change

Then we can remove the private attributes and just use local variables from the stack to call notifyPositionChanged(). I'll do that

> 
> > Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:222
> > +    m_altitudeAccuracy = 0; // altitude accuracy is not supported now
> 
> Ditto.

Ok.

> 
> > Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.h:46
> > +    // To be used from signal callbacks.
> > +    void setGeoclueManagerProxyAndGetClient(GClueManager*);
> > +    void createGeoclueClientProxyAndInitialize(const gchar*);
> > +    void setGeoclueClientProxyAndStart(GClueClient*);
> > +    void updateLocation(GClueLocation*);
> > +    void errorOccurred(const char*);
> 
> Can we make the callbacks static members and we don't need all this API to be public?
> 

That was what Anton originally did and I changed it (together with refactoring some logic), thinking it was the normal way to do it as I could see it from other source files. But I can change it back to static members, sure.

> > Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.h:55
> > +    GRefPtr<GClueLocation> m_locationProxy;
> 
> This looks unused.
> 

Ok.

> > Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2Client.h:31
> > +class GeolocationProviderGeoclue2Client {
> > +public:
> > +    virtual void notifyPositionChanged(int timestamp, double latitude, double longitude, double altitude, double accuracy, double altitudeAccuracy) = 0;
> > +    virtual void notifyErrorOccurred(const char* message) = 0;
> > +};
> 
> Why are we duplicating this? Can't we use the same provider client for both geoclue 1 and 2? This looks exactly the same that GeolocationProviderGeoclueClient.h
> 

That's right, I didn't notice it. Will refactor that too.

> > Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.h:46
> > +#if !defined(GEOCLUE_API_VERSION_2)
> > +typedef GeolocationProviderGeoclueClient GeolocationProviderClient;
> > +#else
> > +typedef GeolocationProviderGeoclue2Client GeolocationProviderClient;
> > +#endif
> 
> I guess we don't need this, if we use the same client, because it's the same in the end, no?
> 

I'll try to refactor all that in the next patch and then this should be gone, yes.

> > Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.h:76
> > +#if defined(GEOCLUE_API_VERSION_2)
> > +    WebCore::GeolocationProviderGeoclue2 m_provider;
> > +#else
> >      WebCore::GeolocationProviderGeoclue m_provider;
> > +#endif
> 
> Since we are compiling one or the other, and the api is the same, why not using the same name and we don't need more #ifdefs here?

Same here

[Carlos Garcia Campos]

(In reply to comment #30)
> Thanks for the review. See my comments below...
> 
> (In reply to comment #29)
> > (From update of attachment 225392 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=225392&action=review
> > 
> > Thanks for the patch. I prefer if the geoclue version is detected automatically instead of having to pass an option to configure, if possible. 
> 
> I thought you said you wanted this to be optional, that's why Anton added that flag (and I kept it). But I can change that of course.

And I want it to be optional, that's orthogonal to be automatically detected. What I don't want is to bump the min requirements. 

> > As martin said, we need the cmake support too.
> 
> Couldn't this be added as a separate patch? I'm not very fond on CMake to be honest and so I feel like this would delay this patch a bit, which should build fine anyway if autotools support is kept for a while.

I'm sure martin can help us with this.

> > > Source/Platform/GNUmakefile.am:15
> > > +	-I$(top_builddir)/DerivedSources/geoclue2
> > 
> > This should probably be something like DerivedSources/WebCore/geoclue2. Do we really need generated code for this?
> 
> I think having that generated code is handy because it hides all the D-Bus complexity from WebCore code. Besides, it's not a big deal to generate it either (and it's following the suggested way from Geoclue2, as far as I know from Anton).

As long as you don't break distcheck it's fine with me.

> About the name, I will change it to DerivedSources/WebCore/geoclue2.

Thanks.

> > > Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:41
> > > +using namespace WebCore;
> > > +
> > > +namespace {
> > 
> > Why?
> 
> This is to avoid defining static functions in the implementation file. As far as I understand, it's the recommended alternative technique to using static functions in C++, so that's why I used it. 

I had to go up and down all the time to follow the calls.

> But if you prefer, I can remove that unnamed namespace and make all those callbacks static, as in plain C.

No, I said static members not static as in plain C, keeping the callbacks and the internal methods private in the class.

[Zeeshan Ali]

(In reply to comment #29)
> (From update of attachment 225392 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225392&action=review
> 
> Thanks for the patch. I prefer if the geoclue version is detected automatically instead of having to pass an option to configure, if possible. As martin said, we need the cmake support too. I think we should use a common class name and header files, and different implementation (cpp files only) for geoclue 1 and 2. That way we don't even change anything in WebKit layer, because the provider api used by the api layer is the same.

Would it still be possible for webkit to provide the destkop ID of the application then? We require it in geoclue2 to be able to implement secure application authorization when we have means to reliably verify an app's desktop ID.

[Mario Sanchez Prada]

Created attachment 225475 [details]
Patch Proposal

Please see attached a WIP patch proposal addressing all the comments by Carlos. I'm not asking r? yet, because there are a couple of things I need to check first:

 - CMake build for Geoclue1 (I suspect is broken with this patch)
 - CMake build for Geoclue2 (I want to give it a try anyway :)
 - Providing the right desktop ID to geoclue (as it seems the dummy desktop ID included with this patch won't be good enough)

I'll try to do that on Monday, but feel free to inspect this new patch (which I tested locally and works as the previous one) if you want to

Have you all a nice weekend!

[Allison Lortie (desrt)]

Ideas for determining the desktop file name:

 - use the GApplication app-id

   - will be equal in increasing numbers of cases

 - use g_get_prgname()

   - will be equal as long as people follow the
     prgname == wmclass == desktop name convention


at this point in time, I'd go with option #2.  That should cover most cases.

One tricky thing to consider, however -- epiphany webapps.  They have their own separate desktop files...  Do you want to use that desktop ID here, or the main one for epiphany?

[Martin Robinson]

(In reply to comment #34)

> One tricky thing to consider, however -- epiphany webapps.  They have their own separate desktop files...  Do you want to use that desktop ID here, or the main one for epiphany?

Is the application ID used for permissions requests? WebKit will always need it's own permissions request mechanism, because it's per-security domain instead of per-application.

[Zeeshan Ali]

(In reply to comment #35)
> (In reply to comment #34)
> 
> > One tricky thing to consider, however -- epiphany webapps.  They have their own separate desktop files...  Do you want to use that desktop ID here, or the main one for epiphany?
> 
> Is the application ID used for permissions requests?

Yes.

> WebKit will always need it's own permissions request mechanism,
> because it's per-security domain instead of per-application.

I know of this special case of web browsers and I already have put at least some of the apps in the whitelist. The application ID will still be needed to be able to identify the app to determine that its whitelisted.

[Martin Robinson]

Comment on attachment 225475 [details]
Patch Proposal

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

> Source/WebCore/GNUmakefile.am:392
> +if GEOCLUE_API_VERSION_2
> +DerivedSources/WebCore/geoclue2/Geoclue2Interface.c: DerivedSources/WebCore/geoclue2/Geoclue2Interface.h
> +DerivedSources/WebCore/geoclue2/Geoclue2Interface.h:
> +	$(AM_V_GEN)gdbus-codegen --interface-prefix org.freedesktop.GeoClue2. --c-namespace GClue --generate-c-code DerivedSources/WebCore/geoclue2/Geoclue2Interface $(GEOCLUE_DBUS_INTERFACE)
> +endif

It's so strange that there's not an API to geoclue2 beyond the DBus API. Are there plans for a frontend?

[Zeeshan Ali]

(In reply to comment #37)
> (From update of attachment 225475 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225475&action=review
> 
> > Source/WebCore/GNUmakefile.am:392
> > +if GEOCLUE_API_VERSION_2
> > +DerivedSources/WebCore/geoclue2/Geoclue2Interface.c: DerivedSources/WebCore/geoclue2/Geoclue2Interface.h
> > +DerivedSources/WebCore/geoclue2/Geoclue2Interface.h:
> > +	$(AM_V_GEN)gdbus-codegen --interface-prefix org.freedesktop.GeoClue2. --c-namespace GClue --generate-c-code DerivedSources/WebCore/geoclue2/Geoclue2Interface $(GEOCLUE_DBUS_INTERFACE)
> > +endif
> 
> It's so strange that there's not an API to geoclue2 beyond the DBus API.

Keeping in mind that it has only one developer working on it and that he has other projects to maintain and develop, it is not strange at all. :)

>  Are there plans for a frontend?

https://bugs.freedesktop.org/show_bug.cgi?id=68658

[Mario Sanchez Prada]

Created attachment 225774 [details]
Patch proposal

Attaching a new patch that tries to address all the issues mentioned in this thread, in a way or another. See comments below...

(In reply to comment #33)
> Created an attachment (id=225475) [details]
> Patch Proposal
> 
> Please see attached a WIP patch proposal addressing all the comments by Carlos. I'm not asking r? yet, because there are a couple of things I need to check first:
>  - CMake build for Geoclue1 (I suspect is broken with this patch)

Indeed it was broken, but fixing this was easy. Done :)

>  - CMake build for Geoclue2 (I want to give it a try anyway :)

That was not that easy for a CMake newbie, but I think it has been properly addressed as well in this new patch (with a bit of help from Martin. Thanks!)

>  - Providing the right desktop ID to geoclue (as it seems the dummy desktop ID included with this patch won't be good enough)

I'm a bit reluctant to do "too many assumptions" from the Geoclue2 provider in terms of which kind of application is embedding WebKit, since it might be an app that does not have a .desktop file or even an app embedding other port different than WebKitGTK+, since this provider is meant to be generic enough to be shared with other platforms (e.g. EFL).

So, instead of trying the original idea commented on IRC of getting the top level window and extracting the app ID from there, in this patch I'm just providing the program name as the ID, which in many cases will match the name of the .desktop file anyway (not for the case of ephy web apps, though). It may not be the perfect solution, but I hope it will be good enough for now. At least, geoclue should have enough information now to add some GNOME apps using webkit (e.g. epiphany) to the white list, for instance.

Anyway, other ideas, comments and even personal attacks are welcome, should you had any :)

> I'll try to do that on Monday, but feel free to inspect this new patch (which I tested locally and works as the previous one) if you want to

It's Tuesday already, hope it's not too late!

Please review, thanks!

[WebKit Commit Bot]

Attachment 225774 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mario Sanchez Prada]

Created attachment 225775 [details]
Patch proposal

New patch fixing style error in the ChangeLog

[Martin Robinson]

Comment on attachment 225775 [details]
Patch proposal

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

The CMake parts look okay. Instead of adding the source files to the list conditionally, I would add them both unconditionally and guard them with a preprocessor define. I prefer GEOCLUE_API_VERSION_2 to be named WTF_USE_GEOCLUE2. The GTK+ version one is not a great example to follow. The last general comment I have is that we avoid polluting the command-line flags any further, if we can avoid it. Can you try to make this option available in the config.h file. I can show you how to do it for CMake.

> Source/Platform/GNUmakefile.am:109
> +if GEOCLUE_API_VERSION_2
> +libPlatform_la_CPPFLAGS += \
> +	$(GEOCLUE2_CFLAGS)
> +else
> +libPlatform_la_CPPFLAGS += \
> +	$(GEOCLUE_CFLAGS)
> +endif

You can use GEOCLUE_CFLAGS for both GeoClue1 and GeoClue2 and avoid this change.

> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue.h:37
> +#if defined(GEOCLUE_API_VERSION_2)
> +#include "Geoclue2Interface.h"
> +#else
>  #include <geoclue/geoclue-master.h>
>  #include <geoclue/geoclue-position.h>
> +#endif
>  #include <wtf/gobject/GRefPtr.h>
> +#if defined(GEOCLUE_API_VERSION_2)
> +#include <wtf/text/WTFString.h>
> +#endif

You should split off the conditional includes at the end of the list instead of trying to keep them alphabetical. This will make it a bit simpler here. :)

> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue2.cpp:36
> +#define GEOCLUE_BUS_NAME "org.freedesktop.GeoClue2"
> +#define GEOCLUE_MANAGER_PATH "/org/freedesktop/GeoClue2/Manager"

Please use static const char* for these.

> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue2.cpp:44
> +    GClueAccuracyLevelCountry = 1,
> +    GClueAccuracyLevelCity = 4,
> +    GClueAccuracyLevelStreet = 6,
> +    GClueAccuracyLevelExact = 8,

GClue -> GeoClue.

[Mario Sanchez Prada]

Comment on attachment 225775 [details]
Patch proposal

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

(In reply to comment #42)
> (From update of attachment 225775 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225775&action=review
> 
> The CMake parts look okay.

Thanks!

> Instead of adding the source files to the list conditionally, I would add them
> both unconditionally and guard them with a preprocessor define. I prefer
> GEOCLUE_API_VERSION_2 to be named WTF_USE_GEOCLUE2. The GTK+ version one is
> not a great example to follow.

You mean defining then both GEOCLUE_API_VERSION_1 and GEOCLUE_API_VERSION_2, making them both available to the code (so we can disable one file or the other), right?

> The last general comment I have is that we avoid polluting the command-line
> flags any further, if we can avoid it. Can you try to make this option
> available in the config.h file.

Ok, I'll do

> I can show you how to do it for CMake.

Thanks, that would be very much appreciated :)

>> Source/Platform/GNUmakefile.am:109
>> +endif
> 
> You can use GEOCLUE_CFLAGS for both GeoClue1 and GeoClue2 and avoid this change.

ok

>> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue.h:37
>> +#endif
> 
> You should split off the conditional includes at the end of the list instead of trying to keep them alphabetical. This will make it a bit simpler here. :)

ok

>> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue2.cpp:36
>> +#define GEOCLUE_MANAGER_PATH "/org/freedesktop/GeoClue2/Manager"
> 
> Please use static const char* for these.

No problem

>> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue2.cpp:44
>> +    GClueAccuracyLevelExact = 8,
> 
> GClue -> GeoClue.

ok

[Martin Robinson]

(In reply to comment #43)
> (From update of attachment 225775 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225775&action=review
> 

> You mean defining then both GEOCLUE_API_VERSION_1 and GEOCLUE_API_VERSION_2, making them both available to the code (so we can disable one file or the other), right?

I mean to use neither of those, but to define WTF_USE_GEOCLUE2. Before one file you would do something like this:

#if ENABLE(GEOLOCATION) && !USE(GEOCLUE2)

and the other this:

#if ENABLE(GEOLOCATION) && USE(GEOCLUE2)

[Mario Sanchez Prada]

(In reply to comment #44)
> (In reply to comment #43)
> > (From update of attachment 225775 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=225775&action=review
> > 
> 
> > You mean defining then both GEOCLUE_API_VERSION_1 and GEOCLUE_API_VERSION_2, making them both available to the code (so we can disable one file or the other), right?
> 
> I mean to use neither of those, but to define WTF_USE_GEOCLUE2.

Ok.

Before one file you would do something like this:
> 
> #if ENABLE(GEOLOCATION) && !USE(GEOCLUE2)
> 
> and the other this:
> 
> #if ENABLE(GEOLOCATION) && USE(GEOCLUE2)

Alright

[Mario Sanchez Prada]

Created attachment 225788 [details]
Patch proposal

New patch addressing the issues commented by Martin

[Mario Sanchez Prada]

Just a quick heads-up to mention that I just finished four full builds from scratch right now, one using autotools and the other using cmake, to check whether I broke any of the Geoclue1 or Geoclue2 building paths, and everything went OK.

Sorry not to have done this yesterday but I was in a hurry. Just mentioning it now so to state that the latest changes in the build systems are properly tested as well.

[Martin Robinson]

Comment on attachment 225788 [details]
Patch proposal

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

Looks good to me, in general, but Carlos should probably do a review as well since he's managing the stable branch.

> Source/Platform/GNUmakefile.am:16
> -	-I$(top_builddir)/DerivedSources/Platform
> +	-I$(top_builddir)/DerivedSources/Platform \
> +	-I$(top_builddir)/DerivedSources/WebCore
>  

It probably makes sense to generate the files in DerivedSources/Platform for the autotools port.

> Source/WebCore/PlatformGTK.cmake:279
> +         COMMAND gdbus-codegen --interface-prefix org.freedesktop.GeoClue2. --c-namespace GClue --generate-c-code ${DERIVED_SOURCES_WEBCORE_DIR}/Geoclue2Interface ${GEOCLUE_DBUS_INTERFACE}

So, if possible it would be could to use GeoClue as the C namespace.

> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue2.cpp:36
> +const char* GeoClueBusName = "org.freedesktop.GeoClue2";
> +const char* GeoClueManagerPath = "/org/freedesktop/GeoClue2/Manager";

These should be named like gGeoClueBusName and gGeoClueManagerPath.

> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue2.cpp:117
> +    // The provider will take ownershipt of the managerProxy.

ownershipt->ownership

[Carlos Garcia Campos]

Comment on attachment 225788 [details]
Patch proposal

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

> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue.h:28
> +#include <wtf/text/WTFString.h>

Why are you adding this? I don't see any String used by this header.

> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue2.cpp:50
> +    , m_managerProxy(0)
> +    , m_clientProxy(0)

You don't need to initialize GRefPtr, and in case of doing so, use nullptr instead of 0

> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue2.cpp:118
> +    provider->setGeoclueManagerProxyAndGetClient(managerProxy);

The idea of using static memebers for callbacks was also to simplify this, and clarify the ownership. I think now you can directly here do 

provider->m_managerProxy = adoptGRef(gclue_manager_proxy_new_for_bus_finish(result, &error.outPtr()));
if (error) {
    provider->errorOccurred(error->message);
    return;
}

gclue_manager_call_get_client(provider->m_managerProxy.get(), nullptr, reinterpret_cast<GAsyncReadyCallback>(getGeoclueClientCallback), provider);

no? we make the code simpler and easier to follow, avoiding jumping up and down all the time. Another approach would be the opposite, move all the handling to the object itself, leaving the callbacks with a single line, for example:

provider->handleGeoclueManagerProxyCreated(result);

> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue2.cpp:162
> +    // Geoclue2 requires the client to provide a desktop ID for security
> +    // reasons, which should identify the application requesting the location.
> +    CString programName = getCurrentExecutablePath();
> +    gclue_client_set_desktop_id(m_clientProxy.get(), basename(programName.data()));

So, I guess the program name works as desktop file id in the end? or is it just a workaround? Wouldn't it be easier to directly use g_get_prgname() that returns a const char *?

[Mario Sanchez Prada]

Created attachment 225978 [details]
Patch proposal

Thanks Martin and Carlos for the reviews. I'm attaching a new patch addressing all those issues.

See some comments of mine below...

(In reply to comment #48)
> (From update of attachment 225788 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225788&action=review
> 
> Looks good to me, in general, but Carlos should probably do a review as well since he's managing the stable branch.

Sure.

> > Source/Platform/GNUmakefile.am:16
> > -	-I$(top_builddir)/DerivedSources/Platform
> > +	-I$(top_builddir)/DerivedSources/Platform \
> > +	-I$(top_builddir)/DerivedSources/WebCore
> >  
> 
> It probably makes sense to generate the files in DerivedSources/Platform for the autotools port.
> 
Done.

> > Source/WebCore/PlatformGTK.cmake:279
> > +         COMMAND gdbus-codegen --interface-prefix org.freedesktop.GeoClue2. --c-namespace GClue --generate-c-code ${DERIVED_SOURCES_WEBCORE_DIR}/Geoclue2Interface ${GEOCLUE_DBUS_INTERFACE}
> 
> So, if possible it would be could to use GeoClue as the C namespace.
> 
Might I suggest changing "GClue" to "Geoclue" (lower case "c") instead of to "GeoClue" here?

The reason why I suggest that is to keep the signature of the generated methods more similar to the Geoclue1 counterpart, having things like geoclue_location_get_latitude() instead of geo_clue_location_get_latitude(). 

In the new patch I did that ("Geoclue"), but I can change it to GeoClue if the naming of the generated methods is not an issue.

> > Source/WebCore/platform/geoclue/GeolocationProviderGeoclue2.cpp:36
> > +const char* GeoClueBusName = "org.freedesktop.GeoClue2";
> > +const char* GeoClueManagerPath = "/org/freedesktop/GeoClue2/Manager";
> 
> These should be named like gGeoClueBusName and gGeoClueManagerPath.
> 
> > Source/WebCore/platform/geoclue/GeolocationProviderGeoclue2.cpp:117
> > +    // The provider will take ownershipt of the managerProxy.
> 
> ownershipt->ownership

Done.

(In reply to comment #49)
> (From update of attachment 225788 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225788&action=review
> 
> > Source/WebCore/platform/geoclue/GeolocationProviderGeoclue.h:28
> > +#include <wtf/text/WTFString.h>
> 
> Why are you adding this? I don't see any String used by this header.
> 
It was in the original patch from Anton and forgot to remove it.

Now removed.

> > Source/WebCore/platform/geoclue/GeolocationProviderGeoclue2.cpp:50
> > +    , m_managerProxy(0)
> > +    , m_clientProxy(0)
> 
> You don't need to initialize GRefPtr, and in case of doing so, use nullptr instead of 0
> 

Removed those 2 lines.

> > Source/WebCore/platform/geoclue/GeolocationProviderGeoclue2.cpp:118
> > +    provider->setGeoclueManagerProxyAndGetClient(managerProxy);
> 
> The idea of using static memebers for callbacks was also to simplify this, and clarify the ownership. I think now you can directly here do 
> 
> provider->m_managerProxy = adoptGRef(gclue_manager_proxy_new_for_bus_finish(result, &error.outPtr()));
> if (error) {
>     provider->errorOccurred(error->message);
>     return;
> }
> 
> gclue_manager_call_get_client(provider->m_managerProxy.get(), nullptr, reinterpret_cast<GAsyncReadyCallback>(getGeoclueClientCallback), provider);
> 
> no? we make the code simpler and easier to follow, avoiding jumping up and down all the time. 

Done (changed the code to implement the approach you described here)

> Another approach would be the opposite, move all the handling to the object itself, leaving the callbacks with a single line, for example:
> 
> provider->handleGeoclueManagerProxyCreated(result);
> 

Nah. The other approach sounds good to me as well (and avoids polluting the header file)

> > Source/WebCore/platform/geoclue/GeolocationProviderGeoclue2.cpp:162
> > +    // Geoclue2 requires the client to provide a desktop ID for security
> > +    // reasons, which should identify the application requesting the location.
> > +    CString programName = getCurrentExecutablePath();
> > +    gclue_client_set_desktop_id(m_clientProxy.get(), basename(programName.data()));
> 
> So, I guess the program name works as desktop file id in the end? or is it just a workaround? Wouldn't it be easier to directly use g_get_prgname() that returns a const char *?

For some reason the compiler complained last time I tried to use that one and so I ended up using this getCurrentExecutablePath() from WTF. But I've tried now again and got no issue at all (I might have misread the error output previously), so I replaced those two lines with g_get_prgname(). Done.

[Mario Sanchez Prada]

Created attachment 226005 [details]
Patch proposal

New patch fixing a mistake in Source/WebCore/GNUmakefile.list.am that was causing trouble when building with build-webkit --gtk (it was fine using ./autogen.sh && make).

AS much as I hate rushing people for reviews, I would appreciate if you (Martin & Carlos) could take another look soon, since tomorrow is Friday and next release is approaching :)

Thanks!

[Build Bot]

Comment on attachment 226005 [details]
Patch proposal

Attachment 226005 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6437091126804480

New failing tests:
fast/text/selection-rect-line-height-too-big.html
editing/selection/inline-closest-leaf-child.html
fast/text/selection-rect-line-height-too-small.html
fast/inline-block/14498-positionForCoordinates.html

[Build Bot]

Created attachment 226010 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 226005 [details]
Patch proposal

Attachment 226005 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5006350815854592

New failing tests:
fast/text/selection-rect-line-height-too-big.html
editing/selection/inline-closest-leaf-child.html
fast/text/selection-rect-line-height-too-small.html
fast/inline-block/14498-positionForCoordinates.html

[Build Bot]

Created attachment 226016 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 226005 [details]
Patch proposal

Attachment 226005 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5914514671271936

New failing tests:
fast/text/selection-rect-line-height-too-big.html
editing/selection/inline-closest-leaf-child.html
fast/text/selection-rect-line-height-too-small.html
fast/inline-block/14498-positionForCoordinates.html

[Build Bot]

Created attachment 226024 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Mario Sanchez Prada]

Created attachment 226084 [details]
Patch proposal

(In reply to comment #51)
> Created an attachment (id=226005) [details]
> Patch proposal
> 
> New patch fixing a mistake in Source/WebCore/GNUmakefile.list.am that was causing trouble when building with build-webkit --gtk (it was fine using ./autogen.sh && make).
> 
> AS much as I hate rushing people for reviews, I would appreciate if you (Martin & Carlos) could take another look soon, since tomorrow is Friday and next release is approaching :)
> 
> Thanks!

Argh.. It seems I uploaded the wrong patch by mistake. Now uploading the right one, sorry for the hassle.

[Carlos Garcia Campos]

Comment on attachment 226084 [details]
Patch proposal

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

> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue.h:28
> +#include <wtf/text/WTFString.h>

Please, read my previous review, or am I wrong and String is actually used by this header?

> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue.h:55
> +    void setGeoclueManagerProxyAndGetClient(GeoclueManager*);
> +    void setGeoclueClientProxyAndStart(GeoclueClient*);

Are these methods implemented somewhere?

> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue2.cpp:142
> +    // Geoclue2 requires the client to provide a desktop ID for security
> +    // reasons, which should identify the application requesting the location.
> +    geoclue_client_set_desktop_id(provider->m_clientProxy.get(), g_get_prgname());

I still don't know whether using the program name is the right thing in the end or a workaround. If it's a workaround, please file a bug report and add a FIXME here pointing to the bug report. Because at the moment your comment is confusing here, it says geoclue requires a desktop ID, but we are providing an application name

[Mario Sanchez Prada]

(In reply to comment #59)
> (From update of attachment 226084 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226084&action=review
> 
> > Source/WebCore/platform/geoclue/GeolocationProviderGeoclue.h:28
> > +#include <wtf/text/WTFString.h>
> 

I'd swear I removed those, but must forgot to add the change to the commit.

> Please, read my previous review, or am I wrong and String is actually used by this header?
> 
> > Source/WebCore/platform/geoclue/GeolocationProviderGeoclue.h:55
> > +    void setGeoclueManagerProxyAndGetClient(GeoclueManager*);
> > +    void setGeoclueClientProxyAndStart(GeoclueClient*);
> 
> Are these methods implemented somewhere?
> 
Nope, probably the same mistake creating the patch.

> > Source/WebCore/platform/geoclue/GeolocationProviderGeoclue2.cpp:142
> > +    // Geoclue2 requires the client to provide a desktop ID for security
> > +    // reasons, which should identify the application requesting the location.
> > +    geoclue_client_set_desktop_id(provider->m_clientProxy.get(), g_get_prgname());
> 
> I still don't know whether using the program name is the right thing in the end or a workaround.
> If it's a workaround, please file a bug report and add a FIXME here pointing to the bug report.
> Because at the moment your comment is confusing here, it says geoclue requires a desktop ID,
> but we are providing an application name

In theory we should provide the "application name" as the desktop ID for geoclue to be able to handle, where "application name" is typically the name of the .desktop file (if any), or what you get from calling g_application_get_application_id(). 

The problem is that knowing that information from this point seems to be tricky. We couldgo the route of finding the top level GtkWindow and extracting the application ID from there, but then you would be introducing a dependency on GTK here, which I'm not sure that's what we want as, for instance, this provider might be used from other platforms too (e.g EFL).

Other option could be to provide specific API (not now) for the application to specify an "application ID" that webkit might use for different purposes, if present, such as to pass it through geoclue, or even to let the application willing to use WebKit with geolocation capabilities to talk directly to Geoclue to let it know the application ID.

Unfortunately, it is not clear to me which one would be the best option, but introducing the dependency on GTK seems to be the weirdest one, so in the end I preferred to go with g_get_prgname() for now because (1) in many cases it will provide the same string than the GNOME application name and (2) because it should be enough, at least for now, for geoclue to decide whether to whitelist some trusted apps known to use WebKitGTK+, such as epiphany.

So, in a nutshell, whether we should file a bug or not is still not clear to me, but sure I can at list put a FIXME in the code to reflect this situation, so we don't forget about it and file a bug once we agree on whether it's needed or not.

I'll update the patch now and upload a new version soon. Thanks for the review

[Mario Sanchez Prada]

Created attachment 226115 [details]
Patch proposal

Removed the include and the unused functions

[Carlos Garcia Campos]

(In reply to comment #60)
> > I still don't know whether using the program name is the right thing in the end or a workaround.
> > If it's a workaround, please file a bug report and add a FIXME here pointing to the bug report.
> > Because at the moment your comment is confusing here, it says geoclue requires a desktop ID,
> > but we are providing an application name
> 
> In theory we should provide the "application name" as the desktop ID for geoclue to be able to handle, where "application name" is typically the name of the .desktop file (if any), or what you get from calling g_application_get_application_id(). 

typically

> The problem is that knowing that information from this point seems to be tricky. We couldgo the route of finding the top level GtkWindow and extracting the application ID from there, but then you would be introducing a dependency on GTK here, which I'm not sure that's what we want as, for instance, this provider might be used from other platforms too (e.g EFL).

Why would that introduce any dependency? We could have a method in WebCore platform to get the application name that GTK+ and EFL can implement. But in any case, I'm not concerned about the actual solution at the moment, I'm just asking whether this is a workaround to have a bug filed and a FIXME comment here so that we don't forget about this.

[Mario Sanchez Prada]

(In reply to comment #62)
> [...]
> > The problem is that knowing that information from this point seems to be tricky. We couldgo the route of finding the top level GtkWindow and extracting the application ID from there, but then you would be introducing a dependency on GTK here, which I'm not sure that's what we want as, for instance, this provider might be used from other platforms too (e.g EFL).
> 
> Why would that introduce any dependency? We could have a method in WebCore platform to get the application name that GTK+ and EFL can implement. 

Yes, we could do that. That's right.

> But in any case, I'm not concerned about the actual solution at the moment, I'm just asking whether this is a workaround to have a bug filed and a FIXME comment here so that we don't forget about this.

As I said before, I can not really answer this question because I'm not 100% sure whether that proposal of using g_application_get_application_id() for the desktop ID would be good enough for Geoclue. I think we need Zeeshan input once again at this point to clarify that. If he confirms that would be Ok, I'd be happy to file a bug and put the link in the FIXME here.

[Zeeshan Ali]

(In reply to comment #63)
> (In reply to comment #62)
> > [...]
> > > The problem is that knowing that information from this point seems to be tricky. We couldgo the route of finding the top level GtkWindow and extracting the application ID from there, but then you would be introducing a dependency on GTK here, which I'm not sure that's what we want as, for instance, this provider might be used from other platforms too (e.g EFL).
> > 
> > Why would that introduce any dependency? We could have a method in WebCore platform to get the application name that GTK+ and EFL can implement. 
> 
> Yes, we could do that. That's right.
> 
> > But in any case, I'm not concerned about the actual solution at the moment, I'm just asking whether this is a workaround to have a bug filed and a FIXME comment here so that we don't forget about this.
> 
> As I said before, I can not really answer this question because I'm not 100% sure whether that proposal of using g_application_get_application_id() for the desktop ID would be good enough for Geoclue. I think we need Zeeshan input once again at this point to clarify that. If he confirms that would be Ok, I'd be happy to file a bug and put the link in the FIXME here.

So here is the thing: Geoclue will expect you (the app) to provide the desktop ID: i-e the name of your desktop file without the '.desktop' extension. Now both get_prgname() and g_application_get_application_id() are fine as long as  you get the same value from them as the desktop ID from all your apps.

Having said that, I think g_application_get_application_id() is the better choice here. AFAIK, that is what most modern glib apps set correctly and in fact set this to same value as their desktop file name. So instead of having to add new webkit api, you can simply document that apps should ensure that they set this and that it should be the same as their desktop file. AFAIK existing apps wont' even have to change anything, right?

[Carlos Garcia Campos]

(In reply to comment #64)
> (In reply to comment #63)
> > (In reply to comment #62)
> > > [...]
> > > > The problem is that knowing that information from this point seems to be tricky. We couldgo the route of finding the top level GtkWindow and extracting the application ID from there, but then you would be introducing a dependency on GTK here, which I'm not sure that's what we want as, for instance, this provider might be used from other platforms too (e.g EFL).
> > > 
> > > Why would that introduce any dependency? We could have a method in WebCore platform to get the application name that GTK+ and EFL can implement. 
> > 
> > Yes, we could do that. That's right.
> > 
> > > But in any case, I'm not concerned about the actual solution at the moment, I'm just asking whether this is a workaround to have a bug filed and a FIXME comment here so that we don't forget about this.
> > 
> > As I said before, I can not really answer this question because I'm not 100% sure whether that proposal of using g_application_get_application_id() for the desktop ID would be good enough for Geoclue. I think we need Zeeshan input once again at this point to clarify that. If he confirms that would be Ok, I'd be happy to file a bug and put the link in the FIXME here.
> 
> So here is the thing: Geoclue will expect you (the app)

This is the main problem we are not an app.

> to provide the desktop ID: i-e the name of your desktop file without the '.desktop' extension. Now both get_prgname() and g_application_get_application_id() are fine as long as  you get the same value from them as the desktop ID from all your apps.

So, it's indeed a workaround.

> Having said that, I think g_application_get_application_id() is the better choice here. AFAIK, that is what most modern glib apps set correctly and in fact set this to same value as their desktop file name. So instead of having to add new webkit api, you can simply document that apps should ensure that they set this and that it should be the same as their desktop file. AFAIK existing apps wont' even have to change anything, right?

We can't expect all apps using webkit being a GApplication. We could use the program name as fallback when the toplevel window is not a GtkApplicationWindow, of course, but this still sounds to me like a workaround.

[Mario Sanchez Prada]

Created attachment 226123 [details]
Patch proposal

New patch adding the newly reported bug together with the FIXME

[Zeeshan Ali]

(In reply to comment #65)
> (In reply to comment #64)
> > (In reply to comment #63)
> > > (In reply to comment #62)
> > > > [...]
> > > > > The problem is that knowing that information from this point seems to be tricky. We couldgo the route of finding the top level GtkWindow and extracting the application ID from there, but then you would be introducing a dependency on GTK here, which I'm not sure that's what we want as, for instance, this provider might be used from other platforms too (e.g EFL).
> > > > 
> > > > Why would that introduce any dependency? We could have a method in WebCore platform to get the application name that GTK+ and EFL can implement. 
> > > 
> > > Yes, we could do that. That's right.
> > > 
> > > > But in any case, I'm not concerned about the actual solution at the moment, I'm just asking whether this is a workaround to have a bug filed and a FIXME comment here so that we don't forget about this.
> > > 
> > > As I said before, I can not really answer this question because I'm not 100% sure whether that proposal of using g_application_get_application_id() for the desktop ID would be good enough for Geoclue. I think we need Zeeshan input once again at this point to clarify that. If he confirms that would be Ok, I'd be happy to file a bug and put the link in the FIXME here.
> > 
> > So here is the thing: Geoclue will expect you (the app)
> 
> This is the main problem we are not an app.

I know that. :)

> > to provide the desktop ID: i-e the name of your desktop file without the '.desktop' extension. Now both get_prgname() and g_application_get_application_id() are fine as long as  you get the same value from them as the desktop ID from all your apps.
> 
> So, it's indeed a workaround.

If you don't mandate your apps to set_prgname() or g_application_set_application_id() to desktop id, indeed it is.

> > Having said that, I think g_application_get_application_id() is the better choice here. AFAIK, that is what most modern glib apps set correctly and in fact set this to same value as their desktop file name. So instead of having to add new webkit api, you can simply document that apps should ensure that they set this and that it should be the same as their desktop file. AFAIK existing apps wont' even have to change anything, right?
> 
> We can't expect all apps using webkit being a GApplication. We could use the program name as fallback when the toplevel window is not a GtkApplicationWindow, of course, but this still sounds to me like a workaround.

You can give them a choice, if they don't use GApplication, they just have to use g_set_prgname() instead. I don't think that is too much to ask.

[Mario Sanchez Prada]

Created attachment 226260 [details]
Patch proposal

New patch fixing an issue in FindGeoClue2.cmake

[Carlos Garcia Campos]

Comment on attachment 226260 [details]
Patch proposal

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

> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue1.cpp:79
> +    , m_geoclueClient(0)
> +    , m_geocluePosition(0)

Remove these ones here too, or use nullptr.

[Mario Sanchez Prada]

Created attachment 226296 [details]
Patch proposal

New patch addressing the last comments from Carlos

[Carlos Garcia Campos]

Comment on attachment 226296 [details]
Patch proposal

Again

[Mario Sanchez Prada]

Comment on attachment 226296 [details]
Patch proposal

Thanks for the review

[WebKit Commit Bot]

Comment on attachment 226296 [details]
Patch proposal

Clearing flags on attachment: 226296

Committed r165418: <http://trac.webkit.org/changeset/165418>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.