desktop-packages team mailing list archive
-
desktop-packages team
-
Mailing list archive
-
Message #96619
[Bug 1100326] Re: Location requested by websites should be able to use GPS/mobile positioning
Launchpad has imported 75 comments from the remote bug at
https://bugs.webkit.org/show_bug.cgi?id=120185.
If you reply to an imported comment from within Launchpad, your comment
will be sent to the remote bug automatically. Read more about
Launchpad's inter-bugtracker facilities at
https://help.launchpad.net/InterBugTracking.
------------------------------------------------------------------------
On 2013-08-23T00:28:55+00:00 Zeeshan Ali wrote:
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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/27
------------------------------------------------------------------------
On 2013-08-23T08:23:55+00:00 Zan-f wrote:
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/
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/28
------------------------------------------------------------------------
On 2013-08-23T11:52:32+00:00 Zeeshan Ali wrote:
(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/
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/29
------------------------------------------------------------------------
On 2013-08-23T11:54:53+00:00 Zan-f wrote:
(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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/30
------------------------------------------------------------------------
On 2013-09-03T17:00:44+00:00 A-obzhirov wrote:
Started working on geoclue2 provider.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/34
------------------------------------------------------------------------
On 2013-09-03T17:05:22+00:00 Zeeshan Ali wrote:
(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
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/35
------------------------------------------------------------------------
On 2013-09-03T17:06:06+00:00 Zeeshan Ali wrote:
(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. :)
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/36
------------------------------------------------------------------------
On 2013-09-10T15:54:03+00:00 A-obzhirov wrote:
(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?
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/37
------------------------------------------------------------------------
On 2013-09-10T16:11:13+00:00 Zeeshan Ali wrote:
(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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/38
------------------------------------------------------------------------
On 2013-09-11T09:09:02+00:00 A-obzhirov wrote:
(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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/39
------------------------------------------------------------------------
On 2013-09-11T12:31:18+00:00 Zeeshan Ali wrote:
(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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/40
------------------------------------------------------------------------
On 2013-09-11T14:25:18+00:00 A-obzhirov wrote:
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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/41
------------------------------------------------------------------------
On 2013-09-13T13:38:50+00:00 A-obzhirov wrote:
(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?
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/42
------------------------------------------------------------------------
On 2013-09-13T13:39:39+00:00 A-obzhirov wrote:
What's you IRC account BTW?
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/43
------------------------------------------------------------------------
On 2013-09-13T13:51:50+00:00 Zeeshan Ali wrote:
(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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/44
------------------------------------------------------------------------
On 2013-09-13T14:33:40+00:00 Zeeshan Ali wrote:
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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/45
------------------------------------------------------------------------
On 2013-09-20T16:22:11+00:00 A-obzhirov wrote:
Created attachment 212180
Patch
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/46
------------------------------------------------------------------------
On 2013-09-20T16:24:33+00:00 Commit-queue wrote:
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
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/47
------------------------------------------------------------------------
On 2013-09-23T10:08:55+00:00 I-mario wrote:
Comment on attachment 212180
Patch
View in context:
https://bugs.webkit.org/attachment.cgi?id=212180&action=review
Thanks for the patch, Anton. It looks quite well already, but there's
definitely more work to do (e.g. tests) so setting r- for now.
Also, I really think some more people familiar with these part of the
code should review it, specially when it comes to how to incorporate
this into trunk.
> Source/Platform/ChangeLog:8
> +
> + * GNUmakefile.am:
You need at least a short description for the change here. Same
consideration applies to the other ChangeLogs
> Source/WebCore/GNUmakefile.am:375
> +# Geoclue interface
> +DerivedSources/geoclue2/geoclue-interface.c: DerivedSources/geoclue2/geoclue-interface.h
> +DerivedSources/geoclue2/geoclue-interface.h:
> + $(AM_V_GEN)gdbus-codegen --interface-prefix org.freedesktop.GeoClue2. --c-namespace GClue --generate-c-code DerivedSources/geoclue2/geoclue-interface $(GEOCLUE_DBUS_INTERFACE)
> +endif
I'd rather use "Geoclue2" as the generated namespace instead of the cut-
down version "GClue".
Anyway, I'm not sure enough about this bit since I feel like I lack some
information to provide a proper review. Thus, I'd rather have someone
else reviewing the changes in this file.
> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:66
> +void GeolocationProviderGeoclue2::start()
I would rename this function to something more descriptive, such as
startGeoclueClient or startGeoclueSubsystem
> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:72
> + if (!manager())
> + return;
> +
> + if (!client())
> + return;
You probably can combine this two in one if
> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:74
> + // Set the requirement for the client
Missing period.
> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:76
> + gclue_client_call_start(m_clientProxy.get(), 0, onStart, this);
What happens here if m_clientProxy is not valid? Shouldn't we control
that situation if that's a possibility or just ASSERT for non-null
otherwise?
Also, "onStart" looks like to me like a too simple name for the
callback. What about something like geoclueClientStartCallback?
> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:79
> +void GeolocationProviderGeoclue2::stop()
I would also rename this function to something more descriptive, such as
stopGeoclueClient or stopGeoclueSubsystem
> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:85
> + m_locationProxy.clear();
> + m_clientProxy.clear();
Shouldn't you better do this on a callback for gclue_client_call_stop?
> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:108
> +void GeolocationProviderGeoclue2::onManagerProxyReady(GObject *sourceObject, GAsyncResult *result, void* self)
Wrong placement for *
> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:110
> + GOwnPtr<GError> error;
Define this variable right before you need it (after the if)
> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:117
> + provider->errorOccurred(String("Failed to create Geoclue manager: ") + error->message);
You can use String::format() for this, or the StringBuilder class if
you're planning to concatenate many things (which does not seem to be
the case)
> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:124
> +GClueManager* GeolocationProviderGeoclue2::manager()
What about using geoclueManager() as the name?
> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:130
> + gclue_manager_proxy_new_for_bus(G_BUS_TYPE_SYSTEM, G_DBUS_PROXY_FLAGS_NONE, GEOCLUE_BUS_NAME, GEOCLUE_MANAGER_PATH,
> + 0, onManagerProxyReady, this);
You can make this one a single line
> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:137
> + GOwnPtr<GError> error;
> + GOwnPtr<gchar> path;
Move this definitions down, right before you need it.
> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:143
> + provider->errorOccurred(String("Failed to get client: ") + error->message);
String::format()
> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:148
> + gclue_client_proxy_new_for_bus(G_BUS_TYPE_SYSTEM, G_DBUS_PROXY_FLAGS_NONE, GEOCLUE_BUS_NAME, path.get(),
> + 0, onClientProxyReady, self);
One line
> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:151
> +GClueClient* GeolocationProviderGeoclue2::client()
What about using geoclueClient() as the name?
> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:158
> + gclue_manager_call_get_client(m_managerProxy.get(), 0, onGetClientReady, this);
For the sake of consistency, I would call the callback used here this
something like geoclueManagerGetClientCallback
> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:162
> +void GeolocationProviderGeoclue2::onClientProxyReady(GObject *sourceObject, GAsyncResult *result, void* self)
Wrong placement for *
> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:164
> + GOwnPtr<GError> error;
Move this definition down
> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:171
> + provider->errorOccurred(String("Failed to get client proxy: ") + error->message);
String::format
> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:176
> + g_signal_connect(provider->m_clientProxy.get(), "location-updated",
> + G_CALLBACK(onLocationUpdated), self);
One line
> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:181
> +void GeolocationProviderGeoclue2::onLocationUpdated(GClueClient *client, const gchar* oldPath, const gchar* newPath, void* self)
Wrong placement for *
> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:188
> + gclue_location_proxy_new_for_bus(G_BUS_TYPE_SYSTEM, G_DBUS_PROXY_FLAGS_NONE, GEOCLUE_BUS_NAME, newPath,
> + 0, onLocationProxyReady, self);
One line. And again, I would suggest another name for the callback. What
about createGeoclueLocationProxyCallback?
> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:193
> + GOwnPtr<GError> error;
Move this down
> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:199
> + provider->errorOccurred(String("Failed to start: ") + error->message);
String::format
> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:202
> +void GeolocationProviderGeoclue2::onLocationProxyReady(GObject *sourceObject, GAsyncResult *result, void* self)
Wrong placement for *
> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:204
> + GOwnPtr<GError> error;
Move this down
> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:211
> + provider->errorOccurred(String("Failed to create location proxy: ") + error->message);
String::format
> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:221
> +void GeolocationProviderGeoclue2::updateClientRequirements()
> +{
> + // FIXME: implement when accuracy API is available
> +}
If this is is an optional feature, I'd rather leave this function out of
the patch. However, according to the W3C spec, accuracy does not seem
like optional:
"6.2.2 The Geolocation API must provide information about the accuracy
of the retrieved location data."
In any case, later in this patch you use a call to
gclue_location_get_accuracy() so I'm not entirely sure about what you
mean with this FIXME here
> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.h:31
> +#define GEOCLUE_BUS_NAME "org.freedesktop.GeoClue2"
> +#define GEOCLUE_MANAGER_PATH "/org/freedesktop/GeoClue2/Manager"
You don't need this in the header file, just in the implementation file
> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.h:50
> + GeolocationProviderGeoclue2Client* m_client;
I would move this attribute declaration down after the private
functions, together with the rest of private attributes
> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.h:57
> + static void onManagerProxyReady(GObject*, GAsyncResult*, void*);
> + static void onGetClientReady(GObject*, GAsyncResult*, void*);
> + static void onClientProxyReady(GObject*, GAsyncResult*, void*);
> + static void onStart(GObject*, GAsyncResult*, void*);
> + static void onStop(GObject*, GAsyncResult*, void*);
> + static void onLocationUpdated(GClueClient*, const gchar*, const gchar*, void*);
> + static void onLocationProxyReady(GObject*, GAsyncResult*, void*);
You do not need to make this callbacks static members of this class,
just static functions in the implementation file (C-style) or, even
better, private functions in the implementation file members of an
unnamed namespace
> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.h:47
> +#if !defined(GEOCLUE_API_VERSION_2)
> namespace WebCore {
> class Geolocation;
> +typedef GeolocationProviderGeoclueClient GeolocationProviderClient;
> +}
> +#else
> +namespace WebCore {
> +typedef GeolocationProviderGeoclue2Client GeolocationProviderClient;
> }
> +#endif
I think you still want the forward class declaration of Geolocation if
using geoclue2, meaning that the only difference is the typedef.
What about something like this:
namespace WebCore {
class Geolocation;
#if !defined(GEOCLUE_API_VERSION_2)
typedef GeolocationProviderGeoclueClient GeolocationProviderClient;
#else
typedef GeolocationProviderGeoclue2Client GeolocationProviderClient;
#endif
}
> Tools/gtk/jhbuild.modules:33
> + <dep package="json-glib"/>
> + <dep package="geoclue"/>
This should be moved to the list of optional modules, at least while
it's meant to be an experimental thing.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/48
------------------------------------------------------------------------
On 2013-12-11T10:56:40+00:00 A-obzhirov wrote:
Comment on attachment 212180
Patch
View in context:
https://bugs.webkit.org/attachment.cgi?id=212180&action=review
I was wondering about the testing. I should be able to use Geolocation
layout tests. Also I should probably write new API tests. The main
problem for me that I am not sure how to setup system D-BUS GeoClue2
service for the testing. May be I can develop some fake GeoClue2 D-BUS
interface. How do you usually handle testing requiring specific D-BUS
services?
>> Source/Platform/ChangeLog:8
>> + * GNUmakefile.am:
>
> You need at least a short description for the change here. Same consideration applies to the other ChangeLogs
yes
>> Source/WebCore/GNUmakefile.am:375
>> +endif
>
> I'd rather use "Geoclue2" as the generated namespace instead of the cut-down version "GClue".
>
> Anyway, I'm not sure enough about this bit since I feel like I lack some information to provide a proper review. Thus, I'd rather have someone else reviewing the changes in this file.
Can be done. I need to check latest version of the API though first.
>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:66
>> +void GeolocationProviderGeoclue2::start()
>
> I would rename this function to something more descriptive, such as startGeoclueClient or startGeoclueSubsystem
ok
>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:72
>> + return;
>
> You probably can combine this two in one if
ok
>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:74
>> + // Set the requirement for the client
>
> Missing period.
ok
>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:76
>> + gclue_client_call_start(m_clientProxy.get(), 0, onStart, this);
>
> What happens here if m_clientProxy is not valid? Shouldn't we control that situation if that's a possibility or just ASSERT for non-null otherwise?
>
> Also, "onStart" looks like to me like a too simple name for the callback. What about something like geoclueClientStartCallback?
It will be valid here since "if (!client()) return statement" will
ensure that m_clientProxy is not null. But ASSERT can be added here
anyway.
>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:79
>> +void GeolocationProviderGeoclue2::stop()
>
> I would also rename this function to something more descriptive, such as stopGeoclueClient or stopGeoclueSubsystem
ok
>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:85
>> + m_clientProxy.clear();
>
> Shouldn't you better do this on a callback for gclue_client_call_stop?
void GeolocationProviderGeoclue2::stop() is synchronous so I use
synchronous gclue_client_call_stop here and I would prefer to do cleanup
synchronously since it should reduce possible mess with start/stop
calls.
>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:108
>> +void GeolocationProviderGeoclue2::onManagerProxyReady(GObject *sourceObject, GAsyncResult *result, void* self)
>
> Wrong placement for *
ok
>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:110
>> + GOwnPtr<GError> error;
>
> Define this variable right before you need it (after the if)
ok
>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:117
>> + provider->errorOccurred(String("Failed to create Geoclue manager: ") + error->message);
>
> You can use String::format() for this, or the StringBuilder class if you're planning to concatenate many things (which does not seem to be the case)
ok
>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:124
>> +GClueManager* GeolocationProviderGeoclue2::manager()
>
> What about using geoclueManager() as the name?
ok
>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:137
>> + GOwnPtr<gchar> path;
>
> Move this definitions down, right before you need it.
ok
>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:143
>> + provider->errorOccurred(String("Failed to get client: ") + error->message);
>
> String::format()
ok
>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:148
>> + 0, onClientProxyReady, self);
>
> One line
ok
>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:151
>> +GClueClient* GeolocationProviderGeoclue2::client()
>
> What about using geoclueClient() as the name?
ok
>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:158
>> + gclue_manager_call_get_client(m_managerProxy.get(), 0, onGetClientReady, this);
>
> For the sake of consistency, I would call the callback used here this something like geoclueManagerGetClientCallback
ok
>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:162
>> +void GeolocationProviderGeoclue2::onClientProxyReady(GObject *sourceObject, GAsyncResult *result, void* self)
>
> Wrong placement for *
ok
>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:164
>> + GOwnPtr<GError> error;
>
> Move this definition down
ok
>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:171
>> + provider->errorOccurred(String("Failed to get client proxy: ") + error->message);
>
> String::format
ok
>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:176
>> + G_CALLBACK(onLocationUpdated), self);
>
> One line
ok
>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:181
>> +void GeolocationProviderGeoclue2::onLocationUpdated(GClueClient *client, const gchar* oldPath, const gchar* newPath, void* self)
>
> Wrong placement for *
ok
>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:188
>> + 0, onLocationProxyReady, self);
>
> One line. And again, I would suggest another name for the callback. What about createGeoclueLocationProxyCallback?
ok
>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:193
>> + GOwnPtr<GError> error;
>
> Move this down
ok
>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:202
>> +void GeolocationProviderGeoclue2::onLocationProxyReady(GObject *sourceObject, GAsyncResult *result, void* self)
>
> Wrong placement for *
ok
>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:204
>> + GOwnPtr<GError> error;
>
> Move this down
ok
>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:211
>> + provider->errorOccurred(String("Failed to create location proxy: ") + error->message);
>
> String::format
ok
>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.cpp:221
>> +}
>
> If this is is an optional feature, I'd rather leave this function out of the patch. However, according to the W3C spec, accuracy does not seem like optional:
>
> "6.2.2 The Geolocation API must provide information about the accuracy of the retrieved location data."
>
> In any case, later in this patch you use a call to gclue_location_get_accuracy() so I'm not entirely sure about what you mean with this FIXME here
I need to check the latest version of GeoClue2 API, because the API
wasn't available when I wrote this code.
>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.h:31
>> +#define GEOCLUE_MANAGER_PATH "/org/freedesktop/GeoClue2/Manager"
>
> You don't need this in the header file, just in the implementation file
ok
>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.h:50
>> + GeolocationProviderGeoclue2Client* m_client;
>
> I would move this attribute declaration down after the private functions, together with the rest of private attributes
ok
>> Source/WebCore/platform/geoclue2/GeolocationProviderGeoclue2.h:57
>> + static void onLocationProxyReady(GObject*, GAsyncResult*, void*);
>
> You do not need to make this callbacks static members of this class, just static functions in the implementation file (C-style) or, even better, private functions in the implementation file members of an unnamed namespace
Hm, I need to think about it :) Static member function allows access to
private attributes of the class.
>> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.h:47
>> +#endif
>
> I think you still want the forward class declaration of Geolocation if using geoclue2, meaning that the only difference is the typedef.
>
> What about something like this:
>
> namespace WebCore {
> class Geolocation;
>
> #if !defined(GEOCLUE_API_VERSION_2)
> typedef GeolocationProviderGeoclueClient GeolocationProviderClient;
> #else
> typedef GeolocationProviderGeoclue2Client GeolocationProviderClient;
> #endif
>
> }
Let me try.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/49
------------------------------------------------------------------------
On 2014-01-28T19:13:37+00:00 Zeeshan Ali wrote:
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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/50
------------------------------------------------------------------------
On 2014-01-29T10:02:55+00:00 A-obzhirov wrote:
(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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/52
------------------------------------------------------------------------
On 2014-02-03T16:23:24+00:00 Emilio Pozuelo Monfort wrote:
(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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/53
------------------------------------------------------------------------
On 2014-02-04T07:26:49+00:00 Cgarcia-f wrote:
(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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/54
------------------------------------------------------------------------
On 2014-02-04T10:02:46+00:00 A-obzhirov wrote:
Guys, I got :) Will try to do it as soon as possible - will try to find
sometime at the end of the week.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/55
------------------------------------------------------------------------
On 2014-02-17T13:49:50+00:00 Zeeshan Ali wrote:
(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
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/56
------------------------------------------------------------------------
On 2014-02-27T18:23:59+00:00 I-mario wrote:
Created attachment 225392
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!
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/57
------------------------------------------------------------------------
On 2014-02-27T18:31:15+00:00 Mrobinson-d wrote:
(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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/58
------------------------------------------------------------------------
On 2014-02-27T18:40:18+00:00 Cgarcia-f wrote:
(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
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/59
------------------------------------------------------------------------
On 2014-02-28T10:09:49+00:00 Cgarcia-f wrote:
Comment on attachment 225392
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?
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/60
------------------------------------------------------------------------
On 2014-02-28T10:40:52+00:00 I-mario wrote:
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
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/61
------------------------------------------------------------------------
On 2014-02-28T12:36:48+00:00 Cgarcia-f wrote:
(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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/62
------------------------------------------------------------------------
On 2014-02-28T13:14:56+00:00 Zeeshan Ali wrote:
(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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/63
------------------------------------------------------------------------
On 2014-02-28T18:24:26+00:00 I-mario wrote:
Created attachment 225475
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!
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/64
------------------------------------------------------------------------
On 2014-03-02T15:53:58+00:00 Ryan Lortie wrote:
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?
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/65
------------------------------------------------------------------------
On 2014-03-02T16:51:41+00:00 Mrobinson-d wrote:
(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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/66
------------------------------------------------------------------------
On 2014-03-02T18:52:37+00:00 Zeeshan Ali wrote:
(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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/67
------------------------------------------------------------------------
On 2014-03-03T17:50:15+00:00 Mrobinson-d wrote:
Comment on attachment 225475
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?
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/68
------------------------------------------------------------------------
On 2014-03-03T18:44:51+00:00 Zeeshan Ali wrote:
(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
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/69
------------------------------------------------------------------------
On 2014-03-04T15:14:46+00:00 I-mario wrote:
Created attachment 225774
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!
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/72
------------------------------------------------------------------------
On 2014-03-04T15:17:22+00:00 Commit-queue wrote:
Attachment 225774 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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/73
------------------------------------------------------------------------
On 2014-03-04T15:23:17+00:00 I-mario wrote:
Created attachment 225775
Patch proposal
New patch fixing style error in the ChangeLog
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/74
------------------------------------------------------------------------
On 2014-03-04T16:24:09+00:00 Mrobinson-d wrote:
Comment on attachment 225775
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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/75
------------------------------------------------------------------------
On 2014-03-04T16:43:50+00:00 I-mario wrote:
Comment on attachment 225775
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
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/76
------------------------------------------------------------------------
On 2014-03-04T16:55:18+00:00 Mrobinson-d wrote:
(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)
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/77
------------------------------------------------------------------------
On 2014-03-04T17:39:17+00:00 I-mario wrote:
(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
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/78
------------------------------------------------------------------------
On 2014-03-04T18:08:14+00:00 I-mario wrote:
Created attachment 225788
Patch proposal
New patch addressing the issues commented by Martin
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/79
------------------------------------------------------------------------
On 2014-03-05T13:52:49+00:00 I-mario wrote:
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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/80
------------------------------------------------------------------------
On 2014-03-06T00:29:26+00:00 Mrobinson-d wrote:
Comment on attachment 225788
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
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/81
------------------------------------------------------------------------
On 2014-03-06T08:06:01+00:00 Cgarcia-f wrote:
Comment on attachment 225788
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 *?
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/82
------------------------------------------------------------------------
On 2014-03-06T12:05:44+00:00 I-mario wrote:
Created attachment 225978
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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/83
------------------------------------------------------------------------
On 2014-03-06T18:03:22+00:00 I-mario wrote:
Created attachment 226005
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!
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/84
------------------------------------------------------------------------
On 2014-03-06T19:04:02+00:00 Buildbot-3 wrote:
Comment on attachment 226005
Patch proposal
Attachment 226005 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
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/85
------------------------------------------------------------------------
On 2014-03-06T19:04:09+00:00 Buildbot-3 wrote:
Created attachment 226010
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
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/86
------------------------------------------------------------------------
On 2014-03-06T19:35:04+00:00 Buildbot-3 wrote:
Comment on attachment 226005
Patch proposal
Attachment 226005 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
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/87
------------------------------------------------------------------------
On 2014-03-06T19:35:08+00:00 Buildbot-3 wrote:
Created attachment 226016
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
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/88
------------------------------------------------------------------------
On 2014-03-06T20:07:20+00:00 Buildbot-3 wrote:
Comment on attachment 226005
Patch proposal
Attachment 226005 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
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/89
------------------------------------------------------------------------
On 2014-03-06T20:07:28+00:00 Buildbot-3 wrote:
Created attachment 226024
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
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/90
------------------------------------------------------------------------
On 2014-03-07T05:55:47+00:00 I-mario wrote:
Created attachment 226084
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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/91
------------------------------------------------------------------------
On 2014-03-07T10:26:03+00:00 Cgarcia-f wrote:
Comment on attachment 226084
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
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/92
------------------------------------------------------------------------
On 2014-03-07T12:15:45+00:00 I-mario wrote:
(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
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/93
------------------------------------------------------------------------
On 2014-03-07T12:27:01+00:00 I-mario wrote:
Created attachment 226115
Patch proposal
Removed the include and the unused functions
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/94
------------------------------------------------------------------------
On 2014-03-07T12:35:27+00:00 Cgarcia-f wrote:
(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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/95
------------------------------------------------------------------------
On 2014-03-07T13:02:40+00:00 I-mario wrote:
(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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/96
------------------------------------------------------------------------
On 2014-03-07T14:25:21+00:00 Zeeshan Ali wrote:
(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?
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/97
------------------------------------------------------------------------
On 2014-03-07T14:31:27+00:00 Cgarcia-f wrote:
(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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/98
------------------------------------------------------------------------
On 2014-03-07T14:54:49+00:00 I-mario wrote:
Created attachment 226123
Patch proposal
New patch adding the newly reported bug together with the FIXME
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/99
------------------------------------------------------------------------
On 2014-03-07T15:13:39+00:00 Zeeshan Ali wrote:
(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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/100
------------------------------------------------------------------------
On 2014-03-09T22:17:31+00:00 I-mario wrote:
Created attachment 226260
Patch proposal
New patch fixing an issue in FindGeoClue2.cmake
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/101
------------------------------------------------------------------------
On 2014-03-10T10:59:13+00:00 Cgarcia-f wrote:
Comment on attachment 226260
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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/102
------------------------------------------------------------------------
On 2014-03-10T11:44:21+00:00 I-mario wrote:
Created attachment 226296
Patch proposal
New patch addressing the last comments from Carlos
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/103
------------------------------------------------------------------------
On 2014-03-10T11:52:49+00:00 Cgarcia-f wrote:
Comment on attachment 226296
Patch proposal
Again
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/104
------------------------------------------------------------------------
On 2014-03-10T23:45:09+00:00 I-mario wrote:
Comment on attachment 226296
Patch proposal
Thanks for the review
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/105
------------------------------------------------------------------------
On 2014-03-11T00:17:29+00:00 Commit-queue wrote:
Comment on attachment 226296
Patch proposal
Clearing flags on attachment: 226296
Committed r165418: <http://trac.webkit.org/changeset/165418>
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/106
------------------------------------------------------------------------
On 2014-03-11T00:17:40+00:00 Commit-queue wrote:
All reviewed patches have been landed. Closing bug.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1100326/comments/107
** Changed in: webkit-open-source
Status: Unknown => Fix Released
** Changed in: webkit-open-source
Importance: Unknown => Medium
** Bug watch added: freedesktop.org Bugzilla #68658
https://bugs.freedesktop.org/show_bug.cgi?id=68658
--
You received this bug notification because you are a member of Desktop
Packages, which is subscribed to chromium-browser in Ubuntu.
https://bugs.launchpad.net/bugs/1100326
Title:
Location requested by websites should be able to use GPS/mobile
positioning
Status in Chromium Browser:
Fix Committed
Status in The Mozilla Firefox Browser:
Confirmed
Status in GeoClue: The Geoinformation Service:
Won't Fix
Status in The WebKit Open Source Project:
Fix Released
Status in chromium-browser package in Ubuntu:
New
Status in firefox package in Ubuntu:
New
Status in geoclue-2.0 package in Ubuntu:
Fix Released
Status in geoclue-providers package in Ubuntu:
Invalid
Bug description:
It would be nice if location requested by websites could use location
found from GPS or GSM/CDMA positioning.
To manage notifications about this bug go to:
https://bugs.launchpad.net/chromium-browser/+bug/1100326/+subscriptions
