Bug 129879 - Geoclue2 based backend should provide the right desktop ID
Summary: Geoclue2 based backend should provide the right desktop ID
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adrian Perez
URL:
Keywords:
: 175824 (view as bug list)
Depends on:
Blocks:
 
Reported: 2014-03-07 06:51 PST by Mario Sanchez Prada
Modified: 2017-08-24 01:45 PDT (History)
10 users (show)

See Also:


Attachments
Patch (2.29 KB, patch)
2017-08-23 14:09 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff
Patch (3.83 KB, patch)
2017-08-23 16:00 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff
Patch (3.81 KB, patch)
2017-08-24 01:21 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mario Sanchez Prada 2014-03-07 06:51:12 PST
According to the discussion that happened in bug 120185, we need to provide a better string than the output of g_get_prgname() as the desktop ID for geoclue2, so the app embedding WebKit is properly identified.

See the conversation in bug 120185 (from comment 59 to 65) for more details.
Comment 1 Carlos Alberto Lopez Perez 2017-08-23 06:10:12 PDT
*** Bug 175824 has been marked as a duplicate of this bug. ***
Comment 2 Carlos Alberto Lopez Perez 2017-08-23 06:13:05 PDT
I wonder if its even possible to provide the right desktop ID on all cases?

I think we can at least document this on our API and leave the job of providing the right desktop ID to the application using the API (ir should know better).
Comment 3 Zeeshan Ali 2017-08-23 06:28:36 PDT
(In reply to Carlos Alberto Lopez Perez from comment #2)
> I wonder if its even possible to provide the right desktop ID on all cases?

The only place where it really matters is sandboxed apps running inside flatpak and there you're required to have a desktop ID AFAIK.

For cases where it doesn't make sense, just invent a desktop ID and install a very basic .desktop file for it.

> I think we can at least document this on our API and leave the job of
> providing the right desktop ID to the application using the API (ir should
> know better).

I agree.
Comment 4 Michael Catanzaro 2017-08-23 07:04:00 PDT
(In reply to Carlos Alberto Lopez Perez from comment #2)
> I wonder if its even possible to provide the right desktop ID on all cases?

No it's not possible, not unless we add new API. That's not needed, because it's easier to just require that g_get_prgname() match the desktop ID. I think we should change nothing and just document this somewhere.
Comment 5 Zeeshan Ali 2017-08-23 07:21:04 PDT
(In reply to Michael Catanzaro from comment #4)
> (In reply to Carlos Alberto Lopez Perez from comment #2)
> > I wonder if its even possible to provide the right desktop ID on all cases?
> 
> No it's not possible, not unless we add new API. That's not needed, because
> it's easier to just require that g_get_prgname() match the desktop ID. I
> think we should change nothing and just document this somewhere.

Oh sorry I completely misunderstood. I thought the question was if all apps can provide the desktop ID. I agree, apps should be doing this.
Comment 6 Adrian Perez 2017-08-23 07:47:03 PDT
(In reply to Michael Catanzaro from comment #4)
> (In reply to Carlos Alberto Lopez Perez from comment #2)
> > I wonder if its even possible to provide the right desktop ID on all cases?
> 
> No it's not possible, not unless we add new API. That's not needed, because
> it's easier to just require that g_get_prgname() match the desktop ID. I
> think we should change nothing and just document this somewhere.

There is a much better way than using g_get_prgname() — GApplication
(https://developer.gnome.org/gio/stable/GApplication.html):

   const char* appid = nullptr;
   GApplication* app = g_application_get_default();
   if (app) {
     appid = g_application_get_application_id(app);
   }
   // Maybe add a fall-back to g_get_prgname()


This allows different values for the application identifier, and the program
name. It is very common that they are different. Example: the program name
for Epiphany is “epiphany” (the name of the binary on disk), and the application
identifier is “org.gnome.Epiphany”.

We should use GApplcation when GLib 2.32+ is available, and document that
setting an application identifier using GApplication/GtkApplication is
needed for geolocation to work.
Comment 7 Adrian Perez 2017-08-23 14:09:32 PDT
Created attachment 318916 [details]
Patch

Tentative patch.

Do we want to document the need to set an application identifier,
preferrably via GApplication/GtkApplication, for things to work
correctly? If yes, where in the documentation would that go?
Comment 8 Michael Catanzaro 2017-08-23 14:49:47 PDT
Comment on attachment 318916 [details]
Patch

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

> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue.cpp:141
> +    // also fall-back to our old behaviour of using g_get_prgname().

fallback

behavior
Comment 9 Michael Catanzaro 2017-08-23 14:51:14 PDT
This won't work for Epiphany web applications, but Epiphany is on the whitelist so that's OK.

I don't think there's really any good place to document this.
Comment 10 Adrian Perez 2017-08-23 15:56:51 PDT
(In reply to Michael Catanzaro from comment #8)
> Comment on attachment 318916 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=318916&action=review
> 
> > Source/WebCore/platform/geoclue/GeolocationProviderGeoclue.cpp:141
> > +    // also fall-back to our old behaviour of using g_get_prgname().
> 
> fallback

Right, it's an actual word without a dash, and I'll change it also in
the ChangeLog.

> behavior

Mmh, “behaviour” is correct → http://www.wordreference.com/definition/behaviour

I'll post an updated version with some notes for developers in the
documentation page for WebKitGeolocationPermissionRequest — it is the
the only place in the API documentation where geolocation is mentioned.
Comment 11 Adrian Perez 2017-08-23 16:00:10 PDT
Created attachment 318934 [details]
Patch

Version with notes for developers added. The location is not
great, but it is the only place in the whole API reference where
geolocation is mentioned. Better than nothing, I presume :-D
Comment 12 Build Bot 2017-08-23 16:02:05 PDT
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
Comment 13 Adrian Perez 2017-08-23 16:05:57 PDT
JFTR: The usage of GApplication is unconditional because GApplication
appeared in GLib 2.28, and WebKitGTK+ already requires GLib 2.36 — we
don't need to add guards in the code for this.
Comment 14 Michael Catanzaro 2017-08-23 17:42:35 PDT
(In reply to Adrian Perez from comment #10)
> > behavior
> 
> Mmh, “behaviour” is correct →
> http://www.wordreference.com/definition/behaviour

That's British. We use American English in WebKit, with the exception of Source/WebCore/platform/gtk/po/en_GB.po, which is a real thing.

But I'm afraid we can't provide .po files for source code comments. :)

> I'll post an updated version with some notes for developers in the
> documentation page for WebKitGeolocationPermissionRequest — it is the
> the only place in the API documentation where geolocation is mentioned.

Good idea.
Comment 15 Michael Catanzaro 2017-08-23 17:46:18 PDT
Comment on attachment 318934 [details]
Patch

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

> Source/WebCore/platform/geoclue/GeolocationProviderGeoclue.cpp:141
> +    // also fallback to our old behaviour of using g_get_prgname().

So I would still prefer if you changed the spelling.

> Source/WebKit/UIProcess/API/glib/WebKitGeolocationPermissionRequest.cpp:43
> + * ## Developer Notes

All of the documentation is developer notes, so this heading is useless. I'd just remove it.

> Source/WebKit/UIProcess/API/glib/WebKitGeolocationPermissionRequest.cpp:58
> + * during initialization *might* be needed when the name of the executable

It will be needed, not "might."
Comment 16 Adrian Perez 2017-08-24 01:21:02 PDT
Created attachment 318974 [details]
Patch

Updated with Michael's latest review suggestions.
Comment 17 Adrian Perez 2017-08-24 01:45:26 PDT
Committed r221132: <http://trac.webkit.org/changeset/221132>