Bug 133854 – [GTK] Implement connected and disconnected events of GAMEPAD API with libmanette

• Component: New Bugs
• Status: RESOLVED FIXED

[Michal Debski]

This adds gamepad provider implementation for GTK port.

[Michal Debski]

Created attachment 233594 [details]
Patch

[ChangSeok Oh]

Created attachment 395125 [details]
Patch

[ChangSeok Oh]

> src/meson.build:116:6: 
> : Program(s) ['vapigen'] not found or not executable

The build bot seems to not have vala package..

[ChangSeok Oh]

Created attachment 395133 [details]
Patch

[Philippe Normand]

(In reply to ChangSeok Oh from comment #3)
> > src/meson.build:116:6: 
> > : Program(s) ['vapigen'] not found or not executable
> 
> The build bot seems to not have vala package..

Please disable that and the "introspection" options in the meson build.

[ChangSeok Oh]

Created attachment 395149 [details]
Patch

[Philippe Normand]

Comment on attachment 395149 [details]
Patch

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

> Tools/gtk/jhbuild.modules:449
> +  <meson id="manette" mesonargs="-Dintrospection=false">

-Dvapi=false too please :)

[ChangSeok Oh]

Created attachment 395150 [details]
Patch

[ChangSeok Oh]

(In reply to Philippe Normand from comment #7)
> Comment on attachment 395149 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=395149&action=review
> 
> > Tools/gtk/jhbuild.modules:449
> > +  <meson id="manette" mesonargs="-Dintrospection=false">
> 
> -Dvapi=false too please :)

Thanks!

[ChangSeok Oh]

Created attachment 395240 [details]
Patch

[Don Olmstead]

Comment on attachment 395240 [details]
Patch

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

Any new Find modules should expose a target see https://github.com/WebKit/webkit/blob/master/Source/cmake/FindOpenJPEG.cmake for something you should be able to copy pasta for the most part.

> Source/cmake/FindManette.cmake:49
> +find_package(PkgConfig)
> +pkg_check_modules(PC_MANETTE manette-0.2)
> +
> +find_path(MANETTE_INCLUDE_DIRS
> +    NAMES libmanette.h
> +    HINTS ${PC_MANETTE_INCLUDEDIR}
> +          ${PC_MANETTE_INCLUDE_DIRS}
> +)
> +
> +find_library(MANETTE_LIBRARIES
> +    NAMES manette-0.2
> +    HINTS ${PC_MANETTE_LIBDIR}
> +          ${PC_MANETTE_LIBRARY_DIRS}
> +)
> +
> +include(FindPackageHandleStandardArgs)
> +find_package_handle_standard_args(Manette
> +    FOUND_VAR MANETTE_FOUND
> +    REQUIRED_VARS MANETTE_INCLUDE_DIRS MANETTE_LIBRARIES
> +)

Please add a proper target in CMake.

[ChangSeok Oh]

Created attachment 396169 [details]
Patch

[ChangSeok Oh]

(In reply to Don Olmstead from comment #11)
> Comment on attachment 395240 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=395240&action=review
> 
> Any new Find modules should expose a target see
> https://github.com/WebKit/webkit/blob/master/Source/cmake/FindOpenJPEG.cmake
> for something you should be able to copy pasta for the most part.
> 
> > Source/cmake/FindManette.cmake:49
> > +find_package(PkgConfig)
> > +pkg_check_modules(PC_MANETTE manette-0.2)
> > +
> > +find_path(MANETTE_INCLUDE_DIRS
> > +    NAMES libmanette.h
> > +    HINTS ${PC_MANETTE_INCLUDEDIR}
> > +          ${PC_MANETTE_INCLUDE_DIRS}
> > +)
> > +
> > +find_library(MANETTE_LIBRARIES
> > +    NAMES manette-0.2
> > +    HINTS ${PC_MANETTE_LIBDIR}
> > +          ${PC_MANETTE_LIBRARY_DIRS}
> > +)
> > +
> > +include(FindPackageHandleStandardArgs)
> > +find_package_handle_standard_args(Manette
> > +    FOUND_VAR MANETTE_FOUND
> > +    REQUIRED_VARS MANETTE_INCLUDE_DIRS MANETTE_LIBRARIES
> > +)
> 
> Please add a proper target in CMake.

O.K.

[Adrian Perez]

Comment on attachment 396169 [details]
Patch

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

This looks all good to me, module a few nits. It would not hurt to have
someone else take a quick look and rubber-stamp it, though.

> Source/WebCore/platform/gamepad/manette/ManetteGamepad.cpp:35
> +    , m_device(adoptGRef(static_cast<ManetteDevice*>(g_object_ref(device))))

You don't need to use adopGRef() + g_object_ref() by here, you can pass the
pointer to the GRefPtr constructor and it will increase the reference count
automatically. The following should be enough:

    ManetteGamePad(ManetteDevice* device, unsigned index):
        : PlatformGamepad(index)
        , m_device(device)
    {
    }

=)

