[webkit-reviews] review granted: [Bug 195994] [GTK] Remove build time dependency on Geoclue2 : [Attachment 365531] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at igalia.com> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 195994: [GTK] Remove build time dependency on Geoclue2
https://bugs.webkit.org/show_bug.cgi?id=195994

Attachment 365531: Patch

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




--- Comment #5 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 365531
  --> https://bugs.webkit.org/attachment.cgi?id=365531
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?