Skip to content

[WPE][GTK] Implement portal-based geolocation#25496

Merged
webkit-commit-queue merged 1 commit into
WebKit:mainfrom
GeorgesStavracas:eng/WPEGTK-Geolocation-does-not-work-under-flatpak
Mar 6, 2024
Merged

[WPE][GTK] Implement portal-based geolocation#25496
webkit-commit-queue merged 1 commit into
WebKit:mainfrom
GeorgesStavracas:eng/WPEGTK-Geolocation-does-not-work-under-flatpak

Conversation

@GeorgesStavracas

@GeorgesStavracas GeorgesStavracas commented Mar 5, 2024

Copy link
Copy Markdown
Contributor

18a97d2

[WPE][GTK] Implement portal-based geolocation
https://bugs.webkit.org/show_bug.cgi?id=220253

Reviewed by Michael Catanzaro.

This turned into a somewhat messy patch, but the principle behind it is
to try and use the Location portal first, and fallback to the direct
Geoclue calls that it already does.

The Location portal has two particular steps: (1) create a geolocation
session, and (2) start the session and get geolocation updates.

Step 1 is where the location accuracy is set. For this reason, unlike
the direct Geoclue code path, we have to close and recreate the Location
portal session when high accuracy mode changes.

Talking about high accuracy mode, apparently the new value was never
stored into m_isHighAccuracyEnabled! This is fixed in this patch too.

Step 2 is where the Location session is started. Only after this call
that we'd receive geolocation position updates. In theory we'd be able
to pass a parent window handler in this step, and that would allow the
desktop environment to nicely place access control requests on it; that
is not currently supported by WebKit though, so just pass empty string
for now.

The destroy timer is used to release both Portal and Geoclue resources
so name the timer and the corresponding methods to match that. Also
rename Geoclue-specific methods to explicitly mention Geoclue, which
helps following the callback chain.

* Source/WebKit/UIProcess/geoclue/GeoclueGeolocationProvider.cpp:
(WebKit::GeoclueGeolocationProvider::GeoclueGeolocationProvider):
(WebKit::GeoclueGeolocationProvider::start):
(WebKit::GeoclueGeolocationProvider::stop):
(WebKit::GeoclueGeolocationProvider::setEnableHighAccuracy):
(WebKit::GeoclueGeolocationProvider::destroyStateLater):
(WebKit::GeoclueGeolocationProvider::destroyState):
(WebKit::GeoclueGeolocationProvider::acquirePortalProxy):
(WebKit::GeoclueGeolocationProvider::setupPortalProxy):
(WebKit::GeoclueGeolocationProvider::createPortalSession):
(WebKit::GeoclueGeolocationProvider::startPortalSession):
(WebKit::GeoclueGeolocationProvider::portalLocationUpdated):
(WebKit::GeoclueGeolocationProvider::stopPortalSession):
(WebKit::GeoclueGeolocationProvider::createGeoclueManager):
(WebKit::GeoclueGeolocationProvider::setupGeoclueManager):
(WebKit::GeoclueGeolocationProvider::createGeoclueClient):
(WebKit::GeoclueGeolocationProvider::setupGeoclueClient):
(WebKit::GeoclueGeolocationProvider::startGeoclueClient):
(WebKit::GeoclueGeolocationProvider::stopGeoclueClient):
(WebKit::GeoclueGeolocationProvider::requestAccuracyLevel):
(WebKit::GeoclueGeolocationProvider::destroyManagerLater): Deleted.
(WebKit::GeoclueGeolocationProvider::destroyManager): Deleted.
(WebKit::GeoclueGeolocationProvider::setupManager): Deleted.
(WebKit::GeoclueGeolocationProvider::createClient): Deleted.
(WebKit::GeoclueGeolocationProvider::setupClient): Deleted.
(WebKit::GeoclueGeolocationProvider::startClient): Deleted.
(WebKit::GeoclueGeolocationProvider::stopClient): Deleted.
* Source/WebKit/UIProcess/geoclue/GeoclueGeolocationProvider.h:

Canonical link: https://commits.webkit.org/275746@main

c6d1734

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
🧪 ios-wk2-wpt ✅ 🛠 wpe-skia
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
🛠 tv-sim ✅ 🧪 api-gtk
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@GeorgesStavracas GeorgesStavracas requested a review from cdumez as a code owner March 5, 2024 19:41
@GeorgesStavracas GeorgesStavracas self-assigned this Mar 5, 2024
@GeorgesStavracas GeorgesStavracas added the WebKitGTK Bugs related to the Gtk API layer. label Mar 5, 2024
@GeorgesStavracas GeorgesStavracas requested a review from a team March 5, 2024 19:42
@webkit-early-warning-system

webkit-early-warning-system commented Mar 5, 2024

Copy link
Copy Markdown
Collaborator

@mcatanzaro mcatanzaro left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it. Thanks!

