Bug 195994 – [GTK] Remove build time dependency on Geoclue2

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

We could just use GDBus API to talk to Geoclue2 service instead.

[Carlos Garcia Campos]

Created attachment 365332 [details]
Patch

[Carlos Garcia Campos]

Created attachment 365531 [details]
Patch

[EWS Watchlist]

Comment on attachment 365531 [details]
Patch

Attachment 365531 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11597478

New failing tests:
http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable.html

[EWS Watchlist]

Created attachment 365535 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2

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

[Michael Catanzaro]

Comment on attachment 365531 [details]
Patch

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

Can we have WebKitGeolocationManager own the GeoclueGeolocationProvider, instead of making it a singleton? We know from experience that singleton classes are dangerous/problematic due to order-of-initialization and order-of-destruction issues, so it's good to avoid singletons whenever possible. Consider also that the WebKitGeolocationManager is owned by WebKitWebContext, so we can have multiple unrelated WebKitGeolocationManagers in the same process that will be fighting with each other to control the singleton GeoclueGeolocationProvider. E.g. if one WebKitGeolocationManager executes GeoclueGeolocationProvider::singleton().start(), the other might execute GeoclueGeolocationProvider::singleton().stop() and defeat it, and that's no good. So I think using the singleton is just not right here. r=me assumes you change this (or add some sort of refcounting to the start/stop requests to ensure one WebKitGeolocationManager cannot stomp on another by turning off the geolocation, but making it non-singleton seems better; it's OK to have multiple clients of the same D-Bus service in the same process IMO).

Good job getting rid of the build dependency, btw.

> Source/WebKit/UIProcess/geoclue/GeoclueGeolocationProvider.cpp:35
> +#if USE(GLIB_EVENT_LOOP)
> +#include <wtf/glib/RunLoopSourcePriority.h>
> +#endif

It's already a GLib-specific file, do we really need #if USE(GLIB_EVENT_LOOP) guards?

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #5)
> Comment on attachment 365531 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=365531&action=review
> 
> Can we have WebKitGeolocationManager own the GeoclueGeolocationProvider,
> instead of making it a singleton? We know from experience that singleton
> classes are dangerous/problematic due to order-of-initialization and
> order-of-destruction issues, so it's good to avoid singletons whenever
> possible. Consider also that the WebKitGeolocationManager is owned by
> WebKitWebContext, so we can have multiple unrelated
> WebKitGeolocationManagers in the same process that will be fighting with
> each other to control the singleton GeoclueGeolocationProvider. E.g. if one
> WebKitGeolocationManager executes
> GeoclueGeolocationProvider::singleton().start(), the other might execute
> GeoclueGeolocationProvider::singleton().stop() and defeat it, and that's no
> good. So I think using the singleton is just not right here. r=me assumes
> you change this (or add some sort of refcounting to the start/stop requests
> to ensure one WebKitGeolocationManager cannot stomp on another by turning
> off the geolocation, but making it non-singleton seems better; it's OK to
> have multiple clients of the same D-Bus service in the same process IMO).

I don't think there's any problem with this singleton from the point of view of construction and destruction order (it's never destroyed indeed), but you are right we should have one per process pool.

> Good job getting rid of the build dependency, btw.

Thanks

> > Source/WebKit/UIProcess/geoclue/GeoclueGeolocationProvider.cpp:35
> > +#if USE(GLIB_EVENT_LOOP)
> > +#include <wtf/glib/RunLoopSourcePriority.h>
> > +#endif
> 
> It's already a GLib-specific file, do we really need #if
> USE(GLIB_EVENT_LOOP) guards?

It's consistent with the definition of setProperty in RunLoop, it's harmless in any case

[Carlos Garcia Campos]

Committed r243409: <https://trac.webkit.org/changeset/243409>