Bug 228056 – [GLib] Remove libportal dependency

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Philippe Normand]

Let's do magic with GDBus instead.

[Philippe Normand]

Created attachment 433748 [details]
Patch

[Philippe Normand]

Created attachment 433751 [details]
Patch

[Patrick Griffis]

I'm against this change. We are only going to depend on more and more portals and libportal does abstract complexity we shouldn't need to care about such as org.freedesktop.portal.Request objects which are easy to get wrong.

[Patrick Griffis]

To be clear, I just mean we shouldn't *avoid* libportal. Not a review of the rest of the changes which appear to fix issues.

[Philippe Normand]

I don't mind either way, we just need to agree on one approach or the other.

Two things that would be nice to have in libportal, support for embedding the mouse cursor in screencast sessions and a way to know which device type was selected by the user.

[Carlos Garcia Campos]

Comment on attachment 433751 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=433751&action=review

There's too much sync DBus, I think, does that happen in the main thread?

> Source/WebCore/platform/mediastream/gstreamer/GStreamerCaptureDeviceManager.h:97
> +    public:

You don't need public for a struct

> Source/WebCore/platform/mediastream/gstreamer/GStreamerDisplayCaptureDeviceManager.cpp:65
> +    if (m_sessions.contains(device.persistentId())) {
> +        const auto it = m_sessions.find(device.persistentId());

Instead of constains + find you can do only find and check it != end

[Philippe Normand]

(In reply to Carlos Garcia Campos from comment #6)
> Comment on attachment 433751 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=433751&action=review
> 
> There's too much sync DBus, I think, does that happen in the main thread?
> 

Yes. And this is a synchronous method, no way to avoid sync dbus with the current architecture.

[Michael Catanzaro]

(In reply to Patrick Griffis from comment #4)
> To be clear, I just mean we shouldn't *avoid* libportal. Not a review of the
> rest of the changes which appear to fix issues.

If Phil doesn't want to use it for this particular portal, we can always bring it back later when we have something else that we want to use it for. I agree that we should use libportal whenever it's convenient to do so.

(In reply to Philippe Normand from comment #7)
> Yes. And this is a synchronous method, no way to avoid sync dbus with the
> current architecture.

Sync D-Bus on the main thread is awful, but sometimes it's very hard to avoid. :/

[Philippe Normand]

Comment on attachment 433751 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=433751&action=review

>> Source/WebCore/platform/mediastream/gstreamer/GStreamerCaptureDeviceManager.h:97
>> +    public:
> 
> You don't need public for a struct

clang disagrees :)

../../Source/WebCore/platform/mediastream/gstreamer/GStreamerDisplayCaptureDeviceManager.cpp:67:24: error: 'fd' is a private member of 'WebCore::GStreamerDisplayCaptureDeviceManager::Session'
            it->value->fd, { }, constraints, device.type());
                       ^
../../Source/WebCore/platform/mediastream/gstreamer/GStreamerCaptureDeviceManager.h:101:13: note: declared private here
        int fd;
            ^
../../Source/WebCore/platform/mediastream/gstreamer/GStreamerDisplayCaptureDeviceManager.cpp:159:47: error: 'path' is a private member of 'WebCore::GStreamerDisplayCaptureDeviceManager::Session'
        "org.freedesktop.portal.Desktop", it->path.ascii().data(), "org.freedesktop.portal.Session", nullptr, nullptr));
                                              ^
../../Source/WebCore/platform/mediastream/gstreamer/GStreamerCaptureDeviceManager.h:102:16: note: declared private here
        String path;
               ^

[Philippe Normand]

Created attachment 433868 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 433868 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=433868&action=review

> Source/WebCore/platform/mediastream/gstreamer/GStreamerDisplayCaptureDeviceManager.cpp:147
> +    fd = g_unix_fd_list_get(fdList.get(), fdOut, nullptr);

Where is this fd closed?

> Source/WebCore/platform/mediastream/gstreamer/GStreamerDisplayCaptureDeviceManager.cpp:156
> +    auto it = m_sessions.take(persistentID);

Take doesn't return an iterator, this is session. I guess we should close the fd here, or in the Session destructor and it will be closed here too when out of scope.

[Philippe Normand]

Created attachment 434004 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 434004 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=434004&action=review

> Source/WebCore/platform/mediastream/gstreamer/GStreamerCaptureDeviceManager.h:96
> +        WTF_MAKE_FAST_ALLOCATED;

Why did you remove the macro for structs? Can we also make this non copyable? It this is copied we would end up closing the fd twice

[Philippe Normand]

I messed up the patch, I'll re-upload one. Sorry!

[Philippe Normand]

Created attachment 434008 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 434008 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=434008&action=review

> Source/WebCore/platform/mediastream/gstreamer/GStreamerDisplayCaptureDeviceManager.cpp:72
> +    m_proxy = adoptGRef(g_dbus_proxy_new_for_bus_sync(G_BUS_TYPE_SESSION,
> +        static_cast<GDBusProxyFlags>(G_DBUS_PROXY_FLAGS_DO_NOT_CONNECT_SIGNALS | G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES), nullptr,
> +        "org.freedesktop.portal.Desktop", "/org/freedesktop/portal/desktop", "org.freedesktop.portal.ScreenCast", nullptr, nullptr));

Sorry that I didn't notice this in previous reviews but we should handle errors here.

> Source/WebCore/platform/mediastream/gstreamer/GStreamerDisplayCaptureDeviceManager.cpp:83
> +    GUniqueOutPtr<GError> error;
> +    auto result = adoptGRef(g_dbus_proxy_call_sync(m_proxy.get(), "CreateSession", g_variant_new("(a{sv})", &options),
> +        G_DBUS_CALL_FLAGS_NONE, s_dbusCallTimeout.millisecondsAs<int>(), nullptr, &error.outPtr()));

This error here isn't handled either.

> Source/WebCore/platform/mediastream/gstreamer/GStreamerDisplayCaptureDeviceManager.cpp:159
> +    auto proxy = adoptGRef(g_dbus_proxy_new_for_bus_sync(G_BUS_TYPE_SESSION,
> +        static_cast<GDBusProxyFlags>(G_DBUS_PROXY_FLAGS_DO_NOT_CONNECT_SIGNALS | G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES), nullptr,
> +        "org.freedesktop.portal.Desktop", session->path.ascii().data(), "org.freedesktop.portal.Session", nullptr, nullptr));

And here proxy could null too if g_dbus_proxy_new_for_bus_sync fails

[Philippe Normand]

Committed r280239 (239909@main): <https://commits.webkit.org/239909@main>

[Radar WebKit Bug Importer]

<rdar://problem/81011828>