Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

webextensions: add a portal for managing WebExtensions native messaging servers #1537

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

xhorak
Copy link
Contributor

@xhorak xhorak commented Dec 17, 2024

This MR continues the work from the abandoned MR #705.

The following updates and improvements have been made:

  • rebase
  • annotation for qt
  • fixed test

@jhenstridge thank you for your work on this! Let me know if you'd like to collaborate further.

This is intended to provide a way for a confined web browser to start native code helpers for their extensions. At present it can start the servers installed on the host system. But in future this could be extended to cover sandboxed native messaging servers too.

@Rob--W
Copy link

Rob--W commented Dec 17, 2024

Has the other PR really been abandoned, or is the author merely focused on other tasks or even away from work? The latter would not be surprising considering that we are approaching the end of the year.

This patch squashes all changes from #705 into one, along with new changes. I suppose that this was the easiest way to rebase, but it causes the lost of context because some individual commits offered context that are missing from the squashed commit.

Out of all, I think that it would make most sense to include the context of 59d7b4b addresses #769 . E.g. by including the issue reference in the commit message ("Fixes xxx") and a code comment pointing to the discussion.
I have not reviewed the updated patch yet, but note that in the previous PR there was still an active discussion on this topic at #705 (comment)

@swick
Copy link
Contributor

swick commented Dec 17, 2024

I'd really like if the test was moved over to python.

@xhorak
Copy link
Contributor Author

xhorak commented Dec 17, 2024

I've been trying to contact authors since late October but did not received any reply. I would happily keep the original PR but I want to move this forward. Especially now when the Firefox part landed.

The squash was suggested, so I did it. I'll check the 59d7b4b
I plan to do some testing because I'm getting various app_ids as I've mentioned in #705 (comment)

@xhorak
Copy link
Contributor Author

xhorak commented Dec 17, 2024

In latest update I've fixed the xdp_app_info_get_gappinfo call and stopped to use g_desktop_app_info_new.

…ng servers.

Rebase, fix and continue work on webextensions: add a portal for managing WebExtensions
native messaging servers:
flatpak#705

This commit builds on the work done in the original MR authored by @jhenstridge
but resolves pending items and brings it closer to completion.

This is intended to provide a way for a confined web browser to start
native code helpers for their extensions. At present it can start the
servers installed on the host system. But in future this could be
extended to cover sandboxed native messaging servers too.
@grulja
Copy link
Contributor

grulja commented Dec 18, 2024

I plan to do some testing because I'm getting various app_ids as I've mentioned in #705 (comment)

That's the usual issue we have with host apps. The application id comes from cgroups, which is set depending on the way you start the app, e.g. you start it from a terminal app it might get app id of the terminal app. Or you start it with Alt + F2 in GNOME and the app id will be just firefox. Flatpak apps are reliable in this case.

See #1512 what we did for the camera portal, where this was causing issues.

@swick
Copy link
Contributor

swick commented Dec 18, 2024

Regarding the appid mismatch that unfortunately still happens: I'd much rather see us adding #1521 than special casing every portal that firefox actually depends on.

@xhorak
Copy link
Contributor Author

xhorak commented Dec 19, 2024

I'd really like if the test was moved over to python.

Looking at the python vs C tests, there seems to be missing permission store service in Python. I'm not sufficiently familiar with the code to implement it for python tests or is it done somewhere already?

@swick
Copy link
Contributor

swick commented Dec 19, 2024

The permission store is supposed to get started (https://github.com/flatpak/xdg-desktop-portal/blob/1c902cc77e53b422d49988bab58b6cca0ed9b112/tests/conftest.py#L458C5-L458C25) when the test case uses either the xdg_permission_store or the more general portals fixtures.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Needs Triage
Development

Successfully merging this pull request may close these issues.

5 participants