Comment thread Source/WebKit/UIProcess/geoclue/GeoclueGeolocationProvider.cpp Outdated
Comment thread Source/WebKit/UIProcess/geoclue/GeoclueGeolocationProvider.cpp Outdated
Comment on lines 314 to 315

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't think it's worth adding a small callback just to print any potential error? Could be useful in the future if something goes unexpectedly wrong. (I wouldn't pass the GeoclueGeolocationProvider since you surely don't want to keep it alive for the callback or bother with the cancellable here. I would just print a warning if the call fails and do nothing else.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the Close method, specifically, I don't think it's worth. Because the Session may have been closed already on the bus, due to a number of external reasons. So it'll log a scary warning for something that's essentially harmless, and outside of WebKit's control.

For the other calls below, I think it's a good idea. Will do.

Comment thread Source/WebKit/UIProcess/geoclue/GeoclueGeolocationProvider.cpp Outdated
Comment thread Source/WebKit/UIProcess/geoclue/GeoclueGeolocationProvider.cpp Outdated
Comment thread Source/WebKit/UIProcess/geoclue/GeoclueGeolocationProvider.cpp Outdated
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Mar 6, 2024
@GeorgesStavracas GeorgesStavracas removed the merging-blocked Applied to prevent a change from being merged label Mar 6, 2024
@GeorgesStavracas GeorgesStavracas force-pushed the eng/WPEGTK-Geolocation-does-not-work-under-flatpak branch from 52a1cf4 to c6d1734 Compare March 6, 2024 13:52
@GeorgesStavracas GeorgesStavracas added the request-merge-queue Request a pull request to be added to merge-queue once ready label Mar 6, 2024
@mcatanzaro mcatanzaro added merge-queue Applied to send a pull request to merge-queue and removed request-merge-queue Request a pull request to be added to merge-queue once ready labels Mar 6, 2024
https://bugs.webkit.org/show_bug.cgi?id=220253

Reviewed by Michael Catanzaro.

This turned into a somewhat messy patch, but the principle behind it is
to try and use the Location portal first, and fallback to the direct
Geoclue calls that it already does.

The Location portal has two particular steps: (1) create a geolocation
session, and (2) start the session and get geolocation updates.

Step 1 is where the location accuracy is set. For this reason, unlike
the direct Geoclue code path, we have to close and recreate the Location
portal session when high accuracy mode changes.

Talking about high accuracy mode, apparently the new value was never
stored into m_isHighAccuracyEnabled! This is fixed in this patch too.

Step 2 is where the Location session is started. Only after this call
that we'd receive geolocation position updates. In theory we'd be able
to pass a parent window handler in this step, and that would allow the
desktop environment to nicely place access control requests on it; that
is not currently supported by WebKit though, so just pass empty string
for now.

The destroy timer is used to release both Portal and Geoclue resources
so name the timer and the corresponding methods to match that. Also
rename Geoclue-specific methods to explicitly mention Geoclue, which
helps following the callback chain.

* Source/WebKit/UIProcess/geoclue/GeoclueGeolocationProvider.cpp:
(WebKit::GeoclueGeolocationProvider::GeoclueGeolocationProvider):
(WebKit::GeoclueGeolocationProvider::start):
(WebKit::GeoclueGeolocationProvider::stop):
(WebKit::GeoclueGeolocationProvider::setEnableHighAccuracy):
(WebKit::GeoclueGeolocationProvider::destroyStateLater):
(WebKit::GeoclueGeolocationProvider::destroyState):
(WebKit::GeoclueGeolocationProvider::acquirePortalProxy):
(WebKit::GeoclueGeolocationProvider::setupPortalProxy):
(WebKit::GeoclueGeolocationProvider::createPortalSession):
(WebKit::GeoclueGeolocationProvider::startPortalSession):
(WebKit::GeoclueGeolocationProvider::portalLocationUpdated):
(WebKit::GeoclueGeolocationProvider::stopPortalSession):
(WebKit::GeoclueGeolocationProvider::createGeoclueManager):
(WebKit::GeoclueGeolocationProvider::setupGeoclueManager):
(WebKit::GeoclueGeolocationProvider::createGeoclueClient):
(WebKit::GeoclueGeolocationProvider::setupGeoclueClient):
(WebKit::GeoclueGeolocationProvider::startGeoclueClient):
(WebKit::GeoclueGeolocationProvider::stopGeoclueClient):
(WebKit::GeoclueGeolocationProvider::requestAccuracyLevel):
(WebKit::GeoclueGeolocationProvider::destroyManagerLater): Deleted.
(WebKit::GeoclueGeolocationProvider::destroyManager): Deleted.
(WebKit::GeoclueGeolocationProvider::setupManager): Deleted.
(WebKit::GeoclueGeolocationProvider::createClient): Deleted.
(WebKit::GeoclueGeolocationProvider::setupClient): Deleted.
(WebKit::GeoclueGeolocationProvider::startClient): Deleted.
(WebKit::GeoclueGeolocationProvider::stopClient): Deleted.
* Source/WebKit/UIProcess/geoclue/GeoclueGeolocationProvider.h:

Canonical link: https://commits.webkit.org/275746@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/WPEGTK-Geolocation-does-not-work-under-flatpak branch from c6d1734 to 18a97d2 Compare March 6, 2024 18:45
@webkit-commit-queue

Copy link
Copy Markdown
Collaborator

Committed 275746@main (18a97d2): https://commits.webkit.org/275746@main

Reviewed commits have been landed. Closing PR #25496 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit 18a97d2 into WebKit:main Mar 6, 2024
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Mar 6, 2024
@GeorgesStavracas GeorgesStavracas deleted the eng/WPEGTK-Geolocation-does-not-work-under-flatpak branch March 6, 2024 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

WebKitGTK Bugs related to the Gtk API layer.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants