[webkit-reviews] review requested: [Bug 120185] [GTK] Add support for Geoclue2 : [Attachment 225774] Patch proposal

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <mario at webkit.org> has asked  for review:
Bug 120185: [GTK] Add support for Geoclue2
https://bugs.webkit.org/show_bug.cgi?id=120185

Attachment 225774: Patch proposal
https://bugs.webkit.org/attachment.cgi?id=225774&action=review

------- Additional Comments from Mario Sanchez Prada <mario at webkit.org>
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!