We could just use GDBus API to talk to Geoclue2 service instead.
Created attachment 365332 [details] Patch
Created attachment 365531 [details] Patch
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
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
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?
(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
Committed r243409: <https://trac.webkit.org/changeset/243409>