Geoclue2 based backend should provide the right desktop ID

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Mario Sanchez Prada]

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.

[Carlos Alberto Lopez Perez]

*** Bug 175824 has been marked as a duplicate of this bug. ***

[Carlos Alberto Lopez Perez]

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

[Zeeshan Ali]

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

[Michael Catanzaro]

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

[Zeeshan Ali]

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

[Adrian Perez]

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

[Adrian Perez]

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?

[Michael Catanzaro]

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

[Michael Catanzaro]

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.

[Adrian Perez]

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

[Adrian Perez]

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

[Build Bot]

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

[Adrian Perez]

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.

[Michael Catanzaro]

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

[Michael Catanzaro]

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

[Adrian Perez]

Created attachment 318974 [details]
Patch

Updated with Michael's latest review suggestions.

[Adrian Perez]

Committed r221132: <http://trac.webkit.org/changeset/221132>