> Source/WebCore/platform/gamepad/manette/ManetteGamepadProvider.cpp:104
> +    std::unique_ptr<ManetteGamepad> gamepad = makeUnique<ManetteGamepad>(device, index);

You could use “auto” for the variable type here, because it is clear
from the right side of the assignment which type it will have.

> Source/cmake/FindManette.cmake:56
> +pkg_check_modules(PC_MANETTE manette-0.2)

I would add the IMPORTED_TARGET flag here, and all lines from
from 58 to 71 will not be needed.

> Source/cmake/FindManette.cmake:87
> +endif ()

…and here use the PkgConfig::PC_MANETTE target:

    if (TARGET PkgConfig::PC_MANETTE AND NOT TARGET Manette::Manette)
        add_library(Manette::Manette INTERFACE IMPORTED GLOBAL)
        set_property(TARGET Manette::Manette INTERFACE_LINK_LIBRARIES PkgConfig::PC_MANETTE)
    endif ()

This is what we currently do in Source/cmake/FindGTK.cmake :)

> Source/cmake/FindManette.cmake:91
> +if (Manette_FOUND)

If the PkgConfig target is used, this if-block is not needed.

[Carlos Garcia Campos]

Comment on attachment 396169 [details]
Patch

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

> Source/WebCore/platform/gamepad/manette/ManetteGamepad.h:38
> +class ManetteGamepad : public PlatformGamepad {

We can mark the class final too.

> Source/WebCore/platform/gamepad/manette/ManetteGamepadProvider.cpp:50
> +    if (provider)

Can this really be nullptr? The provider is a singleton.

> Source/WebCore/platform/gamepad/manette/ManetteGamepadProvider.cpp:56
> +    if (provider)

Ditto.

> Source/WebCore/platform/gamepad/manette/ManetteGamepadProvider.cpp:61
> +    : m_shouldDispatchCallbacks(false)

This could be initialized in the header.

> Source/WebCore/platform/gamepad/manette/ManetteGamepadProvider.cpp:66
> +    g_signal_connect(G_OBJECT(m_monitor.get()), "device-connected", G_CALLBACK(onDeviceConnected), this);
> +    g_signal_connect(G_OBJECT(m_monitor.get()), "device-disconnected", G_CALLBACK(onDeviceDisconnected), this);

No need to cast GObject for g_signal_connect

> Source/WebCore/platform/gamepad/manette/ManetteGamepadProvider.cpp:76
> +    if (shouldOpenAndScheduleManager) {

We could early return instead.

> Source/WebCore/platform/gamepad/manette/ManetteGamepadProvider.cpp:166
> +    auto i = m_gamepadVector.find(result.get());

i -> index

> Source/WebCore/platform/gamepad/manette/ManetteGamepadProvider.h:48
> +    WEBCORE_EXPORT static ManetteGamepadProvider& singleton();
> +
> +    WEBCORE_EXPORT void startMonitoringGamepads(GamepadProviderClient&) final;
> +    WEBCORE_EXPORT void stopMonitoringGamepads(GamepadProviderClient&) final;

WEBCORE_EXPORT macros aren't needed for GTK port.

> Source/WebKit/UIProcess/Gamepad/gtk/UIGamepadProviderGtk.cpp:46
> +    if (WEBKIT_IS_WEB_VIEW(widget) && gtk_widget_is_visible(widget))
> +        return &webkitWebViewGetPage(WEBKIT_WEB_VIEW(widget));

In case of being a WebView and not visible, I guess we want to return nullptr, not iterate the web view children, no?

> Source/WebKit/UIProcess/Gamepad/gtk/UIGamepadProviderGtk.cpp:58
> +    ASSERT(hasProcessPrivilege(ProcessPrivilege::CanCommunicateWithWindowServer));

What is this?

> Source/WebKit/UIProcess/Gamepad/gtk/UIGamepadProviderGtk.cpp:62
> +        if (!GTK_IS_WINDOW(iter->data))

WebCore::widgetIsOnscreenToplevelWindow()

[ChangSeok Oh]

Comment on attachment 396169 [details]
Patch

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

Thanks for your review! A new patch is coming.

>> Source/WebCore/platform/gamepad/manette/ManetteGamepad.cpp:35
>> +    , m_device(adoptGRef(static_cast<ManetteDevice*>(g_object_ref(device))))
> 
> You don't need to use adopGRef() + g_object_ref() by here, you can pass the
> pointer to the GRefPtr constructor and it will increase the reference count
> automatically. The following should be enough:
> 
>     ManetteGamePad(ManetteDevice* device, unsigned index):
>         : PlatformGamepad(index)
>         , m_device(device)
>     {
>     }
> 
> =)

Good to know. Thanks.

>> Source/WebCore/platform/gamepad/manette/ManetteGamepad.h:38
>> +class ManetteGamepad : public PlatformGamepad {
> 
> We can mark the class final too.

O.K.

>> Source/WebCore/platform/gamepad/manette/ManetteGamepadProvider.cpp:50
>> +    if (provider)
> 
> Can this really be nullptr? The provider is a singleton.

No. the if condition is removed.

>> Source/WebCore/platform/gamepad/manette/ManetteGamepadProvider.cpp:56
>> +    if (provider)
> 
> Ditto.

Done.

>> Source/WebCore/platform/gamepad/manette/ManetteGamepadProvider.cpp:61
>> +    : m_shouldDispatchCallbacks(false)
> 
> This could be initialized in the header.

Moved to the header.

>> Source/WebCore/platform/gamepad/manette/ManetteGamepadProvider.cpp:66
>> +    g_signal_connect(G_OBJECT(m_monitor.get()), "device-disconnected", G_CALLBACK(onDeviceDisconnected), this);
> 
> No need to cast GObject for g_signal_connect

G_OBJECT removed.

>> Source/WebCore/platform/gamepad/manette/ManetteGamepadProvider.cpp:76
>> +    if (shouldOpenAndScheduleManager) {
> 
> We could early return instead.

Done.

>> Source/WebCore/platform/gamepad/manette/ManetteGamepadProvider.cpp:104
>> +    std::unique_ptr<ManetteGamepad> gamepad = makeUnique<ManetteGamepad>(device, index);
> 
> You could use “auto” for the variable type here, because it is clear
> from the right side of the assignment which type it will have.

O.K.

>> Source/WebCore/platform/gamepad/manette/ManetteGamepadProvider.cpp:166
>> +    auto i = m_gamepadVector.find(result.get());
> 
> i -> index

Fixed.

>> Source/WebCore/platform/gamepad/manette/ManetteGamepadProvider.h:48
>> +    WEBCORE_EXPORT void stopMonitoringGamepads(GamepadProviderClient&) final;
> 
> WEBCORE_EXPORT macros aren't needed for GTK port.

Removed.

>> Source/WebKit/UIProcess/Gamepad/gtk/UIGamepadProviderGtk.cpp:46
>> +        return &webkitWebViewGetPage(WEBKIT_WEB_VIEW(widget));
> 
> In case of being a WebView and not visible, I guess we want to return nullptr, not iterate the web view children, no?

You are right. Let me change the return statement to 'return gtk_widget_is_visible(widget) ? &webkitWebViewGetPage(WEBKIT_WEB_VIEW(widget)) : nullptr;'

>> Source/WebKit/UIProcess/Gamepad/gtk/UIGamepadProviderGtk.cpp:58
>> +    ASSERT(hasProcessPrivilege(ProcessPrivilege::CanCommunicateWithWindowServer));
> 
> What is this?

That is a security check in process level for mac port but I blindly copied it here. Let's remove it.

>> Source/WebKit/UIProcess/Gamepad/gtk/UIGamepadProviderGtk.cpp:62
>> +        if (!GTK_IS_WINDOW(iter->data))
> 
> WebCore::widgetIsOnscreenToplevelWindow()

Great! Thanks.

>> Source/cmake/FindManette.cmake:56
>> +pkg_check_modules(PC_MANETTE manette-0.2)
> 
> I would add the IMPORTED_TARGET flag here, and all lines from
> from 58 to 71 will not be needed.

Wow! I didn't know this macro. It folds many lines up. Thanks.

>> Source/cmake/FindManette.cmake:87
>> +endif ()
> 
> …and here use the PkgConfig::PC_MANETTE target:
> 
>     if (TARGET PkgConfig::PC_MANETTE AND NOT TARGET Manette::Manette)
>         add_library(Manette::Manette INTERFACE IMPORTED GLOBAL)
>         set_property(TARGET Manette::Manette INTERFACE_LINK_LIBRARIES PkgConfig::PC_MANETTE)
>     endif ()
> 
> This is what we currently do in Source/cmake/FindGTK.cmake :)

Done.

>> Source/cmake/FindManette.cmake:91
>> +if (Manette_FOUND)
> 
> If the PkgConfig target is used, this if-block is not needed.

👍

[ChangSeok Oh]

Created attachment 396900 [details]
Patch

[ChangSeok Oh]

@philn, could you bump up the version of libmanette to 0.2.4 for webkit flatpak sdk? https://ftp.gnome.org/pub/gnome/sources/libmanette/0.2/

[Philippe Normand]

(In reply to ChangSeok Oh from comment #18)
> @philn, could you bump up the version of libmanette to 0.2.4 for webkit
> flatpak sdk? https://ftp.gnome.org/pub/gnome/sources/libmanette/0.2/

It's already in the SDK. Make sure you use the latest version:

$ webkit-flatpak --command=bash
[📦 org.webkit.Webkit WebKit]$ pkg-config --modversion manette-0.2
0.2.4

[ChangSeok Oh]

(In reply to Philippe Normand from comment #19)
> (In reply to ChangSeok Oh from comment #18)
> > @philn, could you bump up the version of libmanette to 0.2.4 for webkit
> > flatpak sdk? https://ftp.gnome.org/pub/gnome/sources/libmanette/0.2/
> 
> It's already in the SDK. Make sure you use the latest version:
> 
> $ webkit-flatpak --command=bash
> [📦 org.webkit.Webkit WebKit]$ pkg-config --modversion manette-0.2
> 0.2.4

How fast your are! Thanks.

That said, the new patch is not guilty to the red window port.

[ChangSeok Oh]

Any comment or r+?

[ChangSeok Oh]

Created attachment 399815 [details]
Patch

Rebase

[Carlos Garcia Campos]

Comment on attachment 399815 [details]
Patch

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

> Source/WebKit/UIProcess/Gamepad/gtk/UIGamepadProviderGtk.cpp:31
> +#include "WebKitWebViewPrivate.h"

Better use WebKitWebViewBase

[ChangSeok Oh]

Comment on attachment 399815 [details]
Patch

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

>> Source/WebKit/UIProcess/Gamepad/gtk/UIGamepadProviderGtk.cpp:31
>> +#include "WebKitWebViewPrivate.h"
> 
> Better use WebKitWebViewBase

You meant WebKitWebViewBasePrivate?

[ChangSeok Oh]

Created attachment 399882 [details]
Patch to land

[ChangSeok Oh]

Comment on attachment 399882 [details]
Patch to land

Thanks!

[EWS]

Committed r261965: <https://trac.webkit.org/changeset/261965>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399882 [details].

[Radar WebKit Bug Importer]

<rdar://problem/63466509>