[webkit-reviews] review granted: [Bug 217991] [GLIB] MediaSession is not enabled : [Attachment 436268] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at gnome.org> has granted Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 217991: [GLIB] MediaSession is not enabled
https://bugs.webkit.org/show_bug.cgi?id=217991

Attachment 436268: Patch

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




--- Comment #22 from Michael Catanzaro <mcatanzaro at gnome.org> ---
Comment on attachment 436268
  --> https://bugs.webkit.org/attachment.cgi?id=436268
Patch

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

Perhaps overly cynical, but I like to assume that any code that involves D-Bus
is plagued with problems that we won't notice until users complain about
crashes. But I didn't spot any myself, and there's only one way to find out. ;)

> Source/WebCore/ChangeLog:9
> +
> +

Extra blank line here

> Source/WTF/wtf/glib/GRefPtr.h:34
> +    GDBusNodeInfo* g_dbus_node_info_ref(GDBusNodeInfo*);
> +    void g_dbus_node_info_unref(GDBusNodeInfo*);

These two should be declared in the .cpp file, since that's where they're used.

> Source/WebCore/platform/audio/glib/MediaSessionManagerGLib.cpp:135
> +static inline std::optional<PlatformMediaSession::RemoteControlCommandType>
getCommand(const char* name)

Why inline?

> Source/WebCore/platform/audio/glib/MediaSessionManagerGLib.cpp:173
> +enum class Property : uint8_t {

This is such a generic name, I would be worried about potential ODR violations
if any other file has anything named "Property," especially now that distros
are building with LTO enabled. Simplest solution is to name it something else,
or you could enclose it in an anonymous namespace to restrict it to file scope:

namespace {

enum class Property : uint8_t {
    // ...
};

} // anonymous namespace

> Source/WebCore/platform/audio/glib/MediaSessionManagerGLib.cpp:234
> +    auto& manager = *reinterpret_cast<MediaSessionManagerGLib*>(userData);
> +    switch (property.value()) {
> +    case Property::NoProperty:
> +	   ASSERT_NOT_REACHED();

I don't think this ASSERT is correct because it shouldn't be possible to
trigger an assert by passing invalid input over D-Bus. ASSERT should mean
programmer error in WebKit itself.

> Source/WebCore/platform/audio/glib/MediaSessionManagerGLib.cpp:319
> +	   g_warning("Failed at root registration: %s", error->message);

Maybe "Failed to register MPRIS D-Bus object: %s" or something like that. "root
registration" sounds meaningless to me.

> Source/WebCore/platform/audio/glib/MediaSessionManagerGLib.cpp:327
> +	   g_warning("Failed at object registration: %s", error->message);

Could mention MPRIS here too? Same for the two warnings in
MediaSessionManagerGLib::nameLost.

> Source/WebCore/platform/audio/glib/MediaSessionManagerGLib.cpp:484
> +    // FIXME: Fix this layering violation.
> +    if (auto element =
HTMLMediaElement::bestMediaElementForRemoteControls(MediaElementSession::Playba
ckControlsPurpose::NowPlaying))
> +	   return &element->mediaSession();

The right way to do this without making Carlos Garcia sad is to add a "client"
(delegate) interface outside WebCore/platform and then implement it here. It's
a bit annoying to do, though. I guess you know all that already. I haven't seen
any serious action on platform layering violations in a long time, so you
probably won't be tarred and feathered if you leave it like this. I'd never
have noticed had you not added the FIXME comment.

> Source/WebCore/platform/audio/glib/MediaSessionManagerGLib.cpp:537
> +    g_variant_builder_init(&builder, G_VARIANT_TYPE("a{sv}"));

Important: all of your calls to g_variant_builder_init() need to be paired with
a g_variant_builder_clear(), otherwise they will leak. Same in
MediaSessionManagerGLib::sessionStateChanged.

> Source/WebCore/platform/glib/RemoteCommandListenerGLib.h:30
> +    RemoteCommandListenerGLib(RemoteCommandListenerClient&);

explicit

> Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:227
> +	       g_error("Unable to own DBus MPRIS name in the WebKit sandbox:
%s", error->message);

Don't you think crashing is pretty harsh? The code up above is fatal because
there are going to be a *lot* of problems (e.g. broken fonts) if
xdg-desktop-portal isn't working at all. But MPRIS? I think we can survive
without MPRIS. So I would print a warning and move on if this isn't working.

Also: DBus -> D-Bus