Bug 195994 - [GTK] Remove build time dependency on Geoclue2
Summary: [GTK] Remove build time dependency on Geoclue2
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on: 195940
Blocks:
  Show dependency treegraph
 
Reported: 2019-03-20 02:59 PDT by Carlos Garcia Campos
Modified: 2019-03-23 03:37 PDT (History)
4 users (show)

See Also:


Attachments
Patch (45.61 KB, patch)
2019-03-20 03:04 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (45.61 KB, patch)
2019-03-21 04:10 PDT, Carlos Garcia Campos
mcatanzaro: review+
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-highsierra-wk2 (3.07 MB, application/zip)
2019-03-21 05:16 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2019-03-20 02:59:17 PDT
We could just use GDBus API to talk to Geoclue2 service instead.
Comment 1 Carlos Garcia Campos 2019-03-20 03:04:05 PDT
Created attachment 365332 [details]
Patch
Comment 2 Carlos Garcia Campos 2019-03-21 04:10:29 PDT
Created attachment 365531 [details]
Patch
Comment 3 Build Bot 2019-03-21 05:16:16 PDT
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
Comment 4 Build Bot 2019-03-21 05:16:18 PDT
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 5 Michael Catanzaro 2019-03-22 07:57:32 PDT
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?
Comment 6 Carlos Garcia Campos 2019-03-23 02:58:32 PDT
(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
Comment 7 Carlos Garcia Campos 2019-03-23 03:37:49 PDT
Committed r243409: <https://trac.webkit.org/changeset/243409>